Improve VBA logic #6230

marcaaron · 2021-11-05T01:37:50Z

Details

Clean up for Free Plan - improving the VBA API logic and breaking things down into more digestible parts

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/173363

Tests

Automated tests should cover some of the changes. The logic should not have changed.
We should run through the VBA flow once to be sure there are no regressions

QA Steps

Regression test for adding a bank account via New Expensify

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

…tore

marcaaron · 2021-11-11T15:40:08Z

Gonna leave this on HOLD til the 19th because the changes are significant, but will open for reviews.

luacmartins

LGTM! Still need to test it though. Left one comment. Great job!

luacmartins · 2021-11-17T16:40:52Z

src/libs/actions/ReimbursementAccount/fetchFreePlanVerifiedBankAccount.js

+        return hasTriedToUpgrade ? CONST.BANK_ACCOUNT.STEP.VALIDATION : CONST.BANK_ACCOUNT.STEP.COMPANY;
+    }
+
+    return CONST.BANK_ACCOUNT.STEP.ENABLE;


I see the logic hasn't changed, but I feel like we should probably default to the bank account step just to be safe.

I'm unsure about defaulting to the BankAccountStep because if we miss a case where the user should be passed to the enable step then they'll get stuck and we won't might not realize that we missed this case.

Maybe we can leave the logic intact and log an alert when we end up navigating to this step for an unexpected reason?

Something like

if (!shouldGoToEnableStep) { Log.alert('Unexpectedly sent user to EnableStep', params); } return CONST.BANK_ACCOUNT.STEP.ENABLE;

Sounds good!

Ah actually, thought about it some more and I think the only possibility is that the bank account is not in an open state, but we are already returning early above when we don't have a bank account and it is not in an open state. So I think it might just be redundant to do anything else.

Can you think of a case where we end up here and it would not be safe to go to the enable step?

Maybe it would be best to wait for that day to arrive...

I can't think of any. Given that this would be a rare edge case, if it exists at all, I'm ok keeping it as is.

luacmartins · 2021-11-17T17:27:40Z

Tests well 👍

luacmartins · 2021-11-25T19:58:51Z

I did some more testing and have two comments:

I'm hitting a wall when going through the flow. When I use the information to trigger the Onfido step, I somehow always get redirected back to the Personal information step. That is working well on main so maybe something being introduced here.
Another thing that I noticed is that the Incorporation date and DOB fields are no longer being populated with the draft, but that's also the case on main. I think it's because the value stored in draft might be in the wrong format, e.g. "1990-01-01T07:00:00.000Z"

luacmartins · 2021-11-30T00:54:04Z

Fixing the date issue here - #6524. Onfido is still a problem though.

marcaaron · 2021-11-30T01:39:04Z

I'm hitting a wall when going through the flow. When I use the information to trigger the Onfido step, I somehow always get redirected back to the Personal information step

Ok I'll need to check this again after the Plaid OAuth stuff is done. Or maybe you have some ideas about why the redirect happens.

luacmartins · 2021-11-30T21:38:40Z

From a quick glance, it seems that sdkToken is not being set in achData so we never render the Onfido component.

marcaaron · 2021-11-30T22:36:13Z

From a quick glance, it seems that sdkToken is not being set in achData so we never render the Onfido component.

Ah, yes, I see my mistake now. Thanks!!!

marcaaron · 2021-11-30T22:39:34Z

Updated and re-tested

luacmartins

Code LGTM! The Onfido issue is gone 🎉
As always, great work @marcaaron!

OSBotify · 2021-12-01T15:38:53Z

✋ 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-12-07T00:21:14Z

🚀 Deployed to staging by @marcaaron in version: 1.1.17-8 🚀

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

marcaaron · 2021-12-07T20:45:54Z

Regression test for adding a bank account via New Expensify

Something is super wrong with this flow.. getting caught in an infinite loop after the CompanyStep and looking into it now.

Unsure if this is because of these changes or some of the Plaid changes.

marcaaron · 2021-12-07T21:05:27Z

Hmm so I think not. Tried reverting and the issue is still there 😬

OSBotify · 2021-12-08T20:13:03Z

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

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

hungvu193 · 2024-08-09T07:42:24Z

src/libs/actions/ReimbursementAccount/validateBankAccount.js

+
+            // We are generically showing any other backend errors that might pop up in the validate step
+            errors.showBankAccountErrorModal(response.message);
+            Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: false});


Phew! Finally 😂
We should also add a default optimistic errors to prevent #45246
More details in this comment: #45246 (comment)

marcaaron added 10 commits November 3, 2021 14:18

Break up BankAccounts.js into smaller modules and make Plaid a mini-s…

430ba0f

…tore

Start breaking ReimbursementAccount up

7a91e72

Break down more

304ae70

Move large methods into separate files

a7f11cf

improve condtional logic for picking out step

19e753e

add unit test

4da297e

add unit test

7dea38d

Fixup setupWithdrawalAccount

447634d

Rename BeneficialOwners to ACHContractStep

1b12513

Add some comments

f6a4c60

marcaaron self-assigned this Nov 5, 2021

marcaaron added 4 commits November 5, 2021 17:45

fix broken stuff

97e7109

fix conflicts

d0c7013

add Network.setIsReady() check

c51e95d

fix conflicts

4063e1d

marcaaron changed the title ~~[WIP][HOLD] Improve VBA logic~~ [HOLD] Improve VBA logic Nov 11, 2021

marcaaron marked this pull request as ready for review November 11, 2021 15:40

marcaaron requested a review from a team as a code owner November 11, 2021 15:40

MelvinBot requested review from joelbettner and removed request for a team November 11, 2021 15:40

luacmartins self-requested a review November 12, 2021 14:06

marcaaron changed the title ~~[HOLD] Improve VBA logic~~ [HOLD 11/19] Improve VBA logic Nov 12, 2021

luacmartins reviewed Nov 17, 2021

View reviewed changes

marcaaron changed the title ~~[HOLD 11/19] Improve VBA logic~~ Improve VBA logic Nov 22, 2021

Merge remote-tracking branch 'origin' into marcaaron-vbaRefactor

9dccb85

luacmartins previously approved these changes Nov 23, 2021

View reviewed changes

marcaaron added 2 commits November 24, 2021 11:25

fix conflicts

bd5be64

Fix lint errors

bc1a7d6

fix typo

6d01778

marcaaron dismissed luacmartins’s stale review via 6d01778 November 24, 2021 21:58

Merge remote-tracking branch 'origin' into marcaaron-vbaRefactor

adf5f13

use sdkToken not token

8f5a450

luacmartins approved these changes Nov 30, 2021

View reviewed changes

marcaaron merged commit 0360fd7 into main Dec 1, 2021

marcaaron deleted the marcaaron-vbaRefactor branch December 1, 2021 15:37

OSBotify mentioned this pull request Dec 6, 2021

Deploy Checklist: New Expensify 2021-12-06 #6605

Closed

95 tasks

marcaaron mentioned this pull request Dec 7, 2021

VBA - Continuous loading spinner appears after company information page #6620

Closed

mananjadhav mentioned this pull request Nov 18, 2022

PAID [$500] Web - Workspace - After deleting a bank account, workspace options remain available initially #11512

Closed

mateuuszzzzz mentioned this pull request Mar 6, 2024

[TS migration] Migrate 'ReimbursementAccount' to TypeScript #36495

Merged

55 tasks

hungvu193 reviewed Aug 9, 2024

View reviewed changes

hungvu193 mentioned this pull request Aug 9, 2024

[HOLD for payment 2024-08-09] [$250] Bank account - No error message is displayed for incorrect validation amounts #45246

Closed

6 tasks

Improve VBA logic #6230

Improve VBA logic #6230

Uh oh!

Conversation

marcaaron commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

marcaaron commented Nov 11, 2021

Uh oh!

luacmartins left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luacmartins Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

marcaaron Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

marcaaron Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

luacmartins Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

marcaaron Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

luacmartins Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

luacmartins commented Nov 17, 2021

Uh oh!

luacmartins commented Nov 25, 2021

Uh oh!

luacmartins commented Nov 30, 2021

Uh oh!

marcaaron commented Nov 30, 2021

Uh oh!

luacmartins commented Nov 30, 2021

Uh oh!

marcaaron commented Nov 30, 2021

Uh oh!

marcaaron commented Nov 30, 2021

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Dec 1, 2021

Uh oh!

OSBotify commented Dec 7, 2021

Uh oh!

marcaaron commented Dec 7, 2021

Uh oh!

marcaaron commented Dec 7, 2021

Uh oh!

OSBotify commented Dec 8, 2021

Uh oh!

hungvu193 Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcaaron commented Nov 5, 2021 •

edited

Loading

luacmartins left a comment •

edited

Loading