Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Refactor FlagCommentPage to access parent report directly from Onyx #38356

Merged
merged 8 commits into from
Mar 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,7 @@ function getReport(reportID: string | undefined): OnyxEntry<Report> | 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<Report> | EmptyObject): OnyxEntry<Report> | EmptyObject {
if (!report?.parentReportID) {
Expand Down Expand Up @@ -574,6 +574,9 @@ function getRootParentReport(report: OnyxEntry<Report> | 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 {};
Expand Down Expand Up @@ -5420,7 +5423,6 @@ export {
isSettled,
isAllowedToComment,
getBankAccountRoute,
getParentReport,
getRootParentReport,
getReportPreviewMessage,
isMoneyRequestReportPendingDeletion,
Expand Down
22 changes: 20 additions & 2 deletions src/libs/actions/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<OnyxTypes.Report, 'reportName' | 'managerID' | 'participantAccountIDs' | 'notificationPreference' | 'pendingFields' | 'visibleChatMemberAccountIDs'>;
Expand Down Expand Up @@ -71,6 +72,13 @@ Onyx.connect({
},
});

let allReports: OnyxCollection<OnyxTypes.Report>;
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (value) => (allReports = value),
});

/**
* Clears out the task info from the store
*/
Expand Down Expand Up @@ -747,6 +755,16 @@ function getParentReportAction(report: OnyxEntry<OnyxTypes.Report>): ReportActio
return allReportActions?.[report.parentReportID]?.[report.parentReportActionID] ?? {};
}

/**
* Returns the parentReport if the given report is a thread
*/
function getParentReport(report: OnyxEntry<OnyxTypes.Report> | EmptyObject): OnyxEntry<OnyxTypes.Report> | EmptyObject {
if (!report?.parentReportID) {
return {};
}
return allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID}`] ?? {};
}
Comment on lines +761 to +766
Copy link
Contributor

Choose a reason for hiding this comment

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

I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)

Seems we are intentionally doing this, but just mentioning...

Copy link
Contributor

Choose a reason for hiding this comment

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

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

found this explaination justifiable 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out!


/**
* Cancels a task by setting the report state to SUBMITTED and status to CLOSED
*/
Expand All @@ -758,7 +776,7 @@ function deleteTask(report: OnyxEntry<OnyxTypes.Report>) {
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 = 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 ?? '');
Expand Down Expand Up @@ -927,7 +945,7 @@ function canModifyTask(taskReport: OnyxEntry<OnyxTypes.Report>, sessionAccountID
return false;
}

const parentReport = ReportUtils.getParentReport(taskReport);
const parentReport = getParentReport(taskReport);
if (ReportUtils.isArchivedRoom(parentReport)) {
return false;
}
Expand Down
4 changes: 2 additions & 2 deletions src/pages/FlagCommentPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ type OnyxProps = {
/** The report currently being looked at */
report: OnyxEntry<OnyxTypes.Report>;

/** The parent report if the current report is a thread and it has a parent */
parentReport: OnyxEntry<OnyxTypes.Report>;

/** The report metadata */
reportMetadata: OnyxEntry<OnyxTypes.ReportMetadata>;

Expand Down Expand Up @@ -103,6 +106,9 @@ export default function <TProps extends WithReportAndReportActionOrNotFoundProps
report: {
key: ({route}) => `${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}`,
},
Expand Down
20 changes: 20 additions & 0 deletions tests/actions/EnforceActionExportRestrictions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import * as ReportUtils from '@libs/ReportUtils';
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
// 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', () => {
// @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 test is asserting that it's undefined, so the TS error is normal
expect(Task.getParentReport).toBeUndefined();
});
});
Loading