[WIP] Improve VBA flow error handling to show all errors at the same time #4890

aldo-expensify · 2021-08-27T19:22:55Z

Related Web-Secure PR: https://github.com/Expensify/Web-Secure/pull/2021

Details

Target:

Highlight fields with error even for error that comes from the back-end
Highlight all fields with errors at the same time

So far:

Move error modal from CompanyStep to ReimbursementAccountPage and handle open state in Onyx. Added helper function setErrorModalVisible in BankAccount.js for this.

Fixed Issues

$ #4785

Tests

Go to /bank-account/company
...

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

aldo-expensify · 2021-08-27T20:06:46Z

I noticed that any key stroke in an input updates the state at CompanyStep holding the inputs values, causing a re-render of the whole form. I have seen in applications with big forms that it can be a big performance saver to update a local state within the input component and debounce the final update to the form state, so we get less re-renders of the whole form. In each keystroke, before the debounce hits, you are just updating the local state at the input (normally a leaf in the react tree) and re-rendering only that input.

marcaaron · 2021-08-27T20:38:05Z

I noticed that any key stroke in an input updates the state at CompanyStep holding the inputs values, causing a re-render of the whole form. I have seen in applications with big forms that it can be a big performance saver to update a local state within the input component and debounce the final update to the form state, so we get less re-renders of the whole form. In each keystroke, before the debounce hits, you are just updating the local state at the input (normally a leaf in the react tree) and re-rendering only that input.

Nice. Sounds like something to follow up on after we are done here?

marcaaron

Looks great so far! I know this is still a draft but thought I'd leave some initial comments.

marcaaron · 2021-08-27T20:47:04Z

src/pages/ReimbursementAccount/CompanyStep.js

-
-        return true;
+        // TODO: set the validation errors outside?
+        setBankAccountFormValidationErrors(errors);


Feels fine to do this here to me. Less sure about isValidIdentity that we can worry about next.

marcaaron · 2021-08-27T20:47:32Z

src/pages/ReimbursementAccount/CompanyStep.js

-        return true;
+        // TODO: set the validation errors outside?
+        setBankAccountFormValidationErrors(errors);
+        return Object.keys(errors).length === 0;


maybe can do _.size()

marcaaron · 2021-08-27T20:49:13Z

src/pages/ReimbursementAccount/CompanyStep.js

-        const error = this.props.reimbursementAccount.error;
+
+        const errors = lodashGet(this.props, ['reimbursementAccount', 'errors'], {});
+        const getTextChangeHandler = (valueKey, errorKey) => (newValue) => {


This can be a method on the React class so we don't have to re-create it each render.

As a convention, we define methods by what they do and not what they handle so this should probably not be a "getter" or include the word "handler"... more context here

This can be a method on the React class so we don't have to re-create it each render.

Totally agree, I didn't think about that because I'm used to functional react

marcaaron · 2021-08-27T20:56:27Z

src/pages/ReimbursementAccount/CompanyStep.js

-                                }
-                                this.setState({addressStreet});
-                            }}
+                            onChangeText={getTextChangeHandler('addressStreet', 'companyStepAddressStreet')}


This is kind of fancy to take advantage of closures. But I feel it's maybe easier to understand if we do

onChangeText={value => validateAndSetTextValue('addressStreet', value)}

What do you think about dropping the error key for now and instead we can clear errors when we submit the form instead of on text change?

I think that if we don't clear the errors immediately when the user is changing the value, it may confuse the user into thinking that it is still wrong. Maybe we could do:

validateAndSetTextValue(valueKey, value) { this.setState({[valueKey]: value}, () => this.validate()); // or setBankAccountFormValidationErrors({}) instead of this.validate() }

And I think it will be even better if we just get rid of the error of the field that is being changed, but for that we need the errorKey. What do you think about deriving it consistently from the valueKey like:

function getErrorKey (valueKey) { return `companyStep${valueKey[0].toUpperCase()}${valueKey.slice(1)}`; }

if we don't clear the errors immediately when the user is changing the value, it may confuse the user into thinking that it is still wrong

That's a good point. Should we bring this back to the Slack thread before continuing?
Maybe @kevinksullivan and @MitchExpensify can advise.

And I think it will be even better if we just get rid of the error of the field that is being changed, but for that we need the errorKey. What do you think about deriving it consistently from the valueKey

I'm not too sure what the getErrorKey() method is doing there. Maybe we can start with something simpler for now?

I'm not too sure what the getErrorKey() method is doing there. Maybe we can start with something simpler for now?

This is just assuming that in the reimbursementAccount.errors we will be storing errors like companyStepPhoneNumber instead of just phoneNumber. If we store them just with the """key""" of the input (phoneNumber) instead of prefixing it with companyStep it would make our life easier :)

Ok got it! Let's start with the easy thing and see how far we can get :)

marcaaron · 2021-08-27T21:01:20Z

src/pages/ReimbursementAccount/CompanyStep.js

-                            errorText={error === this.props.translate('bankAccount.error.addressStreet')
-                                ? this.props.translate('bankAccount.error.addressStreet')
-                                : ''}
+                            errorText={getErrorText('companyStepAddressStreet', 'bankAccount.error.addressStreet')}


We could maybe define the translation key in the method or on the React class (this.errorTranslationKeys) or even outside of it instead of having to pass it here - the translation keys should never change.

hmm maybe we could have a map ({}) stored in the class to get either the translationKey or the errorKey from a valueKey. Just like when we define the array this.requiredFields

A map like:

this.fields = { companyName: { required: true, translationKey: 'bankAccount.error.companyName', errorKey: 'companyStepCompanyName', }, addressStreet: { required: true, translationKey: 'bankAccount.error.addressStreet', errorKey: 'companyStepAddressStreet', }, addressCity: { required: true, translationKey: 'bankAccount.error.addressCity', errorKey: 'companyStepAddressCity', }, addressState: { required: true, translationKey: 'bankAccount.error.addressState', errorKey: 'companyStepAddressState', }, addressZipCode: { required: true, translationKey: 'bankAccount.error.addressZipCode', errorKey: 'companyStepAddressZipCode', }, website: { required: true, translationKey: 'bankAccount.error.website', errorKey: 'companyStepWebsite', }, companyTaxID: { required: true, translationKey: 'bankAccount.error.companyTaxID', errorKey: 'companyStepCompanyTaxID', }, incorporationDate: { required: true, translationKey: 'bankAccount.error.incorporationDate', errorKey: 'companyStepIncorporationDate', }, incorporationState: { required: true, translationKey: 'bankAccount.error.incorporationState', errorKey: 'companyStepIncorporationState', }, incorporationType: { required: true, translationKey: 'bankAccount.error.incorporationType', errorKey: 'companyStepIncorporationType', }, industryCode: { required: true, translationKey: 'bankAccount.error.industryCode', errorKey: 'companyStepIndustryCode', }, password: { required: true, translationKey: 'bankAccount.error.password', errorKey: 'companyStepPassword', }, };

~~But I'm not entirely convinced that this could be better than passing the translationKey as a hardcoded parameter like it is doing it now.~~

Actually, I'm repeating the errorKeys in three places now, I guess it would be good to have either a function to produce them based on the inputKey (aka valueKey in my previous comments) or have a map somewhere to obtain it.

Do we need the errorKey at all? It seems pretty similar to the field name. What if we have a map like this then we should be able to find out which error to use for that field

{ password: 'bankAccount.error.password', industryCode: 'bankAccount.error.industryCode', ...etc, }

If we go for storing the errors without the companyStep prefix in reimbursementAccount.errors we shouldn't need a special errorKey

I got rid of the prefix 'companyStep' in the errorKeys, and implemented the suggested map to get the translation keys.

aldo-expensify · 2021-08-27T22:51:03Z

What do we think about using a validation library like yup for our validations?

marcaaron · 2021-08-27T23:08:46Z

What do we think about using a validation library like yup for our validations?

I'm not too familiar with yup, but we generally try to avoid 3rd party dependencies if we can. That said, sometimes there are dependencies that are too valuable to leave out. It's always good to get more opinions in #engineering-chat on stuff like that. Also helps if you do a bit of research first and tell people what the benefits might be. 🙃

marcaaron · 2021-08-27T23:54:30Z

src/libs/actions/BankAccounts.js

-                if (response.jsonCode === 402) {
+                } else if (JSON_CODE_TO_ERROR_KEYS[response.jsonCode]) {
+                    setBankAccountFormValidationErrors({[JSON_CODE_TO_ERROR_KEYS[response.jsonCode]]: true});
+                    setErrorModalVisible(true);


This is an interesting approach, but I feel it maybe makes it difficult to tell which errors we are handling here.

I haven't thought of a great solution yet, but I think this is a pattern we're going to want to apply pretty regularly so we should think about it some more and maybe be cautious about optimizing too soon.

I sort of imagined this being more explicit for now like...

} else if (response.jsonCode === CONST.BANK_ACCOUNT.ERROR_CODE.INVALID_PHONE) { errors.phoneNumber = true; } else if (response.jsonCode === CONST.BANK_ACCOUNT.ERROR_CODE.INVALID_DATE) { errors.date = true; ... if (_.size(errors)) { showBankAccountFormValidationErrors(errors); setErrorModalVisible(true); }

This way we can look at the response handler and just really clearly see what kinds of things can happen.

Updated. You may noticed that the errors variable is not defined in the closest scope, it is in a scope next to error. I made it like that because eventually it is supposed to be used with other steps. But if you think I should move it to the closest scope, let me know.

Add map to get translation keys for each field

aldo-expensify · 2021-09-02T23:57:37Z

Closing this PR, we are working this on separate PRs, smaller increments.

Move error modal to ReimbursementAccountPage

c83b032

aldo-expensify requested a review from marcaaron August 27, 2021 19:22

aldo-expensify requested a review from a team as a code owner August 27, 2021 19:22

MelvinBot requested review from tgolen and removed request for a team August 27, 2021 19:23

Pass strictly true false to ConfirmModal.isVisible

7de3907

First version highlighting multiple errors

4339757

marcaaron reviewed Aug 27, 2021

View reviewed changes

Highlight errors using jsonCode from web-secure

76a28b7

marcaaron assigned aldo-expensify Aug 27, 2021

marcaaron reviewed Aug 27, 2021

View reviewed changes

aldo-expensify added 2 commits August 27, 2021 17:13

Get rid of companyStep prefix for error keys

5c1a396

Add map to get translation keys for each field

Handle each jsonCode error more explicitly

277b275

aldo-expensify changed the title ~~[WIP] Improve VBA flow error handling~~ [WIP] Improve VBA flow error handling to show all errors at the same time Sep 2, 2021

aldo-expensify closed this Sep 2, 2021

This was referenced Sep 3, 2021

Highlight all errors together in BankAccount step #5041

Merged

Highlight all errors in VBA flow - Company step #5073

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Improve VBA flow error handling to show all errors at the same time #4890

[WIP] Improve VBA flow error handling to show all errors at the same time #4890

aldo-expensify commented Aug 27, 2021 •

edited

Loading

aldo-expensify commented Aug 27, 2021 •

edited

Loading

marcaaron commented Aug 27, 2021

marcaaron left a comment

marcaaron Aug 27, 2021

marcaaron Aug 27, 2021

marcaaron Aug 27, 2021

marcaaron Aug 27, 2021

aldo-expensify Aug 27, 2021

marcaaron Aug 27, 2021

aldo-expensify Aug 27, 2021

marcaaron Aug 27, 2021

aldo-expensify Aug 27, 2021 •

edited

Loading

marcaaron Aug 28, 2021

marcaaron Aug 27, 2021

aldo-expensify Aug 27, 2021 •

edited

Loading

aldo-expensify Aug 27, 2021 •

edited

Loading

marcaaron Aug 27, 2021 •

edited

Loading

aldo-expensify Aug 27, 2021 •

edited

Loading

aldo-expensify Aug 28, 2021

aldo-expensify commented Aug 27, 2021

marcaaron commented Aug 27, 2021

marcaaron Aug 27, 2021

aldo-expensify Aug 28, 2021

aldo-expensify commented Sep 2, 2021

[WIP] Improve VBA flow error handling to show all errors at the same time #4890

[WIP] Improve VBA flow error handling to show all errors at the same time #4890

Conversation

aldo-expensify commented Aug 27, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

aldo-expensify commented Aug 27, 2021 • edited Loading

marcaaron commented Aug 27, 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

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 Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldo-expensify Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

aldo-expensify Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

marcaaron Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

aldo-expensify Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldo-expensify commented Aug 27, 2021

marcaaron commented Aug 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldo-expensify commented Sep 2, 2021

aldo-expensify commented Aug 27, 2021 •

edited

Loading

aldo-expensify commented Aug 27, 2021 •

edited

Loading

aldo-expensify Aug 27, 2021 •

edited

Loading

aldo-expensify Aug 27, 2021 •

edited

Loading

aldo-expensify Aug 27, 2021 •

edited

Loading

marcaaron Aug 27, 2021 •

edited

Loading

aldo-expensify Aug 27, 2021 •

edited

Loading