Trim the value when going back on connect bank account #28305

dukenv0307 · 2023-09-27T07:24:36Z

Details

We use state and value prop to override the draft value that is not trimmed on connect bank account

Fixed Issues

$ #25996
PROPOSAL: #25996 (comment)

Tests

Login with any account
Create a workspace
Go to the workspace and connect bank account
Enter the valid routing number, account number and go to the next step
For each field that can enter the value, enter the valid value with some spacing before
Fill in all other fields and click on save button
After going to the next step click on back button in the header
Verify that all text input values is trimmed

Verify that no errors appear in the JS console

Offline tests

No offline test step since bank account flow needs to enable the network

QA Steps

Login with any account
Create a workspace
Go to the workspace and connect bank account
Enter the valid routing number, account number and go to the next step
For each field that can enter the value, enter the valid value with some spacing before
Fill in all other fields and click on save button
After going to the next step click on back button in the header
Verify that all text input values is trimmed

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-09-27.at.13.48.56.mov

Mobile Web - Chrome

Record_2023-09-27-13-55-25.mp4

Mobile Web - Safari

Screen.Recording.2023-09-27.at.14.01.06.mov

Desktop

Screen.Recording.2023-09-27.at.14.13.12.mov

iOS

Screen.Recording.2023-09-27.at.14.10.10.mov

Android

Screen.Recording.2023-09-27.at.16.40.03.mov

melvin-bot · 2023-09-27T07:24:40Z

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

melvin-bot · 2023-09-27T09:50:15Z

@Ollyws Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

Ollyws · 2023-09-27T11:46:16Z

src/pages/ReimbursementAccount/CompanyStep.js

@@ -54,6 +54,23 @@ const defaultProps = {
 };

 function CompanyStep({reimbursementAccount, reimbursementAccountDraft, getDefaultStateForField, onBackButtonPress, translate, session, user, policyID}) {
+    const defaultWebsite = useMemo(() => (lodashGet(user, 'isFromPublicDomain', false) ? 'https://' : `https://www.${Str.extractEmailDomain(session.email, '')}`), [user, session]);
+    const [companyInfomation, setCompanyInfomation] = useState({


Typo: should be ...Information

Ollyws · 2023-09-27T12:32:39Z

@dukenv0307 A problem here is that the values will use getDefaultStateForField whenever the page has been refreshed.
Ideally we want to only use getDefaultStateForField after the Save & Continue button has been pressed, and otherwise continue using the draft.

dukenv0307 · 2023-09-27T14:13:31Z

A problem here is that the values will use getDefaultStateForField whenever the page has been refreshed.
Ideally we want to only use getDefaultStateForField after the Save & Continue button has been pressed, and otherwise continue using the draft.

@Ollyws I found another solution that can this issue and the problem above and updated it in the PR. We will save data that returns from BE to draft value instead of using state to control the value. That also can fix for personal detail step as well.
Please help to review again.

Ollyws · 2023-09-27T14:50:48Z

Thanks for the changes I like the new approach.

Ollyws · 2023-09-27T14:52:18Z

src/pages/ReimbursementAccount/ReimbursementAccountPage.js

+    getFieldsOfCurrentStep(currentStep) {
+        switch (currentStep) {
+            case CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT:
+                return ['routingNumber', 'accountNumber', 'bankName', 'plaidAccountID', 'plaidAccessToken', 'isSavings'];


It feels like we could be repeating ourselves a bit here, as if someone wanted to add/remove a field they would have to update this list too. Is there any way we could reference the fields in one place?

@Ollyws Here are the list values of the step and this is only called after api update success. If we want to update the change field to draft we will need to compare draft value and achData, I think it will take the same amount of time when we merge all fields of the step.

@dukenv0307 What I mean is, in RequestorStep for example we already have a list of input IDs could we perhaps use this list instead of creating a new list here, and implement something similar for the other steps?

The list here is the list that I copied in each step.

Should we create a const for the list and re-use this.

I just updated it.

@dukenv0307 Thanks but I was specifically referring to this list of input IDs, which we could then perhaps get the values of using _.values()? Let me know what you think.

@Ollyws I see that we only use this in the special form like AddressForm, other fields doesn't use this. So I think we don't need to create the list input key like this.

Ok well if we're not going to implement that then I don't really see the need for a const, just to be used for the requiredFields. Could you revert those two commits? Thanks!

…x/25996

Ollyws · 2023-09-28T16:18:05Z

@dukenv0307 Something I've noticed is if you press the back button while the page is still loading the draft values are never updated:

backbutton_example.mp4

VIDEO

dukenv0307 · 2023-09-29T07:21:35Z

@Ollyws I updated.

flodnv · 2023-10-04T07:42:44Z

Could you please fill in the Offline tests section?

flodnv · 2023-10-04T07:44:42Z

src/libs/actions/BankAccounts.js

@@ -57,10 +57,10 @@ function clearOnfidoToken() {

 /**
 * Helper method to build the Onyx data required during setup of a Verified Business Bank Account
- *
+ * @param {String | undefined} currentStep


Can you update the doc to explain why it's valid to pass undefined and what we should expect to happen in this strange case?

dukenv0307 · 2023-10-04T07:47:00Z

No offline test step since bank account flow needs to enable the network

@flodnv Added the note in offline tests. The bank account flow need to enable network to submit form.

flodnv · 2023-10-04T09:10:19Z

src/libs/actions/BankAccounts.js

@@ -57,7 +57,7 @@ function clearOnfidoToken() {

 /**
 * Helper method to build the Onyx data required during setup of a Verified Business Bank Account
- * @param {String | undefined} currentStep
+ * @param {String | undefined} currentStep The step that we need to update the data from BE to draft value


Suggested change

* @param {String | undefined} currentStep The step that we need to update the data from BE to draft value

* @param {String | undefined} currentStep The step that we need to update the data from backend to draft value

Is this what it means? If yes, let's update it to use the full word, I don't find it necessary to save a few characters. And if yes, I honestly don't really understand the sentence, perhaps you can try rephrasing it?

Also, you did not clarify what happens if you pass undefined?

I think something like The name of the step for which we will update the draft value when data has been sucessfully submitted to the backend. would be clearer.

Undefined here is just for the default value currentStep = undefined as far as I'm aware, in which case the draft will not be updated as this condition will be undefined.

I think something like The name of the step for which we will update the draft value when data has been sucessfully submitted to the backend. would be clearer.

Agreed, I do understand this better. Also, can we say "API" instead of "backend", since that's what we use everywhere?

dukenv0307 · 2023-10-04T09:30:38Z

src/libs/actions/BankAccounts.js

@@ -79,6 +79,10 @@ function getVBBADataForOnyx() {
                value: {
                    isLoading: false,
                    errors: null,
+                    // The value of some fields of the currentStep are changed i.e. being trimmed or phone number is parsed when backend returns the data


@flodnv Updated more explanation here.

Suggestion:

// The value of some fields of the currentStep are changed (i.e. trimmed, or phone number is formatted) when the API returns the data.

flodnv · 2023-10-06T12:33:46Z

src/libs/actions/BankAccounts.js

@@ -79,6 +79,10 @@ function getVBBADataForOnyx() {
                value: {
                    isLoading: false,
                    errors: null,
+                    // The value of some fields of the currentStep are changed i.e. being trimmed or phone number is parsed when backend returns the data


Suggestion:

// The value of some fields of the currentStep are changed (i.e. trimmed, or phone number is formatted) when the API returns the data.

flodnv · 2023-10-06T12:34:47Z

src/libs/actions/BankAccounts.js

@@ -79,6 +79,9 @@ function getVBBADataForOnyx(currentStep = undefined) {
                value: {
                    isLoading: false,
                    errors: null,
+                    // The value of some fields of the currentStep are changed i.e. being trimmed or phone number is parsed when backend returns the data
+                    // So we need to add this field in successData to update the current data from reimbursement account to the draft value of the form


I'm sorry, I read this 5 times and I don't understand what it's trying to convey, can you perhaps rephrase it in other words, and with more punctuation?

@flodnv Is this more clear?

// In bank account flow, we save the form value to the draft in Onyx. // When we update the information in a step, the value of some fields that are updated from backend // can be different from the value that we stored to the draft in Onyx (i.e. trimmed, or phone number is formatted). // So we should store the current step which we call the update API to update the data from backend to the draft in Onyx. // If currentStep is undefined that means this step don't need to update the data to the draft in Onyx

Better, maybe this is even better:

// When setting up a bank account, we save the draft form values in Onyx. // When we update the information for a step, the value of some fields that are returned from the API // can be different from the value that we stored as the draft in Onyx (i.e. the phone number is formatted). // This is why we store the current step used to call the API in order to update the corresponding draft data in Onyx. // If currentStep is undefined that means this step don't need to update the data of the draft in Onyx.

Do we need to move this to the method's doc

Hmmm, maybe yes?

flodnv · 2023-10-06T12:35:05Z

src/libs/actions/BankAccounts.js

@@ -79,6 +79,9 @@ function getVBBADataForOnyx(currentStep = undefined) {
                value: {
                    isLoading: false,
                    errors: null,
+                    // The value of some fields of the currentStep are changed i.e. being trimmed or phone number is parsed when backend returns the data
+                    // So we need to add this field in successData to update the current data from reimbursement account to the draft value of the form
+                    // if currentStep is undefined that means the step doesn't need update to the draft


Why isn't this in the method's doc?

src/libs/actions/BankAccounts.js

flodnv · 2023-10-06T13:06:56Z

src/libs/actions/BankAccounts.js

+                    // The value of some fields of the currentStep are changed i.e. being trimmed or phone number is parsed when backend returns the data
+                    // So we need to add this field in successData to update the current data from reimbursement account to the draft value of the form
+                    // if currentStep is undefined that means the step doesn't need update to the draft
+                    stepToUpdateToDraft: currentStep,


So why is this called stepToUpdateToDraft and not simply currentStep?

We already have a field named currentStep in achData of reimbursementAccount

Right... maybe draftStep then?

Co-authored-by: Florent De'Neve <florent@expensify.com>

src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Co-authored-by: Florent De'Neve <florent@expensify.com>

dukenv0307 · 2023-10-13T02:54:01Z

@flodnv Any update here.

flodnv · 2023-10-13T10:28:47Z

Sorry, in the future please re-request a review, I filter out commit emails because they're so noisy.

OSBotify · 2023-10-13T10:35:30Z

✋ 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 · 2023-10-13T21:11:10Z

🚀 Deployed to staging by https://github.com/flodnv in version: 1.3.84-0 🚀

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

OSBotify · 2023-10-16T21:11:38Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀

platform	result
🤖 android 🤖	skipped 🚫
🖥 desktop 🖥	skipped 🚫
🍎 iOS 🍎	skipped 🚫
🕸 web 🕸	skipped 🚫

OSBotify · 2023-10-16T21:47:58Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀

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

OSBotify · 2023-10-16T21:50:54Z

🚀 Deployed to staging by https://github.com/flodnv in version: 1.3.85-0 🚀

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

OSBotify · 2023-10-17T23:15:39Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.3.85-4 🚀

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

Trim the value when going back on connect bank account

183e3fe

use draft if the default value is empty

99477df

dukenv0307 marked this pull request as ready for review September 27, 2023 09:50

dukenv0307 requested a review from a team as a code owner September 27, 2023 09:50

melvin-bot bot requested review from Ollyws and removed request for a team September 27, 2023 09:50

Ollyws reviewed Sep 27, 2023

View reviewed changes

fix typo

04e7887

dukenv0307 added 2 commits September 27, 2023 21:10

save value to draft after updating data successfully

161abe5

fix merge main

15ef37f

dukenv0307 added 3 commits September 27, 2023 21:17

fix jest

883db65

fix lint

10f72f8

Merge branch 'main' into fix/25996

0fa5251

Ollyws reviewed Sep 27, 2023

View reviewed changes

dukenv0307 added 8 commits September 27, 2023 22:02

add comment

4532a36

create a const for fields of the step

3c4be81

fix lint

936289c

Merge branch 'fix/25996' of https://github.com/dukenv0307/App into fi…

88fc114

…x/25996

reuse the const

25c2366

revert const

6caa796

fix jest

270eddf

fix lint

12f5f05

fix no update when clicking on go back

a58a10e

melvin-bot bot requested a review from flodnv October 3, 2023 17:16

flodnv reviewed Oct 4, 2023

View reviewed changes

add description

abb6b20

flodnv reviewed Oct 4, 2023

View reviewed changes

add more detail description

bec41b8

dukenv0307 commented Oct 4, 2023

View reviewed changes

fix typo

4e75345

flodnv requested changes Oct 6, 2023

View reviewed changes

dukenv0307 and others added 2 commits October 6, 2023 20:09

Update src/libs/actions/BankAccounts.js

c0e74b9

Co-authored-by: Florent De'Neve <florent@expensify.com>

rename the variable

0f48962

flodnv requested changes Oct 9, 2023

View reviewed changes

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

dukenv0307 and others added 2 commits October 9, 2023 16:07

Update src/pages/ReimbursementAccount/ReimbursementAccountPage.js

8d692b0

Co-authored-by: Florent De'Neve <florent@expensify.com>

fix lint

26e9552

flodnv approved these changes Oct 13, 2023

View reviewed changes

flodnv merged commit c9ccae3 into Expensify:main Oct 13, 2023
11 checks passed

OSBotify mentioned this pull request Oct 13, 2023

Deploy Checklist: New Expensify 2023-10-13 #29585

Closed

97 tasks

OSBotify mentioned this pull request Oct 16, 2023

Deploy Checklist: New Expensify 2023-10-16 #29717

Closed

melvin-bot bot mentioned this pull request Oct 16, 2023

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [$1000] Bank account - Phone number field is not trimmed for spaces in connect bank account #25996

Closed

6 tasks

s77rt mentioned this pull request Feb 7, 2024

[HOLD PR#34498] [$500] Bank account - Form is not saved with new changes after coming back to it #35780

Closed

6 tasks

	* @param {String \| undefined} currentStep The step that we need to update the data from BE to draft value
	* @param {String \| undefined} currentStep The step that we need to update the data from backend to draft value

Trim the value when going back on connect bank account #28305

Trim the value when going back on connect bank account #28305

Conversation

dukenv0307 commented Sep 27, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Sep 27, 2023

melvin-bot bot commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ollyws commented Sep 27, 2023 • edited Loading

dukenv0307 commented Sep 27, 2023 • edited Loading

Ollyws commented Sep 27, 2023

Choose a reason for hiding this comment

dukenv0307 Sep 27, 2023 • edited Loading

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

dukenv0307 Sep 27, 2023 • edited Loading

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

Ollyws commented Sep 28, 2023

dukenv0307 commented Sep 29, 2023

flodnv commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dukenv0307 commented Oct 4, 2023

Choose a reason for hiding this comment

Ollyws Oct 4, 2023 • edited Loading

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

Choose a reason for hiding this comment

flodnv Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dukenv0307 commented Oct 13, 2023

flodnv commented Oct 13, 2023

OSBotify commented Oct 13, 2023

OSBotify commented Oct 13, 2023

OSBotify commented Oct 16, 2023

OSBotify commented Oct 16, 2023

OSBotify commented Oct 16, 2023

OSBotify commented Oct 17, 2023

dukenv0307 commented Sep 27, 2023 •

edited

Loading

Ollyws commented Sep 27, 2023 •

edited

Loading

dukenv0307 commented Sep 27, 2023 •

edited

Loading

dukenv0307 Sep 27, 2023 •

edited

Loading

dukenv0307 Sep 27, 2023 •

edited

Loading

Ollyws Oct 4, 2023 •

edited

Loading

flodnv Oct 6, 2023 •

edited

Loading