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

[LOW] [Splits] [$500] IOU - Error appears after login via IOU URL when using public account #34528

Closed
1 of 6 tasks
lanitochka17 opened this issue Jan 15, 2024 · 91 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 15, 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: 1.4.25-1
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:

Precondition: User is logged in https://staging.new.expensify.com/. in public account

  1. Create a IOU in a conversation
  2. Navigate to the IOU report (conversation)
  3. Copy the URL
  4. Log out of the app
  5. Navigate to the IOU URL
  6. Log in

Expected Result:

You're redirected to the correct IOU report after login

Actual Result:

Hmm it's not here page displayed in conversation history when using public account. User need to refresh page

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

Add any screenshot/video evidence

Bug6343351_1705344279251.bandicam_2024-01-15_20-38-10-218.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016573ea8e6bf934e1
  • Upwork Job ID: 1746977953315536896
  • Last Price Increase: 2024-02-05
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 15, 2024
@melvin-bot melvin-bot bot changed the title IOU - Error appears after login via IOU URL [$500] IOU - Error appears after login via IOU URL Jan 15, 2024
Copy link

melvin-bot bot commented Jan 15, 2024

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

Copy link

melvin-bot bot commented Jan 15, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 15, 2024
Copy link

melvin-bot bot commented Jan 15, 2024

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

@kbecciv kbecciv changed the title [$500] IOU - Error appears after login via IOU URL [$500] IOU - Error appears after login via IOU URL when using public account Jan 15, 2024
@jliexpensify
Copy link
Contributor

@lanitochka17 I think there's an issue with GH not playing videos properly

@jliexpensify
Copy link
Contributor

@lanitochka17 @hoangzinh I can't repro this on Staging v1.4.25-4. When I use a new Incognito window and use the IOU URL, it takes me to that exact IOU. Can you try repro-ing and share a video? Thanks!

@jliexpensify jliexpensify added the Needs Reproduction Reproducible steps needed label Jan 16, 2024
@jliexpensify jliexpensify changed the title [$500] IOU - Error appears after login via IOU URL when using public account [NEEDS REPRO][$500] IOU - Error appears after login via IOU URL when using public account Jan 16, 2024
@legendAhsan
Copy link

@jliexpensify, I'm able to reproduce this error on both staging url and on my local machine.
You will able to reproduce when you go further down the requests or to single request url. I'm also trying to fix it on my side as well.

Copy link

melvin-bot bot commented Jan 16, 2024

📣 @legendAhsan! 📣
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>

@jliexpensify
Copy link
Contributor

@legendAhsan do you mind sharing a video and version number? Thanks in advance!

@hoangzinh
Copy link
Contributor

I can reproduce in Staging v1.4.25-10

Screen.Recording.2024-01-17.at.07.10.46.mov

@jliexpensify jliexpensify removed the Needs Reproduction Reproducible steps needed label Jan 17, 2024
@jliexpensify jliexpensify changed the title [NEEDS REPRO][$500] IOU - Error appears after login via IOU URL when using public account [$500] IOU - Error appears after login via IOU URL when using public account Jan 17, 2024
@tienifr
Copy link
Contributor

tienifr commented Jan 17, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Hmm it's not here page displayed in conversation history when using public account. User need to refresh page

What is the root cause of that problem?

There're a couple of commands where we should ignore the lastUpdateID as specified here, the code comment clearly explains why.

And we're using that here, so the code below does not run and we don't save the lastUpdateID to Onyx.

However we're not doing the same inside apply here. The lastUpdateID that's supposed to be ignored for the above commands, are still used and saved to Onyx. This leads to the OpenReport command that's executed later to not having response data being applied due to this logic. Because its lastUpdateID is considered to be outdated due to the wrongly saved lastUpdateID of OpenApp, ReconnectApp.

What changes do you think we should make in order to solve the problem?

We can simply ignore the lastUpdateID in this line for requestsToIgnoreLastUpdateID as well by adding:

&& !requestsToIgnoreLastUpdateID.includes(request?.command)

Then the lastUpdateID of the above commands will not be wrongly saved as ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT in Onyx and will not interfere with the OpenReport command.

That should fix the issue. We can consider doing the same for this line and other applicable places.

What alternative solutions did you explore? (Optional)

In this line, add the check that if both the command is OpenReport and clientLastReadTime is null, we will not skip the updates, because in this case the we should use the API updates regardless of the lastUpdateID of the client, as apparent in this issue.

@melvin-bot melvin-bot bot added the Overdue label Jan 19, 2024
Copy link

melvin-bot bot commented Jan 22, 2024

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

@jliexpensify
Copy link
Contributor

Hi @hoangzinh - can you review the proposal above? Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Jan 22, 2024
@hoangzinh
Copy link
Contributor

Sorry for late update. I will review within today

@hoangzinh
Copy link
Contributor

hoangzinh commented Jan 25, 2024

Hi @tienifr, thanks for your proposal. I'm thinking is it a correct fix?

  • I see that all of read API commands don't return lastUpdateID. I.e:
Screenshot 2024-01-25 at 22 43 37
  • But OpenReport API in this case return lastUpdateID
Screenshot 2024-01-25 at 22 46 47
  • It looks like it's related to clientLastReadTime, because in the first load, we haven't had clientLastReadTime yet, then BE returns lastUpdateID in OpenReport API
Screenshot 2024-01-25 at 22 47 51

=> Because OpenReport API in this case returns lastUpdateID, the same as lastUpdateID of OpenApp API, thus it goes to this condition then couldn't update onyxData => Show error page

=> I think this issue could be fixed in BE so that OpenReport API always does not return lastUpdateID

@tienifr
Copy link
Contributor

tienifr commented Jan 26, 2024

I see that all of read API commands don't return lastUpdateID. I.e:

@hoangzinh Yes, but I believe OpenReport is a write command, so it's expected that it could have lastUpdateID.

@hoangzinh
Copy link
Contributor

But why the API OpenReport only returns lastUpdateID when call it with clientLastReadTime is null

@MonilBhavsar
Copy link
Contributor

I've got a draft PR to fix this from server side.
In this case, when user B opens the new report directly through URL, clientLastReadTime argument is passed as null for OpenReport. OpenReport, sends back the lastReadTime as null in an onyx update that I think is unnecessary and can be skipped. I'll do some testing around it and submit it for review.

@MonilBhavsar MonilBhavsar added the Reviewing Has a PR in review label Mar 22, 2024
@MonilBhavsar
Copy link
Contributor

I have submitted PR for review. Thanks all for the help here!

@MonilBhavsar
Copy link
Contributor

Internal PR is deployed to staging. So issue should be fixed on staging when using staging server

@hoangzinh
Copy link
Contributor

Thanks @MonilBhavsar. @jliexpensify should we ask QA to test this issue again to confirm it's fixed?

@MonilBhavsar
Copy link
Contributor

They ran QA when internal PR was deployed. And issue is now fixed on production too. So I think we can close this now?

@s77rt
Copy link
Contributor

s77rt commented Mar 29, 2024

I'm still able to reproduce

Screen.Recording.2024-03-29.at.10.36.09.PM.mov

@arielgreen arielgreen changed the title [$500] IOU - Error appears after login via IOU URL when using public account [LOW] [Splits] [$500] IOU - Error appears after login via IOU URL when using public account Apr 3, 2024
Copy link

melvin-bot bot commented Apr 5, 2024

@hoangzinh, @jliexpensify, @MonilBhavsar Eep! 4 days overdue now. Issues have feelings too...

@jliexpensify
Copy link
Contributor

Bump @MonilBhavsar - looks like the issue wasn't fixed :(

Copy link

melvin-bot bot commented Apr 15, 2024

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

@jliexpensify
Copy link
Contributor

Waiting on @MonilBhavsar for this one!

Copy link

melvin-bot bot commented Apr 23, 2024

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

@jliexpensify
Copy link
Contributor

@MonilBhavsar bumping you here - looks like the issue wasn't fixed.

Copy link

melvin-bot bot commented May 1, 2024

@hoangzinh, @jliexpensify, @MonilBhavsar Whoops! This issue is 2 days overdue. Let's get this updated quick!

@jliexpensify
Copy link
Contributor

I think we're waiting on @MonilBhavsar?

@hoangzinh
Copy link
Contributor

Can we ask QA to test this issue again? @MonilBhavsar said that this issue has passed QA test #34528 (comment)

@jliexpensify jliexpensify added the retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause label May 2, 2024
@jliexpensify
Copy link
Contributor

Sure thing, I dropped the retest label.

@jliexpensify jliexpensify added Weekly KSv2 and removed Daily KSv2 labels May 2, 2024
@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot removed the Weekly KSv2 label May 27, 2024
Copy link

melvin-bot bot commented May 27, 2024

This issue has not been updated in over 15 days. @hoangzinh, @jliexpensify, @MonilBhavsar 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 May 27, 2024
@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Third week)

@jliexpensify
Copy link
Contributor

@MonilBhavsar @hoangzinh I think we can close this right?

@MonilBhavsar
Copy link
Contributor

Yes, we can close this 👍

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 Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests