[$500] Public room - Focus mode pop-up opens if navigate to public room as anonymous user

[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.27-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:

Precondition: user is logged in with account that has many reports, not a new account

1. Go to https://staging.new.expensify.com/
2. Go to incognito mode (close and reopen if it is open)
3. Log in
4. Create a public room
5. Send a reply in the room and also reply in a thread
6. Copy public room link
7. Log out
8. Navigate via copied public room link

Expected Result:

Focus mode pop-up not opens if navigate to public room as anonymous user

Actual Result:

Focus mode pop-up opens if navigate to public room as anonymous user

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

Bug6346626_1705589665312.focus_mode_pop-up_opens.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01150febaacdca6d76
• Upwork Job ID: 1748014073803857920
• Last Price Increase: 2024-02-08

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[lanitochka17]

#vip-vsb

[abzokhattab]

Proposal

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

Focus mode pop-up opens if navigate to public room as anonymous user

What is the root cause of that problem?

Here we check if the currentAccountID is not null and subsequently sets the focus state to true in Onyx. However, when an anonymous user enters a public room, the backend provides a non-empty accountID, leading to the focus being incorrectly set to true.

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

as we can see the backend returns the account object and it includes the authTokenType as anonymousAccount. Therefore, we should adjust the aforementioned code to avoid setting the focus to true when val.authTokenType equals anonymousAccount.

so we need to update oynx.connect to save if the user is anonymous or not :

let currentUserAccountID: number | undefined | null;
let isAnonymousUser;
Onyx.connect({
    key: ONYXKEYS.SESSION,
    callback: (val) => {
        isAnonymousUser = val.authTokenType === 'anonymousAccount';
        currentUserAccountID = val?.accountID;
    },
});

and then this line to :

        if (!currentUserAccountID && !isAnonymousUser) {

[tienifr]

Proposal

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

Focus mode pop-up opens if navigate to public room as anonymous user

What is the root cause of that problem?

In here, when the policy is removed from Onyx, it will merge {hasDraft: false} to its related reports.

The problem is when we clear Onyx data on Log out, it also set policy to null/undefined, triggering that logic. So after Logout and all the report data is cleared, all the policy-related reports will appear again in Onyx with value {hasDraft: false}. This leads to the report count here being wrong when we navigate to the public room, it will be the total count of policy-related reports of the old account before logged out. So it will show the focus popup because other conditions are also satisfied.

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

Do not do the data merge here if the user is in the process of clearing the data when logging out.

if the user is in the process of clearing the data when logging out

There're multiple ways to check this, one of it is to set a variable isClearingDataOnLogout similar to this. Before we do Onyx.clear, we'll set it to true, after the clearing is done, set it to false.

If it's true then do not do the data merge here

What alternative solutions did you explore? (Optional)

There could be other ways to check that the user is signing out, like relying on Onyx state, ... but the core approach is the same.

Not related to this issue but the reportCount currently doesn't reflect 100% accurate the number of reports that the user will have in LHN (for which we need the focus mode), if we want to align them that means we need to additionally check conditions like notificationPreference must not be HIDDEN, reportID should exist, amongst others (can refer to how we retrieve the list of reports for LHN).

[shahinyan11]

Proposal

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

Public room - Focus mode pop-up opens if navigate to public room as anonymous user

What is the root cause of that problem?

The above proposal describes the root cause correct

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

I suggest to calculate the reportCount another way. We can count those that have the reportId key (or another key).
To do that we can change this line with below code

const reportCount = Object.values(allReports ?? {}).filter((report)=> Object.hasOwn(report, 'reportId')).length;

What alternative solutions did you explore? (Optional)

[tienifr]

Proposal updated to mention an observed unrelated issue.

[slafortune]

@eVoloshchak is there a proposal we can move forward with here?

[eVoloshchak]

I think we should proceed with @tienifr's proposal

While @abzokhattab's proposal is simpler and does prevent the issue from happening, it doesn't resolve the root cause (incorrect report count)

🎀👀🎀 C+ reviewed!

[melvin-bot]

Triggered auto assignment to @youssef-lr, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[shahinyan11]

@eVoloshchak I would also like to see feedback on my proposal

[tienifr]

@eVoloshchak I would also like to see feedback on my #34740 (comment)

@shahinyan11 I think it also doesn't fix the root cause because there'll still be an orphaned list of reports with just {hasDraft: false} data in Onyx.

[melvin-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]

@eVoloshchak, @youssef-lr, @slafortune Huh... This is 4 days overdue. Who can take care of this?

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[eVoloshchak]

Same, unable to reproduce the issue anymore
Let's wait for the second week KI tests then

[melvin-bot]

@eVoloshchak @youssef-lr @slafortune 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!

[slafortune]

One more test this week

[tienifr]

@eVoloshchak @slafortune Looks like it's this PR that fixed it temporarily using a "short term fix" (as clarified in the PR description)

Short term fix: Apply some sane filtering to to the reports since the only thing we are checking is if there is a report key in Onyx

While the accepted proposal here is the "Long term fix" that addresses the root cause

Long term fix: Figure out what is preventing Onyx data from clearing and fix it

I think we should move forward here to fix the root cause since it's still reproducible, it could also be regarded as a security issue since Onyx is retaining the reports data even after the user logged out.

cc @marcaaron since you worked on the "Short term fix" PR

[melvin-bot]

@eVoloshchak @youssef-lr @slafortune this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[melvin-bot]

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

[marcaaron]

WRT to the issue. I am not too convinced we need to optimize this for anonymous users they are likely not going to have large numbers reports before becoming a real user.

Onyx is retaining the reports data even after the user logged out

I agree with @tienifr that this continues to be a strange problem to diagnose and has been around for a while. If we can get a reproduction then it would be good to fix. I tried to raise an issue about this but it was closed so I'm not sure it's really a near term priority for us.

security issue since Onyx is retaining the reports data even after the user logged out

Thats an interesting take. I agree it's a problem in theory as someone could use the app on a public computer or network and that behavior would be very concerning. However, we really need a reproduction. I think it requires a lot of data to exist in Onyx for this to happen. And haven't been able to reliably reproduce it.

[slafortune]

Seems like we should be retesting this a time or two before closing

[melvin-bot]

@eVoloshchak, @youssef-lr, @slafortune Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[slafortune]

Will be tested again this week

[melvin-bot]

@eVoloshchak @youssef-lr @slafortune this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot]

Current assignee @eVoloshchak is eligible for the Internal assigner, not assigning anyone new.

[slafortune]

Since we haven't been able to reliably reproduce this, and there are some fixes in place currently, AND this is related to an anonymous user - seems very safe to close this.