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

[$500] Login – No Abracadabra page when logging in via Magic link #44600

Closed
3 of 6 tasks
izarutskaya opened this issue Jun 28, 2024 · 85 comments
Closed
3 of 6 tasks

[$500] Login – No Abracadabra page when logging in via Magic link #44600

izarutskaya opened this issue Jun 28, 2024 · 85 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review

Comments

@izarutskaya
Copy link

izarutskaya commented Jun 28, 2024

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: 9.0.3-1
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4678326
Email or phone of affected tester (no customers): applausetester+jp_228624@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Log in with new Expensifail account
  3. Copy the Magic link and change it to staging
  4. Open a new tab and navigate to the staging magic link

Expected Result:

Abracadabra page is displayed

Actual Result:

Inbox page opens. Abracadabra page isn’t displayed

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

Bug6527186_1719565721053.no_Abracadabra_page.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e239faee992134cd
  • Upwork Job ID: 1806768165074984740
  • Last Price Increase: 2024-08-13
  • Automatic offers:
    • HezekielT | Contributor | 103331186
Issue OwnerCurrent Issue Owner: @kirillzyusko
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Jun 28, 2024
Copy link

melvin-bot bot commented Jun 28, 2024

Triggered auto assignment to @garrettmknight (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Copy link

melvin-bot bot commented Jun 28, 2024

Triggered auto assignment to @dangrous (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Jun 28, 2024
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.

@izarutskaya
Copy link
Author

@garrettmknight I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@dangrous
Copy link
Contributor

This is definitely external. Haven't figured out the cause yet, I need to remember how to send magic links from dev.

@dangrous dangrous removed the DeployBlocker Indicates it should block deploying the API label Jun 28, 2024
@dangrous
Copy link
Contributor

So I just tried this with the production build and the production server on my local and still had it occur.

I think this might be something just with copying and modifying the link, rather than literally clicking on it... I vaguely remember something about that in another bug in the past. But I'm not confident yet - other ideas welcome!

@dangrous
Copy link
Contributor

Trying to get some assistance here - https://expensify.slack.com/archives/C01GTK53T8Q/p1719583721705269

@garrettmknight garrettmknight added Daily KSv2 Hourly KSv2 and removed Hourly KSv2 Daily KSv2 DeployBlockerCash This issue or pull request should block deployment labels Jun 28, 2024
@garrettmknight
Copy link
Contributor

The abracadabra page is just a confirmation so I don't think we need to call this a blocker.

@garrettmknight garrettmknight added the External Added to denote the issue can be worked on by a contributor label Jun 28, 2024
Copy link

melvin-bot bot commented Jun 28, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01e239faee992134cd

@melvin-bot melvin-bot bot changed the title Login – No Abracadabra page when logging in via Magic link [$250] Login – No Abracadabra page when logging in via Magic link Jun 28, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 28, 2024
@hannojg
Copy link
Contributor

hannojg commented Aug 29, 2024

why email link can be received in 2 formats? Sometimes it is

To answer that part I think that happens through the email tool expensify uses to send the mails and its a question of whether your email client followed the redirect yet. You can put the url in curl to get the /v/ url, as it won't follow the redirect:

Screenshot 2024-08-29 at 09 41 24

@dangrous
Copy link
Contributor

@hannojg I saw an email about a comment you sent confirming expected behavior, since that's deleted I'm guessing you figured it out? Short version - yes, opening the magic link should authenticate the original tab/window.

@hannojg
Copy link
Contributor

hannojg commented Aug 30, 2024

Yeah, sorry, when writing that comment I was still confused 😄

So, we've found a solution, however, that solution currently doesn't work in strict mode (debug), but will work in release. How open are we to "only" fix that for release and not debug?

We can spend more hours making this fix also viable for debug, just wanted to check with you first if its worth the effort?

@hannojg
Copy link
Contributor

hannojg commented Aug 30, 2024

Note: we actually have a solution available now that works also in strict mode 😊

@dangrous
Copy link
Contributor

oh awesome!

@hannojg
Copy link
Contributor

hannojg commented Aug 30, 2024

We have a PR here which fixes the issue:

Screen.Recording.2024-08-30.at.17.42.11.mov

The main fix we found is to disable the cleaning mechanism entirely when switching navigators in react-navigation. When we sign in and switch from our PublicScreens to AuthScreens we explicitly expect the navigation state to persist. There are no built-in mechanisms to stop the cleanup from happen (it has internal logic for that but it is buggy as Kiryl pointed out).
Instead we add an explicit clean point when we signout, as this is the only place to my understanding where we'd ever want to drop our navigation state.

Alternative solutions

The main problem is, that react-navigation's cleanup mechanism is build on the assumption that when you unmount a navigator, you either instantly mount a new one - or no new navigator is mounted instantly, in which case we want to clear the state.

In Expensify's case our navigators are wrapped with <Suspense /> and it can happen that our navigators "are rendered" but are only really mounted after X milliseconds. This breaks react-navigation's assumption.

We could work out a reproduction for react-navigation, open an issue there, and work on a fix with Satya, but I believe this could take days. Not sure if thats worth for that issue?

@hannojg
Copy link
Contributor

hannojg commented Aug 30, 2024

I will finish the PR with testing all platforms tomorrow - however, we could already assign someone for testing (or do an adhoc build and ask people to test it). I am slightly unsure whether I overlooked some case where we'd also need to clean the navigator state.

@dangrous
Copy link
Contributor

okay great! Yeah I think if it's only specifically this issue, we can fix it the way you said rather than going into react-navigation. Probably not worth that.

As for testing, @aimane-chnaif you up for starting to take a look at it?

@hannojg
Copy link
Contributor

hannojg commented Sep 2, 2024

Note: I am OOO, back on Wednesday. I realised that in the PR I do a thing that shouldn't be needed, and there seems to be a bug in react-navigation with that. Will continue on Wednesday (or @kirillzyusko feel free to take over while I am away if you're available).

Note: probably not worth to review the PR now, lets wait until we have a fix react for react-navigation @aimane-chnaif

@dangrous
Copy link
Contributor

dangrous commented Sep 9, 2024

How are we looking on this one @hannojg? Thanks!

@melvin-bot melvin-bot bot added the Overdue label Sep 9, 2024
@kirillzyusko
Copy link
Contributor

@dangrous Hanno is OOO for two weeks, so I'm taking this over again and will try to fix it ASAP!

@melvin-bot melvin-bot bot removed the Overdue label Sep 10, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Sep 19, 2024
@melvin-bot melvin-bot bot removed the Weekly KSv2 label Oct 14, 2024
Copy link

melvin-bot bot commented Oct 14, 2024

This issue has not been updated in over 15 days. @garrettmknight, @dangrous, @kirillzyusko, @HezekielT, @aimane-chnaif 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 Oct 14, 2024
@dangrous
Copy link
Contributor

Waiting on library review

@dangrous
Copy link
Contributor

dangrous commented Nov 4, 2024

we're discussing expected behavior for this one again, we may not end up needing it?

https://expensify.slack.com/archives/C03TQ48KC/p1730456781086199

@dangrous
Copy link
Contributor

dangrous commented Nov 6, 2024

Okay we're closing this issue, and will update the expected behavior to match the current behavior. Thanks for investigating @kirillzyusko!!

Can anyone confirm whether or not we need payment here? It's been a long road with a lot of PRs (many of which reverted) so I'm a little confused haha

@dangrous
Copy link
Contributor

i'm going to go ahead and close and if anyone needs payment we can reopen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests