Closed
Bug 1110155
Opened 9 years ago
Closed 9 years ago
Loop desktop client should validate new room name on rename operation
Categories
(Hello (Loop) :: Client, defect, P2)
Hello (Loop)
Client
Tracking
(firefox35+ fixed, firefox36+ fixed, firefox37 fixed)
backlog | Fx35+ |
People
(Reporter: NiKo, Unassigned)
Details
Attachments
(3 files, 1 obsolete file)
167.46 KB,
image/png
|
Details | |
6.29 KB,
patch
|
mikedeboer
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
3.22 KB,
patch
|
standard8
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Caught while reviewing bug 1100595, it's actually possible to rename a room using only spaces/invisible characters, like " " (three spaces). This results in a confusing display (see attached capture). We should probably trim entered string and raise an error if no visible character remains.
Assignee | ||
Comment 1•9 years ago
|
||
Comment 3•9 years ago
|
||
I still would very much like to fix this in Fx35, but we won't block release on this.
Priority: P1 → P2
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → nperriault
Iteration: --- → 37.1
Points: --- → 3
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8537818 -
Flags: review?(standard8)
Comment 5•9 years ago
|
||
Comment on attachment 8537818 [details] [diff] [review] Added minimal Loop room name validation. Review of attachment 8537818 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/content/js/roomViews.jsx @@ +94,5 @@ > handleFormSubmit: function(event) { > event.preventDefault(); > > + // If name wasn't updated, skipping. > + if (this.state.roomName === newRoomName) { How about we do this and the trim in RoomStore#RenameRoom - that way, we can fix the panel and the conversation window at the same time?
Attachment #8537818 -
Flags: review?(standard8)
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #5) > How about we do this and the trim in RoomStore#RenameRoom - that way, we can > fix the panel and the conversation window at the same time? Good idea, updated patch.
Attachment #8537818 -
Attachment is obsolete: true
Attachment #8539258 -
Flags: review?(mdeboer)
Comment 7•9 years ago
|
||
Comment on attachment 8539258 [details] [diff] [review] Added minimal Loop room name validation. Review of attachment 8539258 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: browser/components/loop/content/js/roomViews.jsx @@ +97,5 @@ > + this.props.dispatcher.dispatch( > + new sharedActions.RenameRoom({ > + roomToken: this.state.roomToken, > + newRoomName: this.state.newRoomName > + })); nit: please align the last closing brace below. ::: browser/components/loop/content/shared/js/roomStore.js @@ +425,5 @@ > + return; > + } > + > + // Ensure we submit a valid room name. > + var newRoomName = actionData.newRoomName.trim(); Please move this all the way up, and change the if statement to `if (!newRoomName || this.getStoreState("roomName") === newRoomName) {`
Attachment #8539258 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d9029ab107a8
Target Milestone: --- → mozilla37
Comment 9•9 years ago
|
||
Comment on attachment 8539258 [details] [diff] [review] Added minimal Loop room name validation. Approval Request Comment [Feature/regressing bug #]: Room name validation [User impact if declined]: Without this, the user could enter a room name that was confusing or problematic. This provides minimal validation that the room name will work in Hello [Describe test coverage new/current, TBPL]: tbpl, manual testing [Risks and why]: minimal risk to Hello, no risk outside of Hello [String/UUID change made/needed]: no strings
Attachment #8539258 -
Flags: approval-mozilla-beta?
Attachment #8539258 -
Flags: approval-mozilla-aurora?
Comment 10•9 years ago
|
||
[Tracking Requested - why for this release]: See Comment 9
status-firefox35:
--- → affected
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
tracking-firefox35:
--- → ?
tracking-firefox36:
--- → ?
Comment 11•9 years ago
|
||
Hasn't landed yet. SO "affected" for Fx37. Once it lands, we'll verify it and then uplift if approved.
https://hg.mozilla.org/mozilla-central/rev/d9029ab107a8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Updated•9 years ago
|
Comment 13•9 years ago
|
||
Comment on attachment 8539258 [details] [diff] [review] Added minimal Loop room name validation. Pulling approval requests until we have a verified fix
Attachment #8539258 -
Flags: approval-mozilla-beta?
Attachment #8539258 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 14•9 years ago
|
||
This is a followup from the previous patch, preventing the EditInPlace component to update from an invalid value during a rename operation.
Attachment #8540210 -
Flags: review?(standard8)
Assignee | ||
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•9 years ago
|
Attachment #8540210 -
Flags: review?(standard8) → review+
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/facac3d68bd8
Comment 16•9 years ago
|
||
Comment on attachment 8539258 [details] [diff] [review] Added minimal Loop room name validation. For both patches: Approval Request Comment [Feature/regressing bug #]: Room name validation [User impact if declined]: Without this, the user could enter a room name that was confusing or problematic. This provides minimal validation that the room name will work in Hello [Describe test coverage new/current, TBPL]: tbpl, manual testing (which was how we caught the need for the followup) [Risks and why]: minimal risk to Hello, no risk outside of Hello [String/UUID change made/needed]: no strings
Updated•9 years ago
|
Attachment #8539258 -
Flags: approval-mozilla-beta?
Attachment #8539258 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Attachment #8540210 -
Flags: approval-mozilla-release?
Attachment #8540210 -
Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/facac3d68bd8
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Updated•9 years ago
|
Attachment #8539258 -
Flags: approval-mozilla-beta?
Attachment #8539258 -
Flags: approval-mozilla-beta+
Attachment #8539258 -
Flags: approval-mozilla-aurora?
Attachment #8539258 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8540210 -
Flags: approval-mozilla-release?
Attachment #8540210 -
Flags: approval-mozilla-beta+
Attachment #8540210 -
Flags: approval-mozilla-aurora?
Attachment #8540210 -
Flags: approval-mozilla-aurora+
Comment 18•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/901b7325b2c8 https://hg.mozilla.org/releases/mozilla-aurora/rev/73276b690a0e
Comment 19•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/1aa585c2fdee https://hg.mozilla.org/releases/mozilla-beta/rev/d4fcb3badc01
Updated•9 years ago
|
Updated•9 years ago
|
Iteration: 37.1 → 37.2
You need to log in
before you can comment on or make changes to this bug.
Description
•