1214582 - Adjust how room titles are displayed/managed

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Mark Banner (:standard8)]

As part of the user journey rework, we need to adjust how the room titles are displayed/managed. This depends on bug 1212357 to avoid some bitrot.

See the user story for more detail.

Comment 1

[Mark Banner (:standard8)]

Sevaan, what should we use as a fallback string if the room hasn't been given a name, and the shared web page doesn't have a title?

Comment 2

[Mark Banner (:standard8)]

Note: bug 1214590 will stop assigning the room names to rooms. For this we'll need to cover the non-room-name case via testing, which we'll need to do anyway.

Comment 3

[Sevaan Franks [:sevaan]]

(In reply to Mark Banner (:standard8) from comment #1)
> Sevaan, what should we use as a fallback string if the room hasn't been
> given a name, and the shared web page doesn't have a title?

Let's use the URL in that case. It will look a little ugly, but at least it will have some meaning.

Comment 4

[Manuel Casas Barrado [:mancas]]

Attachment: Adjust how room titles are displayed/managed

Hey Mark! This patch is ready to review

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8674828 [details] [diff] [review]
Adjust how room titles are displayed/managed

Review of attachment 8674828 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start, but we also need to handle the conversation window title (DesktopRoomConversationView) and the standalone one (StandaloneRoomView)

Comment 6

[Ian Bicking (:ianbicking)]

When the work is complete please confirm that the context menu problem from Bug 1213846 isn't reproduced in the new design (the bug demonstrated in this screenshot: https://bug1213846.bmoattachments.org/attachment.cgi?id=8672606)

Comment 7

[Mark Banner (:standard8)]

(In reply to Ian Bicking (:ianb) from comment #6)
> When the work is complete please confirm that the context menu problem from
> Bug 1213846 isn't reproduced in the new design (the bug demonstrated in this
> screenshot: https://bug1213846.bmoattachments.org/attachment.cgi?id=8672606)

That bug is all about positioning of the drop-down. It isn't going to be affected by room titles - and not related to the code in this bug. I think we should keep it as a separate issue unless you disagree Ian?

Comment 8

[Manuel Casas Barrado [:mancas]]

Perhaps we could fix that problem  in bug 1212357 which is related to the dropdown.

Comment 9

[Manuel Casas Barrado [:mancas]]

Attachment: Adjust how room titles are displayed/managed

Mark could you check if the patch works fine? If there is something wrong let me know.

Also I've added unit test for each part.

Thanks!

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8677276 [details] [diff] [review]
Adjust how room titles are displayed/managed

Review of attachment 8677276 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable, but I think there's some tidy up that can be done. Unfortunately there's some bit rot due to the eslint patches that recently landed, although addressing the comments in the test will probably simplify a lot of it.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +743,5 @@
>        });
>      });
> +
> +    describe("Room name priority", function() {
> +      it("should use room name", function() {

I think it would be worth changing this test to "should use the room name by default", and extend the decryptedContext here to set a context url.

@@ +744,5 @@
>      });
> +
> +    describe("Room name priority", function() {
> +      it("should use room name", function() {
> +        var roomEntry = mountRoomEntry({

Please move the mountRoomEntry to a beforeEach - its the same in all three of these `it` sections.

@@ +748,5 @@
> +        var roomEntry = mountRoomEntry({
> +          dispatcher: dispatcher,
> +          room: new loop.store.Room(roomData)
> +        });
> +        var updatedRoom = new loop.store.Room(_.extend({}, roomData, {

It might be nice to create a utility function for this in-line (similar to mountRoomEntry), that just gets passed the decryptedContext data.

@@ +759,5 @@
> +        roomEntry.setProps({room: updatedRoom});
> +
> +        expect(
> +          roomEntry.getDOMNode().textContent)
> +        .eql("Room name");

I realise you copied & pasted this formatting, but I think this is short enough that it can all go on one line (and the instances in the other two tests).

::: browser/components/loop/test/desktop-local/roomViews_test.js
@@ +653,5 @@
> +        it("should use room name", function() {
> +          activeRoomStore.setStoreState({
> +            participants: [{}],
> +            roomState: ROOM_STATES.JOINED,
> +            roomName: "fakeName"

I think it would be worth changing this test to "should use the room name by default", and extend the store state here to set a context url.

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +140,5 @@
>  
>      it("should display a join room button if the state is not ROOM_JOINED", function() {
>        activeRoomStore.setStoreState({
> +        roomState: ROOM_STATES.READY,
> +        roomName: "fakeName"

I think we should set the room name in a beforeEach section, seeing as you're setting it for virtually every test in this section.

If a test explicitly doesn't want it, then you can do setStoreState({ roomName: null }) or something like that.

@@ +429,5 @@
>      describe("#componentWillUpdate", function() {
>        it("should set document.title to roomName and brand name when the READY state is dispatched", function() {
>          activeRoomStore.setStoreState({roomName: "fakeName", roomState: ROOM_STATES.INIT});
>          view = mountTestComponent();
> +        activeRoomStore.setStoreState({roomState: ROOM_STATES.READY, roomName: "fakeName"});

Again, I think we can set the roomName in a beforeEach. Additionally, you shouldn't have to set it twice in an it() function - once should be enough.

@@ +526,5 @@
>          it("should send the second `TileShown` after ending and rejoining",
>            function() {
>              // Trigger the first message then rejoin and wait
>              clock.tick(1);
> +            activeRoomStore.setStoreState({roomState: ROOM_STATES.ENDED, roomName: "fakeName"});

Ditto with the beforeEach.

@@ +600,5 @@
>  
>        describe("Empty room message", function() {
>          it("should not display an message immediately in the JOINED state",
>            function() {
> +            activeRoomStore.setStoreState({roomState: ROOM_STATES.JOINED, roomName: "fakeName"});

You're already setting the room name in the before each here, so you shouldn't have to repeat it.

Please also check the rest of this file for unnecessary repeats.

Comment 11

[Manuel Casas Barrado [:mancas]]

Attachment: Adjust how room titles are displayed/managed

Hey Mark, I've addressed all of your comments. Please review that patch when you get a chance.

Thanks!

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8678764 [details] [diff] [review]
Adjust how room titles are displayed/managed

Review of attachment 8678764 [details] [diff] [review]:
-----------------------------------------------------------------

That's better, r=Standard8

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/839ec883c9dc

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/839ec883c9dc

Comment 15

[Chris Rafuse :crafuse]

Get error now on localhost:
...
Loop:promiseRegisteredWithServers: registration already completed or in progress: 2 MozLoopService.jsm:449
Loop:hawkRequestInternal:  2 /rooms GET MozLoopService.jsm:608
Loop:notifyStatusChanged with reason: room-add MozLoopService.jsm:308
[Dispatcher] Dispatching action Object { roomList: Array[143], name: "updateRoomList" } dispatcher.js:92:9
[Dispatcher] Dispatching action caused an exception:  TypeError: this.props.room.decryptedContext.urls is undefined
Stack trace:
RoomEntry<.render@chrome://browser/content/loop/js/panel.js:430:1
[40]</L._renderValidatedComponent<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:19780
[40]</L.mountComponent<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:16175
[69]</v.Mixin.mountChildren@chrome://browser/content/loop/shared/libs/react-0.12.2.js:14:26548
[45]</i.Mixin._createContentMarkup@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:26397
[45]</i.Mixin.mountComponent<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:25684
[69]</v.Mixin._mountChildByNameAtIndex@chrome://browser/content/loop/shared/libs/react-0.12.2.js:14:27885
[69]</v.Mixin._updateChildren@chrome://browser/content/loop/shared/libs/react-0.12.2.js:14:27290
[69]</v.Mixin.updateChildren@chrome://browser/content/loop/shared/libs/react-0.12.2.js:14:26878
[45]</i.Mixin._updateDOMChildren@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:28149
[45]</i.Mixin.updateComponent<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:26750
[37]</d.Mixin.performUpdateIfNecessary@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:10127
[37]</d.Mixin.receiveComponent@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:9886
[45]</i.Mixin.receiveComponent@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:26525
[40]</L.updateComponent<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:19107
[40]</L._performComponentUpdate@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:18643
[40]</L.performUpdateIfNecessary@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:18258
a@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:11401
[104]</r.perform@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:27988
[104]</r.perform@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:27988
[88]</<.perform@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:12567
[88]</M<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:12797
[104]</r.closeAll@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:28609
[104]</r.perform@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:28068
[54]</p.batchedUpdates@chrome://browser/content/loop/shared/libs/react-0.12.2.js:14:7657
s@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15:11690
[40]</L.replaceState@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:16904
[40]</L.setState@chrome://browser/content/loop/shared/libs/react-0.12.2.js:13:16743
RoomList<._onStoreStateChanged@chrome://browser/content/loop/js/panel.js:666:7
triggerEvents@chrome://browser/content/loop/shared/libs/backbone-1.2.1.js:351:32
triggerApi@chrome://browser/content/loop/shared/libs/backbone-1.2.1.js:339:19
eventsApi@chrome://browser/content/loop/shared/libs/backbone-1.2.1.js:137:1
Events.trigger@chrome://browser/content/loop/shared/libs/backbone-1.2.1.js:329:1
baseStorePrototype.setStoreState@chrome://browser/content/loop/shared/js/store.js:54:7
loop.store.RoomStore<.updateRoomList@chrome://browser/content/loop/js/roomStore.js:487:1
Dispatcher.prototype._dispatchNextAction/<@chrome://browser/content/loop/shared/js/dispatcher.js:97:11
Dispatcher.prototype._dispatchNextAction@chrome://browser/content/loop/shared/js/dispatcher.js:95:7
Dispatcher.prototype.dispatch@chrome://browser/content/loop/shared/js/dispatcher.js:69:7
baseStorePrototype.dispatchAction@chrome://browser/content/loop/shared/js/store.js:29:7
loop.store.RoomStore<.getAllRooms/<@chrome://browser/content/loop/js/roomStore.js:464:9

Comment 16

[Chris Rafuse :crafuse]

Fixed this by navigating to loop-server/tools editing and running the following command:

edit remove_old_keys.js

Change var to following:
var TWO_WEEKS = 1;

Save then run:
node remove_old_keys.js

This will remove all keys and you will need to sign-up for account again.
You can revert remove_old_keys.js if needed after running this.

Mark is filling a bug on fall back or default for about:_blank context condition and no room name.

Comment 17

[Manuel Casas Barrado [:mancas]]

From my point of view, we should not allow the users to create rooms without context. I mean, about:blank, etc. pages. Since our main goal is to share the web, it has no sense to share a tab with no content.

What do you think about it?

Comment 18

[Mark Banner (:standard8)]

(In reply to Manuel Casas Barrado [:mancas] from comment #17)
> From my point of view, we should not allow the users to create rooms without
> context. I mean, about:blank, etc. pages. Since our main goal is to share
> the web, it has no sense to share a tab with no content.

Whilst I see your point, I think that not allowing them to create rooms in particular states is potentially going to cause confusion on the part of the user.

I've filed bug 1219600 for follow-up about this, as I think we need to handle fallback not just in the about cases but also for general issues such as not being able to decrypt our own data (for whatever reason) or server loss issues.