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

marcochavezf commented 1 year ago

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:

Open a public room as unauthenticated user (i.e. https://staging.new.expensify.com/r/8813851443373010)
Click on sign-in
Click on the back button

https://github.com/Expensify/App/assets/6829422/4958ecad-0ed8-48a4-86f3-37b5960ed48c

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?

[x] Android / native
[x] Android / Chrome
[ ] iOS / native
[x] iOS / Safari
[x] 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[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

marcochavezf commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

📣 @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:

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.
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.
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 commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

marcochavezf commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

https://github.com/Expensify/App/assets/13544910/47eb5d8f-e84a-48df-919d-4a919c559d7a

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

📣 @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 commented 1 year ago

Thanks @marcochavezf I will apply to upwork then. I expect to have the PR done by tomorrow(June 12) end of day so you can check it on Tuesday, I'll let you know any important update. Also any chance you could help me request the slack access? I sent it last week to the contributors email but still no response.(Sent it with my personal email dloz1996@gmail.com)

daordonez11 commented 1 year ago

Hey @marcochavezf I already have part of the PR ready but I have 2 questions

How should I manage the code that is only for android and only for web? I'm currently testing with something like this:

useEffect(() => {
    if(lastOpenedRoomId && lastOpenedRoomId!==''){
        // Push state is required to trigger the popstate event
        window.history.pushState({}, '', ROUTES.getReportRoute(lastOpenedRoomId));
    }
    const browserBackPressed = () => {
        if(lastOpenedRoomId && lastOpenedRoomId!==''){
            // Use deeplink implementation to change navigation stack
            Report.openReportFromDeepLink(ROUTES.getReportRoute(lastOpenedRoomId), false)
        }else{
            window.history.back();
        }
    };
    const androidBackButtonPressed = () => {
        if(lastOpenedRoomId && lastOpenedRoomId!==''){
            Report.openReportFromDeepLink(ROUTES.getReportRoute(lastOpenedRoomId), false)
        }else{
            BackHandler.exitApp()
        }
      return true;
    };
    // Add event listener for the browser back button press and android back button
    if(typeof window !== 'undefined' && window.addEventListener ) {
        window.addEventListener('popstate', browserBackPressed);
    }
    else {
        const backHandler = BackHandler.addEventListener(
            'hardwareBackPress',
            androidBackButtonPressed,
          );
    }

    // Clean up the event listeners when the component unmounts
    return () => {
        if(typeof window !== 'undefined' && window.addEventListener) {
            window.removeEventListener('popstate', browserBackPressed);
        }
        else {
            backHandler.remove();
        }
    };
  }, [lastOpenedRoomId]);

But I don't feel all this window ifs are the way of solving it

How can I test deeplink in android native? I already have the environment working but I'm not sure how to run the app beginning on the public room

thesahindia commented 1 year ago

Hey @dylanexpensify @marcochavezf, please assign another C+ here. There is too much on my plate right now.

marcochavezf commented 1 year ago

@mollfpr was assigned automatically in the PR, so I will assign you also here :)

daordonez11 commented 1 year ago

Quick summary of the status of the issue: With the help of @marcochavezf and @mountiny, it is decided that the proposal was not enough to solve the RC, hence the scope and reward of the issue is being re-evaluated. Currently, 2 alternatives are being discussed and evaluated with POC

Unifying AuthScreens and PublicScreens to a single RootStack, this enables navigation through the whole application and avoids losing the history stack. This change already has a POC branch here this one is tricky since it might require testing a lot of navigation scenarios and touch the core of the navigation of the app
Parting from this issue 18321 there is already a solution focused on embedding the SignInPage in a modal, the idea would be to reuse this and create a solution that includes Signin in the RHP so the final user can go back.

marcochavezf commented 1 year ago

@daordonez11 can you create a WIP PR of the POC? So it will be easier to check/review the changes made so far. And thanks again for the analysis and the effort!

daordonez11 commented 1 year ago

For sure @marcochavezf here is the draft with the initial POC code: https://github.com/Expensify/App/pull/20883

dylanexpensify commented 1 year ago

Nice one! Looks like we're making good process here

daordonez11 commented 1 year ago

Update for everyone! After some discussion in https://github.com/Expensify/App/pull/20883 it is found that joining both stacks doesn't quite solve the issue hence it is concluded that the next approach tested should be trying SignInPage in the RHP. I will work on a new PR on the weekend with this approach. I expect to have something working for Monday so all of the interested stakeholders can give it a check and test it out.

TLDR; We will start testing option 2 of my last comment

melvin-bot[bot] commented 1 year ago

@marcochavezf, @daordonez11, @dylanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

daordonez11 commented 1 year ago

I have now updated the branch and signin modal is shown and working. I'm still trying to figure out a way to make the SignInPage adaptive without using withWindowDimensions or finding a way to override isSmallScreenWidth

daordonez11 commented 1 year ago

Hey @marcochavezf third is the charm lets hope! Here is the draft PR so you can give it a check, POC of signin modal in RHP https://github.com/Expensify/App/pull/22005

marcochavezf commented 1 year ago

Hi @daordonez11, the POC looks good! Let's continue with the RHP sign-in implementation. Just a few issues I noticed:

we're showing the scrollbar in the RHP (we should remove it)
when the user enters the validation code the validation page is still shown
when the user clicks on one item from the FAB menu, the FAB menu is still open

https://github.com/Expensify/App/assets/6829422/e31cd27e-6224-421d-aa41-f2b379e6d9a9

But overall the POC is great!

daordonez11 commented 1 year ago

Hey Marco! Thanks for the feedback! Will continue working on it then! Also, any feedback from UX? Regarding error #2 it occurs because authToken is cleaned, without that the authentication flow fails. This is automatic due to the logic in AppNavigator I'll think about a workaround on how to avoid it. That is the same reason I created signInAnonymousAccount to handle the specific flow of cleaning authTokenType to trigger the auth flow. Maybe with an optimistic flow I don't need to clean it but use the response or a fake token while I receive the response from the server. I'll test and let you know. I'll check the other issues too.

mountiny commented 1 year ago

Just noting probably obvious but the header of the RHP should have same colour as the page itself.

Once this is going to be more ready we can ask Shawn our designer for a review of the PR from design perspective.

marcochavezf commented 1 year ago

Regarding error https://github.com/Expensify/App/pull/2 it occurs because authToken is cleaned, without that the authentication flow fails. This is automatic due to the logic in AppNavigator I'll think about a workaround on how to avoid it.

Hi @daordonez11! I was exploring some options to solve this issue and seems there's no easy solution because the app is designed to handle one authToken and I saw how clearing it will display the PublicScreens for a second. I think some possible solutions are:

Use the new ClaimAnonymousAccount to change the authToken when the user enters the validation code. But I think we'd need to call some other functions like App.openApp() (which is called in AuthScreens) when the new authToken is generated.
Display a loading page when the user enters the validation code, but maybe that could be a bit disruptive for the user.

Tomorrow I will continue with some tests with the ClaimAnonymousAccount.

dylanexpensify commented 1 year ago

@marcochavezf any updates? Also this should have a C+ right?

marcochavezf commented 1 year ago

Ah sorry I didn't post an update yesterday, but I'm still working on the ClaimAnonymousAccount, I realized that when we're calling BeginSignIn we're creating an account for the entered email, so we'd need to merge accounts in ClaimAnonymousAccount. Today I will continue with the testing/implementation and post an update here at EOD

For C+, yes once the PR is ready for review (which I think will happen after having the ClaimAnonymousAccount command and I estimate it will live next week) we'd need a C+ for review

daordonez11 commented 1 year ago

I'll work on the other mentioned bugs while Marco works on the backend command. Also https://github.com/Expensify/App/issues/20558#issuecomment-1590347789 I think we already had @mollfpr assigned as C+ ! For the first PR

melvin-bot[bot] commented 1 year ago

📣 @mollfpr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

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 📖

marcochavezf commented 1 year ago

Also https://github.com/Expensify/App/issues/20558#issuecomment-1590347789 I think we already had @mollfpr assigned as C+ ! For the first PR

Oh that's correct! @mollfpr should be assigned to the final PR, assigning you to the GH issue

marcochavezf commented 1 year ago

I was playing with the new command (and removing this part of the code to avoid removing the authToken) and the functionality looks good so far. We'd need a way to hide the RHP when the command resolves (I think we'd need to send a new onyx variable from the server to do this):

https://github.com/Expensify/App/assets/6829422/f16f4a46-cc5b-4057-ae2f-68e5fe0c6b05

marcochavezf commented 1 year ago

Hi @daordonez11, I've been making progress with the command, and I realized that we will keep using the Session.signIn function. The app will automatically send the authToken of the anonymous account, and we'll do the "claiming" account in the server if the anonymous authToken is present.

One additional thing that we'd need to do in the App is close the RHP when we insert the new user data into Onyx. I imagine it could be something like this:

Onyx.connect({
    key: ONYXKEYS.SESSION,
    callback: () => {
        if (Session.isAnonymousUser()) {
            return;
        }
        Navigation.navigate(ROUTES.HOME);
    }
});

I'm still not sure where that code should be called, and probably we'd need to check if the RHP is open (but probably not). The changes I'm doing in the server will insert the new authToken and delete the authTokenType property (so Session.isAnonymousUser() will return false as soon as the new data is inserted. I hope those changes will be live next week.

melvin-bot[bot] commented 1 year ago

@marcochavezf, @daordonez11, @mollfpr, @dylanexpensify Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 1 year ago

@marcochavezf, @daordonez11, @mollfpr, @dylanexpensify Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 1 year ago

@marcochavezf, @daordonez11, @mollfpr, @dylanexpensify 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

marcochavezf commented 1 year ago

One of two backend PRs was merged. The other one is still in review.

marcochavezf commented 1 year ago

The other PR was merged 🎉 we'll need to be deployed. I will post an update when the changes are live.

marcochavezf commented 1 year ago

Hi @daordonez11, the last PR was deployed to staging, so the "claim" logic should be live now there. We're just re-using the Session.signIn action to merge the authenticated account into the anonymous account. The app sends under the hood the authToken, we verify it's an anonymous authToken and then do the merge.

I tested the changes with this PR, where I added some "dirty" changes based on yours. Feel free to update your PR using part of the new changes (it's ok to discard my PR since I only used it to test the backend changes quickly 😄).

daordonez11 commented 1 year ago

Got it Marco thanks will continue working on this tomorrow then 🫡

Expensify / App