-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor the way of displaying errors in several forms #15978
Merged
neil-marcellini
merged 13 commits into
Expensify:main
from
TMisiukiewicz:display-multiple-error-messages
Mar 22, 2023
Merged
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
c8d7d89
use `addErrorMessage` util in forms
TMisiukiewicz 7af8033
remove non-necessary usage of error util
TMisiukiewicz b3cf4cf
code review updates
TMisiukiewicz 7966ed4
add tests
TMisiukiewicz 9aace99
add tests
TMisiukiewicz f3dd12f
update validation for report settings
TMisiukiewicz 1b47492
remove unnecessary error message
TMisiukiewicz 2fe536a
update input name
TMisiukiewicz c3eec81
apply requested changes
koko57 b019c33
indentation fix
koko57 cbeee8b
change the roomNameReservedError message
koko57 6967078
Merge branch 'main' into display-multiple-error-messages
koko57 99fa02b
do not display 2 error messages for dob
koko57 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
neil-marcellini marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| import * as ErrorUtils from '../../src/libs/ErrorUtils'; | ||
|
|
||
| describe('ErrorUtils', () => { | ||
| test('should add a new error message for a given inputID', () => { | ||
| const errors = {}; | ||
| ErrorUtils.addErrorMessage(errors, 'username', 'Username cannot be empty'); | ||
|
|
||
| expect(errors).toEqual({username: 'Username cannot be empty'}); | ||
| }); | ||
|
|
||
| test('should append an error message to an existing error message for a given inputID', () => { | ||
| const errors = {username: 'Username cannot be empty'}; | ||
| ErrorUtils.addErrorMessage(errors, 'username', 'Username must be at least 6 characters long'); | ||
|
|
||
| expect(errors).toEqual({username: 'Username cannot be empty\nUsername must be at least 6 characters long'}); | ||
| }); | ||
|
|
||
| test('should add an error to input which does not contain any errors yet', () => { | ||
| const errors = {username: 'Username cannot be empty'}; | ||
| ErrorUtils.addErrorMessage(errors, 'password', 'Password cannot be empty'); | ||
|
|
||
| expect(errors).toEqual({username: 'Username cannot be empty', password: 'Password cannot be empty'}); | ||
| }); | ||
|
|
||
| test('should not mutate the errors object when message is empty', () => { | ||
| const errors = {username: 'Username cannot be empty'}; | ||
| ErrorUtils.addErrorMessage(errors, 'username', ''); | ||
|
|
||
| expect(errors).toEqual({username: 'Username cannot be empty'}); | ||
| }); | ||
|
|
||
| test('should not mutate the errors object when inputID is null', () => { | ||
| const errors = {username: 'Username cannot be empty'}; | ||
| ErrorUtils.addErrorMessage(errors, null, 'InputID cannot be null'); | ||
|
|
||
| expect(errors).toEqual({username: 'Username cannot be empty'}); | ||
| }); | ||
|
|
||
| test('should not mutate the errors object when message is null', () => { | ||
| const errors = {username: 'Username cannot be empty'}; | ||
| ErrorUtils.addErrorMessage(errors, 'username', null); | ||
|
|
||
| expect(errors).toEqual({username: 'Username cannot be empty'}); | ||
| }); | ||
|
|
||
| test('should add multiple error messages for the same inputID', () => { | ||
| const errors = {}; | ||
| ErrorUtils.addErrorMessage(errors, 'username', 'Username cannot be empty'); | ||
| ErrorUtils.addErrorMessage(errors, 'username', 'Username must be at least 6 characters long'); | ||
| ErrorUtils.addErrorMessage(errors, 'username', 'Username must contain at least one letter'); | ||
|
|
||
| expect(errors).toEqual({username: 'Username cannot be empty\nUsername must be at least 6 characters long\nUsername must contain at least one letter'}); | ||
| }); | ||
|
|
||
| test('should append multiple error messages to an existing error message for the same inputID', () => { | ||
| const errors = {username: 'Username cannot be empty\nUsername must be at least 6 characters long'}; | ||
| ErrorUtils.addErrorMessage(errors, 'username', 'Username must contain at least one letter'); | ||
| ErrorUtils.addErrorMessage(errors, 'username', 'Username must not contain special characters'); | ||
TMisiukiewicz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| expect(errors).toEqual( | ||
| {username: 'Username cannot be empty\nUsername must be at least 6 characters long\nUsername must contain at least one letter\nUsername must not contain special characters'}, | ||
| ); | ||
| }); | ||
| }); | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed your suggestion on reverting to two separate errors for restricted and existing room names and changed the roomNameReservedError message a little bit - I didn’t use the same pattern as in name’s containsReservedWord error message, because here we are checking for the exact #admins and #announce names and we are able to create a name including both words like e.g. #ck-admins. (Or maybe we should change the validation here?) If it doesn’t sound ok or you have an idea for a different message, please let me know, I’ll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks pretty good. From what I read in Slack I think from we want an error message for each default room name. We can use a translation function like this to achieve that.
App/src/languages/en.js
Line 64 in bb8dff1
#admins is a default room on all workspaces. Please choose another name.#announce is a default room on all workspaces. Please choose another name.