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

Migrate tasks to use childManagerAccountID #21781

Merged
merged 11 commits into from
Jul 6, 2023
Merged

Migrate tasks to use childManagerAccountID #21781

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0b00788
Update util function to use new reportAction prop
Beamanator Jun 28, 2023
36d2536
Get assignee login from assignee accountID
Beamanator Jun 28, 2023
a7381f4
Refactor prop types
Beamanator Jun 28, 2023
74f4a7a
Merge branch 'main' of github.com:Expensify/App into beaman-migrateTo…
Beamanator Jun 29, 2023
778891a
A bit of lint cleanup
Beamanator Jun 29, 2023
c8f63aa
Rename prop & add default
Beamanator Jun 29, 2023
e6c0e7f
Merge branch 'main' of github.com:Expensify/App into beaman-migrateTo…
Beamanator Jul 3, 2023
7583e4f
Merge branch 'main' of github.com:Expensify/App into beaman-migrateTo…
Beamanator Jul 4, 2023
a1ef3e5
Remove use of childManagerEmail
Beamanator Jul 4, 2023
a76e4a5
Merge branch 'main' of github.com:Expensify/App into beaman-migrateTo…
Beamanator Jul 6, 2023
87ca096
Get personal displayName if no login in personalDetails
Beamanator Jul 6, 2023
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
12 changes: 11 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 @@ -2,6 +2,7 @@ import React from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import lodashGet from 'lodash/get';
import compose from '../../libs/compose';
import styles from '../../styles/styles';
import ONYXKEYS from '../../ONYXKEYS';
Expand All @@ -18,8 +19,12 @@ import reportActionPropTypes from '../../pages/home/report/reportActionPropTypes
import * as TaskUtils from '../../libs/actions/Task';
import RenderHTML from '../RenderHTML';
import PressableWithoutFeedback from '../Pressable/PressableWithoutFeedback';
import personalDetailsPropType from '../../pages/personalDetailsPropType';

const propTypes = {
/** All personal details asssociated with user */
personalDetailsList: personalDetailsPropType,

/** The ID of the associated taskReport */
taskReportID: PropTypes.string.isRequired,

Expand All @@ -46,6 +51,7 @@ const propTypes = {
};

const defaultProps = {
personalDetailsList: {},
taskReport: {},
isHovered: false,
};
Expand All @@ -58,7 +64,8 @@ function TaskPreview(props) {
? 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 taskAssignee = props.taskReport.managerEmail || props.action.childManagerEmail;
const taskAssigneeAccountID = TaskUtils.getTaskAssigneeAccountID(props.taskReport);
Copy link
Contributor

Choose a reason for hiding this comment

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

props.action already has childManagerAccountID. Is there any problem using it directly?

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 don't think so - but since we were defaulting to managerEmail first, I thought it would be best if we default to managerID first as well, THEN use childManagerAccountID - and that logic exists in this function so I thought it would be better to reuse it, do you disagree?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok agree. But as I reported, we should fallback to original logic if report.managerID or action.childManagerAccountID doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"original logic" being props.taskReport.managerEmail || props.action.childManagerEmail;? We're trying to get rid of those :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

We didn't consider the case where the taskReport could be empty and hence it caused a regression here, because the task assignment didn't show an email address.

Copy link
Contributor Author

@Beamanator Beamanator Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

hmm if task report could be empty, would this have crashed before too? 🤔 (since we used to directly access props.taskReport.managerEmail)

const taskAssignee = lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'login'], '');
const htmlForTaskPreview = taskAssignee ? `<comment><mention-user>@${taskAssignee}</mention-user> ${taskTitle}</comment>` : `<comment>${taskTitle}</comment>`;

return (
Expand Down Expand Up @@ -103,5 +110,8 @@ export default compose(
taskReport: {
key: ({taskReportID}) => `${ONYXKEYS.COLLECTION.REPORT}${taskReportID}`,
},
personalDetailsList: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
}),
)(TaskPreview);
1 change: 0 additions & 1 deletion src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1277,7 +1277,6 @@ function buildOptimisticTaskCommentReportAction(taskReportID, taskTitle, taskAss
reportAction.reportAction.parentReportID = parentReportID;
reportAction.reportAction.childType = CONST.REPORT.TYPE.TASK;
reportAction.reportAction.childReportName = taskTitle;
reportAction.reportAction.childManagerEmail = taskAssignee;
reportAction.reportAction.childManagerAccountID = taskAssigneeAccountID;
reportAction.reportAction.childStatusNum = CONST.REPORT.STATUS.OPEN;
reportAction.reportAction.childStateNum = CONST.REPORT.STATE_NUM.OPEN;
Expand Down
9 changes: 1 addition & 8 deletions src/libs/actions/Task.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ 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';

/**
Expand Down Expand Up @@ -648,13 +647,7 @@ function getTaskAssigneeAccountID(taskReport) {
}

const reportAction = ReportActionsUtils.getParentReportAction(taskReport);
const childManagerEmail = lodashGet(reportAction, 'childManagerEmail', '');

if (!childManagerEmail) {
return null;
}

return PersonalDetailsUtils.getAccountIDsByLogins([childManagerEmail])[0];
return lodashGet(reportAction, 'childManagerAccountID');
}

/**
Expand Down
6 changes: 4 additions & 2 deletions src/pages/workspace/WorkspaceMembersPage.js
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noticed this needed to be updated, maybe it belongs in a different PR? 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

What's this file change for? I don't see any lint issue in latest main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can see I'm just including extra prop types / default prop types that were missing. We used withCurrentUserPersonalDetails already, but didn't declare the prop types.

Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,15 @@ import * as UserUtils from '../../libs/UserUtils';
import FormHelpMessage from '../../components/FormHelpMessage';
import TextInput from '../../components/TextInput';
import KeyboardDismissingFlatList from '../../components/KeyboardDismissingFlatList';
import withCurrentUserPersonalDetails from '../../components/withCurrentUserPersonalDetails';
import withCurrentUserPersonalDetails, {withCurrentUserPersonalDetailsPropTypes, withCurrentUserPersonalDetailsDefaultProps} from '../../components/withCurrentUserPersonalDetails';
import * as PolicyUtils from '../../libs/PolicyUtils';
import PressableWithFeedback from '../../components/Pressable/PressableWithFeedback';
import usePrevious from '../../hooks/usePrevious';
import Log from '../../libs/Log';
import * as PersonalDetailsUtils from '../../libs/PersonalDetailsUtils';

const propTypes = {
/** The personal details of the person who is logged in */
/** All personal details asssociated with user */
personalDetails: personalDetailsPropType,

/** URL Route params */
Expand All @@ -61,6 +61,7 @@ const propTypes = {
...policyPropTypes,
...withLocalizePropTypes,
...windowDimensionsPropTypes,
...withCurrentUserPersonalDetailsPropTypes,
network: networkPropTypes.isRequired,
};

Expand All @@ -70,6 +71,7 @@ const defaultProps = {
accountID: 0,
},
...policyDefaultProps,
...withCurrentUserPersonalDetailsDefaultProps,
};

function WorkspaceMembersPage(props) {
Expand Down