Skip to content

Remove a call to getReportNameValuePairs when checking if a thread is disabled #63607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
blimpich merged 7 commits into from
Jun 6, 2025
Merged

Remove a call to getReportNameValuePairs when checking if a thread is disabled #63607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
88fb51b
Remove unused method
tgolen Jun 5, 2025
d9cddb5
Fix the tests for deleted report actions
tgolen Jun 5, 2025
1dd70c3
Fix test descriptions
tgolen Jun 5, 2025
050b4a0
Add the proper archived tests
tgolen Jun 5, 2025
6baa0d1
Refactor method with new parameter
tgolen Jun 5, 2025
365d037
Implement new parameter
tgolen Jun 5, 2025
d2b013f
Remove bad test
tgolen Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 2 additions & 72 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8380,71 +8380,6 @@ function canLeaveInvoiceRoom(report: OnyxEntry<Report>): boolean {
return true;
}

/**
* Allows a user to leave a policy room according to the following conditions of the visibility or chatType rNVP:
* `public` - Anyone can leave (because anybody can join)
* `public_announce` - Only non-policy members can leave (it's auto-shared with policy members)
* `policy_admins` - Nobody can leave (it's auto-shared with all policy admins)
* `policy_announce` - Nobody can leave (it's auto-shared with all policy members)
* `policyExpenseChat` - Nobody can leave (it's auto-shared with all policy members)
* `policy` - Anyone can leave (though only policy members can join)
* `domain` - Nobody can leave (it's auto-shared with domain members)
* `dm` - Nobody can leave (it's auto-shared with users)
* `private` - Anybody can leave (though you can only be invited to join)
* `invoice` - Invoice sender, invoice receiver and auto-invited admins cannot leave
*/
function canLeaveRoom(report: OnyxEntry<Report>, isPolicyEmployee: boolean): boolean {

@tgolen tgolen Jun 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function was not being referenced anywhere, so I just removed it.

if (isInvoiceRoom(report)) {
// This will get removed as part of https://github.com/Expensify/App/issues/59961
// eslint-disable-next-line deprecation/deprecation
if (isArchivedNonExpenseReport(report, getReportNameValuePairs(report?.reportID))) {
return false;
}

const invoiceReport = getReportOrDraftReport(report?.iouReportID);

if (invoiceReport?.ownerAccountID === currentUserAccountID) {
return false;
}

if (invoiceReport?.managerID === currentUserAccountID) {
return false;
}

const isSenderPolicyAdmin = getPolicy(report?.policyID)?.role === CONST.POLICY.ROLE.ADMIN;

if (isSenderPolicyAdmin) {
return false;
}

const isReceiverPolicyAdmin =
report?.invoiceReceiver?.type === CONST.REPORT.INVOICE_RECEIVER_TYPE.BUSINESS ? getPolicy(report?.invoiceReceiver?.policyID)?.role === CONST.POLICY.ROLE.ADMIN : false;

if (isReceiverPolicyAdmin) {
return false;
}

return true;
}

if (!report?.visibility) {
if (
report?.chatType === CONST.REPORT.CHAT_TYPE.POLICY_ADMINS ||
report?.chatType === CONST.REPORT.CHAT_TYPE.POLICY_ANNOUNCE ||
report?.chatType === CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT ||
report?.chatType === CONST.REPORT.CHAT_TYPE.DOMAIN_ALL ||
report?.chatType === CONST.REPORT.CHAT_TYPE.SELF_DM ||
!report?.chatType
) {
// DM chats don't have a chatType
return false;
}
} else if (isPublicAnnounceRoom(report) && isPolicyEmployee) {
return false;
}
return true;
}

function isCurrentUserTheOnlyParticipant(participantAccountIDs?: number[]): boolean {
return !!(participantAccountIDs?.length === 1 && participantAccountIDs?.at(0) === currentUserAccountID);
}
Expand Down Expand Up @@ -9154,23 +9089,19 @@ function getNonHeldAndFullAmount(iouReport: OnyxEntry<Report>, shouldExcludeNonR
* - The action is a whisper action and it's neither a report preview nor IOU action
* - The action is the thread's first chat
*/
function shouldDisableThread(reportAction: OnyxInputOrEntry<ReportAction>, reportID: string, isThreadReportParentAction: boolean): boolean {
function shouldDisableThread(reportAction: OnyxInputOrEntry<ReportAction>, reportID: string, isThreadReportParentAction: boolean, isReportArchived = false): boolean {
const isSplitBillAction = isSplitBillReportAction(reportAction);
const isDeletedActionLocal = isDeletedAction(reportAction);
const isReportPreviewActionLocal = isReportPreviewAction(reportAction);
const isIOUAction = isMoneyRequestAction(reportAction);
const isWhisperActionLocal = isWhisperAction(reportAction) || isActionableTrackExpense(reportAction);

// This will get removed as part of https://github.com/Expensify/App/issues/59961
// eslint-disable-next-line deprecation/deprecation
const isArchived = isArchivedNonExpenseReport(getReportOrDraftReport(reportID), getReportNameValuePairs(reportID));
const isActionDisabled = CONST.REPORT.ACTIONS.THREAD_DISABLED.some((action: string) => action === reportAction?.actionName);

return (
isActionDisabled ||
isSplitBillAction ||
(isDeletedActionLocal && !reportAction?.childVisibleActionCount) ||
(isArchived && !reportAction?.childVisibleActionCount) ||
(isReportArchived && !reportAction?.childVisibleActionCount) ||
(isWhisperActionLocal && !isReportPreviewActionLocal && !isIOUAction) ||
isThreadReportParentAction
);
Expand Down Expand Up @@ -10816,7 +10747,6 @@ export {
canEditWriteCapability,
canFlagReportAction,
isNonAdminOrOwnerOfPolicyExpenseChat,
canLeaveRoom,
canJoinChat,
canLeaveChat,
canReportBeMentionedWithinPolicy,
Expand Down
4 changes: 2 additions & 2 deletions src/pages/home/report/ContextMenu/ContextMenuActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,11 +269,11 @@ const ContextMenuActions: ContextMenuAction[] = [
isAnonymousAction: false,
textTranslateKey: 'reportActionContextMenu.replyInThread',
icon: Expensicons.ChatBubbleReply,
shouldShow: ({type, reportAction, reportID, isThreadReportParentAction}) => {
shouldShow: ({type, reportAction, reportID, isThreadReportParentAction, isArchivedRoom}) => {
if (type !== CONST.CONTEXT_MENU_TYPES.REPORT_ACTION || !reportID) {
return false;
}
return !shouldDisableThread(reportAction, reportID, isThreadReportParentAction);
return !shouldDisableThread(reportAction, reportID, isThreadReportParentAction, isArchivedRoom);
},
onPress: (closePopover, {reportAction, reportID}) => {
const originalReportID = getOriginalReportID(reportID, reportAction);
Expand Down
216 changes: 180 additions & 36 deletions tests/unit/ReportUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1234,42 +1234,6 @@ describe('ReportUtils', () => {
expect(shouldDisableThread(reportAction, reportID, false)).toBeTruthy();
});

it('should disable on deleted and not-thread actions', () => {

@tgolen tgolen Jun 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored this test to be more explicit, like the new archived tests

const reportAction = {
message: [
{
translationKey: '',
type: 'COMMENT',
html: '',
text: '',
isEdited: true,
},
],
childVisibleActionCount: 1,
} as ReportAction;
expect(shouldDisableThread(reportAction, reportID, false)).toBeFalsy();

reportAction.childVisibleActionCount = 0;
expect(shouldDisableThread(reportAction, reportID, false)).toBeTruthy();
});

it('should disable on archived reports and not-thread actions', () => {

@tgolen tgolen Jun 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bad test because it wasn't actually testing archived reports. Archived reports would have required doing something with the report NVPs, not the status or state.

Then, the test is really only testing the childVisibleActionCount, which isn't great.

However, since the report wasn't actually archived, the logic falls into the same path as deleted threads since reportAction is missing the message property.

Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
statusNum: CONST.REPORT.STATUS_NUM.CLOSED,
stateNum: CONST.REPORT.STATE_NUM.APPROVED,
})
.then(() => waitForBatchedUpdates())
.then(() => {
const reportAction = {
childVisibleActionCount: 1,
} as ReportAction;
expect(shouldDisableThread(reportAction, reportID, false)).toBeFalsy();

reportAction.childVisibleActionCount = 0;
expect(shouldDisableThread(reportAction, reportID, false)).toBeTruthy();
});
});

it("should disable on a whisper action and it's neither a report preview nor IOU action", () => {
const reportAction = {
actionName: CONST.REPORT.ACTIONS.TYPE.MODIFIED_EXPENSE,
Expand All @@ -1286,6 +1250,186 @@ describe('ReportUtils', () => {
} as ReportAction;
expect(shouldDisableThread(reportAction, reportID, true)).toBeTruthy();
});

describe('deleted threads', () => {
it('should be enabled if the report action is not-deleted and child visible action count is 1', () => {
// Given a normal report action with one child visible action count
const reportAction = {
message: [
{
translationKey: '',
type: 'COMMENT',
html: 'test',
text: 'test',
},
],
childVisibleActionCount: 1,
} as ReportAction;

// When it's checked to see if the thread should be disabled
const isThreadDisabled = shouldDisableThread(reportAction, reportID, false);

// Then the thread should be enabled
expect(isThreadDisabled).toBeFalsy();
});

it('should be enabled if the report action is not-deleted and child visible action count is 0', () => {
// Given a normal report action with zero child visible action count
const reportAction = {
message: [
{
translationKey: '',
type: 'COMMENT',
html: 'test',
text: 'test',
},
],
childVisibleActionCount: 0,
} as ReportAction;

// When it's checked to see if the thread should be disabled
const isThreadDisabled = shouldDisableThread(reportAction, reportID, false);

// Then the thread should be enabled
expect(isThreadDisabled).toBeFalsy();
});
it('should be enabled if the report action is deleted and child visible action count is 1', () => {
// Given a normal report action with one child visible action count
const reportAction = {
message: [
{
translationKey: '',
type: 'COMMENT',
html: '',
text: '',
},
],
childVisibleActionCount: 1,
} as ReportAction;

// When it's checked to see if the thread should be disabled
const isThreadDisabled = shouldDisableThread(reportAction, reportID, false);

// Then the thread should be enabled
expect(isThreadDisabled).toBeFalsy();
});

it('should be disabled if the report action is deleted and child visible action count is 0', () => {
// Given a normal report action with zero child visible action count
const reportAction = {
message: [
{
translationKey: '',
type: 'COMMENT',
html: '',
text: '',
},
],
childVisibleActionCount: 0,
} as ReportAction;

// When it's checked to see if the thread should be disabled
const isThreadDisabled = shouldDisableThread(reportAction, reportID, false);

// Then the thread should be disabled
expect(isThreadDisabled).toBeTruthy();
});
});

describe('archived report threads', () => {
it('should be enabled if the report is not-archived and child visible action count is 1', () => {
// Given a normal report action with one child visible action count
const reportAction = {
message: [
{
translationKey: '',
type: 'COMMENT',
html: 'test',
text: 'test',
},
],
childVisibleActionCount: 1,
} as ReportAction;

// And a report that is not archived
const isReportArchived = false;

// When it's checked to see if the thread should be disabled
const isThreadDisabled = shouldDisableThread(reportAction, reportID, false, isReportArchived);

// Then the thread should be enabled
expect(isThreadDisabled).toBeFalsy();
});
it('should be enabled if the report is not-archived and child visible action count is 0', () => {
// Given a normal report action with zero child visible action counts
const reportAction = {
message: [
{
translationKey: '',
type: 'COMMENT',
html: 'test',
text: 'test',
},
],
childVisibleActionCount: 1,
} as ReportAction;

// And a report that is not archived
const isReportArchived = false;

// When it's checked to see if the thread should be disabled
const isThreadDisabled = shouldDisableThread(reportAction, reportID, false, isReportArchived);

// Then the thread should be enabled
expect(isThreadDisabled).toBeFalsy();
});
it('should be enabled if the report is archived and child visible action count is 1', () => {
// Given a normal report action with one child visible action count
const reportAction = {
message: [
{
translationKey: '',
type: 'COMMENT',
html: 'test',
text: 'test',
},
],
childVisibleActionCount: 1,
} as ReportAction;

// And a report that is not archived
const isReportArchived = true;

// When it's checked to see if the thread should be disabled
const isThreadDisabled = shouldDisableThread(reportAction, reportID, false, isReportArchived);

// Then the thread should be enabled
expect(isThreadDisabled).toBeFalsy();
});
it('should be disabled if the report is archived and child visible action count is 0', () => {
// Given a normal report action with zero child visible action counts
const reportAction = {
message: [
{
translationKey: '',
type: 'COMMENT',
html: 'test',
text: 'test',
},
],
childVisibleActionCount: 0,
} as ReportAction;

// And a report that is not archived
const isReportArchived = true;

// When it's checked to see if the thread should be disabled
const isThreadDisabled = shouldDisableThread(reportAction, reportID, false, isReportArchived);

// Then the thread should be disabled
expect(isThreadDisabled).toBeTruthy();
});
});
});

describe('getAllAncestorReportActions', () => {
Expand Down