[Due for payment][$250] Remove call to `getReportNameValuePairs()` in `canEditRoomVisibility()`

[tgolen]

Coming from #59961

Problem

canEditRoomVisibility() is not a pure function because it references the report's NVPs from a global variable here. This is an anti-pattern that we are actively trying to unwind.

Solution

Using test driven development:

1. Make sure a unit test exists for canEditRoomVisibility() in its current form (or create one if it doesn't exist)
2. Refactor canEditRoomVisibility() to be a pure function, adding isReportArchived = false as a new parameter
3. Implement the new parameter in all code that calls canEditRoomVisibility()
4. Ensure the unit tests still pass

Important

Please be sure that your proposal follows these best practices:

• Don't add any more references to data obtained via Onyx.connect
• Don't add any more references to isArchivedReport(nameValuePairs) unless absolutely necessary
• Focus on connecting to the data in a UI component and passing the data all the way down with params
• The React hook useReportIsArchived() is very useful for these
• Look for existing references in the functions for the report's NVPs or see if isReportArchived has already been passed or calculated

Issue Owner

Current Issue Owner: @

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021945234426116211782
• Upwork Job ID: 1945234426116211782
• Last Price Increase: 2025-07-15

Issue Owner

Current Issue Owner: @

[melvin-bot]

Triggered auto assignment to @johncschuster (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.

[samranahm]

🚨 Edited by proposal-police: This proposal was edited at 2025-06-11 20:33:15 UTC.

Proposal

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

Remove call to getReportNameValuePairs() in canEditRoomVisibility()

What is the root cause of that problem?

Refactor util function

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

we want to remove isArchivedNonExpenseReport(report, getReportNameValuePairs(report?.reportID)) here and use isArchivedReport coming directly from parent component where we check this value using useReportIsArchived hook

App/src/libs/ReportUtils.ts

Lines 8679 to 8683 in 36a723f

	function canEditRoomVisibility(report: OnyxEntry<Report>, policy: OnyxEntry<Policy>): boolean {
	// This will get removed as part of https://github.com/Expensify/App/issues/59961
	// eslint-disable-next-line deprecation/deprecation
	return isPolicyAdminPolicyUtils(policy) && !isArchivedNonExpenseReport(report, getReportNameValuePairs(report?.reportID));
	}

here and add new pram
isReportArchived = false and then we can pass this argument from ReportSettingsPage

    const isArchivedReport = useReportIsArchived(reportID);

in ReportSettingsPage component

App/src/pages/settings/Report/ReportSettingsPage.tsx

Line 46 in 36a723f

const shouldAllowChangeVisibility = useMemo(() => ReportUtils.canEditRoomVisibility(report, linkedWorkspace), [report, linkedWorkspace]);

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Add new test canEditRoomVisibility in it's corresponding location that should cover this new change as well

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[Tony-MK]

Proposal

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

Remove call to getReportNameValuePairs() in canEditRoomVisibility()

What is the root cause of that problem?

canEditRoomVisibility is not a pure function.

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

In canEditRoomVisibility, replace !isArchivedNonExpenseReport(report, getReportNameValuePairs(report?.reportID)) with isReportArchived and default it to false.

In ReportSettingsPage, use the useReportIsArchived hook to get isReportArchived to pass it to canEditRoomVisibility.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Create a unit test for canEditRoomVisibility in ReportUtilsTest since it does not exist.

 describe('canEditRoomVisibility', () => {
        it('should return true for expense chat', () => {
            const workspaceChat: Report = {
                ...createRandomReport(1),
                chatType: CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT,
            };
            expect(canEditRoomVisibility(workspaceChat, {...policy, role: CONST.POLICY.ROLE.ADMIN})).toBe(true);
        });

        it('should return false for archived expense chat', () => {
            const workspaceChat: Report = {
                ...createRandomReport(1),
                chatType: CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT,
            };
            expect(canEditRoomVisibility(workspaceChat, {...policy, role: CONST.POLICY.ROLE.ADMIN}, true)).toBe(false);
        });
        // Also, other cases for non admins.
    });

What alternative solutions did you explore? (Optional)

N/A

[DylanDylann]

@tgolen Could I help on this stuff as C+?

[tgolen]

Sure!