1102432 - Rooms list should clean up when I log in and out of FxA

Status: VERIFIED FIXED

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

Description

[Maire Reavy [:mreavy]]

Rooms I see as a logged in user should not appear when I log out and become Guest.  Similarly, Rooms I see as Guest should not appear in my Rooms list when I log in to FxA.

Comment 1

[Mike de Boer [:mikedeboer]]

This is _very_ similar to the database switch done for LoopContacts. I can work on this, but if there's another taker, I advice you to take that logic as a starting point.

There's definitely prior art here, so this shouldn't be hard to implement.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Marco, can you please add this to the 37.1 iteration tracking? Thanks!

Comment 3

[Marco Mucci [:MarcoM]]

Added to IT 37.1

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike de Boer [:mikedeboer] from comment #1)
> This is _very_ similar to the database switch done for LoopContacts. I can
> work on this, but if there's another taker, I advice you to take that logic
> as a starting point.
> 
> There's definitely prior art here, so this shouldn't be hard to implement.

Kind of a cryptic message for those of us that can't read your mind ;) Are you referring to http://hg.mozilla.org/mozilla-central/annotate/ced1402861b8/browser/components/loop/MozLoopAPI.jsm#l267 ?

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Kind of a cryptic message for those of us that can't read your mind ;) Are
> you referring to
> http://hg.mozilla.org/mozilla-central/annotate/ced1402861b8/browser/
> components/loop/MozLoopAPI.jsm#l267 ?

I apologize for not being more specific! The code I'm referring to is https://hg.mozilla.org/integration/fx-team/annotate/2273193cc525/browser/components/loop/MozLoopService.jsm#l241

So when the profile changes from FxA account to null --> clear the rooms cache in LoopRooms.jsm and set the dirty flag. Additionally, an event should be emitted to notify the UI that the list was cleared/ reset.
When the profile changes from null to FxA account --> same. Nothing will be cleared, but an event should be emitted by LoopRooms to inform the UI that it should fetch the list of rooms again.

Comment 6

[Romain Testard [:RT]]

My experience:
Fx profile 1: Sign into FxA, create room 1, sign out, close Firefox
Fx profile 2: Open Firefox with a different profile and sign in: room 1 does not appear on the list

If I close Firefox and open it again with profile 2, after a small delay room 1 appears.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> When the profile changes from null to FxA account --> same. Nothing will be
> cleared, but an event should be emitted by LoopRooms to inform the UI that
> it should fetch the list of rooms again.

"Nothing will be cleared"? From our discussion during yesterday's standup it sounded like we wanted to clear out rooms that were created by a guest when sign-in occurred.

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> "Nothing will be cleared"? From our discussion during yesterday's standup it
> sounded like we wanted to clear out rooms that were created by a guest when
> sign-in occurred.

Ah of course, guests can create rooms too. All the better! Makes the implementation simpler :)

Comment 9

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:
We need this fix for "Rooms", which is already enabled in Fx35.

Comment 10

[:shell escalante]

Hi Jared,  Is this a bug you're able to do this week so we can uplift into Beta Monday for testing?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: WIP touch base

Hi Jared, just to touch base here, I wanted to show you what I was thinking about as a possible route to solving this issue.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8533703 [details] [diff] [review]
WIP touch base

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

Yeah, this is what I arrived at when I was working with you last week, but I thought maybe we could make our getAll() function smarter and have that remove the rooms that didn't come back from the server. That way we wouldn't need the global.

But this is probably simpler to follow, at the expense of introducing more global state tracking. I'm unable to test this patch out on my machine because I don't have the backend room server setup (I can't log-in or create rooms with just my localhost:3000/make runserver setup). I'll file a bug about allowing the locally run server to communicate with the development server backend.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> I'll file a bug
> about allowing the locally run server to communicate with the development
> server backend.

Filed bug 1109269.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8533703 [details] [diff] [review]
WIP touch base

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

::: browser/components/loop/LoopRooms.jsm
@@ +489,5 @@
> +    gCurrentUser = user;
> +    gDirty = true;
> +    this.rooms.clear();
> +    eventEmitter.emit("refresh");
> +    this.getAll(() => {});

This should be, this.getAll(null, () => {});

Comment 16

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> But this is probably simpler to follow, at the expense of introducing more
> global state tracking. I'm unable to test this patch out on my machine
> because I don't have the backend room server setup (I can't log-in or create
> rooms with just my localhost:3000/make runserver setup). I'll file a bug
> about allowing the locally run server to communicate with the development
> server backend.

Cool! Note that you can use the dev server too by setting the `loop.server` pref to 'https://loop-dev.stage.mozaws.net/v0/' (source: https://wiki.mozilla.org/CloudServices/Loop/Deploy#Dev).

I'll work on adding a test next.

Comment 18

[Marco Mucci [:MarcoM]]

Added to IT 37.2

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: refresh the list of rooms upon account switch or logout

Improved patch, now with tests!

Niko, welcome to the desktop team! :-) Can you review the unit tests? Thanks!

Comment 20

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8534961 [details] [diff] [review]
Patch v2: refresh the list of rooms upon account switch or logout

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

I'm definitely not familiar with chrome-style JavaScript, so some remarks might be inaccurate/missing the point. The patch fixes the issue so r=me here, though feel free to address nits if you feel so.

::: browser/components/loop/LoopRooms.jsm
@@ +34,5 @@
>  // of date. The Push server may notify us of this event, which will set the global
>  // 'dirty' flag to TRUE.
>  let gDirty = true;
> +// Global variable that keeps track of the currently used account.
> +let gCurrentUser = null;

Note: I won't be too picky about using a global here as it seems to be common practice in Gecko code, but I'm a little concerned with mutable state and testability here. Hope we'll have some good discussion about this someday.

@@ +482,5 @@
>      this.getAll(version, () => {});
>    },
> +
> +  /**
> +   * When a user log in or logs out, this method should be invoked to check

Nit: s/log in or logs out/logs in or out

@@ +487,5 @@
> +   * whether the rooms cache needs to be refreshed.
> +   *
> +   * @param {String|null} user The FxA userID or NULL
> +   */
> +  maybeRefresh: function(user = null) {

Is this a monad? :D

More seriously, I'd have naturally expected the LoopRooms "module" to listen to an event (triggered from the service), though as it has no constructor I can't see any obvious place where we could do that :/

@@ +496,5 @@
> +    gCurrentUser = user;
> +    gDirty = true;
> +    this.rooms.clear();
> +    eventEmitter.emit("refresh");
> +    this.getAll(null, () => {});

Nit: It's still a bit strange to me that we need to call *get*Something to perform side-effects… Maybe that's something we could file a [tech-debt] bug for?

::: browser/components/loop/MozLoopService.jsm
@@ +243,5 @@
>    notifyStatusChanged: function(aReason = null) {
>      log.debug("notifyStatusChanged with reason:", aReason);
>      let profile = MozLoopService.userProfile;
>      LoopStorage.switchDatabase(profile ? profile.uid : null);
> +    LoopRooms.maybeRefresh(profile ? profile.uid : null);

Nit: profile && profile.uid || null is an alternative idiom (I personally tend to favor it though no big deal.)

::: browser/components/loop/content/shared/js/roomStore.js
@@ +193,5 @@
> +    _onRoomsRefresh: function(eventName) {
> +      this.dispatchAction(new sharedActions.UpdateRoomList({
> +        roomList: []
> +      }));
> +    },

This should be tested, have a look in roomStore_test.js. Note that tests might be added as a part 2, though it's often useful to read the tests along implementation :)

Comment 21

[Mark Banner (:standard8)]

I just tried applying this and test_loopservice_busy.js failed.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

> ::: browser/components/loop/MozLoopService.jsm
> @@ +243,5 @@
> >    notifyStatusChanged: function(aReason = null) {
> >      log.debug("notifyStatusChanged with reason:", aReason);
> >      let profile = MozLoopService.userProfile;
> >      LoopStorage.switchDatabase(profile ? profile.uid : null);
> > +    LoopRooms.maybeRefresh(profile ? profile.uid : null);
> 
> Nit: profile && profile.uid || null is an alternative idiom (I personally
> tend to favor it though no big deal.)

Actually in this case, as well as the case for LoopStorage.switchDatabase, we don't need to pass the UID or null. We can just pass (profile && profile.uid) since both of these only care about the presence of a UID and not actually what that UID is.

Comment 23

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> Actually in this case, as well as the case for LoopStorage.switchDatabase,
> we don't need to pass the UID or null. We can just pass (profile &&
> profile.uid) since both of these only care about the presence of a UID and
> not actually what that UID is.

Not true, LoopStorage _does_ care what the value of uid is.

Comment 24

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #23)
> Not true, LoopStorage _does_ care what the value of uid is.

But I think I see what you meant now; LoopStorage.switchDatabase treats all falsy values the same. So yeah, we can use the shorthand here.

Comment 25

[Mike de Boer [:mikedeboer]]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #20)
> Note: I won't be too picky about using a global here as it seems to be
> common practice in Gecko code, but I'm a little concerned with mutable state
> and testability here. Hope we'll have some good discussion about this
> someday.

Globals in Gecko JS modules (.jsm files, usually), are evaluated in their own compartment, which is 'alive' during the lifetime of that module. In other words; global variables like this are only global to the modules' internals, not outside of it. Neat, eh?

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Patch v3: refresh the list of rooms upon account switch or logout

Comment 27

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8537230 [details] [diff] [review]
Patch v3: refresh the list of rooms upon account switch or logout

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

Looks good, ship it :)

Comment 28

[Mike de Boer [:mikedeboer]]

Nice.

Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/5f189920c4d1

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f189920c4d1

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Mike, could you fill the uplift request for aurora & beta?
thanks

Comment 31

[Mike de Boer [:mikedeboer]]

Comment on attachment 8537230 [details] [diff] [review]
Patch v3: refresh the list of rooms upon account switch or logout

Approval Request Comment
[Feature/regressing bug #]: loop rooms
[User impact if declined]: When the user logs in with a second Fx Account, he/ she will see the old list of rooms in the UI.
[Describe test coverage new/current, TBPL]: Landed on m-c, tests added and pass
[Risks and why]: minor
[String/UUID change made/needed]: n/a.

Comment 32

[Maire Reavy [:mreavy]]

Thanks, Mike.

FYI for sheriffs: Once we get aurora and beta approvals for this patch, Randell (Jesup) and I will uplift this along with other approved Hello patches.  (The target is to get everything uplifted before Monday.)

Comment 33

[Maire Reavy [:mreavy]]

We will be uplifting this to Fx35 this weekend (before Monday), but the patch is not there yet.

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee96aeb0cf23

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

I checked this on aurora before pushing, BTW

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/234d5a49345f

Comment 37

[Paul Silaghi, QA [:pauly]]

Verified fixed 35RC, 36.0a2 (2015-01-06), 37.0a1 (2015-01-05) Win 7