[HOLD for payment 2023-07-05] [Pay $250 reporting bonus] Editing parent comment in the thread are not reflected until the thread is revisited

[kavimuru]

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. Open any chat
2. Send a message
3. Click on Reply in thread icon
4. Hover over the comment
5. Click on the Edit comment icon
6. Edit the comment
7. Click on the Save changes icon
8. Observe the changes are not reflected
9. Switch to some other chat
10. Go back to the thread
11. Observe the changes are reflected now

Expected Result:

The edited comment is displayed immediately

Actual Result:

The edited comment is not displayed until the thread is revisited

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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: 1.3.16.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

Screen.Recording.2023-05-18.at.1.29.33.AM.mov

Recording.687.mp4

Expensify/Expensify Issue URL:
Issue reported by: @adeel0202
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1684355738069459

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c4411ddc089f97a5
• Upwork Job ID: 1660794802839674880
• Last Price Increase: 2023-06-05

[melvin-bot]

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[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

[allroundexperts]

My proposal here will fix this as well. Both of them have the same root cause.

[strepanier03]

@allroundexperts - You'll still need to put your proposal for this fix on this GH once it moves to external. You won't be able to claim multiple GHs with a single proposal, everyone will get their fair chance at providing the best proposal to this problem.

[strepanier03]

Easy to recreate, thanks for raising!

[melvin-bot]

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

[melvin-bot]

Current assignee @strepanier03 is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @Beamanator (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[allroundexperts]

@allroundexperts - You'll still need to put your proposal for this fix on this GH once it moves to external. You won't be able to claim multiple GHs with a single proposal, everyone will get their fair chance at providing the best proposal to this problem.

@strepanier03 My understanding was that we would merge this one into the other one since all of them have the same root cause. I'm more than happy to post the proposal here as well 😄

[allroundexperts]

Proposal

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

Editing parent comment in the thread are not reflected until the thread is revisited

Context

Each thread is a report on its own and each message in a thread has a report action as well. Each thread report also references a parent report and a parent report action (along which the thread was started). With our implementation of threads, the first message of the thread does not have a report action in the thread report itself. Rather, the first message (also called thread title) uses the report action id of the parent report directly.

When a thread is created, it initially does not contain a report action item for the thread title (since we're using the reportAction of the parent directly for the thread title)

What is the root cause of that problem?

The root cause of this issue is that when editing thread titles, we're editing it in the thread report instead of the parent report. Since the thread title does not exist in the thread report itself, the updated attributes get optimistically merged into an empty report action object in the thread. This causes an incomplete action (which does not have a timestamp and other properties and has the message only) to be created in the thread report.

When the data is received from the backend via Pusher for the update, the report action id in the response from the backend corresponds to the correct parent report action id. This causes the data to be correctly merged into the parent report action item for the message while the thread report still has an incomplete report action item for the title (which is ignored because we're using the parent report action for the thread title).

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

We can replace this with:

// parent report can be fetched from onyx storage using props.parentReportID prop
 report={props.parentReport}

However, doing this will cause the following to show up in the thread:

To avoid this, we can add a new prop called displayThreadReplies in ReportActionItem component (such that thread replies are not shown if the prop is set as false) and set its value to false here.

If above is not an option, then we can also pass the parent report id and parent report action to the editReportComment function here IF thread title is being edited. This will ensure that edits are being done to the parent report instead of creating an incomplete action in the thread report. The editReportComment function is being called here. We can check here if the report action is a thread parent. If it is, then we can pass parent params to the editReportComment function.

Result

Screen.Recording.2023-05-21.at.3.55.40.PM.mov

What alternative solutions did you explore? (Optional)

In the editReportComment, we first have to check if the given originalReportAction is isThreadParent. We also have to get the parentReportId and parentReportActionId ie:

    const isThreadParent = ReportUtils.isThreadParent(originalReportAction);
    const {parentReportID: threadParentReportId, parentReportActionID: threadParentReportActionId} = allReports[reportID];

If the originalReportAction is a thread parent, then in all the subsequent optimistic actions and api calls, we have to use parentReportId instead of the reportID and parentReportActionId instead of the reportActionId. Also, we have to update the thread report name, if the message being edited is thread parent. ie:

if (isThreadParent) {
        optimisticData.push({
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
            value: {
                displayName: textForNewComment,
            },
        });
    }

[strepanier03]

My understanding was that we would merge this one into the other one since all of them have the same root cause. I'm more than happy to post the proposal here as well 😄

@allroundexperts - I'll double check on this, I could easily have missed something changing and I wouldn't want to hold up fixes unnecessarily.

[strepanier03]

@Beamanator and @abdulrahuman5196 - Can you both check these other GHs (#19352 and #19348) and see if you agree that all three GHs have the same root cause?

If they have the same root cause then should we merge these three into one GH?

@allroundexperts has posted a proposal that solves all three in one.

Context here for @Beamanator.

[Beamanator]

It does look like both issues deal with modifications to the parent comment in the thread, so I could see how these could have the same root cause 👍

However I think we would only "merge" the issues if they're the same issue... They seem very similar but may not be exactly the same issue.

If 1 PR fixes both issues I think the PR should get the reward for fixing both PRs, right @strepanier03 ?

[abdulrahuman5196]

@allroundexperts
Could you add more information on the root cause section. I am unable to fully understand the root cause you are mentioning.

This causes an incomplete action (which does not have a timestamp and other properties and has message only) to be created in the thread report.

If there is only some incomplete/wrong action, how does the action gets updated to proper one when we switch back?

[allroundexperts]

@allroundexperts Could you add more information on the root cause section. I am unable to fully understand the root cause you are mentioning.

This causes an incomplete action (which does not have a timestamp and other properties and has message only) to be created in the thread report.

If there is only some incomplete/wrong action, how does the action gets updated to proper one when we switch back?

@abdulrahuman5196 Updated the proposal with more context. I hope it makes sense now.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[strepanier03]

Thank you @allroundexperts - I'll follow the thread and ping some people/teams directly if we don't get any responses.

[melvin-bot]

@Beamanator @strepanier03 @allroundexperts @abdulrahuman5196 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[abdulrahuman5196]

The thread is still ongoing.

[abdulrahuman5196]

The thread is still ongoing

[abdulrahuman5196]

@strepanier03 It seems there is an internal effort to solve issues revolving around thread edits and reactions. We should be able to close this issue - https://expensify.slack.com/archives/C01GTK53T8Q/p1686778600350519?thread_ts=1686259969.958099&cid=C01GTK53T8Q

https://github.com/Expensify/Expensify/issues/282991

cc: @chiragsalian

[strepanier03]

Thank you @abdulrahuman5196! I believe we would still pay the reporting bonus for this as reporting these behaviors has influenced our discussions and moved things forward.

I'll update this GH for that and get the reporting job up for @adeel0202.

[strepanier03]

@adeel0202 - I have hired you for reporting job in Upwork, please accept and I'll check in about paying later.

[melvin-bot]

Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[strepanier03]

@abekkala - I'm heading out on sabbatical but couldn't finish paying this before I went. We're just waiting for @adeel0202 to accept the job offer for the reporting bonus in Upwork and then we can pay and close the GH.

Thank you for wrapping this up!

[adeel0202]

@strepanier03, I have accepted.

[abekkala]

@adeel0202 payment sent and contract ended - thank you! 🎉

[melvin-bot]

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

• Fix: Unmatched parent report ID and report action ID #20574

If no regressions arise, payment will be issued on 2023-07-05. 🎊

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

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[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:

• [@allroundexperts / @abdulrahuman5196] The PR that introduced the bug has been identified. Link to the PR:
• [@allroundexperts / @abdulrahuman5196] 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:
• [@allroundexperts / @abdulrahuman5196] 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:
• [@allroundexperts / @abdulrahuman5196] Determine if we should create a regression test for this bug.
• [@allroundexperts / @abdulrahuman5196] 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.
• [@abekkala] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: