[$500 BONUS PAYOUT] [$1000] Web - Chat - '# Reply' is not displayed after replying to root message without network

[kbecciv]

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

---

Issue found when executing PR #19902

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Disconnect network.
3. Go to any chat.
4. Right click on the message > Reply in thread.
5. Send a reply.
6. Return to the original chat in Step 3.

Expected Result:

'1 Reply' is displayed under the parent message.

Actual Result:

'1 Reply' is not displayed under the parent message.

Workaround:

Unknown

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.24.4

Reproducible in staging?: yes

Reproducible in production?: yes

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

Bug6082139_bandicam_2023-06-06_20-52-04-009.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013f046c6eb69a6d32
• Upwork Job ID: 1670922075208019968
• Last Price Increase: 2023-07-03

[melvin-bot]

Triggered auto assignment to @anmurali (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

[melvin-bot]

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

[anmurali]

Reproduced, assigned for external contributors to pick up!

[melvin-bot]

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

[anmurali]

Reproduced

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[asadath1395]

Proposal

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

Chat - '# Reply' is not displayed after replying to root message without network

What is the root cause of that problem?

We are not adding the thread related data to optimistic data. When we add a normal comment, the following is the API response we get, if you see the response there are only 2 items in the merge related items in the array.

but when users reply as a thread, the API response contains 3 merge related items in the array

Whereas in the code for optimistic data, we don't even have the 3rd merge related item Ref: https://github.com/Expensify/App/blob/main/src/libs/actions/Report.js#L260 which is how the replies are shown.
Ref: https://github.com/Expensify/App/blob/main/src/pages/home/report/ReportActionItem.js#L323

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

If we add the 3rd merge item related to replies in thread to the optimistic data, it should work as expected
We need to add a new param to this function called parentReport which will be default by null and it will have all the data required to 3rd merge related item in the optimistic data. We will add the following merge item related to replies in the same function

if (parentReport) {
      optimisticData.push({
          onyxMethod: Onyx.METHOD.MERGE,
          key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${parentReport.parentReportID}`,
          value: ReportUtils.buildOptimisticReplyToThreadCommentReportAction(parentReport),
      });
  }

the buildOptimisticReplyToThreadCommentReportAction function will look like this

function buildOptimisticReplyToThreadCommentReportAction(parentReport) {
    return {
        [parentReport.parentReportActionID]: {
            childCommenterCount: 1,
            childLastVisibleActionCreated: DateUtils.getDBTime(),
            childOldestFourEmails: parentReport.action.childCommenterCount || 1,
            childOldestFourAccountIDs: parentReport.action.childOldestFourEmails,
            childVisibleActionCount: parentReport.action.childOldestFourAccountIDs,
        },
    };
}

Also we will have to update the onSubmitComment function here like this

onSubmitComment(text) {
    let report;
    if (this.props.report.parentReportID) {
        report = this.props.report;
    }
    Report.addComment(getReportID(this.props.route), text, report);
}

What alternative solutions did you explore? (Optional)

NA

[melvin-bot]

@anmurali @thesahindia this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[thesahindia]

if we add the 3rd merge item related to replies in thread to the optimistic data, it should work as expected

@asadath1395, can you add more details about the changes you will be making to achieve this?

[asadath1395]

@thesahindia I have updated with the implementation details. Let me know if more info has to be added. Thanks for reviewing the proposal

[thesahindia]

@0xmiroslav is going take the C+ role here. Please assign him @anmurali.

[0xmiros]

@dukenv0307 please check if our PR fixes this issue as well.

[dukenv0307]

@0xmiroslav Yes. With updating optimisticData of parent report action we can fix this issue.

[0xmiros]

#20762 is now deployed to staging. Waiting for production and regression period

[anmurali]

It's now in production! Taking this off-hold.

[melvin-bot]

Triggered auto assignment to @jliexpensify (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

[anmurali]

Rotating assignment as I am OOO

[melvin-bot]

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

[jliexpensify]

@0xmiroslav would love for you to verify, but I think this is now fixed?

2023-08-01_11-30-56.mp4

[0xmiros]

@jliexpensify yes it's fixed. We might be eligible for some compensation for one PR fixing both issues?

[jliexpensify]

@0xmiroslav - I can certainly put this forth to the team. Can you clarify who "we" are, and how much you feel you deserve? Thanks!

[0xmiros]

Can you clarify who "we" are, and how much you feel you deserve?

C: @dukenv0307, C+: me

We had lots of discussions in PR to reach the final optimistic data.
We got paid 1k in #19445 so appreciate if 50% is eligible here, which is equivalent to urgency bonus. No payment is also fine.

[jliexpensify]

Ok cool, let me bring this one up internally for a second set of eyes!

[melvin-bot]

@jliexpensify, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

[jliexpensify]

I'm so sorry, this one dropped off my radar. Reviewing the Slack thread now.

[jliexpensify]

Ok so here's where we landed: https://expensify.slack.com/archives/C01SKUP7QR0/p1690858683561199

Payment Summary

• Contributor: @dukenv0307 $500 (special bonus)
• C+: @0xmiroslav $500 (special bonus)

@0xmiroslav does the checklist need to be completed here?

EDIT: New Upworks job

[0xmiros]

@0xmiroslav does the checklist need to be completed here?

I don't think so. This is more like feature request. We never implemented optimistic data before.

[jliexpensify]

Everyone paid and job closed.