Highlight all errors in VBA flow - Company step #5073

aldo-expensify · 2021-09-03T22:33:02Z

Brought some of the functionality implemented in this dead PR

Investigate: can functions like clearErrorAndSetValue be reused somehow? We will need to repeat this and other functions in every step.

Details

Changes:

Added missing errors for fields in Company Step (i.e. City)
Show all highlighted errors together in Company Step
The "Save & continue" button is disabled if the checkbox next to "I confirm that this company...." has not been checked or if there is any field left empty. Enabling the input in all cases will be handled when we address this GH issue

Fixed Issues

$ #4785

We are working in this issue step by step. This PR updates the error handling specifically for the company information step in the VBA flow.

Tests AND QA Steps

Get to the step where we fill the "Company Information", two options:
1. In a browser, navigate to /bank-account/company
2. or do the following steps:
  1. Start with a non-public domain account that has not yet set up a workspace or bank account
  2. Create a new workspace by tapping on the green FAB button
  3. Navigate to /bank-account by selecting the workspace and tapping the "Get Started" button
  4. Tap button to add account manually: Log in into your bank
  5. Enter data following step (3) Add Bank Account in this SO
Fill all fields, and check the checkbox next to "I confirm that this company is not on the...". Make sure there is invalid data in Zip code, Company website, Tax ID number and Industry classification code. Invalid data could be for example 1.
Once all fields have been filled, click "Save & continue"
Expected result: You should get the error modal with the generic message: Please double check any highlighted fields and try again. and see all fields with inline highlighted errors at the same time:
Focus on any of the text inputs with errors, type something more.
Expected result: the inline error and highlight should disappear only for that input.
Fill all fields of the form with valid values and input an invalid "Phone Number" like 123. Valid values:
- Zip Code: 123456
- Company website: https://test.com
- Tax ID number: 123456789
- Industry classification code: 123456
Click "Save & continue"
Expected result: We should see the loading transition (data is sent to back-end), then we should see the error modal with a message like "Please provide a valid Phone Number", and no fields should be highlighted.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Add props to receive error in ExpensiPicker Set red outline based on the error received Display optional error text in ExpensiPicker

…rors-vba-company

aldo-expensify · 2021-09-09T22:06:54Z

Update: Removed WIP/Draft. Handle error outline for StatePicker.

src/components/ExpensiPicker.js

src/components/ExpensiTextInput/propTypes.js

src/components/StatePicker.js

Julesssss · 2021-09-10T09:47:32Z

src/pages/ReimbursementAccount/CompanyStep.js

@@ -80,58 +80,107 @@ class CompanyStep extends React.Component {
            'password',
            'companyPhone',
        ];
+
+        // Keys in this.errorTranslationKeys are associated to inputs, they are a subset of the keys found in this.state


NAB: I read this comment a few times and couldn't understand the purpose until I looked at usages in the code.

I changed it, is it better or worse? :P

src/pages/ReimbursementAccount/CompanyStep.js

Julesssss

I think there's some potential to improve ExpensiTextInput further and to delegate some of the error logic to the component, but this isn't an N6 blocker.

Julesssss · 2021-09-13T10:21:48Z

src/components/ExpensiPicker.js

+    /** Should the input be styled for errors  */
+    hasError: PropTypes.bool,


NAB: It kinda feels like we're tying the prop to UI here, I would have thought the prop should simply describe state, rather than what we do with that state. But I could be wrong, and this is not a huge deal I guess.

What would the alternative be here? FWIW this seems to be how we're handling things in the ExpensiTextInput

App/src/components/ExpensiTextInput/propTypes.js

Lines 13 to 17 in 3e11093

/** Error text to display */

errorText: PropTypes.string,

/** Should the input be styled for errors */

hasError: PropTypes.bool,

NAB: It kinda feels like we're tying the prop to UI here, I would have thought the prop should simply describe state, rather than what we do with that state. But I could be wrong, and this is not a huge deal I guess.

Are you saying that the comment should be updated? or the itself prop is not well defined?

Julesssss

Tested web and the forms are working as expected.

I see a few improvements (scroll to error on mobile, highlight API forms), but I see that we're aware of this and still discussing here.

Julesssss · 2021-09-13T10:28:32Z

Hey @marcaaron, did you want (and have time) to review this PR? If yes then I'll await your review.

marcaaron

Looking good ! Left some comments / notes.

marcaaron · 2021-09-13T17:25:38Z

src/components/ExpensiPicker.js

+    /** Should the input be styled for errors  */
+    hasError: PropTypes.bool,


What would the alternative be here? FWIW this seems to be how we're handling things in the ExpensiTextInput

App/src/components/ExpensiTextInput/propTypes.js

Lines 13 to 17 in 3e11093

/** Error text to display */

errorText: PropTypes.string,

/** Should the input be styled for errors */

hasError: PropTypes.bool,

src/components/ExpensiPicker.js

src/components/ExpensiTextInput/propTypes.js

src/components/StatePicker.js

src/pages/ReimbursementAccount/CompanyStep.js

marcaaron · 2021-09-13T17:55:38Z

src/pages/ReimbursementAccount/CompanyStep.js

                                    value={this.state.addressState}
+                                    hasError={this.getErrors().addressState}


Are we going to add a label for this one?

I was just putting the red outline because there is little space under the StatePicker, do you want me to test how it would look with the error text there?

Could we show the inline message in the space here?

Any thoughts @shawnborton ? Or should we just let this one go?

src/pages/ReimbursementAccount/CompanyStep.js

Can't click "Save & continue" when there are inputs that are blank

aldo-expensify · 2021-09-13T23:55:37Z

Update: I decided to stick to leaving the "Save & continue" button disabled if there is any empty field (including terms and conditions checkbox). Why?:

To implement the "Save & continue" always enabled behaviour I needed to add the error text and outline for another component that doesn't have it yet: CheckboxWithLabel.
Adding the error outline + text to CheckboxWithLabel would probably require some feedback from a designer.
I was already adding error text + outline support for ExpensiPicker and StatePicker which was feeling like going too far with this PR.

Considering this, the following changed:

This PR is sticking to the strategy of only enabling the "Save & continue" button if there is NO empty field. This is consistent with the "add manually bank account step"
Error text + outline for ExpensiPicker and StatePicker is not necessary in this PR anymore, so it was removed
I'm still outlining and showing all errors together for inputs of type ExpensiTextInput

The following should be addressed in a future PR:

Add support for error text + outline for ExpensiPicker, StatePicker and CheckboxWithLabel
Make the "Save & continue" button always enabled

We have a GH issue and @pecanoro has a PR already on the way for it.

marcaaron

Just a couple more comments. This is looking really great.

src/pages/ReimbursementAccount/CompanyStep.js

marcaaron · 2021-09-14T01:48:56Z

src/libs/ValidationUtils.js

+        return !_.isEmpty(value);
+    }
+    return Boolean(value);
+}


NAB, I like this method. My one hesitation is whether the boolean case makes the name confusing b.c. a boolean with a false is technically "present" it's just false.

For the boolean case we are just returning the boolean again not whether it has a value or not.

I had the same concern about booleans, maybe it would be cleaner to consider true and false as present. Then, we just handle the case of the "terms and conditions" checkbox without this function.

Ok, made a small change here. false and true (booleans) are considered "present". Therefore, we are not using this function to test the value for hasNoConnectionToCannabis.

Ok cool I think this works for now. Probably we will want to think about this some more when we build a more generic form component and need a generic way to test that a field marked as "required" when a boolean will be tested for "existing" and "true".

Ok cool I think this works for now. Probably we will want to think about this some more when we build a more generic form component and need a generic way to test that a field marked as "required" when a boolean will be tested for "existing" and "true".

Yes, I think this is a good idea. Postpone thinking further about this until we have more cases or a more generic.

marcaaron · 2021-09-14T02:07:36Z

I was already adding error text + outline support for ExpensiPicker and StatePicker which was feeling like going too far with this PR.

Ok good call. Sounds like this is a blocker for this issue to remove the disabled feature so we can maybe take care of this there.

Error text + outline for ExpensiPicker and StatePicker is not necessary in this PR anymore, so it was removed

Just to clarify, since we are not able to add this for the checkbox you'd think it would be better to add inline handling later as well?

aldo-expensify · 2021-09-14T17:55:27Z

Just to clarify, since we are not able to add this for the checkbox you'd think it would be better to add inline handling later as well?

Yes. In this PR we are keeping the "Save & continue" button disabled if there is any empty field. The "Save & continue" button doesn't become enabled until we have chosen all states (StatePicker) and the company type (ExpensiPicker), so these highlights were not going to be used/shown under this configuration. Same thing applies for the inline highlight of the checkbox component.

I think these things should be added together with making the "Save & continue" button always enabled, because then we will have the case where we want to inline an error in those inputs.

src/libs/ValidationUtils.js

marcaaron

LGTM. I only really have one small note left about the comment in the isValuePresent method. Once that is resolved I'm ready to merge.

marcaaron · 2021-09-14T18:51:38Z

src/pages/ReimbursementAccount/CompanyStep.js

}
-

NAB, removing these new lines seems unnecessary. I like them personally since things are less visually compressed, but that's just my opinion and there are no style rules about this as far as I know.

Got mixed up with the php guidelines. I assumed that a similar rule would apply here... reverting!

aldo-expensify · 2021-09-14T21:27:12Z

Update:

isValuePresent became isRequiredFulfilled
isRequiredFulfilled returns false if value is false
Added hasNoConnectionToCannabis to required fields in CompanyStep.js

marcaaron · 2021-09-14T22:12:26Z

Going to merge this so we can keep plugging away at the remaining steps. @Julesssss thanks for your review and please let us know if you've got any additional comments that we can follow up on if they were not resolved.

OSBotify · 2021-09-14T22:13: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 · 2021-09-15T21:16:08Z

🚀 Deployed to staging by @marcaaron in version: 1.0.98-2 🚀

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

OSBotify · 2021-09-20T21:01:16Z

🚀 Deployed to production by @Jag96 in version: 1.0.99-4 🚀

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

Highlight all errors company step

80735c2

aldo-expensify requested a review from marcaaron September 3, 2021 22:33

aldo-expensify added 3 commits September 3, 2021 16:33

Display error for Company Type in Company Step

79d07ed

Add props to receive error in ExpensiPicker Set red outline based on the error received Display optional error text in ExpensiPicker

Add error outline for StatePicker

046abd2

Merge branch 'main' of github.com:Expensify/App into aldo_show-all-er…

bf88c8e

…rors-vba-company

aldo-expensify changed the title ~~[WIP] Highlight all errors in VBA flow - Company step~~ Highlight all errors in VBA flow - Company step Sep 9, 2021

aldo-expensify marked this pull request as ready for review September 9, 2021 22:05

aldo-expensify requested a review from a team as a code owner September 9, 2021 22:05

MelvinBot requested review from Julesssss and removed request for a team September 9, 2021 22:05

Julesssss assigned aldo-expensify Sep 10, 2021

Julesssss reviewed Sep 10, 2021

View reviewed changes

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

Julesssss reviewed Sep 10, 2021

View reviewed changes

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

Julesssss reviewed Sep 10, 2021

View reviewed changes

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

Julesssss reviewed Sep 10, 2021

View reviewed changes

src/components/ExpensiTextInput/propTypes.js Show resolved Hide resolved

Julesssss reviewed Sep 10, 2021

View reviewed changes

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

Julesssss reviewed Sep 10, 2021

View reviewed changes

src/pages/ReimbursementAccount/CompanyStep.js Outdated Show resolved Hide resolved

Julesssss reviewed Sep 10, 2021

View reviewed changes

aldo-expensify added 2 commits September 10, 2021 09:43

Merge branch 'main' - Solve conflicts

48c9c18

Fix indentation and remove empty line

21bfaa0

Julesssss reviewed Sep 13, 2021

View reviewed changes

marcaaron requested changes Sep 13, 2021

View reviewed changes

aldo-expensify added 4 commits September 13, 2021 16:35

Follow style guidelines, refactoring

c2a569d

Can't click "Save & continue" when there are inputs that are blank

Remove error highlighting from ExpensiPicker

77f2320

Merge branch 'main'

6db9f9c

Fix incorrect merge

2b15b07

Update import after propTypes file rename

ddd6ac1

marcaaron requested changes Sep 14, 2021

View reviewed changes

aldo-expensify added 2 commits September 14, 2021 10:41

Remove redundant validations

87c1358

Consider false a present value for required fields

03cc2e5

marcaaron reviewed Sep 14, 2021

View reviewed changes

src/libs/ValidationUtils.js Show resolved Hide resolved

marcaaron requested changes Sep 14, 2021

View reviewed changes

aldo-expensify added 2 commits September 14, 2021 12:17

Put back newlines

f3467e1

Update isRequiredFulfilled boolean handling

52aef15

marcaaron approved these changes Sep 14, 2021

View reviewed changes

marcaaron merged commit 0f4abe1 into main Sep 14, 2021

marcaaron deleted the aldo_show-all-errors-vba-company branch September 14, 2021 22:12

aldo-expensify mentioned this pull request Sep 14, 2021

Not all errors are outlined in the VBA flow #4785

Closed

OSBotify mentioned this pull request Sep 15, 2021

Deploy Checklist: New Expensify 2021-09-15 #5279

Closed

27 tasks

luacmartins mentioned this pull request Sep 22, 2021

Do not save user password in reimbursementAccountDraft #5428

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Highlight all errors in VBA flow - Company step #5073

Highlight all errors in VBA flow - Company step #5073

aldo-expensify commented Sep 3, 2021 •

edited

Loading

aldo-expensify commented Sep 9, 2021 •

edited

Loading

Julesssss Sep 10, 2021 •

edited

Loading

aldo-expensify Sep 14, 2021

Julesssss left a comment

Julesssss Sep 13, 2021

marcaaron Sep 13, 2021

aldo-expensify Sep 13, 2021

Julesssss left a comment

Julesssss commented Sep 13, 2021

marcaaron left a comment

marcaaron Sep 13, 2021

marcaaron Sep 13, 2021

aldo-expensify Sep 13, 2021

marcaaron Sep 13, 2021

aldo-expensify commented Sep 13, 2021 •

edited

Loading

marcaaron left a comment

marcaaron Sep 14, 2021

aldo-expensify Sep 14, 2021

aldo-expensify Sep 14, 2021

marcaaron Sep 14, 2021

aldo-expensify Sep 14, 2021

marcaaron commented Sep 14, 2021

aldo-expensify commented Sep 14, 2021 •

edited

Loading

marcaaron left a comment

marcaaron Sep 14, 2021

aldo-expensify Sep 14, 2021

aldo-expensify commented Sep 14, 2021

marcaaron commented Sep 14, 2021

OSBotify commented Sep 14, 2021

OSBotify commented Sep 15, 2021

OSBotify commented Sep 20, 2021

		/** Should the input be styled for errors */
		hasError: PropTypes.bool,

	/** Error text to display */
	errorText: PropTypes.string,

	/** Should the input be styled for errors */
	hasError: PropTypes.bool,

		value={this.state.addressState}
		hasError={this.getErrors().addressState}

Highlight all errors in VBA flow - Company step #5073

Highlight all errors in VBA flow - Company step #5073

Conversation

aldo-expensify commented Sep 3, 2021 • edited Loading

Details

Fixed Issues

Tests AND QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

aldo-expensify commented Sep 9, 2021 • edited Loading

Julesssss Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julesssss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julesssss left a comment

Choose a reason for hiding this comment

Julesssss commented Sep 13, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldo-expensify commented Sep 13, 2021 • edited Loading

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Sep 14, 2021

aldo-expensify commented Sep 14, 2021 • edited Loading

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldo-expensify commented Sep 14, 2021

marcaaron commented Sep 14, 2021

OSBotify commented Sep 14, 2021

OSBotify commented Sep 15, 2021

OSBotify commented Sep 20, 2021

aldo-expensify commented Sep 3, 2021 •

edited

Loading

aldo-expensify commented Sep 9, 2021 •

edited

Loading

Julesssss Sep 10, 2021 •

edited

Loading

aldo-expensify commented Sep 13, 2021 •

edited

Loading

aldo-expensify commented Sep 14, 2021 •

edited

Loading