Closed Bug 1110155 Opened 5 years ago Closed 5 years ago

Loop desktop client should validate new room name on rename operation

Categories

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

defect
Points:
3

Tracking

(firefox35+ fixed, firefox36+ fixed, firefox37 fixed)

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

People

(Reporter: NiKo, Unassigned)

Details

Attachments

(3 files, 1 obsolete file)

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.
This would be great to fix and uplift.
backlog: --- → Fx35+
Priority: -- → P1
I still would very much like to fix this in Fx35, but we won't block release on this.
Priority: P1 → P2
Assignee: nobody → nperriault
Iteration: --- → 37.1
Points: --- → 3
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)
(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 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+
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?
[Tracking Requested - why for this release]:
See Comment 9
Hasn't landed yet.  SO "affected" for Fx37.  Once it lands, we'll verify it and then uplift if approved.
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?
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8540210 - Flags: review?(standard8) → review+
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
Attachment #8539258 - Flags: approval-mozilla-beta?
Attachment #8539258 - Flags: approval-mozilla-aurora?
Attachment #8540210 - Flags: approval-mozilla-release?
Attachment #8540210 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/facac3d68bd8
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Attachment #8539258 - Flags: approval-mozilla-beta?
Attachment #8539258 - Flags: approval-mozilla-beta+
Attachment #8539258 - Flags: approval-mozilla-aurora?
Attachment #8539258 - Flags: approval-mozilla-aurora+
Attachment #8540210 - Flags: approval-mozilla-release?
Attachment #8540210 - Flags: approval-mozilla-beta+
Attachment #8540210 - Flags: approval-mozilla-aurora?
Attachment #8540210 - Flags: approval-mozilla-aurora+
Flags: qe-verify-
Iteration: 37.1 → 37.2
You need to log in before you can comment on or make changes to this bug.