[HOLD for payment 2023-11-02] [$500] Fix the inconsistency of not being able to create threads on all system messages in NewDot

[trjExpensify]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Create a request
2. Edit the amount to generate a "change" system message
3. Observe you can't start a thread on the system message
4. Pay the request to generate a "paid" system message
5. Observe that you can start a thread on the paid system message

Expected Result:

You should be able to start a thread on all system messages in NewDot

Actual Result:

We're inconsistent. Some you can, some you can't for no good reason.

Workaround:

N/A

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: v1.3.77-5
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:
Issue reported by: @shawnborton
Slack conversation: (Internal) https://expensify.slack.com/archives/C03U7DCU4/p1696435492003729

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017c13143627c2ed36
• Upwork Job ID: 1709972345317482496
• Last Price Increase: 2023-10-05
• Automatic offers:
• dukenv0307 | Contributor | 27163513

[melvin-bot]

Current assignee @trjExpensify is eligible for the Bug assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[dukenv0307]

Proposal

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

inconsistency of not being able to create threads on all system messages in NewDot

What is the root cause of that problem?

We are accepting reply in thread option for some special options here

App/src/pages/home/report/ContextMenu/ContextMenuActions.js

Line 131 in 9d4e3bd

return isCommentAction || isReportPreviewAction || isIOUAction;

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

We should update the shouldShow of reply in thread option to allow this action for all system messages like this.

const isCommentAction = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && !ReportUtils.isThreadFirstChat(reportAction, reportID);
const isReportPreviewAction = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW;
const isIOUAction = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && !ReportActionsUtils.isSplitBillAction(reportAction);
const isModifiedExpenseAction = ReportActionsUtils.isModifiedExpenseAction(reportAction);
const isTaskAction = ReportActionsUtils.isTaskAction(reportAction);
return isCommentAction || isReportPreviewAction || isIOUAction || isModifiedExpenseAction || isTaskAction;

App/src/pages/home/report/ContextMenu/ContextMenuActions.js

Line 131 in 9d4e3bd

return isCommentAction || isReportPreviewAction || isIOUAction;

We can list the full system messages in the PR.

What alternative solutions did you explore? (Optional)

[namhihi237]

Proposal

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

should be able to start a thread on all system messages

What is the root cause of that problem?

With the message change request, we have actionName: "MODIFIEDEXPENSE but here we only check actionName is 'IOU'

App/src/pages/home/report/ContextMenu/ContextMenuActions.js

Lines 130 to 131 in 9d4e3bd

	const isIOUAction = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && !ReportActionsUtils.isSplitBillAction(reportAction);
	return isCommentAction || isReportPreviewAction || isIOUAction;

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

We should update like this:

const isIOUAction = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && !ReportActionsUtils.isSplitBillAction(reportAction) || reportAction.actionName ===  CONST.REPORT.ACTIONS.TYPE.MODIFIEDEXPENSE

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

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

[trjExpensify]

@thesahindia can you review these proposals, please?

[thesahindia]

@dukenv0307's proposal looks good to me!

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @youssef-lr, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

📣 @thesahindia Please request via NewDot manual requests for the Reviewer role ($500)

[melvin-bot]

📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[youssef-lr]

Not overdue!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.91-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Show reply in thread option for all system messages #29528

If no regressions arise, payment will be issued on 2023-11-02. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @thesahindia does not require payment (Eligible for Manual Requests)
• @dukenv0307 requires payment offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@thesahindia] The PR that introduced the bug has been identified. Link to the PR:
• [@thesahindia] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@thesahindia] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@thesahindia] Determine if we should create a regression test for this bug.
• [@thesahindia] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[trjExpensify]

👋 @thesahindia checklist time!

[thesahindia]

We weren't handling this case. I think we just have to add a test case.

Steps -

1. Create a money request
2. Go to the transaction thread report
3. Edit any field of the request
4. Hover or long press over the modified message.
5. Verify that the reply in thread option appears

[trjExpensify]

We weren't handling this case. I think we just have to add a test case.

I wonder if there's something we can do to try and prevent this from happening again in the future? It seems as though new report actions were added and showing reply in thread on them weren't considered. As a result, that introduced the inconsistency. Would something in the PR checklist help with this perhaps?

[thesahindia]

As a result, that introduced the inconsistency. Would something in the PR checklist help with this perhaps?

We can add a check for it but we rarely add this type of report action. So, maybe we should just announce it in the channel and keep this in mind.

[trjExpensify]

Hm, I'm not sure. I'd imagine we're going to be adding report actions a fair bit through reunification aren't we. @tgolen is always a good sense check on this kind of stuff for me. Tim, am I being a bit overkill suggesting adding a note to the PR checklist for making sure if you add a new report action to NewDot, you don't forget to not hide the Reply in thread comment action menu item?

We ran into this issue here where some were available to start threads on and others weren't just because of oversight on needing to do this.

Thanks!

[tgolen]

This one is tricky. I don't think adding anything to the checklist is going to help, but it's also not going to hurt. People just don't read the checklist probably 95% of the time.

To have a full proof way to prevent this, it would be best to have an automated solution like:

• A custom ESLint rule
• If ^ isn't possible, then add a GH action which sees if a PR has added a new action (maybe based on file name? Are all actions stored in the same place?) and then checks to see if the logic for this line was also updated in the same PR.
• Maybe better to change the logic for Reply in thread which wouldn't require it to be updated for every new action (for example, rather than checking for all the actions you CAN reply to, only check for the ones you CANNOT reply to, or vice versa, whatever)
• We could have an action configuration file that lists all the actions, and whether or not they can be replied to (this makes it easier for people to understand that they have to think about this setting when they create a new action)

[trjExpensify]

Thanks, Tim! Given that being able to thread on pretty much anything is a pretty integral part of the NewDot experience, I think it's important that we ensure the consistency on starting them on report actions. @thesahindia what do you think about running with this?

[trjExpensify]

bump @thesahindia for your thoughts. :)

[thesahindia]

It makes sense to me!

[trjExpensify]

Okay great, so how do we go about putting that in place?

[thesahindia]

We can log it as low priority GH and get a volunteer to explore all the possible solutions and find the best way to handle it.

[melvin-bot]

@youssef-lr, @trjExpensify, @thesahindia, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[trjExpensify]

Created an issue here and I've added you to it for the C+ and context. @dukenv0307 if this is something you would be interested in putting together as a follow-up, do jump in.

[trjExpensify]

Confirming payments for this issue as follows:

• @dukenv0307 $750 for the fix & #urgency bonus as per here.
• @thesahindia $750 for the C+ review & #urgency bonus.

Bonus applies as the PR was created prior to the cut off for urgency bonus.

@dukenv0307 I've paid you, @thesahindia you can request on NewDot. Closing! 👍

[JmillsExpensify]

$750 payment approved for @thesahindia based on comment above.