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.26k stars 2.69k forks source link

[$250] Chats don’t load when logging in using Google SSO from public room banner. #41246

Closed m-natarajan closed 2 weeks ago

m-natarajan commented 3 months 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: 1.4.67-2 Reproducible in staging?: Yes Reproducible in production?: yes 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: @garrettmknight Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1714391455919649

Action Performed:

  1. Go to staging on Chrome - log out if you need to
  2. Access public room - https://staging.new.expensify.com/r/2376199970894587
  3. Navigate to the self DM that populates in LHN
  4. Click ‘Sign in’ in the banner
  5. Sign in w/ Google

    Expected Result:

    Your signed in profile’s chats load

    Actual Result:

    No additional chats load, ‘Hmm…’ flashes

    Workaround:

    Unknown

    Platforms:

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

    • [ ] Android: Native
    • [ ] Android: mWeb Chrome
    • [ ] iOS: Native
    • [ ] iOS: mWeb Safari
    • [X] MacOS: Chrome / Safari
    • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/38435837/faf8e560-0869-4fd9-b9ab-f7ac8ed5156a

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0140408c8876b6a1dd
  • Upwork Job ID: 1785085627943718912
  • Last Price Increase: 2024-05-14
  • Automatic offers:
    • alitoshmatov | Reviewer | 0
Issue OwnerCurrent Issue Owner: @cristipaval
melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

nitinigi2 commented 3 months ago

Just add a page reload once user's SSO signIn is done. That will do the job.

melvin-bot[bot] commented 3 months ago

📣 @nitinigi2! 📣 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. Make sure you've read and understood the contributing guidelines.
  2. 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.
  3. 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.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
lschurr commented 3 months ago

Hi @alitoshmatov - Can you take a looksie at this one?

alitoshmatov commented 3 months ago

Reproduced, couldn't find root cause yet

melvin-bot[bot] commented 3 months ago

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

lschurr commented 3 months ago

Any update @alitoshmatov? Are we just waiting for proposals?

alitoshmatov commented 3 months ago

Yes waiting for proposals here

lschurr commented 3 months ago

Still waiting on proposals.

lschurr commented 3 months ago

Still waiting on proposals.

melvin-bot[bot] commented 3 months ago

@lschurr @alitoshmatov 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!

melvin-bot[bot] commented 3 months ago

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

rezkiy37 commented 3 months ago

Hi, I’m Michael (Mykhailo) from Callstack and I would like to work on this issue.

rezkiy37 commented 3 months ago

Hey! I confirm that the bug is replicable. Moreover, the bug repeats for Apple SSO as well.

I've investigated data that the app gets from the backend for the SignInWithGoogle and SignInWithApple commands. Actually, there is a huge difference between responses for SigninUser and SSO. The backend does not return reports and many other data.

Details

Screenshot 2024-05-15 at 14 47 00 Screenshot 2024-05-15 at 14 48 11 Screenshot 2024-05-15 at 14 51 47

There are 2 obvious solutions:

  1. Sync responses for all kinds of authorization and send the same data.
  2. For the SSO authorizations, the app can call OpenApp or ReconnectApp right after the authorization has happened.

I read contributingGuides/APPLE_GOOGLE_SIGNIN.md. I've not found any info that this type of authorization omits some data to send to the app. Also, it requires some additional configs to test the development mode due to the SSO restrictions. However, it is not required here, because I assume the bug should be fixed on the backend side.

So we need an internal engineer to help with the issue.

lschurr commented 3 months ago

@alitoshmatov - what do you think?

melvin-bot[bot] commented 3 months ago

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

lschurr commented 3 months ago

Not sure why the Overdue persists. Bump here @alitoshmatov

lschurr commented 3 months ago

@rezkiy37 @alitoshmatov - Do we also need to make this internal?

melvin-bot[bot] commented 3 months ago

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

Offer link Upwork job

rezkiy37 commented 3 months ago

@rezkiy37 @alitoshmatov - Do we also need to make this internal?

Looks like yes.

alitoshmatov commented 3 months ago

Sorry for late response.

Yes we need internal engineer here based on @rezkiy37 observation

lschurr commented 3 months ago

Added Internal here to find an engineer to work on this.

lschurr commented 2 months ago

No update on this one.

lschurr commented 2 months ago

Asked in Slack - https://expensify.slack.com/archives/C066HJM2CAZ/p1717084703979449

lschurr commented 2 months ago

Bumped the Slack thread.

lschurr commented 2 months ago

No update.

lschurr commented 2 months ago

Still don't have an internal eng volunteer.

lschurr commented 2 months ago

Asking in newdot-quality: https://expensify.slack.com/archives/C05LX9D6E07/p1718293036755239

alitoshmatov commented 2 months ago

@rezkiy37 It has been a while but can you take a look at it again: reports data is being received with openApp request, any idea why it is not being applied?

Screenshot 2024-06-13 at 10 22 49 PM
rezkiy37 commented 2 months ago

Yes, I can confirm that the bug no longer exists.

Details

https://github.com/Expensify/App/assets/57314631/6e3144c7-7005-46b9-a972-8ac43e162e5b https://github.com/Expensify/App/assets/57314631/badc98d8-f607-42e9-a211-637ba5800ee9

cc @lschurr

lschurr commented 2 months ago

Ah, perfect thanks! Closing.

alitoshmatov commented 2 months ago

@rezkiy37 I think you missed couple of steps to reproduce. It is still reproducible for me. I was just sharing my finding here

https://github.com/Expensify/App/assets/59907218/4bf97842-dc40-402b-bcc6-b24bf1bbbe8b

cc: @lschurr

rezkiy37 commented 2 months ago

Ah, exactly. My bad. Let's reopen the issue. Seems like the app should call OpenApp for this scenario.

rezkiy37 commented 2 months ago

I've found out why the app does not show much data alongside reports.

The app calls OpenApp right after Google or Apple authorization. However, the app initially calls ReconnectApp. It sets lastUpdateID from the very beginning. It is 438474121 in my case. The OpenApp response has lastUpdateID as well. It is 438465717.

Screenshoots Screenshot 2024-06-18 at 16 52 33 Screenshot 2024-06-18 at 16 52 58

There is a condition in how the app checks whether it should apply new coming onyx data. It compares the current lastUpdateID with the last one.

https://github.com/Expensify/App/blob/f6085d284bfb69c837fbac7e3b4e3fba9853df26/src/libs/actions/OnyxUpdates.ts#L101

https://github.com/Expensify/App/blob/f6085d284bfb69c837fbac7e3b4e3fba9853df26/src/libs/actions/OnyxUpdates.ts#L115-L116

As we can see above, if there was an update with greater lastUpdateID, the app omits onyxData.

https://github.com/Expensify/App/blob/f6085d284bfb69c837fbac7e3b4e3fba9853df26/src/libs/actions/OnyxUpdates.ts#L104-L105

Therefore we can see that the OpenApp's lastUpdateID is less when ReconnectApp's lastUpdateID.

438465717 < 438474121 // true

The app does not take Onyx data into account.

Regarding this comment - https://github.com/Expensify/App/issues/41246#issuecomment-2168447420. I tested with the primary login page. The app does not call ReconnectApp from the very beginning on the login page. That's why the app applies OpenApp's Onyx data.

Please re-open the issue 🙂

cc @alitoshmatov @lschurr

alitoshmatov commented 2 months ago

Thank you @rezkiy37 for pretty in-depth explanation 👍, that makes sense. @lschurr Looks like we do need internal engineer after all, since we have problem with backend response.

lschurr commented 2 months ago

Bumped the Slack thread - https://expensify.slack.com/archives/C05LX9D6E07/p1718293036755239

lschurr commented 1 month ago

Still need an engineer here.

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @flodnv (AutoAssignerNewDotQuality)

flodnv commented 1 month ago

I read the issue, and I don't understand why this needs to be Internal. Can we get an external contributor to work on it?

rezkiy37 commented 1 month ago

@flodnv, because it requires changes on the backend side.

flodnv commented 1 month ago

Right, I read your comment 3 times and I'm sorry but I still don't understand which change we need on the backend side. If you had access to the backend code, what change would you make? Or maybe you can present this in a Problem/Solution form so I can understand better?

rezkiy37 commented 1 month ago

Well, lastUpdateID comes from the backend. This value is deprecated for this scenario. So the backend should increment it correctly. Does it make sense?

alitoshmatov commented 1 month ago

@flodnv I think the main issue we are struggling here is that even though reconnectApp is called first it is getting higher lastUpdateID then openApp request which is called after this and its changes are not applied due to its lower lastUpdateID. These lastUpdateIDs are issued from backend. I think naturally request order should also reflect lastUpdateID.

Maybe there is a race condition going on?

flodnv commented 1 month ago

Okay, thanks for the (re)explanation, I can look into that. My first step is obviously to reproduce myself.

Go to staging on Chrome - log out if you need to Access public room - staging.new.expensify.com/r/2376199970894587 Navigate to the self DM that populates in LHN

Note: I don't see a self DM.

Click ‘Sign in’ in the banner Sign in w/ Google

I was able to reproduce though.

@rezkiy37 @alitoshmatov do we call the same set of APIs (in the same order) from the App when signing in via Google as when signing in via magic code in this specific flow? It's surprising to me that this bug only exists on Google Signin.

flodnv commented 1 month ago

the app initially calls ReconnectApp

I tested a few times watching the network console and I do not see a call to ReconnectApp

Screenshot

image

However, I do see 2 calls to OpenApp. @rezkiy37 @alitoshmatov what are your thoughts?

rezkiy37 commented 1 month ago

@flodnv, it does call ReconnectApp for me on the dev, but it does not call on the staging. So I tried to test the staging and what I found. There is an OpenReport response that has the same lastUpdateID as OpenApp has. This OpenReport has been called before and sets its own lastUpdateID. The next OpenApp is skipped due to the condition:

https://github.com/Expensify/App/blob/f6085d284bfb69c837fbac7e3b4e3fba9853df26/src/libs/actions/OnyxUpdates.ts#L101

Screenshots Screenshot 2024-07-01 at 17 45 24 Screenshot 2024-07-01 at 17 45 54 Screenshot 2024-07-01 at 17 46 11

Does it make sense now?

lschurr commented 1 month ago

Any update here @alitoshmatov @flodnv?

lschurr commented 1 month ago

Just bumping again for an update @alitoshmatov @flodnv :)