Closed Bug 1074702 Opened 5 years ago Closed 5 years ago

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

Categories

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

enhancement
Points:
3

Tracking

(firefox35 fixed, firefox36 fixed)

VERIFIED FIXED
mozilla36
Iteration:
36.2
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
Blocking Flags:
backlog Fx35+

People

(Reporter: standard8, Assigned: standard8)

References

()

Details

(Whiteboard: [rooms][strings])

User Story

As a user, I should be able to join a room when I select, so that I can view the room and details before I decide to join

UX:
* Define the approximate conversation layout, display:
** Join the Conversation button [Removed "Ready to join the room?" from the UX, replaced original UX string "Join now"]
** Conversation title
** ToS
**  mozilla logo
** Firefox Hello logo

Other parts do not need to be part of this bug, unless it makes sense (e.g. video layout may make sense as there are existing styles)

Strings:
* Join the Conversation
* (re-use- updated with Hello) By using Hello, you agree to the Terms of Use and Privacy Notice
* (re-use- updated with Hello) Firefox Hello

Implementation:
*  Create an appropriate view, that is capable of having different strings/streams displayed in the middle (as per mock-ups)
* For this bug, it needs to display the join button
* Re-use as much as possible the existing standalone link clicker UI
* Extend the roomStore to GET /rooms/{token} when displaying the room
* Display the title when the information is received
* When the Join the Conversation button is clicked, trigger the POST /rooms/{token} with action "join" as per the conversation window implementation

Attachments

(3 files, 6 obsolete files)

452.26 KB, image/png
Details
38.32 KB, patch
NiKo
: review+
Details | Diff | Splinter Review
35.56 KB, patch
standard8
: review+
Details | Diff | Splinter Review
Attached image Expected UX
No description provided.
User Story: (updated)
Blocks: 1074705
Blocks: 1074707
Blocks: 1074709
User Story: (updated)
User Story: (updated)
backlog: --- → Fx35+
Priority: -- → P1
Assignee: nobody → standard8
Iteration: --- → 36.2
Points: --- → 3
Depends on: 1094128
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.
Updated patch that comments & update standaloneMozLoop, and finshes a couple of things I had outstanding. Now ready for review.
Attachment #8518808 - Flags: review?(nperriault)
Attachment #8518517 - Attachment is obsolete: true
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
});
(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.
Updated patch for review comments.
Attachment #8518937 - Flags: review?(nperriault)
Attachment #8518808 - Attachment is obsolete: true
Attachment #8518808 - Flags: review?(nperriault)
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!
Attachment #8518937 - Flags: review?(nperriault) → review+
Target Milestone: --- → mozilla36
Flags: qe-verify+
Flags: in-testsuite?
Flags: in-moztrap?
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
Attachment #8518937 - Flags: approval-mozilla-aurora?
Working implementation! Still needs polishing, and testing, but I'd like some preliminary feedback at this stage.
Attachment #8520680 - Flags: feedback?(standard8)
This is now ready for review, with tests added.
Attachment #8520680 - Attachment is obsolete: true
Attachment #8520680 - Flags: feedback?(standard8)
Attachment #8520741 - Flags: review?(standard8)
Attachment #8518937 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Fixed an issue with the local video border being erroneously shown in READY state. Also fixed glitches in the UI showcase. Also, improved tests.
Attachment #8520741 - Attachment is obsolete: true
Attachment #8520741 - Flags: review?(standard8)
Attachment #8520917 - Flags: review?(standard8)
Removed debug statements & obsolete test comments.
Attachment #8520917 - Attachment is obsolete: true
Attachment #8520917 - Flags: review?(standard8)
Attachment #8521204 - Flags: review?(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
Attachment #8521204 - Flags: review?(standard8) → review-
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.
Attachment #8521369 - Flags: review?(standard8) → review+
Only one patch landed on aurora, so downgrading from fixed for 35.
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(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.
Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.