From 2bfd0b51dcaa3d5d963b6c12f8bc7b410e488a63 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 14 Mar 2024 12:29:56 -0600 Subject: [PATCH 1/8] Mark method as deprecated --- src/libs/ReportUtils.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index 8dc1c9967f13..5e652ad5e8cc 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -574,6 +574,9 @@ function getRootParentReport(report: OnyxEntry | undefined | EmptyObject return getRootParentReport(!isEmptyObject(parentReport) ? parentReport : null); } +/** + * @deprecated Use withOnyx or Onyx.connect() instead + */ function getPolicy(policyID: string | undefined): Policy | EmptyObject { if (!allPolicies || !policyID) { return {}; From 94e658f06a1cdb2264eebb83f612ab37baa50574 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 14 Mar 2024 12:34:53 -0600 Subject: [PATCH 2/8] Get parent report from onyx --- src/pages/FlagCommentPage.tsx | 4 ++-- .../home/report/withReportAndReportActionOrNotFound.tsx | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/pages/FlagCommentPage.tsx b/src/pages/FlagCommentPage.tsx index 216196c17d55..53aac0ce2a8b 100644 --- a/src/pages/FlagCommentPage.tsx +++ b/src/pages/FlagCommentPage.tsx @@ -54,7 +54,7 @@ function getReportID(route: FlagCommentPageNavigationProps['route']) { return route.params.reportID.toString(); } -function FlagCommentPage({parentReportAction, route, report, reportActions}: FlagCommentPageProps) { +function FlagCommentPage({parentReportAction, route, report, parentReport, reportActions}: FlagCommentPageProps) { const styles = useThemeStyles(); const {translate} = useLocalize(); @@ -130,7 +130,7 @@ function FlagCommentPage({parentReportAction, route, report, reportActions}: Fla // Handle threads if needed if (ReportUtils.isChatThread(report) && reportAction?.reportActionID === parentReportAction?.reportActionID) { - reportID = ReportUtils.getParentReport(report)?.reportID; + reportID = parentReport?.reportID; } if (reportAction && ReportUtils.canFlagReportAction(reportAction, reportID)) { diff --git a/src/pages/home/report/withReportAndReportActionOrNotFound.tsx b/src/pages/home/report/withReportAndReportActionOrNotFound.tsx index 3fe43e96266a..73bd3532bfc8 100644 --- a/src/pages/home/report/withReportAndReportActionOrNotFound.tsx +++ b/src/pages/home/report/withReportAndReportActionOrNotFound.tsx @@ -22,6 +22,9 @@ type OnyxProps = { /** The report currently being looked at */ report: OnyxEntry; + /** The parent report if the current report is a thread and it has a parent */ + parentReport: OnyxEntry; + /** The report metadata */ reportMetadata: OnyxEntry; @@ -103,6 +106,9 @@ export default function `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`, }, + parentReport: { + key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT}${report ? report.parentReportID : '0'}`, + }, reportMetadata: { key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_METADATA}${route.params.reportID}`, }, From f5f252c55384363790170aac5eba36eb26acc7c4 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 14 Mar 2024 12:50:14 -0600 Subject: [PATCH 3/8] Remove outside calls for getting parent report --- src/libs/actions/Task.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/libs/actions/Task.ts b/src/libs/actions/Task.ts index 48ab7cce9186..adf6a300181c 100644 --- a/src/libs/actions/Task.ts +++ b/src/libs/actions/Task.ts @@ -71,6 +71,13 @@ Onyx.connect({ }, }); +let allReports: OnyxCollection; +Onyx.connect({ + key: ONYXKEYS.COLLECTION.REPORT, + waitForCollectionCallback: true, + callback: (value) => (allReports = value), +}); + /** * Clears out the task info from the store */ @@ -758,7 +765,7 @@ function deleteTask(report: OnyxEntry) { const optimisticCancelReportAction = ReportUtils.buildOptimisticTaskReportAction(report.reportID ?? '', CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED, message); const optimisticReportActionID = optimisticCancelReportAction.reportActionID; const parentReportAction = getParentReportAction(report); - const parentReport = ReportUtils.getParentReport(report); + const parentReport = report?.parentReportID ? allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID}`] ?? {} : {}; // If the task report is the last visible action in the parent report, we should navigate back to the parent report const shouldDeleteTaskReport = !ReportActionsUtils.doesReportHaveVisibleActions(report.reportID ?? ''); @@ -927,7 +934,7 @@ function canModifyTask(taskReport: OnyxEntry, sessionAccountID return false; } - const parentReport = ReportUtils.getParentReport(taskReport); + const parentReport = taskReport?.parentReportID ? allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${taskReport.parentReportID}`] ?? {} : {}; if (ReportUtils.isArchivedRoom(parentReport)) { return false; } From 26766e7e2cbd59b692bb1c87069474ee1b24ec99 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 14 Mar 2024 12:50:57 -0600 Subject: [PATCH 4/8] Enforce that method should never be exported --- src/libs/ReportUtils.ts | 3 +-- tests/actions/EnforceActionExportRestrictions.js | 11 +++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 tests/actions/EnforceActionExportRestrictions.js diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index 5e652ad5e8cc..0f7d40f3c7d1 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -545,7 +545,7 @@ function getReport(reportID: string | undefined): OnyxEntry | EmptyObjec } /** - * Returns the parentReport if the given report is a thread. + * Returns the parentReport if the given report is a thread */ function getParentReport(report: OnyxEntry | EmptyObject): OnyxEntry | EmptyObject { if (!report?.parentReportID) { @@ -5423,7 +5423,6 @@ export { isSettled, isAllowedToComment, getBankAccountRoute, - getParentReport, getRootParentReport, getReportPreviewMessage, isMoneyRequestReportPendingDeletion, diff --git a/tests/actions/EnforceActionExportRestrictions.js b/tests/actions/EnforceActionExportRestrictions.js new file mode 100644 index 000000000000..bc0bf837b7d6 --- /dev/null +++ b/tests/actions/EnforceActionExportRestrictions.js @@ -0,0 +1,11 @@ +import * as ReportUtils from '@libs/ReportUtils'; + +// There are some methods that are OK to use inside an action file, but should not be exported. These are typically methods that look up and return Onyx data. +// The correct pattern to use is that every file will use it's own withOnyx or Onyx.connect() to access the Onyx data it needs. This prevents data from becoming stale +// and prevents side-effects that you may not be aware of. It also allows each file to access Onyx data in the most performant way. More context can be found in +// https://github.com/Expensify/App/issues/27262 +describe('ReportUtils', () => { + it('does not export getParentReport', () => { + expect(ReportUtils.getParentReport).toBeUndefined(); + }) +}); From 4f24c587577a3c793a76786485e8d3da1dd8914c Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 14 Mar 2024 12:58:04 -0600 Subject: [PATCH 5/8] Fix TS errors and DRY code --- src/libs/actions/Task.ts | 15 +++++++++++++-- tests/actions/EnforceActionExportRestrictions.js | 7 +++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/libs/actions/Task.ts b/src/libs/actions/Task.ts index adf6a300181c..491a75f9c643 100644 --- a/src/libs/actions/Task.ts +++ b/src/libs/actions/Task.ts @@ -24,6 +24,7 @@ import type {Icon} from '@src/types/onyx/OnyxCommon'; import type {ReportActions} from '@src/types/onyx/ReportAction'; import type ReportAction from '@src/types/onyx/ReportAction'; import {isEmptyObject} from '@src/types/utils/EmptyObject'; +import type {EmptyObject} from '@src/types/utils/EmptyObject'; import * as Report from './Report'; type OptimisticReport = Pick; @@ -754,6 +755,16 @@ function getParentReportAction(report: OnyxEntry): ReportActio return allReportActions?.[report.parentReportID]?.[report.parentReportActionID] ?? {}; } +/** + * Returns the parentReport if the given report is a thread + */ +function getParentReport(report: OnyxEntry | EmptyObject): OnyxEntry | EmptyObject { + if (!report?.parentReportID) { + return {}; + } + return allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID}`] ?? {}; +} + /** * Cancels a task by setting the report state to SUBMITTED and status to CLOSED */ @@ -765,7 +776,7 @@ function deleteTask(report: OnyxEntry) { const optimisticCancelReportAction = ReportUtils.buildOptimisticTaskReportAction(report.reportID ?? '', CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED, message); const optimisticReportActionID = optimisticCancelReportAction.reportActionID; const parentReportAction = getParentReportAction(report); - const parentReport = report?.parentReportID ? allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID}`] ?? {} : {}; + const parentReport = getParentReport(report); // If the task report is the last visible action in the parent report, we should navigate back to the parent report const shouldDeleteTaskReport = !ReportActionsUtils.doesReportHaveVisibleActions(report.reportID ?? ''); @@ -934,7 +945,7 @@ function canModifyTask(taskReport: OnyxEntry, sessionAccountID return false; } - const parentReport = taskReport?.parentReportID ? allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${taskReport.parentReportID}`] ?? {} : {}; + const parentReport = getParentReport(taskReport); if (ReportUtils.isArchivedRoom(parentReport)) { return false; } diff --git a/tests/actions/EnforceActionExportRestrictions.js b/tests/actions/EnforceActionExportRestrictions.js index bc0bf837b7d6..1f9499df8d28 100644 --- a/tests/actions/EnforceActionExportRestrictions.js +++ b/tests/actions/EnforceActionExportRestrictions.js @@ -1,4 +1,5 @@ import * as ReportUtils from '@libs/ReportUtils'; +import * as Task from '@libs/Task'; // There are some methods that are OK to use inside an action file, but should not be exported. These are typically methods that look up and return Onyx data. // The correct pattern to use is that every file will use it's own withOnyx or Onyx.connect() to access the Onyx data it needs. This prevents data from becoming stale @@ -9,3 +10,9 @@ describe('ReportUtils', () => { expect(ReportUtils.getParentReport).toBeUndefined(); }) }); + +describe('Task', () => { + it('does not export getParentReport', () => { + expect(Task.getParentReport).toBeUndefined(); + }) +}); From 444e68dbfe5539a84547ec9e0701af1a0996503e Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 14 Mar 2024 14:34:39 -0600 Subject: [PATCH 6/8] Fix import path and style --- tests/actions/EnforceActionExportRestrictions.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/actions/EnforceActionExportRestrictions.js b/tests/actions/EnforceActionExportRestrictions.js index 1f9499df8d28..efd2bbe97139 100644 --- a/tests/actions/EnforceActionExportRestrictions.js +++ b/tests/actions/EnforceActionExportRestrictions.js @@ -1,5 +1,5 @@ import * as ReportUtils from '@libs/ReportUtils'; -import * as Task from '@libs/Task'; +import * as Task from '@userActions/Task'; // There are some methods that are OK to use inside an action file, but should not be exported. These are typically methods that look up and return Onyx data. // The correct pattern to use is that every file will use it's own withOnyx or Onyx.connect() to access the Onyx data it needs. This prevents data from becoming stale @@ -8,11 +8,11 @@ import * as Task from '@libs/Task'; describe('ReportUtils', () => { it('does not export getParentReport', () => { expect(ReportUtils.getParentReport).toBeUndefined(); - }) + }); }); describe('Task', () => { it('does not export getParentReport', () => { expect(Task.getParentReport).toBeUndefined(); - }) + }); }); From 4e597ab8c2d275a3fde4d65816f9135ec6fa5803 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 14 Mar 2024 14:51:35 -0600 Subject: [PATCH 7/8] Write tests in TS --- ...ExportRestrictions.js => EnforceActionExportRestrictions.ts} | 2 ++ 1 file changed, 2 insertions(+) rename tests/actions/{EnforceActionExportRestrictions.js => EnforceActionExportRestrictions.ts} (82%) diff --git a/tests/actions/EnforceActionExportRestrictions.js b/tests/actions/EnforceActionExportRestrictions.ts similarity index 82% rename from tests/actions/EnforceActionExportRestrictions.js rename to tests/actions/EnforceActionExportRestrictions.ts index efd2bbe97139..10088b90c2db 100644 --- a/tests/actions/EnforceActionExportRestrictions.js +++ b/tests/actions/EnforceActionExportRestrictions.ts @@ -7,12 +7,14 @@ import * as Task from '@userActions/Task'; // https://github.com/Expensify/App/issues/27262 describe('ReportUtils', () => { it('does not export getParentReport', () => { + // @ts-expect-error the tets is asserting that it's undefined, so the TS error is normal expect(ReportUtils.getParentReport).toBeUndefined(); }); }); describe('Task', () => { it('does not export getParentReport', () => { + // @ts-expect-error the tets is asserting that it's undefined, so the TS error is normal expect(Task.getParentReport).toBeUndefined(); }); }); From b5da053b09610b50312af338dd339bae40cae0e5 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 14 Mar 2024 14:52:19 -0600 Subject: [PATCH 8/8] Fix typo --- tests/actions/EnforceActionExportRestrictions.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/actions/EnforceActionExportRestrictions.ts b/tests/actions/EnforceActionExportRestrictions.ts index 10088b90c2db..92d96869d02d 100644 --- a/tests/actions/EnforceActionExportRestrictions.ts +++ b/tests/actions/EnforceActionExportRestrictions.ts @@ -7,14 +7,14 @@ import * as Task from '@userActions/Task'; // https://github.com/Expensify/App/issues/27262 describe('ReportUtils', () => { it('does not export getParentReport', () => { - // @ts-expect-error the tets is asserting that it's undefined, so the TS error is normal + // @ts-expect-error the test is asserting that it's undefined, so the TS error is normal expect(ReportUtils.getParentReport).toBeUndefined(); }); }); describe('Task', () => { it('does not export getParentReport', () => { - // @ts-expect-error the tets is asserting that it's undefined, so the TS error is normal + // @ts-expect-error the test is asserting that it's undefined, so the TS error is normal expect(Task.getParentReport).toBeUndefined(); }); });