[C+ payment required, Situ out until May] [HOLD for payment 2024-04-03] [$500] Desktop - Login - Unable to enter the 2FA code or exit the opened screen on desktop

[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.23-0 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:

Prerequisites: User must be logged into web app as User A.

1. Launch the Desktop app
2. Enter an email for an existing account with 2FA (User b)
3. Go to the email inbox and locate the magic code
4. Copy the Magic link and modify it to staging
5. On the web, open a new tab and navigate to the staging magic link
6. Confirm that the user is prompted to open the desktop app
7. Click on the "Open app" option

Expected Result:

When the desktop is launched by the magic code link, it glitches, briefly shows a skeleton and then returns to the first login screen

Actual Result:

When the desktop is launched by the magic code link, there's just a blocker screen asking to enter the magic code, with no field and no option to exit the screen

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
• [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/f07d7998-7fff-41b6-9f79-6dc2ef507cbd

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0183573e4ff04521ad
• Upwork Job ID: 1744794019492372480
• Last Price Increase: 2024-03-06
• Automatic offers:
• situchan | Contributor | 28115507

Issue Owner

Current Issue Owner: @trjExpensify

[trjExpensify]

Ah cool, I'll defer to @situchan to take a look as is then! Perhaps @johnmlee101 or @NikkiWines would like to be the secondary reviewer from our team internally when ready.

I don't have the full context of this issue but i request to put it on hold in favour of https://github.com/Expensify/App/pull/35359 if this both are related, We migrated the files to typescript and the PR will be merged in coming days

Seems like this might be wise though. What does everyone think?

[ikevin127]

Not sure, my solution requires a small addition within src/pages/ValidateLoginPage/index.website.js which I don't see as part of the migration within the mentioned PR 👀

[!NOTE] As far as I can tell that PR doesn't touch on the 2FA logic we're looking at improving on this issue, but rather it's related to the 2FA from within the Settings page (where you set it up).

Anybody else ?

cc @GandalfGwaihir

[situchan]

No need to hold for TS migration

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[trjExpensify]

Cool, @situchan can you jump on the proposal review then, please? Thanks!

[trjExpensify]

Bump, @situchan. What's your ETA on reviewing?

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[situchan]

reviewing today/tomorrow based on latest discussion

[situchan]

still evaluating proposals. eta: end of week

[trjExpensify]

@situchan what's going on here? It has been 2 weeks now.

P.s @greg-schroeder I'm moving this into wave6 as LOW. It's a sign-in bug that can have an impact on anyone with 2FA, but given submitters is the largest population of users, wave6 makes sense to me.

[greg-schroeder]

I guess so, but it's not really in the scope of any of our upcoming releases, so I'll leave it at LOW until a home becomes clear or it just gets resolved

[situchan]

@ikevin127 @tienifr can you please share test branches? It should fix both 2fa and no 2fa cases.

[situchan]

And please update your proposals with the correct root cause. Especially, why is not user prompted to open the desktop app? (both 2fa and no 2fa)

[ikevin127]

The review took so long to get started that navigation refactor was performed in the meantime and my initial proposal doesn't work anymore, specifically the Navigation.navigate() part which doesn't work anymore if no arguments are passed as per this comment here https://github.com/Expensify/App/issues/35782#issuecomment-1927837173:

Also Navigation.navigate() without any parameters doesn't seem to work after navigation refactor.

And those parts of the old logic in the ValidateLoginPage were updated by the following PR https://github.com/Expensify/App/pull/35850 to account for the recent navigation refactor since Navigation.navigate() without any parameters doesn't seem to work anymore.

I'm looking into the logic and if I can find a way to solve the issue just as my initial proposal did, before navigation refactor, then I'll get back and update my proposal. If I don't come back with any update then it means I'm out of the race for this issue 🏳

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[situchan]

awaiting proposals based on latest codebase

[trjExpensify]

Any luck, @ikevin127?

[ikevin127]

@trjExpensify Unfortunately, no luck 😔

Did some extensive digging and I could not get my initial proposal to work with the new navigation logic (bottom tab navigator). Since the new navigation was implemented, Navigation.navigate() does not work anymore in calling navigate to the home route like it did before and I suspect this has to do with the new navigation logic -> which I cannot fully understand the inner workings of.

Here is the involved code (before / after)

| Before | After | | ------------- | ------------- | | https://github.com/Expensify/App/blob/76e5b1cbd80e7dc55c52f47c5572b2984a7f7ed6/src/libs/Navigation/linkingConfig.ts#L33-L36 | https://github.com/Expensify/App/blob/9bfe961622d03de5ac8e18652bbb56dda5ed9f60/src/libs/Navigation/linkingConfig/config.ts#L26-L38 | | Before | After | | ------------- | ------------- | | https://github.com/Expensify/App/blob/76e5b1cbd80e7dc55c52f47c5572b2984a7f7ed6/src/libs/Navigation/AppNavigator/PublicScreens.tsx#L19-L23 | https://github.com/Expensify/App/blob/9bfe961622d03de5ac8e18652bbb56dda5ed9f60/src/libs/Navigation/AppNavigator/PublicScreens.tsx#L20-L25 |

Besides these important changes in routing structure, we also had some important changes to the linkTo function which actually calls navigate when Navigation.navigate() is called.

If we replace Navigation.navigate() with Navigation.goBack() as the other logic was replaced since new navigation logic, in our case that would just navigate us to the "Here is your magic code" page where if we click 'just sign in here' button nothing happens on Desktop.

Maybe we could ask some of the people that worked on the new navigation logic for some hints as to how we could achieve having the magic code step performed and be redirected to the 2FA page as demonstrated in my after video (@ 0:20 seconds):

Video

https://github.com/Expensify/App/assets/56457735/7d429304-1d87-4ac2-895f-292ab662f7ba

cc @adamgrzybowski (not entirely sure I'm tagging the right person, if I'm wrong I apologize)

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot[bot]]

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

[situchan]

still awaiting proposals

[trjExpensify]

Thanks, @ikevin127. @adamgrzybowski is definitely the right person, though I think he might be OoO right now. CC: @WojtekBoman @kosmydel @mountiny for some input on this please, thanks!

[WojtekBoman]

Hey! @ikevin127 Could you share the branch with your changes? I'll try to reproduce it and check what's wrong

[ikevin127]

@WojtekBoman Sorry I don't have a branch, all I have is my initial proposal - which worked as described in solution (see videos) before the new navigation.

Also this is another issue and its PR https://github.com/Expensify/App/pull/35850 that changed the old implementation to use Navigation.goBack() because after the new navigation Navigation.navigate() did not work anymore like it used to be.

So I guess what would help here is to better understand how we can call a similar navigation function o achieve the same as we did previously using Navigation.navigate() - because if how the new bottom tab navigator is structured currently, calling navigate with home route while non-authenticated does not seem to work at all anymore.

Another outcome that would help here is to have a way of opening web prompt with magic link code in desktop app, process the code in the desktop app and:

• if the account has 2FA, redirect to 2FA code page
• if account doesnt have 2FA, perform login

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[trjExpensify]

@WojtekBoman any thoughts on the above?

[adamgrzybowski]

Hi! I will try to help

[trjExpensify]

Going to assign @adamgrzybowski so he can help @ikevin127 here.

[adamgrzybowski]

@ikevin127 Not sure if I understand the problem completely but you can try to use goBack() to pop the screen and then navigate()

[ikevin127]

Updated proposal

• changed initial solution code of stack popping method, since new navigation logic was implemented and the old code did not work anymore

@situchan As per #34177 (comment) ->

And please update your proposals with the correct root cause.

Please explain what you mean by correct root cause so I know what other details need to be added to the proposal, if any. Below are detailed steps + video to consistently reproduce and test the issue / proposed solution:

1. Open incognito browser tab.
2. Navigate to https://staging.new.expensify.com.
3. Login with any account (once logged in, a prompt to Open in New Expensify / Electron [for dev] should appear if you have the desktop app installed / running in dev).
4. Once logged in, open the Desktop app (dev or staging - depending on what you test).
5. Login with 2FA account.
6. Copy the magic link received via email as a result of login within the Desktop app.
7. Open a new incognito tab next to the already logged in account.
8. Paste the magic link and add staging. as a prefix, eg. email magic link: https://new.expensify.com/v/15879736/903350 will become https://staging.new.expensify.com/v/15879736/903350.
9. Press enter and navigate to the staging magic link, this will open the prompt to Open in New Expensify / Electron (for dev).

Video

https://github.com/Expensify/App/assets/56457735/28f256e6-acf1-4168-a32d-0a0e7c693bc5

[!note]

1. In order to repeat the cycle, make sure to clear all onyx keys from Application > IndexedDB, otherwise you will stay locked on the 2FA lock screen.
2. The video above was filmed at the time of this comment -> proving the issue is still reproducible.
3. Here's the test branch w/ my proposal's solution applied.

@adamgrzybowski Thanks for the input, after doing some more digging I was able to figure out how to re-write the fix which worked before the new navigation implementation.

Since just navigate() or goBack() + navigate() wouldn't work anymore because of the new navigation implementation, I figured that what navigate() alone was doing before was popping the stack, so I did the same now but more directly by using navigationRef.current?.dispatch(StackActions.popToTop()).

[trjExpensify]

Thanks, @ikevin127. Perhaps @adamgrzybowski & @situchan can check out that test branch?

[adamgrzybowski]

@ikevin127 Thanks for the great description and instructions for reproduction 🙇 ! Makes my life much easier 😄

I tested it and it works 🎉 I think it should work fine in this case.

Do you have a PR open so I can look into diff easier?

BTW it looks like the solution you found navigationRef.current?.dispatch(StackActions.popToTop()) may be handy in some other use cases. I talked with @WojtekBoman and we think that it may be a good idea to create a function for that in Navigation.ts just to make reusing it easier.

[ikevin127]

@adamgrzybowski Thanks for the great feedback! I'm glad the instructions made testing this easy 🙌

Do you have a PR open so I can look into diff easier?

I don't have a PR because @situchan did not review the proposals yet and I am not assigned to this as a Contributor, hence opening a PR is not recommended. But I can do it if this is an exception - let me know!

I created the branch in order to make testing easier for the reviewers.

I talked with WojtekBoman and we think that it may be a good idea to create a function for that in Navigation.ts just to make reusing it easier.

If assigned as a Contributor to this issue I'm fully on board w/ adding this in the PR as a function within Navigation and using it both in my proposal and other parts of the app (where needed) 🚀

What do you think about Navigation.popToTop() as a name ? Open to more suggestive names!

[situchan]

@adamgrzybowski you can check diff here: https://github.com/Expensify/App/compare/main...ikevin127:Expensify:fix/34177

[adamgrzybowski]

We had discussion with @WojtekBoman. The function that he needs will require to do popToTop on two navigators (root and bottom tab)

I think for now you can just create Navigation.popToTop() function and later we will check if his function works also in your case. If yes then we could leave only the version with popToTop on two navigators.

Sorry if this is confusing but he will be OOO soon and I don't want to block you with his work 😄

[mountiny]

@situchan @adamgrzybowski seems like you are happy to go with this @ikevin127 proposal, any concerns?

[situchan]

sure, let's assign @ikevin127

[adamgrzybowski]

@mountiny it's good ✅

[mountiny]

Thanks

[melvin-bot[bot]]

❌ There was an error making the offer to @ikevin127 for the Contributor role. The BZ member will need to manually hire the contributor.

[ikevin127]

PR https://github.com/Expensify/App/pull/38142 is ready for review! 🚀

[ikevin127]

We had discussion with @WojtekBoman. The function that he needs will require to do popToTop on two navigators (root and bottom tab).

Just solved a conflict on the PR and I ended up using newly added Navigation.resetToHome() as it's more robust and it accomplishes the same thing for our issue (double-checked).

Edit: added this to my proposal to correctly reflect the latest PR implementation.

[adamgrzybowski]

Great to hear that it this function works for your use case as well! Thanks for testing this 🙇

[ikevin127]

@situchan 🙋‍♂️ It's been 3 days since the PR is ready for review - is there anything blocking the process ? Please let us know!

[situchan]

I reviewed and commented yesterday. Going ahead today.

[ikevin127]

👋 From what I know the Weekly label on an issue means that the proposals should be reviewed on a weekly basis.

Does that also apply to the PR reviewing process ?

And if the answer is yes - given the following turn of events:

• reviewer attempts review for the first time
• there are some conflicts, reviewer tags author to let them know
• conflicts solved the same day by author

Next time when the PR is checked again could be up to one week later ?

I'm trying to figure out what do the time related labels mean in the context of the PR being ready for review and what are the expected timelines for completion.

cc @stitesExpensify @mountiny

[mountiny]

I dont think the priority label influences how the PR should be reviewed or proposals should be reviewed. That should still be ideally ever day or once in 48 hours at worst.

Made it daily to avoid confusion

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.56-8 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/38142

If no regressions arise, payment will be issued on 2024-04-03. :confetti_ball:

For reference, here are some details about the assignees on this issue:

• @ikevin127 requires payment (Needs manual offer from BZ)
• @adamgrzybowski does not require payment (Contractor)
• @situchan requires payment automatic offer (Contributor)

[melvin-bot[bot]]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [ ] [@situchan] The PR that introduced the bug has been identified. Link to the PR:
• [ ] [@situchan] 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:
• [ ] [@situchan] 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:
• [ ] [@situchan] Determine if we should create a regression test for this bug.
• [ ] [@situchan] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [ ] [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: