Closed Bug 1074688 Opened 5 years ago Closed 5 years ago

As a user, I should be automatically joined into a room when I preview it (JoinRoom)

Categories

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

enhancement
Points:
5

Tracking

(firefox35+ verified, firefox36 verified)

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

People

(Reporter: standard8, Assigned: standard8)

References

()

Details

(Whiteboard: [rooms])

User Story

* As a user, I should be automatically joined into a room when I preview it, so that my peers can join me straight away

UX:
* Nothing extra over bug 1074686

Implementation:
* When opening a room preview and gathering data:
* Do a POST /rooms/{token} with action "join"
* This returns the API keys, these are saved for later
* Schedule follow-up POST /rooms/{token} with action "refresh" to refresh the room at the "expires" time in the response
* Schedule further follow-up posts to refresh the room when the server responds with the "expires" time
* When the preview is closed, POST /rooms/{token} with action "leave"

Attachments

(4 files, 3 obsolete files)

Attached image Expected UX
No description provided.
Severity: normal → enhancement
User Story: (updated)
Summary: As a user, I should see the other room member → As a user, I should be automatically joined into a room when I preview it
Blocks: 1074690
Whiteboard: [rooms]
backlog: --- → Fx35+
So this is a little interesting, in that we need to decide how this will work w.r.t. the API we're fleshing out here: https://etherpad.mozilla.org/loop-rooms-rest-stuff.  In particular, when pkerr and I were chatting about this yesterday, we speculated that it makes sense to have the chrome code behind mozLoop automatically do most of this stuff as soon as a successful creation callback returns.

Figuring out how the callback dance works here is probably worth a video chat.
Summary: As a user, I should be automatically joined into a room when I preview it → As a user, I should be automatically joined into a room when I preview it (JoinRoom)
Priority: -- → P1
Some cleanup to begin with, to make the following patch(es) clearer. This drops some of the old code from when we were going to open the window at initial room creation time.

This also renames the view to make it clearer as to what it'll end up doing. I went for DesktopRoomView to distinguish it from the standalone side, although I suspect we'll name views differently there, but in any case, its clear where the target is. Better names welcome.
Attachment #8516124 - Flags: review?(nperriault)
Assignee: nobody → standard8
Iteration: --- → 36.2
Points: --- → 5
Comment on attachment 8516124 [details] [diff] [review]
Part 1 Rename the existing EmptyRoomView to be DesktopRoomView, and clean it up, in preparation for the Loop room view implementation.

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

LGTM with comment addressed.

::: browser/components/loop/content/shared/js/mixins.js
@@ +65,5 @@
> +  var DocumentTitleMixin = {
> +    setTitle: function(newTitle) {
> +      rootObject.document.title = newTitle;
> +    }
> +  };

I think we want a small generic test for this (more for regression testing purpose than anything else).
Attachment #8516124 - Flags: review?(nperriault) → review+
Landed part 1 to allow other work to continue:

https://hg.mozilla.org/integration/fx-team/rev/a8567b311260
Keywords: leave-open
[Tracking Requested - why for this release]: nominated to track since this is a Loop 35 blocker.
Flags: qe-verify+
Flags: in-testsuite?
This adds join/refresh/leave to the mozLoop.rooms API. I think this is what Mike suggested to me. I want the automatic refresh to be handled in the content side of things, as then we can share that code with the standalone API. You can see this in Part 3 - for which I'm just about to put up a WIP.

I had a few issues with the Leave function - as I need to be able to call it in the unload callback, it ended up that it was throwing errors due to the cloneInto function failing - it was attempting to clone into a dead window object.

Since we don't actually need any data back from the leave function, I decided to make the callback optional (its useful to have the callback from tests).
Attachment #8517808 - Flags: review?(mdeboer)
This is still WIP - its functional as far as the code I've written, and I think its probably review worthy, except I've not finished writing the tests yet. Hence feedback welcome.
Attachment #8517809 - Flags: feedback?(nperriault)
Comment on attachment 8517809 [details] [diff] [review]
WIP Part 3. Hook the new activeRoomStore into the standalone views, and also extend the store to manage joining rooms on the Loop server.

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

Looks awesome, this is shaping good.

::: browser/components/loop/content/js/conversation.jsx
@@ +670,3 @@
>        navigator.mozLoop.calls.clearCallInProgress(windowId);
> +
> +      dispatcher.dispatch(new sharedActions.WindowUnload());

This will get hard to test, as simulating a window unload event is complicated. I wonder if we shouldn't start thinking of creating a wrapper for the window objet at some point, which would make it much more easily mockable (just a thought).

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +145,5 @@
> +
> +          // For the conversation window, we need to automatically
> +          // join the room.
> +          this._dispatcher.dispatch(
> +            new sharedActions.JoinRoom());

Nit: this could fit on a single line.

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

Nit: add an XXX referencing the bug number for adding a mozLoop compatible object to standalone.
Attachment #8517809 - Flags: feedback?(nperriault) → feedback+
Comment on attachment 8517808 [details] [diff] [review]
Part 2 Add Join/Refresh/Leave room functions to the mozLoop API.

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

r=me with comments addressed. Nice!

::: browser/components/loop/LoopRooms.jsm
@@ +276,5 @@
>  
> +  /**
> +   * Internal function to handle POSTs to a room.
> +   *
> +   * @param {String} roomToken The room token.

nit: can you align the param docs as columns?

@@ +285,5 @@
> +   */
> +  _postToRoom(roomToken, postData, callback) {
> +    let url = "/rooms/" + encodeURIComponent(roomToken);
> +    MozLoopService.hawkRequest(this.sessionType, url, "POST", postData).then(response => {
> +      // Delete doesn't have a body return

nit: missing period.

@@ +403,5 @@
> +  join: function(roomToken, callback) {
> +    return LoopRoomsInternal.join(roomToken, callback);
> +  },
> +
> +  refresh: function(roomToken, sessionToken, callback) {

I think it'd be more clear to rename this to `refreshMembership`

::: browser/components/loop/test/xpcshell/test_looprooms.js
@@ +329,5 @@
>  });
>  
> +// Test if joining a room works as expected.
> +add_task(function* test_joinRoom() {
> +  // We need these set up for getting the email address

nit: missing period.
Attachment #8517808 - Flags: review?(mdeboer) → review+
Attachment #8517808 - Attachment is obsolete: true
Attachment #8517809 - Attachment is obsolete: true
Attachment #8518137 - Flags: review+
Now with tests plus a few bug fixes, and also addressed the feedback comments.
Attachment #8518140 - Flags: review?(nperriault)
Comment on attachment 8518140 [details] [diff] [review]
Part 3 Hook the new activeRoomStore into the standalone views, and also extend the store to manage joining rooms on the Loop server.

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

Looks great! Small nits to be addressed.

::: browser/components/loop/content/js/roomViews.jsx
@@ +57,5 @@
> +
> +      if (this.state.roomState === ROOM_STATES.FAILED) {
> +        return <loop.conversation.GenericFailureView
> +              cancelCall={this.closeWindow}
> +          />;

Nit: tiny indentation problem here.

::: browser/components/loop/test/shared/activeRoomStore_test.js
@@ +82,5 @@
>        fakeToken = "337-ff-54";
> +      fakeRoomData = {
> +        roomName: "Monkeys",
> +        roomOwner: "Alfred",
> +        roomUrl: "invalid.com"

Nit: This is an actually valid domain, let me suggest using http://invalid instead.

@@ +166,5 @@
> +      fakeRoomInfo = {
> +        roomName: "Its a room",
> +        roomOwner: "Me",
> +        roomToken: "fakeToken",
> +        roomUrl: "invalid.com"

Same remark as above.

@@ +182,5 @@
> +
> +      expect(store._storeState.roomName).eql(fakeRoomInfo.roomName);
> +      expect(store._storeState.roomOwner).eql(fakeRoomInfo.roomOwner);
> +      expect(store._storeState.roomToken).eql(fakeRoomInfo.roomToken);
> +      expect(store._storeState.roomUrl).eql(fakeRoomInfo.roomUrl);

Please add a comment so we don't forget to use getStoreState(keyname) as soon as it's available.

@@ +255,5 @@
> +      store.joinedRoom(fakeJoinedData);
> +
> +      expect(store._storeState.apiKey).eql(fakeJoinedData.apiKey);
> +      expect(store._storeState.sessionToken).eql(fakeJoinedData.sessionToken);
> +      expect(store._storeState.sessionId).eql(fakeJoinedData.sessionId);

Same remark as above. Maybe in the meanwhile you can do:

var state = store.getStoreState();
expect(state.apiKey).eql(fakeJoinedData.apiKey);
…

@@ +262,5 @@
> +    it("should call mozLoop.rooms.refreshMembership before the expiresTime",
> +      function() {
> +        store.joinedRoom(fakeJoinedData);
> +
> +        sandbox.clock.tick(fakeJoinedData.expires * 1000);

This is beautiful.
Attachment #8518140 - Flags: review?(nperriault) → review+
Had to fix a small bit of unit test bustage due to an audio sounds patch that landed earlier today:

https://hg.mozilla.org/integration/fx-team/rev/b82e0ba1f833
Flags: in-moztrap?
Comment on attachment 8516124 [details] [diff] [review]
Part 1 Rename the existing EmptyRoomView to be DesktopRoomView, and clean it up, in preparation for the Loop room view implementation.

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8516124 - Flags: approval-mozilla-aurora?
Comment on attachment 8518137 [details] [diff] [review]
Part 2 Add Join/Refresh/Leave room functions to the mozLoop API.

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8518137 - Flags: approval-mozilla-aurora?
Comment on attachment 8518215 [details] [diff] [review]
Part 3 Hook the new activeRoomStore into the standalone views, and also extend the store to manage joining rooms on the Loop server.

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8518215 - Flags: approval-mozilla-aurora?
Attachment #8516124 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8518137 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8518215 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Is the 'rename' button in the https://bugzilla.mozilla.org/attachment.cgi?id=8497347 still a requirement?
Flags: needinfo?(rtestard)
It's only a nice to have (was identified as P3 when prioritizing room features).
We now have the ability to rename the room from within the panel so not having this ability to rename the room from within the room when someone else is with me in the room is fine with me. 
No bug has been created for this feature for that reason.
Flags: needinfo?(rtestard)
Verified fixed FF 35b4, 36.0a2 (2014-12-18) Ubuntu 13.04
Status: RESOLVED → VERIFIED
Now covered by functional tests.
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-moztrap?
You need to log in before you can comment on or make changes to this bug.