[$500] Web - App takes user two steps back after clicking on browser back button from Year page

[kbecciv]

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

---

Action Performed:

1. Go to Profile -> Personal Details -> Date of birth -> Year
2. Click on the browser back button

Expected Result:

App should take user to the date of birth page

Actual Result:

App takes user two steps back to the Personal Details page

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.70.5
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
Notes/Photos/Videos: Any additional supporting documentation

Recording.4496.mp4

2023-09-14.14.43.04.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Nathan-Mulugeta
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694691923038999

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01991ba5bf527a5951
• Upwork Job ID: 1703744307185139712
• Last Price Increase: 2023-09-18
• Automatic offers:
• b4s36t4 | Contributor | 27665448
• ishpaul777 | Contributor | 28064011

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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]

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

[melvin-bot]

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

[b4s36t4]

Proposal

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

Web - App takes user two steps back after clicking on browser back button from Year page

What is the root cause of that problem?

Basically year selector is a modal on which we don't have any navigation. So for browser or react-navigation the route we're in date-of-birth.

When we click on browser back button it's taking the previous route related to date-of-birth route.

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

So as far as I see it's the expected behavior because the YearSelector is a modal

But we show the modal as full-screen element which previews like it's another screen.

1. We can make the YearSelector as route and give it a push/pop state we can make browser back button work as expected.

2. We can make the YearSelector to show it's a modal (a bit complicated in my view and need design review as well).

In my view making this as route is plausible solution.

Updates required

1. Removing the Component Rendering

   App/src/components/NewDatePicker/CalendarPicker/index.js

   Line 237 in db4d85b

   <YearPickerModal

2. App/src/components/NewDatePicker/CalendarPicker/index.js

   Line 138 in db4d85b

   onPress={() => this.setState({isYearPickerVisible: true})}

   Update this line to Push the route instead of updating the state.

3. Use Onyx to mutating the state. The calendar right now is managing everything in state. But to workout with Year, we can use Onyx and have a key like userDOBYear or something which would provide the way to read the selected year inside this component

   App/src/components/NewDatePicker/CalendarPicker/index.js

   Line 42 in db4d85b

   class CalendarPicker extends React.PureComponent {

   .

4. App/src/components/NewDatePicker/CalendarPicker/index.js

   Line 80 in db4d85b

   onYearSelected(year) {

   Make this function which handles year change to support the onyx key change. Like componentDidUpdate if the year got updated we run this function.

Basically running every reference making sure Onyx keys are updated and glued together to make the YearModal as a route and support value coupling.

We should also fix the same issues with other modals like StateSelector modal's etc. I feel we can raise multiple PRs to eliminate regressions.

What alternative solutions did you explore? (Optional)

NA

[deep-explorer]

Hi how are you?
I think we should make new route for select year.
Now current code, there is date and select year are same route so if user clicks history back, it should be gone back to personal detail page.
I wanna start to work on your project.
Thanks

[melvin-bot]

📣 @deep-explorer! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[deep-explorer]

email: fesikkerya@gmail.com
upwork: https://www.upwork.com/freelancers/~017ebcbab58324ab10

[ishpaul777]

same root cause as #27160

[deep-explorer]

Well, I got your point.
For this we should call this page again when we click the modal just like Slack.

[mallenexpensify]

Thanks @ishpaul777 for the link.
@thesahindia do you think we should we hold this issue, pending

• Dev: Web - Incorrect Screen when Clicking CMD+K while Year Selection Modal is opened. #27160

or... do you think we should hold #27160 since we have a proposal here?

or... neither :)

[thesahindia]

or... do you think we should hold #27160 since we have a proposal here?

Yeah we should hold. I believe #27160 will be fixed by the solution.

[thesahindia]

@b4s36t4's proposal looks good to me. We need a route.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[srikarparsi]

@roryabraham, do you think you could take at this proposal. The route solution (number 1) looks similar to your solution here but just want to make sure we're all on the same page.

[ishpaul777]

Hii @mallenexpensify @srikarparsi, curious if I am eligible for reporting bonus as this issue has same root cause and fix as #27160 and #27160 was reported first.

[mallenexpensify]

Hii @mallenexpensify @srikarparsi, curious if I am eligible for reporting bonus as this issue has same root cause and fix as #27160 and #27160 was reported first.

Possibly? Can you 'prove' it @ishpaul777 ? ie. drop in details and links for review and so we have an audit trail if we do issue payment.

[ishpaul777]

#27160 was reported on Slack on 11 Sept. - https://expensify.slack.com/archives/C049HHMV9SM/p1694443603230179?thread_ts=1694443603.230179&cid=C049HHMV9SM

And this one was reported on slack on 14 Sept. - https://expensify.slack.com/archives/C049HHMV9SM/p1694691923038999?thread_ts=1694691923.038999&cid=C049HHMV9SM

[Nathan-Mulugeta]

Hey guys i think this issue might not even have a reporting bonus as per this comment

[ishpaul777]

Sorry! This got off my radar.. I'll complete the PR late this week

[mallenexpensify]

Thanks for the post @ishpaul777 , I completely missed it cuz, with the Reviewing label, the issue never went overdue

[roryabraham]

Removing the Reviewing label to bring this back on everyone's radar

[mallenexpensify]

@ishpaul777 , when can we expect the PR?

[ishpaul777]

Sorry for delay, I'll try to priortize and complete this week.

[ishpaul777]

Hey, as I'm working on the PR, the changes are getting pretty big and messy. Honestly, I'm starting to feel like the bug isn't that crucial – more on the low-value side. Plus, it's making using the DatePicker a bit tricky and might cause issues down the road.

I'm cool with moving forward, but i'd appreciate if @roryabraham/ @thesahindia could check out the changes upto this point and see if they feel the same way. The thing is, we not only use the Component Independently but also inside other components. Like, for example, the Identityform is used in a bunch of places and has a DatePicker we have to create many routes as i expected previously and becoming very anti-pattern and question is if the bug is really critical enough to solve with this approach.

Personally i'd advocate for the 3rd option @roryabraham suggested here, to "Rethink the push-to-page design for these components and redesign the datepicker component to make it an inline component" only if the bug is considered critical enough and worth solving.

[roryabraham]

Ok, I appreciate the restraint and trust your judgment @ishpaul777. I think we should have a broader discussion about this.

[mallenexpensify]

"Rethink the push-to-page design for these components and redesign the datepicker component to make it an inline component"

Where we're at now

[mallenexpensify]

I propose

1. We close this as an edge case.
2. I check with a wave/VIP project to see if this fits, then discuss next steps
3. We put this on hold (which I imagine we'd likely just close sometime after, so I'd vote for Some initial fixes and code style updates #1 unless someone ideas on how to move this forward)
4. Check somewhere to get a better understanding on "push-to-page design" and where this issue might fall with any work being done there.

[roryabraham]

I mostly agree with @mallenexpensify's summary but I think our options more succinctly are:

1. :donothing:, close this issue
2. Attach this to a wave project, and rethink the push-to-page design in this case
3. Proceed with @ishpaul777's PR to create separate routes for each usage of the date picker

Looking at this with fresh eyes, I'd recommend either 1 or 3, not 2. I don't think that reopening discussions around push-to-page is really a good option for our team's time right now.

[mallenexpensify]

Thanks Rory, If we go with #3, will the fix only benefit this one edge case? If so, I'm leaning towards #1.

[mallenexpensify]

Coming from this conversation in #expensify-open-source
https://expensify.slack.com/archives/C01GTK53T8Q/p1707769898749359

I'm going to close this, taking into account @ishpaul777 's comment in the link above too.

My vote would be :donothing: because this is not something which blocks any feature functionalities just a edge case and the alternatives,

[ishpaul777]

@roryabraham @mallenexpensify
Even though we did't merge any PR but i feel i have invested a decent effort on this issue so i'd kindly like to request a partial compensation if possible 🙇‍♂️

[mallenexpensify]

@ishpaul777 please review the internal C+ process doc and follow the details there (which.. is mostly what you've done, I'm in the process of updating the doc which is partially why I want to share here)
https://docs.google.com/document/d/1BvohU05MTaHnjOD_vwJv_aqDAirv-ChkyRnKCAvOVyQ/edit#bookmark=id.wfm6lkgp0c6

[ishpaul777]

here i am a contributor (option A) so no need to post in #contributor-plus group right?

[roryabraham]

A partial compensation of $250 feels fair to me in this case.

[ishpaul777]

A partial compensation of $250 feels fair to me in this case.

I trust your judgment on what is fair in this situation. Whatever amount you believe is appropriate, I am in agreement with. 👍

[ishpaul777]

gentle bump @mallenexpensify / @roryabraham can we reopen/pay out this if all are in agreement for $250 ?

[roryabraham]

@mallenexpensify can we issue a discretionary bonus of $250 to @ishpaul777 for his work on this issue?

[mallenexpensify]

Contributor+: @ishpaul777 paid $250 via Upwork.