1110155 - Loop desktop client should validate new room name on rename operation

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

Attachment: Confusing display when renaming a room using invisible characters.

Comment 2

[Maire Reavy [:mreavy]]

This would be great to fix and uplift.

Comment 3

[Maire Reavy [:mreavy]]

I still would very much like to fix this in Fx35, but we won't block release on this.

Comment 4

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

Attachment: Added minimal Loop room name validation.

Comment 5

[Mark Banner (:standard8)]

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?

Comment 6

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

Attachment: Added minimal Loop room name validation.

(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.

Comment 7

[Mike de Boer [:mikedeboer]]

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) {`

Comment 8

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

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

Comment 9

[Maire Reavy [:mreavy]]

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

Comment 10

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:
See Comment 9

Comment 11

[Maire Reavy [:mreavy]]

Hasn't landed yet.  SO "affected" for Fx37.  Once it lands, we'll verify it and then uplift if approved.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d9029ab107a8

Comment 13

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

Comment on attachment 8539258 [details] [diff] [review]
Added minimal Loop room name validation.

Pulling approval requests until we have a verified fix

Comment 14

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

Attachment: Fixed renamed Loop room not reflected into the panel.

This is a followup from the previous patch, preventing the EditInPlace component to update from an invalid value during a rename operation.

Comment 15

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

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

Comment 16

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

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/facac3d68bd8

Comment 18

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/901b7325b2c8
https://hg.mozilla.org/releases/mozilla-aurora/rev/73276b690a0e

Comment 19

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

https://hg.mozilla.org/releases/mozilla-beta/rev/1aa585c2fdee
https://hg.mozilla.org/releases/mozilla-beta/rev/d4fcb3badc01