Do not always clear Onyx VBA data when accessing workspace sections #5771

Gonals · 2021-10-12T15:11:18Z

@MonilBhavsar, @tgolen

I'm not super happy with this solution, so I'm open to suggestions 😬

Fixed Issues

$ #5760

Tests/ QA Steps

On NewDot, Log into an account with no VBA in its free workspace.
Open the workspace and add an OPEN account.
Go to the Workspace->Pay Bills. You should immediately see the option to pay the bills:

Tested On

Gonals · 2021-10-12T15:12:55Z

src/libs/actions/BankAccounts.js

+                    // one on the server side, let's make sure we use just the server-side data.
+                    if (hasLocalOpenAccount && lodashGet(achData, 'state', '') !== BankAccount.STATE.OPEN) {
+                        Onyx.set(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: false, error: ''});
+                        reimbursementAccountInSetup = {};


When we do Onyx.set(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: false, error: ''}); on line 348, it happens before reimbursementAccountInSetup is set, so it never gets populated. In this case, we need to clear it manually.

I think my first question is.... why are we using Onyx.set() here and above instead of Onyx.merge()? If there is no reason for it, then I think we should just use merge() in both places and then I don't think you need any of the changes to the signature of fetchFreePlanVerifiedBankAccount()

I think we should just use merge() in both places

Correction: I mean, I think we can just use merge() above, and that's the only change this PR needs to make.

If we use merge and the bank account has been deleted on the server side (but still remains in Onyx), it will remain in Oxyx instead of getting cleared out. Handling this situation was the annoying part of the issue 😬

How often would we expect that case to actually happen? Is that really a valid flow?

I don't expect it to happen often at all. It can easily happen by deleting the bank account from oldDot, though.
Even not counting that flow, though I am not sure about using merge all the time. This function is used in bank account setup and I worry using merge in that flow will cause unwanted behavior (after all, the comments do mention we are using set specifically and consciously.

If we don't worry about the deleted bank account situation, though, we could get rid of this part and just have the set/merge split at the top.

Yeah, I saw the comment for the original set:

We are using set here since we will rely on data from the server (not local data) to populate the VBA flow

I don't know that this is really the desired behavior. This is making the app the opposite of offline-first. I think we should be fine rendering whatever is in Onyx, and then load new data on top of whatever is already there. Thinking about the setup flow, if the UI was showing what was in Onyx already, then why wouldn't it be OK to keep showing it when you're offline? I would try merge() and just verify it doesn't change anything about the VBA flow.

Regarding the delete account flow, I spoke with you 1:1 to make sure I had the full context around that and I don't think it is a flow we should worry about right now because it's a very edge case. The proper solution for handling the bank account deletion is to implement a pusher event just like we do when a new action is added to a report.

Yep. I have been doing some testing on the VBA flow with merge and I haven't noticed any issues/differences. Let's move ahead with it, then!

github-actions · 2021-10-12T16:29:36Z

⚠️ ⚠️ 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-10-12T17:26:49Z

I'm trying to think of a way we can do this without altering the logic in a flow that is mostly working OK. This change makes me a little uncomfortable as there may be other places that depend on the "reset" logic.

marcaaron · 2021-10-12T17:29:32Z

Another way to look at this problem is that we are displaying options while we are still "loading" some information we need to determine which option to show. So, a safer move at this stage of the game might be to hold showing these option until loading is finished.

Gonals · 2021-10-12T21:20:08Z

Another way to look at this problem is that we are displaying options while we are still "loading" some information we need to determine which option to show. So, a safer move at this stage of the game might be to hold showing these option until loading is finished.

You can take a look at my previous commit. You might like it better :)

marcaaron · 2021-10-12T21:40:31Z

The solution I had in mind is to only render the children if we have reimbursementAccount.loading == false

App/src/pages/workspace/WorkspacePageWithSections.js

Line 72 in 24cc6ff

{this.props.children(hasVBA, policyID, isUsingECard)}

Otherwise we are passing hasVBA with a value that might not be correct until we are done loading.

tgolen · 2021-10-12T21:52:39Z

I think this boils down to:

Can we display something in Onyx that may or may not be stale?

I think that almost everywhere else in the app we have decided that this is OK to do that. We show what's in Onyx, and then we update it silently in the background (think of the network reconnection logic).

So, there isn't anything about this case which makes me think that we should treat it differently, but this is possibly where I am open to reason.

marcaaron · 2021-10-12T22:22:14Z

I think that almost everywhere else in the app we have decided that this is OK to do that
So, there isn't anything about this case which makes me think that we should treat it differently, but this is possibly where I am open to reason.

Only thing I can add is that this feature was not implemented with that idea in mind and the interactions with local data and API are fairly complex in comparison to other simpler flows in the app. That's not to say we can't change how it works, but I think that's something that should have been established in the design stage.

marcaaron · 2021-10-12T23:03:34Z

Did a quick test and found a couple of things that aren't working correctly. I'll continue testing and help find other issues if these are not decided to be blockers or we want to find solutions to them.

Issue 1 - Unexpectedly dropped into the company step flow

Repro steps:

Create a new account + workspace
Select Plaid option
Advance to the Company Step
Bail on the flow, refresh the page, and reopen it again

Expected:
User is brought back to the BankAccountStep since a bank account is not officially created until they get past the CompanyStep

Actual:
User lands in the CompanyStep again

Issue 2 - Error messages persist on page refresh

Repro steps:

Create a new account + workspace
Select Plaid option
Advance to the Company Step + submit with no information
Bail on the flow, refresh the page, and reopen it again

Expected:
Next time user lands on this flow error messages are cleared

Actual:
Error messages persist page refresh

marcaaron

Not sure about the first issue found above. Maybe it's not a blocker if everything works OK? I only tested that the behavior changed. But I think the errors should clear if we are visiting a flow again so I would request we come up with a solution to that new problem introduced.

marcaaron · 2021-10-13T00:35:32Z

src/libs/actions/BankAccounts.js

-    // 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, error: ''});
+    Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: true, error: ''});


Kinda want to propose a less risky change here. Here's how it goes...

Since the /bank-account flow may be expecting multiple fields to be "reset" before we fetch the bank account then we should try to only carry over the state that we need for now.

I think to solve the current issue we could do something like:

const initialData = {loading: true, error: ''}; const state = lodashGet(reimbursementAccountInSetup, 'achData.state'); if (state === BankAccount.STATE.OPEN) { initialData.achData = {state: BankAccount.STATE.OPEN}; } Onyx.set(ONYXKEYS.REIMBURSEMENT_ACCOUNT, initialData);

I think this is a pretty good solution, as it shouldn't affect the add bank account flow, which was my main worry with the current one. However, it won't work as-is. At this point, reimbursementAccountInSetup is still empty, so there's no state there.
We can either pass a flag from WorspacePageWithSections, as I was doing before, or do an Onyx.connect. I think the first option might be better, but I can go for either. Any preference @marcaaron?

I've pushed changes for solution #1 (with the flag). LMK what you think

Ah bummer sounds like a good case for Onyx.isInitialized() if it existed 😄

src/libs/actions/BankAccounts.js

Do not always clear Onyx VBA data when accessing workspace sections (cherry picked from commit d61248f)

OSBotify · 2021-10-13T19:03:54Z

🚀 Cherry-picked to staging by @marcaaron in version: 1.1.7-7 🚀

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.

OSBotify · 2021-10-15T05:48:23Z

🚀 Deployed to production by @AndrewGable in version: 1.1.7-24 🚀

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

OSBotify · 2021-10-15T06:40:16Z

🚀 Deployed to staging by @marcaaron in version: 1.1.7-25 🚀

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

OSBotify · 2021-10-23T00:42:38Z

🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀

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

Do not always clear Onyx VBA data when accessing workspace sections

aca6c23

Gonals requested a review from tgolen October 12, 2021 15:11

Gonals self-assigned this Oct 12, 2021

Gonals requested a review from a team as a code owner October 12, 2021 15:11

MelvinBot requested review from MonilBhavsar and removed request for a team October 12, 2021 15:11

Gonals commented Oct 12, 2021

View reviewed changes

Gonals added 2 commits October 12, 2021 18:15

Simplify solution

07001fa

remove extra line

48345e7

tgolen previously approved these changes Oct 12, 2021

View reviewed changes

tgolen requested review from marcaaron and removed request for MonilBhavsar October 12, 2021 16:29

tgolen added the CP Staging label Oct 12, 2021

marcaaron requested changes Oct 12, 2021

View reviewed changes

marcaaron reviewed Oct 13, 2021

View reviewed changes

Keep state in onyx

fee575e

Gonals dismissed tgolen’s stale review via fee575e October 13, 2021 14:26

marcaaron reviewed Oct 13, 2021

View reviewed changes

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

Pass state instead

ab5acf4

marcaaron approved these changes Oct 13, 2021

View reviewed changes

marcaaron merged commit d61248f into main Oct 13, 2021

marcaaron deleted the alberto-workspaceSections branch October 13, 2021 17:43

github-actions bot pushed a commit that referenced this pull request Oct 13, 2021

Merge pull request #5771 from Expensify/alberto-workspaceSections

a0689e4

Do not always clear Onyx VBA data when accessing workspace sections (cherry picked from commit d61248f)

This was referenced Oct 13, 2021

🍒 Cherry pick PR #5771 to staging 🍒 #5809

Merged

Deploy Checklist: New Expensify 2021-10-07 #5718

Closed

kavimuru mentioned this pull request Oct 14, 2021

VBA - Error message for manually entered address in the field in company info and unable to proceed #5841

Closed

OSBotify mentioned this pull request Oct 15, 2021

Deploy Checklist: New Expensify 2021-10-15 #5879

Closed

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

Do not always clear Onyx VBA data when accessing workspace sections #5771

Do not always clear Onyx VBA data when accessing workspace sections #5771

Gonals commented Oct 12, 2021 •

edited

Loading

Gonals Oct 12, 2021

tgolen Oct 12, 2021

tgolen Oct 12, 2021

Gonals Oct 12, 2021 •

edited

Loading

tgolen Oct 12, 2021

Gonals Oct 12, 2021 •

edited

Loading

tgolen Oct 12, 2021

Gonals Oct 12, 2021

github-actions bot commented Oct 12, 2021

marcaaron commented Oct 12, 2021

marcaaron commented Oct 12, 2021

Gonals commented Oct 12, 2021

marcaaron commented Oct 12, 2021

tgolen commented Oct 12, 2021

marcaaron commented Oct 12, 2021

marcaaron commented Oct 12, 2021

marcaaron left a comment

marcaaron Oct 13, 2021

Gonals Oct 13, 2021 •

edited

Loading

Gonals Oct 13, 2021

marcaaron Oct 13, 2021

OSBotify commented Oct 13, 2021

OSBotify commented Oct 15, 2021

OSBotify commented Oct 15, 2021

OSBotify commented Oct 23, 2021

Do not always clear Onyx VBA data when accessing workspace sections #5771

Do not always clear Onyx VBA data when accessing workspace sections #5771

Conversation

Gonals commented Oct 12, 2021 • edited Loading

Fixed Issues

Tests/ QA Steps

Tested On

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gonals Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gonals Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 12, 2021

marcaaron commented Oct 12, 2021

marcaaron commented Oct 12, 2021

Gonals commented Oct 12, 2021

marcaaron commented Oct 12, 2021

tgolen commented Oct 12, 2021

marcaaron commented Oct 12, 2021

marcaaron commented Oct 12, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gonals Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Oct 13, 2021

OSBotify commented Oct 15, 2021

OSBotify commented Oct 15, 2021

OSBotify commented Oct 23, 2021

Gonals commented Oct 12, 2021 •

edited

Loading

Gonals Oct 12, 2021 •

edited

Loading

Gonals Oct 12, 2021 •

edited

Loading

Gonals Oct 13, 2021 •

edited

Loading