Closed Bug 1100595 Opened 5 years ago Closed 5 years ago

Add UI for indicating if renaming a room failed

Categories

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

defect
Points:
3

Tracking

(firefox35 wontfix, firefox36+ fixed, firefox37 fixed)

RESOLVED FIXED
mozilla37
Iteration:
37.2
Tracking Status
firefox35 --- wontfix
firefox36 + fixed
firefox37 --- fixed
Blocking Flags:
backlog Fx36+

People

(Reporter: standard8, Assigned: jaws)

References

Details

(Whiteboard: [rooms] [strings])

Attachments

(2 files, 5 obsolete files)

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.
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?
backlog: --- → Fx36+
Flags: needinfo?(sfranks)
Flags: needinfo?(dhenein)
Flags: needinfo?(Mnovak)
Priority: -- → P1
ok - i lied.  need strings and a basic UX ASAP so we can get them in this week or it's 37.
I would say something like: 

"Conversation cannot be renamed"

Would that work?
Flags: needinfo?(Mnovak)
Whiteboard: [rooms] [strings]
sounds good Matej on the error words, thank you. 

Hi Darrin or Sevaan, in put on the UX for the Error we should use?
Marco, can you please add this to the 37.1 iteration tracking? Thanks!
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 37.1
Points: --- → 2
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Added to IT 37.1
Flags: needinfo?(mmucci)
Flags: qe-verify?
Flags: needinfo?(dhenein)
Depends on: 1105537
[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.
In what cases would renaming a room fail? Would it be possible for the user have the option to retry?
Attached image Room Rename Error (obsolete) —
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?
Flags: needinfo?(sfranks)
Attachment #8533255 - Flags: review?(Mnovak)
(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.
Thanks Matej. Updated.
Attachment #8533255 - Attachment is obsolete: true
Attachment #8533255 - Flags: review?(Mnovak)
Attachment #8533288 - Flags: review?(standard8)
Tracking. Please land that during the aurora cycle.
Iteration: 37.1 → 37.2
Looks fine to me.
Attachment #8533288 - Flags: review?(standard8) → review+
We also allow renaming a room from the panel.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8534672 - Flags: review?(standard8)
Comment on attachment 8534672 [details] [diff] [review]
Patch

I'm afk tommorow & friday, so redirecting review.
Attachment #8534672 - Flags: review?(standard8) → review?(nperriault)
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?
Attachment #8534672 - Flags: review?(nperriault) → review-
Attached patch Patch for uplift (obsolete) — Splinter Review
Addressed feedback, added test, and using preexisting string for uplift.
Attachment #8534672 - Attachment is obsolete: true
Attachment #8535716 - Flags: review?(nperriault)
Attached patch Patch for m-c (obsolete) — Splinter Review
Attachment #8535720 - Flags: review?(nperriault)
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!
Flags: needinfo?(standard8)
Flags: needinfo?(jaws)
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.
Attachment #8535716 - Flags: review?(nperriault) → review-
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 :)
Attachment #8535720 - Flags: review?(nperriault) → review-
Flags: needinfo?(standard8)
Attached patch Patch v1.1 (obsolete) — Splinter Review
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)"
Attachment #8535716 - Attachment is obsolete: true
Attachment #8535720 - Attachment is obsolete: true
Flags: needinfo?(jaws)
Attachment #8536715 - Flags: review?(nperriault)
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).
Attachment #8536715 - Flags: review?(nperriault) → review-
Attached patch Patch v1.2Splinter Review
The string in the properties file should have been removed, it was leftover that I missed in the previous patch.
Attachment #8536715 - Attachment is obsolete: true
Attachment #8536796 - Flags: review?(nperriault)
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.
Attachment #8536796 - Flags: review?(nperriault) → review+
(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.
(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).
https://hg.mozilla.org/mozilla-central/rev/95cb099f4675
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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
Attachment #8536796 - Flags: approval-mozilla-beta?
Attachment #8536796 - Flags: approval-mozilla-aurora?
Attachment #8536796 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8536796 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
You need to log in before you can comment on or make changes to this bug.