Closed
Bug 1074694
Opened 11 years ago
Closed 10 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)
Hello (Loop)
Client
Tracking
(firefox35 fixed, firefox36 fixed)
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)
239.95 KB,
image/png
|
Details | |
26.88 KB,
patch
|
NiKo
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•11 years ago
|
User Story: (updated)
Whiteboard: [rooms] → [rooms][strings]
Updated•11 years ago
|
User Story: (updated)
Updated•11 years ago
|
backlog: --- → Fx35+
Updated•10 years ago
|
Priority: -- → P1
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → standard8
Iteration: --- → 36.3
Points: --- → 5
Assignee | ||
Comment 1•10 years ago
|
||
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+
… and bug 1099085 landed, obviously.
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
Target Milestone: --- → mozilla36
Comment 6•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 7•10 years ago
|
||
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?
Updated•10 years ago
|
status-firefox35:
--- → affected
status-firefox36:
--- → fixed
Updated•10 years ago
|
Attachment #8522975 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 8•10 years ago
|
||
Updated•10 years ago
|
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.
Description
•