1074680 - As a user, I should be able to create a new room

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: Expected UX

Comment 1

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

Do we agree that HTTP request will be made through the mozLoop API here?

Comment 2

[Mark Banner (:standard8)]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #1)
> Do we agree that HTTP request will be made through the mozLoop API here?

I would expect to use a mozLoop.hawkRequest, but in such a way, that we're extending client.js with a new function that invokes it.

Comment 3

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

Attachment: Part 1 - Loop Rooms: Start a Conversation UI.

Ready for review.

Comment 4

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

Comment on attachment 8502679 [details] [diff] [review]
Part 1 - Loop Rooms: Start a Conversation UI.

Just realized that strings for Fx35 has landed already, and they introduce a `rooms_default_room_name_template` which is incompatible with this patch.

Will adapt it and resubmit, canceling review for now.

Comment 5

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

Attachment: Part 1 - Loop Rooms: Start a Conversation UI.

Updated patch to use the new room default name template string introduced lately.

Comment 6

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

Comment on attachment 8503038 [details] [diff] [review]
Part 1 - Loop Rooms: Start a Conversation UI.

This breaks the UI showcase. Canceling review for now; sorry for the noise.

Comment 7

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

Attachment: Part 1 - Loop Rooms: Start a Conversation UI.

Fixed the issue previously mentioned.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8503067 [details] [diff] [review]
Part 1 - Loop Rooms: Start a Conversation UI.

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

Looking good. Giving f+ rather than r+ as I'd like to look at it again briefly once the comments are addressed.

::: browser/components/loop/content/js/panel.jsx
@@ +537,3 @@
>        this.listenTo(this.props.store, "change", this._onRoomListChanged);
>  
>        this.props.dispatcher.dispatch(new sharedActions.GetAllRooms());

Side note: I just realised this is going to cause us to update the rooms list every time we switch to the view, despite the fact the store will be self-updating later on due to being notified from mozLoop.

Its possibly not too much of an issue as we should be handling updates nicely and there won't be massive amounts of rooms, but it feels to me that this dispatch is happening in the wrong place.

::: browser/components/loop/content/shared/js/actions.js
@@ +131,5 @@
> +     * Creates a new room.
> +     * XXX: should move to some roomActions module - refs bug 1079284
> +     */
> +    CreateRoom: Action.define("createRoom", {
> +      // The template to use to name the new room

nit: Might be worth a bit of clarity making this "The localised template..."

::: browser/components/loop/content/shared/js/roomListStore.js
@@ +96,5 @@
>     * @param {Object} options Options object.
>     */
>    function RoomListStore(options) {
>      options = options || {};
> +    this.storeState = {

These states need documentation, it isn't entirely clear at a glance what they are or when they might be set.

@@ +221,5 @@
> +
> +      if (!this.mozLoop.hasOwnProperty("rooms")) {
> +        // Faking for now until bug 1074664 lands.
> +        setTimeout(function() {
> +          this.onRoomCreated(temporaryCreatedRoom);

Although this temporary, I think it'd be nice for testing just to add:

temporaryCreatedRoom.roomName = this._generateNewRoomName(actionData.nameTemplate);

@@ +246,5 @@
> +     *
> +     * @param  {Object} rawCreatedRoomData The raw created room information.
> +     */
> +    onRoomCreated: function(rawCreatedRoomData) {
> +      this.setStoreState({

Just to note, this is using a different way from what I've done in the conversationStore - where items that receive data from the server dispatch new actions.

I went for the action approach as that gave better debug possibilities for tracking actions/events through the system.

It may be worth considering if we need consistency, but I don't have a strong opinion here.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +654,5 @@
>        sinon.assert.calledOnce(dispatch);
>        sinon.assert.calledWithExactly(dispatch, new sharedActions.GetAllRooms());
>      });
> +
> +    it("should dispatch a CreateRoom action when clicking on the Start a " +

I think we also need a test somewhere that the button gets disabled when the state is pending.

::: browser/components/loop/test/shared/roomListStore_test.js
@@ +126,5 @@
> +
> +          expect(store.findNextAvailableRoomNumber(fakeNameTemplate)).eql(2);
> +        });
> +
> +      it("should not be sensible to initial list order", function() {

nit: s/sensible/sensitive/

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: UI to create a new Hello room, v2

This is rebased against PauKerr/bug/1074699* github branch.  It gets the tests passing with the updated APIs, and things mostly seem to work when actually creating a room from the UI.  Right now, roomOwner is hard-coded, because it doesn't seem to live in any single sane place that I can find, so that needs to be cleaned up, among other things.  Left maxSize hardcoded to two, based on the agile theory of not adding unused code before you need it.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Mark Banner (:standard8) from comment #8)
> Comment on attachment 8503067 [details] [diff] [review]
> Part 1 - Loop Rooms: Start a Conversation UI.
> 
> Review of attachment 8503067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good. Giving f+ rather than r+ as I'd like to look at it again
> briefly once the comments are addressed.
> 
> ::: browser/components/loop/content/js/panel.jsx
> @@ +537,3 @@
> >        this.listenTo(this.props.store, "change", this._onRoomListChanged);
> >  
> >        this.props.dispatcher.dispatch(new sharedActions.GetAllRooms());
> 
> Side note: I just realised this is going to cause us to update the rooms
> list every time we switch to the view, despite the fact the store will be
> self-updating later on due to being notified from mozLoop.
> 
> Its possibly not too much of an issue as we should be handling updates
> nicely and there won't be massive amounts of rooms, but it feels to me that
> this dispatch is happening in the wrong place.

I've added an XXX comment that we should remove this once we've got a better mechanism, such as bug 1074665.
 
> ::: browser/components/loop/content/shared/js/actions.js
> @@ +131,5 @@
> > +     * Creates a new room.
> > +     * XXX: should move to some roomActions module - refs bug 1079284
> > +     */
> > +    CreateRoom: Action.define("createRoom", {
> > +      // The template to use to name the new room
> 
> nit: Might be worth a bit of clarity making this "The localised template..."

Tweaked.

> @@ +221,5 @@
> > +
> > +      if (!this.mozLoop.hasOwnProperty("rooms")) {
> > +        // Faking for now until bug 1074664 lands.
> > +        setTimeout(function() {
> > +          this.onRoomCreated(temporaryCreatedRoom);
> 
> Although this temporary, I think it'd be nice for testing just to add:
> 
> temporaryCreatedRoom.roomName =
> this._generateNewRoomName(actionData.nameTemplate);

I got the sense from talking to Niko today that these will likely go away by the time this patch lands, because we'll land it after pkerr's patch in bug 1074699.

> @@ +246,5 @@
> > +     *
> > +     * @param  {Object} rawCreatedRoomData The raw created room information.
> > +     */
> > +    onRoomCreated: function(rawCreatedRoomData) {
> > +      this.setStoreState({
> 
> Just to note, this is using a different way from what I've done in the
> conversationStore - where items that receive data from the server dispatch
> new actions.
> 
> I went for the action approach as that gave better debug possibilities for
> tracking actions/events through the system.
> 
> It may be worth considering if we need consistency, but I don't have a
> strong opinion here.

I do see your point, as you say, it's a little unclear how much value there is in consistency here.  Given our current deadlines, I'm inclined to say we should postpone any such change until such time as we become convinced that it's going to make a non-trivial difference in debugability.  FWIW, localRoomStore.js is using a similar pattern to this one at the moment.

> ::: browser/components/loop/test/shared/roomListStore_test.js
> @@ +126,5 @@
> > +
> > +          expect(store.findNextAvailableRoomNumber(fakeNameTemplate)).eql(2);
> > +        });
> > +
> > +      it("should not be sensible to initial list order", function() {
> 
> nit: s/sensible/sensitive/

Fixed.

These two still need addressing:

> ::: browser/components/loop/test/desktop-local/panel_test.js
> @@ +654,5 @@
> >        sinon.assert.calledOnce(dispatch);
> >        sinon.assert.calledWithExactly(dispatch, new sharedActions.GetAllRooms());
> >      });
> > +
> > +    it("should dispatch a CreateRoom action when clicking on the Start a " +
> 
> I think we also need a test somewhere that the button gets disabled when the
> state is pending.

> ::: browser/components/loop/content/shared/js/roomListStore.js
> @@ +96,5 @@
> >     * @param {Object} options Options object.
> >     */
> >    function RoomListStore(options) {
> >      options = options || {};
> > +    this.storeState = {
> 
> These states need documentation, it isn't entirely clear at a glance what
> they are or when they might be set.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: UI to create a new Hello room, v3

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: UI to create a new Hello room, v4

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8508267 [details] [diff] [review]
UI to create a new Hello room, v4

So this has addressed all of Standard8's review comments, _except_ the one about the documentation of the pending state.  That state doesn't seem coherent to me.  Which is to say, it's used for both a pending room creation, to disable the button, but it's also set in getAllRooms, which seems curious to me.  If it's supposed to represent "any operation in the store being pending", it does seem to do that, but it strikes me as odd that we wouldn't allow room creation just because the room list hasn't yet loaded.  It's possible, on the other hand, that these were intended to be two separate states.

Niko, can you elaborate on how you think this ought to work?  And if you want to fix any docs/code to make that happene, I wouldn't be upset.  :-)

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Note that we do still want to take advantage of the retval that pkerr added in the API so that the conversation list can be updated immediately, and I haven't added that to this patch.  I think it'd be fine to do that either as part of this patch or part of a separate one.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: UI to create a new Hello room, v5 (WIP)

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8509209 [details] [diff] [review]
UI to create a new Hello room, v5 (WIP)

This patch is based on a local branch of today's fx-team, with pkerr's patches for bug 1074663, bug 1074664, and bug 1074699 applied on top, in that order.

Alternately, it's available on github on my "niko-create-2" branch.  

<https://github.com/dmose/gecko-dev/compare/dmose:pkerr-review-699...niko-create-2> has a compare.

As you can see, it's very much WIP.  I switched to using dispatch for all store modifications in callbacks for some combination of simplicity & safety.

Some key next steps include filling out the XXXes in roomListStore_test.js, then getting rid of the pending states, and making corresponding updates to the views.

I'm especially interested in hearing your thoughts on this direction.  If you're inclined to do more, continuing to move this patch forward would rock, otherwise I'd love to at least chat about it in the Pacific morning.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #16)
> I switched to using dispatch for all store modifications in callbacks [...]

By which I really mean, "I started to switch to..."

Comment 18

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

Comment on attachment 8509209 [details] [diff] [review]
UI to create a new Hello room, v5 (WIP)

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

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #16)
> This patch is based on a local branch of today's fx-team, with pkerr's
> patches for bug 1074663, bug 1074664, and bug 1074699 applied on top, in
> that order.

Makes me think this workflow is really complicated. We need to find a better way.

> As you can see, it's very much WIP.  I switched to using dispatch for all
> store modifications in callbacks for some combination of simplicity & safety.

I now think this is the way to go; all async data rerieval calls should go through the dispatcher, as we discussed yesterday on IRC. I'm still unsure with actions matching the callback(err, data) signature, but I need to spend more time thinking of better alternatives.

> Some key next steps include filling out the XXXes in roomListStore_test.js,
> then getting rid of the pending states, and making corresponding updates to
> the views.

I'll take care of that if I can find the time.

::: browser/components/loop/content/js/panel.jsx
@@ +516,5 @@
>  
>      propTypes: {
>        store: React.PropTypes.instanceOf(loop.store.RoomListStore).isRequired,
>        dispatcher: React.PropTypes.instanceOf(loop.Dispatcher).isRequired,
> +      displayName: React.PropTypes.string.isRequired  // for room creation

Let's be very explicit about this and use `authUserDisplayName`. Ideally I'd like we get a proper AuthUser object instance passed in here, eventually (later).

@@ +551,5 @@
>  
> +    handleCreateButtonClick: function() {
> +      this.props.dispatcher.dispatch(new sharedActions.CreateRoom({
> +        nameTemplate: mozL10n.get("rooms_default_room_name_template"),
> +        roomOwner: this.props.displayName

roomOwner: this.props.authUserDisplayName

@@ +657,2 @@
>       */
> +    _renderRoomsTab: function(displayName) {

s/displayName/authUserDisplayName

@@ +662,5 @@
>        return (
>          <Tab name="rooms">
>            <RoomList dispatcher={this.props.dispatcher}
> +                    store={this.props.roomListStore}
> +                    displayName={displayName}/>

authUserDisplayName={displayName}

@@ +706,5 @@
>                                 callUrl={this.props.callUrl} />
>                  <ToSView />
>                </div>
>              </Tab>
> +            {this._renderRoomsTab(displayName)}

We should rather extract the user displayName computation in a function reusable within _renderRoomsTab.

::: browser/components/loop/content/shared/js/actions.js
@@ +134,5 @@
> +    CreateRoom: Action.define("createRoom", {
> +      // The localized template to use to name the new room
> +      // (eg. "Conversation {{conversationLabel}}").
> +      nameTemplate: String,
> +      roomOwner: String

This also needs a proper description.

@@ +159,5 @@
> +     * XXX
> +     */
> +    UpdateRoomData: Action.define("updateRoomData", {
> +      localRoomId: String,
> +      err: [Error, null],

Wow. I don't much like passing an error object in the action. I'll try to find a better solution.

::: browser/components/loop/content/shared/js/localRoomStore.js
@@ +102,5 @@
>            error: error,
>            localRoomId: actionData.localRoomId,
>            serverData: roomData
> +        };
> +        this.setStoreState(storeData);

Nit: what's the purpose of avoiding passing the object inline?

::: browser/components/loop/content/shared/js/roomListStore.js
@@ +259,5 @@
> +          //this.onRoomCreated(rawCreatedRoomData);
> +        }.bind(this));
> +
> +      var augmentedRoomCreationData = _.extend({localRoomId: localRoomId}, roomCreationData);
> +      augmentedRoomCreationData.localRoomId = localRoomId;

augmentedRoomCreationData = _.extend(roomCreationData, {localRoomId: localRoomId}) is probably simpler.

Comment 19

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

Clearing needinfo.

Comment 20

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

Attachment: Bug 1089547 - Create a Loop room (WiP)

This is for early feedback only. Questions:

- it seems the "add" event is triggered twice after a successful call to mozLoop.rooms.create(); I couln't find the root cause for this in the patch, though I may be missing smthg obvious
- I'm uncomfortable with the verbosity of error handling in the roomListStore; I'd like to hear about better alternatives, if any
- the patch misses test, don't look for them just yet :)

Comment 21

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

Attachment: Bug 1089547 - Create a Loop room (WiP)

Updated WiP patch. Still need to hear some feedback, especially for how two "add" events are triggered when we create a new room.

Comment 22

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

Attachment: Bug 1089547 - Create a Loop room (WiP)

Updated WiP patch with partially revamped tests (for the better). The patch works, but still needs more polishing and tests to write. Would be happy to hear feedback, still.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 8514396 [details] [diff] [review]
Bug 1089547 - Create a Loop room (WiP)

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

I think in general this is good. I can understand what you mean by bloat in the store.

It feels right to separate out the success & error results into separate actions - it gives clear indications of what's going on.

The actions for updating the store from the add/update/remove events do seem a bit strange. This almost feels like there should be a driver listening to the events, and the driver then dispatches the actions. Though that's probably a bit excessive for what we need at the moment. I don't think there's much else that we can do to optimise that code.

Comment 24

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

Attachment: Bug 1089547 - Create a Loop room (WiP)

Thanks for the feedback. This is now ready for review.

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 8514894 [details] [diff] [review]
Bug 1089547 - Create a Loop room (WiP)

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

Looks good. r=Standard8 with the comments fixed. I realise there's an issue with new rooms appearing twice, but that'll be fixed with the patch in bug 1074666.

::: browser/components/loop/content/shared/js/roomListStore.js
@@ +305,5 @@
> +
> +        // We can only start listening to room events after getAll() has been
> +        // called executed first.
> +        // XXX This should be going away as soon as we move to using push
> +        //     notifications only - bug 1074665.

As discussed on irc, this comment should go away as we're doing the performance optimised thing here.

::: browser/components/loop/test/shared/roomListStore_test.js
@@ +152,5 @@
> +    describe("#startListeningToRoomEvents", function() {
> +      it("should start listening to MozLoop room events", function() {
> +        store.startListeningToRoomEvents();
> +
> +        sinon.assert.called(fakeMozLoop.rooms.on);

Aren't these already tested via the "MozLoop rooms event listeners" section above?

@@ +228,5 @@
> +        sandbox.stub(fakeMozLoop.rooms, "create", function(data, cb) {
> +          cb(err);
> +        });
> +
> +        dispatcher.dispatch(new sharedActions.CreateRoom(fakeRoomCreationData));

I think using the direct call (store.createRoom) format is probably fine, it'd make it more consistent for the times when we do need to stub dispatcher.dispatch.

However, to be consistent, you should change it here as well ;-)

Comment 26

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

Attachment: Bug 1089547 - Create a Loop room

Addressed comments. Try build:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1572de9e107a
https://tbpl.mozilla.org/?tree=Try&rev=1572de9e107a

Comment 27

[Mike de Boer [:mikedeboer]]

NiKo`: can you assign a point value?

Pushed to fx-team with the correct bug no.: https://hg.mozilla.org/integration/fx-team/rev/ca4948a63866

Comment 28

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

(In reply to Mike de Boer [:mikedeboer] from comment #27)
> NiKo`: can you assign a point value?

Always a bit odd to do that afterwards, but done :)

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

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

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

Comment 31

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

Comment on attachment 8514974 [details] [diff] [review]
Bug 1089547 - Create a Loop room

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

Comment 32

[Paul Silaghi, QA [:pauly]]

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