[$250] LHN-Archived workspace shows green dot in LHN.

[izarutskaya]

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.10
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4751910
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Launch app
2. Tap fab - create a new workspace
3. Tap on workspace chat
4. Submit a expense using plus icon near compose
5. Navite to LHN and note green dot next to workspace chat
6. Tap profile icon -- workspaces -- workspace
7. Tap 3 dots next to workspace and delete the workspace
8. Navigate to LHN

Expected Result:

Archived workspace must not show green dot in LHN.

Actual Result:

Archived workspace shows green dot in LHN.

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

Bug6548982_1721612000550.dot.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017beae0c789f3f2ea
• Upwork Job ID: 1816740887268923324
• Last Price Increase: 2024-08-02
• Automatic offers:
• situchan | Reviewer | 103412587
• cretadn22 | Contributor | 103412589

Issue Owner

Current Issue Owner: @situchan

[melvin-bot]

Triggered auto assignment to @abekkala (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]

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

[github-actions]

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

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

[izarutskaya]

We think this issue might be related to the #vip-vsb

[izarutskaya]

Production

video_2024-07-22_10-09-35.mp4

[mountiny]

Discussing here

This is a consequence of the changes we made to archiving reports; when the workspace chat is archived, the expense reports are not closed but only marked as archived. So I think we need to find a solution for this, but it does not have to be a blocker imho

[abekkala]

@mountiny is this an external fix?

[mountiny]

Still not clear, asked what is the conclusion here https://expensify.slack.com/archives/C01GTK53T8Q/p1721948318212529?thread_ts=1721641499.655229&cid=C01GTK53T8Q

[srikarparsi]

I just checked the code and we do currently check if the report is archived before showing a green dot indicator on the LHN. And we use the isArchived rNVP to do that so I'm not too sure what's going wrong.

Also, I followed the reproduction steps and I'm not able to reproduce this on mac chrome:

[techievivek]

I am still able to reproduce this. Sending it to External to hide the GBR when the workspace is deleted and workspace chat is archived.

[melvin-bot]

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

[cretadn22]

Because requiresAttentionFromCurrentUser is a util function, we should limit adding logic to this function. My solution is safest and easiest way to address this bug

[techievivek]

@srikarparsi I will let you handle this GH since you seem to have context over it. Thanks.

[nkdengineer]

Add more explanations about my proposal:

1. If we get isArchivedRoom here we only need to call isArchivedRoom function one time and we can reuse this as we do here.

And requiresAttentionFromCurrentUser also accepts optionOrReport with type OptionData. So we can use isArchivedRoom here if this field exists in optionOrReport otherwise use isArchivedRoom function. With this, we can also reduce the unnecessary calling isArchivedRoom and getReportNameValuePairs function here

App/src/libs/SidebarUtils.ts

Line 310 in 750c34e

result.isArchivedRoom = ReportUtils.isArchivedRoom(report, reportNameValuePairs);

2. If we only want to accept Report type in requiresAttentionFromCurrentUser function, my alternative solution already suggested this

[situchan]

I don't like passing optionItem to ReportUtils.isArchivedRoom(report: OnyxInputOrEntry<Report>) in current codebase. It's against TS.

result.private_isArchived = report.private_isArchived; - this is risky (might cause unexpected regression) since result.isArchived already used in many places. And all keys in OptionData are camelCase, while private_isArchived is snake.

using optionItem.isArchivedRoom in requiresAttentionFromCurrentUser is safe and simplest solution

I am 👍 on ^ (@nkdengineer's alternative solution 2)

[cretadn22]

@situchan @srikarparsi I have some important points to consider; please keep them in mind while making your decision. Thank you very much.

1. In this situation, we can only determine whether a report is archived by using private_isArchived. Note that isArchivedRoom function and requiresAttentionFromCurrentUser function also relies on private_isArchived internally. The issue arises because private_isArchived is null/undefined due to its absence in optionItem.

2. Regarding this point

And all keys in OptionData are camelCase, while private_isArchived is snake.

Since private_isArchived is used in our app to determine isArchived, if the naming is an issue, we should consider renaming this field to camelCase rather than avoiding its use out of concern for potential regressions.

3. Regarding the chosen solution, the requiresAttentionFromCurrentUser function also uses private_isArchived internally to determine whether a report is archived. If that's the case, why do we need to include requiresAttentionFromCurrentUser in optionItem when we only need to include private_isArchived?

4. In the future, if we use reportNameValuePairs instead of private_isArchived, this issue will be resolved, as the bug occurred due to the absence of private_isArchived.

[srikarparsi]

I will try to get to this tomorrow, in the meanwhile @situchan can you please take a look at @cretadn22's comment?

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

@abekkala @srikarparsi @situchan 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!

[situchan]

We're close

[srikarparsi]

Reviewed both and I'm still in favor of @cretadn22's proposal. The OptionData type currently has two properties that both refer to the same data: isArchivedRoom and private_isArchived (from the Report type). We should only have one property for a single piece of data and should have removed isArchivedRoom from OptionData when we introduced private_isArchived to Report because OptionData "extends" Report.

As for the risky and might cause regressions point, we should remove isArchivedRoom from OptionData so that type checks can help and can also search for something on the lines of "isArchivedRoom = ".

[melvin-bot]

📣 @situchan 🎉 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]

📣 @cretadn22 🎉 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 📖

[nkdengineer]

@srikarparsi Thanks for your feedback. I still have a concern here. report.private_isArchived is only a temporary here #45568 while isArchivedRoom always correct even we will be removed in the further since it's used isArchivedRoom function. Using it can also reduce the number of calls to the isArchivedRoom function and getReportNameValuePairs

Finally, I'm happy with your decision.

[srikarparsi]

Yeah I see what you're saying but I don't think we should ever have two properties that point to the same value (private_isArchived and isArchivedRoom in OptionReport). I agree that switching over to private_isArchived will require more changes once we switch private_isArchived to be a part of the reportNameValuePairs collection but I think it's a better approach as it avoids redundant variables.

[nkdengineer]

Thanks.

[melvin-bot]

This issue has not been updated in over 15 days. @abekkala, @srikarparsi, @situchan, @cretadn22 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!

[cretadn22]

@abekkala It is time to process payment. We have deployed the PR to production since Sep 20