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

VERIFIED FIXED in Firefox 35

Status

enhancement
P1
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: standard8, Unassigned)

Tracking

unspecified
mozilla36
Points:
3
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap ?
qe-verify +

Firefox Tracking Flags

(firefox35 verified, firefox36 verified)

Details

(Whiteboard: [rooms][strings], )

User Story

* As a user, I should be able to create a new room, so that I can share the URL with people.

UX:
* When the "Start a Conversation" button is pressed, a new conversation should be added to the conversation list.
* The panel should stay open
* The conversation should be created with a default expiry time of 8 weeks

Strings:
* Start a Conversation (replaced original mock-up of "New Room")
* (Plural form) Conversations <n> (replaced original mock-up of "Room")

Not sure if this strings is used in this bug (putting here in case):
* Start a conversation (replaces current icon tool tip of "Invite someone to talk")

Implementation:
* When the start a conversation button is pressed:
* Find a new name for the conversation, look through the existing list for "Conversation <n>" select the first (1..n) available number
* POST /rooms to the server
** expiry time 8 weeks
** maxSize "2"

https://wiki.mozilla.org/Loop/Architecture/Rooms#POST_.2Frooms

Attachments

(2 attachments, 11 obsolete attachments)

194.56 KB, image/png
Details
43.35 KB, patch
jesup
: review+
Details | Diff | Splinter Review
Posted image Expected UX
No description provided.
User Story: (updated)
Severity: normal → enhancement
User Story: (updated)
User Story: (updated)
Do we agree that HTTP request will be made through the mozLoop API here?
(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 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.
Attachment #8502679 - Flags: review?(standard8)
Updated patch to use the new room default name template string introduced lately.
Attachment #8502679 - Attachment is obsolete: true
Attachment #8503038 - Flags: review?(standard8)
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.
Attachment #8503038 - Flags: review?(standard8)
Fixed the issue previously mentioned.
Attachment #8503038 - Attachment is obsolete: true
Attachment #8503067 - Flags: review?(standard8)
Depends on: 1074664
No longer depends on: 1074672
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/
Attachment #8503067 - Flags: review?(standard8) → feedback+
backlog: --- → Fx35+
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.
(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.
Attachment #8503067 - Attachment is obsolete: true
Attachment #8506632 - Attachment is obsolete: true
Attachment #8507311 - Attachment is obsolete: true
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.  :-)
Flags: needinfo?(nperriault)
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 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.
Attachment #8509209 - Flags: feedback?(nperriault)
(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 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.
Attachment #8509209 - Flags: feedback?(nperriault) → feedback+
Clearing needinfo.
Flags: needinfo?(nperriault)
Priority: -- → P1
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 :)
Attachment #8509209 - Attachment is obsolete: true
Attachment #8513677 - Flags: feedback?
Attachment #8513677 - Flags: feedback?(standard8)
Attachment #8513677 - Flags: feedback?(mdeboer)
Attachment #8513677 - Flags: feedback?
Updated WiP patch. Still need to hear some feedback, especially for how two "add" events are triggered when we create a new room.
Attachment #8513677 - Attachment is obsolete: true
Attachment #8513677 - Flags: feedback?(standard8)
Attachment #8513677 - Flags: feedback?(mdeboer)
Attachment #8514127 - Flags: feedback?(standard8)
Assignee: nobody → nperriault
Iteration: --- → 36.2
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.
Attachment #8514127 - Attachment is obsolete: true
Attachment #8514127 - Flags: feedback?(standard8)
Attachment #8514127 - Flags: feedback?(mdeboer)
Attachment #8514396 - Flags: feedback?(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.
Attachment #8514396 - Flags: feedback?(standard8) → feedback+
Thanks for the feedback. This is now ready for review.
Attachment #8514396 - Attachment is obsolete: true
Attachment #8514894 - Flags: review?(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 ;-)
Attachment #8514894 - Flags: review?(standard8) → review+
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
(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 :)
Points: --- → 3
https://hg.mozilla.org/mozilla-central/rev/ca4948a63866
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Flags: qe-verify+
Flags: in-moztrap?
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
Attachment #8514974 - Flags: review+
Attachment #8514974 - Flags: approval-mozilla-aurora?
Attachment #8514974 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed FF 35b4, 36.0a2 (2014-12-17) Ubuntu 13.04
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.