Refactor AddPersonalBankAccount to use Form #11076

youssef-lr · 2022-09-17T20:32:02Z

Details

cc @luacmartins

Fixed Issues

$ #9577

Tests and QA

Navigate to Settings -> Payments -> Add Payment Method -> Bank Account
Go through the Plaid process to add an account
Select an account from the dropdown
Keep password field empty and submit the form: verify Password is a required field error is show.
Enter an invalid password: Verify that an Incorrect password is shown above the submit button.
Enter your valid password and verify the account gets added succesfully.

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

Verify that no errors appear in the JS console

Screenshots

Web

Screen.Recording.2022-09-22.at.22.38.24.mov

Mobile Web

Desktop

iOS

Android

melvin-bot · 2022-09-17T20:32:04Z

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

dangrous · 2022-09-22T23:55:44Z

src/pages/AddPersonalBankAccountPage.js

@@ -163,7 +122,14 @@ class AddPersonalBankAccountPage extends React.Component {
                        </FixedFooter>
                    </>
                ) : (
-                    <FormScrollView>
+                    <Form
+                        formID={ONYXKEYS.PERSONAL_BANK_ACCOUNT}


I know this already exists, but I'm hazy on how the form IDs work - would it make more sense to add a new ONYXKEYS.FORMS.PERSONAL_BANK_ACCOUNT to separate it out from the key for the value itself?

Otherwise this is looking good when I skimmed; I will look closer and also test tomorrow!

Yes one of the reasons I didn't wanna move the key under FORMS is that there are other external PRs ongoing that touch the same keys so we figured out that we'll keep keys as they are now and then once everything is merged we'll move the keys under the FORMS key. But I'm gonna double check in case this specific key is only used here then I can move it under FORMS. Good eye!

dangrous · 2022-09-24T00:16:15Z

Hey so I tested this on web and it seems good EXCEPT when I put in an incorrect password, there's no feedback - the button acts as if it's clicked but immediately returns to the same text, no error visible. Putting in the correct password works as expected.

Is there any other branch I need to check out locally other than this one (e.g. something in Web-E or otherwise) to make this work? Let me know!

youssef-lr · 2022-09-24T21:09:38Z

Oh it looks like I need to refactor Form.js in this PR to take into account the new errors key that gets returned from the API, previously we returned error.

youssef-lr · 2022-09-29T19:14:39Z

Updated!

dangrous

This looks good, and errors work now! Can you add testing steps into the overview (just write out what the video shows)? Then I can fill out that other checklist and get this merged, unless @luacmartins has any further notes.

Thanks!

luacmartins

Looking good. Couple of minor comments.

luacmartins · 2022-09-29T23:06:43Z

src/pages/AddPersonalBankAccountPage.js

     */
-    validate() {
+    validate(values) {
        const errors = {};
        if (_.isUndefined(this.state.selectedPlaidBankAccount)) {


Does it make sense to validate selectedPlaidBankAccount if it doesn't have an inputID and we hide the submit button if no value is selected?

You're right, I'll remove it.

luacmartins · 2022-09-29T23:07:03Z

src/pages/AddPersonalBankAccountPage.js

        const errors = {};
        if (_.isUndefined(this.state.selectedPlaidBankAccount)) {
            errors.selectedBank = true;
        }

-        if (this.props.isPasswordRequired && _.isEmpty(this.state.password)) {
+        if (this.props.isPasswordRequired && _.isEmpty(values.password)) {
            errors.password = true;


We need to convert this error to a translated error string instead of bool

Looks like isPasswordRequired is undefined, so code inside will never run. I think we only wanna show an error if the pasword is empty? Display something like "This field is required"? Or is the backend error enough?

Oh if we don't require a password to add a personal bank account, we don't need to validate it in the front-end either. We could just remove this check.

I think it is required to add a personal bank account. But for some reason we never set isPasswordRequired to true. Because if you send an empty password the backend sends back an "invalid password" error. I can either remove the check and rely on the backend, or add a check to validate the field is not empty.

The password field is required, so I think that we can remove mentions to that prop and:

Add a check for empty password

Add check for invalid password

Add check for [invalid password]

I think we're asking the user there to input their existing password? So it should already be valid, unless it's incorrect then the backend will handle that case. What do you think?

I think that's fine too. I don't have strong feelings about 2. We should still update the validate method to account for an empty password and remove the check for selectedPlaidBankAccount

@youssef-lr any updates regarding my comment above?

On it, sorry! Was OOO (sick).

luacmartins · 2022-09-29T23:09:47Z

Also the PR title and linked issue mention AddPlaidBankAccount, but the refactored page is AddPersonalBankAccount page 😬

youssef-lr · 2022-09-29T23:13:51Z

Also the PR title and linked issue mention AddPlaidBankAccount, but the refactored page is AddPersonalBankAccount page 😬

Yes I realize this 😁 I'm not really sue what to refactor inside of AddPlaidBankAccount, any insights? As for AddPersonalBankAccount, I refactored it because you mentioned it in this comment. Let me know what needs to be done in AddPlaidBankAccount.

Should I change this issue title? Or create separate issue for it and link to it here?

luacmartins · 2022-09-29T23:17:46Z

I think just changing the issue title and mentioning that comment in the original post is enough. As for the refactor in AddPlaidBankAccount, that comment points to this part

youssef-lr · 2022-10-06T19:48:11Z

Updated and added tests!

luacmartins

Almost there! Just a few minor style changes that I noticed while testing, we are adding extra margins around the form. I suggested changes that fix that.

luacmartins · 2022-10-07T15:22:30Z

src/pages/AddPersonalBankAccountPage.js

+                        onSubmit={this.submit}
+                        validate={this.validate}
+                        style={[styles.mh5, styles.mb5, styles.flex1]}
+                    >
                        <View style={[styles.mh5, styles.mb5, styles.flex1]}>


This View is duplicating the styles applied by Form. I think we should just make it a fragment.

Suggested change

<View style={[styles.mh5, styles.mb5, styles.flex1]}>

<>

luacmartins · 2022-10-07T15:22:38Z

src/pages/AddPersonalBankAccountPage.js

                                        autoCompleteType="password"
                                        textContentType="password"
                                        autoCapitalize="none"
                                        autoFocus={canFocusInputOnScreenFocus()}
-                                        onChangeText={text => this.setState({password: text})}
-                                        errorText={this.getErrorText('password')}
-                                        hasError={this.getErrors().password}
                                    />
                                </View>
                            )}
                        </View>


Suggested change

</View>

</>

luacmartins · 2022-10-07T15:22:55Z

src/pages/AddPersonalBankAccountPage.js

@@ -177,30 +140,18 @@ class AddPersonalBankAccountPage extends React.Component {
                            {!_.isUndefined(this.state.selectedPlaidBankAccount) && (
                                <View style={[styles.mb5]}>


We can remove this view

Suggested change

<View style={[styles.mb5]}>

luacmartins · 2022-10-07T15:23:01Z

src/pages/AddPersonalBankAccountPage.js

                                        autoCompleteType="password"
                                        textContentType="password"
                                        autoCapitalize="none"
                                        autoFocus={canFocusInputOnScreenFocus()}
-                                        onChangeText={text => this.setState({password: text})}
-                                        errorText={this.getErrorText('password')}
-                                        hasError={this.getErrors().password}
                                    />
                                </View>


Suggested change

</View>

luacmartins · 2022-10-07T15:23:20Z

src/pages/AddPersonalBankAccountPage.js

+                        submitButtonText={this.props.translate('common.saveAndContinue')}
+                        onSubmit={this.submit}
+                        validate={this.validate}
+                        style={[styles.mh5, styles.mb5, styles.flex1]}


We don't need the extra bottom margin

Suggested change

style={[styles.mh5, styles.mb5, styles.flex1]}

style={[styles.mh5, styles.flex1]}

youssef-lr · 2022-10-07T15:40:28Z

Updated!

luacmartins

LGTM

OSBotify · 2022-10-07T16:14:46Z

✋ 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-10-12T00:06:05Z

🚀 Deployed to staging by @luacmartins in version: 1.2.13-0 🚀

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

yuwenmemon · 2022-10-13T05:07:47Z

Tested, it works! Checking off ✅

OSBotify · 2022-10-13T05:56:04Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.13-5 🚀

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

…dplaidaccount_form Refactor AddPersonalBankAccount to use Form

Refactor AddPersonalBankAccountPage to use Form

b31092a

youssef-lr self-assigned this Sep 18, 2022

youssef-lr marked this pull request as ready for review September 22, 2022 21:40

youssef-lr requested a review from a team as a code owner September 22, 2022 21:40

melvin-bot bot requested review from dangrous and removed request for a team September 22, 2022 21:41

Use new format of server errors

f78bcb5

dangrous reviewed Sep 22, 2022

View reviewed changes

youssef-lr mentioned this pull request Sep 23, 2022

Refactor ValidationStep Form #10946

Merged

93 tasks

luacmartins self-requested a review September 26, 2022 15:03

youssef-lr added 2 commits September 29, 2022 20:04

Merge branch 'main' into youssef_refactor_addplaidaccount_form

04a61a0

Use errors Onyx key in Form

2273fac

dangrous previously approved these changes Sep 29, 2022

View reviewed changes

luacmartins requested changes Sep 29, 2022

View reviewed changes

youssef-lr changed the title ~~Refactor AddPlaidBankAccount to use Form~~ Refactor AddPersonalBankAccount to use Form Sep 30, 2022

youssef-lr mentioned this pull request Sep 30, 2022

Form refactor bankaccountstep #10900

Merged

youssef-lr added 2 commits October 6, 2022 20:40

Merge branch 'main' into youssef_refactor_addplaidaccount_form

5a4c919

Remove unnecessary validation for Plaid and validate empty password

b935ac9

youssef-lr dismissed dangrous’s stale review via b935ac9 October 6, 2022 19:45

luacmartins requested changes Oct 7, 2022

View reviewed changes

youssef-lr added 2 commits October 7, 2022 16:37

Apply requested changes

26e26ec

Remove unneccessary margin

3a7e3e4

luacmartins approved these changes Oct 7, 2022

View reviewed changes

luacmartins merged commit 73c6279 into main Oct 7, 2022

luacmartins deleted the youssef_refactor_addplaidaccount_form branch October 7, 2022 16:10

OSBotify mentioned this pull request Oct 11, 2022

Deploy Checklist: New Expensify 2022-10-11 #11741

Closed

48 tasks

smrutiparida pushed a commit to autosave-app/App that referenced this pull request Oct 13, 2022

Merge pull request Expensify#11076 from Expensify/youssef_refactor_ad…

18b1007

…dplaidaccount_form Refactor AddPersonalBankAccount to use Form

		@@ -177,30 +140,18 @@ class AddPersonalBankAccountPage extends React.Component {
		{!_.isUndefined(this.state.selectedPlaidBankAccount) && (
		<View style={[styles.mb5]}>

	style={[styles.mh5, styles.mb5, styles.flex1]}
	style={[styles.mh5, styles.flex1]}

Refactor AddPersonalBankAccount to use Form #11076

Refactor AddPersonalBankAccount to use Form #11076

Conversation

youssef-lr commented Sep 17, 2022 • edited by luacmartins Loading

Details

Fixed Issues

Tests and QA

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

melvin-bot bot commented Sep 17, 2022

Choose a reason for hiding this comment

youssef-lr Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

dangrous commented Sep 24, 2022

youssef-lr commented Sep 24, 2022

youssef-lr commented Sep 29, 2022

dangrous left a comment

Choose a reason for hiding this comment

luacmartins 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins commented Sep 29, 2022 • edited Loading

youssef-lr commented Sep 29, 2022

luacmartins commented Sep 29, 2022

youssef-lr commented Oct 6, 2022

luacmartins 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

youssef-lr commented Oct 7, 2022

luacmartins left a comment

Choose a reason for hiding this comment

OSBotify commented Oct 7, 2022

OSBotify commented Oct 12, 2022

yuwenmemon commented Oct 13, 2022

OSBotify commented Oct 13, 2022

youssef-lr commented Sep 17, 2022 •

edited by luacmartins

Loading

youssef-lr Sep 23, 2022 •

edited

Loading

luacmartins commented Sep 29, 2022 •

edited

Loading