Closed Bug 1074694 Opened 5 years ago Closed 5 years ago

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

Categories

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

enhancement
Points:
5

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.3
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
Blocking Flags:
backlog Fx35+

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [rooms][strings])

User Story

* As a user, I should be able to rename a conversation, so that I can choose a name I like

UX:
* Implement a text field in the middle of the view, with placeholder "Name this conversation" (replaced original UX text "Enter room name")
* When focus is lost, the conversation name is updated

Strings:
* Name this conversation

Implementation
* When the room name is updated, do a PATCH /rooms/{token} request to the server
** include the conversation name
** expiresAt should be set to 8 hours

Attachments

(2 files)

Attached image Expected UX
No description provided.
User Story: (updated)
Whiteboard: [rooms] → [rooms][strings]
User Story: (updated)
backlog: --- → Fx35+
Priority: -- → P1
Assignee: nobody → standard8
Iteration: --- → 36.3
Points: --- → 5
Depends on: 1099085
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.
Attachment #8522975 - Flags: review?(nperriault)
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).
Attachment #8522975 - Flags: review?(nperriault) → review+
(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.
https://hg.mozilla.org/integration/fx-team/rev/62da96758a24
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/62da96758a24
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Attachment #8522975 - Flags: approval-mozilla-aurora?
Attachment #8522975 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assuming the provided test coverage is sufficient. Please need-info me to request further test coverage.
Flags: qe-verify-
Flags: in-testsuite+
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.