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

[TS migration] Migrate Taskpreview.js component to TypeScript #33852

Merged
merged 20 commits into from
Jan 11, 2024
Merged
Changes from 9 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
Original file line number Diff line number Diff line change
@@ -1,19 +1,16 @@
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import Str from 'expensify-common/lib/str';
import React from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import {OnyxEntry, withOnyx} from 'react-native-onyx';
import Checkbox from '@components/Checkbox';
import Icon from '@components/Icon';
import * as Expensicons from '@components/Icon/Expensicons';
import {usePersonalDetails} from '@components/OnyxProvider';
import PressableWithoutFeedback from '@components/Pressable/PressableWithoutFeedback';
import refPropTypes from '@components/refPropTypes';
import RenderHTML from '@components/RenderHTML';
import {showContextMenuForReport} from '@components/ShowContextMenuContext';
import withCurrentUserPersonalDetails, {withCurrentUserPersonalDetailsDefaultProps, withCurrentUserPersonalDetailsPropTypes} from '@components/withCurrentUserPersonalDetails';
import withLocalize, {withLocalizePropTypes} from '@components/withLocalize';
import withCurrentUserPersonalDetails, {WithCurrentUserPersonalDetailsProps} from '@components/withCurrentUserPersonalDetails';
import useLocalize from '@hooks/useLocalize';
import useStyleUtils from '@hooks/useStyleUtils';
import useThemeStyles from '@hooks/useThemeStyles';
import compose from '@libs/compose';
Expand All @@ -24,141 +21,135 @@ 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 Session from '@userActions/Session';
import * as Task from '@userActions/Task';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type {Policy, Report, ReportAction} from '@src/types/onyx';
import {isNotEmptyObject} from '@src/types/utils/EmptyObject';

const propTypes = {
/** The ID of the associated taskReport */
taskReportID: PropTypes.string.isRequired,

/** Whether the task preview is hovered so we can modify its style */
isHovered: PropTypes.bool,

/** The linked reportAction */
action: PropTypes.shape(reportActionPropTypes).isRequired,
type PolicyRole = {
role: string;
};
Copy link
Contributor

@tgolen tgolen Jan 5, 2024

Choose a reason for hiding this comment

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

It looks like some comments are getting dropped. I find the comments very helpful, is it a standard process to remove them? I'd like to keep them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tgolen We preserve all comments, and sometimes we even add new ones for better clarity :)


type TaskPreviewOnyxProps = {
/* Onyx Props */

taskReport: PropTypes.shape({
/** Title of the task */
reportName: PropTypes.string,

/** AccountID of the manager in this iou report */
managerID: PropTypes.number,

/** AccountID of the creator of this iou report */
ownerAccountID: PropTypes.number,
}),
taskReport: OnyxEntry<Report>;

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment for this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

/** The policy of root parent report */
rootParentReportpolicy: PropTypes.shape({
/** The role of current user */
role: PropTypes.string,
}),

/** The chat report associated with taskReport */
chatReportID: PropTypes.string.isRequired,

/** Popover context menu anchor, used for showing context menu */
contextMenuAnchor: refPropTypes,

/** Callback for updating context menu active state, used for showing context menu */
checkIfContextMenuActive: PropTypes.func,

/* Onyx Props */
...withLocalizePropTypes,

...withCurrentUserPersonalDetailsPropTypes,
rootParentReportpolicy: OnyxEntry<PolicyRole>;
};

const defaultProps = {
...withCurrentUserPersonalDetailsDefaultProps,
taskReport: {},
rootParentReportpolicy: {},
isHovered: false,
};

function TaskPreview(props) {
type TaskPreviewProps = WithCurrentUserPersonalDetailsProps &
TaskPreviewOnyxProps & {
/** The ID of the associated policy */
// eslint-disable-next-line
policyID: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

why this is needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a lint error that shows policyId is defined but not used anywhere so I added it.

Copy link
Contributor

Choose a reason for hiding this comment

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

A few things:

  • If the prop is not used anywhere, remove it
  • If the eslint-disable-next-line needs to stay, disable the specific rule instead of all rules and also add a code comment for why the prop needs to be there even though it isn't used anywhere

I believe the reason policyID is here is because it is used in withOnyx to fetch rootParentReportpolicy. Since ESLint won't detect it's usage in withOnyx, that's why it throws the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please do one of the options I posted in my previous comment, I don't see that either of them were followed.

/** The ID of the associated taskReport */
taskReportID: string;

/** Whether the task preview is hovered so we can modify its style */
isHovered: boolean;

/** The linked reportAction */
action: OnyxEntry<ReportAction>;

/** The chat report associated with taskReport */
chatReportID: string;

/** Popover context menu anchor, used for showing context menu */
contextMenuAnchor: Element;

/** Callback for updating context menu active state, used for showing context menu */
checkIfContextMenuActive: () => void;
};

function TaskPreview({
taskReport,
taskReportID,
action,
contextMenuAnchor,
Copy link
Contributor

Choose a reason for hiding this comment

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

@blazejkustra Is there a general rule about when to use destructuring with defaults and when not to? It feels to me like it would make the code easier to follow if we used defaults in the destructuring. The only case I think it won't work is when you need to know if the value was undefined or null, but a simple fasly check like action ?? {} should be fine, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so may I update the code to use default value in destructuring if possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

The rule of thumb is to avoid unnecessary default values such as undefined, null, {}, [], '' and () => {} - when we started migration we left some of these, but we can clean them as a follow up once whole TS migration is finished.

Two examples for useful default values:

function Avatar({
    size = CONST.AVATAR_SIZE.DEFAULT,
    fallbackIcon = Expensicons.FallbackAvatar,
    type = CONST.ICON_TYPE_AVATAR,
}: AvatarProps) {}

function GenericPressable(
    {
        shouldUseHapticsOnLongPress = false,
        shouldUseHapticsOnPress = false,
        shouldUseAutoHitSlop = false,
        enableInScreenReaderStates = CONST.SCREEN_READER_STATES.ALL,
        accessible = true,
        ...rest
    }: PressableProps) {}

chatReportID,
checkIfContextMenuActive,
isHovered,
currentUserPersonalDetails,
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing default value here:

Suggested change
isHovered,
isHovered = false,

rootParentReportpolicy,
}: TaskPreviewProps) {
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const personalDetails = usePersonalDetails() || CONST.EMPTY_OBJECT;
const {translate} = useLocalize();
// The reportAction might not contain details regarding the taskReport
// Only the direct parent reportAction will contain details about the taskReport
// Other linked reportActions will only contain the taskReportID and we will grab the details from there
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 = _.escape(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'], '');
const isTaskCompleted = isNotEmptyObject(taskReport)
? taskReport?.stateNum === CONST.REPORT.STATE_NUM.SUBMITTED && taskReport.statusNum === CONST.REPORT.STATUS.APPROVED
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to deprecate isNotEmptyObject, let's use !isEmptyObject instead

: action?.childStateNum === CONST.REPORT.STATE_NUM.SUBMITTED && action?.childStatusNum === CONST.REPORT.STATUS.APPROVED;
const taskTitle = Str.htmlEncode(TaskUtils.getTaskTitle(taskReportID, action?.childReportName ?? ''));
const taskAssigneeAccountID = Task.getTaskAssigneeAccountID(taskReport ?? {}) ?? action?.childManagerAccountID ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure these implementations give the same result:

    const taskTitle = _.escape(TaskUtils.getTaskTitle(props.taskReportID, props.action.childReportName));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

image
Same result.

const assigneeLogin = personalDetails[taskAssigneeAccountID]?.login ?? '';
const assigneeDisplayName = personalDetails[taskAssigneeAccountID]?.displayName ?? '';
const taskAssignee = assigneeDisplayName || LocalePhoneNumber.formatPhoneNumber(assigneeLogin);
const htmlForTaskPreview =
taskAssignee && taskAssigneeAccountID !== 0
? `<comment><mention-user accountid="${taskAssigneeAccountID}">@${taskAssignee}</mention-user> ${taskTitle}</comment>`
: `<comment>${taskTitle}</comment>`;
const isDeletedParentAction = ReportUtils.isCanceledTaskReport(props.taskReport, props.action);
const isDeletedParentAction = ReportUtils.isCanceledTaskReport(taskReport, action);

if (isDeletedParentAction) {
return <RenderHTML html={`<comment>${props.translate('parentReportAction.deletedTask')}</comment>`} />;
return <RenderHTML html={`<comment>${translate('parentReportAction.deletedTask')}</comment>`} />;
}

return (
<View style={[styles.chatItemMessage]}>
<PressableWithoutFeedback
onPress={() => Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(props.taskReportID))}
onPress={() => Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(taskReportID))}
onPressIn={() => DeviceCapabilities.canUseTouchScreen() && ControlSelection.block()}
onPressOut={() => ControlSelection.unblock()}
onLongPress={(event) => showContextMenuForReport(event, props.contextMenuAnchor, props.chatReportID, props.action, props.checkIfContextMenuActive)}
onLongPress={(event) => showContextMenuForReport(event, contextMenuAnchor, chatReportID, action ?? {}, checkIfContextMenuActive)}
style={[styles.flexRow, styles.justifyContentBetween]}
role={CONST.ROLE.BUTTON}
accessibilityLabel={props.translate('task.task')}
accessibilityLabel={translate('task.task')}
>
<View style={[styles.flex1, styles.flexRow, styles.alignItemsStart]}>
<Checkbox
style={[styles.mr2]}
containerStyle={[styles.taskCheckbox]}
isChecked={isTaskCompleted}
disabled={!Task.canModifyTask(props.taskReport, props.currentUserPersonalDetails.accountID, lodashGet(props.rootParentReportpolicy, 'role', ''))}
disabled={!Task.canModifyTask(taskReport ?? {}, currentUserPersonalDetails.accountID, rootParentReportpolicy ? rootParentReportpolicy.role : '')}
onPress={Session.checkIfActionIsAllowed(() => {
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
disabled={!Task.canModifyTask(taskReport ?? {}, currentUserPersonalDetails.accountID, rootParentReportpolicy ? rootParentReportpolicy.role : '')}
disabled={!Task.canModifyTask(taskReport ?? {}, currentUserPersonalDetails.accountID, rootParentReportpolicy?.role ?? '')}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just applied.

if (isTaskCompleted) {
Task.reopenTask(props.taskReport);
Task.reopenTask(taskReport ?? {});
} else {
Task.completeTask(props.taskReport);
Task.completeTask(taskReport ?? {});
}
})}
accessibilityLabel={props.translate('task.task')}
accessibilityLabel={translate('task.task')}
/>
<RenderHTML html={htmlForTaskPreview} />
</View>
<Icon
src={Expensicons.ArrowRight}
fill={StyleUtils.getIconFillColor(getButtonState(props.isHovered))}
fill={StyleUtils.getIconFillColor(getButtonState(isHovered))}
/>
</PressableWithoutFeedback>
</View>
);
}

TaskPreview.propTypes = propTypes;
TaskPreview.defaultProps = defaultProps;
TaskPreview.displayName = 'TaskPreview';

export default compose(
withLocalize,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please refer to TS guidelines, unfortunately we have to remove compose here, as it isn't working as expected. link

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just updated.

withCurrentUserPersonalDetails,
withOnyx({
withOnyx<TaskPreviewProps, TaskPreviewOnyxProps>({
taskReport: {
key: ({taskReportID}) => `${ONYXKEYS.COLLECTION.REPORT}${taskReportID}`,
initialValue: {},
},
rootParentReportpolicy: {
key: ({policyID}) => `${ONYXKEYS.COLLECTION.POLICY}${policyID || '0'}`,
selector: (policy) => _.pick(policy, ['role']),
key: ({policyID}) => `${ONYXKEYS.COLLECTION.POLICY}${policyID ?? '0'}`,
selector: (policy: Policy | null) => ({role: policy?.role ?? ''}),
},
}),
withCurrentUserPersonalDetails,
)(TaskPreview);
Loading