[InternalQA] Prevent UI change on VBA flow during data fetch #4480
Conversation
src/libs/actions/BankAccounts.js
Outdated
| @@ -330,7 +330,11 @@ function fetchUserWallet() { | |||
| function fetchFreePlanVerifiedBankAccount(stepToOpen) { | |||
| // We are using set here since we will rely on data from the server (not local data) to populate the VBA flow | |||
| // and determine which step to navigate to. | |||
| Onyx.set(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: true}); | |||
| // We temporarily keep the achData state to prevent UI changes while fetching. | |||
There was a problem hiding this comment.
What happens if we do not do this? At what point is it getting overwritten?
There was a problem hiding this comment.
also I think this comment should be placed above the line with achData
There was a problem hiding this comment.
It is overwritten by setting achData.state = lodashGet(achData, 'state', '');
Hmm where does that happen? Sorry I'm not seeing it 🙃
There was a problem hiding this comment.
No problem! Hopefully I can clarify it below:
We are passing achData with the new state to goToWithdrawalAccountSetupStep and then merging it here. Because ...achData is called after ...newACHData, we are overriding the value that was temporarily stored in newACHData.
There was a problem hiding this comment.
Just posted a semi-related issue in #4477 (comment), linking for context in case its the same issue
There was a problem hiding this comment.
@Jag96 Took a quick look, but not sure if they have the same "root cause" besides the local state is reset each time we fetch information about the bank account. One caveat to keep in mind is that we attempted to port this logic over from Web-Secure and retain the functionality we have there - but I'm sure there will be some edge cases to work through as the flows are not exactly the same.
There was a problem hiding this comment.
Gonna approve this since I can't come up with a better suggestion atm and we need to iron out the kinks in this flow. I do think we should create an issue to address future improvements to this flow.
We've run into a few UI bugs and error handling issues related to this stuff so it would be good to spend some time thinking about how to improve things and what the resulting pain points are - since we are likely to run into them again when migrating other code to the new architecture.
|
I created a follow up issue. Feel free to add more context/details to it. |
|
Self-merging as discussed 1:1 with @marcaaron |
|
Hi @luacmartins, any chance you can run the internal QA steps and check this PR off #4554 if it works please? Thanks! |
|
I confirmed the steps in staging! |
Fixed Issues
$ #4467
Tests
PENDINGstate following this SO.Finish Setup, close the modal.Get Startedwhile fetching.QA Steps
Internal QA
Tested On
Screenshots
Web
web.mov
Mobile Web
Desktop
desktop.mov
iOS
Android