Closed Bug 1102432 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Points:
3

Tracking

(firefox35+ verified, firefox36+ verified, firefox37 verified)

VERIFIED FIXED
mozilla37
Iteration:
37.2
Tracking Status
firefox35 + verified
firefox36 + verified
firefox37 --- verified
backlog Fx35+

People

(Reporter: mreavy, Assigned: mikedeboer, Mentored)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
backlog: --- → Fx35+
Priority: -- → P1
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.
Mentor: mdeboer
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
OS: Windows 7 → All
Hardware: x86_64 → All
Marco, can you please add this to the 37.1 iteration tracking? Thanks!
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 37.1
Flags: needinfo?(mmucci)
Added to IT 37.1
Flags: needinfo?(mmucci)
(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 ?
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(mdeboer)
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.
(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.
(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 :)
[Tracking Requested - why for this release]:
We need this fix for "Rooms", which is already enabled in Fx35.
Hi Jared,  Is this a bug you're able to do this week so we can uplift into Beta Monday for testing?
Flags: needinfo?(mreavy)
Flags: needinfo?(jaws)
Attached patch WIP Patch (obsolete) — Splinter Review
Flags: needinfo?(jaws)
Flags: needinfo?(mreavy)
Iteration: 37.1 → 37.2
Attached patch WIP touch base (obsolete) — Splinter Review
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 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.
Attachment #8533703 - Flags: review+
(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 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, () => {});
Assignee: jaws → mdeboer
Flags: needinfo?(mmucci)
(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.
Duplicate of this bug: 1109575
Added to IT 37.2
Flags: needinfo?(mmucci)
Improved patch, now with tests!

Niko, welcome to the desktop team! :-) Can you review the unit tests? Thanks!
Attachment #8532282 - Attachment is obsolete: true
Attachment #8533703 - Attachment is obsolete: true
Attachment #8534961 - Flags: review?(nperriault)
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 :)
Attachment #8534961 - Flags: review?(nperriault) → review+
I just tried applying this and test_loopservice_busy.js failed.
> ::: 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.
(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.
(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.
(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?
Attachment #8534961 - Attachment is obsolete: true
Attachment #8537230 - Flags: review?(nperriault)
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 :)
Attachment #8537230 - Flags: review?(nperriault) → review+
https://hg.mozilla.org/mozilla-central/rev/5f189920c4d1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Mike, could you fill the uplift request for aurora & beta?
thanks
Flags: needinfo?(mdeboer)
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.
Flags: needinfo?(mdeboer)
Attachment #8537230 - Flags: approval-mozilla-beta?
Attachment #8537230 - Flags: approval-mozilla-aurora?
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.)
Attachment #8537230 - Flags: approval-mozilla-beta?
Attachment #8537230 - Flags: approval-mozilla-beta+
Attachment #8537230 - Flags: approval-mozilla-aurora?
Attachment #8537230 - Flags: approval-mozilla-aurora+
We will be uplifting this to Fx35 this weekend (before Monday), but the patch is not there yet.
I checked this on aurora before pushing, BTW
Verified fixed 35RC, 36.0a2 (2015-01-06), 37.0a1 (2015-01-05) Win 7
You need to log in before you can comment on or make changes to this bug.