Return to the correct sub step in VBA flow after error in adding Bank Account manually #5186

aldo-expensify · 2021-09-10T00:48:44Z

cc @marcaaron: copying Mark here because he may know the reason why we are copying this value to a local state.

Details

The subStep (reimbursementAccount.achData.subStep) in the VBA flow was being copied from Onyx into a local state in the BankAccountStep component (here). Then, when rendering BankAccountStep, we read the subStep using this.state.bankAccountAddMethod (i.e. here).

The problem with this approach is that we have now to sources of truth for the subStep:

Onyx (reimbursementAccount.achData.subStep)
Local state in the component

This can potentially cause bugs when these states are not synchronized properly.

I this case:

When we click "Connect Manually" in the BankAccountStep, we just update the subStep in the local state (here), Onyx doesn't know about this update.
When we submit the form with the "Routing Number" and "Account Number", we call this method which will end up setting reimbursementAccount.loading to true in Onyx (here)
reimbursementAccount.loading with true value in Onyx will make the loading indicator appear (here), destroying the BankAccountStep and its local state.
Once the loading finishes and the backend returns an error, reimbursementAccount.loading is set to false here
ReimbursementAccountPage removes VBALoadingIndicator and creates BankAccountStep step again.
BankAccountStep copies again the subStep to its local state (constructor called again). The subStep is null at this point.
The user is taken back to the screen where he chooses "Connect Manually" or plaid.

There can be more than one approach to solve this, but here I'll mention two:

Get rid of the multiple sources of truth so we don't have to care about any possible desynchronization
Make sure that Onyx gets updated properly when we update the local state

The implementation I'm trying to do is (1.) because in my experience dealing with states that get copied and have to be updated together makes things more complex, and I prefer to avoid it if possible. It can cause bugs like this that are not so easy to catch and it is hard to see all cases where things could become unsynchronized .

When clearing the subStep from Onyx, I was only able to do so setting it to null. Setting it to undefined didn't do anything.

Fixed Issues

$ #5028

Tests AND QA

Case 1: Invalid routing number error

Navigate to /bank-account/
Click "Connect manually"
Input an invalid routing number, i.e. 12312312344
Input any bank number, i.e. 123
Click "Save & continue" button

Expected results:

We should see the loading indicator (form not visible)
The form showing the "Routing Number" and "Bank number" should appear again with an inline error for the "Routing number".

Case 2: Unverified account error

Create an account using OldDot (enter email, click Join)
Choose "Set up my company for free" (seems like you only get these options on web mobile)
You will be taken to NewDot
Click "Expensify Card", then "Get Started"
In the "Add Bank Account" step (first), click "Connect manually"
Input any routing number / bank number
Click "Save & Continue"

Expected results

We should see the loading indicator (form not visible)
The form appears again with an error at the bottom about the account being unverified.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

The subState in the VBA flow was being copied from Onyx into a local state in the BankAccountStep component and it was read from this local state. This caused that when we update the subState from BankAccounts.js in Onyx, the component BankAccountStep missed the update because it was reading it from the local state.

Withdrawal account data was being loaded from AuthScreens component, and then again from ReiumbursementAccountPage on mount. Moved the code handling the step from route to componentDidUpdate in ReimbursementAccountPage and is not fetching anymore.

…ubstep-after-error

aldo-expensify · 2021-09-10T01:50:53Z

Hey @marcaaron , can you have a look at this? I think this fixes the problem and avoid some unnecessary double fetching of data from the backend, but then maybe I'm missing something.

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

Hey @aldo-expensify, I can take a look at this but I'm having some trouble following the explanation in the details section. Can you try using a bulleted list with links or code snippets (permalinks) to specific things in the code you are calling out?

marcaaron

I took a quick look at this PR. I haven't yet taken the time to understand what it is doing or why but just ran through to give some suggestions on style and convention. Thanks!

src/libs/actions/BankAccounts.js

src/pages/ReimbursementAccount/BankAccountStep.js

aldo-expensify · 2021-09-10T17:31:19Z

Hey @aldo-expensify, I can take a look at this but I'm having some trouble following the explanation in the details section. Can you try using a bulleted list with links or code snippets (permalinks) to specific things in the code you are calling out?

ok, I'll do that!

aldo-expensify · 2021-09-10T18:42:10Z

It was true that the details were a bit cryptic, I had to debug it again to understand what was going on. Hopefully the new explanation is easier to understand.

marcaaron · 2021-09-10T19:35:31Z

Get rid of the multiple sources of truth so we don't have to care about any possible desynchronization

Ok I read the first half of what you've wrote and I think I agree that it would be best to only rely on the achData Onyx to tell us which subStep we should show.

On a side note, I see that if we get to the Company Step, then we close the sidebar, then click again in the Workspace's "Get Started" button, we still start from the beginning. This made me doubt about the purpose of keeping the subStep and currentStep (or any other "step" like information) in Onyx.

We need to know the currentStep since bank accounts are saved in the server with information about which step the user needs to finish completing. If you check out the API layer this part will make more sense. As for the subStep I don't think the API cares about it (but not sure) and it's mostly a way to tell the front end to show the plaid or manual flows.

Are we keeping it locally because we destroy step components when we show the VBALoadingIndicator (here)?

I don't really know for sure to tell you the truth. If I had to guess we did it this way because it's not easy to set just one property of the achData in Onyx and before some recent change the subStep was not null (unless I'm misremembering that this was working fine recently).

As for the second half of this problem... to be honest, I think it should maybe be handled in another PR / issue. If you wait enough time for the requests to all finish then we wouldn't have the issue. So it seems unrelated to the first problem and more related to the fact that we are making an extra http request.

aldo-expensify · 2021-09-10T19:44:28Z

Ok I read the first half of what you've wrote and I think I agree that it would be best to only rely on the achData Onyx to tell us which subStep we should show.

👍

We need to know the currentStep since bank accounts are saved in the server with information about which step the user needs to finish completing. If you check out the API layer this part will make more sense. As for the subStep I don't think the API cares about it (but not sure) and it's mostly a way to tell the front end to show the plaid or manual flows.

Ok, makes sense, I haven't looked into that code yet.

As for the second half of this problem... to be honest, I think it should maybe be handled in another PR / issue. If you wait enough time for the requests to all finish then we wouldn't have the issue. So it seems unrelated to the first problem and more related to the fact that we are making an extra http request.

The double fetch was already there, but it didn't cause any visible problem because of the local state keeping a copy of the subStep, but with my change, it causes visible problems. I was fixing it here because my PR is causing this other problem to appear, but if you prefer, we can split it as you say.

marcaaron · 2021-09-10T22:57:32Z

The double fetch was already there, but it didn't cause any visible problem because of the local state keeping a copy of the subStep, but with my change, it causes visible problems. I was fixing it here because my PR is causing this other problem to appear, but if you prefer, we can split it as you say.

My preference will always be to fix one issue at a time mostly because if the one half of this has a regression then you don't have to roll back 100% of it. But also less complexity makes things move faster.

…ubstep-after-error

aldo-expensify · 2021-09-14T01:00:45Z

Moving the details about the double fetch here to clean up the PR's description. Also, this is not causing problems anymore, so it there it is low priority to do something about it now.

After the change I did to read the subStep always from what we had in Onyx, the following problem started to happen:

trimmed.mov

Basically, if you navigate to /bank-account/new and ""quickly"" click "Connect Manually", you get bounced back to the previous view.

This is happening because:

Fetch withdrawal account information when app starts (here)
Set reimbursementAccount.loading to true in Onyx (here)
ReimbursementAccountPage component is created, in its constructor we fetch again the information for the withdrawal account (here)
The first fetch comes back with the information, we set reimbursementAccount.loading to false (here). The information fetched comes with subStep = null.
The user can see the options "Connect Manually" and "Log into your bank"
Clicking "Connect Manually" sets reimbursementAccount.achData.subStep in Onyx to 'manual' (here, here), we show the form to input "Routing Number" and "Account Number"
The second fetch comes back, again with the subStep = null, overriding the value that the user just set when clicked "Connect Manually". This takes the user back to the first screen.

Ways to solve the second problem:

If we are doing multiple fetches, increment some number somewhere that would invalidate the previous one if a new one is ongoing.
Avoid doing a second fetch

I took the approach of avoiding to do a second fetch.

aldo-expensify · 2021-09-14T01:01:57Z

Update:

Removed the code related to avoiding double fetch of withdrawal account info
Reverted change in code in setupWithdrawalAccount that was causing the issue initially. No more double promise to wait for next tick.
Moved Onyx.merge code from BankAccountStep.js to action BankAccounts.js
Fixed code styling

marcaaron

These changes look good to me, but I'm not 100% sure about the change in BankAccounts.js gonna do some testing and tag in @Jag96 to take a look at my comment.

src/libs/actions/BankAccounts.js

marcaaron

Passes the initial tests for me, but I ran into an issue when performing the following steps:

Add manual account information (as specified in existing test steps)
Verified error message appeared
Navigated back to previous step
Selected the Plaid menu item
Signed into chase test bank
Error still shows..

Selected the savings account option and advanced to the company step.
Pressed to go back to previous step
Manual step shown (I'm not sure if that is expected or we should return to the Plaid screen?)

Enter a valid routing and account number (acct: 011401533 rtg: 1111222233331111)
Advance to company step
Go back to previous step
Enter 11 for account number
We are able to advance to the company step now despite entering an invalid account number

Not sure if these are blockers or if they are happening on main as well. Going to test there next to be sure we don't have any regressions.

marcaaron · 2021-09-23T20:17:10Z

Ok I can repro all that stuff on main so I don't think it has to do with anything here 😄

aldo-expensify · 2021-09-23T20:19:49Z

Ok I can repro all that stuff on main so I don't think it has to do with anything here 😄

😌 , do you want me to have a look at those other problems you found?

marcaaron · 2021-09-23T20:21:59Z

do you want me to have a look at those other problems you found

I would do something like:

Determine if they are blockers or polish for N6
See if we need to look at them urgently or not
See who is available to help or fix them ourselves

marcaaron · 2021-09-23T20:32:51Z

Also can we add test steps to make sure this error is showing?

aldo-expensify · 2021-09-23T20:57:46Z

Also can we add test steps to make sure this error is showing?

Sure, to get there with an unverified account you have to do the OldDot -> NewDot flow, right?

marcaaron · 2021-09-23T21:01:22Z

Yep! Left a comment here about this -> https://github.com/Expensify/Web-Secure/pull/2042#issuecomment-926155516

aldo-expensify · 2021-09-23T21:02:58Z

Yep! Left a comment here about this -> Expensify/Web-Secure#2042 (comment)

Cool, I didn't know exactly how to get there, those steps help :)

github-actions · 2021-09-23T21:28:13Z

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

marcaaron · 2021-09-23T21:28:51Z

Going to CP this but let's make sure to update steps and test the unvalidated flow on staging

…error Return to the correct sub step in VBA flow after error in adding Bank Account manually (cherry picked from commit b05a7a7)

aldo-expensify · 2021-09-23T21:32:59Z

Going to CP this but let's make sure to update steps and test the unvalidated flow on staging

yes yes, sorry for the slowness on that.. I'm still struggling to get from OldDot to NewDot

Following @robertjchen 's PR steps, after "Select Expensify Card option ONLY in WelcomeBusiness inbox task", I see this, what then 🤷

I tried a few options, but I haven't been able to see something that takes me to new Dot. If I just try to load NewDot using localhost:8080, I cannot log in because the account is not verified.

I'm missing how to do the "start the VBA flow" part.

aldo-expensify · 2021-09-23T21:39:59Z

Just saw that if I open it with a small screen (mobile), I see a button that takes me to NewDot :)

aldo-expensify · 2021-09-23T21:53:33Z

Update: Added the "unverified account error" to the test / QA.

OSBotify · 2021-09-23T22:47:32Z

🚀 Cherry-picked to staging by @marcaaron in version: 1.1.1-8 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

kadiealexander · 2021-09-23T23:21:22Z

Update: Only found this mention https://expensify.slack.com/archives/C020EPP9B9A/p1631770824318400?thread_ts=1631562971.004600&cid=C020EPP9B9A where @kadiealexander was asking for assistance on figuring out if they were n6-hold or not.

It was decided that this is a blocker for n6, so a hold won't apply here.

kevinksullivan · 2021-09-23T23:25:09Z

I was able to test this successfully! Can we check it off @applausebot @isagoico ?

OSBotify · 2021-09-23T23:55:48Z

🚀 Deployed to production by @Jag96 in version: 1.1.1-8 🚀

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

OSBotify · 2021-09-24T01:45:44Z

🚀 Deployed to staging by @marcaaron in version: 1.1.1-9 🚀

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

OSBotify · 2021-09-28T19:33:24Z

🚀 Deployed to production by @timszot in version: 1.1.2-7 🚀

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

aldo-expensify added 3 commits September 9, 2021 16:52

Merge branch 'main' of github.com:Expensify/App into aldo_fix-wrong-s…

251ee97

…ubstep-after-error

aldo-expensify marked this pull request as ready for review September 10, 2021 01:49

aldo-expensify requested a review from a team as a code owner September 10, 2021 01:49

aldo-expensify changed the title ~~[WIP] Return to the correct sub step in VBA flow after error in adding Bank Account manually~~ Return to the correct sub step in VBA flow after error in adding Bank Account manually Sep 10, 2021

MelvinBot removed the request for review from a team September 10, 2021 01:50

aldo-expensify requested review from marcaaron and a team September 10, 2021 01:50

MelvinBot requested review from marcochavezf and removed request for a team September 10, 2021 01:50

marcaaron requested changes Sep 10, 2021

View reviewed changes

aldo-expensify changed the title ~~Return to the correct sub step in VBA flow after error in adding Bank Account manually~~ [WIP] Return to the correct sub step in VBA flow after error in adding Bank Account manually Sep 10, 2021

marcaaron assigned aldo-expensify Sep 10, 2021

aldo-expensify added 5 commits September 13, 2021 17:31

Merge branch 'main' of github.com:Expensify/App into aldo_fix-wrong-s…

619e4c6

…ubstep-after-error

Revert Onyx merge separate tick and double fetch

ce5357e

Move Onyx.merge to actions

abb39df

Remove unwanted destructuring

0137eeb

Remove empty line

a8005c8

aldo-expensify changed the title ~~[WIP] Return to the correct sub step in VBA flow after error in adding Bank Account manually~~ Return to the correct sub step in VBA flow after error in adding Bank Account manually Sep 14, 2021

Merge branch 'main' - Resolve conflicts

6bc9c36

marcaaron reviewed Sep 23, 2021

View reviewed changes

src/libs/actions/BankAccounts.js Show resolved Hide resolved

marcaaron self-requested a review September 23, 2021 19:59

marcaaron reviewed Sep 23, 2021

View reviewed changes

marcaaron approved these changes Sep 23, 2021

View reviewed changes

marcaaron added the CP Staging label Sep 23, 2021

marcaaron merged commit b05a7a7 into main Sep 23, 2021

marcaaron deleted the aldo_fix-wrong-substep-after-error branch September 23, 2021 21:28

This was referenced Sep 23, 2021

🍒 Cherry pick PR #5186 to staging 🍒 #5472

Merged

Deploy Checklist: New Expensify 2021-09-21 #5410

Closed

OSBotify mentioned this pull request Sep 23, 2021

Deploy Checklist: New Expensify 2021-09-23 #5477

Closed

27 tasks

OSBotify mentioned this pull request Aug 16, 2022

Deploy Checklist: New Expensify 2022-08-04 #10253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return to the correct sub step in VBA flow after error in adding Bank Account manually #5186

Return to the correct sub step in VBA flow after error in adding Bank Account manually #5186

aldo-expensify commented Sep 10, 2021 •

edited

Loading

aldo-expensify commented Sep 10, 2021

marcaaron commented Sep 10, 2021

marcaaron left a comment

aldo-expensify commented Sep 10, 2021

aldo-expensify commented Sep 10, 2021

marcaaron commented Sep 10, 2021 •

edited

Loading

aldo-expensify commented Sep 10, 2021

marcaaron commented Sep 10, 2021

aldo-expensify commented Sep 14, 2021

aldo-expensify commented Sep 14, 2021 •

edited

Loading

marcaaron left a comment

marcaaron left a comment •

edited

Loading

marcaaron commented Sep 23, 2021

aldo-expensify commented Sep 23, 2021

marcaaron commented Sep 23, 2021

marcaaron commented Sep 23, 2021

aldo-expensify commented Sep 23, 2021

marcaaron commented Sep 23, 2021

aldo-expensify commented Sep 23, 2021

github-actions bot commented Sep 23, 2021

marcaaron commented Sep 23, 2021

aldo-expensify commented Sep 23, 2021 •

edited

Loading

aldo-expensify commented Sep 23, 2021

aldo-expensify commented Sep 23, 2021

OSBotify commented Sep 23, 2021

kadiealexander commented Sep 23, 2021

kevinksullivan commented Sep 23, 2021

OSBotify commented Sep 23, 2021

OSBotify commented Sep 24, 2021

OSBotify commented Sep 28, 2021

Return to the correct sub step in VBA flow after error in adding Bank Account manually #5186

Return to the correct sub step in VBA flow after error in adding Bank Account manually #5186

Conversation

aldo-expensify commented Sep 10, 2021 • edited Loading

Details

Fixed Issues

Tests AND QA

Case 1: Invalid routing number error

Case 2: Unverified account error

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

aldo-expensify commented Sep 10, 2021

marcaaron commented Sep 10, 2021

marcaaron left a comment

Choose a reason for hiding this comment

aldo-expensify commented Sep 10, 2021

aldo-expensify commented Sep 10, 2021

marcaaron commented Sep 10, 2021 • edited Loading

aldo-expensify commented Sep 10, 2021

marcaaron commented Sep 10, 2021

aldo-expensify commented Sep 14, 2021

aldo-expensify commented Sep 14, 2021 • edited Loading

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron left a comment • edited Loading

Choose a reason for hiding this comment

marcaaron commented Sep 23, 2021

aldo-expensify commented Sep 23, 2021

marcaaron commented Sep 23, 2021

marcaaron commented Sep 23, 2021

aldo-expensify commented Sep 23, 2021

marcaaron commented Sep 23, 2021

aldo-expensify commented Sep 23, 2021

github-actions bot commented Sep 23, 2021

marcaaron commented Sep 23, 2021

aldo-expensify commented Sep 23, 2021 • edited Loading

aldo-expensify commented Sep 23, 2021

aldo-expensify commented Sep 23, 2021

OSBotify commented Sep 23, 2021

kadiealexander commented Sep 23, 2021

kevinksullivan commented Sep 23, 2021

OSBotify commented Sep 23, 2021

OSBotify commented Sep 24, 2021

OSBotify commented Sep 28, 2021

aldo-expensify commented Sep 10, 2021 •

edited

Loading

marcaaron commented Sep 10, 2021 •

edited

Loading

aldo-expensify commented Sep 14, 2021 •

edited

Loading

marcaaron left a comment •

edited

Loading

aldo-expensify commented Sep 23, 2021 •

edited

Loading