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

[$250] Threads - Reply in threads option shown for parent message in thread #50262

Open
1 of 6 tasks
lanitochka17 opened this issue Oct 4, 2024 · 10 comments
Open
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@lanitochka17
Copy link

lanitochka17 commented Oct 4, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.44-6
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): Negasofonias@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to any chat and open a thread or create one
  2. open the thread and right click on the parent message and observe that reply in a thread option is available
  3. Click on it and observe it just focuses the composer box

Expected Result:

Reply in a thread option should not be shown for the parent message

Actual Result:

Reply in a thread option is available with out any purpose

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
Bug6617626_1727458359699.scrnli_9_27_2024_8-23-11_PM.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021843831874684578762
  • Upwork Job ID: 1843831874684578762
  • Last Price Increase: 2024-10-09
Issue OwnerCurrent Issue Owner: @allgandalf
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 4, 2024
Copy link

melvin-bot bot commented Oct 4, 2024

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@lanitochka17
Copy link
Author

@anmurali FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@Nodebrute
Copy link
Contributor

Nodebrute commented Oct 4, 2024

Edited by proposal-police: This proposal was edited at 2024-10-04 19:38:57 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Reply in threads option shown for parent message in thread

What is the root cause of that problem?

We use this check to show Reply In thread.

return !ReportUtils.shouldDisableThread(reportAction, reportID);

It was working fine until we added ancestor.report.

This function checks if it's the thread's first chat. However, the ancestor.report we're using now passes the parent report's reportID instead of the current report's. As a result, this check will return false.

App/src/libs/ReportUtils.ts

Lines 1559 to 1561 in 8f35228

function isThreadFirstChat(reportAction: OnyxInputOrEntry<ReportAction>, reportID: string): boolean {
return reportAction?.childReportID?.toString() === reportID;
}

What changes do you think we should make in order to solve the problem?

We should pass the correct reportID to isThreadFirstChat. In BaseReportActionContextMenu we can use useCurrentReportID() to get the reportID and then we can pass that reportID here

}: BaseReportActionContextMenuProps) {

And then we can pass currentReportIDValue?.currentReportID to this function instead of reportID

return !ReportUtils.shouldDisableThread(reportAction, reportID);

Optional: We can pass currentReportIDValue?.currentReportID as third parameter in shouldDisableThread and only use it to pass it to isThreadFirstChat.

We can check for other places too where we need to pass the correct Id.

What alternative solutions did you explore? (Optional)

We can create a function in ContextMenuActions.tsx to get current report Id.

pseudoCode

function getCurrentReportID(){
    const currentReportIDValue = useCurrentReportID();
    return currentReportIDValue?.currentReportID ?? ''
}

and then in here we can get the currentReportId value

      const currentReportId = getCurrentReportID()

and then we can pass it to isThreadFirstChat

Optional: We can pass currentReportID as third parameter in shouldDisableThread and only use it to pass it to isThreadFirstChat.

@bernhardoj
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Reply in thread option appears for thread first chat.

What is the root cause of that problem?

We check whether the user can reply in thread, but only if it's not a thread first chat.

isThreadFirstChat(reportAction, reportID)

isThreadFirstChat compares the action childReportID with the passed reportID.

App/src/libs/ReportUtils.ts

Lines 1559 to 1561 in 99f280b

function isThreadFirstChat(reportAction: OnyxInputOrEntry<ReportAction>, reportID: string): boolean {
return reportAction?.childReportID?.toString() === reportID;
}

The reportID is coming from ReportActionItem. However, after this PR, the reportID contains the reportID of the report where the action belongs to.

For example, if we create a thread from report A, the reportID will be report A instead of B. So, isThreadFirstChat will always be false.

This also causes the Join Thread to shows up for thread first chat.

What changes do you think we should make in order to solve the problem?

Instead of using isThreadFirstChat() function which requires a reportID that can be anything, we can have a new prop for ReportActionItem also called isThreadFirstChat. It will only be true if the ReportActionItem is coming from ReportActionItemParentAction.

<ReportActionItem
onPress={
ReportUtils.canCurrentUserOpenReport(ancestorReports.current?.[ancestor?.report?.reportID ?? '-1'])

Then, pass the prop down to BaseReportActionContextMenu. BaseReportActionContextMenu will use it for the shouldShow function.

contextAction.shouldShow(
type,
reportAction,
isArchivedRoom,
betas,
anchor,
isChronosReport,
reportID,
isPinnedChat,
isUnreadChat,
!!isOffline,
isMini,
isProduction,
moneyRequestAction,
areHoldRequirementsMet,
),

Then, we pass it to shouldDisableThread.

shouldShow: (type, reportAction, isArchivedRoom, betas, menuTarget, isChronosReport, reportID) => {
if (type !== CONST.CONTEXT_MENU_TYPES.REPORT_ACTION) {
return false;
}
return !ReportUtils.shouldDisableThread(reportAction, reportID);
},

And replace the function here with the new isThreadFirstChat param

isThreadFirstChat(reportAction, reportID)

We will replace all isThreadFirstChat() usage with the new prop.

@melvin-bot melvin-bot bot added the Overdue label Oct 7, 2024
Copy link

melvin-bot bot commented Oct 7, 2024

@anmurali Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Oct 8, 2024

@anmurali Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@anmurali anmurali added the External Added to denote the issue can be worked on by a contributor label Oct 9, 2024
@melvin-bot melvin-bot bot changed the title Threads - Reply in threads option shown for parent message in thread [$250] Threads - Reply in threads option shown for parent message in thread Oct 9, 2024
Copy link

melvin-bot bot commented Oct 9, 2024

Job added to Upwork: https://www.upwork.com/jobs/~021843831874684578762

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 9, 2024
Copy link

melvin-bot bot commented Oct 9, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allgandalf (External)

@melvin-bot melvin-bot bot removed the Overdue label Oct 9, 2024
Copy link

melvin-bot bot commented Oct 14, 2024

@anmurali, @allgandalf Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot melvin-bot bot added the Overdue label Oct 14, 2024
@allgandalf
Copy link
Contributor

Not overdue sir melvin! 🙇

@melvin-bot melvin-bot bot removed the Overdue label Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

5 participants