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

fix: Make all task titles/descriptions read-only to all but the author/assignee. #53749

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
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
10 changes: 7 additions & 3 deletions src/components/ReportActionItem/TaskPreview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ function TaskPreview({taskReportID, action, contextMenuAnchor, chatReportID, che
: action?.childStateNum === CONST.REPORT.STATE_NUM.APPROVED && action?.childStatusNum === CONST.REPORT.STATUS_NUM.APPROVED;
const taskTitle = Str.htmlEncode(TaskUtils.getTaskTitleFromReport(taskReport, action?.childReportName ?? ''));
const taskAssigneeAccountID = Task.getTaskAssigneeAccountID(taskReport) ?? action?.childManagerAccountID ?? -1;
const taskOwnerAccountID = taskReport?.ownerAccountID ?? action?.actorAccountID;
const hasAssignee = taskAssigneeAccountID > 0;
const personalDetails = usePersonalDetails();
const avatar = personalDetails?.[taskAssigneeAccountID]?.avatar ?? Expensicons.FallbackAvatar;
Expand Down Expand Up @@ -106,12 +107,15 @@ function TaskPreview({taskReportID, action, contextMenuAnchor, chatReportID, che
<Checkbox
style={[styles.mr2]}
isChecked={isTaskCompleted}
disabled={!Task.canModifyTask(taskReport, currentUserPersonalDetails.accountID) || !Task.canActionTask(taskReport, currentUserPersonalDetails.accountID)}
disabled={
!Task.canModifyTask(taskReport, currentUserPersonalDetails.accountID, true, taskOwnerAccountID, taskAssigneeAccountID) ||
!Task.canActionTask(taskReport, currentUserPersonalDetails.accountID, taskOwnerAccountID, taskAssigneeAccountID)
}
onPress={Session.checkIfActionIsAllowed(() => {
if (isTaskCompleted) {
Task.reopenTask(taskReport);
Task.reopenTask(taskReport, taskReportID);
} else {
Task.completeTask(taskReport);
Task.completeTask(taskReport, taskReportID);
}
})}
accessibilityLabel={translate('task.task')}
Expand Down
27 changes: 15 additions & 12 deletions src/components/ReportActionItem/TaskView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ import OfflineWithFeedback from '@components/OfflineWithFeedback';
import {usePersonalDetails} from '@components/OnyxProvider';
import PressableWithSecondaryInteraction from '@components/PressableWithSecondaryInteraction';
import Text from '@components/Text';
import withCurrentUserPersonalDetails from '@components/withCurrentUserPersonalDetails';
import type {WithCurrentUserPersonalDetailsProps} from '@components/withCurrentUserPersonalDetails';
import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails';
import useLocalize from '@hooks/useLocalize';
import useStyleUtils from '@hooks/useStyleUtils';
import useThemeStyles from '@hooks/useThemeStyles';
Expand All @@ -27,27 +26,28 @@ import CONST from '@src/CONST';
import ROUTES from '@src/ROUTES';
import type {Report} from '@src/types/onyx';

type TaskViewProps = WithCurrentUserPersonalDetailsProps & {
type TaskViewProps = {
/** The report currently being looked at */
report: Report;
};

function TaskView({report, ...props}: TaskViewProps) {
function TaskView({report}: TaskViewProps) {
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const currentUserPersonalDetails = useCurrentUserPersonalDetails();
const personalDetails = usePersonalDetails();
useEffect(() => {
Task.setTaskReport(report);
}, [report]);
const personalDetails = usePersonalDetails();
const taskTitle = convertToLTR(report.reportName ?? '');
const assigneeTooltipDetails = ReportUtils.getDisplayNamesWithTooltips(
OptionsListUtils.getPersonalDetailsForAccountIDs(report.managerID ? [report.managerID] : [], personalDetails),
false,
);
const isCompleted = ReportUtils.isCompletedTaskReport(report);
const isOpen = ReportUtils.isOpenTaskReport(report);
const canModifyTask = Task.canModifyTask(report, props.currentUserPersonalDetails.accountID);
const canActionTask = Task.canActionTask(report, props.currentUserPersonalDetails.accountID);
const isCompleted = ReportUtils.isCompletedTaskReport(report);
const canModifyTask = Task.canModifyTask(report, currentUserPersonalDetails.accountID);
const canActionTask = Task.canActionTask(report, currentUserPersonalDetails.accountID);
const disableState = !canModifyTask;
const isDisableInteractive = !canModifyTask || !isOpen;
const {translate} = useLocalize();
Expand Down Expand Up @@ -77,10 +77,10 @@ function TaskView({report, ...props}: TaskViewProps) {
styles.ph5,
styles.pv2,
StyleUtils.getButtonBackgroundColorStyle(getButtonState(hovered, pressed, false, disableState, !isDisableInteractive), true),
isDisableInteractive && !disableState && styles.cursorDefault,
isDisableInteractive && styles.cursorDefault,
]}
disabled={disableState}
accessibilityLabel={taskTitle || translate('task.task')}
disabled={isDisableInteractive}
>
{({pressed}) => (
<OfflineWithFeedback pendingAction={report.pendingFields?.reportName}>
Expand All @@ -104,7 +104,7 @@ function TaskView({report, ...props}: TaskViewProps) {
containerBorderRadius={8}
caretSize={16}
accessibilityLabel={taskTitle || translate('task.task')}
disabled={!canModifyTask || !canActionTask}
disabled={!canActionTask}
/>
<View style={[styles.flexRow, styles.flex1]}>
<Text
Expand Down Expand Up @@ -141,6 +141,7 @@ function TaskView({report, ...props}: TaskViewProps) {
shouldGreyOutWhenDisabled={false}
numberOfLinesTitle={0}
interactive={!isDisableInteractive}
shouldUseDefaultCursorWhenDisabled
/>
</OfflineWithFeedback>
<OfflineWithFeedback pendingAction={report.pendingFields?.managerID}>
Expand All @@ -160,6 +161,7 @@ function TaskView({report, ...props}: TaskViewProps) {
shouldGreyOutWhenDisabled={false}
interactive={!isDisableInteractive}
titleWithTooltips={assigneeTooltipDetails}
shouldUseDefaultCursorWhenDisabled
/>
) : (
<MenuItemWithTopDescription
Expand All @@ -170,6 +172,7 @@ function TaskView({report, ...props}: TaskViewProps) {
wrapperStyle={[styles.pv2]}
shouldGreyOutWhenDisabled={false}
interactive={!isDisableInteractive}
shouldUseDefaultCursorWhenDisabled
/>
)}
</OfflineWithFeedback>
Expand All @@ -180,4 +183,4 @@ function TaskView({report, ...props}: TaskViewProps) {

TaskView.displayName = 'TaskView';

export default withCurrentUserPersonalDetails(TaskView);
export default TaskView;
22 changes: 6 additions & 16 deletions src/components/TaskHeaderActionButton.tsx
Original file line number Diff line number Diff line change
@@ -1,30 +1,24 @@
import React from 'react';
import {View} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import {withOnyx} from 'react-native-onyx';
import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
import * as ReportUtils from '@libs/ReportUtils';
import * as TaskUtils from '@libs/TaskUtils';
import * as Session from '@userActions/Session';
import * as Task from '@userActions/Task';
import ONYXKEYS from '@src/ONYXKEYS';
import type * as OnyxTypes from '@src/types/onyx';
import Button from './Button';
import {useSession} from './OnyxProvider';

type TaskHeaderActionButtonOnyxProps = {
/** Current user session */
session: OnyxEntry<OnyxTypes.Session>;
};

type TaskHeaderActionButtonProps = TaskHeaderActionButtonOnyxProps & {
type TaskHeaderActionButtonProps = {
/** The report currently being looked at */
report: OnyxTypes.Report;
};

function TaskHeaderActionButton({report, session}: TaskHeaderActionButtonProps) {
function TaskHeaderActionButton({report}: TaskHeaderActionButtonProps) {
const {translate} = useLocalize();
const styles = useThemeStyles();
const session = useSession();

if (!ReportUtils.canWriteInReport(report)) {
return null;
Expand All @@ -34,7 +28,7 @@ function TaskHeaderActionButton({report, session}: TaskHeaderActionButtonProps)
<View style={[styles.flexRow, styles.alignItemsCenter, styles.justifyContentEnd]}>
<Button
success
isDisabled={!Task.canModifyTask(report, session?.accountID ?? -1) || !Task.canActionTask(report, session?.accountID ?? -1)}
isDisabled={!Task.canModifyTask(report, session?.accountID ?? -1, true) || !Task.canActionTask(report, session?.accountID ?? -1)}
text={translate(ReportUtils.isCompletedTaskReport(report) ? 'task.markAsIncomplete' : 'task.markAsComplete')}
onPress={Session.checkIfActionIsAllowed(() => {
// If we're already navigating to these task editing pages, early return not to mark as completed, otherwise we would have not found page.
Expand All @@ -55,8 +49,4 @@ function TaskHeaderActionButton({report, session}: TaskHeaderActionButtonProps)

TaskHeaderActionButton.displayName = 'TaskHeaderActionButton';

export default withOnyx<TaskHeaderActionButtonProps, TaskHeaderActionButtonOnyxProps>({
session: {
key: ONYXKEYS.SESSION,
},
})(TaskHeaderActionButton);
export default TaskHeaderActionButton;
32 changes: 23 additions & 9 deletions src/libs/actions/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -365,8 +365,8 @@ function getOutstandingChildTask(taskReport: OnyxEntry<OnyxTypes.Report>) {
/**
* Complete a task
*/
function completeTask(taskReport: OnyxEntry<OnyxTypes.Report>) {
const taskReportID = taskReport?.reportID ?? '-1';
function completeTask(taskReport: OnyxEntry<OnyxTypes.Report>, reportIDFromAction?: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function completeTask(taskReport: OnyxEntry<OnyxTypes.Report>, reportIDFromAction?: string) {
function completeTask(taskReport: OnyxEntry<OnyxTypes.Report>) {

Do we have a use case that requires using this parameter? If the change comes from a previous update, I think we can consider remove this prop (the same with the reopenTask function)

const taskReportID = taskReport?.reportID ?? reportIDFromAction ?? '-1';
const message = `marked as complete`;
const completedTaskReportAction = ReportUtils.buildOptimisticTaskReportAction(taskReportID, CONST.REPORT.ACTIONS.TYPE.TASK_COMPLETED, message);
const parentReport = getParentReport(taskReport);
Expand Down Expand Up @@ -450,8 +450,8 @@ function completeTask(taskReport: OnyxEntry<OnyxTypes.Report>) {
/**
* Reopen a closed task
*/
function reopenTask(taskReport: OnyxEntry<OnyxTypes.Report>) {
const taskReportID = taskReport?.reportID ?? '-1';
function reopenTask(taskReport: OnyxEntry<OnyxTypes.Report>, reportIDFromAction?: string) {
const taskReportID = taskReport?.reportID ?? reportIDFromAction ?? '-1';
const message = `marked as incomplete`;
const reopenedTaskReportAction = ReportUtils.buildOptimisticTaskReportAction(taskReportID, CONST.REPORT.ACTIONS.TYPE.TASK_REOPENED, message);
const parentReport = getParentReport(taskReport);
Expand Down Expand Up @@ -1216,7 +1216,19 @@ function getTaskOwnerAccountID(taskReport: OnyxEntry<OnyxTypes.Report>): number
/**
* Check if you're allowed to modify the task - anyone that has write access to the report can modify the task, except auditor
*/
function canModifyTask(taskReport: OnyxEntry<OnyxTypes.Report>, sessionAccountID: number): boolean {
function canModifyTask(
taskReport: OnyxEntry<OnyxTypes.Report>,
sessionAccountID: number,
allowEditingConciergeTask?: boolean,
taskOwnerAccountID?: number,
taskAssigneeAccountID?: number,
Comment on lines +1223 to +1224
Copy link
Contributor

@suneox suneox Dec 15, 2024

Choose a reason for hiding this comment

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

Suggested change
taskOwnerAccountID?: number,
taskAssigneeAccountID?: number,

Do we have a use case that requires using this parameter? If the change comes from a previous update, I think we can consider remove this prop. (the same with the canActionTask function)

): boolean {
const ownerAccountID = getTaskOwnerAccountID(taskReport) ?? taskOwnerAccountID;
const assigneeAccountID = getTaskAssigneeAccountID(taskReport) ?? taskAssigneeAccountID;
if (ownerAccountID === CONST.ACCOUNT_ID.CONCIERGE && !allowEditingConciergeTask) {
return false;
}

if (ReportUtils.isCanceledTaskReport(taskReport)) {
return false;
}
Expand All @@ -1227,7 +1239,7 @@ function canModifyTask(taskReport: OnyxEntry<OnyxTypes.Report>, sessionAccountID
return false;
}

if (sessionAccountID === getTaskOwnerAccountID(taskReport) || sessionAccountID === getTaskAssigneeAccountID(taskReport)) {
if (sessionAccountID === ownerAccountID || sessionAccountID === assigneeAccountID) {
return true;
}

Expand All @@ -1241,8 +1253,10 @@ function canModifyTask(taskReport: OnyxEntry<OnyxTypes.Report>, sessionAccountID
/**
* Check if you can change the status of the task (mark complete or incomplete). Only the task owner and task assignee can do this.
*/
function canActionTask(taskReport: OnyxEntry<OnyxTypes.Report>, sessionAccountID: number): boolean {
return sessionAccountID === getTaskOwnerAccountID(taskReport) || sessionAccountID === getTaskAssigneeAccountID(taskReport);
function canActionTask(taskReport: OnyxEntry<OnyxTypes.Report>, sessionAccountID: number, taskOwnerAccountID?: number, taskAssigneeAccountID?: number): boolean {
const ownerAccountID = getTaskOwnerAccountID(taskReport) ?? taskOwnerAccountID;
const assigneeAccountID = getTaskAssigneeAccountID(taskReport) ?? taskAssigneeAccountID;
return sessionAccountID === ownerAccountID || sessionAccountID === assigneeAccountID;
}

function clearTaskErrors(reportID: string) {
Expand Down Expand Up @@ -1286,9 +1300,9 @@ export {
getTaskAssigneeAccountID,
clearTaskErrors,
canModifyTask,
canActionTask,
setNewOptimisticAssignee,
getNavigationUrlOnTaskDelete,
canActionTask,
};

export type {PolicyValue, Assignee, ShareDestination};
6 changes: 5 additions & 1 deletion src/pages/home/HeaderView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import SubscriptAvatar from '@components/SubscriptAvatar';
import TaskHeaderActionButton from '@components/TaskHeaderActionButton';
import Text from '@components/Text';
import Tooltip from '@components/Tooltip';
import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails';
import useLocalize from '@hooks/useLocalize';
import usePolicy from '@hooks/usePolicy';
import useResponsiveLayout from '@hooks/useResponsiveLayout';
Expand Down Expand Up @@ -75,6 +76,7 @@ function HeaderView({report, parentReportAction, reportID, onNavigationMenuButto
const [parentReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${report?.parentReportID || report?.reportID || '-1'}`);
const policy = usePolicy(report?.policyID);
const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST);
const {accountID} = useCurrentUserPersonalDetails();

const {translate} = useLocalize();
const theme = useTheme();
Expand Down Expand Up @@ -288,7 +290,9 @@ function HeaderView({report, parentReportAction, reportID, onNavigationMenuButto
</PressableWithoutFeedback>
<View style={[styles.reportOptions, styles.flexRow, styles.alignItemsCenter]}>
{!shouldUseNarrowLayout && isChatUsedForOnboarding && freeTrialButton}
{isTaskReport && !shouldUseNarrowLayout && ReportUtils.isOpenTaskReport(report, parentReportAction) && <TaskHeaderActionButton report={report} />}
{isTaskReport && !shouldUseNarrowLayout && ReportUtils.isOpenTaskReport(report, parentReportAction) && Task.canActionTask(report, accountID) && (
<TaskHeaderActionButton report={report} />
)}
Comment on lines +293 to +295
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{isTaskReport && !shouldUseNarrowLayout && ReportUtils.isOpenTaskReport(report, parentReportAction) && Task.canActionTask(report, accountID) && (
<TaskHeaderActionButton report={report} />
)}
{isTaskReport && !shouldUseNarrowLayout && ReportUtils.isOpenTaskReport(report, parentReportAction) && <TaskHeaderActionButton report={report} />}

We should revert the condition checking canActionTask to display the TaskHeaderActionButton, as this component already includes logic to handle disabling based on canActionTask. If there is no use case to hide ActionButton please revert the same change on ReportScreen

{!isParentReportLoading && canJoin && !shouldUseNarrowLayout && joinButton}
</View>
{shouldDisplaySearchRouter && <SearchButton style={styles.ml2} />}
Expand Down
3 changes: 2 additions & 1 deletion src/pages/home/ReportScreen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import * as ValidationUtils from '@libs/ValidationUtils';
import type {AuthScreensParamList} from '@navigation/types';
import * as ComposerActions from '@userActions/Composer';
import * as Report from '@userActions/Report';
import * as Task from '@userActions/Task';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
Expand Down Expand Up @@ -784,7 +785,7 @@ function ReportScreen({route, currentReportID = '', navigation}: ReportScreenPro
needsOffscreenAlphaCompositing
>
{headerView}
{!!report && ReportUtils.isTaskReport(report) && shouldUseNarrowLayout && ReportUtils.isOpenTaskReport(report, parentReportAction) && (
{!!report && shouldUseNarrowLayout && ReportUtils.isOpenTaskReport(report, parentReportAction) && Task.canActionTask(report, currentUserAccountID) && (
<View style={[styles.borderBottom]}>
<View style={[styles.appBG, styles.pl0]}>
<View style={[styles.ph5, styles.pb3]}>
Expand Down
Loading