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 reply in thread shows for thread parent #51721

Merged
26 changes: 8 additions & 18 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1600,13 +1600,6 @@ function isWorkspaceThread(report: OnyxEntry<Report>): boolean {
return isThread(report) && isChatReport(report) && CONST.WORKSPACE_ROOM_TYPES.some((type) => chatType === type);
}

/**
* Returns true if reportAction is the first chat preview of a Thread
*/
function isThreadFirstChat(reportAction: OnyxInputOrEntry<ReportAction>, reportID: string): boolean {
return reportAction?.childReportID?.toString() === reportID;
}

/**
* Checks if a report is a child report.
*/
Expand Down Expand Up @@ -1852,10 +1845,9 @@ function canDeleteReportAction(reportAction: OnyxInputOrEntry<ReportAction>, rep
return false;
}

const linkedReport = isThreadFirstChat(reportAction, reportID) ? getReportOrDraftReport(report?.parentReportID) : report;
if (isActionOwner) {
if (!isEmptyObject(linkedReport) && (isMoneyRequestReport(linkedReport) || isInvoiceReport(linkedReport))) {
return canDeleteTransaction(linkedReport);
if (!isEmptyObject(report) && (isMoneyRequestReport(report) || isInvoiceReport(report))) {
return canDeleteTransaction(report);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

canDeleteReportAction is only being used in ContextMenuActions.

const linkedReport = isThreadFirstChat(reportAction, reportID) ? getReportOrDraftReport(report?.parentReportID) : report;

This logic was added in #40287 because the reportID of the thread parent message is the currently view report. But because the reportID now contains the reportID of the report action, we don't need this anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense to me

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, but can we please write a manual test that will verify this?

return true;
}
Expand Down Expand Up @@ -7276,10 +7268,9 @@ function getOriginalReportID(reportID: string, reportAction: OnyxInputOrEntry<Re
const reportActions = allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`];
const currentReportAction = reportActions?.[reportAction?.reportActionID ?? '-1'] ?? null;
const transactionThreadReportID = ReportActionsUtils.getOneTransactionThreadReportID(reportID, reportActions ?? ([] as ReportAction[]));
const isThreadReportParentAction = reportAction?.childReportID?.toString() === reportID;
if (Object.keys(currentReportAction ?? {}).length === 0) {
return isThreadFirstChat(reportAction, reportID)
? ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.parentReportID
: transactionThreadReportID ?? reportID;
return isThreadReportParentAction ? ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.parentReportID : transactionThreadReportID ?? reportID;
}
return reportID;
}
Expand Down Expand Up @@ -7747,9 +7738,9 @@ function hasOnlyHeldExpenses(iouReportID: string): boolean {
/**
* Checks if thread replies should be displayed
*/
function shouldDisplayThreadReplies(reportAction: OnyxInputOrEntry<ReportAction>, reportID: string): boolean {
function shouldDisplayThreadReplies(reportAction: OnyxInputOrEntry<ReportAction>, isThreadReportParentAction: boolean): boolean {
const hasReplies = (reportAction?.childVisibleActionCount ?? 0) > 0;
return hasReplies && !!reportAction?.childCommenterCount && !isThreadFirstChat(reportAction, reportID);
return hasReplies && !!reportAction?.childCommenterCount && !isThreadReportParentAction;
}

/**
Expand Down Expand Up @@ -7807,7 +7798,7 @@ function getNonHeldAndFullAmount(iouReport: OnyxEntry<Report>, policy: OnyxEntry
* - The action is a whisper action and it's neither a report preview nor IOU action
* - The action is the thread's first chat
*/
function shouldDisableThread(reportAction: OnyxInputOrEntry<ReportAction>, reportID: string): boolean {
function shouldDisableThread(reportAction: OnyxInputOrEntry<ReportAction>, reportID: string, isThreadReportParentAction: boolean): boolean {
const isSplitBillAction = ReportActionsUtils.isSplitBillAction(reportAction);
const isDeletedAction = ReportActionsUtils.isDeletedAction(reportAction);
const isReportPreviewAction = ReportActionsUtils.isReportPreviewAction(reportAction);
Expand All @@ -7822,7 +7813,7 @@ function shouldDisableThread(reportAction: OnyxInputOrEntry<ReportAction>, repor
(isDeletedAction && !reportAction?.childVisibleActionCount) ||
(isArchivedReport && !reportAction?.childVisibleActionCount) ||
(isWhisperAction && !isReportPreviewAction && !isIOUAction) ||
isThreadFirstChat(reportAction, reportID)
isThreadReportParentAction
);
}

Expand Down Expand Up @@ -8678,7 +8669,6 @@ export {
isSystemChat,
isTaskReport,
isThread,
isThreadFirstChat,
isTrackExpenseReport,
isUnread,
isUnreadWithMention,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ type BaseReportActionContextMenuProps = {
/** Flag to check if the chat is unread in the LHN. Used for the Mark as Read/Unread action */
isUnreadChat?: boolean;

/**
* Is the action a thread's parent reportAction viewed from within the thread report?
* It will be false if we're viewing the same parent report action from the report it belongs to rather than the thread.
*/
isThreadReportParentAction?: boolean;

/** Content Ref */
contentRef?: RefObject<View>;

Expand All @@ -101,6 +107,7 @@ function BaseReportActionContextMenu({
isVisible = false,
isPinnedChat = false,
isUnreadChat = false,
isThreadReportParentAction = false,
selection = '',
draftMessage = '',
reportActionID,
Expand Down Expand Up @@ -194,6 +201,7 @@ function BaseReportActionContextMenu({
reportID,
isPinnedChat,
isUnreadChat,
isThreadReportParentAction,
isOffline: !!isOffline,
isMini,
isProduction,
Expand Down Expand Up @@ -284,6 +292,7 @@ function BaseReportActionContextMenu({
true,
() => {},
true,
isThreadReportParentAction,
);
};

Expand Down
19 changes: 13 additions & 6 deletions src/pages/home/report/ContextMenu/ContextMenuActions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ type ShouldShow = (args: {
reportID: string;
isPinnedChat: boolean;
isUnreadChat: boolean;
isThreadReportParentAction: boolean;
isOffline: boolean;
isMini: boolean;
isProduction: boolean;
Expand Down Expand Up @@ -178,11 +179,11 @@ const ContextMenuActions: ContextMenuAction[] = [
isAnonymousAction: false,
textTranslateKey: 'reportActionContextMenu.replyInThread',
icon: Expensicons.ChatBubbleReply,
shouldShow: ({type, reportAction, reportID}) => {
shouldShow: ({type, reportAction, reportID, isThreadReportParentAction}) => {
if (type !== CONST.CONTEXT_MENU_TYPES.REPORT_ACTION) {
return false;
}
return !ReportUtils.shouldDisableThread(reportAction, reportID);
return !ReportUtils.shouldDisableThread(reportAction, reportID, isThreadReportParentAction);
},
onPress: (closePopover, {reportAction, reportID}) => {
const originalReportID = ReportUtils.getOriginalReportID(reportID, reportAction);
Expand Down Expand Up @@ -301,16 +302,22 @@ const ContextMenuActions: ContextMenuAction[] = [
isAnonymousAction: false,
textTranslateKey: 'reportActionContextMenu.joinThread',
icon: Expensicons.Bell,
shouldShow: ({reportAction, isArchivedRoom, reportID}) => {
shouldShow: ({reportAction, isArchivedRoom, isThreadReportParentAction}) => {
const childReportNotificationPreference = ReportUtils.getChildReportNotificationPreference(reportAction);
const isDeletedAction = ReportActionsUtils.isDeletedAction(reportAction);
const shouldDisplayThreadReplies = ReportUtils.shouldDisplayThreadReplies(reportAction, reportID);
const shouldDisplayThreadReplies = ReportUtils.shouldDisplayThreadReplies(reportAction, isThreadReportParentAction);
const subscribed = childReportNotificationPreference !== 'hidden';
const isThreadFirstChat = ReportUtils.isThreadFirstChat(reportAction, reportID);
const isWhisperAction = ReportActionsUtils.isWhisperAction(reportAction) || ReportActionsUtils.isActionableTrackExpense(reportAction);
const isExpenseReportAction = ReportActionsUtils.isMoneyRequestAction(reportAction) || ReportActionsUtils.isReportPreviewAction(reportAction);
const isTaskAction = ReportActionsUtils.isCreatedTaskReportAction(reportAction);
return !subscribed && !isWhisperAction && !isTaskAction && !isExpenseReportAction && !isThreadFirstChat && (shouldDisplayThreadReplies || (!isDeletedAction && !isArchivedRoom));
return (
!subscribed &&
!isWhisperAction &&
!isTaskAction &&
!isExpenseReportAction &&
!isThreadReportParentAction &&
(shouldDisplayThreadReplies || (!isDeletedAction && !isArchivedRoom))
);
},
onPress: (closePopover, {reportAction, reportID}) => {
const childReportNotificationPreference = ReportUtils.getChildReportNotificationPreference(reportAction);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ function PopoverReportActionContextMenu(_props: unknown, ref: ForwardedRef<Repor
const [isChronosReportEnabled, setIsChronosReportEnabled] = useState(false);
const [isChatPinned, setIsChatPinned] = useState(false);
const [hasUnreadMessages, setHasUnreadMessages] = useState(false);
const [isThreadReportParentAction, setIsThreadReportParentAction] = useState(false);
const [disabledActions, setDisabledActions] = useState<ContextMenuAction[]>([]);
const [shoudSwitchPositionIfOverflow, setShoudSwitchPositionIfOverflow] = useState(false);

Expand Down Expand Up @@ -172,6 +173,7 @@ function PopoverReportActionContextMenu(_props: unknown, ref: ForwardedRef<Repor
shouldCloseOnTarget = false,
setIsEmojiPickerActive = () => {},
isOverflowMenu = false,
isThreadReportParentActionParam = false,
) => {
const {pageX = 0, pageY = 0} = extractPointerEvent(event);
contextMenuAnchorRef.current = contextMenuAnchor;
Expand Down Expand Up @@ -220,6 +222,7 @@ function PopoverReportActionContextMenu(_props: unknown, ref: ForwardedRef<Repor
setIsChronosReportEnabled(isChronosReport);
setIsChatPinned(isPinnedChat);
setHasUnreadMessages(isUnreadChat);
setIsThreadReportParentAction(isThreadReportParentActionParam);
setShoudSwitchPositionIfOverflow(isOverflowMenu);
});
};
Expand Down Expand Up @@ -347,6 +350,7 @@ function PopoverReportActionContextMenu(_props: unknown, ref: ForwardedRef<Repor
isChronosReport={isChronosReportEnabled}
isPinnedChat={isChatPinned}
isUnreadChat={hasUnreadMessages}
isThreadReportParentAction={isThreadReportParentAction}
anchor={contextMenuTargetNode}
contentRef={contentRef}
originalReportID={originalReportIDRef.current}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ type ShowContextMenu = (
shouldCloseOnTarget?: boolean,
setIsEmojiPickerActive?: (state: boolean) => void,
isOverflowMenu?: boolean,
isThreadReportParentAction?: boolean,
) => void;

type ReportActionContextMenu = {
Expand Down Expand Up @@ -118,6 +119,7 @@ function showContextMenu(
shouldCloseOnTarget = false,
setIsEmojiPickerActive = () => {},
isOverflowMenu = false,
isThreadReportParentAction = false,
) {
if (!contextMenuRef.current) {
return;
Expand All @@ -142,6 +144,7 @@ function showContextMenu(
shouldCloseOnTarget,
setIsEmojiPickerActive,
isOverflowMenu,
isThreadReportParentAction,
);
};

Expand Down
28 changes: 23 additions & 5 deletions src/pages/home/report/ReportActionItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,15 @@ type ReportActionItemProps = {
/** If this is the first visible report action */
isFirstVisibleReportAction: boolean;

/**
* Is the action a thread's parent reportAction viewed from within the thread report?
* It will be false if we're viewing the same parent report action from the report it belongs to rather than the thread.
*/
isThreadReportParentAction?: boolean;

/** IF the thread divider line will be used */
shouldUseThreadDividerLine?: boolean;

hideThreadReplies?: boolean;

/** Whether context menu should be displayed */
shouldDisplayContextMenu?: boolean;
};
Expand All @@ -154,8 +158,8 @@ function ReportActionItem({
shouldShowSubscriptAvatar = false,
onPress = undefined,
isFirstVisibleReportAction = false,
isThreadReportParentAction = false,
shouldUseThreadDividerLine = false,
hideThreadReplies = false,
shouldDisplayContextMenu = true,
parentReportActionForTransactionThread,
}: ReportActionItemProps) {
Expand Down Expand Up @@ -356,9 +360,22 @@ function ReportActionItem({
disabledActions,
false,
setIsEmojiPickerActive as () => void,
undefined,
isThreadReportParentAction,
);
},
[draftMessage, action, reportID, toggleContextMenuFromActiveReportAction, originalReportID, shouldDisplayContextMenu, disabledActions, isArchivedRoom, isChronosReport],
[
draftMessage,
action,
reportID,
toggleContextMenuFromActiveReportAction,
originalReportID,
shouldDisplayContextMenu,
disabledActions,
isArchivedRoom,
isChronosReport,
isThreadReportParentAction,
],
);

// Handles manual scrolling to the bottom of the chat when the last message is an actionable whisper and it's resolved.
Expand Down Expand Up @@ -784,7 +801,7 @@ function ReportActionItem({
}
const numberOfThreadReplies = action.childVisibleActionCount ?? 0;

const shouldDisplayThreadReplies = !hideThreadReplies && ReportUtils.shouldDisplayThreadReplies(action, reportID);
const shouldDisplayThreadReplies = ReportUtils.shouldDisplayThreadReplies(action, isThreadReportParentAction);
const oldestFourAccountIDs =
action.childOldestFourAccountIDs
?.split(',')
Expand Down Expand Up @@ -970,6 +987,7 @@ function ReportActionItem({
displayAsGroup={displayAsGroup}
disabledActions={disabledActions}
isVisible={hovered && draftMessage === undefined && !hasErrors}
isThreadReportParentAction={isThreadReportParentAction}
draftMessage={draftMessage}
isChronosReport={isChronosReport}
checkIfContextMenuActive={toggleContextMenuFromActiveReportAction}
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/report/ReportActionItemParentAction.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ function ReportActionItemParentAction({
index={index}
isFirstVisibleReportAction={isFirstVisibleReportAction}
shouldUseThreadDividerLine={shouldUseThreadDividerLine}
hideThreadReplies
isThreadReportParentAction
/>
)}
</OfflineWithFeedback>
Expand Down
16 changes: 8 additions & 8 deletions tests/unit/ReportUtilsTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -793,7 +793,7 @@ describe('ReportUtils', () => {

it('should disable on thread-disabled actions', () => {
const reportAction = ReportUtils.buildOptimisticCreatedReportAction('email1@test.com');
expect(ReportUtils.shouldDisableThread(reportAction, reportID)).toBeTruthy();
expect(ReportUtils.shouldDisableThread(reportAction, reportID, false)).toBeTruthy();
});

it('should disable thread on split expense actions', () => {
Expand All @@ -805,7 +805,7 @@ describe('ReportUtils', () => {
[{login: 'email1@test.com'}, {login: 'email2@test.com'}],
NumberUtils.rand64(),
) as ReportAction;
expect(ReportUtils.shouldDisableThread(reportAction, reportID)).toBeTruthy();
expect(ReportUtils.shouldDisableThread(reportAction, reportID, false)).toBeTruthy();
});

it('should disable on deleted and not-thread actions', () => {
Expand All @@ -821,10 +821,10 @@ describe('ReportUtils', () => {
],
childVisibleActionCount: 1,
} as ReportAction;
expect(ReportUtils.shouldDisableThread(reportAction, reportID)).toBeFalsy();
expect(ReportUtils.shouldDisableThread(reportAction, reportID, false)).toBeFalsy();

reportAction.childVisibleActionCount = 0;
expect(ReportUtils.shouldDisableThread(reportAction, reportID)).toBeTruthy();
expect(ReportUtils.shouldDisableThread(reportAction, reportID, false)).toBeTruthy();
});

it('should disable on archived reports and not-thread actions', () => {
Expand All @@ -837,10 +837,10 @@ describe('ReportUtils', () => {
const reportAction = {
childVisibleActionCount: 1,
} as ReportAction;
expect(ReportUtils.shouldDisableThread(reportAction, reportID)).toBeFalsy();
expect(ReportUtils.shouldDisableThread(reportAction, reportID, false)).toBeFalsy();

reportAction.childVisibleActionCount = 0;
expect(ReportUtils.shouldDisableThread(reportAction, reportID)).toBeTruthy();
expect(ReportUtils.shouldDisableThread(reportAction, reportID, false)).toBeTruthy();
});
});

Expand All @@ -851,14 +851,14 @@ describe('ReportUtils', () => {
whisperedTo: [123456],
},
} as ReportAction;
expect(ReportUtils.shouldDisableThread(reportAction, reportID)).toBeTruthy();
expect(ReportUtils.shouldDisableThread(reportAction, reportID, false)).toBeTruthy();
});

it('should disable on thread first chat', () => {
const reportAction = {
childReportID: reportID,
} as ReportAction;
expect(ReportUtils.shouldDisableThread(reportAction, reportID)).toBeTruthy();
expect(ReportUtils.shouldDisableThread(reportAction, reportID, true)).toBeTruthy();
});
});

Expand Down
Loading