[HOLD for payment 2024-05-06] [$500] Connect manually - Connect bank account page does not appear when kill app

izarutskaya commented 9 months ago

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

Version Number: 1.4.12-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause-Internal Team Slack conversation: @

Action Performed:

Go to Settings> Workspace> Bank Account> Connect manually
Kill the app and reopen
Go to Settings> Workspace> Bank Account

Expected Result:

When tap on Bank account user should be navigated to Connect bank account page

Actual Result:

Connect bank account page where user can select between "Connect online with Plate or "Connect manually" does not appear. User directly navigates to Connect manually

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android: Native
[ ] Android: mWeb Chrome
[x] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/a562d1d3-fc98-4366-86a9-862ce9d4a236

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01f44da8781ffd87c4
Upwork Job ID: 1735236084437544960
Last Price Increase: 2024-03-28
Automatic offers:

shubham1206agra | Reviewer | 0
bernhardoj | Contributor | 0

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

Triggered auto assignment to @zanyrenney (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 9 months ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] commented 9 months ago

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

dukenv0307 commented 9 months ago

Proposal

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

Connect bank account page where user can select between "Connect online with Plate or "Connect manually" does not appear. User directly navigates to Connect manually

What is the root cause of that problem?

hasInProgressVBBA function returns false because anchData now has no bankAccount data which make shouldShowContinueSetupButton is false

https://github.com/Expensify/App/blob/5c0f3e49129ed3a839523aeab1464e898a02535d/src/pages/ReimbursementAccount/ReimbursementAccountPage.js#L224

Additionally, when we click on Connect Manually, reimbursementAccount stores currentStep and subStep as bank account steps. So user is redirected to the first step of connect bank account manually

https://github.com/Expensify/App/blob/5c0f3e49129ed3a839523aeab1464e898a02535d/src/pages/ReimbursementAccount/BankAccountStep.js#L83

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

In ReimbursementAccountPage, we should check if hasInProgressVBBA returns false, we will clear the reimbursementAccount

What alternative solutions did you explore? (Optional)

NA

shubham1206agra commented 9 months ago

@mountiny Please put this on hold till VBBA refactor

bernhardoj commented 9 months ago

Proposal

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

When we start a manual bank account step, close the app, and revisit the bank account page, it shows the routing and bank account number step (1st step).

What is the root cause of that problem?

When we press connect manually, it will set the subStep to manual. https://github.com/Expensify/App/blob/c270b5e233c61f833fbfbc83498724f5496a7766/src/pages/ReimbursementAccount/BankAccountStep.js#L161

Having subStep as manual will show the routing and bank account number step. https://github.com/Expensify/App/blob/c270b5e233c61f833fbfbc83498724f5496a7766/src/pages/ReimbursementAccount/BankInfo/BankInfo.tsx#L95

If we close the app, the subStep is still manual in the Onyx.

When we reopen the bank account page, fetchData will be called and the current subStep will be passed to it. After the API is done, the subStep that we pass is returned. https://github.com/Expensify/App/blob/5c0f3e49129ed3a839523aeab1464e898a02535d/src/pages/ReimbursementAccount/ReimbursementAccountPage.js#L266-L276

Because the subStep is still manual, we see the routing and bank account number, not the connect option page.

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

We can pass an empty subStep to the API request if the bank account page is mounted, but only if the current step is empty.

function fetchData(ignoreLocalCurrentStep, ignoreLocalSubStep) {
    ...
    BankAccounts.openReimbursementAccountPage(stepToOpen, ignoreLocalSubStep ? '' : subStep, ignoreLocalCurrentStep ? '' : localCurrentStep);
}

useEffect(() => {
    fetchData(false,  getStepToOpenFromRouteParams(route) === '')
}, [])

This will only work for online case, to cover for offline case, we can immediately clear the sub step when the component mounts.

useEffect(() => {
    BankAccounts.setBankAccountSubStep(null);
    fetchData(false)
}, [])

What alternative solutions did you explore? (Optional)

Reset the onyx subStep when we press the Bank Account button. https://github.com/Expensify/App/blob/c270b5e233c61f833fbfbc83498724f5496a7766/src/libs/actions/ReimbursementAccount/navigation.js#L21-L23

BankAccounts.setBankAccountSubStep(null);
Navigation.navigate(ROUTES.BANK_ACCOUNT_WITH_STEP_TO_OPEN.getRoute('', policyId, backTo));

The downside of this is, if the user deep link to the bank account page, they will still see the routing and bank account number.

melvin-bot[bot] commented 9 months ago

@zanyrenney, @shubham1206agra Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

shubham1206agra commented 9 months ago

Issue ON HOLD

zanyrenney commented 9 months ago

Hold, moving to weekly.

zanyrenney commented 8 months ago

on HOLD>

zanyrenney commented 8 months ago

ON HOLD.

zanyrenney commented 8 months ago

VBBA refactor isn't complete yet so still holding on this.

zanyrenney commented 7 months ago

HOLD.

zanyrenney commented 7 months ago

holding on VBA refactor project.

shubham1206agra commented 7 months ago

@zanyrenney Can you get this OFF HOLD and get it retested?

melvin-bot[bot] commented 6 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

zanyrenney commented 6 months ago

Off Hold

shubham1206agra commented 6 months ago

I can repro this issue. @bernhardoj and @dukenv0307, please update your proposals.

bernhardoj commented 6 months ago

Updated my proposal

melvin-bot[bot] commented 6 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

zanyrenney commented 6 months ago

@shubham1206agra please can you re-review the proposal from @bernhardoj

melvin-bot[bot] commented 6 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

shubham1206agra commented 6 months ago

@zanyrenney I tested @bernhardoj's proposal. It looks good to me. But there's a weird bug on the staging server where I stop the flow between the substep of a step. It will reset to the start of the same step, not the substep on which I left the flow.

wildan-m commented 6 months ago

Proposal

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

Connect manually - Connect bank account page does not appear when kill app

What is the root cause of that problem?

When forcefully closed, the clearReimbursementAccount activities here are not executed.

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

Re-initialize ONYXKEYS.REIMBURSEMENT_ACCOUNT on boot Change this code to:

import { reimbursementAccountDefaultProps } from '@pages/ReimbursementAccount/reimbursementAccountPropTypes';

.......
        initialKeyStates: {
            // Clear any loading and error messages so they do not appear on app startup
            [ONYXKEYS.SESSION]: {loading: false},
            [ONYXKEYS.ACCOUNT]: CONST.DEFAULT_ACCOUNT_DATA,
            [ONYXKEYS.NETWORK]: CONST.DEFAULT_NETWORK_DATA,
            [ONYXKEYS.IS_SIDEBAR_LOADED]: false,
            [ONYXKEYS.SHOULD_SHOW_COMPOSE_INPUT]: true,
            [ONYXKEYS.MODAL]: {
                isVisible: false,
                willAlertModalBecomeVisible: false,
            },
            // Always open the home route on app startup for native platforms by clearing the lastVisitedPath
            [ONYXKEYS.LAST_VISITED_PATH]: initializeLastVisitedPath(),
            // @ts-expect-error: ONYXKEYS.REIMBURSEMENT_ACCOUNT is conflicting with ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM
            [ONYXKEYS.REIMBURSEMENT_ACCOUNT]: reimbursementAccountDefaultProps,
        },

And add subStep: null to achData.subStep default props.

const reimbursementAccountDefaultProps = {
    achData: {
        state: BankAccount.STATE.SETUP,
        subStep: null,
    },
......

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 6 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

zanyrenney commented 5 months ago

I've been OOO. Now back :tada

zanyrenney commented 5 months ago

@shubham1206agra what do you think of the proposals above?

shubham1206agra commented 5 months ago

I will go ahead with @bernhardoj's proposal.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 5 months ago

Triggered auto assignment to @grgia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

shubham1206agra commented 5 months ago

@grgia I am thinking of combining this with https://github.com/Expensify/App/issues/33670 since both have the same solution.

grgia commented 5 months ago

@shubham1206agra which issue shall we close?

shubham1206agra commented 5 months ago

Close the other one.

grgia commented 5 months ago

@bernhardoj, assigning! Please make sure to test steps in https://github.com/Expensify/App/issues/33670 as well while testing your PR, thank you!

melvin-bot[bot] commented 5 months ago

📣 @shubham1206agra 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 5 months ago

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

bernhardoj commented 5 months ago

PR is ready

cc: @shubham1206agra

melvin-bot[bot] commented 4 months ago

This issue has not been updated in over 15 days. @grgia, @bernhardoj, @zanyrenney, @shubham1206agra eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

melvin-bot[bot] commented 4 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 4 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.67-7 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/39633

If no regressions arise, payment will be issued on 2024-05-06. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@bernhardoj requires payment automatic offer (Contributor)
@shubham1206agra requires payment automatic offer (Reviewer)

melvin-bot[bot] commented 4 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[ ] [@shubham1206agra] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@shubham1206agra] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@shubham1206agra] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@shubham1206agra] Determine if we should create a regression test for this bug.
[ ] [@shubham1206agra] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@zanyrenney] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

zanyrenney commented 4 months ago

payment summary

paid @bernhardoj $500 via upwork @shubham1206agra never accepted the offer so having to send new payment offer:

zanyrenney commented 4 months ago

https://www.upwork.com/nx/wm/offer/102216967 @shubham1206agra please accept ASAP for payout.

zanyrenney commented 4 months ago

payment summary

paid @bernhardoj $500 via upwork paid @shubham1206agra $500 via upwork

Expensify / App