-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Improve VBA logic #6230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve VBA logic #6230
Conversation
Gonna leave this on HOLD til the 19th because the changes are significant, but will open for reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Still need to test it though. Left one comment. Great job!
return hasTriedToUpgrade ? CONST.BANK_ACCOUNT.STEP.VALIDATION : CONST.BANK_ACCOUNT.STEP.COMPANY; | ||
} | ||
|
||
return CONST.BANK_ACCOUNT.STEP.ENABLE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the logic hasn't changed, but I feel like we should probably default to the bank account step just to be safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like
if (!shouldGoToEnableStep) {
Log.alert('Unexpectedly sent user to EnableStep', params);
}
return CONST.BANK_ACCOUNT.STEP.ENABLE;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Tests well 👍 |
I did some more testing and have two comments:
|
Fixing the date issue here - #6524. Onfido is still a problem though. |
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. |
From a quick glance, it seems that |
Ah, yes, I see my mistake now. Thanks!!! |
Updated and re-tested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM! The Onfido issue is gone 🎉
As always, great work @marcaaron!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @marcaaron in version: 1.1.17-8 🚀
|
Something is super wrong with this flow.. getting caught in an infinite loop after the Unsure if this is because of these changes or some of the Plaid changes. |
Hmm so I think not. Tried reverting and the issue is still there 😬 |
🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀
|
|
||
// 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}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew! Finally 😂
We should also add a default optimistic errors to prevent #45246
More details in this comment: #45246 (comment)
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
QA Steps
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android