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

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: Expected UX

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 2

[Mark Banner (:standard8)]

Attachment: Part 1 Rename the existing EmptyRoomView to be DesktopRoomView, and clean it up, in preparation for the Loop room view implementation.

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.

Comment 3

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

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).

Comment 4

[Mark Banner (:standard8)]

Landed part 1 to allow other work to continue:

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

Comment 5

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

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

Comment 6

[u279076]

[Tracking Requested - why for this release]: nominated to track since this is a Loop 35 blocker.

Comment 7

[Mark Banner (:standard8)]

Attachment: Part 2 Add Join/Refresh/Leave room functions to the mozLoop API.

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).

Comment 8

[Mark Banner (:standard8)]

Attachment: WIP Part 3. Hook the new activeRoomStore into the standalone views, and also extend the store to manage joining rooms on the Loop server.

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.

Comment 9

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

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.

Comment 10

[Mike de Boer [:mikedeboer]]

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.

Comment 11

[Mark Banner (:standard8)]

Attachment: Part 2 Add Join/Refresh/Leave room functions to the mozLoop API.

Comment 12

[Mark Banner (:standard8)]

Attachment: Part 3 Hook the new activeRoomStore into the standalone views, and also extend the store to manage joining rooms on the Loop server.

Now with tests plus a few bug fixes, and also addressed the feedback comments.

Comment 13

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

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.

Comment 14

[Mark Banner (:standard8)]

Attachment: Part 3 Hook the new activeRoomStore into the standalone views, and also extend the store to manage joining rooms on the Loop server.

Updated for review comments.

Comment 15

[Mark Banner (:standard8)]

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

Comment 16

[Mark Banner (:standard8)]

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

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/e5503802d876
https://hg.mozilla.org/mozilla-central/rev/ca16d47debf8
https://hg.mozilla.org/mozilla-central/rev/b82e0ba1f833

Comment 18

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b6da93be552
https://hg.mozilla.org/releases/mozilla-aurora/rev/259292d13712
https://hg.mozilla.org/releases/mozilla-aurora/rev/921694002337
https://hg.mozilla.org/releases/mozilla-aurora/rev/dbf5852b8fd3

Comment 19

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

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

Comment 20

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

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

Comment 21

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

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

Comment 22

[Paul Silaghi, QA [:pauly]]

Is the 'rename' button in the https://bugzilla.mozilla.org/attachment.cgi?id=8497347 still a requirement?

Comment 23

[Romain Testard [:RT]]

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.

Comment 24

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 35b4, 36.0a2 (2014-12-18) Ubuntu 13.04

Comment 25

[Mark Banner (:standard8)]

Now covered by functional tests.