1074694 - As a user, when I am alone in a room, I should be able to rename it

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: Expected UX

Comment 1

[Mark Banner (:standard8)]

Attachment: Allow rooms to be renamed from the conversation window.

This does the necessary work to get renames going. I'm tempted to say that the conversation window name should update straight away from the callback, rather than waiting for the push notification, but lets see how it looks when we get it on the real servers, and make a call then - its not difficult to change, although we'll still want the existing flows.

Comment 2

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

Comment on attachment 8522975 [details] [diff] [review]
Allow rooms to be renamed from the conversation window.

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

Looks great, r+ with comments addressed.

::: browser/components/loop/LoopRooms.jsm
@@ +365,5 @@
> +    let room = this.rooms.get(roomToken);
> +    let url = "/rooms/" + encodeURIComponent(roomToken);
> +
> +    let origRoom = this.rooms.get(roomToken);
> +    MozLoopService.log.error(origRoom);

This should be removed.

@@ +374,5 @@
> +      roomOwner: origRoom.roomOwner
> +    };
> +    MozLoopService.hawkRequest(this.sessionType, url, "PATCH", patchData)
> +      .then(response => {
> +        let data = JSON.parse(response.body);

This probably wants to be wrapped in a try/catch clause and callback(err) on parsing error.

@@ +375,5 @@
> +    };
> +    MozLoopService.hawkRequest(this.sessionType, url, "PATCH", patchData)
> +      .then(response => {
> +        let data = JSON.parse(response.body);
> +        extend(room, data);

Why not using Object.assign here? (this is a more general comment)

::: browser/components/loop/content/js/roomViews.jsx
@@ +66,5 @@
>      },
>  
>      getInitialState: function() {
>        return {
>          copiedUrl: false

Please add an initial state value for newRoomName set to "" (empty string), so we're explicit about available state properties.

@@ +75,5 @@
>        event.preventDefault();
> +
> +      var newRoomName = this.state.newRoomName;
> +
> +      if (newRoomName !== "" && this.state.roomName != newRoomName) {

Nit: if (newRoomName && this.state.roomName !== newRoomName)

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +199,1 @@
>              roomToken: actionData.roomToken,

Nit: Indentation.

@@ +234,4 @@
>      },
>  
>      /**
> +     * Handles the setupRoomInfo action. Updates the room data and

s/Updates the room data/Setups the initial room data/

::: browser/components/loop/content/shared/js/roomStore.js
@@ +414,5 @@
>        this._mozLoop.rooms.open(actionData.roomToken);
> +    },
> +
> +    /**
> +     * Renames a room

Nit: Missing dot.

@@ +423,5 @@
> +      this._mozLoop.rooms.rename(actionData.roomToken, actionData.newRoomName,
> +        function(err) {
> +          if (err) {
> +            console.error("Failed to rename the room", err);
> +          }

Please add a comment so we don't forget about adding visual notification for end-users (ideally with a bug number).

Comment 3

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

… and bug 1099085 landed, obviously.

Comment 4

[Mark Banner (:standard8)]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #2)
> > +    MozLoopService.hawkRequest(this.sessionType, url, "PATCH", patchData)
> > +      .then(response => {
> > +        let data = JSON.parse(response.body);
> 
> This probably wants to be wrapped in a try/catch clause and callback(err) on
> parsing error.

Not necessary, as there's a catch on the entire promise.

> @@ +375,5 @@
> > +    };
> > +    MozLoopService.hawkRequest(this.sessionType, url, "PATCH", patchData)
> > +      .then(response => {
> > +        let data = JSON.parse(response.body);
> > +        extend(room, data);
> 
> Why not using Object.assign here? (this is a more general comment)

Looks like that was introduced in FF 34, so probably a bit recent ;-) I'm going to suggest that as we use extend here like we do in other places, we keep it like this for now, and look at using Object.assign later.

Comment 5

[Mark Banner (:standard8)]

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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62da96758a24

Comment 7

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

Comment on attachment 8522975 [details] [diff] [review]
Allow rooms to be renamed from the conversation window.

Approval Request Comment

requirement for Loop Rooms support for Aurora/35
On m-c; testing with rooms server changes now live in production

[String/UUID change made/needed]: none

Comment 8

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

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

Comment 9

[u279076]

Assuming the provided test coverage is sufficient. Please need-info me to request further test coverage.