[$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?

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

Screenshots/Videos

https://github.com/user-attachments/assets/f98801f1-2905-46ae-b11d-261609611008

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

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

[github-actions[bot]]

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

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

https://github.com/user-attachments/assets/d97f5dcf-f350-46a6-aac6-120b7f259aa9

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

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

[melvin-bot[bot]]

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

[techievivek]

I was able to reproduce this on Firefox but not on Chrome. Also, the QA team has only reported this for Android.

[techievivek]

@situchan Do you have an android device? Can you try reproducing this locally either on Android or Firefox?

[nkdengineer]

Proposal

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

Archived workspace shows green dot in LHN.

What is the root cause of that problem?

In LHN, we use requiresAttentionFromCurrentUser function to check whether the user has require action

https://github.com/Expensify/App/blob/750c34eca1531a48ace4fd5aca24caf3398d9176/src/components/LHNOptionsList/OptionRowLHN.tsx#L85

And in this function, we return early if the report is archived https://github.com/Expensify/App/blob/750c34eca1531a48ace4fd5aca24caf3398d9176/src/libs/ReportUtils.ts#L2400

But isArchivedRoom function only returns true if report. private_isArchived is not empty https://github.com/Expensify/App/blob/750c34eca1531a48ace4fd5aca24caf3398d9176/src/libs/ReportUtils.ts#L1314

For LHN optionOrReport only has isArchived field so requiresAttentionFromCurrentUser function still returns true

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

We should change this function to return true if report.isArchived is true

function isArchivedRoom(report: OnyxInputOrEntry<Report> | OptionData, reportNameValuePairs?: OnyxInputOrEntry<ReportNameValuePairs>): boolean {
    return !!report?.private_isArchived || (report && 'isArchivedRoom' in report && report.isArchivedRoom);
}

https://github.com/Expensify/App/blob/750c34eca1531a48ace4fd5aca24caf3398d9176/src/libs/ReportUtils.ts#L1314

What alternative solutions did you explore? (Optional)

Instead of calling requiresAttentionFromCurrentUser in OptionRowLHN, we can return an extra field in getOptionData function like this

1. Add an extra field here. We also need to update OptionData type

   result = {
   ...
   requiresAttentionFromCurrentUser: false
   }

https://github.com/Expensify/App/blob/750c34eca1531a48ace4fd5aca24caf3398d9176/src/libs/SidebarUtils.ts#L238

2. Update result.requiresAttentionFromCurrentUser

result.requiresAttentionFromCurrentUser = ReportUtils.requiresAttentionFromCurrentUser(report, result.parentReportAction)

https://github.com/Expensify/App/blob/750c34eca1531a48ace4fd5aca24caf3398d9176/src/libs/SidebarUtils.ts#L356

3. Use optionItem.requiresAttentionFromCurrentUser here

const shouldShowGreenDotIndicator = !hasBrickError && optionItem.requiresAttentionFromCurrentUser; 

https://github.com/Expensify/App/blob/750c34eca1531a48ace4fd5aca24caf3398d9176/src/components/LHNOptionsList/OptionRowLHN.tsx#L85

What alternative solutions did you explore? (Optional 2)

Use isArchivedRoom to check here.

if (
    isArchivedRoom(optionOrReport, getReportNameValuePairs(optionOrReport?.reportID)) || ('isArchivedRoom' in optionOrReport && optionOrReport.isArchivedRoom) ||
    isArchivedRoom(getReportOrDraftReport(optionOrReport.parentReportID), getReportNameValuePairs(optionOrReport?.reportID))
) {
    return false;
}

https://github.com/Expensify/App/blob/98d8a5ae4bb52539669f79e007246417eb8c8f1e/src/libs/ReportUtils.ts#L2418-L2419

[srikarparsi]

Hm, I'm not sure if that is the optimal solution. optionItem is of type OptionData which includes Report which has the private_isArchived property. I think we might just not be updating it properly based on changes to Onyx. We do update report here but we're passing in optionItem into requiresAttentionFromCurrentUser.

I think we might just be able to pass in report into requiresAttentionFromCurrentUser because the optionOrReport parameter is of type Report anyway but I haven't tested this.

[nkdengineer]

@srikarparsi We can have another solution: we can return an extra field in getOptionData by calling requiresAttentionFromCurrentUser function using the report data.

https://github.com/Expensify/App/blob/1908bd183b8d1953d81b69ce074c172f30826f90/src/libs/SidebarUtils.ts#L238

[cretadn22]

Proposal

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

Archived workspace shows green dot in LHN

What is the root cause of that problem?

We use private_isArchived to check if a report is archived, but this field is not part of optionItem because SidebarUtils.getOptionData does not return it.

https://github.com/Expensify/App/blob/5e6527613d0f234fcb22507b878697a2bec356c5/src/components/LHNOptionsList/OptionRowLHNData.tsx#L42-L46

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

In SidebarUtils.getOptionData, we need replace this line to

     result.private_isArchived = report.private_isArchived;

Further explanation is needed on why this bug only occurs on Android:

• On Android, requiresAttentionFromCurrentUser returns true because isArchivedRoom condition is false, and hasOutstandingChildRequest is true. The issue with hasOutstandingChildRequest might be due to a problem with Onyx data on Android.
• On the website, requiresAttentionFromCurrentUser returns false because isArchivedRoom condition is false, and no other conditions apply. Consequently, the function defaults to returning false. https://github.com/Expensify/App/blob/5e6527613d0f234fcb22507b878697a2bec356c5/src/libs/ReportUtils.ts#L2437

  What alternative solutions did you explore? (Optional)

[nkdengineer]

Updated proposal

• Add alternative solution based on this comment https://github.com/Expensify/App/issues/45881#issuecomment-2253613862

[srikarparsi]

Hi @situchan, do you think you could take a look at these proposals. @cretadn22's proposal seems like it would work to me but I haven't tested it.

[nkdengineer]

@srikarparsi we used the report data here to check the report is archived or not and we assign it to isArchivedRoom field of optionItem in LHN so I don't think we need to add an extra field private_isArchived. What do you think about my alternative solution?

https://github.com/Expensify/App/blob/98d8a5ae4bb52539669f79e007246417eb8c8f1e/src/libs/SidebarUtils.ts#L310

[nkdengineer]

Additional information, this bug also reproduces on Web

https://github.com/user-attachments/assets/4b976e6d-a7fd-4ba9-a9de-9b7bb3ed32bd

[srikarparsi]

Hm, I'm not really sure about the alternate solution. I don't think that change is needed?

I feel like the most straightforward solution is changing this line to

result.private_isArchived = report.private_isArchived;

But I'll let @situchan weigh in here before proceeding in case there's something I'm missing.

[nkdengineer]

Thanks for your feedback. As I mentioned above, result.isArchivedRoom is already used ReportUtils.isArchivedRoom(report, reportNameValuePairs) to check whether the report is deleted. So if we want to have a solution to simply check the report is archived or not we can use optionOrReport.isArchivedRoom here and no need to update or add any other field. Updated proposal to include this solution.

if (
    isArchivedRoom(optionOrReport, getReportNameValuePairs(optionOrReport?.reportID)) || ('isArchivedRoom' in optionOrReport && optionOrReport.isArchivedRoom) ||
    isArchivedRoom(getReportOrDraftReport(optionOrReport.parentReportID), getReportNameValuePairs(optionOrReport?.reportID))
) {
    return false;
}

https://github.com/Expensify/App/blob/98d8a5ae4bb52539669f79e007246417eb8c8f1e/src/libs/ReportUtils.ts#L2418-L2419

[situchan]

reviewing today

[cretadn22]

@situchan I reviewed it again and concur with @srikarparsi's comment that this line is redundant. We should go ahead and remove it.

[cretadn22]

Update proposal to include @srikarparsi's suggestion

[nkdengineer]

result.isArchivedRoom is also used here and we only use private_isArchived to check the report is archived or not as a temporary solution here https://github.com/Expensify/App/pull/45568 and in the further we will use reportNameValuePairs data to check this. So I believe using optionItem.isArchivedRoom in requiresAttentionFromCurrentUser is safe and simplest solution.

[cretadn22]

result.isArchivedRoom is also used here

However, in here, we can use ReportUtils.isArchivedRoom. I don't believe we need to include isArchivedRoom in the result object.

[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

https://github.com/Expensify/App/blob/750c34eca1531a48ace4fd5aca24caf3398d9176/src/libs/SidebarUtils.ts#L310

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 usingprivate_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[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[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[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[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 https://github.com/Expensify/App/pull/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[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