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 empty assignee in TaskHeader and update permission to mark Task complete/incomplete/cancel in Task report #20689

Merged
merged 16 commits into from
Jun 18, 2023
Merged
33 changes: 28 additions & 5 deletions src/components/TaskHeader.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, {useEffect} from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import lodashGet from 'lodash/get';
import {withOnyx} from 'react-native-onyx';
import reportPropTypes from '../pages/reportPropTypes';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
import * as ReportUtils from '../libs/ReportUtils';
Expand All @@ -22,6 +23,7 @@ import Button from './Button';
import * as TaskUtils from '../libs/actions/Task';
import * as UserUtils from '../libs/UserUtils';
import PressableWithFeedback from './Pressable/PressableWithFeedback';
import ONYXKEYS from '../ONYXKEYS';

const propTypes = {
/** The report currently being looked at */
Expand All @@ -30,13 +32,25 @@ const propTypes = {
/** Personal details so we can get the ones for the report participants */
personalDetails: PropTypes.objectOf(participantPropTypes).isRequired,

/** Current user session */
session: PropTypes.shape({
accountID: PropTypes.number,
}),

...withLocalizePropTypes,
};

const defaultProps = {
session: {
accountID: 0,
},
};

function TaskHeader(props) {
const title = ReportUtils.getReportName(props.report);
const assigneeName = ReportUtils.getDisplayNameForParticipant(props.report.managerID);
const assigneeAvatar = UserUtils.getAvatar(lodashGet(props.personalDetails, [props.report.managerID, 'avatar']), props.report.managerID);
const assigneeAccountID = TaskUtils.getTaskAssigneeAccountID(props.report);
const assigneeName = ReportUtils.getDisplayNameForParticipant(assigneeAccountID);
const assigneeAvatar = UserUtils.getAvatar(lodashGet(props.personalDetails, [assigneeAccountID, 'avatar']), assigneeAccountID);
const isOpen = props.report.stateNum === CONST.REPORT.STATE_NUM.OPEN && props.report.statusNum === CONST.REPORT.STATUS.OPEN;
const isCompleted = ReportUtils.isTaskCompleted(props.report);

Expand All @@ -59,7 +73,7 @@ function TaskHeader(props) {
>
<View style={[styles.flexRow, styles.alignItemsCenter, styles.justifyContentBetween, styles.pv3]}>
<View style={[styles.flexRow, styles.alignItemsCenter, styles.justifyContentBetween]}>
{props.report.managerID && props.report.managerID > 0 && (
{assigneeAccountID && assigneeAccountID > 0 && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Coming from #22398:
I understand this logic already existed but we should avoid this pattern and it's already in checklist.
This could have been fixed earlier while merging main.

I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.

<>
<Avatar
source={assigneeAvatar}
Expand Down Expand Up @@ -92,7 +106,7 @@ function TaskHeader(props) {
) : (
<Button
success
isDisabled={TaskUtils.isTaskCanceled(props.report)}
isDisabled={TaskUtils.isTaskCanceled(props.report) || !TaskUtils.isTaskAssigneeOrTaskOwner(props.report, props.session.accountID)}
medium
text={props.translate('newTaskPage.markAsDone')}
onPress={() => TaskUtils.completeTask(props.report.reportID, title)}
Expand Down Expand Up @@ -123,6 +137,15 @@ function TaskHeader(props) {
}

TaskHeader.propTypes = propTypes;
TaskHeader.defaultProps = defaultProps;
TaskHeader.displayName = 'TaskHeader';

export default compose(withWindowDimensions, withLocalize)(TaskHeader);
export default compose(
withWindowDimensions,
withLocalize,
withOnyx({
session: {
key: ONYXKEYS.SESSION,
},
}),
)(TaskHeader);
2 changes: 1 addition & 1 deletion src/libs/ReportActionsUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ function hasCommentThread(reportAction) {
}

/**
* Returns the parentReportAction if the given report is a thread.
* Returns the parentReportAction if the given report is a thread/task.
*
* @param {Object} report
* @returns {Object}
Expand Down
45 changes: 45 additions & 0 deletions src/libs/actions/Task.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import ROUTES from '../../ROUTES';
import CONST from '../../CONST';
import DateUtils from '../DateUtils';
import * as UserUtils from '../UserUtils';
import * as PersonalDetailsUtils from '../PersonalDetailsUtils';
import * as ReportActionsUtils from '../ReportActionsUtils';

/**
* Clears out the task info from the store
Expand Down Expand Up @@ -627,6 +629,47 @@ function dismissModalAndClearOutTaskInfo() {
clearOutTaskInfo();
}

/**
* Returns Task assignee accountID
*
* @param {Object} taskReport
* @returns {Number|null}
*/
function getTaskAssigneeAccountID(taskReport) {
if (!taskReport) {
return null;
}

if (taskReport.managerID) {
return taskReport.managerID;
}

const reportAction = ReportActionsUtils.getParentReportAction(taskReport);
const childManagerEmail = lodashGet(reportAction, 'childManagerEmail', '');
return PersonalDetailsUtils.getAccountIDsByLogins([childManagerEmail])[0];
}

/**
* Returns Task owner accountID
*
* @param {Object} taskReport
* @returns {Number|null}
*/
function getTaskOwnerAccountID(taskReport) {
return lodashGet(taskReport, 'ownerAccountID', null);
}

/**
* Check if current user is either task assignee or task owner
*
* @param {Object} taskReport
* @param {Number} sessionAccountID
* @returns {Boolean}
*/
function isTaskAssigneeOrTaskOwner(taskReport, sessionAccountID) {
return sessionAccountID === getTaskOwnerAccountID(taskReport) || sessionAccountID === getTaskAssigneeAccountID(taskReport);
}

export {
createTaskAndNavigate,
editTaskAndNavigate,
Expand All @@ -646,4 +689,6 @@ export {
cancelTask,
isTaskCanceled,
dismissModalAndClearOutTaskInfo,
getTaskAssigneeAccountID,
isTaskAssigneeOrTaskOwner,
};
18 changes: 15 additions & 3 deletions src/pages/home/HeaderView.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ const propTypes = {
guideCalendarLink: PropTypes.string,
}),

/** Current user session */
session: PropTypes.shape({
accountID: PropTypes.number,
}),

/** The report actions from the parent report */
// TO DO: Replace with HOC https://github.com/Expensify/App/issues/18769.
// eslint-disable-next-line react/no-unused-prop-types
Expand All @@ -68,6 +73,9 @@ const defaultProps = {
guideCalendarLink: null,
},
parentReport: {},
session: {
accountID: 0,
},
};

function HeaderView(props) {
Expand All @@ -91,7 +99,8 @@ function HeaderView(props) {
const shouldShowCallButton = (isConcierge && guideCalendarLink) || (!isAutomatedExpensifyAccount && !isTaskReport);
const threeDotMenuItems = [];
if (isTaskReport) {
if (props.report.stateNum === CONST.REPORT.STATE_NUM.OPEN && props.report.statusNum === CONST.REPORT.STATUS.OPEN) {
const isTaskAssigneeOrTaskOwner = Task.isTaskAssigneeOrTaskOwner(props.report, props.session.accountID);
if (props.report.stateNum === CONST.REPORT.STATE_NUM.OPEN && props.report.statusNum === CONST.REPORT.STATUS.OPEN && isTaskAssigneeOrTaskOwner) {
threeDotMenuItems.push({
icon: Expensicons.Checkmark,
text: props.translate('newTaskPage.markAsDone'),
Expand All @@ -100,7 +109,7 @@ function HeaderView(props) {
}

// Task is marked as completed
if (props.report.stateNum === CONST.REPORT.STATE_NUM.SUBMITTED && props.report.statusNum === CONST.REPORT.STATUS.APPROVED) {
if (props.report.stateNum === CONST.REPORT.STATE_NUM.SUBMITTED && props.report.statusNum === CONST.REPORT.STATUS.APPROVED && isTaskAssigneeOrTaskOwner) {
threeDotMenuItems.push({
icon: Expensicons.Checkmark,
text: props.translate('newTaskPage.markAsIncomplete'),
Expand All @@ -109,7 +118,7 @@ function HeaderView(props) {
}

// Task is not closed
if (props.report.stateNum !== CONST.REPORT.STATE_NUM.SUBMITTED && props.report.statusNum !== CONST.REPORT.STATUS.CLOSED) {
if (props.report.stateNum !== CONST.REPORT.STATE_NUM.SUBMITTED && props.report.statusNum !== CONST.REPORT.STATUS.CLOSED && isTaskAssigneeOrTaskOwner) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this one. Should we allow assignee to cancel the task?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so. If this is a flow we do not want in the future we can create another issue for it. But I believe an assignee should be able to cancel a task.

threeDotMenuItems.push({
icon: Expensicons.Trashcan,
text: props.translate('common.cancel'),
Expand Down Expand Up @@ -258,5 +267,8 @@ export default compose(
parentReport: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID || report.reportID}`,
},
session: {
key: ONYXKEYS.SESSION,
},
}),
)(HeaderView);