[HOLD for payment 2023-08-30] [$1000] Navigate back to public room after clicking on Sign-in as unauthenticated user

[marcochavezf]

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. Open a public room as unauthenticated user (i.e. https://staging.new.expensify.com/r/8813851443373010)
2. Click on sign-in
3. Click on the back button

Screen.Recording.2023-06-09.at.18.41.52.mov

Expected Result:

It should redirect you to the public room

Actual Result:

It just redirects you the previous browser page

Workaround:

Click again on the link

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: v1.3.25-8
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:
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017a21cdc86c5eee7f
• Upwork Job ID: 1667332458840076288
• Last Price Increase: 2023-06-10

[melvin-bot]

Triggered auto assignment to @dylanexpensify (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]

Job added to Upwork: https://www.upwork.com/jobs/~017a21cdc86c5eee7f

[melvin-bot]

Current assignee @dylanexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

Current assignee @marcochavezf is eligible for the External assigner, not assigning anyone new.

[marcochavezf]

Making it external in case someone wants to propose a solution during the weekend. Just some additional info, we're setting the reportID of the public room here in Onyx when we "log out" the anonymous user (maybe we can navigate to that reportID when the user clicks on the back button).

[daordonez11]

Proposal

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

When accessing a public room as an anonymous user and then clicking on sign in the navigation stack is lost

What is the root cause of that problem?

When sign in is pressed signOutAndRedirectToSignIn method is called, finally calling clearStorageAndRedirect. "removing the authToken redirects the user to the Sign-in page". AppNavigator class switches the navigation to PublicScreens.

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

Override the back button of the browser and send to the report same as deeplinks when the button is pressed and the id exists: example of the effect in SignInPage working (POC code):

useEffect(() => {
        history.pushState({}, '', '/'+ROUTES.getReportRoute(lastOpenedRoomId));
        const handleBackButton = () => {
            if(lastOpenedRoomId && lastOpenedRoomId!==''){
                //USe deeplink implementation to change stack
                Report.openReportFromDeepLink(ROUTES.getReportRoute(lastOpenedRoomId), false)
            }else{
                history.back();
            }
        };
    
        // Add event listener for the browser back button press
        window.addEventListener('popstate', handleBackButton);
    
        // Clean up the event listener when the component unmounts
        return () => {
          window.removeEventListener('popstate', handleBackButton);
        };
      }, []);

What alternative solutions did you explore? (Optional)

First I thought about saving the last link before navigating and cleaning but then I realized that save part was already done and explained by @marcochavezf
PS: History pushState is a must, if the history stack is empty the listener of popstate is never called. This is just a first approach for the POC to work

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[melvin-bot]

📣 @daordonez11! 📣
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. 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.
2. 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.
3. 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>

[daordonez11]

Contributor details
Your Expensify account email: dloz1996@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01e96b8da58232f028

[melvin-bot]

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

[marcochavezf]

Hi @daordonez11 and thanks for the proposal! Could you share a quick video of that proposal working? Most of the proposal looks good, but I think I'm not understanding completely the "why" of this part:

I found "in order to add a history item onto the browser history stack one would need to use the "push" action." Here

[daordonez11]

Hey @marcochavezf for sure working on it! Any specific branch? I'll do it on the commit you shared, yesterday I was working on main. Also quick question, you could save lastOpenesPublicRoom before calling "redirectToSignIn" and including your key in keysToPreserve in the method clearStorageAndRedirect. Any specific reason why it is currently being done after? Sometimes in my test the signin component is loaded before the key exists in onyx so I need to handle updates.

[daordonez11]

Hey @marcochavezf here is a video with the fix and I will update the proposal

New_Recording_-_10_6_2023._23_00_13.mp4

Some important stuff, the original proposal didn't work because PuclicScreens and AuthScreens are different navigators, so you cannot simply navigate between both, hence resetting the navigation doesn't work.
The final solution was to listen to navigator popstate, BackHandler didn't work in the web app either, and open the report the same way as the deep link does. Also editing the history stack is a must for the listener to work, I found some details here otherwise the listener does nothing

[marcochavezf]

you could save lastOpenesPublicRoom before calling "redirectToSignIn" and including your key in keysToPreserve in the method clearStorageAndRedirect. Any specific reason why it is currently being done after?

Oh this was hotfix and is just a temporal solution, since we're still changing things in the backend to share the report with the user signed in. When I implemented it I forgot we could include it in the keysToPerserve, thanks for pointing that!

[marcochavezf]

Also thanks for updating the proposal! Yeah, given the current structure of the navigators I think it makes sense to listen the popstate, but I think this solution will work only for web, no? Could you also check a solution for Android native?

[daordonez11]

Oh for sure! I focused on a solution that could work on web. For Android Native I tested BackHandler, practically the same solution but different handler (hardwareBackPress). Later when I'm on the PC I can share a video.

[marcochavezf]

Sounds good, thanks! I think that will be enough for Android and we can address any detail in the PR. Assigning @daordonez11 🚀

[melvin-bot]

📣 @daordonez11 You have been assigned to this job by @marcochavezf!
Please apply to this job in Upwork 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 📖

[daordonez11]

Hey guys! Thanks for the guidance and patience everyone. So finally seeing we got this through I wanted to open a discussion for reward on the issue. As discussed in this slack thread the scope of the issue changed and there were some additional tasks done to achieve the final goal. For your analysis of the reward I wanted to list all the executed tasks in the issue:

• Approved proposal and first PR got rejected due to the specificity of code for the specific issue.
  • Scope creep and we decide to handle it with navigation
• First attempt PR was unifying the stack and discarded due to side effects found also seeing that it didn't fix the original issue.
• Second attempt and final PR is Signin in RHP which was approved yesterday and included some improvements for AuthScreens

Finally thank you very much everyone for the opportunity and teachings it took some time but it is awesome to finally see this reaching main. And of course @mollfpr for the help with the final details to make it work smoothly!

Of course, I'll be ready for any follow-up in terms of navigation, maybe some handling of events or navigation with the sign-in could be required in steps we didn't check. Still, nothing should be blocked with this implementation.

cc @marcochavezf @mountiny @dylanexpensify

[marcochavezf]

Thanks @daordonez11 for your effort and I agree that this issue required exploration and back and forth discussion to reach to an optimal solution. Given the complexity of the task I think it would be ideal to double the bounty here. Thoughts @mountiny? (Since you were in the loop from the first attempt to solve the back navigation issue).

[mountiny]

Agreed

[dylanexpensify]

Waiting for regression period to pass, then will pay w/double bounty!

[daordonez11]

Hey there! @dylanexpensify this was deployed to prod on 23-08 I think regression period is already covered also I haven't seen any report yet 🫡.

Cc @marcochavezf @mountiny

[mountiny]

This seems to be ready for payment then #24083

We should add a test for this for sure @daordonez11 can you suggest test steps please to cover this thoroughly?

[daordonez11]

This seems to be ready for payment then #24083

We should add a test for this for sure @daordonez11 can you suggest test steps please to cover this thoroughly?

Yeah for sure! I'll create something close to the BZ list and post it today

[daordonez11]

• 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:
  There is no offending PR this was created as sis since the definition of AuthScreens, the login page was just ina different RootStack
• 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:
  NA, this turned into a feature
• Determine if we should create a regression test for this bug.
  Here is the list of steps I would recommend:

1. Open a public room as unauthenticated user (i.e. https://staging.new.expensify.com/r/8813851443373010)
2. Click on sign-in
3. Click on the back button(Native or Header button), you should see the public room instead of closing the app
4. Enter sign-in in RHP and finish the sign-in flow with a new account
5. Open a new window in incognito mode, sign in with another account, and post something in the public room
6. Verify the message appears in the public room for the first user that claimed the anonymous account

• Do we agree 👍 or 👎

[daordonez11]

Friendly bump @dylanexpensify

[daordonez11]

Hey guys, @marcochavezf @mountiny do you know if maybe @dylanexpensify is OOO? It's already been 2 weeks after the expected payment date.

[mountiny]

He was at the conference we were launching so I think he fell behind these chores @dylanexpensify could you look into these ones please?

[dylanexpensify]

Yes! On it - thanks for the ping!

[daordonez11]

Hey @dylanexpensify hope everything went well at the conference! Maybe a new task will be required since the original job is no longer available https://www.upwork.com/jobs/~017a21cdc86c5eee7f

[dylanexpensify]

Agree! On it now!

[dylanexpensify]

Payment summary:

• Contributor: @daordonez11 $2,000
• Contributor+: @mollfpr $2,000

Upwork Job! Please apply!

[mollfpr]

@dylanexpensify Applied, thanks!

[daordonez11]

Applied too thanks @dylanexpensify

[dylanexpensify]

Woot!

[dylanexpensify]

Offers sent!

[dylanexpensify]

payments sent! TY both!!