From 61f7b5fc1d512fd49fc83f425c7954e9f00c7e10 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 30 Jun 2025 11:59:02 -0500 Subject: [PATCH 01/22] Refactor and implement the changes --- src/libs/ReportUtils.ts | 6 ++---- src/pages/FlagCommentPage.tsx | 4 +++- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index 37f5806d0e55..b9bc1948eb6b 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -8444,12 +8444,10 @@ function canFlagReportAction(reportAction: OnyxInputOrEntry, repor /** * Whether flag comment page should show */ -function shouldShowFlagComment(reportAction: OnyxInputOrEntry, report: OnyxInputOrEntry): boolean { +function shouldShowFlagComment(reportAction: OnyxInputOrEntry, report: OnyxInputOrEntry, isReportArchived = false): boolean { return ( canFlagReportAction(reportAction, report?.reportID) && - // This will get removed as part of https://github.com/Expensify/App/issues/59961 - // eslint-disable-next-line deprecation/deprecation - !isArchivedNonExpenseReport(report, !!getReportNameValuePairs(report?.reportID)?.private_isArchived) && + !isArchivedNonExpenseReport(report, isReportArchived) && !chatIncludesChronos(report) && !isConciergeChatReport(report) && reportAction?.actorAccountID !== CONST.ACCOUNT_ID.CONCIERGE diff --git a/src/pages/FlagCommentPage.tsx b/src/pages/FlagCommentPage.tsx index 8db504520529..c31f2d65cc6f 100644 --- a/src/pages/FlagCommentPage.tsx +++ b/src/pages/FlagCommentPage.tsx @@ -10,6 +10,7 @@ import ScreenWrapper from '@components/ScreenWrapper'; import ScrollView from '@components/ScrollView'; import Text from '@components/Text'; import useLocalize from '@hooks/useLocalize'; +import useReportIsArchived from '@hooks/useReportIsArchived'; import useThemeStyles from '@hooks/useThemeStyles'; import Navigation from '@libs/Navigation/Navigation'; import type {PlatformStackScreenProps} from '@libs/Navigation/PlatformStackNavigation/types'; @@ -49,6 +50,7 @@ function getReportID(route: FlagCommentPageNavigationProps['route']) { function FlagCommentPage({parentReportAction, route, report, parentReport, reportAction}: FlagCommentPageProps) { const styles = useThemeStyles(); const {translate} = useLocalize(); + const isReportArchived = useReportIsArchived(report?.reportID); const severities: SeverityItemList = [ { @@ -135,7 +137,7 @@ function FlagCommentPage({parentReportAction, route, report, parentReport, repor testID={FlagCommentPage.displayName} > {({safeAreaPaddingBottomStyle}) => ( - + Navigation.goBack(route.params.backTo)} From 7fdde61e02480f19f8a89404837ffd19275c3f7f Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 30 Jun 2025 12:23:16 -0500 Subject: [PATCH 02/22] Write tests --- tests/unit/ReportUtilsTest.ts | 115 +++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 1 deletion(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index bd9940a82f88..86c62003539a 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -58,6 +58,7 @@ import { shouldDisableRename, shouldDisableThread, shouldReportBeInOptionList, + shouldShowFlagComment, temporary_getMoneyRequestOptions, } from '@libs/ReportUtils'; import type {OptionData} from '@libs/ReportUtils'; @@ -2241,7 +2242,7 @@ describe('ReportUtils', () => { ).toBeFalsy(); }); - it('should return false when the user’s email is domain-based and the includeDomainEmail is false', () => { + it('should return false when the users email is domain-based and the includeDomainEmail is false', () => { const report = LHNTestUtils.getFakeReport(); const currentReportId = ''; const isInFocusMode = false; @@ -3605,4 +3606,116 @@ describe('ReportUtils', () => { expect(result).toBe(false); }); }); + + describe('shouldShowFlagComment', () => { + let validReportAction: ReportAction = { + ...createRandomReportAction(1), + actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, + + // Actor is not the current user or Concierge + actorAccountID: 123456, + }; + + describe('chat reports', () => { + let chatReport: Report; + + beforeAll(async () => { + chatReport = { + ...createRandomReport(60000), + type: CONST.REPORT.TYPE.CHAT, + }; + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, chatReport); + }); + + afterAll(async () => { + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); + }); + + it('should return true for an archived report', () => { + expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(true); + }); + + it('should return false for a non-archived report', () => { + expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); + }); + }); + + describe('expense reports', () => { + let expenseReport: Report; + + beforeAll(async () => { + expenseReport = { + ...createRandomReport(60000), + type: CONST.REPORT.TYPE.EXPENSE, + }; + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${expenseReport.reportID}`, expenseReport); + }); + + afterAll(async () => { + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${expenseReport.reportID}`, null); + }); + + it('should return true for an archived report', () => { + expect(shouldShowFlagComment(validReportAction, expenseReport, false)).toBe(true); + }); + + it('should return false for a non-archived report', () => { + expect(shouldShowFlagComment(validReportAction, expenseReport, false)).toBe(false); + }); + }); + + describe('Action from Concierge', () => { + let chatReport: Report; + let actionFromConcierge: ReportAction = { + ...createRandomReportAction(1), + actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, + actorAccountID: CONST.ACCOUNT_ID.CONCIERGE, + }; + + beforeAll(async () => { + chatReport = { + ...createRandomReport(60000), + type: CONST.REPORT.TYPE.CHAT, + }; + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, chatReport); + }); + + afterAll(async () => { + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); + }); + + it('should return true for an archived report', () => { + expect(shouldShowFlagComment(actionFromConcierge, chatReport, false)).toBe(true); + }); + + it('should return false for a non-archived report', () => { + expect(shouldShowFlagComment(actionFromConcierge, chatReport, false)).toBe(false); + }); + }); + + describe('Action from Concierge', () => { + let chatReport: Report; + + beforeAll(async () => { + chatReport = { + ...createRandomReport(60000), + type: CONST.REPORT.TYPE.CHAT, + participants: buildParticipantsFromAccountIDs([currentUserAccountID, CONST.ACCOUNT_ID.CONCIERGE]), + }; + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, chatReport); + }); + + afterAll(async () => { + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); + }); + + it('should return true for an archived report', () => { + expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); + }); + + it('should return false for a non-archived report', () => { + expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); + }); + }); + }); }); From b21b8cff548ba52c341119b70c35b9f473ef1307 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 30 Jun 2025 12:25:34 -0500 Subject: [PATCH 03/22] Get tests passing --- tests/unit/ReportUtilsTest.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 86c62003539a..3f6279dc0724 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -3608,7 +3608,7 @@ describe('ReportUtils', () => { }); describe('shouldShowFlagComment', () => { - let validReportAction: ReportAction = { + const validReportAction: ReportAction = { ...createRandomReportAction(1), actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, @@ -3635,8 +3635,8 @@ describe('ReportUtils', () => { expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(true); }); - it('should return false for a non-archived report', () => { - expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); + it('should return true for a non-archived report', () => { + expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(true); }); }); @@ -3659,14 +3659,14 @@ describe('ReportUtils', () => { expect(shouldShowFlagComment(validReportAction, expenseReport, false)).toBe(true); }); - it('should return false for a non-archived report', () => { - expect(shouldShowFlagComment(validReportAction, expenseReport, false)).toBe(false); + it('should return true for a non-archived report', () => { + expect(shouldShowFlagComment(validReportAction, expenseReport, false)).toBe(true); }); }); describe('Action from Concierge', () => { let chatReport: Report; - let actionFromConcierge: ReportAction = { + const actionFromConcierge: ReportAction = { ...createRandomReportAction(1), actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, actorAccountID: CONST.ACCOUNT_ID.CONCIERGE, @@ -3684,8 +3684,8 @@ describe('ReportUtils', () => { await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); }); - it('should return true for an archived report', () => { - expect(shouldShowFlagComment(actionFromConcierge, chatReport, false)).toBe(true); + it('should return false for an archived report', () => { + expect(shouldShowFlagComment(actionFromConcierge, chatReport, false)).toBe(false); }); it('should return false for a non-archived report', () => { @@ -3709,7 +3709,7 @@ describe('ReportUtils', () => { await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); }); - it('should return true for an archived report', () => { + it('should return false for an archived report', () => { expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); }); From f576ec15ea346b3acf0f972ce03b7fe1f2173fcf Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 30 Jun 2025 14:15:01 -0500 Subject: [PATCH 04/22] Add more tests and comments --- src/libs/ReportUtils.ts | 8 +++++ tests/unit/ReportUtilsTest.ts | 59 +++++++++++++++++++++++++++++++---- 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index b9bc1948eb6b..8e953452ce8e 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -8445,6 +8445,14 @@ function canFlagReportAction(reportAction: OnyxInputOrEntry, repor * Whether flag comment page should show */ function shouldShowFlagComment(reportAction: OnyxInputOrEntry, report: OnyxInputOrEntry, isReportArchived = false): boolean { + console.log(` + canFlagReportAction: ${canFlagReportAction(reportAction, report?.reportID)} + isArchivedNonExpenseReport: ${isArchivedNonExpenseReport(report, isReportArchived)} + chatIncludesChronos: ${chatIncludesChronos(report)} + isConciergeChatReport: ${isConciergeChatReport(report)} + reportAction?.actorAccountID: ${reportAction?.actorAccountID} + Concierge: ${CONST.ACCOUNT_ID.CONCIERGE} + `); return ( canFlagReportAction(reportAction, report?.reportID) && !isArchivedNonExpenseReport(report, isReportArchived) && diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 3f6279dc0724..bd0fee569b09 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -3631,11 +3631,15 @@ describe('ReportUtils', () => { await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); }); - it('should return true for an archived report', () => { - expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(true); + it('should return false for an archived chat report', () => { + // It will be false here because it's a chat report and therefore, it depends on if the report is archived or not + // so isArchivedNonExpenseReport(report, true) will be true, and shouldShowFlagComment checks for false. + expect(shouldShowFlagComment(validReportAction, chatReport, true)).toBe(false); }); - it('should return true for a non-archived report', () => { + it('should return true for a non-archived chat report', () => { + // It will be true here because it's a chat report and therefore, it depends on if the report is archived or not + // so isArchivedNonExpenseReport(report, false) will be false, and shouldShowFlagComment checks for false. expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(true); }); }); @@ -3656,10 +3660,14 @@ describe('ReportUtils', () => { }); it('should return true for an archived report', () => { - expect(shouldShowFlagComment(validReportAction, expenseReport, false)).toBe(true); + // It will always be true here because it's an expense report and not a chat report + // so isArchivedNonExpenseReport(report, true) will always be false + expect(shouldShowFlagComment(validReportAction, expenseReport, true)).toBe(true); }); it('should return true for a non-archived report', () => { + // It will always be true here because it's an expense report and not a chat report + // so isArchivedNonExpenseReport(report, false) will always be false expect(shouldShowFlagComment(validReportAction, expenseReport, false)).toBe(true); }); }); @@ -3685,15 +3693,48 @@ describe('ReportUtils', () => { }); it('should return false for an archived report', () => { - expect(shouldShowFlagComment(actionFromConcierge, chatReport, false)).toBe(false); + // It will be false here because it's a chat report with an action from Concierge + // so it doesn't matter if the report is archived or not, it will always be false. + expect(shouldShowFlagComment(actionFromConcierge, chatReport, true)).toBe(false); }); it('should return false for a non-archived report', () => { + // It will be false here because it's a chat report with an action from Concierge + // so it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(actionFromConcierge, chatReport, false)).toBe(false); }); }); - describe('Action from Concierge', () => { + describe('Chat with Chronos', () => { + let chatReport: Report; + + beforeAll(async () => { + chatReport = { + ...createRandomReport(60000), + type: CONST.REPORT.TYPE.CHAT, + participants: buildParticipantsFromAccountIDs([currentUserAccountID, CONST.ACCOUNT_ID.CHRONOS]), + }; + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, chatReport); + }); + + afterAll(async () => { + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); + }); + + it('should return false for an archived report', () => { + // It will be false here because it's a chat report with Chronos and therefore + // it doesn't matter if the report is archived or not, it will always be false. + expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); + }); + + it('should return false for a non-archived report', () => { + // It will be false here because it's a chat report with Chronos and therefore + // it doesn't matter if the report is archived or not, it will always be false. + expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); + }); + }); + + describe('Chat with Concierge', () => { let chatReport: Report; beforeAll(async () => { @@ -3703,17 +3744,23 @@ describe('ReportUtils', () => { participants: buildParticipantsFromAccountIDs([currentUserAccountID, CONST.ACCOUNT_ID.CONCIERGE]), }; await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, chatReport); + await Onyx.set(`${ONYXKEYS.CONCIERGE_REPORT_ID}`, chatReport.reportID); }); afterAll(async () => { await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); + await Onyx.set(`${ONYXKEYS.CONCIERGE_REPORT_ID}`, null); }); it('should return false for an archived report', () => { + // It will be false here because it's a chat report with Concierge and therefore + // it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); }); it('should return false for a non-archived report', () => { + // It will be false here because it's a chat report with Concierge and therefore + // it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); }); }); From da339bcfb94ee62915b351c157ece5f9a9c49bfe Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 30 Jun 2025 14:15:31 -0500 Subject: [PATCH 05/22] Change order of tests --- tests/unit/ReportUtilsTest.ts | 52 +++++++++++++++++------------------ 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index bd0fee569b09..5a058118b911 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -3672,18 +3672,14 @@ describe('ReportUtils', () => { }); }); - describe('Action from Concierge', () => { + describe('Chat with Chronos', () => { let chatReport: Report; - const actionFromConcierge: ReportAction = { - ...createRandomReportAction(1), - actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, - actorAccountID: CONST.ACCOUNT_ID.CONCIERGE, - }; beforeAll(async () => { chatReport = { ...createRandomReport(60000), type: CONST.REPORT.TYPE.CHAT, + participants: buildParticipantsFromAccountIDs([currentUserAccountID, CONST.ACCOUNT_ID.CHRONOS]), }; await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, chatReport); }); @@ -3693,75 +3689,79 @@ describe('ReportUtils', () => { }); it('should return false for an archived report', () => { - // It will be false here because it's a chat report with an action from Concierge - // so it doesn't matter if the report is archived or not, it will always be false. - expect(shouldShowFlagComment(actionFromConcierge, chatReport, true)).toBe(false); + // It will be false here because it's a chat report with Chronos and therefore + // it doesn't matter if the report is archived or not, it will always be false. + expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); }); it('should return false for a non-archived report', () => { - // It will be false here because it's a chat report with an action from Concierge - // so it doesn't matter if the report is archived or not, it will always be false. - expect(shouldShowFlagComment(actionFromConcierge, chatReport, false)).toBe(false); + // It will be false here because it's a chat report with Chronos and therefore + // it doesn't matter if the report is archived or not, it will always be false. + expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); }); }); - describe('Chat with Chronos', () => { + describe('Chat with Concierge', () => { let chatReport: Report; beforeAll(async () => { chatReport = { ...createRandomReport(60000), type: CONST.REPORT.TYPE.CHAT, - participants: buildParticipantsFromAccountIDs([currentUserAccountID, CONST.ACCOUNT_ID.CHRONOS]), + participants: buildParticipantsFromAccountIDs([currentUserAccountID, CONST.ACCOUNT_ID.CONCIERGE]), }; await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, chatReport); + await Onyx.set(`${ONYXKEYS.CONCIERGE_REPORT_ID}`, chatReport.reportID); }); afterAll(async () => { await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); + await Onyx.set(`${ONYXKEYS.CONCIERGE_REPORT_ID}`, null); }); it('should return false for an archived report', () => { - // It will be false here because it's a chat report with Chronos and therefore + // It will be false here because it's a chat report with Concierge and therefore // it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); }); it('should return false for a non-archived report', () => { - // It will be false here because it's a chat report with Chronos and therefore + // It will be false here because it's a chat report with Concierge and therefore // it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); }); }); - describe('Chat with Concierge', () => { + describe('Action from Concierge', () => { let chatReport: Report; + const actionFromConcierge: ReportAction = { + ...createRandomReportAction(1), + actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, + actorAccountID: CONST.ACCOUNT_ID.CONCIERGE, + }; beforeAll(async () => { chatReport = { ...createRandomReport(60000), type: CONST.REPORT.TYPE.CHAT, - participants: buildParticipantsFromAccountIDs([currentUserAccountID, CONST.ACCOUNT_ID.CONCIERGE]), }; await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, chatReport); - await Onyx.set(`${ONYXKEYS.CONCIERGE_REPORT_ID}`, chatReport.reportID); }); afterAll(async () => { await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); - await Onyx.set(`${ONYXKEYS.CONCIERGE_REPORT_ID}`, null); }); it('should return false for an archived report', () => { - // It will be false here because it's a chat report with Concierge and therefore - // it doesn't matter if the report is archived or not, it will always be false. - expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); + // It will be false here because it's a chat report with an action from Concierge + // so it doesn't matter if the report is archived or not, it will always be false. + expect(shouldShowFlagComment(actionFromConcierge, chatReport, true)).toBe(false); }); it('should return false for a non-archived report', () => { - // It will be false here because it's a chat report with Concierge and therefore - // it doesn't matter if the report is archived or not, it will always be false. - expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); + // It will be false here because it's a chat report with an action from Concierge + // so it doesn't matter if the report is archived or not, it will always be false. + expect(shouldShowFlagComment(actionFromConcierge, chatReport, false)).toBe(false); }); }); }); From 02f22e45e2de12c8e10526a423d0c78b17bdce30 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 30 Jun 2025 14:33:04 -0500 Subject: [PATCH 06/22] Add tests for flaggable actions --- tests/unit/ReportUtilsTest.ts | 61 ++++++++++++++++++++++++++++++----- 1 file changed, 53 insertions(+), 8 deletions(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 5a058118b911..1bd0f48f116c 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -3616,6 +3616,51 @@ describe('ReportUtils', () => { actorAccountID: 123456, }; + describe('can flag report action', () => { + let expenseReport: Report; + const reportActionFromConcierge: ReportAction = { + ...createRandomReportAction(1), + actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, + actorAccountID: CONST.ACCOUNT_ID.CONCIERGE, + }; + + beforeAll(async () => { + expenseReport = { + ...createRandomReport(60000), + type: CONST.REPORT.TYPE.EXPENSE, + }; + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${expenseReport.reportID}`, expenseReport); + }); + + afterAll(async () => { + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${expenseReport.reportID}`, null); + }); + + it('should return true for an archived expense report with an action that can be flagged', () => { + // It will be true here because the report action can be flagged since it was a whisper from someone that is not Concierge + // It doesn't matter if the expense report is archived or not + expect(shouldShowFlagComment(validReportAction, expenseReport, true)).toBe(true); + }); + + it('should return true for a non-archived expense report with an action that can be flagged', () => { + // It will be true here because the report action can be flagged since it was a whisper from someone that is not Concierge + // It doesn't matter if the expense report is archived or not + expect(shouldShowFlagComment(validReportAction, expenseReport, false)).toBe(true); + }); + + it('should return false for an archived expense report with an action that cannot be flagged', () => { + // It will be false here because the report action cannot be flagged since it was a whisper from Concierge + // It doesn't matter if the expense report is archived or not + expect(shouldShowFlagComment(reportActionFromConcierge, expenseReport, true)).toBe(false); + }); + + it('should return false for a non-archived expense report with an action that cannot be flagged', () => { + // It will be false here because the report action cannot be flagged since it was a whisper from Concierge + // It doesn't matter if the expense report is archived or not + expect(shouldShowFlagComment(reportActionFromConcierge, expenseReport, false)).toBe(false); + }); + }); + describe('chat reports', () => { let chatReport: Report; @@ -3659,13 +3704,13 @@ describe('ReportUtils', () => { await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${expenseReport.reportID}`, null); }); - it('should return true for an archived report', () => { + it('should return true for an archived expense report', () => { // It will always be true here because it's an expense report and not a chat report // so isArchivedNonExpenseReport(report, true) will always be false expect(shouldShowFlagComment(validReportAction, expenseReport, true)).toBe(true); }); - it('should return true for a non-archived report', () => { + it('should return true for a non-archived expense report', () => { // It will always be true here because it's an expense report and not a chat report // so isArchivedNonExpenseReport(report, false) will always be false expect(shouldShowFlagComment(validReportAction, expenseReport, false)).toBe(true); @@ -3688,13 +3733,13 @@ describe('ReportUtils', () => { await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); }); - it('should return false for an archived report', () => { + it('should return false for an archived chat report', () => { // It will be false here because it's a chat report with Chronos and therefore // it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); }); - it('should return false for a non-archived report', () => { + it('should return false for a non-archived chat report', () => { // It will be false here because it's a chat report with Chronos and therefore // it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); @@ -3719,13 +3764,13 @@ describe('ReportUtils', () => { await Onyx.set(`${ONYXKEYS.CONCIERGE_REPORT_ID}`, null); }); - it('should return false for an archived report', () => { + it('should return false for an archived chat report', () => { // It will be false here because it's a chat report with Concierge and therefore // it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); }); - it('should return false for a non-archived report', () => { + it('should return false for a non-archived chat report', () => { // It will be false here because it's a chat report with Concierge and therefore // it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); @@ -3752,13 +3797,13 @@ describe('ReportUtils', () => { await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); }); - it('should return false for an archived report', () => { + it('should return false for an archived chatreport', () => { // It will be false here because it's a chat report with an action from Concierge // so it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(actionFromConcierge, chatReport, true)).toBe(false); }); - it('should return false for a non-archived report', () => { + it('should return false for a non-archived chat report', () => { // It will be false here because it's a chat report with an action from Concierge // so it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(actionFromConcierge, chatReport, false)).toBe(false); From da93875f7260cffa9fcf22f33999593752a5924d Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 30 Jun 2025 14:41:01 -0500 Subject: [PATCH 07/22] Remove debug --- src/libs/ReportUtils.ts | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index 3545c55916a2..948e26d4bef9 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -8401,14 +8401,6 @@ function canFlagReportAction(reportAction: OnyxInputOrEntry, repor * Whether flag comment page should show */ function shouldShowFlagComment(reportAction: OnyxInputOrEntry, report: OnyxInputOrEntry, isReportArchived = false): boolean { - console.log(` - canFlagReportAction: ${canFlagReportAction(reportAction, report?.reportID)} - isArchivedNonExpenseReport: ${isArchivedNonExpenseReport(report, isReportArchived)} - chatIncludesChronos: ${chatIncludesChronos(report)} - isConciergeChatReport: ${isConciergeChatReport(report)} - reportAction?.actorAccountID: ${reportAction?.actorAccountID} - Concierge: ${CONST.ACCOUNT_ID.CONCIERGE} - `); return ( canFlagReportAction(reportAction, report?.reportID) && !isArchivedNonExpenseReport(report, isReportArchived) && From e67002d3979e74d03455674f70ffb00c29edbab0 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 30 Jun 2025 14:41:13 -0500 Subject: [PATCH 08/22] Fix import --- tests/unit/ReportUtilsTest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index cd75bc720dfe..70d85cea0654 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -58,8 +58,8 @@ import { shouldDisableRename, shouldDisableThread, shouldReportBeInOptionList, - shouldShowFlagComment, shouldReportShowSubscript, + shouldShowFlagComment, temporary_getMoneyRequestOptions, } from '@libs/ReportUtils'; import type {OptionData} from '@libs/ReportUtils'; From 96bb800472c145008556842e2d448e6544caa6c1 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 30 Jun 2025 14:52:53 -0500 Subject: [PATCH 09/22] Type objects --- tests/utils/collections/policies.ts | 10 +++++----- tests/utils/collections/reportActions.ts | 2 +- tests/utils/collections/reports.ts | 2 +- tests/utils/collections/transaction.ts | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/utils/collections/policies.ts b/tests/utils/collections/policies.ts index ba88837b7c71..ac4500983aef 100644 --- a/tests/utils/collections/policies.ts +++ b/tests/utils/collections/policies.ts @@ -7,7 +7,7 @@ export default function createRandomPolicy(index: number, type?: ValueOf, typeof CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MANUAL> => frequency !== CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MANUAL, ), - ), + ) as Policy['autoReportingFrequency'], harvesting: { enabled: randBoolean(), }, autoReportingOffset: 1, preventSelfApproval: randBoolean(), outputCurrency: randCurrencyCode(), - role: rand(Object.values(CONST.POLICY.ROLE)), + role: rand(Object.values(CONST.POLICY.ROLE)) as Policy['role'], owner: randEmail(), ownerAccountID: index, avatarURL: randAvatar(), isFromFullPolicy: randBoolean(), lastModified: randPastDate().toISOString(), - pendingAction: rand(Object.values(CONST.RED_BRICK_ROAD_PENDING_ACTION)), + pendingAction: rand(Object.values(CONST.RED_BRICK_ROAD_PENDING_ACTION)) as Policy['pendingAction'], errors: {}, customUnits: {}, errorFields: {}, - approvalMode: rand(Object.values(CONST.POLICY.APPROVAL_MODE)), + approvalMode: rand(Object.values(CONST.POLICY.APPROVAL_MODE)) as Policy['approvalMode'], }; } diff --git a/tests/utils/collections/reportActions.ts b/tests/utils/collections/reportActions.ts index 9a05ba6ad992..4c6b27e80c5d 100644 --- a/tests/utils/collections/reportActions.ts +++ b/tests/utils/collections/reportActions.ts @@ -69,7 +69,7 @@ export default function createRandomReportAction(index: number): ReportAction { automatic: randBoolean(), shouldShow: randBoolean(), lastModified: getRandomDate(), - pendingAction: rand(Object.values(CONST.RED_BRICK_ROAD_PENDING_ACTION)), + pendingAction: rand(Object.values(CONST.RED_BRICK_ROAD_PENDING_ACTION)) as ReportAction['pendingAction'], delegateAccountID: index, errors: {}, isAttachmentOnly: randBoolean(), diff --git a/tests/utils/collections/reports.ts b/tests/utils/collections/reports.ts index 66731df148e8..eb8279b67ab3 100644 --- a/tests/utils/collections/reports.ts +++ b/tests/utils/collections/reports.ts @@ -9,7 +9,7 @@ import type {Report} from '@src/types/onyx'; function createRandomReport(index: number): Report { return { reportID: index.toString(), - chatType: rand(Object.values(CONST.REPORT.CHAT_TYPE)), + chatType: rand(Object.values(CONST.REPORT.CHAT_TYPE)) as Report['chatType'], currency: randCurrencyCode(), ownerAccountID: index, isPinned: randBoolean(), diff --git a/tests/utils/collections/transaction.ts b/tests/utils/collections/transaction.ts index 3f9fdf7ca1e3..463e33e7a58a 100644 --- a/tests/utils/collections/transaction.ts +++ b/tests/utils/collections/transaction.ts @@ -33,12 +33,12 @@ export default function createRandomTransaction(index: number): Transaction { merchant: randWord(), modifiedMerchant: randWord(), originalAmount: randAmount(), - originalCurrency: rand(Object.values(CONST.CURRENCY)), + originalCurrency: rand(Object.values(CONST.CURRENCY)) as Transaction['originalCurrency'], reportID: index.toString(), transactionID: index.toString(), tag: randWord(), parentTransactionID: index.toString(), - status: rand(Object.values(CONST.TRANSACTION.STATUS)), + status: rand(Object.values(CONST.TRANSACTION.STATUS)) as Transaction['status'], receipt: {}, reimbursable: randBoolean(), hasEReceipt: randBoolean(), From 3c05e7f49dddb8a532c81270014ba6079eaba6e5 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 1 Jul 2025 07:37:02 -0500 Subject: [PATCH 10/22] Revert type defs --- tests/utils/collections/policies.ts | 10 +++++----- tests/utils/collections/reportActions.ts | 2 +- tests/utils/collections/reports.ts | 2 +- tests/utils/collections/transaction.ts | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/utils/collections/policies.ts b/tests/utils/collections/policies.ts index ac4500983aef..ba88837b7c71 100644 --- a/tests/utils/collections/policies.ts +++ b/tests/utils/collections/policies.ts @@ -7,7 +7,7 @@ export default function createRandomPolicy(index: number, type?: ValueOf, typeof CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MANUAL> => frequency !== CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MANUAL, ), - ) as Policy['autoReportingFrequency'], + ), harvesting: { enabled: randBoolean(), }, autoReportingOffset: 1, preventSelfApproval: randBoolean(), outputCurrency: randCurrencyCode(), - role: rand(Object.values(CONST.POLICY.ROLE)) as Policy['role'], + role: rand(Object.values(CONST.POLICY.ROLE)), owner: randEmail(), ownerAccountID: index, avatarURL: randAvatar(), isFromFullPolicy: randBoolean(), lastModified: randPastDate().toISOString(), - pendingAction: rand(Object.values(CONST.RED_BRICK_ROAD_PENDING_ACTION)) as Policy['pendingAction'], + pendingAction: rand(Object.values(CONST.RED_BRICK_ROAD_PENDING_ACTION)), errors: {}, customUnits: {}, errorFields: {}, - approvalMode: rand(Object.values(CONST.POLICY.APPROVAL_MODE)) as Policy['approvalMode'], + approvalMode: rand(Object.values(CONST.POLICY.APPROVAL_MODE)), }; } diff --git a/tests/utils/collections/reportActions.ts b/tests/utils/collections/reportActions.ts index 4c6b27e80c5d..9a05ba6ad992 100644 --- a/tests/utils/collections/reportActions.ts +++ b/tests/utils/collections/reportActions.ts @@ -69,7 +69,7 @@ export default function createRandomReportAction(index: number): ReportAction { automatic: randBoolean(), shouldShow: randBoolean(), lastModified: getRandomDate(), - pendingAction: rand(Object.values(CONST.RED_BRICK_ROAD_PENDING_ACTION)) as ReportAction['pendingAction'], + pendingAction: rand(Object.values(CONST.RED_BRICK_ROAD_PENDING_ACTION)), delegateAccountID: index, errors: {}, isAttachmentOnly: randBoolean(), diff --git a/tests/utils/collections/reports.ts b/tests/utils/collections/reports.ts index eb8279b67ab3..66731df148e8 100644 --- a/tests/utils/collections/reports.ts +++ b/tests/utils/collections/reports.ts @@ -9,7 +9,7 @@ import type {Report} from '@src/types/onyx'; function createRandomReport(index: number): Report { return { reportID: index.toString(), - chatType: rand(Object.values(CONST.REPORT.CHAT_TYPE)) as Report['chatType'], + chatType: rand(Object.values(CONST.REPORT.CHAT_TYPE)), currency: randCurrencyCode(), ownerAccountID: index, isPinned: randBoolean(), diff --git a/tests/utils/collections/transaction.ts b/tests/utils/collections/transaction.ts index 463e33e7a58a..3f9fdf7ca1e3 100644 --- a/tests/utils/collections/transaction.ts +++ b/tests/utils/collections/transaction.ts @@ -33,12 +33,12 @@ export default function createRandomTransaction(index: number): Transaction { merchant: randWord(), modifiedMerchant: randWord(), originalAmount: randAmount(), - originalCurrency: rand(Object.values(CONST.CURRENCY)) as Transaction['originalCurrency'], + originalCurrency: rand(Object.values(CONST.CURRENCY)), reportID: index.toString(), transactionID: index.toString(), tag: randWord(), parentTransactionID: index.toString(), - status: rand(Object.values(CONST.TRANSACTION.STATUS)) as Transaction['status'], + status: rand(Object.values(CONST.TRANSACTION.STATUS)), receipt: {}, reimbursable: randBoolean(), hasEReceipt: randBoolean(), From 6aff80fb4e49d031c6d3a89effbaa35fd1d81263 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 1 Jul 2025 07:37:54 -0500 Subject: [PATCH 11/22] Typo --- tests/unit/ReportUtilsTest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 70d85cea0654..2497f06f00e2 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -3975,7 +3975,7 @@ describe('ReportUtils', () => { await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); }); - it('should return false for an archived chatreport', () => { + it('should return false for an archived chat report', () => { // It will be false here because it's a chat report with an action from Concierge // so it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(actionFromConcierge, chatReport, true)).toBe(false); From 4f45533d72fb7e121d59b85cfbe10758653ba16f Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 1 Jul 2025 07:49:02 -0500 Subject: [PATCH 12/22] Clean up tests from review --- tests/unit/ReportUtilsTest.ts | 82 ++--------------------------------- 1 file changed, 4 insertions(+), 78 deletions(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 2497f06f00e2..7981f3e47345 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -3785,6 +3785,8 @@ describe('ReportUtils', () => { }); }); + // Note: shouldShowFlagComment() calls isArchivedNonExpenseReport() which has it's own unit tests, so whether + // the report is an expense report or not does not need to be tested here. describe('shouldShowFlagComment', () => { const validReportAction: ReportAction = { ...createRandomReportAction(1), @@ -3815,86 +3817,22 @@ describe('ReportUtils', () => { }); it('should return true for an archived expense report with an action that can be flagged', () => { - // It will be true here because the report action can be flagged since it was a whisper from someone that is not Concierge - // It doesn't matter if the expense report is archived or not expect(shouldShowFlagComment(validReportAction, expenseReport, true)).toBe(true); }); it('should return true for a non-archived expense report with an action that can be flagged', () => { - // It will be true here because the report action can be flagged since it was a whisper from someone that is not Concierge - // It doesn't matter if the expense report is archived or not expect(shouldShowFlagComment(validReportAction, expenseReport, false)).toBe(true); }); it('should return false for an archived expense report with an action that cannot be flagged', () => { - // It will be false here because the report action cannot be flagged since it was a whisper from Concierge - // It doesn't matter if the expense report is archived or not expect(shouldShowFlagComment(reportActionFromConcierge, expenseReport, true)).toBe(false); }); it('should return false for a non-archived expense report with an action that cannot be flagged', () => { - // It will be false here because the report action cannot be flagged since it was a whisper from Concierge - // It doesn't matter if the expense report is archived or not expect(shouldShowFlagComment(reportActionFromConcierge, expenseReport, false)).toBe(false); }); }); - describe('chat reports', () => { - let chatReport: Report; - - beforeAll(async () => { - chatReport = { - ...createRandomReport(60000), - type: CONST.REPORT.TYPE.CHAT, - }; - await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, chatReport); - }); - - afterAll(async () => { - await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); - }); - - it('should return false for an archived chat report', () => { - // It will be false here because it's a chat report and therefore, it depends on if the report is archived or not - // so isArchivedNonExpenseReport(report, true) will be true, and shouldShowFlagComment checks for false. - expect(shouldShowFlagComment(validReportAction, chatReport, true)).toBe(false); - }); - - it('should return true for a non-archived chat report', () => { - // It will be true here because it's a chat report and therefore, it depends on if the report is archived or not - // so isArchivedNonExpenseReport(report, false) will be false, and shouldShowFlagComment checks for false. - expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(true); - }); - }); - - describe('expense reports', () => { - let expenseReport: Report; - - beforeAll(async () => { - expenseReport = { - ...createRandomReport(60000), - type: CONST.REPORT.TYPE.EXPENSE, - }; - await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${expenseReport.reportID}`, expenseReport); - }); - - afterAll(async () => { - await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${expenseReport.reportID}`, null); - }); - - it('should return true for an archived expense report', () => { - // It will always be true here because it's an expense report and not a chat report - // so isArchivedNonExpenseReport(report, true) will always be false - expect(shouldShowFlagComment(validReportAction, expenseReport, true)).toBe(true); - }); - - it('should return true for a non-archived expense report', () => { - // It will always be true here because it's an expense report and not a chat report - // so isArchivedNonExpenseReport(report, false) will always be false - expect(shouldShowFlagComment(validReportAction, expenseReport, false)).toBe(true); - }); - }); - describe('Chat with Chronos', () => { let chatReport: Report; @@ -3912,14 +3850,10 @@ describe('ReportUtils', () => { }); it('should return false for an archived chat report', () => { - // It will be false here because it's a chat report with Chronos and therefore - // it doesn't matter if the report is archived or not, it will always be false. - expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); + expect(shouldShowFlagComment(validReportAction, chatReport, true)).toBe(false); }); it('should return false for a non-archived chat report', () => { - // It will be false here because it's a chat report with Chronos and therefore - // it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); }); }); @@ -3943,14 +3877,10 @@ describe('ReportUtils', () => { }); it('should return false for an archived chat report', () => { - // It will be false here because it's a chat report with Concierge and therefore - // it doesn't matter if the report is archived or not, it will always be false. - expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); + expect(shouldShowFlagComment(validReportAction, chatReport, true)).toBe(false); }); it('should return false for a non-archived chat report', () => { - // It will be false here because it's a chat report with Concierge and therefore - // it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); }); }); @@ -3976,14 +3906,10 @@ describe('ReportUtils', () => { }); it('should return false for an archived chat report', () => { - // It will be false here because it's a chat report with an action from Concierge - // so it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(actionFromConcierge, chatReport, true)).toBe(false); }); it('should return false for a non-archived chat report', () => { - // It will be false here because it's a chat report with an action from Concierge - // so it doesn't matter if the report is archived or not, it will always be false. expect(shouldShowFlagComment(actionFromConcierge, chatReport, false)).toBe(false); }); }); From 14dbb5682addf0fa8b84ce582f8e00b74f656699 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 1 Jul 2025 07:51:43 -0500 Subject: [PATCH 13/22] Have more clear variable names --- tests/unit/ReportUtilsTest.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 7981f3e47345..09eb03d4cd31 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -3798,9 +3798,16 @@ describe('ReportUtils', () => { describe('can flag report action', () => { let expenseReport: Report; - const reportActionFromConcierge: ReportAction = { + const reportActionThatCanBeFlagged: ReportAction = { + ...validReportAction, + }; + + // eslint-disable-next-line rulesdir/no-negated-variables + const reportActionThatCannotBeFlagged: ReportAction = { ...createRandomReportAction(1), actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, + + // If the actor is Concierge, the report action cannot be flagged actorAccountID: CONST.ACCOUNT_ID.CONCIERGE, }; @@ -3817,19 +3824,19 @@ describe('ReportUtils', () => { }); it('should return true for an archived expense report with an action that can be flagged', () => { - expect(shouldShowFlagComment(validReportAction, expenseReport, true)).toBe(true); + expect(shouldShowFlagComment(reportActionThatCanBeFlagged, expenseReport, true)).toBe(true); }); it('should return true for a non-archived expense report with an action that can be flagged', () => { - expect(shouldShowFlagComment(validReportAction, expenseReport, false)).toBe(true); + expect(shouldShowFlagComment(reportActionThatCanBeFlagged, expenseReport, false)).toBe(true); }); it('should return false for an archived expense report with an action that cannot be flagged', () => { - expect(shouldShowFlagComment(reportActionFromConcierge, expenseReport, true)).toBe(false); + expect(shouldShowFlagComment(reportActionThatCannotBeFlagged, expenseReport, true)).toBe(false); }); it('should return false for a non-archived expense report with an action that cannot be flagged', () => { - expect(shouldShowFlagComment(reportActionFromConcierge, expenseReport, false)).toBe(false); + expect(shouldShowFlagComment(reportActionThatCannotBeFlagged, expenseReport, false)).toBe(false); }); }); From 87eecac63d28f62dd10a93945f35ca0e76f80024 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 2 Jul 2025 15:43:02 -0500 Subject: [PATCH 14/22] Add some test skeletons --- tests/unit/ReportUtilsTest.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 09eb03d4cd31..b54d5924d1f7 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -3785,6 +3785,25 @@ describe('ReportUtils', () => { }); }); + describe('isWhisperAction', () => {}); + + describe('canFlagReportAction', () => { + describe('a whisper action', () => { + it('cannot be flagged if it is from concierge', () => {}); + it('cannot be flagged if it is from the current user', () => {}); + it('can be flagged if it is not from concierge or the current user', () => {}); + }); + describe('a non-whisper action', () => { + it('cannot be flagged if it is from the current user', () => {}); + it('cannot be flagged if the action name is something other than ADD_COMMENT', () => {}); + it('cannot be flagged if the action is deleted', () => {}); + it('cannot be flagged if the action is a created task report', () => {}); + it('cannot be flagged if the report does not exist', () => {}); + it('cannot be flagged if the report is not allowed to be commented on', () => {}); + it('can be flagged', () => {}); + }); + }); + // Note: shouldShowFlagComment() calls isArchivedNonExpenseReport() which has it's own unit tests, so whether // the report is an expense report or not does not need to be tested here. describe('shouldShowFlagComment', () => { From a637e33db41aa5d16bc57b70a563a4fa9acdf7bd Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 2 Jul 2025 15:50:38 -0500 Subject: [PATCH 15/22] More test skeletons --- tests/unit/ReportUtilsTest.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index b54d5924d1f7..71089eb957af 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -3785,7 +3785,10 @@ describe('ReportUtils', () => { }); }); - describe('isWhisperAction', () => {}); + describe('isWhisperAction', () => { + it('is a whisper action if the reportAction.message.whisperedTo is not empty', () => {}); + it('is not a whisper action if the reportAction.originalMessage.whisperedTo is empty', () => {}); + }); describe('canFlagReportAction', () => { describe('a whisper action', () => { From adcc3c1790dfa28591d4d5b29ca168efdcdb6bef Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 2 Jul 2025 15:57:08 -0500 Subject: [PATCH 16/22] Add tests for whisper actions --- tests/unit/ReportUtilsTest.ts | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 71089eb957af..276160b69e1a 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -9,7 +9,7 @@ import {putOnHold} from '@libs/actions/IOU'; import type {OnboardingTaskLinks} from '@libs/actions/Welcome/OnboardingFlow'; import DateUtils from '@libs/DateUtils'; import {translateLocal} from '@libs/Localize'; -import {getOriginalMessage} from '@libs/ReportActionsUtils'; +import {getOriginalMessage, isWhisperAction} from '@libs/ReportActionsUtils'; import { buildOptimisticChatReport, buildOptimisticCreatedReportAction, @@ -3786,8 +3786,23 @@ describe('ReportUtils', () => { }); describe('isWhisperAction', () => { - it('is a whisper action if the reportAction.message.whisperedTo is not empty', () => {}); - it('is not a whisper action if the reportAction.originalMessage.whisperedTo is empty', () => {}); + it('an action where reportAction.message.whisperedTo has accountIDs is a whisper action', () => { + const whisperReportAction: ReportAction = { + ...createRandomReportAction(1), + }; + expect(isWhisperAction(whisperReportAction)).toBe(true); + }); + it('an action where reportAction.originalMessage.whisperedTo does not exist is not a whisper action', () => { + const nonWhisperReportAction = { + ...createRandomReportAction(1), + message: [ + { + whisperedTo: undefined, + }, + ], + }; + expect(isWhisperAction(nonWhisperReportAction as ReportAction)).toBe(false); + }); }); describe('canFlagReportAction', () => { From df59716a540e7b7846ef7cfa682eae298bc554cc Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 2 Jul 2025 16:03:01 -0500 Subject: [PATCH 17/22] Write tests for whisper actions --- tests/unit/ReportUtilsTest.ts | 44 +++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 5 deletions(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 276160b69e1a..caddf649546b 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -25,6 +25,7 @@ import { canEditReportDescription, canEditRoomVisibility, canEditWriteCapability, + canFlagReportAction, canHoldUnholdReportAction, findLastAccessedReport, getAllAncestorReportActions, @@ -3800,18 +3801,51 @@ describe('ReportUtils', () => { whisperedTo: undefined, }, ], - }; - expect(isWhisperAction(nonWhisperReportAction as ReportAction)).toBe(false); + } as ReportAction; + expect(isWhisperAction(nonWhisperReportAction)).toBe(false); }); }); describe('canFlagReportAction', () => { describe('a whisper action', () => { - it('cannot be flagged if it is from concierge', () => {}); - it('cannot be flagged if it is from the current user', () => {}); - it('can be flagged if it is not from concierge or the current user', () => {}); + const whisperReportAction: ReportAction = { + ...createRandomReportAction(1), + }; + + it('cannot be flagged if it is from concierge', () => { + const whisperReportActionFromConcierge = { + ...whisperReportAction, + actorAccountID: CONST.ACCOUNT_ID.CONCIERGE, + }; + + // The reportID doesn't matter because there is an early return for whisper actions and the report is not looked at + expect(canFlagReportAction(whisperReportActionFromConcierge, '123456')).toBe(false); + }); + + it('cannot be flagged if it is from the current user', () => { + const whisperReportActionFromCurrentUser = { + ...whisperReportAction, + actorAccountID: currentUserAccountID, + }; + + // The reportID doesn't matter because there is an early return for whisper actions and the report is not looked at + expect(canFlagReportAction(whisperReportActionFromCurrentUser, '123456')).toBe(false); + }); + + it('can be flagged if it is not from concierge or the current user', () => { + expect(canFlagReportAction(whisperReportAction, '123456')).toBe(true); + }); }); + describe('a non-whisper action', () => { + const nonWhisperReportAction = { + ...createRandomReportAction(1), + message: [ + { + whisperedTo: undefined, + }, + ], + } as ReportAction; it('cannot be flagged if it is from the current user', () => {}); it('cannot be flagged if the action name is something other than ADD_COMMENT', () => {}); it('cannot be flagged if the action is deleted', () => {}); From da8e7042fb327fdf2d22ce6e40ef4e2d6dbd6bf5 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 2 Jul 2025 16:03:19 -0500 Subject: [PATCH 18/22] Move logic after early return --- src/libs/ReportUtils.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index 1e669f50b08f..97c0cb895819 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -8372,13 +8372,6 @@ function chatIncludesChronosWithID(reportOrID?: string | SearchReport): boolean * - It's an ADD_COMMENT that is not an attachment */ function canFlagReportAction(reportAction: OnyxInputOrEntry, reportID: string | undefined): boolean { - let report = getReportOrDraftReport(reportID); - - // If the childReportID exists in reportAction and is equal to the reportID, - // the report action being evaluated is the parent report action in a thread, and we should get the parent report to evaluate instead. - if (reportAction?.childReportID?.toString() === reportID?.toString()) { - report = getReportOrDraftReport(report?.parentReportID); - } const isCurrentUserAction = reportAction?.actorAccountID === currentUserAccountID; if (isWhisperAction(reportAction)) { // Allow flagging whispers that are sent by other users @@ -8390,6 +8383,14 @@ function canFlagReportAction(reportAction: OnyxInputOrEntry, repor return false; } + let report = getReportOrDraftReport(reportID); + + // If the childReportID exists in reportAction and is equal to the reportID, + // the report action being evaluated is the parent report action in a thread, and we should get the parent report to evaluate instead. + if (reportAction?.childReportID?.toString() === reportID?.toString()) { + report = getReportOrDraftReport(report?.parentReportID); + } + return !!( !isCurrentUserAction && reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT && From fd1348a4eac3d8188985486439884aaba1719ede Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 2 Jul 2025 16:13:29 -0500 Subject: [PATCH 19/22] Start adding tests for non whispers --- tests/unit/ReportUtilsTest.ts | 42 +++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index caddf649546b..00376a92da9a 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -77,7 +77,7 @@ import {chatReportR14932 as mockedChatReport} from '../../__mocks__/reportData/r import * as NumberUtils from '../../src/libs/NumberUtils'; import {convertedInvoiceChat} from '../data/Invoice'; import createRandomPolicy from '../utils/collections/policies'; -import createRandomReportAction from '../utils/collections/reportActions'; +import createRandomReportAction, {getRandomDate} from '../utils/collections/reportActions'; import { createAdminRoom, createAnnounceRoom, @@ -3838,6 +3838,9 @@ describe('ReportUtils', () => { }); describe('a non-whisper action', () => { + const report = { + ...createRandomReport(1), + }; const nonWhisperReportAction = { ...createRandomReportAction(1), message: [ @@ -3846,12 +3849,43 @@ describe('ReportUtils', () => { }, ], } as ReportAction; - it('cannot be flagged if it is from the current user', () => {}); - it('cannot be flagged if the action name is something other than ADD_COMMENT', () => {}); - it('cannot be flagged if the action is deleted', () => {}); + + it('cannot be flagged if it is from the current user', () => { + const nonWhisperReportActionFromCurrentUser = { + ...nonWhisperReportAction, + actorAccountID: currentUserAccountID, + }; + expect(canFlagReportAction(nonWhisperReportActionFromCurrentUser, report.reportID)).toBe(false); + }); + + it('cannot be flagged if the action name is something other than ADD_COMMENT', () => { + const nonWhisperReportActionWithDifferentActionName = { + ...nonWhisperReportAction, + actionName: CONST.REPORT.ACTIONS.TYPE.APPROVED, + }; + expect(canFlagReportAction(nonWhisperReportActionWithDifferentActionName, report.reportID)).toBe(false); + }); + + it('cannot be flagged if the action is deleted', () => { + const deletedReportAction = { + ...nonWhisperReportAction, + message: [ + { + ...nonWhisperReportAction.message[0], + html: '', + deleted: getRandomDate(), + }, + ], + } as ReportAction; + expect(canFlagReportAction(deletedReportAction, report.reportID)).toBe(false); + }); + it('cannot be flagged if the action is a created task report', () => {}); + it('cannot be flagged if the report does not exist', () => {}); + it('cannot be flagged if the report is not allowed to be commented on', () => {}); + it('can be flagged', () => {}); }); }); From 2893c5bccaca12bcd08393db9bcde78b399c07c3 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 2 Jul 2025 16:26:53 -0500 Subject: [PATCH 20/22] Finish writing tests --- tests/unit/ReportUtilsTest.ts | 45 +++++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 00376a92da9a..6c63a3415e67 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -3843,6 +3843,7 @@ describe('ReportUtils', () => { }; const nonWhisperReportAction = { ...createRandomReportAction(1), + actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, message: [ { whisperedTo: undefined, @@ -3850,6 +3851,14 @@ describe('ReportUtils', () => { ], } as ReportAction; + beforeAll(async () => { + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, report); + }); + + afterAll(async () => { + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, null); + }); + it('cannot be flagged if it is from the current user', () => { const nonWhisperReportActionFromCurrentUser = { ...nonWhisperReportAction, @@ -3871,7 +3880,7 @@ describe('ReportUtils', () => { ...nonWhisperReportAction, message: [ { - ...nonWhisperReportAction.message[0], + whisperedTo: undefined, html: '', deleted: getRandomDate(), }, @@ -3880,13 +3889,39 @@ describe('ReportUtils', () => { expect(canFlagReportAction(deletedReportAction, report.reportID)).toBe(false); }); - it('cannot be flagged if the action is a created task report', () => {}); + it('cannot be flagged if the action is a created task report', () => { + const createdTaskReportAction = { + ...nonWhisperReportAction, + message: [ + { + whisperedTo: undefined, - it('cannot be flagged if the report does not exist', () => {}); + // This signifies that the action is a created task report along with the ADD_COMMENT action name + taskReportID: '123456', + }, + ], + } as ReportAction; + expect(canFlagReportAction(createdTaskReportAction, report.reportID)).toBe(false); + }); - it('cannot be flagged if the report is not allowed to be commented on', () => {}); + it('cannot be flagged if the report does not exist', () => { + expect(canFlagReportAction(nonWhisperReportAction, 'starwarsisthebest')).toBe(false); + }); - it('can be flagged', () => {}); + it('cannot be flagged if the report is not allowed to be commented on', () => { + // eslint-disable-next-line rulesdir/no-negated-variables + const reportThatCannotBeCommentedOn = { + ...createRandomReport(2), + + // If the permissions does not contain WRITE, then it cannot be commented on + permissions: [], + } as Report; + expect(canFlagReportAction(nonWhisperReportAction, reportThatCannotBeCommentedOn.reportID)).toBe(false); + }); + + it('can be flagged', () => { + expect(canFlagReportAction(nonWhisperReportAction, report.reportID)).toBe(true); + }); }); }); From c0f5319dc42b5b99d003f59c508d1cf5ac55f2d3 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 2 Jul 2025 16:44:53 -0500 Subject: [PATCH 21/22] Fix tests --- tests/unit/ReportUtilsTest.ts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 6c63a3415e67..5b98288be8da 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -3892,14 +3892,10 @@ describe('ReportUtils', () => { it('cannot be flagged if the action is a created task report', () => { const createdTaskReportAction = { ...nonWhisperReportAction, - message: [ - { - whisperedTo: undefined, - - // This signifies that the action is a created task report along with the ADD_COMMENT action name - taskReportID: '123456', - }, - ], + originalMessage: { + // This signifies that the action is a created task report along with the ADD_COMMENT action name + taskReportID: '123456', + }, } as ReportAction; expect(canFlagReportAction(createdTaskReportAction, report.reportID)).toBe(false); }); From 319570613dc453547bdde46a4cd8640002a2139f Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Fri, 4 Jul 2025 09:57:19 -0600 Subject: [PATCH 22/22] Disable spelling --- tests/unit/ReportUtilsTest.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index f23a12754f2a..760ea8005f97 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -4037,6 +4037,7 @@ describe('ReportUtils', () => { }); it('cannot be flagged if the report does not exist', () => { + // cspell:disable-next-line expect(canFlagReportAction(nonWhisperReportAction, 'starwarsisthebest')).toBe(false); });