[PAID] [$500] Web - Sign in modal - Browser back button takes to previous modal while modal back button closes it

[lanitochka17]

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.15-4
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:

1. Log out from ND
2. Navigate to this url: https://staging.new.expensify.com/r/5408450846930023
3. Click on a random email from within the public room
4. Click on Message ...
5. Click on the back button on the sign-in-modal
6. Do step 3 - 4 again
7. Click on the browser back button
8. Compare the results

Expected Result:

Clicking both the browser back button and clicking the modal back icon should have the same functionality. They both should take user back to previous modal

Actual Result:

Browser back button takes user to the previous modal whereas the modal back button closes the modal

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

Add any screenshot/video evidence

Bug6322787_1703169313939._1__New_Expensify_-_Google_Chrome_2023-12-21_16-16-54.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0135197050c7e9cb4f
• Upwork Job ID: 1737855322627493888
• Last Price Increase: 2023-12-21
• Automatic offers:
• s77rt | Reviewer | 28076495
• tienifr | Contributor | 28076496

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @strepanier03 (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 Contributor-plus team member for initial proposal review - @s77rt (External)

[bernhardoj]

Proposal

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

Pressing back on sign in modal close the whole modal.

What is the root cause of that problem?

We call dismissModal when pressing the back button instead of goBack

App/src/pages/signin/SignInModal.js

Line 32 in b966bd4

<HeaderWithBackButton onBackButtonPress={Navigation.dismissModal} />

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

Call goBack when pressing the back button.

or just remove onBackButtonPress because the default value already calls goBack

App/src/components/HeaderWithBackButton/index.js

Line 26 in b966bd4

onBackButtonPress = () => Navigation.goBack(ROUTES.HOME),

More information:
The dismissModal is added on this PR. It was added to fix #29248 (comment).

The issue happens because on the previous commit, they redirect the user to the sign in modal on every nav state change. So, pressing back will loop the user back to the sign in modal.

But it's not reproducible anymore after they move the logic to openReportFromDeepLink.

[rajutkarsh07]

upwork id : https://www.upwork.com/freelancers/~01cfa49ce124da3919?s=1110580755107926016
email : utkarshraj1306@gmail.com

To solve this issue
in the

App/src/pages/signin/SignInModal.js

we have to change the function inside the onBackButtonPress event with a new function handleBack

 const handleBack = () => {
        navigate(-1); 
    };

this is solve the issue

[melvin-bot]

📣 @rajutkarsh07! 📣
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>

[rajutkarsh07]

Contributor details
Your Expensify account email: utkarshraj1306@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01cfa49ce124da3919?s=1110580755107926016

To solve this issue
in the

App/src/pages/signin/SignInModal.js

we have to change the function inside the onBackButtonPress event with a new function handleBack

 const handleBack = () => {
        navigate(-1); 
    };

this will solve the issue

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[tienifr]

Proposal

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

Browser back button takes user to the previous modal whereas the modal back button closes the modal

What is the root cause of that problem?

In here, we're dismissing the sign in modal when clicking on back button, leading to this issue.

Previously we go back when clicking on close button in the modal, but that was changed in this PR to fix the issue where you're able to go back from the sign in modal to the "protected route" (after being forced navigate to the sign in modal when trying to accessing the "protected route" directly.

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

We should remove onBackButtonPress={Navigation.dismissModal} here so it will default to going back.

<HeaderWithBackButton />

the issue where you're able to go back from the sign in modal to the "protected route" (after being forced navigate to the sign in modal when trying to accessing the "protected route" directly.

However, it will cause this issue, we should fix it properly by, when going to the "protected route" via deeplink and being redirected to the sign in modal, we should navigate by "replacing" the current screen, not "pushing" the sign in modal on top of the current protected screen. This will make sure when going back, it will not go back to the protected route because the protected route was already replaced by the sign in modal.

We can do this in several ways, one of them is:

• When signOutAndRedirectToSignIn here, we use CONST.NAVIGATION.TYPE.UP so that the sign in modal will replace current protected screen.

if (shouldReplaceCurrentScreen) {
    Navigation.navigate(ROUTES.SIGN_IN_MODAL, CONST.NAVIGATION.TYPE.UP);
} else {
    Navigation.navigate(ROUTES.SIGN_IN_MODAL);
}

We use a param shouldReplaceCurrentScreen in signOutAndRedirectToSignIn to use this replacing behavior only for the use case where we need to do that, for example here.

What alternative solutions did you explore? (Optional)

Popping current screen before calling signOutAndRedirectToSignIn in the above case should also work

[s77rt]

@bernhardoj Thanks for the proposal. Your RCA is correct and the solution looks good to me.

🎀 👀 🎀 C+ reviewed
Link to proposal

Edit: i think this will cause regression with protected links e.g. https://dev.new.expensify.com:8082/workspace/62041BFC35703E3C

[melvin-bot]

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

[s77rt]

@rajutkarsh07 Thanks for your interest here. Proposals should follow the proposal templete. Please checkout the contributing guide.

[s77rt]

@tienifr Thanks for the proposal. Your RCA is correct and thanks for brining up the deeplink case. The suggested solution looks good to me.

🎀 👀 🎀 C+ reviewed
Link to proposal

[melvin-bot]

Current assignee @jasperhuangg is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[s77rt]

Not overdue. Waiting for assignment

[s77rt]

Same ^

[melvin-bot]

📣 @s77rt 🎉 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]

📣 @tienifr 🎉 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 📖

[jasperhuangg]

Sorry for the delay on this, was OOO, let's move forward with @tienifr's proposal, it makes sense to me.

[melvin-bot]

This issue has not been updated in over 15 days. @strepanier03, @s77rt, @jasperhuangg, @tienifr 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!

[s77rt]

Anyone seen Melvin lately? The PR was deployed to production long ago

[jasperhuangg]

There must have been a problem, good ping. @strepanier03 Can we retest to verify that this bug was solved and issue payment if it works?

[s77rt]

• The PR that introduced the bug has been identified: Fix/28925: Anonymous user can edit profile #29248
• The offending PR has been commented on: Fix/28925: Anonymous user can edit profile #29248 (comment)
• A discussion in #expensify-bugs has been started: I don't think this is needed. This type of bugs can be caught during testing
• Determine if we should create a regression test for this bug: No, the profile page got changed and this bug can no longer be tested

[tienifr]

Anyone seen Melvin lately? The PR was deployed to production long ago

@strepanier03 can you proceed with payments here, thanks!

[tienifr]

@strepanier03 Bump on this, thanks!

[strepanier03]

Was OoO yesterday but missed this because it's still set to monthly. Setting to Daily and will put it on the list to get through.

[tienifr]

Was OoO yesterday but missed this because it's still set to monthly. Setting to Daily and will put it on the list to get through.

@strepanier03 Friendly bump on this.

Thanks!

[melvin-bot]

@strepanier03, @s77rt, @jasperhuangg, @tienifr Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[jasperhuangg]

@strepanier03 friendly bump on this. It seems we still need to pay out the C+ and PR authors for their work on this issue?

@tienifr @s77rt can you confirm?

[s77rt]

Yes that's correct

[strepanier03]

Will handle this today, I've been out of office all week until now.

[strepanier03]

Payment for @tienifr and @s77rt have been completed.