fixed new room page form validation #7098

ABee-Tech · 2022-01-09T08:17:54Z

Details

Explained over here: #6967 (comment)

Fixed Issues

$ #6967

Tests

Put empty room name
Existing room name check

QA Steps

Open app
Create Workspace
Select "+" > New room
Enter any name and delete
Hit Create room with empty room name
User will not be able to create room with blank room name

Tested On

Screenshots

Android

Proposal.-.1.mp4

Proposal.-.2.mp4

Screenshots after all changes

Web

testing-on-web4.mp4

Android

testing-on-android.mp4

ABee-Tech · 2022-01-09T08:26:48Z

I think we should create new statements for:
i. Please enter a room name.
i. Please select a workspace.
ii. Please select a visibility.

I need help on where to define these statements on src/languages/en.js.

src/pages/workspace/WorkspaceNewRoomPage.js

ABee-Tech · 2022-01-09T15:18:13Z

@parasharrajat Thank you for the suggestions. I will update them. can you tell me why is Jest Unit Tests is failing on something called Decrypt OSBotify GPG Key?

parasharrajat · 2022-01-09T15:20:16Z

That is an internal issue and soon be fixed.

ABee-Tech · 2022-01-10T09:41:57Z

And what about translation of the sentence to Español language? @parasharrajat

ABee-Tech · 2022-01-10T10:03:52Z

For translation to Español language I have left empty string on es.js file.

src/pages/workspace/WorkspaceNewRoomPage.js

src/languages/en.js

marcaaron

Nice job so far! Left a few style reminders. Mostly looking good here.

src/pages/workspace/WorkspaceNewRoomPage.js

ABee-Tech · 2022-01-11T11:32:10Z

@parasharrajat @marcaaron I have done changes as per reviews. You can review the changes.

parasharrajat

looks good. only the translation part is remaining.

src/CONST.js

src/languages/en.js

src/languages/es.js

ABee-Tech · 2022-01-13T12:48:10Z

I have changed select to choose and finally added es translations for those error statements. I think everything is fine now.

src/CONST.js

src/languages/es.js

src/languages/en.js

src/languages/es.js

src/pages/workspace/WorkspaceNewRoomPage.js

marcaaron · 2022-01-13T17:16:43Z

Found a few more things to fix. Also there are conflicts that will need to be fixed before we can merge.

ABee-Tech · 2022-01-18T07:35:53Z

@parasharrajat @marcaaron I have fixed all the review changes. Now, I think we should resolve conflicts first before merge. But, help me on resolving those conflicts.

parasharrajat · 2022-01-18T08:52:41Z

@ABee-Tech Please merge Main into your branch and resolvee the conflicts if any. and let me know when this is ready.

parasharrajat

Looks good. Please resolve the conflcits. thanks.

src/pages/workspace/WorkspaceNewRoomPage.js

ABee-Tech · 2022-01-18T13:46:18Z

@parasharrajat @marcaaron should we use RoomNameInput component used on line 203 of WorkspaceNewRoomPage.js or not?

parasharrajat · 2022-02-13T16:16:31Z

The slack post didn't get much attention but it seems we can proceed with the changes.

ABee-Tech · 2022-02-14T04:47:14Z

Okay then.

…to fix-empty-room-name

parasharrajat

Thanks for the changes. I noticed some changed code in your PR. Please make sure that you don't break the fix #7661.

Please adjust the code so that 7661 still works on your changes. And It would be great if you can mark old comments resolved. Thanks.

Modify your screenshots to reflect both changes. new Room page and Room Settings page in the single video for each platform. You can leave Desktop and IOS, I will test that as you don't have access to it.

ABee-Tech · 2022-02-15T14:44:09Z

@parasharrajat Everything is fine. There is no issue prevailing like on the issue of the PR #7661. There is no need for change I think. That works fine. I will attach screenshots for Android and Web including that.

ABee-Tech · 2022-02-15T15:34:23Z

Ohh sorry. I checked on android. It was fine. But, on web the issue exists. I will adjust the code and then upload screenshots.

ABee-Tech · 2022-02-17T05:09:06Z

@parasharrajat I have adjusted the code and tested on web and android. I have uploaded screenshots above.

parasharrajat

Thanks, @ABee-Tech. One more small tweak.

Please tick all platforms.

src/pages/ReportSettingsPage.js

src/components/RoomNameInput.js

parasharrajat

One change and we should be good.

src/pages/ReportSettingsPage.js

ABee-Tech · 2022-02-20T08:33:29Z

@parasharrajat I am having trouble running web version. Can we consider last screenshot just without growl message?

parasharrajat

LGTM. Thanks for the changes @ABee-Tech

cc: @marcaaron

🎀 👀 🎀 C+ reviewed

ABee-Tech · 2022-02-21T05:31:06Z

Android screenshot updated.

marcaaron · 2022-02-22T22:31:35Z

src/libs/ValidationUtils.js

+ * component because then we are not changing the room's name
+ *
+ * @param {String} roomName
+ * @param {Object[]} reports


Suggested change

* @param {Object[]} reports

* @param {Object} reports

marcaaron · 2022-02-22T22:32:19Z

src/libs/ValidationUtils.js

+ */
+function isExistingRoomName(roomName, reports, policyID) {
+    return _.some(
+        _.values(reports),


_.some() should work on object collections so no need to use _.values() here

https://underscorejs.org/#some

marcaaron · 2022-02-22T22:34:52Z

src/pages/ReportSettingsPage.js

                                isLoading={this.props.isLoadingRenamePolicyRoom}
+                                isDisabled={shouldDisableRename}


NAB (as we are conducting an audit of all forms now) but no form buttons should be disabled

I think this needs to be disabled for this case when user is seeing settings for default policies #admins, etc. I haven't seen the new Form style draft so I can't think of anything apart from it.

We'll release the guidelines soon. Basically, no forms should be disabled for any reason.

Ok. Thanks. Also, the input is read-only here so I think disabling the button is justified. Another approach could be to hide the Rename form for default policies.

marcaaron · 2022-02-22T22:40:54Z

src/pages/workspace/WorkspaceNewRoomPage.js

+        }
+
+        // We error if the room name already exists. We don't care if it matches the original name provided in this
+        // component because then we are not changing the room's name.


I don't really understand the comment. In particular...

We don't care if it matches the original name provided in this component because then we are not changing the room's name.

What does this have to do with the check we make? How is it determined that the name in state matches the "original name provided in this component"? Which variable represents that "original name"? Can we be more specific or is the comment out of context?

@marcaaron Yes actually it is out of context for this page so we should remove this and we can be more specific on same comment on ReportSettingsPage by just modifing it as We don't error if the room name matches same as previous.

ABee-Tech · 2022-02-23T05:06:25Z

@marcaaron Please review the changes.

OSBotify · 2022-02-23T17:56:04Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-02-23T21:58:51Z

🚀 Deployed to staging by @marcaaron in version: 1.1.40-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-03-01T18:49:10Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.40-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Stutikuls · 2022-03-08T07:49:29Z

Issue 1 -

Title- [Medium] Chrome +Jaws : 'Room Name' text edit field is not marked as required and announced.
Actual - The fields 'Room name' text field is required to be filled but is not announced as required and no visual "" sign for required field.
Expected -If the form element is required, it should be announced as such for the screen reader users and visual "" sign for required field should be there.
WCAG failure - 1.3.1
Reproducible in staging?: - Yes
Version Number: - v1.1.40-2
Platforms - Web (Chrome + Jaws), Desktop (MAC), iOS(Voiceover), Mobile - web(Safari + Voiceover), Android (Talkback)

7098_.Room.Name.text.edit.field.is.not.marked.as.required.and.announced.mp4

Issue 2 -

Title-[Medium] Chrome + Jaws: Screen reader : Error message is not announced by the screen reader.
Actual - Screen reader does not announce any inline error message appear below to text boxes.
Expected - Inline error message should be conveyed to the users as soon as it appear on the fields so that user can resolve the error.
WCAG failure - 3.3.1
Reproducible in staging?: - Yes
Version Number: - v1.1.40-2
Platforms - Web(Chrome + Jaws), Desktop(MAC), Mobile-web (Safari + Voiceover), iOS(Voiceover), Android (Talkback)

7098_Error.message.is.not.announced.by.the.screen.reader.mp4

Issue 3 -

Title-[Medium] Chrome + Jaws: Screen reader : Screen reader is not provide any information after creating the room.
Actual - Screen reader does not announce any information after creating the room, screen reader is reading some extra hidden information.
Expected - After creating the room, focus should lands on the name of the room(new) and should read the new created room name.
WCAG failure - 1.3.1
Reproducible in staging?: - Yes
Version Number: - v1.1.40-2
Platforms - Web(Chrome + Jaws), Desktop(MAC), Mobile-web (Safari + Voiceover), iOS(Voice over), Android (Talkback)

7098_N0t.provide.any.information.after.creating.the.room.mp4

Issue 4 -

Title-[Medium] Safari+ Voiceover : Screen reader : Screen reader is not reading role for 'create room' control.
Actual - Screen reader only announce "create room".
Note - On Mac reading "create room Group" when focus lands on the create room control.
Expected - Role should define as 'Button' and screen reader should read like "Create room button.
WCAG failure - 4.1.2
Reproducible in staging?: - Yes
Version Number: - v1.1.40-2
Platforms - Desktop(MAC), Mobile-web (Safari + Voiceover)

7098_Not.reading.the.role.for.Create.room.control.mp4

fixed new room page form validation

53d88f6

ABee-Tech requested a review from a team as a code owner January 9, 2022 08:17

MelvinBot requested review from marcaaron and parasharrajat and removed request for a team January 9, 2022 08:17

lint fix

846a446

parasharrajat suggested changes Jan 9, 2022

View reviewed changes

refactor and translations

a09e43d

parasharrajat suggested changes Jan 10, 2022

View reviewed changes

marcaaron requested changes Jan 11, 2022

View reviewed changes

ABee-Tech added 2 commits January 11, 2022 17:01

added ROOM_PREFIX into CONST.js

40730c4

review changes

bfc0b49

parasharrajat suggested changes Jan 11, 2022

View reviewed changes

src/CONST.js Outdated Show resolved Hide resolved

src/languages/en.js Outdated Show resolved Hide resolved

src/languages/es.js Outdated Show resolved Hide resolved

es translations and constant

f212fa4

parasharrajat suggested changes Jan 13, 2022

View reviewed changes

src/CONST.js Outdated Show resolved Hide resolved

src/languages/es.js Outdated Show resolved Hide resolved

marcaaron requested changes Jan 13, 2022

View reviewed changes

review changes fixed

7b9dd79

parasharrajat suggested changes Jan 18, 2022

View reviewed changes

src/pages/workspace/WorkspaceNewRoomPage.js Show resolved Hide resolved

src/pages/workspace/WorkspaceNewRoomPage.js Outdated Show resolved Hide resolved

validateAndCreatePolicyRoom binded

9355c74

no action on same name as previous

c0e595c

ABee-Tech dismissed parasharrajat’s stale review via c0e595c February 14, 2022 16:29

ABee-Tech added 2 commits February 15, 2022 09:48

Merge branch 'main' of https://github.com/ABee-Tech/Expensify.cash in…

3ddfae9

…to fix-empty-room-name

lint fix

474d1b0

parasharrajat suggested changes Feb 15, 2022

View reviewed changes

event handled for cursor movement fix

62fa9e6

parasharrajat suggested changes Feb 17, 2022

View reviewed changes

src/pages/ReportSettingsPage.js Outdated Show resolved Hide resolved

src/components/RoomNameInput.js Outdated Show resolved Hide resolved

ABee-Tech added 2 commits February 18, 2022 19:34

suggested changes

1edf336

lint fix and added comments

d27b049

parasharrajat suggested changes Feb 18, 2022

View reviewed changes

src/pages/ReportSettingsPage.js Outdated Show resolved Hide resolved

moved validate block above growl

faee357

parasharrajat previously approved these changes Feb 20, 2022

View reviewed changes

marcaaron requested changes Feb 22, 2022

View reviewed changes

changes in comments and minor fixes

21cd056

ABee-Tech dismissed parasharrajat’s stale review via 21cd056 February 23, 2022 04:59

marcaaron approved these changes Feb 23, 2022

View reviewed changes

marcaaron merged commit 2f14cb7 into Expensify:main Feb 23, 2022

OSBotify mentioned this pull request Feb 23, 2022

Deploy Checklist: New Expensify 2022-02-23 #7883

Closed

69 tasks

		isLoading={this.props.isLoadingRenamePolicyRoom}
		isDisabled={shouldDisableRename}

fixed new room page form validation #7098

fixed new room page form validation #7098

Conversation

ABee-Tech commented Jan 9, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Android

Screenshots after all changes

Web

Android

ABee-Tech commented Jan 9, 2022

ABee-Tech commented Jan 9, 2022 • edited Loading

parasharrajat commented Jan 9, 2022

ABee-Tech commented Jan 10, 2022

ABee-Tech commented Jan 10, 2022

marcaaron left a comment

Choose a reason for hiding this comment

ABee-Tech commented Jan 11, 2022

parasharrajat left a comment

Choose a reason for hiding this comment

ABee-Tech commented Jan 13, 2022

marcaaron commented Jan 13, 2022

ABee-Tech commented Jan 18, 2022

parasharrajat commented Jan 18, 2022

parasharrajat left a comment

Choose a reason for hiding this comment

ABee-Tech commented Jan 18, 2022

parasharrajat commented Feb 13, 2022

ABee-Tech commented Feb 14, 2022

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

ABee-Tech commented Feb 15, 2022 • edited Loading

ABee-Tech commented Feb 15, 2022 • edited Loading

ABee-Tech commented Feb 17, 2022

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

parasharrajat left a comment

Choose a reason for hiding this comment

ABee-Tech commented Feb 20, 2022

parasharrajat left a comment

Choose a reason for hiding this comment

ABee-Tech commented Feb 21, 2022

marcaaron Feb 22, 2022

Choose a reason for hiding this comment

marcaaron Feb 22, 2022

Choose a reason for hiding this comment

marcaaron Feb 22, 2022

Choose a reason for hiding this comment

parasharrajat Feb 22, 2022

Choose a reason for hiding this comment

marcaaron Feb 23, 2022

Choose a reason for hiding this comment

parasharrajat Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

marcaaron Feb 22, 2022

Choose a reason for hiding this comment

ABee-Tech Feb 23, 2022

Choose a reason for hiding this comment

ABee-Tech commented Feb 23, 2022

OSBotify commented Feb 23, 2022

OSBotify commented Feb 23, 2022

OSBotify commented Mar 1, 2022

Stutikuls commented Mar 8, 2022 • edited Loading

ABee-Tech commented Jan 9, 2022 •

edited

Loading

ABee-Tech commented Jan 9, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

ABee-Tech commented Feb 15, 2022 •

edited

Loading

ABee-Tech commented Feb 15, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat Feb 23, 2022 •

edited

Loading

Stutikuls commented Mar 8, 2022 •

edited

Loading