[HOLD for payment 2024-04-15] [$500] Task - Chat messages are out of order after reassigning task, sometimes beginning of chat is duplicated

[kbecciv]

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.53-2
Reproducible in staging?: y
Reproducible in production?: y
Issue reported by: Applause - Internal Team

Action Performed:

1. Open the app and log into an account where you have multiple chats
2. Navigate to a user from the recent contacts
3. Tap the "+" > Assign task
4. Enter the task name and proceed to the next screen
5. Tap "Assignee" and search for a user you have not messaged yet
6. Assign the task to that user
7. Tap the "Assignee" again and reassign the task to the same user you are sharing it with
8. Create the task

Expected Result:

The task is displayed as the last message in the report

Actual Result:

A duplicated beginning of the report is displayed, sometimes task is shown as the first message

Workaround:

n/a

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

Bug6415381_1710533895456.video_2024-03-15_16-16-07.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01479bcdfebb5e0089
• Upwork Job ID: 1770147572055535616
• Last Price Increase: 2024-03-26
• Automatic offers:
• hungvu193 | Reviewer | 0

[melvin-bot]

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

[kbecciv]

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

[kbecciv]

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

[ahmedGaber93]

Proposal

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

Task - Beginning of chat shown after reassigning task from user you have no chat to recent user

What is the root cause of that problem?

when we select assignee we call setAssigneeChatReport.

App/src/libs/actions/Task.ts

Line 660 in e40fba1

setAssigneeChatReport(chatReport);

which merge the value with the old value.

App/src/libs/actions/Task.ts

Lines 623 to 625 in e40fba1

	function setAssigneeChatReport(chatReport: OnyxTypes.Report) {
	Onyx.merge(ONYXKEYS.TASK, {assigneeChatReport: chatReport});
	}

the root cause is the merge leed to merge ReportUtils.buildOptimisticChatReport for new user with ReportUtils.getChatByParticipants for user chated before and this add optimistic fields like isOptimisticReport to the old report

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

we need to reset the old value of assigneeChatReport before merge the new value here.

App/src/libs/actions/Task.ts

Lines 623 to 625 in e40fba1

	function setAssigneeChatReport(chatReport: OnyxTypes.Report) {
	Onyx.merge(ONYXKEYS.TASK, {assigneeChatReport: chatReport});
	}

we can do that by merge it to null then merge it to the new value.

Onyx.merge(ONYXKEYS.TASK, {assigneeChatReport: null}).then(() => Onyx.merge(ONYXKEYS.TASK, {assigneeChatReport: chatReport}));

What alternative solutions did you explore? (Optional)

[nkdengineer]

Proposal

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

A duplicated beginning of the report is displayed, sometimes task is shown as the first message

What is the root cause of that problem?

When we select an assignee that is a new user, we create an optimistic report here and set it to assigneeChatReport. After we re-select a recent contact, another chat report is set to assigneeChatReport but other fields like isOptimisticReport, pendingFields are in-correct because we merge from old assignee report.

App/src/libs/actions/Task.ts

Line 660 in c915e7f

setAssigneeChatReport(chatReport);

And then a created action is created here.

App/src/libs/ReportUtils.ts

Lines 4813 to 4814 in c915e7f

	if (assigneeChatReport?.isOptimisticReport && assigneeChatReport.pendingFields?.createChat !== CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) {
	optimisticChatCreatedReportAction = buildOptimisticCreatedReportAction(assigneeChatReportID);

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

Instead of storing assignReport in task data, we can store the assigneeChatReportID which is the ID of chat report and then get the correct data from Onyx to check here.

App/src/libs/ReportUtils.ts

Lines 4813 to 4814 in c915e7f

	if (assigneeChatReport?.isOptimisticReport && assigneeChatReport.pendingFields?.createChat !== CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) {
	optimisticChatCreatedReportAction = buildOptimisticCreatedReportAction(assigneeChatReportID);

I checked all use of task.assigneeChatReport and saw that it's only used when we create the task. So we can only store the reportID of chat report and then get the report data from Onyx correctly.

What alternative solutions did you explore? (Optional)

Updated the chat report with updating the optimistic field correctly if the chatReport is an exist report

 setAssigneeChatReport({
    ...chatReport,
    isOptimisticReport: chatReport.isOptimisticReport,
    pendingField: chatReport.pendingField,
    pendingAction: chatReport.pendingAction
}); 

App/src/libs/actions/Task.ts

Line 660 in c915e7f

setAssigneeChatReport(chatReport);

[sakluger]

Updated the issue title and description for clarify.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01479bcdfebb5e0089

[melvin-bot]

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

[hungvu193]

Reviewing today

[hungvu193]

I'm not able to reproduce this one on latest main :/

Screen.Recording.2024-03-20.at.16.35.30.mov

[nkdengineer]

Tap "Assignee" and search for a user you have not messaged yet

@hungvu193 You should search and select a new user first that is important step to reproduce this bug.

[hungvu193]

Tap "Assignee" and search for a user you have not messaged yet

@hungvu193 You should search and select a new user first that is important step to reproduce this bug.

Thank you, I'll try to reproduce today

[hungvu193]

Ok, I can reproduce.

Screen.Recording.2024-03-23.at.22.22.11.mov

But I couldn't reproduce the issue once it already happened with that report. Here's the steps to test:

1. Open the app and log into an account where you have multiple chats
2. Navigate to a user from the recent contacts
3. Tap the "+" > Assign task
4. Enter the task name and proceed to the next screen
5. Tap "Assignee" and search for a user you have not messaged yet
6. Assign the task to that user
7. Tap the "Assignee" again and reassign the task to the same user you are sharing it with
8. Create the task => Saw the issue
9. Reload the app
10. Repeat step 3 to 8.

My question is can you guys still the see issue with my above reproduction steps? If not, why did it only happen the first time, since both proposals pointed out that we're using wrong optimistic data. Thank you.

[ahmedGaber93]

@hungvu193

why did it only happen the first time

App/src/libs/actions/Task.ts

Lines 636 to 638 in e40fba1

	chatReport = ReportUtils.getChatByParticipants([assigneeAccountID]);
	if (!chatReport) {
	chatReport = ReportUtils.buildOptimisticChatReport([assigneeAccountID]);

this because we build optimistic chat report only if chatReport gotten by getChatByParticipants not found.
And after that the chatReport = ReportUtils.getChatByParticipants is already exist, so no need to build optimistic chat again

[nkdengineer]

@hungvu193 After the first time, isOptimisticReport of the chat report is updated to false in successData and then if we repeat the step again, isOptimisticReport will be updated correctly in assigneeChatReport because isOptimisticReport is updated correct.

App/src/libs/ReportUtils.ts

Line 5003 in eba217f

isOptimisticReport: false,

[melvin-bot]

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

[hungvu193]

Thank you both and sorry for late reply, I was sick last few days, but it's close to select proposal.

[hungvu193]

I still can reproduce

trim.777E0B10-577E-43BB-B3A4-4E0477815FD4.MOV

Tips: I used high traffic account and choose a random account on the list that I've never chatted before

[ahmedGaber93]

I also can reproduce in the last main.

Screen.Recording.2024-03-28.at.3.19.22.AM.mov

[nkdengineer]

In this case, the report data in Onyx is correctly, we only have the problem with assigneeChatReport. it's used to create new created reportAction if the report is new report and doesn't update anything to report data so we only need to verify that this will display correct in assign task flow and the logic to create report action will be correct.

To not have any concern about saving assigneeChatReport, we also can do with my main solution to only store assigneeChatReportID and then get the correct data in Onyx.

[srikarparsi]

Cool just looked over everything. I also like @nkdengineer alternate solution since it's more straightforward and is better for performance without the double merge.

[melvin-bot]

📣 @hungvu193 🎉 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]

📣 @nkdengineer You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

[nkdengineer]

@hungvu193 The PR is here.

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

• Fix created action is duplicate when assigning a task #39152

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

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

• @hungvu193 requires payment automatic offer (Reviewer)
• @nkdengineer requires payment (Needs manual offer from BZ)

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

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

[sakluger]

Summarizing payment on this issue:

Contributor: @nkdengineer $500, sent offer via Upwork (https://www.upwork.com/nx/wm/offer/101873781)
Contributor+: @hungvu193 $500, paid via Upwork

@hungvu193 could you please complete the BZ checklist?

[sakluger]

Paid @nkdengineer, just waiting on the checklist now.

[hungvu193]

• The PR that introduced the bug has been identified. Link to the PR: Allow optimistic assignee chat reports #22778
• 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/22778/files#r1566611957
• 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: N/A
• Determine if we should create a regression test for this bug: No

This is an edge case, so I don't think we need regression test for this one.

[sakluger]

Thanks!