1100595 - Add UI for indicating if renaming a room failed

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

Broken out from bug 1074694 - if renaming a room failed in the conversation window, then we don't have any UI to indicate that to the user.

Comment 1

[:shell escalante]

hi Matej,  what string do you recommend for this failure (can we get this week or next)?  we can uplift for Fx36.   Darrin/Sevaan - pointers to a UX we should use?

Comment 2

[:shell escalante]

ok - i lied.  need strings and a basic UX ASAP so we can get them in this week or it's 37.

Comment 3

[Matej Novak [:matej]]

I would say something like: 

"Conversation cannot be renamed"

Would that work?

Comment 4

[:shell escalante]

sounds good Matej on the error words, thank you. 

Hi Darrin or Sevaan, in put on the UX for the Error we should use?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Marco, can you please add this to the 37.1 iteration tracking? Thanks!

Comment 6

[Marco Mucci [:MarcoM]]

Added to IT 37.1

Comment 7

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:
We need this to finish the UI for renaming a room.  Requires strings, but the strings landed already in another bug (while Fx36 was still on Nightly).  Only targeting uplift to Fx36.

Comment 8

[Sevaan Franks [:sevaan]]

In what cases would renaming a room fail? Would it be possible for the user have the option to retry?

Comment 9

[Sevaan Franks [:sevaan]]

Attachment: Room Rename Error

Matej, I changed the string slightly:

"Unable to rename room at this time."

We are calling our conversation windows Rooms, so I thought right off the bat that should be one change. The other is that "Conversation cannot be renamed" seemed like a bit of a dead-end and I wanted to imply to the user that the problem is temporary, and not an error as a result of some permission or something the user may have missed.

Thoughts?

Comment 10

[Matej Novak [:matej]]

(In reply to Sevaan Franks [:sevaan] from comment #9)
> Created attachment 8533255 [details]
> Room Rename Error
> 
> Matej, I changed the string slightly:
> 
> "Unable to rename room at this time."
> 
> We are calling our conversation windows Rooms, so I thought right off the
> bat that should be one change. The other is that "Conversation cannot be
> renamed" seemed like a bit of a dead-end and I wanted to imply to the user
> that the problem is temporary, and not an error as a result of some
> permission or something the user may have missed.
> 
> Thoughts?

We're not using the word "room" anywhere leading up to this moment, so introducing it now seems like it could be confusing. Could the label at the top of the window say "Conversation 1," "Conversation 2," etc.?

Otherwise the above string looks good to me, but again, let's make it "conversation" instead of "room."

Thanks.

Comment 11

[Sevaan Franks [:sevaan]]

Attachment: Conversation Rename Error

Thanks Matej. Updated.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Tracking. Please land that during the aurora cycle.

Comment 13

[Mark Banner (:standard8)]

Looks fine to me.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

We also allow renaming a room from the panel.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

We can't use the same input field for the error, since <input> is restricted to a height of 1-line. Switching to a <textarea> could work, but then we will need to listen for Enter to force a submit of the form, and things start to get ugly pretty quickly.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8534672 [details] [diff] [review]
Patch

I'm afk tommorow & friday, so redirecting review.

Comment 17

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

Comment on attachment 8534672 [details] [diff] [review]
Patch

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

We're not far off, but I think we want to revisit the way we store and handle the error (see comments below). And we also want tests, though they could be added as a part 2 if necessary.

::: browser/components/loop/content/js/roomViews.jsx
@@ +112,5 @@
>        this.setState({copiedUrl: true});
>      },
>  
> +    _onRenameRoomError: function() {
> +      if (this.isMounted()) {

To avoid this test, we can start subscribing to the event in component*Did*Mount.

@@ +126,5 @@
>          <div className="room-invitation-overlay">
> +          <p className={cx({"error": !!this.state.roomRenameError,
> +                            "error-display-area": true})}>
> +            {mozL10n.get("rooms_rename_error_label")}
> +          </p>

This actually prints red text on top of the streaming local video[0]; I suspect readability issue depending on the background picture, though I don't have much better to suggest atm. Probably worth filing an issue about that though.

[0]: https://dl.dropboxusercontent.com/spa/0jzcrapltxzlqds/6jny034v.png

::: browser/components/loop/content/shared/js/roomStore.js
@@ +367,5 @@
>        this._mozLoop.rooms.rename(actionData.roomToken, actionData.newRoomName,
>          function(err) {
>            if (err) {
> +            this.setStoreState({
> +              roomRenameError: err,

Nit: Trailing comma. And this new attribute needs to be added to the getInitialStoreState method.

Moreover, I think we should dispatch a RenameRoomError action here, to keep consistent with what's been done so far for createRoom and deleteRoom; in that case, we could just reuse the existing `error` state property — and watch for changes from the component.

Last, whatever we end up with, this part needs testing (have a look in roomStore_test.js) :)

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +305,5 @@
>  rooms_list_delete_tooltip=Delete conversation
>  rooms_list_deleteConfirmation_label=Are you sure?
>  rooms_list_no_current_conversations=No current conversations
>  rooms_name_this_room_label=Name this conversation
> +rooms_rename_error_label=Unable to rename conversation at this time.

So this is something we won't be able to uplift… Wouldn't we rather want to use something more generic like "Something went wrong" for now?

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for uplift

Addressed feedback, added test, and using preexisting string for uplift.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for m-c

Comment 20

[Maire Reavy [:mreavy]]

Hi Mark, Jared -- I believe we landed the string for this bug in bug 1105537:  
+ rooms_name_change_failed_label=Conversation cannot be renamed

I'm fine with changing the string in Fx37 if Matej and Sevaan want to do that, but I think "Conversation cannot be renamed" is better than the generic "Something went wrong."

If there's something I'm missing and the string we landed in bug 1105537 can't be used in Fx36, please let me know.  (Note: I'm only planning to uplift this to Fx36, not Fx35.)  Thanks!

Comment 21

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

Comment on attachment 8535716 [details] [diff] [review]
Patch for uplift

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

We're not far off, just some mistakes to fix and questions to answer before landing. Thanks!

::: browser/components/loop/LoopRooms.jsm
@@ +455,5 @@
>        roomOwner: origRoom.roomOwner
>      };
>      MozLoopService.hawkRequest(this.sessionType, url, "PATCH", patchData)
>        .then(response => {
>          let data = JSON.parse(response.body);

Side note, we may want to wrap this in a try/catch block and callback(err) in case of a JSON parsing error here. If you don't feel like addressing this in this patch, please ensure a bug is file about it, and reference it in a XXX comment here.

@@ +457,5 @@
>      MozLoopService.hawkRequest(this.sessionType, url, "PATCH", patchData)
>        .then(response => {
>          let data = JSON.parse(response.body);
>          extend(room, data);
> +        callback(new Error("omg error"), room);

Please remove this before landing ;)

::: browser/components/loop/content/js/roomViews.jsx
@@ +67,5 @@
>      getInitialState: function() {
>        return {
>          copiedUrl: false,
> +        newRoomName: "",
> +        error: "",

Nit: Maybe null would be a more sensible default here.

::: browser/components/loop/content/shared/css/conversation.css
@@ +156,5 @@
>    background-color: transparent;
>    opacity: 1;
>  }
>  .conversation-toolbar .media-control:hover {
> +  background-color: rgba(255,255,255,.35);

Just out of curiosity, why these changes?

@@ +765,5 @@
> +.room-invitation-overlay .error-display-area.error,
> +.room-invitation-overlay input[type="text"] {
> +  display: block;
> +  background-color: rgba(0,0,0,.5);
> +  border-radius: 3px;

Nit: I wonder if we couldn't use em here as well, so radius would scale along the font size?

::: browser/components/loop/content/shared/js/actions.js
@@ +268,5 @@
> +     * Renaming a room error.
> +     * XXX: should move to some roomActions module - refs bug 1079284
> +     */
> +    RenameRoomError: Action.define("renameRoomError", {
> +      error: [Error]

Please update to [Error, Object] to avoid serialization issues where type is lost.

::: browser/components/loop/content/shared/js/roomStore.js
@@ +121,5 @@
>          error: null,
>          pendingCreation: false,
>          pendingInitialRetrieval: false,
> +        rooms: [],
> +        roomRenameError: ""

This is no more needed as we're now relying on the error property.

Comment 22

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

Comment on attachment 8535720 [details] [diff] [review]
Patch for m-c

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

See comments made for the uplift version of the patch :)

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Addressed issues, and now using pre-existing string.

As for the spacing changes in the CSS, this was to move us closer to the CSS coding standard that we discussed. In this case, https://wiki.mozilla.org/DevTools/CSSTips says "Add a space after each comma, except within color functions. Example: -moz-linear-gradient(top, black 1px, rgba(255,255,255,0.2) 1px)"

Comment 24

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

Comment on attachment 8536715 [details] [diff] [review]
Patch v1.1

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

I'm so sorry but it seems I've missed things in the previous version… Hope you won't hate me too much :x

I'll ping you on IRC, I can probably take over and land the patch tomorrow morning if you're ****)

::: browser/components/loop/LoopRooms.jsm
@@ +461,5 @@
> +          extend(room, data);
> +          callback(null, room);
> +        } catch (ex) {
> +          callback(ex);
> +        }

I just realized we were defining a .catch() clause for this promise already, hence adding a try/catch block is unnecessary… tbh onelining code like this is usually confusing.

Sorry again though, you can revert to what you did in the previous version of the patch :/

::: browser/components/loop/content/js/roomViews.jsx
@@ +86,5 @@
>  
>        var newRoomName = this.state.newRoomName;
>  
>        if (newRoomName && this.state.roomName != newRoomName) {
> +        this.setState({error: ""});

Now I think about it, it really feels like this state value should be updated by the store itself, in RoomStore#renameRoom… Local component state would then be updated automatically with that new error value. Sorry for having missing this earlier :/

(Also, it should be reset as a null)

@@ +111,5 @@
>  
>        this.setState({copiedUrl: true});
>      },
>  
> +    onError: function() {

Nit: How about s/onError/onRoomError?

@@ +126,5 @@
>        return (
>          <div className="room-invitation-overlay">
> +          <p className={cx({"error": !!this.state.error,
> +                            "error-display-area": true})}>
> +            {mozL10n.get("rooms_name_change_failed_label")}

Be careful, the string added to the properties file is "rooms_rename_error_label".

::: browser/components/loop/content/shared/js/roomStore.js
@@ +378,5 @@
>       *
>       * @param {sharedActions.RenameRoom} actionData
>       */
>      renameRoom: function(actionData) {
>        this._mozLoop.rooms.rename(actionData.roomToken, actionData.newRoomName,

So this is where we could

this.setStoreState({error: null});

::: browser/components/loop/test/shared/roomStore_test.js
@@ +439,5 @@
>        store = new loop.store.RoomStore(dispatcher, {mozLoop: fakeMozLoop});
>      });
>  
>      it("should rename the room via mozLoop", function() {
> +      fakeMozLoop.rooms.rename = sinon.spy();

Please also add the definition for rename to the fakeMozLoop declaration line 78 of this file (a noop function is ok).

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.2

The string in the properties file should have been removed, it was leftover that I missed in the previous patch.

Comment 26

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

Comment on attachment 8536796 [details] [diff] [review]
Patch v1.2

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

Looks great, r=me with the remaining small comments addressed :)

::: browser/components/loop/content/shared/js/roomStore.js
@@ +120,5 @@
>          activeRoom: this.activeRoomStore ? this.activeRoomStore.getStoreState() : {},
>          error: null,
>          pendingCreation: false,
>          pendingInitialRetrieval: false,
> +        rooms: [],

Nit: Trailing comma.

::: browser/components/loop/test/shared/roomStore_test.js
@@ +433,5 @@
>  
>      beforeEach(function() {
>        fakeMozLoop = {
>          rooms: {
> +          rename: null

Why don't you keep the spy defined here?

@@ +440,5 @@
>        store = new loop.store.RoomStore(dispatcher, {mozLoop: fakeMozLoop});
>      });
>  
>      it("should rename the room via mozLoop", function() {
> +      fakeMozLoop.rooms.rename = sinon.spy();

This is actually not needed if we keep it defined line 437.

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #26)
> >      beforeEach(function() {
> >        fakeMozLoop = {
> >          rooms: {
> > +          rename: null
> 
> Why don't you keep the spy defined here?
> 
> @@ +440,5 @@
> >        store = new loop.store.RoomStore(dispatcher, {mozLoop: fakeMozLoop});
> >      });
> >  
> >      it("should rename the room via mozLoop", function() {
> > +      fakeMozLoop.rooms.rename = sinon.spy();
> 
> This is actually not needed if we keep it defined line 437.

The spy can't be defined in the beforeEach since the function is mocked within the other test function.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #26)
> ::: browser/components/loop/content/shared/js/roomStore.js
> @@ +120,5 @@
> >          activeRoom: this.activeRoomStore ? this.activeRoomStore.getStoreState() : {},
> >          error: null,
> >          pendingCreation: false,
> >          pendingInitialRetrieval: false,
> > +        rooms: [],
> 
> Nit: Trailing comma.

I prefer to leave the trailing comma so if the object needs to be appended to in a future patch, the diff will only include the new line and not a change of a previous line (affecting hg blame).

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95cb099f4675

Comment 31

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

Comment on attachment 8536796 [details] [diff] [review]
Patch v1.2

Approval Request Comment
[Feature/regressing bug #]: Rooms

[User impact if declined]: No indication if a rename failed

[Describe test coverage new/current, TBPL]: on m-c, includes test

[Risks and why]: low risk; little added complexity other than indicating an error.

[String/UUID change made/needed]: none

Comment 32

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/4c6a7ec58300

Comment 33

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

https://hg.mozilla.org/releases/mozilla-beta/rev/00b1f4f480dd

Comment 34

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

Backout: this should have been wontfix for 35 because it needs a string not in 35.

https://hg.mozilla.org/releases/mozilla-beta/rev/9ad491807c82