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

Changes from 2 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
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 @@ -3,6 +3,7 @@ import {View} from 'react-native';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import lodashGet from 'lodash/get';
import _ from 'underscore';
import compose from '../../libs/compose';
import styles from '../../styles/styles';
import ONYXKEYS from '../../ONYXKEYS';
Expand Down Expand Up @@ -65,7 +66,7 @@ function TaskPreview(props) {
: props.action.childStateNum === CONST.REPORT.STATE_NUM.SUBMITTED && props.action.childStatusNum === CONST.REPORT.STATUS.APPROVED;
const taskTitle = props.taskReport.reportName || props.action.childReportName;
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 taskAssignee = lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'login'], lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'displayName'], ''));
Copy link
Contributor

Choose a reason for hiding this comment

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

personally not crazy about this nested lodash, would rather see something like:

Suggested change
const taskAssignee = lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'login'], lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'displayName'], ''));
const assigneeLogin = lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'login'], '');
const assigneeDisplayName = lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'displayName'], '');
const taskAssignee = assigneeLogin || assigneeDisplayName;

as it's more clear to me that displayName is a fall back if we don't have a login. instead of trying to reason how the 2nd lodashGet might get called and be a fallback for the first call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh that's totally fair - I'll try to remember to clean this up later - i can see it being useful but not highest priority at the moment

Copy link
Contributor

Choose a reason for hiding this comment

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

Cleanup PR here: #22439

const htmlForTaskPreview = taskAssignee ? `<comment><mention-user>@${taskAssignee}</mention-user> ${taskTitle}</comment>` : `<comment>${taskTitle}</comment>`;

return (
Expand Down