[HOLD for payment 2024-04-15] [$500] Start chat - App shows data from User A after logging out of User A and logging in with User B

kbecciv commented 6 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.60-8 Reproducible in staging?: y Reproducible in production?: n If this was caught during regression testing, add the test name, ID and link from TestRail: Exploratory around https://expensify.testrail.io/index.php?/tests/view/4476067 Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

Account A and B are under the same private domain, and they share the same list of contacts.
Account A and B have different set of Recent contacts in Start chat list.
1. Close and open incognito mode again for a fresh session.
2. Go to staging.new.expensify.com
3. Log in as Account A.
4. Go to FAB > Start chat > Chat.
5. Note that list of five contacts in Recent section.
6. Log out and log in as Account B.
7. Go to FAB > Start chat > Chat.
8. Note that Account B has the exact same list of five contacts in Recent section as Account A.
9. Close and open incognito mode.
10. Repeat Step 6 and 7 for Account B.

Expected Result:

When user logs out and logs in with another account, the Recent section list will follow the data from the logged-in account.

Actual Result:

When user logs out from Account A and logs in with Account B, the Recent section list still shows the data from Account A.

Workaround:

n/a

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/88148a79-aa20-46bc-80a2-7e4213422ec8

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01e820a2af95c4423e
Upwork Job ID: 1776276873995067392
Last Price Increase: 2024-04-05
Automatic offers:

paultsimura | Reviewer | 0
bernhardoj | Contributor | 0

Issue Owner

Current Issue Owner: @puneetlath

melvin-bot[bot] commented 6 months ago

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

kbecciv commented 6 months ago

We think that this bug might be related to #wave-collect - Release 1

github-actions[bot] commented 6 months ago

:wave: 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:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

mountiny commented 6 months ago

I think this does not have to be a blocker

@kbecciv @kavimuru does the account B not know the recents accounts it shows?

kavimuru commented 6 months ago

@mountiny Logged in with old account with recent contacts, then logged out and sign up with a completely new account. The contact list with old account still shows up for new account.

https://github.com/Expensify/App/assets/43996225/5ca3b525-5472-4161-bf5c-6dc9d07f54fa

mountiny commented 6 months ago

@TMisiukiewicz Could it be that we do not clear the cache for the search options in this case?

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

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

mountiny commented 6 months ago

Referring to this PR https://github.com/Expensify/App/pull/38207

melvin-bot[bot] commented 6 months ago

Upwork job price has been updated to $500

mountiny commented 6 months ago

Increasing to $500 since its a blocker, author of the PR is out for the day and its quite annoying bug as you could potentially see someone else's contacts cached

tienifr commented 6 months ago

Proposal

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

When user logs out from Account A and logs in with Account B, the Recent section list still shows the data from Account A.

What is the root cause of that problem?

We're caching the options in OptionsListContextProvider but we don't have any logic to clear the options upon sign out.

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

Add logic to clear the cached options upon sign out.

What alternative solutions did you explore? (Optional)

NA

bernhardoj commented 6 months ago

Proposal

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

The report data is from another user is still cached even after logging out which makes it possible for other user that log in will see it.

What is the root cause of that problem?

We have OptionsListContextProvider that will load the report list (and update it when it's updated) and cache it.

https://github.com/Expensify/App/blob/b10893a2a541a81112bb2a3d22647a9d7eaac94d/src/components/OptionListContextProvider.tsx#L39-L112

The provider is put in App.tsx which means the state (cache) is persisted even after we log out. https://github.com/Expensify/App/blob/b10893a2a541a81112bb2a3d22647a9d7eaac94d/src/App.tsx#L86

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

Move the provider OptionsListContextProvider from App.tsx to AuthScreens. This way, when the user is logged out, the context provider is destroyed.

What alternative solutions did you explore? (Optional)

Use useSession hook and clear both reports and personalDetails and also set areOptionsInitialized to false whenever the user logged out by checking the session auth token.

useEffect(() => {
    if (!session.authToken) {
        setOptions({reports: [], personalDetails: []});
        areOptionsInitialized.current = false;
    }
}, [session.authToken]);

jasperhuangg commented 6 months ago

@bernhardoj your proposal makes sense to me, let's try to implement it quickly!

melvin-bot[bot] commented 6 months ago

📣 @paultsimura 🎉 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

melvin-bot[bot] commented 6 months ago

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

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

bernhardoj commented 6 months ago

@jasperhuangg can you confirm if we are going with my main solution? (moving the provider)

jasperhuangg commented 6 months ago

Ah yes, sorry. I think moving the component makes the most sense since it's a component associated with authenticated users. Thanks for checking.

bernhardoj commented 6 months ago

Great, thanks for the confirmation. PR is ready.

paultsimura commented 6 months ago

Waiting to review the PR, thanks for handling it so far @jasperhuangg

kavimuru commented 6 months ago

@mountiny Bug is fixed in all the platforms but Desktop. Desktop does not have the correct build version.

https://platform.applause.com/services/links/v1/external/4f3f83c90fa54dbe7ff4691984edde71edefd6da9e238ea3414d49eab3cf4534 https://platform.applause.com/services/links/v1/external/a08ef70d6c97c6d745314b5b2ef9786c0b9fb23b6be3c2c2bb255007e15b446d

https://platform.applause.com/services/links/v1/external/72ebf3a8b4239a38df20870a8e42f0e536209ed2d8b8831a2c2edb630a6fcfcd https://platform.applause.com/services/links/v1/external/0bd5c0575b1498f4e4996700b6114144d631289b290a5ff431c4054cbeb4ef00

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.60-13 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/39753

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

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

@paultsimura requires payment automatic offer (Reviewer)
@bernhardoj requires payment automatic offer (Contributor)

Julesssss commented 6 months ago

FYI this appeared on today's checklist as we forgot to remove the blocker label (I think I forgot to do that too when I marked it off this morning)

melvin-bot[bot] commented 6 months ago

Current assignee @puneetlath is eligible for the Bug assigner, not assigning anyone new.

mountiny commented 6 months ago

Thanks @kbecciv it was repro because the desktop build failed before

@puneetlath this will be ready for payment $500 to @bernhardoj and to @paultsimura

puneetlath commented 6 months ago

Sorry, to clarify, you're saying I should pay them now? Or on April 15th?

melvin-bot[bot] commented 6 months ago

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

paultsimura commented 6 months ago

Sorry, to clarify, you're saying I should pay them now? Or on April 15th?

On April 15th.

Not overdue, Melv.

paultsimura commented 6 months ago

The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/38207
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: https://github.com/Expensify/App/pull/38207/files#r1563935428
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: N/A
Determine if we should create a regression test for this bug: Yes
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.

Regression Test Proposal

Precondition:

Accounts A and B share the same list of contacts
Accounts A and B have different sets of "Recent" contacts in the "Start chat" list.

Test:

Log in as Account A
Go to FAB > Start chat > Chat
Note the list of five contacts in the "Recent" section
Log out and log in as Account B
Go to FAB > Start chat > Chat
Verify the "Recent" lists are different (unless they were indeed interacted with in the same order by both accounts A and B).

Do we agree 👍 or 👎

puneetlath commented 6 months ago

All paid. Thanks everyone!

Expensify / App