[HOLD for payment 2023-11-01] [$1000] Web - LHN still displays "Deleted message" although we deleted comment and its thread completely

[kbecciv]

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. Go to any chat report
2. Type a comment
3. Click "Reply on thread" context menu of this comment
4. Type a reply in that thread
5. Remove the parent comment of that thread
6. Verify that the LHN is displays "Deleted message" in original chat report, and in original chat report, we can still see the comment in step 2, with marked as deleted message
7. Delete the reply comment in step 4
8. Go to the original chat report, the comment in step 2 is disappear

Expected Result:

LHN should show the latest visible message.

Actual Result:

LHN still displays "Deleted message" although we deleted comment and its thread completely, and the comment is already disappear in the chat.

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.43-7
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-07-22.at.06.46.55.mov

Recording.3854.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01746b90b47c15e650
• Upwork Job ID: 1694864004183359488
• Last Price Increase: 2023-09-15
• Automatic offers:
• hoangzinh | Reporter | 26781400
• rojiphil | Contributor | 27299208

[melvin-bot]

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

[s-alves10]

It looks like this is an intentional behavior, but I think this is an inconsistency.

[dukenv0307]

hold #19722

[hoangzinh]

It looks like this is an intentional behavior, but I think this is an inconsistency. This is from the PR

This issue is closely related to #23401, and I think we should fix in one issue

I'm unsure if This issue is closely related to #23401, because in #23401, we only delete the main/original comment of a thread. While in this issue, we delete both original comment + comment in thread.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[johncschuster]

@cead22 / @marcaaron it looks like you are both assigned the above-linked issue. Can you investigate whether the linked PR caused a regression?

[marcaaron]

See my comment here @johncschuster #19722 (comment)

[johncschuster]

Awesome. Thanks, @marcaaron! In that case, should we put this one on hold and wait until #19722 has been resolved to see if this behavior is still an issue, or are you confident #19722 will resolve this issue? If yes, I would close this issue.

[hoangzinh]

Although I think they're different issues, and we don't know what is the new approach to fix #19722 there. But If we think they're almost same, I think it's better if we put this card in on-hold for #19722. So we don't need to reopen this issue again if someone report a new duplicated issue.

[marcaaron]

Tbh I'm only involved because I am the deployer this week and something loosely related needed to be reverted to unblock the deploy. I would suggest seeking out and asking the original authors who introduced the regression directly.

[marcaaron]

HOLD does sound like a reasonable plan to me though since deleting the comment thread is not that critical of an action that people will take and doesn't have a great effect on the biz.

[s-alves10]

#19722 (comment)

[rojiphil]

Proposal

Updated proposal to include steps (4) and (5) for handling display of deleted parent action in report screen and few other corner cases.

[johncschuster]

@flodnv @rushatgabhane @dukenv0307, it looks like the PR linked to this issue may have caused a regression. Can you investigate that and leave a comment here so we know how best to handle this issue? Should we close it in favor of #19722?

[rushatgabhane]

@johncschuster yes we can hold this issue on #19722 and make it weekly or something

[johncschuster]

Sweet! Thanks, @rushatgabhane!

[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.90-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix for [Deleted message] keeps showing up #30131

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

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:

• @rojiphil requires payment offer (Contributor)
• @allroundexperts does not require payment (Eligible for Manual Requests)
• @hoangzinh requires payment offer (Reporter)

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

[rojiphil]

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

@johncschuster
Gentle bump on payments here

[melvin-bot]

@rojiphil, @johncschuster, @chiragsalian, @allroundexperts Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[johncschuster]

Payment Summary:

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉 (Comment)

@rojiphil requires payment offer (Contributor) - $1500 - PAID
@allroundexperts does not require payment (Eligible for Manual Requests) - $1500
@hoangzinh requires payment offer (Reporter) - $250 - PAID

[johncschuster]

@hoangzinh can you accept the offer on Upwork? I'll get payment issued to you once that's done.

[hoangzinh]

Accepted. Thanks @johncschuster

[melvin-bot]

@rojiphil, @johncschuster, @chiragsalian, @allroundexperts Huh... This is 4 days overdue. Who can take care of this?

[johncschuster]

I've issued payment to @hoangzinh. @allroundexperts, can you complete the BZ checklist when you get a moment? Thank you!

[melvin-bot]

@rojiphil, @johncschuster, @chiragsalian, @allroundexperts Eep! 4 days overdue now. Issues have feelings too...

[allroundexperts]

Checklist

1. I think this logic has been touched too many times and its hard to find the exact PR causing the issue.
2. N/A
3. A Slack discussion is not needed IMO. I can't think of any addition to the checklist that could have avoided this.
4. A regression test would be an overkill since this is such an edge case. However, If the team disagrees, then the following are the test steps.

Regression Test

1. Go to any chat report and type a comment.
2. Click "Reply on thread" context menu of this comment.
3. Type a reply in that thread
4. Remove the parent comment of that thread
5. Verify that the LHN is displays "Deleted message" in original chat report, and in original chat report, we can still see the comment in step 2, with marked as deleted message
6. Delete the reply comment in step 4
7. Verify that in LHN, the latest message is shown instead of the text Deleted message.

Do we 👍 or 👎 ?

[chiragsalian]

I personally would prefer if we add that as a regression test step. Because if this breaks again its easier to fix if its found sooner vs later.

[JmillsExpensify]

$1,500 payment approved for @allroundexperts based on this comment.

[melvin-bot]

@rojiphil, @johncschuster, @chiragsalian, @allroundexperts Eep! 4 days overdue now. Issues have feelings too...