From dc656f15f787f18accb5b503c6a57000f983a2c9 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 22 Apr 2025 11:09:16 -0600 Subject: [PATCH 1/6] Remove third argument --- src/libs/actions/Task.ts | 5 ++--- src/pages/Search/EmptySearchView.tsx | 2 +- src/pages/tasks/TaskAssigneeSelectorModal.tsx | 2 +- src/pages/tasks/TaskDescriptionPage.tsx | 2 +- tests/actions/TaskTest.ts | 8 ++++---- 5 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/libs/actions/Task.ts b/src/libs/actions/Task.ts index b9882aff1e89..04c6214d1b5e 100644 --- a/src/libs/actions/Task.ts +++ b/src/libs/actions/Task.ts @@ -1219,7 +1219,7 @@ function getTaskOwnerAccountID(taskReport: OnyxEntry): number /** * Check if you're allowed to modify the task - only the author can modify the task */ -function canModifyTask(taskReport: OnyxEntry, sessionAccountID: number, taskOwnerAccountID?: number, isParentReportArchived = false): boolean { +function canModifyTask(taskReport: OnyxEntry, sessionAccountID: number, isParentReportArchived = false): boolean { if (ReportUtils.isCanceledTaskReport(taskReport)) { return false; } @@ -1228,8 +1228,7 @@ function canModifyTask(taskReport: OnyxEntry, sessionAccountID return false; } - const ownerAccountID = getTaskOwnerAccountID(taskReport) ?? taskOwnerAccountID; - return sessionAccountID === ownerAccountID; + return sessionAccountID === taskReport?.ownerAccountID; } /** diff --git a/src/pages/Search/EmptySearchView.tsx b/src/pages/Search/EmptySearchView.tsx index 11b48e679a0e..e0f7785bf147 100644 --- a/src/pages/Search/EmptySearchView.tsx +++ b/src/pages/Search/EmptySearchView.tsx @@ -113,7 +113,7 @@ function EmptySearchView({type, hasResults}: EmptySearchViewProps) { const [viewTourTaskReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${viewTourTaskReportID}`, {canBeMissing: false}); const currentUserPersonalDetails = useCurrentUserPersonalDetails(); const isReportArchived = useReportIsArchived(viewTourTaskReport?.parentReportID); - const canModifyTheTask = canModifyTask(viewTourTaskReport, currentUserPersonalDetails.accountID, undefined, isReportArchived); + const canModifyTheTask = canModifyTask(viewTourTaskReport, currentUserPersonalDetails.accountID, isReportArchived); const canActionTheTask = canActionTask(viewTourTaskReport, currentUserPersonalDetails.accountID); const content = useMemo(() => { diff --git a/src/pages/tasks/TaskAssigneeSelectorModal.tsx b/src/pages/tasks/TaskAssigneeSelectorModal.tsx index 2b2fc7f96ab4..75d9ef1cd780 100644 --- a/src/pages/tasks/TaskAssigneeSelectorModal.tsx +++ b/src/pages/tasks/TaskAssigneeSelectorModal.tsx @@ -202,7 +202,7 @@ function TaskAssigneeSelectorModal() { const isOpen = isOpenTaskReport(report); const isReportArchived = useReportIsArchived(report?.parentReportID); - const canModifyTaskValue = canModifyTask(report, currentUserPersonalDetails.accountID, undefined, isReportArchived); + const canModifyTaskValue = canModifyTask(report, currentUserPersonalDetails.accountID, isReportArchived); const isTaskNonEditable = isTaskReport(report) && (!canModifyTaskValue || !isOpen); useEffect(() => { diff --git a/src/pages/tasks/TaskDescriptionPage.tsx b/src/pages/tasks/TaskDescriptionPage.tsx index 3d09232586e9..f96bf3d593af 100644 --- a/src/pages/tasks/TaskDescriptionPage.tsx +++ b/src/pages/tasks/TaskDescriptionPage.tsx @@ -74,7 +74,7 @@ function TaskDescriptionPage({report, currentUserPersonalDetails}: TaskDescripti const isOpen = isOpenTaskReport(report); const isReportArchived = useReportIsArchived(report?.reportID); - const canActuallyModifyTask = canModifyTask(report, currentUserPersonalDetails.accountID, undefined, isReportArchived); + const canActuallyModifyTask = canModifyTask(report, currentUserPersonalDetails.accountID, isReportArchived); const isTaskNonEditable = isTaskReport(report) && (!canActuallyModifyTask || !isOpen); useFocusEffect( diff --git a/tests/actions/TaskTest.ts b/tests/actions/TaskTest.ts index 7a962020fd7c..dc5b1c5280e1 100644 --- a/tests/actions/TaskTest.ts +++ b/tests/actions/TaskTest.ts @@ -46,19 +46,19 @@ describe('actions/Task', () => { it('returns false if the user modifying the task is not the author', () => { // Simulate how components call canModifyTask() by using the hook useReportIsArchived() to see if the report is archived const {result: isReportArchived} = renderHook(() => useReportIsArchived(report?.reportID)); - expect(canModifyTask(report, employeeAccountID, undefined, isReportArchived.current)).toBe(false); + expect(canModifyTask(report, employeeAccountID, isReportArchived.current)).toBe(false); }); it('returns false if the parent report is archived', () => { const {result: isReportArchived} = renderHook(() => useReportIsArchived(archivedReport?.reportID)); - expect(canModifyTask(archivedReport, managerAccountID, undefined, isReportArchived.current)).toBe(false); + expect(canModifyTask(archivedReport, managerAccountID, isReportArchived.current)).toBe(false); }); it('returns false if the report is a cancelled task report', () => { const {result: isReportArchived} = renderHook(() => useReportIsArchived(cancelledTaskReport?.reportID)); - expect(canModifyTask(cancelledTaskReport, managerAccountID, undefined, isReportArchived.current)).toBe(false); + expect(canModifyTask(cancelledTaskReport, managerAccountID, isReportArchived.current)).toBe(false); }); it('returns true if the user modifying the task is the author and the parent report is not archived or cancelled', () => { const {result: isReportArchived} = renderHook(() => useReportIsArchived(report?.reportID)); - expect(canModifyTask(report, managerAccountID, undefined, isReportArchived.current)).toBe(true); + expect(canModifyTask(report, managerAccountID, isReportArchived.current)).toBe(true); }); }); }); From ff4f3140873ab15ba530b207810d4569e9e74e56 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 22 Apr 2025 11:09:35 -0600 Subject: [PATCH 2/6] Remove unnecessary method --- src/libs/actions/Task.ts | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/libs/actions/Task.ts b/src/libs/actions/Task.ts index 04c6214d1b5e..5115f26a6374 100644 --- a/src/libs/actions/Task.ts +++ b/src/libs/actions/Task.ts @@ -633,7 +633,7 @@ function editTaskAssignee(report: OnyxTypes.Report, sessionAccountID: number, as const assigneeChatReportID = assigneeChatReport?.reportID; const assigneeChatReportMetadata = ReportUtils.getReportMetadata(assigneeChatReportID); const parentReport = getParentReport(report); - const taskOwnerAccountID = getTaskOwnerAccountID(report); + const taskOwnerAccountID = report?.ownerAccountID; const optimisticReport: OptimisticReport = { reportName, managerID: assigneeAccountID ?? report.managerID, @@ -1209,13 +1209,6 @@ function getTaskAssigneeAccountID(taskReport: OnyxEntry): numb return reportAction?.childManagerAccountID; } -/** - * Returns Task owner accountID - */ -function getTaskOwnerAccountID(taskReport: OnyxEntry): number | undefined { - return taskReport?.ownerAccountID; -} - /** * Check if you're allowed to modify the task - only the author can modify the task */ @@ -1248,7 +1241,7 @@ function canActionTask(taskReport: OnyxEntry, sessionAccountID return false; } - const ownerAccountID = getTaskOwnerAccountID(taskReport) ?? taskOwnerAccountID; + const ownerAccountID = taskReport?.ownerAccountID ?? taskOwnerAccountID; const assigneeAccountID = getTaskAssigneeAccountID(taskReport) ?? taskAssigneeAccountID; return sessionAccountID === ownerAccountID || sessionAccountID === assigneeAccountID; } From 0c71a60f570f59232e6eb61899159c08dd1f3c10 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 22 Apr 2025 11:40:05 -0600 Subject: [PATCH 3/6] Use parent report ID --- src/pages/tasks/TaskDescriptionPage.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pages/tasks/TaskDescriptionPage.tsx b/src/pages/tasks/TaskDescriptionPage.tsx index f96bf3d593af..e04b40d8cb8e 100644 --- a/src/pages/tasks/TaskDescriptionPage.tsx +++ b/src/pages/tasks/TaskDescriptionPage.tsx @@ -73,7 +73,7 @@ function TaskDescriptionPage({report, currentUserPersonalDetails}: TaskDescripti const focusTimeoutRef = useRef(null); const isOpen = isOpenTaskReport(report); - const isReportArchived = useReportIsArchived(report?.reportID); + const isReportArchived = useReportIsArchived(report?.parentReportID); const canActuallyModifyTask = canModifyTask(report, currentUserPersonalDetails.accountID, isReportArchived); const isTaskNonEditable = isTaskReport(report) && (!canActuallyModifyTask || !isOpen); From 933562ee079f01261224e82d5dee1d6cd192729c Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 22 Apr 2025 11:40:15 -0600 Subject: [PATCH 4/6] Update tests to more accurately use parent reports --- tests/actions/TaskTest.ts | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/tests/actions/TaskTest.ts b/tests/actions/TaskTest.ts index dc5b1c5280e1..74593f160d72 100644 --- a/tests/actions/TaskTest.ts +++ b/tests/actions/TaskTest.ts @@ -17,16 +17,30 @@ describe('actions/Task', () => { }); describe('canModifyTask', () => { - const managerAccountID = 1; - const employeeAccountID = 2; + const managerAccountID = 3; + const employeeAccountID = 4; + + // Report with a normal parent const report = LHNTestUtils.getFakeReport([managerAccountID, employeeAccountID]); - const archivedReport = LHNTestUtils.getFakeReport([managerAccountID, employeeAccountID]); + const reportParent = LHNTestUtils.getFakeReport([managerAccountID, employeeAccountID]); + + // Cancelled report with a normal parent const cancelledTaskReport = LHNTestUtils.getFakeReport([managerAccountID, employeeAccountID]); + const cancelledTaskReportParent = LHNTestUtils.getFakeReport([managerAccountID, employeeAccountID]); + + // Report with an archived parent + const reportArchived = LHNTestUtils.getFakeReport([managerAccountID, employeeAccountID]); + const reportArchivedParent = LHNTestUtils.getFakeReport([managerAccountID, employeeAccountID]); // Set the manager as the owner of each report report.ownerAccountID = managerAccountID; - archivedReport.ownerAccountID = managerAccountID; cancelledTaskReport.ownerAccountID = managerAccountID; + reportArchived.ownerAccountID = managerAccountID; + + // Set the parent report ID of each report + report.parentReportID = reportParent.reportID; + cancelledTaskReport.parentReportID = cancelledTaskReportParent.reportID; + reportArchived.parentReportID = reportArchivedParent.reportID; // This is what indicates that the report is a cancelled task report (see ReportUtils.isCanceledTaskReport()) cancelledTaskReport.isDeletedParentAction = true; @@ -34,30 +48,33 @@ describe('actions/Task', () => { beforeAll(async () => { // Store all the necessary data in Onyx await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, report); - await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${archivedReport.reportID}`, archivedReport); + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${reportParent.reportID}`, reportParent); await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${cancelledTaskReport.reportID}`, cancelledTaskReport); + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${cancelledTaskReportParent.reportID}`, cancelledTaskReportParent); + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${reportArchived.reportID}`, reportArchived); + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${reportArchivedParent.reportID}`, reportArchivedParent); // This is what indicates that a report is archived (see ReportUtils.isArchivedReport()) - await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${archivedReport.reportID}`, { + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportArchivedParent.reportID}`, { private_isArchived: new Date().toString(), }); }); it('returns false if the user modifying the task is not the author', () => { // Simulate how components call canModifyTask() by using the hook useReportIsArchived() to see if the report is archived - const {result: isReportArchived} = renderHook(() => useReportIsArchived(report?.reportID)); + const {result: isReportArchived} = renderHook(() => useReportIsArchived(report?.parentReportID)); expect(canModifyTask(report, employeeAccountID, isReportArchived.current)).toBe(false); }); it('returns false if the parent report is archived', () => { - const {result: isReportArchived} = renderHook(() => useReportIsArchived(archivedReport?.reportID)); - expect(canModifyTask(archivedReport, managerAccountID, isReportArchived.current)).toBe(false); + const {result: isReportArchived} = renderHook(() => useReportIsArchived(reportArchived?.parentReportID)); + expect(canModifyTask(reportArchived, managerAccountID, isReportArchived.current)).toBe(false); }); it('returns false if the report is a cancelled task report', () => { - const {result: isReportArchived} = renderHook(() => useReportIsArchived(cancelledTaskReport?.reportID)); + const {result: isReportArchived} = renderHook(() => useReportIsArchived(cancelledTaskReport?.parentReportID)); expect(canModifyTask(cancelledTaskReport, managerAccountID, isReportArchived.current)).toBe(false); }); it('returns true if the user modifying the task is the author and the parent report is not archived or cancelled', () => { - const {result: isReportArchived} = renderHook(() => useReportIsArchived(report?.reportID)); + const {result: isReportArchived} = renderHook(() => useReportIsArchived(report?.parentReportID)); expect(canModifyTask(report, managerAccountID, isReportArchived.current)).toBe(true); }); }); From 131778008e5c201d472de9716a720185388e9089 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 22 Apr 2025 11:40:49 -0600 Subject: [PATCH 5/6] Update test account IDs --- tests/actions/TaskTest.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/actions/TaskTest.ts b/tests/actions/TaskTest.ts index 74593f160d72..e9e63ebc0fd2 100644 --- a/tests/actions/TaskTest.ts +++ b/tests/actions/TaskTest.ts @@ -17,8 +17,8 @@ describe('actions/Task', () => { }); describe('canModifyTask', () => { - const managerAccountID = 3; - const employeeAccountID = 4; + const managerAccountID = 1; + const employeeAccountID = 2; // Report with a normal parent const report = LHNTestUtils.getFakeReport([managerAccountID, employeeAccountID]); From 16887096d34610b45434de927957138469dcd004 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 22 Apr 2025 11:41:31 -0600 Subject: [PATCH 6/6] Update comment --- tests/actions/TaskTest.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/actions/TaskTest.ts b/tests/actions/TaskTest.ts index e9e63ebc0fd2..c53f53b2966a 100644 --- a/tests/actions/TaskTest.ts +++ b/tests/actions/TaskTest.ts @@ -20,11 +20,11 @@ describe('actions/Task', () => { const managerAccountID = 1; const employeeAccountID = 2; - // Report with a normal parent + // Report with a non-archived parent const report = LHNTestUtils.getFakeReport([managerAccountID, employeeAccountID]); const reportParent = LHNTestUtils.getFakeReport([managerAccountID, employeeAccountID]); - // Cancelled report with a normal parent + // Cancelled report with a non-archived parent const cancelledTaskReport = LHNTestUtils.getFakeReport([managerAccountID, employeeAccountID]); const cancelledTaskReportParent = LHNTestUtils.getFakeReport([managerAccountID, employeeAccountID]);