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] [$750] mWeb - Click on back button from country picker page redirects to wrong page #23725

Closed
1 of 6 tasks
kbecciv opened this issue Jul 27, 2023 · 111 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@kbecciv
Copy link

kbecciv commented Jul 27, 2023

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


Problem

The country picker in Personal Details > Home Address is made of a component which is controlled in state and it does not have its own route. This was made to simplify reusing the component in various places in the app as its easier to just plug it into some page and let its open/ closed state be controlled from within the parent component as well as take the selected value and save it in the parent form.

This however leads to unexpected navigation behaviour with the back button in browser or in android, because the component is not its own page hence its not a screen in the browser history either.

Now that we got a good solution pattern in place with the IOUCurrencySelection component reused for 1. creating money request, 2. Confirming creation of money request and 3. Editing money request amount, all done using the same component and having its own path which allows for correct and predictable navigation.

Solution

Lets build the Country picker page as we did IOUCurrencySelection using its own route accepting goBack as url param, and making sure the AddressPage will consume the country picked from the ?country url param same as we do with ?currency in the other case.

Action Performed:

  1. Go to Settings > Profile > Personal details > Home address > Country
  2. Click back button of device

Expected Result:

Should navigates to Home address screen

Actual Result:

Navigates to Personal details screen

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.45-8
Reproducible in staging?: y
Reproducible in production?: new feature
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
Notes/Photos/Videos: Any additional supporting documentation

Screen_Recording_20230726_193945_Chrome.mp4
Screen_Recording_20230727_071059_Chrome.mp4

Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690380615025549

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0111fe2b2f552059b7
  • Upwork Job ID: 1697712669744525312
  • Last Price Increase: 2023-09-28
  • Automatic offers:
    • gadhiyamanan | Reporter | 26488219
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Jul 27, 2023
@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.

@melvin-bot
Copy link

melvin-bot bot commented Jul 27, 2023

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

@MariaHCD
Copy link
Contributor

I don't necessarily think this should block the deploy. Looks like this PR introduced the updates for the country picker: #21049

@mountiny
Copy link
Contributor

@MariaHCD This is not a blocker. This is same for Year picker and similar parts of the form. The Year page or in this case the State or Country picker are not its own route/ page in the stack hence when you click back it takes you to the personal details.

I think its something we might have to look at as a general solution as I think we would like this fixed for consistency, but its not a blocker.

I will demote this and add it to the navigation follow ups tracking project.

@mountiny mountiny added Daily KSv2 NewFeature Something to build that is a new item. and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Jul 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 27, 2023

@melvin-bot melvin-bot bot added the Weekly KSv2 label Jul 27, 2023
@mountiny mountiny removed the NewFeature Something to build that is a new item. label Jul 27, 2023
@melvin-bot melvin-bot bot removed the Daily KSv2 label Jul 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 27, 2023

Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature)

@mountiny mountiny added the NewFeature Something to build that is a new item. label Jul 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 27, 2023

Current assignee @greg-schroeder is eligible for the NewFeature assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Jul 27, 2023

Current assignee @shawnborton is eligible for the NewFeature assigner, not assigning anyone new.

@MariaHCD
Copy link
Contributor

Perfect, thanks @mountiny! Not familiar with the NewFeature assigner, how do we move this issue forward? Will we eventually apply the External label here?

@mountiny
Copy link
Contributor

@MariaHCD I will post this in SWM channel to let the guys know an dI think they will take it

posting NewFeature as I think its more true than bug

@WoLewicki
Copy link
Contributor

Isn't it the same for web when clicking browser back button @kbecciv ?

@kbecciv
Copy link
Author

kbecciv commented Jul 28, 2023

Hey @WoLewicki! I only checked mWeb on triaging moment. But let me check on Web in latest build.

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Apr 5, 2024
Copy link

melvin-bot bot commented Apr 5, 2024

This issue has not been updated in over 15 days. @parasharrajat, @greg-schroeder, @WoLewicki, @mountiny, @ygshbht 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 melvin-bot bot added the Monthly KSv2 label Apr 5, 2024
@greg-schroeder
Copy link
Contributor

Merged, awaiting deploy to staging -> prod

@greg-schroeder greg-schroeder added Daily KSv2 and removed Monthly KSv2 labels Apr 5, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Apr 8, 2024
@melvin-bot melvin-bot bot changed the title [$750] mWeb - Click on back button from country picker page redirects to wrong page [HOLD for payment 2024-04-15] [$750] mWeb - Click on back button from country picker page redirects to wrong page Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

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

@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

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 adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@parasharrajat] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

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

Okay this issue is crusty AF, gonna take some time to review who did what along the way here

@greg-schroeder
Copy link
Contributor

greg-schroeder commented Apr 15, 2024

@gadhiyamanan reported this issue 2023-07-26 - $50- Reporter
@ygshbht assigned 2023-09-04 - $1500 - Contributor
@parasharrajat - $1500 - C+

@greg-schroeder
Copy link
Contributor

greg-schroeder commented Apr 15, 2024

I'm going to create manual offers for this one as the original Upwork issue is long since closed, stand by

Payment issue: #40223

Offers sent to reporter and contributor - Rajat you can make a manual request in ND for $1500, then can you complete the checklist? Thanks!

@parasharrajat
Copy link
Member

Regression Test Steps

Verify for places where you have to select an American State from a list:

For Personal Address:

  1. Go to Profile > Addres.
  2. Select US as country.
  3. Open State selection page.
  4. Click back button of device.
  5. Notice it Properly navigates to the Home address screen.

For debit card page:

  1. Go to /settings/wallet/add-debit-card page.
  2. Select US as country.
  3. Open State selection page.
  4. Click back button of device
  5. Notice it properly navigates to Debit card screen.
  6. (Optional) Repeat the same for /send/new/add-debit-card page.

For ReimbusementAccountPage

  1. Go to workspaces.
  2. Select a Workspace > Settings > Connect Bank Account.
  3. Verify for all State Selectors you see in the flow (e.g Company Address, Incorporation State, etc.)
  4. Click the back button of the device when in the respective State selection pages.
  5. Notice it properly navigates to the previous screen.

Do you agree 👍 or 👎 ?

@mountiny
Copy link
Contributor

I think we should add a test for this but one should be enough.

I would say lets use the For Personal Address:

@greg-schroeder would you be able to add this one? thanks!

This comment was marked as off-topic.

@greg-schroeder
Copy link
Contributor

Added, closing!

@parasharrajat
Copy link
Member

Payment requested as per #23725 (comment)

@JmillsExpensify
Copy link

Asked for confirmation on the intended amount in New Expensify.

@JmillsExpensify
Copy link

@greg-schroeder would you mind double confirming that the automated summary is wrong, and that your summary here is right?

@greg-schroeder
Copy link
Contributor

Hey @JmillsExpensify - yes, I was going off of pricing that Vit laid out here:

#23725 (comment)

@JmillsExpensify
Copy link

Thanks! $1,500 approved for @parasharrajat

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 Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
No open projects
Development

No branches or pull requests