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

[HOLD for payment 2024-04-15] [$500] Wallet - Back button returns to the same page after adding a bank account in Wallet page #38558

Closed
4 of 6 tasks
izarutskaya opened this issue Mar 19, 2024 · 29 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@izarutskaya
Copy link

izarutskaya commented Mar 19, 2024

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.54-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Launch New Expensify app.
  2. Go to Profile > Wallet.
  3. Tap Add bank account > Personal bank account.
  4. Add a bank account.
  5. After returning to Wallet page, tap back button on the top left.

Expected Result:

App will return to Profile page.

Actual Result:

App returns to the same Wallet page.

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6418351_1710793704406.RPReplay_Final1710792508.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0172f19f6c44056361
  • Upwork Job ID: 1773070619325939712
  • Last Price Increase: 2024-03-27
  • Automatic offers:
    • bernhardoj | Contributor | 0
@izarutskaya izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 19, 2024
Copy link

melvin-bot bot commented Mar 19, 2024

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

@izarutskaya
Copy link
Author

We think that this bug might be related to the #collect project.

@allgandalf
Copy link
Contributor

allgandalf commented Mar 19, 2024

Proposal

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

Back button returns to the same page after adding a bank account in Wallet page

What is the root cause of that problem?

For Wallet Page, We have Navigation.goBack defined to go back to the last fallback:

<HeaderWithBackButton
title={translate('common.wallet')}
onBackButtonPress={() => Navigation.goBack()}
icon={Illustrations.MoneyIntoWallet}

But when we verify the bank account, the fallback route itself is the wallet page as we were redirected to the wallet page when we added bank account

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

We can set shouldEnforceFallback to true:

function goBack(fallbackRoute?: Route, shouldEnforceFallback = false, shouldPopToTop = false) {

Also we need to update Navigation.goBack to make it fallback to the profile settings page.

What alternative solutions did you explore? (Optional)

Or, Simply update the goBack route to profile settings page:

<HeaderWithBackButton 
     title={translate('common.wallet')} 
     onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS)}

Note

The Wallet Page is a Central Pane and not a RHP hence we can put the correct route inside of goBack, this won't cause any regression with the animation of pages as it is with RHP pages.

Result Video:

2024-03-19-19.07.33.515936459.mp4

@bernhardoj
Copy link
Contributor

bernhardoj commented Mar 19, 2024

Proposal

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

Pressing back button from wallet page returns to the add personal bank account page again.

What is the root cause of that problem?

When we press the continue button after successfully add the bank account, it will call exitFlow which will call goBack with fallback route of wallet page.

const exitFlow = useCallback(
(shouldContinue = false) => {
const exitReportID = personalBankAccount?.exitReportID;
const onSuccessFallbackRoute = personalBankAccount?.onSuccessFallbackRoute ?? '';
if (exitReportID) {
Navigation.dismissModal(exitReportID);
} else if (shouldContinue && onSuccessFallbackRoute) {
PaymentMethods.continueSetup(onSuccessFallbackRoute);
} else {
Navigation.goBack(ROUTES.SETTINGS_WALLET);
}

Because the add bank account is the only page in the route, it will use the fallback route and push it to the nav stack.
[wallet page, add personal bank account page, wallet page]

So, when we press back, we will arrive at add personal bank account page again.

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

Remove the fallback route from goBack.

Navigation.goBack(ROUTES.SETTINGS_WALLET);

btw, a separate thing, I think we can force the fallback here

@ghost
Copy link

ghost commented Mar 19, 2024

Proposal

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

Wallet - Back button returns to the same page after adding a bank account in Wallet page

What is the root cause of that problem?

The root cause is here :

onBackButtonPress={() => Navigation.goBack()}

we are just defining Navigation.goBack()

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

To move back to Profile Settings page. We need to modify this route to by adding this :

Navigation.goBack(ROUTES.SETTINGS)

What alternative solutions did you explore? (Optional)

N/A

@ghost
Copy link

ghost commented Mar 19, 2024

Updated Proposal

@allgandalf
Copy link
Contributor

Proposal Updated

Updated my proposal to add result video and a side note

@alexpensify
Copy link
Contributor

I'll try to test this one tomorrow.

@alexpensify
Copy link
Contributor

No update yet

@melvin-bot melvin-bot bot added the Overdue label Mar 25, 2024
@alexpensify
Copy link
Contributor

On my testing list

@melvin-bot melvin-bot bot removed the Overdue label Mar 26, 2024
@alexpensify
Copy link
Contributor

I tested this one on Android and see a double click on the back button is required to get to the Profile page.

@alexpensify alexpensify added the External Added to denote the issue can be worked on by a contributor label Mar 27, 2024
@melvin-bot melvin-bot bot changed the title Wallet - Back button returns to the same page after adding a bank account in Wallet page [$500] Wallet - Back button returns to the same page after adding a bank account in Wallet page Mar 27, 2024
Copy link

melvin-bot bot commented Mar 27, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 27, 2024
Copy link

melvin-bot bot commented Mar 27, 2024

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

@alexpensify
Copy link
Contributor

@eVoloshchak - can you please review if these proposals will resolve this issue? Thanks!

@alexpensify
Copy link
Contributor

No update yet, this one needs a proposal review.

@eVoloshchak
Copy link
Contributor

I think we should proceed with @bernhardoj's proposal, as it's the most universal one (we don't rely on setting ROUTES.SETTINGS as a fallback route, which would need to be changed in case we move wallet page outside settings)

🎀👀🎀 C+ reviewed!

Copy link

melvin-bot bot commented Mar 29, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Apr 1, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 1, 2024
@bernhardoj
Copy link
Contributor

PR is ready

cc: @eVoloshchak

@alexpensify
Copy link
Contributor

Awesome, the PR is moving forward.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Apr 8, 2024
@melvin-bot melvin-bot bot changed the title [$500] Wallet - Back button returns to the same page after adding a bank account in Wallet page [HOLD for payment 2024-04-15] [$500] Wallet - Back button returns to the same page after adding a bank account in Wallet page Apr 8, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

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

Copy link

melvin-bot bot commented Apr 8, 2024

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

If no regressions arise, payment will be issued on 2024-04-15. 🎊

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

Copy link

melvin-bot bot commented Apr 8, 2024

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:

  • [@eVoloshchak] The PR that introduced the bug has been identified. Link to the PR:
  • [@eVoloshchak] 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:
  • [@eVoloshchak] 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:
  • [@eVoloshchak] Determine if we should create a regression test for this bug.
  • [@eVoloshchak] 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.
  • [@alexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@alexpensify
Copy link
Contributor

I confirmed there is an Upwork job here and will continue with the payment process on Monday.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 14, 2024
@alexpensify
Copy link
Contributor

alexpensify commented Apr 15, 2024

Payouts due:

Upwork job is here.

@eVoloshchak
Copy link
Contributor

  1. The PR that introduced the bug has been identified. Link to the PR: Handle isWaitingOnBankAccount for IOU requests #20821
  2. 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: https://github.com/Expensify/App/pull/20821/files#r1579541672
  3. 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: this is a very simple bug, additional discussion isn't needed

Regression Test Proposal

  1. Go to Settings > Wallet.
  2. Tap Add bank account > Personal bank account.
  3. Add a bank account and complete the process until you see the success screen.
  4. Press continue, it will bring you back to wallet page.
  5. After returning to the Wallet page, tap the back button on the top left.
  6. Verify the wallet page is closed

Do we agree 👍 or 👎

@JmillsExpensify
Copy link

$500 approved for @eVoloshchak though re-opening for Al to handle the regression test.

@alexpensify
Copy link
Contributor

I'll work on the regression test.

@melvin-bot melvin-bot bot removed the Overdue label Apr 29, 2024
@alexpensify
Copy link
Contributor

Closing - the test request has been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Archived in project
Development

No branches or pull requests

7 participants