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: Unmatched parent report ID and report action ID #20574

Merged
Merged
Show file tree
Hide file tree
Changes from 10 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
4 changes: 3 additions & 1 deletion src/components/ShowContextMenuContext.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@ ShowContextMenuContext.displayName = 'ShowContextMenuContext';
* @param {Object} action - ReportAction for ContextMenu
* @param {Function} checkIfContextMenuActive Callback to update context menu active state
* @param {Boolean} [isArchivedRoom=false] - Is the report an archived room
* @param {Boolean} [isParentReport=false] - Is the report action from parent report
*/
function showContextMenuForReport(event, anchor, reportID, action, checkIfContextMenuActive, isArchivedRoom = false) {
function showContextMenuForReport(event, anchor, reportID, action, checkIfContextMenuActive, isArchivedRoom = false, isParentReport = false) {
if (!DeviceCapabilities.canUseTouchScreen()) {
return;
}
Expand All @@ -37,6 +38,7 @@ function showContextMenuForReport(event, anchor, reportID, action, checkIfContex
checkIfContextMenuActive,
checkIfContextMenuActive,
isArchivedRoom,
isParentReport,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this param passed anywhere? showContextMenuForReport is called from various places.

Copy link
Contributor

Choose a reason for hiding this comment

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

i.e. In BasePreRenderer onLongPress event, isParentReport is not passed at all.

Screen.Recording.2023-06-21.at.6.58.34.PM.mov

I started concerning about this approach of passing isParentReport

                {parentReportAction && (
                    <ReportActionItem
                        isParentReport
                        report={props.parentReport}

);
}

Expand Down
3 changes: 2 additions & 1 deletion src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -553,8 +553,9 @@ function isThreadParent(reportAction) {
/**
* Returns true if reportAction is the first chat preview of a Thread
*
* @deprecated
* @param {Object} reportAction
* @param {String} reportID
* @param {String} reportID
* @returns {Boolean}
*/
function isThreadFirstChat(reportAction, reportID) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ class BaseReportActionContextMenu extends React.Component {
this.props.reportID,
this.props.isPinnedChat,
this.props.isUnreadChat,
this.props.isParentReport,
);

/**
Expand Down Expand Up @@ -94,6 +95,7 @@ class BaseReportActionContextMenu extends React.Component {
const payload = {
reportAction: this.props.reportAction,
reportID: this.props.reportID,
isParentReport: this.props.isParentReport,
draftMessage: this.props.draftMessage,
selection: this.props.selection,
close: () => this.setState({shouldKeepOpen: false}),
Expand Down
14 changes: 8 additions & 6 deletions src/pages/home/report/ContextMenu/ContextMenuActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ export default [
icon: Expensicons.ChatBubble,
successTextTranslateKey: '',
successIcon: null,
shouldShow: (type, reportAction, isArchivedRoom, betas, anchor, isChronosReport, reportID) =>
type === CONTEXT_MENU_TYPES.REPORT_ACTION && reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && !ReportUtils.isThreadFirstChat(reportAction, reportID),
shouldShow: (type, reportAction, isArchivedRoom, betas, anchor, isChronosReport, reportID, isPinnedChat, isParentReport) =>
tienifr marked this conversation as resolved.
Show resolved Hide resolved
type === CONTEXT_MENU_TYPES.REPORT_ACTION && reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && !isParentReport,
onPress: (closePopover, {reportAction, reportID}) => {
Report.navigateToAndOpenChildReport(lodashGet(reportAction, 'childReportID', '0'), reportAction, reportID);
if (closePopover) {
Expand Down Expand Up @@ -203,10 +203,11 @@ export default [
const isAttachmentTarget = lodashGet(menuTarget, 'tagName') === 'IMG' && isAttachment;
return Permissions.canUseCommentLinking(betas) && type === CONTEXT_MENU_TYPES.REPORT_ACTION && !isAttachmentTarget && !ReportActionUtils.isMessageDeleted(reportAction);
},
onPress: (closePopover, {reportAction, reportID}) => {
onPress: (closePopover, {reportID, reportAction, isParentReport}) => {
Environment.getEnvironmentURL().then((environmentURL) => {
const reportActionID = parseInt(lodashGet(reportAction, 'reportActionID'), 10);
Clipboard.setString(`${environmentURL}/r/${reportID}/${reportActionID}`);
const activeReportID = isParentReport ? reportAction.childReportID : reportID;
Clipboard.setString(`${environmentURL}/r/${activeReportID}/${reportActionID}`);
});
hideContextMenu(true, ReportActionComposeFocusManager.focus);
},
Expand All @@ -219,8 +220,9 @@ export default [
successIcon: Expensicons.Checkmark,
shouldShow: (type, reportAction, isArchivedRoom, betas, anchor, isChronosReport, reportID, isPinnedChat, isUnreadChat) =>
type === CONTEXT_MENU_TYPES.REPORT_ACTION || (type === CONTEXT_MENU_TYPES.REPORT && !isUnreadChat),
onPress: (closePopover, {reportAction, reportID}) => {
Report.markCommentAsUnread(reportID, reportAction.created);
onPress: (closePopover, {reportAction, reportID, isParentReport}) => {
const activeReportID = isParentReport ? reportAction.childReportID : reportID;
Report.markCommentAsUnread(activeReportID, reportAction.created);
if (closePopover) {
hideContextMenu(true, ReportActionComposeFocusManager.focus);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class PopoverReportActionContextMenu extends React.Component {
isChronosReport: false,
isPinnedChat: false,
isUnreadChat: false,
isParentReport: false,
};
this.onPopoverShow = () => {};
this.onPopoverHide = () => {};
Expand Down Expand Up @@ -130,6 +131,7 @@ class PopoverReportActionContextMenu extends React.Component {
* @param {Boolean} isChronosReport - Flag to check if the chat participant is Chronos
* @param {Boolean} isPinnedChat - Flag to check if the chat is pinned in the LHN. Used for the Pin/Unpin action
* @param {Boolean} isUnreadChat - Flag to check if the chat is unread in the LHN. Used for the Mark as Read/Unread action
* @param {Boolean} [isParentReport] - Is the report action from parent report
*/
showContextMenu(
type,
Expand All @@ -145,6 +147,7 @@ class PopoverReportActionContextMenu extends React.Component {
isChronosReport = false,
isPinnedChat = false,
isUnreadChat = false,
isParentReport = false,
) {
const nativeEvent = event.nativeEvent || {};
this.contextMenuAnchor = contextMenuAnchor;
Expand Down Expand Up @@ -177,6 +180,7 @@ class PopoverReportActionContextMenu extends React.Component {
isChronosReport,
isPinnedChat,
isUnreadChat,
isParentReport,
});
});
}
Expand Down Expand Up @@ -266,6 +270,7 @@ class PopoverReportActionContextMenu extends React.Component {
isChronosReport: false,
isPinnedChat: false,
isUnreadChat: false,
isParentReport: false,
});
}

Expand Down Expand Up @@ -314,6 +319,7 @@ class PopoverReportActionContextMenu extends React.Component {
isChronosReport={this.state.isChronosReport}
isPinnedChat={this.state.isPinnedChat}
isUnreadChat={this.state.isUnreadChat}
isParentReport={this.state.isParentReport}
anchor={this.contextMenuTargetNode}
contentRef={this.contentRef}
/>
Expand Down
3 changes: 3 additions & 0 deletions src/pages/home/report/ContextMenu/ReportActionContextMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const contextMenuRef = React.createRef();
* @param {Boolean} isChronosReport - Flag to check if the chat participant is Chronos
* @param {Boolean} isPinnedChat - Flag to check if the chat is pinned in the LHN. Used for the Pin/Unpin action
* @param {Boolean} isUnreadChat - Flag to check if the chat has unread messages in the LHN. Used for the Mark as Read/Unread action
* @param {Boolean} [isParentReport] - Is the report action from parent report
*/
function showContextMenu(
type,
Expand All @@ -33,6 +34,7 @@ function showContextMenu(
isChronosReport = false,
isPinnedChat = false,
isUnreadChat = false,
isParentReport = false,
) {
if (!contextMenuRef.current) {
return;
Expand All @@ -51,6 +53,7 @@ function showContextMenu(
isChronosReport,
isPinnedChat,
isUnreadChat,
isParentReport,
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ const propTypes = {
/** The report action this context menu is attached to. */
reportAction: PropTypes.shape(reportActionPropTypes).isRequired,

/** Is the report action from parent report */
isParentReport: PropTypes.bool,

/** If true, this component will be a small, row-oriented menu that displays icons but not text.
If false, this component will be a larger, column-oriented menu that displays icons alongside text in each row. */
isMini: PropTypes.bool,
Expand All @@ -23,6 +26,7 @@ const propTypes = {
};

const defaultProps = {
isParentReport: false,
isMini: false,
isVisible: false,
selection: '',
Expand Down
14 changes: 11 additions & 3 deletions src/pages/home/report/ReportActionItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ const propTypes = {
/** Is this the most recent IOU Action? */
isMostRecentIOUReportAction: PropTypes.bool.isRequired,

/** Is this the action from parent report */
isParentReport: PropTypes.bool,

/** Should we display the new marker on top of the comment? */
shouldDisplayNewMarker: PropTypes.bool.isRequired,

Expand Down Expand Up @@ -100,6 +103,7 @@ const defaultProps = {
personalDetails: {},
shouldShowSubscriptAvatar: false,
hasOutstandingIOU: false,
isParentReport: false,
};

function ReportActionItem(props) {
Expand Down Expand Up @@ -183,9 +187,11 @@ function ReportActionItem(props) {
toggleContextMenuFromActiveReportAction,
ReportUtils.isArchivedRoom(props.report),
ReportUtils.chatIncludesChronos(props.report),
null,
tienifr marked this conversation as resolved.
Show resolved Hide resolved
props.isParentReport,
);
},
[props.draftMessage, props.action, props.report, toggleContextMenuFromActiveReportAction],
[props.draftMessage, props.action, props.report, props.isParentReport, toggleContextMenuFromActiveReportAction],
);

const toggleReaction = useCallback(
Expand Down Expand Up @@ -322,7 +328,7 @@ function ReportActionItem(props) {
const numberOfThreadReplies = _.get(props, ['action', 'childVisibleActionCount'], 0);
const hasReplies = numberOfThreadReplies > 0;

const shouldDisplayThreadReplies = hasReplies && props.action.childCommenterCount && !ReportUtils.isThreadFirstChat(props.action, props.report.reportID);
const shouldDisplayThreadReplies = hasReplies && props.action.childCommenterCount && !props.isParentReport;
const oldestFourAccountIDs = lodashGet(props.action, 'childOldestFourAccountIDs', '').split(',');
const draftMessageRightAlign = props.draftMessage ? styles.chatItemReactionsDraftRight : {};

Expand Down Expand Up @@ -443,6 +449,7 @@ function ReportActionItem(props) {
isVisible={hovered && !props.draftMessage && !hasErrors}
draftMessage={props.draftMessage}
isChronosReport={ReportUtils.chatIncludesChronos(props.report)}
isParentReport={props.isParentReport}
/>
<View
style={StyleUtils.getReportActionItemStyle(
Expand Down Expand Up @@ -526,6 +533,7 @@ export default compose(
_.isEqual(prevProps.action, nextProps.action) &&
lodashGet(prevProps.report, 'statusNum') === lodashGet(nextProps.report, 'statusNum') &&
lodashGet(prevProps.report, 'stateNum') === lodashGet(nextProps.report, 'stateNum') &&
prevProps.translate === nextProps.translate,
prevProps.translate === nextProps.translate &&
prevProps.isParentReport === nextProps.isParentReport,
),
);
10 changes: 9 additions & 1 deletion src/pages/home/report/ReportActionItemParentAction.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ const propTypes = {
/** The report currently being looked at */
report: reportPropTypes,

/** The parent report */
parentReport: reportPropTypes,

/** The actions from the parent report */
// TO DO: Replace with HOC https://github.com/Expensify/App/issues/18769.
parentReportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)),
Expand All @@ -40,6 +43,7 @@ const propTypes = {
};
const defaultProps = {
report: {},
parentReport: {},
parentReportActions: {},
shouldHideThreadDividerLine: false,
};
Expand All @@ -62,7 +66,8 @@ function ReportActionItemParentAction(props) {
<View style={[styles.p5, StyleUtils.getReportWelcomeTopMarginStyle(props.isSmallScreenWidth)]} />
{parentReportAction && (
<ReportActionItem
report={props.report}
isParentReport
report={props.parentReport}
action={parentReportAction}
displayAsGroup={false}
isMostRecentIOUReportAction={false}
Expand All @@ -87,6 +92,9 @@ export default compose(
report: {
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
},
parentReport: {
key: ({parentReportID}) => `${ONYXKEYS.COLLECTION.REPORT}${parentReportID}`,
},
parentReportActions: {
key: ({parentReportID}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${parentReportID}`,
canEvict: false,
Expand Down