Support displaying backend errors in Form by youssef-lr · Pull Request #11465 · Expensify/App

youssef-lr · 2022-09-30T02:25:49Z

Details

This PR adds support for displaying errorFields coming from the backend. Previously we could only display a backend error in the Onyx key errors above the submit buttons. This will enable us to display backend errors under the relevant inputs.

Fixed Issues

$ #11267

Tests

Regression test: let's test one of the forms that uses Form and make sure everything works properly.

Go to Settings => Payments => Add a debit card.
Test that form error handling works well:
- Leaving an input empty and focusing somewhere else should show an error.
- Fill in an input, then clear it, make sure an error is displayed.
- Clicking on 'fix the errors' button should focus on the first input with an error.
- Entering valid information clears the error.
- Submitting the valid form sends a request to the server. (No need to add an actual card, just make sure the request is sent)

Testing error displayed under a field with a backend error (must be tested with https://github.com/Expensify/Web-Expensify/pull/35548)

Edit a Workspace name to <b></b> and click Save.
Make sure the error is shown as in the screenshot below:

3. Clicking on 'fix the errors' should focus on the input of the name of the workspace.

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Go to Settings => Payments => Add a debit card.
Test that form error handling works well:
- Leaving an input empty and focusing somewhere else should show an error.
- Fill in an input, then clear it, make sure an error is displayed.
- Clicking on 'fix the errors' button should focus on the first input with an error.
- Entering valid information clears the error.
- Submitting the valid form sends a request to the server. (No need to add an actual card, just make sure the request is sent)

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

melvin-bot · 2022-09-30T02:25:52Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

youssef-lr · 2022-09-30T18:16:40Z

src/components/Form.js

                            isAlertVisible={_.size(this.state.errors) > 0 || Boolean(this.props.formState.error)}
                            isLoading={this.props.formState.isLoading}
-                            message={this.props.formState.error}
+                            message={_.isEmpty(this.props.formState.errorFields) ? this.getErrorMessage() : null}


I figured I should add this check here to avoid displaying a duplicate message on both the error field above the submit button and below the errored input. Instead, we will display the fixTheErrors message and the field error message below the input.

deetergp · 2022-09-30T19:37:20Z

@youssef-lr What are the testing steps? If not having JS Console errors is the only test, this will probably never pass local testing since there is always SOME kind of JS console error going on in local dev 😅

youssef-lr · 2022-09-30T19:53:09Z

Hey Scott! I just added [WIP] to the title, I'm waiting for feedback on the linked issue before I can finalize this PR and then I'll add tests.

deetergp · 2022-11-10T18:17:43Z

Hey @youssef-lr, I am going to drop myself from this PR so it isn't beaming its angry red text at me in my K2. Feel free to add me back when it's no longer an WIP and ready for review.

youssef-lr · 2022-11-10T18:18:29Z

No worries @deetergp!

youssef-lr · 2022-11-16T12:41:01Z

hey @ctkochan22 - I hope you don't mind reviewing this and testing it along with your Web-E PR! I didn't know what to put in the test steps as we're not actively using errorFields in forms in the backend yet. (That is forms that are handled by Form.js in App)

ctkochan22 · 2022-11-16T20:09:16Z

Yeah there doesn't seem to be a way to test for now. We should simply do regression testing.

Can you add regression testing? Like pick a couple forms, and make sure error validations show up normally?

ctkochan22 · 2022-11-16T20:11:16Z

We shoudl update the .md like botify mentions

ctkochan22 · 2022-11-21T21:40:12Z

I'll test and merge, but please also move regression testing to QA so they'll have a chance to test it again

youssef-lr · 2022-11-21T22:04:17Z

Done @ctkochan22.

ctkochan22 · 2022-11-21T22:11:27Z

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

ctkochan22

Please add clear QA regression tests as well before this gets deployed please!

melvin-bot · 2022-11-21T22:12:29Z

@ctkochan22 looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

ctkochan22 · 2022-11-21T22:12:39Z

Checklist filled out

OSBotify · 2022-11-21T22:17:10Z

✋ 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-11-22T16:36:03Z

🚀 Deployed to staging by @ctkochan22 in version: 1.2.30-0 🚀

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

OSBotify · 2022-11-22T17:45:47Z

🚀 Deployed to staging by @ctkochan22 in version: 1.2.30-0 🚀

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

OSBotify · 2022-11-24T00:04:09Z

🚀 Deployed to production by @luacmartins in version: 1.2.30-0 🚀

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

marcaaron · 2022-12-30T06:55:27Z

src/components/Form.js

        return this.props.formState.error || (typeof latestErrorMessage === 'string' ? latestErrorMessage : '');
    }

+    getFirstErroredInput() {


Just come across this randomly but this is missing a JSDoc and should have an @returns

marcaaron · 2022-12-30T06:56:32Z

src/components/Form.js

+            return;
+        }
+
+        return _.first(_.keys(hasStateErrors ? this.state.erorrs : this.props.formState.errorFields));


this.state.erorrs is a typo ?

youssef-lr added 3 commits September 30, 2022 03:20

Add support for errors and errorFields in Form

6ca0473

Add prop to hide the submit button in Form

6f147a1

Clarify comment

69eca30

youssef-lr changed the title ~~Youssef refactor form~~ Support recent Onyx error keys in Form, and add option to hide the submit button Sep 30, 2022

Fix param type

b64bd64

youssef-lr marked this pull request as ready for review September 30, 2022 17:37

youssef-lr requested a review from a team as a code owner September 30, 2022 17:37

melvin-bot bot requested review from deetergp and removed request for a team September 30, 2022 17:38

youssef-lr commented Sep 30, 2022

View reviewed changes

youssef-lr changed the title ~~Support recent Onyx error keys in Form, and add option to hide the submit button~~ [WIP] Support recent Onyx error keys in Form, and add option to hide the submit button Sep 30, 2022

deetergp removed their request for review November 10, 2022 18:26

ctkochan22 mentioned this pull request Nov 11, 2022

[HOLD for payment 2023-02-08] [$250] Strip workspace name of html or handle validation php side reported by @kerupuksambel #12268

Closed

youssef-lr self-assigned this Nov 11, 2022

youssef-lr changed the title ~~[WIP] Support recent Onyx error keys in Form, and add option to hide the submit button~~ Support recent Onyx error keys in Form, and add option to hide the submit button Nov 14, 2022

youssef-lr changed the title ~~Support recent Onyx error keys in Form, and add option to hide the submit button~~ [NO QA] Support recent Onyx error keys in Form, and add option to hide the submit button Nov 14, 2022

Fix conflicts

4cf6f29

youssef-lr changed the title ~~[NO QA] Support recent Onyx error keys in Form, and add option to hide the submit button~~ [NO QA] Support display backend errors in Form Nov 14, 2022

youssef-lr changed the title ~~[NO QA] Support display backend errors in Form~~ [NO QA] Support displaying backend errors in Form Nov 14, 2022

Allow focus on first input that has an API error

7142d73

youssef-lr requested a review from ctkochan22 November 16, 2022 12:35

Fix conflicts

a3ad865

youssef-lr changed the title ~~[NO QA] Support displaying backend errors in Form~~ Support displaying backend errors in Form Nov 21, 2022

Cleanup debugging code

f495ce9

ctkochan22 approved these changes Nov 21, 2022

View reviewed changes

ctkochan22 merged commit 00d7c61 into main Nov 21, 2022

ctkochan22 deleted the youssef_refactor_form branch November 21, 2022 22:12

melvin-bot bot added the Emergency label Nov 21, 2022

ctkochan22 removed the Emergency label Nov 21, 2022

OSBotify mentioned this pull request Nov 22, 2022

Deploy Checklist: New Expensify 2022-11-22 #12934

Closed

40 tasks

kbecciv mentioned this pull request Nov 23, 2022

Workspace- Error message isn't displayed when edit a Workspace name to <b></b> #12966

Closed

ctkochan22 mentioned this pull request Dec 8, 2022

Form doesn't show inline errors unless we click out or tab #13424

Closed

marcaaron reviewed Dec 30, 2022

View reviewed changes

Conversation

youssef-lr commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Regression test: let's test one of the forms that uses Form and make sure everything works properly.

Testing error displayed under a field with a backend error (must be tested with https://github.com/Expensify/Web-Expensify/pull/35548)

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Uh oh!

melvin-bot bot commented Sep 30, 2022

Uh oh!

youssef-lr Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deetergp commented Sep 30, 2022

Uh oh!

youssef-lr commented Sep 30, 2022

Uh oh!

deetergp commented Nov 10, 2022

Uh oh!

youssef-lr commented Nov 10, 2022

Uh oh!

youssef-lr commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctkochan22 commented Nov 16, 2022

Uh oh!

ctkochan22 commented Nov 16, 2022

Uh oh!

ctkochan22 commented Nov 21, 2022

Uh oh!

youssef-lr commented Nov 21, 2022

Uh oh!

ctkochan22 commented Nov 21, 2022

Uh oh!

ctkochan22 left a comment

Choose a reason for hiding this comment

Uh oh!

melvin-bot bot commented Nov 21, 2022

Uh oh!

ctkochan22 commented Nov 21, 2022

Uh oh!

OSBotify commented Nov 21, 2022

Uh oh!

OSBotify commented Nov 22, 2022

Uh oh!

OSBotify commented Nov 22, 2022

Uh oh!

OSBotify commented Nov 24, 2022

Uh oh!

marcaaron Dec 30, 2022

Choose a reason for hiding this comment

Uh oh!

marcaaron Dec 30, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

youssef-lr commented Sep 30, 2022 •

edited

Loading

youssef-lr Sep 30, 2022 •

edited

Loading

youssef-lr commented Nov 16, 2022 •

edited

Loading