[HOLD for payment 2024-04-05] [$500] Avatar of user who sent message disappears after deleting comment

[kavimuru]

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: 1.4.56-0
Reproducible in staging?: y
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

1. Go to https://staging.new.expensify.com/
   and log in as user A
2. In incognito log in as user B
3. As user A, open the chat with user B and send a message
4. As user B, send two messages
5. As user B, delete the last sent message

Expected Result:

The avatar of user B is still displayed next to the sent message

Actual Result:

The avatar of user B disappears after deleting a message

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

Bug6423493_1711131226128.Recording__333.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010d8a4fbe15a51d30
• Upwork Job ID: 1771294914072977408
• Last Price Increase: 2024-03-22
• Automatic offers:
• Pujan92 | Reviewer | 0
• gijoe0295 | Contributor | 0

Issue Owner

Current Issue Owner: @anmurali

[melvin-bot]

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

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @bondydaa (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[kavimuru]

@bondydaa 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.

[kavimuru]

We think this bug might be related to #vip-vsb

[gijoe0295]

Proposal

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

The avatar of user B disappears after deleting a message

What is the root cause of that problem?

Regression from #30269. Before that PR, we filtered out report actions by shouldReportActionBeVisible which also filtered deleted actions. But after that, we retrieved all report actions.

Later in ReportActionsList, we did not take into account the case when the previous action is deleted when evaluating isConsecutiveActionMadeByPreviousActor. So even though the previous message was deleted, displayAsGroup of the second message is still true, causing it to display without sender info.

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

I think for evaluating consecutive actions, we should only take into account visible actions. That's also what we've been doing previously (see RCA for code ref). We should use sortedVisibleReportActions as a param for isConsecutiveActionMadeByPreviousActor.

What alternative solutions did you explore? (Optional)

Add isDeletedAction(previousAction) check to the above function.

if (isDeletedAction(previousAction)) {
    return false;
}

or we can use shouldReportActionBeVisible for previous action in the above function.

[bondydaa]

👀 looking into this

[bondydaa]

I'm sus of this PR #36747, reverting locally and testing.

[bondydaa]

🚫 nope nevermind still reproducible

[bondydaa]

heh was probably the big comment linking PR #30269 then 😅 but i don't know that we're going to revert that.

I'm going to add external b/c I think the existing proposal makes sense, i'm a little confused how this works without anything checking for if the comment is deleted or not (ie: this should probably happen on production as well but 🤷).

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

Offer link
Upwork job

[melvin-bot]

📣 @gijoe0295 🎉 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 📖

[bondydaa]

Great thank you for the discussion and helping get to the bottom of all that 🙇.

I think we should still block the deploy for this one since it is pretty bad to have a message from a different user end up getting shown as if it was from the wrong user.

[bondydaa]

merged the PR, requesting a CP now.

[bondydaa]

@kavimuru can you retest this and confirm it's fixed, CP should have been completed about 40 minutes ago so should be ready on staging.

[yuwenmemon]

Can confirm this does not happen on staging anymore. Removing the blocker label

[gijoe0295]

PR is on production.

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

• Fix: Sender info disappears when delete one of consecutive messages #38945

If no regressions arise, payment will be issued on 2024-04-05. 🎊

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

• @Pujan92 requires payment automatic offer (Reviewer)
• @gijoe0295 requires payment automatic 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:

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

[gijoe0295]

@bondydaa PR was on production 2 weeks ago without any regression. Please handle payment when you have time.

[gijoe0295]

@bondydaa Can you complete payment? @Pujan92 Can you complete the checklist? We are 3 weeks overdue but Melvin is not working.

[bondydaa]

@anmurali will handle payment

[Pujan92]

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:

• [@Pujan92] The PR that introduced the bug has been identified. Link to the PR: HIGH: (Comment linking: Step 4) Main "glue" PR for Comment Linking #30269
• [@Pujan92] 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: https://github.com/Expensify/App/pull/30269/files#r1566259198
• [@Pujan92] 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: NA
• [@Pujan92] Determine if we should create a regression test for this bug. Yes
• [@Pujan92] 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.

Regression Test Steps

1. Send consecutive messages from User A to User B and vice-versa
2. Delete any of the message
3. Verify that the senders info shows correctly with the messages

Do you agree 👍 or 👎 ?

[gijoe0295]

Awaiting @anmurali handling here.

[anmurali]

Paid and added regression test.