1126733 - Brief message appears when entering a standalone room that the user is the only person in the room

Status: RESOLVED FIXED

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

Description

[isabelrios]

Firefox 35.0.1
Scenario 1:
User A logged in mobile loop app
User B logged in desktop loop app
1. User A has created and shared a room with user B
2. User A joins the room
3. User B joins the room

Scenario 2
User A logged in desktop loop app
User B desktop user

1. User A has created and shared a room with user B
2. User A joins the room and then User B joins too

EXPTECTED
User B joins when user A is already in the room, so no message should be shown and the communication should start 

ACTUAL
User B can see: 'You're the first one here' message although he is not the first joining as user A is already in the room

Comment 1

[Mark Banner (:standard8)]

Can you clarify your report slightly please? I'm confused by the ACTUAL results you're seeing.

(In reply to Isabel Rios [:isabel_rios] from comment #0)
> ACTUAL
> User B can see: 'You're the first one here' message although he is not the
> first joining as user A is already in the room

Is the message shown briefly and then it connects to the room, or is this message shown all the time and the two users never connect?

Comment 2

[isabelrios]

The message is shown for a few seconds and then it disappears and the communication starts successfully.

Please let me know any other doubt or comment.
Thanks

Comment 3

[Mark Banner (:standard8)]

Thanks for the clarification.

This is slightly more difficult to solve consistently, as we currently don't get any information on who's in the room until we connect to the sdk - but even when we are connected, there's no information that there is no-one in the room (we get notified if there are people in the room, but not if there are no people in the room).

Adam, any ideas here?

Comment 4

[Adam Roach [:abr]]

(In reply to Mark Banner (:standard8) from comment #3)
> This is slightly more difficult to solve consistently, as we currently don't
> get any information on who's in the room until we connect to the sdk

The response to joining the room *should* include a "participants" array (see https://wiki.mozilla.org/Loop/Architecture/Rooms#GET_.2Frooms.2F.7Btoken.7D). If a party joins a room and that list is longer than one, it would make sense to suppress the "you're the only one in the room" message.

To prevent the odd race situation (e.g., someone is in the process of leaving the room when you join): when we do this (and only when we do this) we should set a timer to re-check room membership periodically, and display the message if the count drops back to zero.

Comment 5

[Adam Roach [:abr]]

(In reply to Adam Roach [:abr] from comment #4)
> ...and display the message if the count drops back to zero.

Of course, I meant "drops back to one".

Comment 7

[Mark Banner (:standard8)]

I think we should probably put this in the backlog somewhere as its something that potentially confuses users.


From implementation perspective, here's a possibility:

- activeRoomStore
-- would need to do some more processing in joinRoom to determine number of participants and set a flag if there's more than 1.
-- if the session connected, and there wasn't more than 1, after a timeout, then drop it back to one.
- Standalone:
-- StandaloneRoomInfoArea could change to only display the only-user message if there was expected to be more than one user.
- Desktop:
-- Looks like this could decide not to render the invitation overlay if there's expected to be more than one participant (probably need to also keep the has_participants state.

Note, that there's a room state of HAS_PARTICIPANTS that might conflict with this new flag a bit, especially on the desktop side. It might be that this could go away in favour of the flag, but then that might mess up some of the state handling.

Comment 8

[Romain Testard [:RT]]

Just marked it as dependency of bug 1138453 so it gets addressed with the link clicker UI visual refresh.

Comment 9

[Romain Testard [:RT]]

Since tiles landed this is more noticeable. I am increasing the priority to reflect this.
Mark, does it make sense to address this as part of the visual refresh?

Comment 10

[Mark Banner (:standard8)]

I don't think we should connect this to the visual refresh, but I'm fine with it being done soon after (its more of a flow issue than a refresh one).

Comment 11

[Ed Lee :Mardak]

Attachment: wip workaround brief wait

Here's a workaround that just waits 2 seconds in the "waiting" state before showing the message. If there's another participant, potentially it'll change state from waiting/room-session-connected to room-has-participants before the 2 seconds is up.

Not sure if this is an okay approach or even a desired workaround.

Another thing I realized is that the room-joined google analytics event is being used to count tile impressions? But if the user only sees the tile for a few milliseconds, there could be some discrepancy between that count and the content services count.

Comment 12

[Ed Lee :Mardak]

Digging into abr's comment 4 and standard8's comment 7, it looks like participants isn't being sent by the server so ActiveRoomStore._getRoomDataForStandalone doesn't have the participants list.

http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/content/shared/js/activeRoomStore.js#317

And if I'm reading loop-server correctly, it only sends the participants list if there's a participantHmac:
https://github.com/mozilla-services/loop-server/blob/master/loop/routes/rooms.js#L299

Comment 13

[Romain Testard [:RT]]

(In reply to Ed Lee :Mardak from comment #11)
> Created attachment 8647757 [details] [diff] [review]
> wip workaround brief wait
> 
> Here's a workaround that just waits 2 seconds in the "waiting" state before
> showing the message. If there's another participant, potentially it'll
> change state from waiting/room-session-connected to room-has-participants
> before the 2 seconds is up.
> 
> Not sure if this is an okay approach or even a desired workaround.
> 
> Another thing I realized is that the room-joined google analytics event is
> being used to count tile impressions? But if the user only sees the tile for
> a few milliseconds, there could be some discrepancy between that count and
> the content services count.

And I guess the CS team reports will be impacted too by reporting a high impression number with low CTR as a result since most impressions are not actually clickable?

Comment 14

[Mark Banner (:standard8)]

(In reply to Ed Lee :Mardak from comment #11)
> Created attachment 8647757 [details] [diff] [review]
> wip workaround brief wait
> 
> Here's a workaround that just waits 2 seconds in the "waiting" state before
> showing the message. If there's another participant, potentially it'll
> change state from waiting/room-session-connected to room-has-participants
> before the 2 seconds is up.
> 
> Not sure if this is an okay approach or even a desired workaround.

I think its a reasonable work around for the standalone side, we don't have any participant data easily accessible. You'll need to clear the timer if someone else comes into the room as well (HAS_PARTICIPANTS).

You probably also need to check room re-entry patterns for when to reset the new flag.

On the desktop side we should have participant data - but we should still possibly have a timeout as well, just in case we don't.

Might also need to check with UX about the period and if anything should be displayed.

> Another thing I realized is that the room-joined google analytics event is
> being used to count tile impressions? But if the user only sees the tile for
> a few milliseconds, there could be some discrepancy between that count and
> the content services count.

I'm not against another event being added if it makes sense.

Comment 15

[Romain Testard [:RT]]

Thanks Ed and Mark.
Sevaan is the work-around Ed proposes OK with you from a UX standpoint?

Comment 16

[Sevaan Franks [:sevaan]]

+1. The work around sounds good to me. Nice thinking, Ed.

Comment 17

[Romain Testard [:RT]]

Thanks Sevaan
Gareth can you please confirm what event we should put in place per Ed's comment above (event sent to track impressions of tiles post "waiting" state).

Comment 18

[Ed Lee :Mardak]

I would guess it would be similar to one of these existing events:

this._storeEvent(METRICS_GA_CATEGORY.general, METRICS_GA_ACTIONS.pageLoad, "Link expired or invalid");
this._storeEvent(METRICS_GA_CATEGORY.general, METRICS_GA_ACTIONS.pageLoad, "Room full");

Perhaps.. ?

this._storeEvent(METRICS_GA_CATEGORY.general, METRICS_GA_ACTIONS.pageLoad, "Tile shown");

sevaan, any preference on how long the wait before showing should be? 1 second? 2 seconds? more?

Comment 19

[Gareth Cull [:garethc]]

How about this? 

this._storeEvent(METRICS_GA_CATEGORY.general, METRICS_GA_ACTIONS.pageLoad, "First one here - Tile shown");

Comment 20

[Sevaan Franks [:sevaan]]

> sevaan, any preference on how long the wait before showing should be? 1
> second? 2 seconds? more?

I thought 2 seconds sounded good. At least it seems so in my head. I may have to see it in action to get a better feel for it.

Comment 21

[Ed Lee :Mardak]

Attachment: v1

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 8652755 [details] [diff] [review]
v1

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

I think the architecture of the patch is right, but there's a few issues that we need to tidy up in there.

Also, just a reminder that we'll need to fix the desktop UI as well (to avoid the intermediate invite someone to join you message). I'm happy for that to be done in a separate patch though, especially if we're having a slightly different approach as we know how many participants are in the room already.

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +103,5 @@
>        window.addEventListener("message", this.recordTileClick);
>      },
>  
> +    _allowRenderWaiting: function() {
> +      this.setState({ waitToRenderWaiting: false });

On standalone, we'll need this to be reset to true when a person leaves a room. I think you'll be safe doing it in the READY/ENDED state as they're the only two where we display the join button.

@@ +116,5 @@
> +        case ROOM_STATES.SESSION_CONNECTED: {
> +          if (this.state.waitToRenderWaiting) {
> +            // There's no need to clear the timeout or reset state as we want
> +            // repeat showing of the message to display immediately.
> +            setTimeout(this._allowRenderWaiting,

Unfortunately, setting multiple timeouts is also causing multiple re-renders, even though they aren't doing anything. I'd like to avoid that.

I'm not quite sure about your comment here either. A room should always transition through the JOINING state. Is it enough to set the delay when we hit that state, or are you concerned about the time it takes to get from JOINING to SESSION_CONNECTED?

@@ +119,5 @@
> +            // repeat showing of the message to display immediately.
> +            setTimeout(this._allowRenderWaiting,
> +              this.constructor.RENDER_WAITING_DELAY);
> +          } else {
> +            this.props.dispatcher.dispatch(new sharedActions.TileShown());

This is currently getting sent multiple times, I think we only want it once for each entrance of the room - why not dispatch this from within the _allowRenderWaiting function?

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +229,3 @@
>            function() {
>              activeRoomStore.setStoreState({roomState: ROOM_STATES.JOINED});
> +            expect(view.getDOMNode().querySelector(".empty-room-message"))

I think it would be clearer to split this into two tests:

it("should not display an message immediately in the JOINED state");
it("should display an empty room message after a wait when in the JOINED state")

ditto for the rest (unless they need changing due to my other comments).

Comment 23

[Ed Lee :Mardak]

(In reply to Mark Banner (:standard8) from comment #22)
> Comment on attachment 8652755 [details] [diff] [review]
> v1
> 
> Review of attachment 8652755 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the architecture of the patch is right, but there's a few issues
> that we need to tidy up in there.
> 
> Also, just a reminder that we'll need to fix the desktop UI as well (to
> avoid the intermediate invite someone to join you message). I'm happy for
> that to be done in a separate patch though, especially if we're having a
> slightly different approach as we know how many participants are in the room
> already.
> 
> ::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
> @@ +103,5 @@
> >        window.addEventListener("message", this.recordTileClick);
> >      },
> >  
> > +    _allowRenderWaiting: function() {
> > +      this.setState({ waitToRenderWaiting: false });
> 
> On standalone, we'll need this to be reset to true when a person leaves a
> room. I think you'll be safe doing it in the READY/ENDED state as they're
> the only two where we display the join button.
> 
> @@ +116,5 @@
> > +        case ROOM_STATES.SESSION_CONNECTED: {
> > +          if (this.state.waitToRenderWaiting) {
> > +            // There's no need to clear the timeout or reset state as we want
> > +            // repeat showing of the message to display immediately.
> > +            setTimeout(this._allowRenderWaiting,
> 
> Unfortunately, setting multiple timeouts is also causing multiple
> re-renders, even though they aren't doing anything. I'd like to avoid that.
> 
> I'm not quite sure about your comment here either. A room should always
> transition through the JOINING state. Is it enough to set the delay when we
> hit that state, or are you concerned about the time it takes to get from
> JOINING to SESSION_CONNECTED?
> 
> @@ +119,5 @@
> > +            // repeat showing of the message to display immediately.
> > +            setTimeout(this._allowRenderWaiting,
> > +              this.constructor.RENDER_WAITING_DELAY);
> > +          } else {
> > +            this.props.dispatcher.dispatch(new sharedActions.TileShown());
> 
> This is currently getting sent multiple times, I think we only want it once
> for each entrance of the room - why not dispatch this from within the
> _allowRenderWaiting function?
> 
> ::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
> @@ +229,3 @@
> >            function() {
> >              activeRoomStore.setStoreState({roomState: ROOM_STATES.JOINED});
> > +            expect(view.getDOMNode().querySelector(".empty-room-message"))
> 
> I think it would be clearer to split this into two tests:
> 
> it("should not display an message immediately in the JOINED state");
> it("should display an empty room message after a wait when in the JOINED
> state")
> 
> ditto for the rest (unless they need changing due to my other comments).

Comment 24

[Ed Lee :Mardak]

Attachment: v2

(In reply to Mark Banner (:standard8) from comment #22)
> Also, just a reminder that we'll need to fix the desktop UI as well (to
> avoid the intermediate invite someone to join you message).
Split off to bug 1198841.

> ::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
> @@ +103,5 @@
> > +    _allowRenderWaiting: function() {
> > +      this.setState({ waitToRenderWaiting: false });
> On standalone, we'll need this to be reset to true when a person leaves a
> room.
Nice. I added tests for the user leaving and rejoining quickly/slowly triggering the right number of events.

> > +            setTimeout(this._allowRenderWaiting,
> Unfortunately, setting multiple timeouts is also causing multiple
> re-renders, even though they aren't doing anything. I'd like to avoid that.
Ah right. I originally had a timer check in the WIP patch in render(). I changed the logic to only set the timer on JOINING.

> I'm not quite sure about your comment here either.
I was thinking more of the other user leaving and rejoining, so we didn't want to delay the message.

> @@ +119,5 @@
> > +            this.props.dispatcher.dispatch(new sharedActions.TileShown());
> This is currently getting sent multiple times, I think we only want it once
> for each entrance of the room - why not dispatch this from within the
> _allowRenderWaiting function?
Done. Added tests too.

> ::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
> @@ +229,3 @@
> >            function() {
> >              activeRoomStore.setStoreState({roomState: ROOM_STATES.JOINED});
> > +            expect(view.getDOMNode().querySelector(".empty-room-message"))
> I think it would be clearer to split this into two tests:
> 
> it("should not display an message immediately in the JOINED state");
> it("should display an empty room message after a wait when in the JOINED
> state")
Split for a bunch of tests and the newly added tests.

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 8652970 [details] [diff] [review]
v2

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

Looks a lot better. r=Standard8 with the comments addressed.

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +102,5 @@
>        // Watch for messages from the waiting-tile iframe
>        window.addEventListener("message", this.recordTileClick);
>      },
>  
> +    _allowRenderWaiting: function() {

Please add a jsdoc for this function (we generally add jsdocs for all non react component lifecycle functions).

@@ +104,5 @@
>      },
>  
> +    _allowRenderWaiting: function() {
> +      delete this._waitTimer;
> +      this.setState({ waitToRenderWaiting: false });

I think this can be in the switch statement as well, to save a re-render cycle if there's someone in the room already.

@@ +120,5 @@
> +    componentDidUpdate: function() {
> +      // Start a timer once from the earliest waiting state if we need to wait
> +      // before showing a message.
> +      if (this.props.roomState === ROOM_STATES.JOINING &&
> +          this.state.waitToRenderWaiting) {

I don't think this will happen, but I'm still a little concerned that we'll get a subsequent update whilst still in the JOINING state, and if so this would file a second time.

I think adding this._waitTimer into the check might be the best way to stop that here.

Comment 26

[Pulsebot]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/efb79dfd0379