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

Jump to bottom

Load parent report action from withOnyx in ReportActionItem #34057

Merged
merged 4 commits into from
Jan 9, 2024
Merged

Load parent report action from withOnyx in ReportActionItem #34057

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
33c40c5
Load parent report action from withOnyx in ReportActionItem
tgolen Jan 5, 2024
e3758e0
Update src/pages/home/report/ReportActionItem.js
tgolen Jan 8, 2024
1470c83
Merge branch 'main' into tgolen-refactor-reportactionitemremovesidelo…
tgolen Jan 8, 2024
c89290b
Remove unnecessary ternary
tgolen Jan 8, 2024
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/pages/home/report/ReportActionItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ const propTypes = {

/** The user's wallet account */
userWallet: userWalletPropTypes,

/** All the report action belonging the report's parent */
tgolen marked this conversation as resolved.
Show resolved Hide resolved
parentReportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)),
};

const defaultProps = {
Expand All @@ -127,6 +130,7 @@ const defaultProps = {
iouReport: undefined,
shouldHideThreadDividerLine: false,
userWallet: {},
parentReportActions: {},
};

function ReportActionItem(props) {
Expand Down Expand Up @@ -569,7 +573,7 @@ function ReportActionItem(props) {
};

if (props.action.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) {
const parentReportAction = ReportActionsUtils.getParentReportAction(props.report);
const parentReportAction = props.parentReportActions[props.report.parentReportActionID];
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we will have a report? I assume yes. But it's a little ambiguous because you have a conditional on line 776 that does:

 key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : 0}`,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proptypes declare that report is required, as well as in both of the parents that use this component, so I am confident in expecting report to always be there (though I know it's not a guarantee). The only reason there is a conditional on 776 is because I copied it from other components. I'll remove the conditional down there.

if (ReportActionsUtils.isTransactionThread(parentReportAction)) {
return (
<ShowContextMenuContext.Provider value={contextValue}>
Expand Down Expand Up @@ -768,6 +772,10 @@ export default compose(
userWallet: {
key: ONYXKEYS.USER_WALLET,
},
parentReportActions: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a little extra context, I did look into subscribing to this higher up in the component tree, but it got pretty convoluted and went all the way up to ReportScreen. Interestingly enough, I have this PR where ReportScreen is now subscribing to the parent report action... so if we really wanted to, we could pass the parentReportAction all the way down as a prop. It would be a bigger change, but it might have less of a possible performance impact on this component (which looks to be highly optimized).

key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : 0}`,
canEvict: false,
},
}),
)(
memo(
Expand All @@ -785,6 +793,8 @@ export default compose(
_.isEqual(prevProps.report.errorFields, nextProps.report.errorFields) &&
lodashGet(prevProps.report, 'statusNum') === lodashGet(nextProps.report, 'statusNum') &&
lodashGet(prevProps.report, 'stateNum') === lodashGet(nextProps.report, 'stateNum') &&
lodashGet(prevProps.report, 'parentReportID') === lodashGet(nextProps.report, 'parentReportID') &&
lodashGet(prevProps.report, 'parentReportActionID') === lodashGet(nextProps.report, 'parentReportActionID') &&
prevProps.translate === nextProps.translate &&
// TaskReport's created actions render the TaskView, which updates depending on certain fields in the TaskReport
ReportUtils.isTaskReport(prevProps.report) === ReportUtils.isTaskReport(nextProps.report) &&
Expand Down
Loading