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.

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

Jump to bottom

fix: Update task title without opening the report #32249

Merged
merged 1 commit into from
Dec 8, 2023
Merged

fix: Update task title without opening the report #32249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/components/ReportActionItem/TaskAction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import React from 'react';
import {View} from 'react-native';
import Text from '@components/Text';
import withLocalize, {withLocalizePropTypes} from '@components/withLocalize';
import * as TaskUtils from '@libs/TaskUtils';
import useThemeStyles from '@styles/useThemeStyles';
import * as Task from '@userActions/Task';

const propTypes = {
/** Name of the reportAction action */
Expand All @@ -22,7 +22,7 @@ function TaskAction(props) {
return (
<>
<View style={[styles.flex1, styles.flexRow, styles.alignItemsCenter]}>
<Text style={[styles.chatItemMessage, styles.colorMuted]}>{Task.getTaskReportActionMessage(props.actionName, props.taskReportID, false)}</Text>
<Text style={[styles.chatItemMessage, styles.colorMuted]}>{TaskUtils.getTaskReportActionMessage(props.actionName)}</Text>
</View>
</>
);
Expand Down
3 changes: 2 additions & 1 deletion src/components/ReportActionItem/TaskPreview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import getButtonState from '@libs/getButtonState';
import * as LocalePhoneNumber from '@libs/LocalePhoneNumber';
import Navigation from '@libs/Navigation/Navigation';
import * as ReportUtils from '@libs/ReportUtils';
import * as TaskUtils from '@libs/TaskUtils';
import reportActionPropTypes from '@pages/home/report/reportActionPropTypes';
import * as StyleUtils from '@styles/StyleUtils';
import useThemeStyles from '@styles/useThemeStyles';
Expand Down Expand Up @@ -83,7 +84,7 @@ function TaskPreview(props) {
const isTaskCompleted = !_.isEmpty(props.taskReport)
? props.taskReport.stateNum === CONST.REPORT.STATE_NUM.SUBMITTED && props.taskReport.statusNum === CONST.REPORT.STATUS.APPROVED
: props.action.childStateNum === CONST.REPORT.STATE_NUM.SUBMITTED && props.action.childStatusNum === CONST.REPORT.STATUS.APPROVED;
const taskTitle = props.taskReport.reportName || props.action.childReportName;
const taskTitle = TaskUtils.getTaskTitle(props.taskReportID, props.action.childReportName);
const taskAssigneeAccountID = Task.getTaskAssigneeAccountID(props.taskReport) || props.action.childManagerAccountID;
const assigneeLogin = lodashGet(personalDetails, [taskAssigneeAccountID, 'login'], '');
const assigneeDisplayName = lodashGet(personalDetails, [taskAssigneeAccountID, 'displayName'], '');
Expand Down
2 changes: 2 additions & 0 deletions src/languages/en.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ import type {
SplitAmountParams,
StepCounterParams,
TagSelectionParams,
TaskCreatedActionParams,
ThreadRequestReportNameParams,
ThreadSentMoneyReportNameParams,
ToValidateLoginParams,
Expand Down Expand Up @@ -1662,6 +1663,7 @@ export default {
assignee: 'Assignee',
completed: 'Completed',
messages: {
created: ({title}: TaskCreatedActionParams) => `task for ${title}`,
completed: 'marked as complete',
canceled: 'deleted task',
reopened: 'marked as incomplete',
Expand Down
2 changes: 2 additions & 0 deletions src/languages/es.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ import type {
SplitAmountParams,
StepCounterParams,
TagSelectionParams,
TaskCreatedActionParams,
ThreadRequestReportNameParams,
ThreadSentMoneyReportNameParams,
ToValidateLoginParams,
Expand Down Expand Up @@ -1686,6 +1687,7 @@ export default {
assignee: 'Usuario asignado',
completed: 'Completada',
messages: {
created: ({title}: TaskCreatedActionParams) => `tarea para ${title}`,
completed: 'marcada como completa',
canceled: 'tarea eliminado',
reopened: 'marcada como incompleta',
Expand Down
3 changes: 3 additions & 0 deletions src/languages/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,8 @@ type TagSelectionParams = {tagName: string};

type WalletProgramParams = {walletProgram: string};

type TaskCreatedActionParams = {title: string};

/* Translation Object types */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
type TranslationBaseValue = string | string[] | ((...args: any[]) => string);
Expand Down Expand Up @@ -321,4 +323,5 @@ export type {
SetTheDistanceParams,
UpdatedTheDistanceParams,
WalletProgramParams,
TaskCreatedActionParams,
};
3 changes: 3 additions & 0 deletions src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import Permissions from './Permissions';
import * as PersonalDetailsUtils from './PersonalDetailsUtils';
import * as ReportActionUtils from './ReportActionsUtils';
import * as ReportUtils from './ReportUtils';
import * as TaskUtils from './TaskUtils';
import * as TransactionUtils from './TransactionUtils';
import * as UserUtils from './UserUtils';

Expand Down Expand Up @@ -410,6 +411,8 @@ function getLastMessageTextForReport(report) {
lastActionName === CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED
) {
lastMessageTextFromReport = lodashGet(lastReportAction, 'message[0].text', '');
} else if (ReportActionUtils.isCreatedTaskReportAction(lastReportAction)) {
lastMessageTextFromReport = TaskUtils.getTaskCreatedMessage(lastReportAction);
} else {
lastMessageTextFromReport = report ? report.lastMessageText || '' : '';
}
Expand Down
4 changes: 2 additions & 2 deletions src/libs/SidebarUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@ import * as OnyxCommon from '@src/types/onyx/OnyxCommon';
import Policy from '@src/types/onyx/Policy';
import Report from '@src/types/onyx/Report';
import ReportAction, {ReportActions} from '@src/types/onyx/ReportAction';
import * as Task from './actions/Task';
import * as CollectionUtils from './CollectionUtils';
import * as LocalePhoneNumber from './LocalePhoneNumber';
import * as Localize from './Localize';
import * as OptionsListUtils from './OptionsListUtils';
import * as PersonalDetailsUtils from './PersonalDetailsUtils';
import * as ReportActionsUtils from './ReportActionsUtils';
import * as ReportUtils from './ReportUtils';
import * as TaskUtils from './TaskUtils';
import * as UserUtils from './UserUtils';

const visibleReportActionItems: ReportActions = {};
Expand Down Expand Up @@ -365,7 +365,7 @@ function getOptionData(
const newName = lastAction?.originalMessage?.newName ?? '';
result.alternateText = Localize.translate(preferredLocale, 'newRoomPage.roomRenamedTo', {newName});
} else if (ReportActionsUtils.isTaskAction(lastAction)) {
result.alternateText = Task.getTaskReportActionMessage(lastAction.actionName, report.reportID, false);
result.alternateText = TaskUtils.getTaskReportActionMessage(lastAction.actionName);
} else if (
lastAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ROOMCHANGELOG.INVITE_TO_ROOM ||
lastAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ROOMCHANGELOG.REMOVE_FROM_ROOM ||
Expand Down
51 changes: 51 additions & 0 deletions src/libs/TaskUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import Onyx, {OnyxEntry} from 'react-native-onyx';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import {Report} from '@src/types/onyx';
import ReportAction from '@src/types/onyx/ReportAction';
import * as CollectionUtils from './CollectionUtils';
import * as Localize from './Localize';

const allReports: Record<string, Report> = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
callback: (report, key) => {
if (!key || !report) {
return;
}
const reportID = CollectionUtils.extractCollectionItemID(key);
allReports[reportID] = report;
},
});
Comment on lines +9 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

This basically line-to-line duplicate of this block, and maybe others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. I once had a somewhat similar question, and this was the response from @tgolen:

I haven’t seen duplicate connect()s be a problem yet, so that’s what I would do.

I may not be aware, is there a way to reuse the Onyx.connect?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, maybe ReportUtils.getReportById or something like that? It seems werid to just copy-paste this because there "hasn't been a problem yet"...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tgolen what would you say? Can we reuse utils inside other utils instead of using Onyx.connect?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a very nuanced topic, so I apologize in advance for the long comment, but I want you to hear my thinking.

  1. There is no technical problem with having the same connect() in two different libs. The only "problem" is that it is repetitive code, which is a somewhat valid argument. However, let's take that logic all the way to the grand conclusion. If we only intend to have one connect() for every key, then that basically results in some kind of file that contains all the connect() calls for every key. This then creates an odd in-memory type store of everything that Onyx already has in memory. This is just a strange place to end up in.

  2. Having duplicate connect()s in the files that need them allows them to be custom-tailored for fast lookups or custom maps depending on what data is being accessed. For example, rather than doing a .filter() or .map() every time the data needs accessed, the connect() callback should store the data in an efficient way so that it can be easily and quickly accessed like reports[reportID] and there is no need to filter or map. This allows for code to be much more optimized, rather than kept in a very general and slow method (because it has to account for different libs wanting to access it in different ways).

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, this makes sense in terms of productivity vs clean code.
Based on the comment above, should we be good to go with this approach @cubuspl42?

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't make sense, at least to me.

The only "problem" is that it is repetitive code, which is a somewhat valid argument

Code duplication is an industry-standard problem with industry-standard solutions.

Even more precisely, even the duplication of data access code is industry-standard, this has been present since the old days of hand-crafted MPA DAOs written in PHP, Java, or whatever. The only twist is the data is reactive here, changing as the time flows.

We've discussed the "big picture", the general case, but coming down to our case, the only argument I can hear for not de-duplicating this is "this might be optimized in the future, so let's keep the duplication", which falls into the YAGNI category for me.

Having said all this, it's not a blocker for me, as I'm kind of busy and can't afford discussing this too much.


/**
* Given the Task reportAction name, return the appropriate message to be displayed and copied to clipboard.
*/
function getTaskReportActionMessage(actionName: string): string {
switch (actionName) {
case CONST.REPORT.ACTIONS.TYPE.TASKCOMPLETED:
return Localize.translateLocal('task.messages.completed');
case CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED:
return Localize.translateLocal('task.messages.canceled');
case CONST.REPORT.ACTIONS.TYPE.TASKREOPENED:
return Localize.translateLocal('task.messages.reopened');
default:
return Localize.translateLocal('task.task');
}
}

function getTaskTitle(taskReportID: string, fallbackTitle = ''): string {
const taskReport = allReports[taskReportID] ?? {};
// We need to check for reportID, not just reportName, because when a receiver opens the task for the first time,
// an optimistic report is created with the only property – reportName: 'Chat report',
// and it will be displayed as the task title without checking for reportID to be present.
return Object.hasOwn(taskReport, 'reportID') && taskReport.reportName ? taskReport.reportName : fallbackTitle;
}

function getTaskCreatedMessage(reportAction: OnyxEntry<ReportAction>) {
const taskReportID = reportAction?.childReportID ?? '';
const taskTitle = getTaskTitle(taskReportID, reportAction?.childReportName);
return Localize.translateLocal('task.messages.created', {title: taskTitle});
}
Comment on lines +45 to +49
Copy link
Contributor

Choose a reason for hiding this comment

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

If taskTitle is falsy we should have returned an empty string as it does not make sense to return task create message with no task title i.e. task for .

Returning empty string enables other other parts of the app to detect failure in getting task created message so we can fallback to other messages.

(Coming from #35002)


export {getTaskReportActionMessage, getTaskTitle, getTaskCreatedMessage};
31 changes: 0 additions & 31 deletions src/libs/actions/Task.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import * as API from '@libs/API';
import DateUtils from '@libs/DateUtils';
import * as ErrorUtils from '@libs/ErrorUtils';
import * as LocalePhoneNumber from '@libs/LocalePhoneNumber';
import * as Localize from '@libs/Localize';
import Navigation from '@libs/Navigation/Navigation';
import * as OptionsListUtils from '@libs/OptionsListUtils';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
Expand Down Expand Up @@ -906,35 +905,6 @@ function clearTaskErrors(reportID) {
});
}

/**
* @param {string} actionName
* @param {string} reportID
* @param {boolean} isCreateTaskAction
* @returns {string}
*/
function getTaskReportActionMessage(actionName, reportID, isCreateTaskAction) {
const report = ReportUtils.getReport(reportID);
if (isCreateTaskAction) {
return `task for ${report.reportName}`;
}
let taskStatusText = '';
switch (actionName) {
case CONST.REPORT.ACTIONS.TYPE.TASKCOMPLETED:
taskStatusText = Localize.translateLocal('task.messages.completed');
break;
case CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED:
taskStatusText = Localize.translateLocal('task.messages.canceled');
break;
case CONST.REPORT.ACTIONS.TYPE.TASKREOPENED:
taskStatusText = Localize.translateLocal('task.messages.reopened');
break;
default:
taskStatusText = Localize.translateLocal('task.task');
}

return `${taskStatusText}`;
}

export {
createTaskAndNavigate,
editTask,
Expand All @@ -956,5 +926,4 @@ export {
getTaskAssigneeAccountID,
clearTaskErrors,
canModifyTask,
getTaskReportActionMessage,
};
10 changes: 6 additions & 4 deletions src/pages/home/report/ContextMenu/ContextMenuActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils';
import ReportActionComposeFocusManager from '@libs/ReportActionComposeFocusManager';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import * as ReportUtils from '@libs/ReportUtils';
import * as TaskUtils from '@libs/TaskUtils';
import * as Download from '@userActions/Download';
import * as Report from '@userActions/Report';
import * as Task from '@userActions/Task';
Expand Down Expand Up @@ -258,15 +259,13 @@ export default [
type === CONTEXT_MENU_TYPES.REPORT_ACTION && !ReportActionsUtils.isReportActionAttachment(reportAction) && !ReportActionsUtils.isMessageDeleted(reportAction),

// If return value is true, we switch the `text` and `icon` on
// `ContextMenuItem` with `successText` and `successIcon` which will fallback to
// `ContextMenuItem` with `successText` and `successIcon` which will fall back to
// the `text` and `icon`
onPress: (closePopover, {reportAction, selection}) => {
const isTaskAction = ReportActionsUtils.isTaskAction(reportAction);
const isCreateTaskAction = ReportActionsUtils.isCreatedTaskReportAction(reportAction);
const isReportPreviewAction = ReportActionsUtils.isReportPreviewAction(reportAction);
const message = _.last(lodashGet(reportAction, 'message', [{}]));
const reportID = lodashGet(reportAction, 'originalMessage.taskReportID', '').toString();
const messageHtml = isTaskAction || isCreateTaskAction ? Task.getTaskReportActionMessage(reportAction.actionName, reportID, isCreateTaskAction) : lodashGet(message, 'html', '');
const messageHtml = isTaskAction ? Task.getTaskReportActionMessage(reportAction) : lodashGet(message, 'html', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, it looks like this method was moved to TaskUtils but this reference wasn't updated. It results in the following webpack warning on main and it will probably cause the app to crash:

[web-server] WARNING in ./src/pages/home/report/ContextMenu/ContextMenuActions.js 277:37-68
[web-server] export 'getTaskReportActionMessage' (imported as 'Task') was not found in '@userActions/Task' (possible exports: canModifyTask, cancelTask, clearOutTaskInfo, clearOutTaskInfoAndNavigate, clearTaskErrors, completeTask, createTaskAndNavigate, dismissModalAndClearOutTaskInfo, editTask, editTaskAssignee, getAssignee, getShareDestination, getTaskAssigneeAccountID, reopenTask, setAssigneeValue, setDescriptionValue, setDetailsValue, setShareDestinationValue, setTaskReport, setTitleValue)
[web-server]  @ ./src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js 24:0-78 39:8-40 65:44-62
[web-server]  @ ./src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js 11:0-72 311:38-65
[web-server]  @ ./src/Expensify.js 43:0-108 209:39-69
[web-server]  @ ./src/App.js 23:0-36 55:97-106
[web-server]  @ ./index.js 6:0-28 11:9-12


const isAttachment = ReportActionsUtils.isReportActionAttachment(reportAction);
if (!isAttachment) {
Expand All @@ -281,6 +280,9 @@ export default [
} else if (ReportActionsUtils.isMoneyRequestAction(reportAction)) {
const displayMessage = ReportUtils.getIOUReportActionDisplayMessage(reportAction);
Clipboard.setString(displayMessage);
} else if (ReportActionsUtils.isCreatedTaskReportAction(reportAction)) {
const taskPreviewMessage = TaskUtils.getTaskCreatedMessage(reportAction);
Clipboard.setString(taskPreviewMessage);
} else if (ReportActionsUtils.isChannelLogMemberAction(reportAction)) {
const logMessage = ReportUtils.getChannelLogMemberMessage(reportAction);
Clipboard.setString(logMessage);
Expand Down
Loading