Skip to content

Remove the side-loading of data in Task action from the ReportActionUtils #37340

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
cead22 merged 5 commits into from
Mar 4, 2024
Merged

Remove the side-loading of data in Task action from the ReportActionUtils #37340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9870988
Refactor and simplify delete task
tgolen Feb 27, 2024
b47c44a
Improve comments
tgolen Feb 28, 2024
dc3aef2
Remove oiptional chaining
tgolen Feb 29, 2024
f1dac85
Merge branch 'main' into tgolen-refactor-sideloading
tgolen Feb 29, 2024
d172d48
Merge branch 'main' into tgolen-refactor-sideloading
tgolen Mar 4, 2024
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
61 changes: 45 additions & 16 deletions src/libs/actions/Task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import * as Expensicons from '@components/Icon/Expensicons';
import * as API from '@libs/API';
import type {CancelTaskParams, CompleteTaskParams, CreateTaskParams, EditTaskAssigneeParams, EditTaskParams, ReopenTaskParams} from '@libs/API/parameters';
import {WRITE_COMMANDS} from '@libs/API/types';
import * as CollectionUtils from '@libs/CollectionUtils';
import DateUtils from '@libs/DateUtils';
import * as ErrorUtils from '@libs/ErrorUtils';
import * as LocalePhoneNumber from '@libs/LocalePhoneNumber';
Expand All @@ -20,6 +21,8 @@ import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type * as OnyxTypes from '@src/types/onyx';
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 * as Report from './Report';

Expand Down Expand Up @@ -55,6 +58,19 @@ Onyx.connect({
callback: (value) => (allPersonalDetails = value),
});

const allReportActions: OnyxCollection<ReportActions> = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
callback: (actions, key) => {
if (!key || !actions) {
return;
}

const reportID = CollectionUtils.extractCollectionItemID(key);
allReportActions[reportID] = actions;
},
});

/**
* Clears out the task info from the store
*/
Expand Down Expand Up @@ -703,19 +719,32 @@ function getShareDestination(reportID: string, reports: OnyxCollection<OnyxTypes
};
}

/**
* Returns the parentReportAction if the given report is a thread/task.
*/
function getParentReportAction(report: OnyxEntry<OnyxTypes.Report>): ReportAction | Record<string, never> {
Copy link
Contributor

@cead22 cead22 Feb 28, 2024

Choose a reason for hiding this comment

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

Would it be possible and make sense to pass Report here and to deleteTask instead of OnyxEntry<OnyxTypes.Report>, so that we don't have to check report?? It looks like we only call deleteTask in one place

It looks like before we were using props.report.reportName and not props.report?.reportName

Copy link
Contributor Author

@tgolen tgolen Feb 28, 2024

Choose a reason for hiding this comment

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

I could revert the changes in deleteTask to go back to using const taskReport = ReportUtils.getReport(taskReportID); which returns a Report and not a OnyxEntry<OnyxTypes.Report>, but here is why I removed it.

  • The view already has the full report object from Onyx
  • The view was passing 4 properties of the report object to deleteTask and then getting the full report object from ReportUtils.getReport()
  • This is inefficient and leads to really bad code and possibly stale data
  • By using the same report reference in all the methods, it's assured that the report object will always be correct

Copy link
Contributor

@cead22 cead22 Feb 29, 2024

Choose a reason for hiding this comment

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

Oh yeah I think the change you made was an improvement, so I wasn't suggesting reverting back to the old version, just to go from getParentReportAction(report: OnyxEntry<OnyxTypes.Report>): to getParentReportAction(report: Report):, and remove a lot of ? that would be unnecessary. Is that doable?

Copy link
Contributor Author

@tgolen tgolen Feb 29, 2024

Choose a reason for hiding this comment

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

You can't simply cast one type to another like that, but I asked about it in Slack and I at least found a way to remove all the optional chaining by adding an early return at the beginning of the method.

Copy link
Contributor

@cead22 cead22 Feb 29, 2024

Choose a reason for hiding this comment

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

That works. I was thinking not just updating the param type --that was to illustrate what I meant, but also the function call Session.checkIfActionIsAllowed(Task.deleteTask(props.report));, with something like Session.checkIfActionIsAllowed(Task.deleteTask(props.report as Report)); or whatever TS magic helps with that.

I like checking before and returning early tho 👍

// If the report is not a thread report, then it won't have a parent and an empty object can be returned.
if (!report?.parentReportID || !report.parentReportActionID) {
Copy link
Contributor

@cead22 cead22 Feb 28, 2024

Choose a reason for hiding this comment

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

Why do we need to check for parentReportID? Can you explain in a comment?

Copy link
Contributor Author

@tgolen tgolen Feb 28, 2024

Choose a reason for hiding this comment

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

Comment added (but it's kind of self-explanatory too when you look at the next line and see how those properties are used).

return {};
}
return allReportActions?.[report.parentReportID]?.[report.parentReportActionID] ?? {};
}

/**
* Cancels a task by setting the report state to SUBMITTED and status to CLOSED
*/
function deleteTask(taskReportID: string, taskTitle: string, originalStateNum: number, originalStatusNum: number) {
const message = `deleted task: ${taskTitle}`;
const optimisticCancelReportAction = ReportUtils.buildOptimisticTaskReportAction(taskReportID, CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED, message);
function deleteTask(report: OnyxEntry<OnyxTypes.Report>) {
if (!report) {
return;
}
const message = `deleted task: ${report.reportName}`;
const optimisticCancelReportAction = ReportUtils.buildOptimisticTaskReportAction(report.reportID ?? '', CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED, message);
const optimisticReportActionID = optimisticCancelReportAction.reportActionID;
const taskReport = ReportUtils.getReport(taskReportID);
const parentReportAction = ReportActionsUtils.getParentReportAction(taskReport);
const parentReport = ReportUtils.getParentReport(taskReport);
const parentReportAction = getParentReportAction(report);
const parentReport = ReportUtils.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(taskReportID);
const shouldDeleteTaskReport = !ReportActionsUtils.doesReportHaveVisibleActions(report.reportID ?? '');
const optimisticReportAction: Partial<ReportUtils.OptimisticTaskReportAction> = {
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
previousMessage: parentReportAction.message,
Expand All @@ -739,7 +768,7 @@ function deleteTask(taskReportID: string, taskTitle: string, originalStateNum: n
const optimisticData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${taskReportID}`,
key: `${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`,
value: {
lastVisibleActionCreated: optimisticCancelReportAction.created,
lastMessageText: message,
Expand All @@ -757,7 +786,7 @@ function deleteTask(taskReportID: string, taskTitle: string, originalStateNum: n
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${taskReportID}`,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`,
value: {
[optimisticReportActionID]: optimisticCancelReportAction as OnyxTypes.ReportAction,
},
Expand Down Expand Up @@ -785,7 +814,7 @@ function deleteTask(taskReportID: string, taskTitle: string, originalStateNum: n
const successData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${taskReportID}`,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`,
value: {
[optimisticReportActionID]: {
pendingAction: null,
Expand All @@ -806,15 +835,15 @@ function deleteTask(taskReportID: string, taskTitle: string, originalStateNum: n
const failureData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${taskReportID}`,
key: `${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`,
value: {
stateNum: originalStateNum,
statusNum: originalStatusNum,
stateNum: report.stateNum ?? '',
statusNum: report.statusNum ?? '',
} as OnyxTypes.Report,
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${taskReportID}`,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`,
value: {
[optimisticReportActionID]: null,
},
Expand All @@ -832,7 +861,7 @@ function deleteTask(taskReportID: string, taskTitle: string, originalStateNum: n

const parameters: CancelTaskParams = {
cancelledTaskReportActionID: optimisticReportActionID,
taskReportID,
taskReportID: report.reportID,
};

API.write(WRITE_COMMANDS.CANCEL_TASK, parameters, {optimisticData, successData, failureData});
Expand Down Expand Up @@ -862,7 +891,7 @@ function getTaskAssigneeAccountID(taskReport: OnyxEntry<OnyxTypes.Report>): numb
return taskReport.managerID;
}

const reportAction = ReportActionsUtils.getParentReportAction(taskReport);
const reportAction = getParentReportAction(taskReport);
return reportAction.childManagerAccountID;
}

Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/HeaderView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ function HeaderView(props) {
isVisible={isDeleteTaskConfirmModalVisible}
onConfirm={() => {
setIsDeleteTaskConfirmModalVisible(false);
Session.checkIfActionIsAllowed(Task.deleteTask(props.reportID, props.report.reportName, props.report.stateNum, props.report.statusNum));
Session.checkIfActionIsAllowed(Task.deleteTask(props.report));
}}
onCancel={() => setIsDeleteTaskConfirmModalVisible(false)}
title={translate('task.deleteTask')}
Expand Down