Skip to content
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

Workspace - Unlock feature is briefly visible after adding a bank account and unlocking all features #5760

Closed
isagoico opened this issue Oct 11, 2021 · 13 comments
Assignees
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering Hourly KSv2

Comments

@isagoico
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Found while QAing #5642

Action Performed:

  1. Navigate to a workspace in a account that does not have a bank account added
  2. Check all the options and verify you're blocked from most of them until you add a bank account
  3. Add a fully verified bank account
  4. Navigate to the same options you were blocked before.

Expected Result:

Features should be unblocked and no visual issue should be present.

Actual Result:

The blocked feature text is visible for a brief moment.

Workaround:

Unknown .

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.7-3

Reproducible in staging?: yes
Reproducible in production?: no

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Screen_Recording_20211011-152409_New.Expensify.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause
Slack conversation:

View all open jobs on GitHub

@isagoico isagoico added DeployBlockerCash This issue or pull request should block deployment Engineering Daily KSv2 labels Oct 11, 2021
@MelvinBot
Copy link

Triggered auto assignment to @Gonals (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@github-actions github-actions bot added Hourly KSv2 and removed Daily KSv2 labels Oct 11, 2021
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@tgolen
Copy link
Contributor

tgolen commented Oct 12, 2021

Ah, I know why this happens. It's because in order to make the pages available even when you're offline, they are showing whatever data is in Onyx first, then calling the API to load the latest bank account information, then the page will update with the new data. This all happens here:

fetchFreePlanVerifiedBankAccount();

The alternative was to show a loading indicator while the API call is being done, but then that wouldn't work in offline mode (ie. you'd see a loading indicator until the app came back online).

What we should possibly consider doing is calling fetchFreePlanVerifiedBankAccount() at the very end of the banking flow (I'm not very sure where this happens, but @marcaaron knows). That way, by the time the API call is finished, the data will have loaded into Onyx before someone goes to those other pages.

@tgolen
Copy link
Contributor

tgolen commented Oct 12, 2021

Oh, another idea instead of making multiple API calls is to just ensure that reimbursementAccount.achData.state is 'OPEN' in Onyx at the end of the VBA flow.

@kevinksullivan
Copy link
Contributor

@tgolen I noticed you mention this is tied in with offline mode, where we're seeing a similar issue, so figured I'd drop it here to start. After enabling the cards I will see Cards are ready! and all correct copy, but if I go offline, the page will flip back to Unlock free expensify cards, and all pages will show copy as if I don't have the card.

image

Is this problem/potential solution related to the issue we're seeing here on loading the pages so they show correct copy? Or do you suggest we get this in a separate issue?

@Gonals
Copy link
Contributor

Gonals commented Oct 12, 2021

WorkspacePageWithSections.js

I think the issue is a bit different. This doesn't happen the first time those sections are open after adding the bank account. It happens every time.

The cause seems to be this line in fetchFreePlanVerifiedBankAccount. We reset the status every time in Onyx for a little bit, when the function is called, so the display changes for a bit.

I think we can fix this simply by checking ONYX first and, if we already have an OPEN account there, not calling fetchFreePlanVerifiedBankAccount. I'm pretty sure we set the status correctly at the end of the process, so it should be fine.

I'll do some testing with that and see if it causes any issues when the bank account is deleted from oldDot.

@Gonals
Copy link
Contributor

Gonals commented Oct 12, 2021

WorkspacePageWithSections.js

I think the issue is a bit different. This doesn't happen the first time those sections are open after adding the bank account. It happens every time.

The cause seems to be this line in fetchFreePlanVerifiedBankAccount. We reset the status every time in Onyx for a little bit, when the function is called, so the display changes for a bit.

I think we can fix this simply by checking ONYX first and, if we already have an OPEN account there, not calling fetchFreePlanVerifiedBankAccount. I'm pretty sure we set the status correctly at the end of the process, so it should be fine.

I'll do some testing with that and see if it causes any issues when the bank account is deleted from oldDot.

Yep, as I expected, we get issues if the account gets deleted in oldDot. The behavior remains as if the account were open until we refresh.
Trying something else after lunch!

@Gonals
Copy link
Contributor

Gonals commented Oct 12, 2021

Fix in place! Do we still consider this a deployblocker? It is a visual bug, but it doesn't break stuff.

@marcaaron
Copy link
Contributor

Oh, another idea instead of making multiple API calls is to just ensure that reimbursementAccount.achData.state is 'OPEN' in Onyx at the end of the VBA flow.

We do this already I think and it should happen after BankAccount_Validate. There's some logic here:

const reimbursementAccount = {
loading: false,
error: '',
achData: {state: BankAccount.STATE.OPEN},
};
if (isUsingExpensifyCard) {
Navigation.dismissModal();
} else {
reimbursementAccount.achData.currentStep = CONST.BANK_ACCOUNT.STEP.ENABLE;
}
Onyx.merge(ONYXKEYS.USER, {isUsingExpensifyCard});
Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, reimbursementAccount);

@marcaaron
Copy link
Contributor

Could we maybe just hide the options until !reimbursementAccount.loading when we fetch the bank account here?

fetchFreePlanVerifiedBankAccount();

@tgolen
Copy link
Contributor

tgolen commented Oct 12, 2021

Ah, yeah, we could. I think the solution in Alberto's PR is a pretty solid one though.

@marcaaron
Copy link
Contributor

It's not bad and maybe even the longer term approach we want to go with. But it makes my stomach hurt a little since there are many things that might be expecting that data to be reset each time we fetch it. As long as we test heavily we should be good.

Mainly proposing the alternative solution because I can guarantee there will be no unintended side effects.

@tgolen
Copy link
Contributor

tgolen commented Oct 12, 2021

OK, got it. Makes sense. I think the testing has been pretty thorough so far, so I have pretty high confidence that we should be able to catch any issues in the flow if they are introduced.

@botify botify closed this as completed Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering Hourly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants