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

Fix thread parent message comment linking bug #40344

Closed
wants to merge 5 commits into from
Closed

Fix thread parent message comment linking bug #40344

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6b716a3
change comment link for parent message to link to the parent report
FitseTLT Apr 17, 2024
93b7bdb
fixed expense report case bug
FitseTLT Apr 17, 2024
8dce9db
replaced with getOriginalReportID
FitseTLT Apr 22, 2024
7bd7396
Merge branch 'main' into fix-thread-parent-message-comment-linking-bug
FitseTLT Apr 23, 2024
0d0cb5c
Merge branch 'main' into fix-thread-parent-message-comment-linking-bug
FitseTLT May 2, 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
10 changes: 6 additions & 4 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5372,12 +5372,14 @@ function canUserPerformWriteAction(report: OnyxEntry<Report>) {
/**
* Returns ID of the original report from which the given reportAction is first created.
*/
function getOriginalReportID(reportID: string, reportAction: OnyxEntry<ReportAction>): string | undefined {
function getOriginalReportID(reportID: string, reportAction: OnyxEntry<ReportAction>, shouldReturnTransactionThread = false): string | undefined {
const reportActions = allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`];
const currentReportAction = reportActions?.[reportAction?.reportActionID ?? ''] ?? null;
const transactionThreadReportID = ReportActionsUtils.getOneTransactionThreadReportID(reportID, reportActions ?? ([] as ReportAction[]));
if (transactionThreadReportID !== null) {
return Object.keys(currentReportAction ?? {}).length === 0 ? transactionThreadReportID : reportID;
if (shouldReturnTransactionThread) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is transactionThreadReportID computation blocked under this flag? Shouldn't it break for calls of getOriginalReportID since only one place we are sending true and others will be false by default?

And the change here is different than your original proposal #36057 (comment), could you provide information on how this change is expected to fix the issue?

@FitseTLT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abdulrahuman5196 I have already commented an explanation for both in Details section on the PR description.

Copy link
Contributor

Choose a reason for hiding this comment

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

@FitseTLT
This is the from the details section

The expected behaviour has changed from OP of the issue so now we navigate to the parent report and highlight on the parent message according to this #36057 (comment)

This is the expected behaviour I get from the issue, but not sure on how it is being fixed with this PR. Because the code change in the proposal is different from the change in this PR. #36057 (comment). Do let me know if there was a different discussion.

I also fixed a regression from
#39474 which caused reportAction used in context menu to be null as it passes the wrong originalReportID when IOU type report action is displayed as the parent message in a thread.

What is the UX regression on this issue?, to check if its fixed? I don't see any regression statement in that PR as well its relevant GH. Do add those in test steps if its a specific case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the expected behaviour I get from the issue, but not sure on how it is being fixed with this PR. Because the code change in the proposal is different from the change in this PR. #36057 (comment). Do let me know if there was a different discussion.

#36057 (comment) is where the expectation changed that's why the pr is different from the proposal.

What is the UX regression on this issue?, to check if its fixed? I don't see any regression statement in that PR as well its relevant GH. Do add those in test steps if its a specific case.

  1. On a collect workspace: Create a money request and ensure it is the only transaction in the expense report
  2. On the transaction thread send a message
  3. "Reply in thread" with it
  4. Now on the new thread right click on the request preview parent message and copy link
  5. Paste the link

Because of the regression caused from #39474 the reportActionID part of the link will be undefined. But the problem it caused is wider as it makes the all context menu operations not to work (like copy to clipboard )

const transactionThreadReportID = ReportActionsUtils.getOneTransactionThreadReportID(reportID, reportActions ?? ([] as ReportAction[]));
if (transactionThreadReportID !== null) {
return Object.keys(currentReportAction ?? {}).length === 0 ? transactionThreadReportID : reportID;
}
}
return isThreadFirstChat(reportAction, reportID) && Object.keys(currentReportAction ?? {}).length === 0
? allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.parentReportID
Expand Down
2 changes: 1 addition & 1 deletion src/libs/actions/ReportActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import * as Report from './Report';
type IgnoreDirection = 'parent' | 'child';

function clearReportActionErrors(reportID: string, reportAction: ReportAction, keys?: string[]) {
const originalReportID = ReportUtils.getOriginalReportID(reportID, reportAction);
const originalReportID = ReportUtils.getOriginalReportID(reportID, reportAction, true);

if (!reportAction?.reportActionID) {
return;
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/report/ContextMenu/ContextMenuActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ const ContextMenuActions: ContextMenuAction[] = [
onPress: (closePopover, {reportAction, reportID}) => {
Environment.getEnvironmentURL().then((environmentURL) => {
const reportActionID = reportAction?.reportActionID;
Clipboard.setString(`${environmentURL}/r/${reportID}/${reportActionID}`);
Clipboard.setString(`${environmentURL}/r/${ReportUtils.getOriginalReportID(reportID, reportAction)}/${reportActionID}`);
});
hideContextMenu(true, ReportActionComposeFocusManager.focus);
},
Expand Down
Loading