1074702 - As a user, I should be able to join a room when I select

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: Expected UX

Comment 1

[Mark Banner (:standard8)]

Attachment: WIP Part 1 Implement join/refresh/leave with the Loop server on the standalone UI.

I'm still fiddling around with a couple of bits of this, but its more just styles, and this has the full set of tests with it. So very nearly ready for review.

Comment 2

[Mark Banner (:standard8)]

Attachment: Part 1 Implement join/refresh/leave with the Loop server on the standalone UI.

Updated patch that comments & update standaloneMozLoop, and finshes a couple of things I had outstanding. Now ready for review.

Comment 3

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

Comment on attachment 8518808 [details] [diff] [review]
Part 1 Implement join/refresh/leave with the Loop server on the standalone UI.

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

This is a first batch of comments, I'll keep reviewing all this after lunch ;)

Looks good, but you might want to consider some comments I made.

::: browser/components/loop/standalone/content/js/standaloneMozLoop.js
@@ +14,5 @@
> +  /**
> +   * Validates a data object to confirm it has the specified properties.
> +   *
> +   * @param  {Object} The data object to verify
> +   * @param  {Array} The list of properties to verify within the object

Nit: please add the argument name.

@@ +18,5 @@
> +   * @param  {Array} The list of properties to verify within the object
> +   * @return Returns all properties if valid, or an empty object if no properties
> +   *         have been specified.
> +   */
> +  function validate(data, properties) {

Couldn't we reuse validate.js stuff here?

function validate(data, schema){
  return new Validator(schema).validate(data);
}

Obviously, the schema passed to the current validate function should be upgraded to an object of type definitions.

@@ +74,5 @@
> +  StandaloneMozLoopRooms.prototype = {
> +    /**
> +     * The maximum number of clients that we currently support.
> +     */
> +    _maxClients: 2,

How about moving this at the top of the root IIFE, into some constant style named var, eg. ROOM_MAX_CLIENTS?

@@ +186,5 @@
> +    this.rooms = new StandaloneMozLoopRooms(options);
> +  };
> +
> +  StandaloneMozLoop.prototype = {
> +  };

Nit: is this really necessary?

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +995,5 @@
>        sdk: OT
>      });
>      var activeRoomStore = new loop.store.ActiveRoomStore({
>        dispatcher: dispatcher,
> +      mozLoop: standaloneMozLoop

Yes!

::: browser/components/loop/test/shared/activeRoomStore_test.js
@@ +163,5 @@
> +    it("should save the token", function() {
> +      store.fetchServerData({
> +        windowType: "room",
> +        token: "fakeToken"
> +      });

I wonder if we shouldn't keep passing actions to store methods… That would add a bit of validation in tests. Thoughts?

::: browser/components/loop/test/standalone/standaloneMozLoop_test.js
@@ +91,5 @@
> +
> +    beforeEach(function() {
> +      mozLoop = new loop.StandaloneMozLoop(
> +        {baseServerUrl: fakeBaseServerUrl}
> +      );

Nit: 

mozLoop = new loop.StandaloneMozLoop({
  baseServerUrl: fakeBaseServerUrl
});

Comment 4

[Mark Banner (:standard8)]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #3)
> ::: browser/components/loop/test/shared/activeRoomStore_test.js
> @@ +163,5 @@
> > +    it("should save the token", function() {
> > +      store.fetchServerData({
> > +        windowType: "room",
> > +        token: "fakeToken"
> > +      });
> 
> I wonder if we shouldn't keep passing actions to store methods… That would
> add a bit of validation in tests. Thoughts?

Yep, I think my brain had gone to sleep at that stage.

Comment 5

[Mark Banner (:standard8)]

Attachment: Part 1 Implement join/refresh/leave with the Loop server on the standalone UI.

Updated patch for review comments.

Comment 6

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

Comment on attachment 8518937 [details] [diff] [review]
Part 1 Implement join/refresh/leave with the Loop server on the standalone UI.

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

Looks great!

Comment 7

[Mark Banner (:standard8)]

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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1cbe75fc5bbc

Comment 9

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

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

Comment 10

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

Comment on attachment 8518937 [details] [diff] [review]
Part 1 Implement join/refresh/leave with the Loop server on the standalone UI.

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only

Comment 11

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

Attachment: Part 2: Room views for Loop standalone.

Working implementation! Still needs polishing, and testing, but I'd like some preliminary feedback at this stage.

Comment 12

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

Attachment: Part 2: Room views for Loop standalone.

This is now ready for review, with tests added.

Comment 13

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

Attachment: Part 2: Room views for Loop standalone. Patch v2

Fixed an issue with the local video border being erroneously shown in READY state. Also fixed glitches in the UI showcase. Also, improved tests.

Comment 14

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

Attachment: Part 2: Room views for Loop standalone. Patch v3

Removed debug statements & obsolete test comments.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8521204 [details] [diff] [review]
Part 2: Room views for Loop standalone. Patch v3

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

Looking good. A few minorish comments, but I'd like to look at it again after the mute stuff is fixed.

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +129,5 @@
> +             this.state.roomState === ROOM_STATES.HAS_PARTICIPANTS;
> +    },
> +
> +    _renderActionButtons: function() {
> +      // XXX render "Download FFx to start your own" btn on over capacity

Might as well reference bug 1074709 here.

@@ +146,2 @@
>      // XXX Implement tests for this view when we do the proper views
>      // - bug 1074705 and others

This XXX can be removed now.

@@ +166,5 @@
> +                <div className={localStreamClasses}></div>
> +              </div>
> +              <sharedViews.ConversationToolbar
> +                video={{enabled: !this.state.videoMuted, visible: true}}
> +                audio={{enabled: !this.state.audioMuted, visible: true}}

I think we want to set visible to this._roomIsActive() or have some other way of disabling them (they are currently enabled outside of the active room). Although it'd be interesting to experiment with later by having them available outside the room before its joined ;-)

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +36,5 @@
> +        activeRoomStore: activeRoomStore
> +      }));
> +  }
> +
> +  describe("#componentWillUpdate", function() {

Doesn't this whole lot need wrapping in a describe for "StandaloneRoomView" in case we add another view in this file?

@@ +37,5 @@
> +      }));
> +  }
> +
> +  describe("#componentWillUpdate", function() {
> +    it("should prompt for media access permission to end user", function() {

I think it'd be clearer to say dispatch an "SetupStreamElements" action, since that's what its doing. The media access permission is a consequence of that action handled by the store.

@@ +44,5 @@
> +      var view = mountTestComponent();
> +
> +      activeRoomStore.setStoreState({roomState: ROOM_STATES.JOINED});
> +
> +      sinon.assert.calledOnce(dispatch);

Need to test for the action type here.

@@ +65,5 @@
> +        videoMuted: true
> +      });
> +    });
> +
> +    it("should mute local audio stream ", function() {

nit: unnecessary space after stream

@@ +75,5 @@
> +        enabled: true
> +      }));
> +    });
> +
> +    it("should mute local video stream ", function() {

nit: unnecessary space after stream

Comment 16

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

Attachment: Part 2: Room views for Loop standalone. Patch v4

Addressed comments.

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 8521369 [details] [diff] [review]
Part 2: Room views for Loop standalone. Patch v4

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

Looks good for a beginning, the other bugs can pick up the improvements.

Comment 18

[Mark Banner (:standard8)]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35a67873666d

Comment 20

[Mark Banner (:standard8)]

Only one patch landed on aurora, so downgrading from fixed for 35.

Comment 21

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

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

Comment 22

[Paul Silaghi, QA [:pauly]]

Verified fixed on the standalone page in FF 35b5, Chrome.

Note that the conversation title is displayed only after joining the call, but remains displayed after leaving the call. Please say if I should file a bug for this.

Comment 23

[Mark Banner (:standard8)]

(In reply to Paul Silaghi, QA [:pauly] from comment #22)
> Note that the conversation title is displayed only after joining the call,
> but remains displayed after leaving the call. Please say if I should file a
> bug for this.

It currently depends on a server bug, but I just filed bug 1114563 for it.