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

[$250] Desktop -BA - Bank account page doesn't load after redirecting from desktop #55241

Open
1 of 8 tasks
IuliiaHerets opened this issue Jan 14, 2025 · 6 comments
Open
1 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Jan 14, 2025

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: v9.0.85-1
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5454828&group_by=cases:section_id&group_order=asc&group_id=283225
Issue reported by: Applause Internal Team
Device used: Mac 15.2
App Component: Workspace Settings

Action Performed:

  1. Open the app
    2.Go to Settings > WS > Workflow > Connect Bank account
    3.Click on "Note: To connect with Chase, Wells Fargo, Capital One or Bank of America, please click here to complete this process in a browser."

Expected Result:

The bank account page should open in a new browser or tab when trying to add a bank account via Desktop app.

Actual Result:

Bank account page doesn't load after redirecting from desktop.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6714196_1736881182130.Untitled.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021879312300881229146
  • Upwork Job ID: 1879312300881229146
  • Last Price Increase: 2025-01-14
Issue OwnerCurrent Issue Owner: @eVoloshchak
@IuliiaHerets IuliiaHerets added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Jan 14, 2025
Copy link

melvin-bot bot commented Jan 14, 2025

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@jliexpensify jliexpensify added the External Added to denote the issue can be worked on by a contributor label Jan 14, 2025
Copy link

melvin-bot bot commented Jan 14, 2025

Job added to Upwork: https://www.upwork.com/jobs/~021879312300881229146

@melvin-bot melvin-bot bot changed the title Desktop -BA - Bank account page doesn't load after redirecting from desktop [$250] Desktop -BA - Bank account page doesn't load after redirecting from desktop Jan 14, 2025
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 14, 2025
Copy link

melvin-bot bot commented Jan 14, 2025

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

@jliexpensify
Copy link
Contributor

Been chatting to Stites about this, as he wrote the design doc for adding VBA's to ND. He mentioned this seems like a general bug, not tied to a project, and can be worked on by contributors.

@bernhardoj
Copy link
Contributor

bernhardoj commented Jan 15, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-01-15 06:12:37 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

BA page shows infinite loading view when redirected from the desktop.

What is the root cause of that problem?

In our case, when redirected from the desktop app, the reimbursementAccount?.isLoading is stuck at true.

const isLoading =
(!!isLoadingApp || !!account?.isLoading || (reimbursementAccount?.isLoading && !reimbursementAccount?.isCreateCorpayBankAccount)) &&
(!plaidCurrentEvent || plaidCurrentEvent === CONST.BANK_ACCOUNT.PLAID.EVENTS_NAME.EXIT);

If we see the open reimbursement onyx data,

function openReimbursementAccountPage(stepToOpen: ReimbursementAccountStep, subStep: ReimbursementAccountSubStep, localCurrentStep: ReimbursementAccountStep, policyID: string) {
const onyxData: OnyxData = {
optimisticData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.REIMBURSEMENT_ACCOUNT,
value: {
isLoading: true,
},
},
],
successData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.REIMBURSEMENT_ACCOUNT,
value: {
isLoading: false,
},
},
],
failureData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.REIMBURSEMENT_ACCOUNT,
value: {
isLoading: false,
},
},
],
};

we already set it to false either success or failure. But in this case, openReimbursementAccountPage is never called. So, how can reimbursementAccount?.isLoading becomes true?

It's set to true whenever the page is mounted.

useEffect(() => {
if (isPreviousPolicy) {
return;
}
BankAccounts.setReimbursementAccountLoading(true);
ReimbursementAccount.clearReimbursementAccountDraft();
// If the step to open is empty, we want to clear the sub step, so the connect option view is shown to the user
const isStepToOpenEmpty = getStepToOpenFromRouteParams(route) === '';
if (isStepToOpenEmpty) {
BankAccounts.setBankAccountSubStep(null);
BankAccounts.setPlaidEvent(null);
}
fetchData();
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps

It was first added by this old PR. At that time, openReimbursementAccountPage will always be called. So, there is no problem.

But in #52812, we prevent calling openReimbursementAccountPage if the policyID param is undefined.

if (policyIDParam) {
BankAccounts.openReimbursementAccountPage(stepToOpen, subStep, localCurrentStep, policyIDParam);
}

Thus, reimbursementAccount?.isLoading is stuck at true.

But why policyID param is undefined? When we build the route, we already include the policyID.

const bankAccountRoute = `${ROUTES.BANK_ACCOUNT_WITH_STEP_TO_OPEN.getRoute('new', policyID, ROUTES.WORKSPACE_INITIAL.getRoute(policyID))}`;

But we open the link using openExternalLinkWithToken which will append the link to the exitTo param.

<TextLink onPress={() => Link.openExternalLinkWithToken(bankAccountRoute)}>{translate(plaidDesktopMessage)}</TextLink>

function buildURLWithAuthToken(url: string, shortLivedAuthToken?: string) {
const authTokenParam = shortLivedAuthToken ? `shortLivedAuthToken=${shortLivedAuthToken}` : '';
const emailParam = `email=${encodeURIComponent(currentUserEmail)}`;
const exitTo = `exitTo=${url}`;
const accountID = `accountID=${currentUserAccountID}`;
const referrer = 'referrer=desktop';
const paramsArray = [accountID, emailParam, authTokenParam, exitTo, referrer];
const params = paramsArray.filter(Boolean).join('&');
return `${CONFIG.EXPENSIFY.NEW_EXPENSIFY_URL}transition?${params}`;
}

We previously using openExternalLink which just opens the link as it is, but it was changed in #44517.

Since the exitTo is a param and the link contains another param (nested param) and we don't encode it, the nested param isn't recognized as part of the exitTo link.

What changes do you think we should make in order to solve the problem?

  1. Encode the link that we want to open. This will fix the missing policyID
    <TextLink onPress={() => Link.openExternalLinkWithToken(bankAccountRoute)}>{translate(plaidDesktopMessage)}</TextLink>

(or we can do it inside buildURLWithAuthToken)

  1. The above is enough for our case, but if the user accesses the page manually without policyID, then the issue will happen again. To fix it,
    we can either allow the API call with an undefined policyID,
    if (policyIDParam) {
    BankAccounts.openReimbursementAccountPage(stepToOpen, subStep, localCurrentStep, policyIDParam);
    }

or

only set the loading immediately if there is a policyID

BankAccounts.setReimbursementAccountLoading(true);

or remove it at all. If we see the reason why it was added, it was mentioned optimisticData might be set later if there are multiple calls in the queue, but that's not true today because optimisticData is always merged first without waiting on anything.

// Show loader right away, as optimisticData might be set only later in case multiple calls are in the queue

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We can probably test rendering the ReimbursementAccountPage without policyID and verify the loading page won't show infinitely.

Copy link
Contributor

⚠️ @bernhardoj Thanks for your proposal. Please update it to follow the proposal template, as proposals are only reviewed if they follow that format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

4 participants