Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.57k stars 2.9k forks source link

[HOLD for payment 2024-11-07] [$250] [CVP] OD Navigation - Login - Multiple loading screens and animations are visible after a new user signs up #50182

Closed isagoico closed 1 week ago

isagoico commented 1 month 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!


Version Number: 9.0.44-0

Reproducible in staging?: Yes Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5043992&group_by=cases:section_id&group_id=327504&group_order=asc

Email or phone of affected tester (no customers): New User Logs: https://stackoverflow.com/c/expensify/questions/4856

Issue reported by: Applause - Internal team

Action Performed:

  1. Navigate to https://staging.expensify.com/
  2. Tap on "Organize my own expenses"
  3. Input an email address that doesn't have an account yet
  4. Tap on the "Get Started" button
  5. Tap on the "Join" button

Expected Result:

A single loading animation or a logo should be displayed after the user taps on "Get Started", right until the onboarding shows.

Actual Result:

After new user taps on "Get Started", the "Join" button has to be tapped. White screen is showed, Expensify logo is visible for a short time, small "E" logo with the green background is visible for a short time, "Launching Expensify" is visible for a short time, loading spinner is visible for a short time

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

https://github.com/user-attachments/assets/ef20bbb2-b9ff-4bf7-82b1-c494ab523196

View all open jobs on GitHub


This was found when executing the newly added flows for CVP https://github.com/Expensify/Expensify/issues/412920.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021843207040930164854
  • Upwork Job ID: 1843207040930164854
  • Last Price Increase: 2024-10-07
Issue OwnerCurrent Issue Owner: @Christinadobrzyn
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @Christinadobrzyn (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.

Christinadobrzyn commented 1 month ago

I can't get this welcome page to load. I'll test on Monday

melvin-bot[bot] commented 1 month ago

Auto-assign attempt failed, all eligible assignees are OOO.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

Christinadobrzyn commented 1 month ago

Okay I can reproduce this. I think this can be external and let's get design on this to get an idea of what we should be showing.

trjExpensify commented 1 month ago

CVP issues go in f1! πŸ‘

waterim commented 1 month ago

Hello, Im Artem from Callstack and would like to help with this issue!

Christinadobrzyn commented 1 month ago

Hi @waterim - sounds great! I added you to the issue

waterim commented 1 month ago

To be clear Im not sure that the expected result is a real expected result

Detailed summary from my perspective regarding the actual results:

After new user taps on "Get Started", the "Join" button has to be tapped. White screen is showed, Expensify logo is visible for a short time, small "E" logo with the green background is visible for a short time, "Launching Expensify" is visible for a short time, loading spinner is visible for a short time

Actual result looks quite okay excepted of the one blink page:

  1. the "Join" button has to be tapped - its needed for a verification if user exists or not, if its a new user "Join appears"
  2. White screen is just a transition between 2 different pages in 2 different apps (from oldDot to newDot)
  3. Expensify logo is a loading state for, but can't reproduce this one, only have a white screen after the join and after E logo
  4. E Logo is a correct one as a loader for newDot
  5. "Launching Expensify, your session is expired" - this one looks like a bug and from what I can see the response is undefined for the first call (actually in the network there is only one call and it has a response) image

And here after we got an undefined we are setting signedInWithShortLivedAuthToken: null

image

And after we are getting a correct response with all the data, but because we had an signedInWithShortLivedAuthToken as null for a second we have this "session is expired" for a moment.

image
  1. loading spinner is a correct state when we've got a new data for the SignInWithShortLivedAuthToken

It looks like a backend issue as we are getting an undefined for a moment after a call and all other steps are looking fine from a perspective we are making a transition from oldDot to a newDot, and we cant do this with one loader because different websites are rendering

carlosmiceli commented 1 month ago

@waterim Oh, just read your comment, I'll look into in the BE and report back if we need to fix anything here. Thank you! πŸ’ͺ

carlosmiceli commented 1 month ago

PR for adding a splash screen to replace the first white screen is ready: https://github.com/Expensify/Web-Expensify/pull/43944

Need to wait for Web-Static deployment to confirm the icon is showing up properly and we can wrap it up.

Moving to the auth issue next.

carlosmiceli commented 1 month ago

@waterim can you help me check something? As I'm seeing, we're sending the shortLivedAuthToken from the BE successfully, but the issue seems to be that isLoading continues to be false and therefore it doesn't pass this check.

We don't send the SignInWithShortLivedAuthToken request twice, right? If it was null, how does it get it a second time? also, route already seems to have all the necessary information?

Screenshot 2024-10-13 at 9 03 32β€―PM

Is it possible there's some logic to update in getShortLivedLoginParams that explains why it does this? Or if not, can you help me see what is it exactly that's missing from the response that would end up in isLoading as false?

I'm a bit new to this part of the App so let me know if I'm way off, but I'm not finding yet an issue in the BE (though there may be one, troubleshooting this even made me discover a duplicate account status call).

carlosmiceli commented 1 month ago

Small bump @waterim πŸ™

waterim commented 1 month ago

Oh, didn't see, will check tomorrow!

waterim commented 1 month ago

@carlosmiceli Okay, thank you, I see

Thats an issue with getShortLivedLoginParams function, it was updated to set signedInWithShortLivedAuthToken: true to run App.openApp and after setting signedInWithShortLivedAuthToken: null to ensure the user is logged out on refresh, don't know what was a point of this

If the user is signing in with a different account from the current app, should not pass the auto-generated login as it may be tied to the old account. scene 1: the user is transitioning to newDot from a different account on oldDot. scene 2: the user is transitioning to desktop app from a different account on web app.

This is the comment from there why it was added. Im not sure we need to change it as it can brake these 2 scenarios with switching to new accounts

carlosmiceli commented 1 month ago

@waterim interesting... but we confirm that this is a FE issue, right?

waterim commented 1 month ago

@carlosmiceli yes, sorry for pinging you with a backend, but looks like its an FE issue, I thought its a failureData, but it was a finallyData and this blink is needed to be sure that new account will be rendered from the beginning, without showing an old data(as far as I understand)

carlosmiceli commented 1 month ago

That's no prob! Do you feel like you can take it and propose a solution? I think @blazejkustra can also give us a hand/hint here since he worked on that function in the past. πŸ™

waterim commented 1 month ago

@carlosmiceli Yes, I can try to figure it out and will be great if @blazejkustra can give us some information about this part

melvin-bot[bot] commented 1 month ago

@carlosmiceli @waterim @Christinadobrzyn @eh2077 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

carlosmiceli commented 1 month ago

@waterim that's great, thank you! Let's not to wait on @blazejkustra though in case he's too busy or OOO, but I'm sure he'll give us some insights if he has some time :)

Let me know if there's anything I can help with in the meantime!

blazejkustra commented 1 month ago

@carlosmiceli Sorry, but I was OOO last week πŸ˜„

I think @blazejkustra can also give us a hand/hint here since he worked on that function in the past. πŸ™

Hmm, I don't remember working on these functions πŸ€” It's possible that I'm in the git blame because I migrated the file to Typescript.

carlosmiceli commented 1 month ago

@blazejkustra Ah got it. @waterim how are things looking?

waterim commented 1 month ago

@carlosmiceli continuing on this, no updates for now

waterim commented 4 weeks ago

@carlosmiceli Do you have an idea how I can access the olddot dev environment to navigate to dev new dot? Because its very hard to find a way to fix it without testing it actually in the dev env of the newDot

carlosmiceli commented 4 weeks ago

Mmm, I'm 99% sure that there's no way to do that, right @trjExpensify? If the issue is in ND but you absolutely need to have access to OD, then maybe I should grab the ND side of this issue as well, but I'll let Tom confirm!

trjExpensify commented 4 weeks ago

That's a good question for #engineering-chat. πŸ‘

carlosmiceli commented 4 weeks ago

Yeah, good call! @waterim Just asked and if you can confirm that this can't be done without access to OD, then I'll have to grab it because there's no way to provide access there for the moment.

waterim commented 4 weeks ago

From my side it looks like impossible to test it properly, I tried to debug just with staging, but can't log anything or test without redirecting to a local newDot, but from my side it looks like we don't want to set signedInWithShortLivedAuthToken: null as this one is causing this "session is expired" and playing with Onyx data for this specific case can be a good solution imo, but cant test to prove this solution unfortunately :(

carlosmiceli commented 4 weeks ago

Right, that's a bummer! No worries, I'll grab it and will let you know how it goes and if you can help out as well πŸ™ Thank you!

carlosmiceli commented 3 weeks ago

Didn't get to this yet, should be able to work on it tomorrow.

carlosmiceli commented 3 weeks ago

The fix ended up being a lot simpler, we needed to use account from Onyx, not as a prop: https://github.com/Expensify/App/pull/51450

Christinadobrzyn commented 3 weeks ago

PR in the works! @carlosmiceli can we move this to weekly while the PR is being worked on?

carlosmiceli commented 3 weeks ago

Sorry, what needs to be worked on? All PR fixes have been merged for this.

carlosmiceli commented 3 weeks ago

But yes, we can move this to weekly until it's closed!

Christinadobrzyn commented 3 weeks ago

Oh perfect! Sorry I meant move to weekly while the PR is going into production. Thanks!

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

If no regressions arise, payment will be issued on 2024-11-07. :confetti_ball:

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

melvin-bot[bot] commented 2 weeks ago

@eh2077 @Christinadobrzyn The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

eh2077 commented 2 weeks ago

C+ wasn't involved in this task, so no payment needed here and unassigning myself

Christinadobrzyn commented 2 weeks ago

Do we need a regression test for this?

Christinadobrzyn commented 1 week ago

Dmd with @carlosmiceli no regression needed here so closing!