[HOLD for payment 2024-11-11] [HOLD for payment 2024-11-05] [$250] De-dupe AddComment, EditComment, and DeleteComment requests

[roryabraham]

Problem

When we make unnecessary network requests, it slows down the app and contributes to higher traffic on our servers. In particular:

• if you are offline and you serialize multiple EditComment requests, only the last one will take effect
• if you serialize both an AddComment and any number of EditComment requests, the result would be the same as one AddComment with the content of the final EditComment
• if you serialize an AddComment followed by a DeleteComment, the end result is the same as not sending any network requests at all
• if you serialize an EditComment request followed by a DeleteComment, the end result is the same as only sending the DeleteComment

Solution

De-dupe the network requests!

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021848400026446902359
• Upwork Job ID: 1848400026446902359
• Last Price Increase: 2024-10-21
• Automatic offers:
• dukenv0307 | Reviewer | 104528746
• c3024 | Contributor | 104528756

Issue Owner

Current Issue Owner: @bfitzexpensify

[roryabraham]

cc @gedu

[gedu]

Hey, I'm Edu from Callstack, I can take this one

[shubham1206agra]

There are several obstacles to achieve this.

1. if you are offline and you serialize multiple EditComment requests, only the last one will take effect

The obstacle here is that if the request fails after replace call, it will show wrong comment.

2. if you serialize an AddComment followed by a DeleteComment, the end result is the same as not sending any network requests at all

This is tricky as we are now reducing the queue size.

[roryabraham]

The obstacle here is that if the request fails after replace call, it will show wrong comment.

isn't that the case today with any EditComment request? Or is there something I'm missing?

[roryabraham]

Going on parental leave. @gedu @mountiny @shubham1206agra I leave this in your capable hands. My parting note is to please use as many automated tests to cover this change as possible 👍🏼

[gedu]

I just made a PR for OpenApp, I will start taking a look at this

[gedu]

@shubham1206agra I'm making me a diagram, and I was thinking, what about a reaction, deleteComment has to delete addEmojiReaction too. Because now if you edit -> edit -> add reaction -> delete, you will see a request for each in that order. When we should see only a delete request, right?

[shubham1206agra]

Yes that is true

[gedu]

@shubham1206agra also just to make sure, if I edit -> create a thread -> add message in that thread -> delete original message, this is the final state. In such case I only delete the editComment the thread should be created and any message in it, right?

This is how it looks now

[shubham1206agra]

Yes

[shubham1206agra]

There's another case
Add comment -> Add comment in thread -> Delete parent comment -> Delete thread comment

No request should be made in this case

[gedu]

There's another case
Add comment -> Add comment in thread -> Delete parent comment -> Delete thread comment

No request should be made in this case

Ohh, yeah, so nothing happens, no new thread is created and no new message is added into that thread. Cool, I'm adding this case too

[gedu]

Just to make sure, these cases can happen at any time, not just offline. Given that the queue is being processed sequentially, the delete could occur at any time, and I have to prevent those messages from being sent, right?

[gedu]

@shubham1206agra Also I was thinking, if I create a thread -> message -> message -> delete last message -> delete thread.
I'm not sure if I found a bug because my chat breaks. I lose my Task, and I should have two [Deleted Message] messages.

delete_message.mp4

[shubham1206agra]

Can you report the bug in slack?

[gedu]

Can you report the bug in slack?

How can I do it? I don't have access to the prompt /bug

[mountiny]

You can just copy the template and fill it manually

[melvin-bot]

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

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

[gedu]

if you serialize both an AddComment and any number of EditComment requests, the result would be the same as one > AddComment with the content of the final EditComment

@mountiny I did the second item, and I'm seeing this behaviour, for a couple of seconds, until the response is back and then it rerenders, you can see the edit label next to the message. Is it ok?

Given that the optimisticData is applied before the request reaches the SequentialQueue, it is showing, I can try to add a rollback case for it, but want to confirm if we want or not to show the edit label. The rollback will be applied as soon as the "conflict" is resolved so probably the user won't see the edit label while being of line, will be like he just updated the comment.

updateComment_addComment.mp4

[mountiny]

@gedu I think from UX it would be best not to show it but I would not block on this if its too complex to achieve not showing edited label

[gedu]

if you serialize both an AddComment and any number of EditComment requests, the result would be the same as one > > AddComment with the content of the final EditComment

Created a new PR: #51422 to add the last point, it depends on #50919, so would be good to merge that one first

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

• Dedupe multiple UpdateComment requests #51149

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

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

• @gedu does not require payment (Contractor)
• @zirgulis does not require payment (Contractor)
• @c3024 requires payment automatic offer (Contributor)
• @dukenv0307 requires payment automatic offer (Reviewer)

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

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

[mountiny]

2 PRs been tested (#50919, #51422) and we merged the one that had more stuff in it. As a result we will pay $250 to both @c3024 and to @dukenv0307

[melvin-bot]

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

• Add comment text update by edit comment #51422

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

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

• @gedu does not require payment (Contractor)
• @zirgulis does not require payment (Contractor)
• @c3024 requires payment automatic offer (Contributor)
• @dukenv0307 requires payment automatic offer (Reviewer)

[melvin-bot]

@c3024 / @dukenv0307 @bfitzexpensify The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[dukenv0307]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR: This is the new implementation so there's no offending PR here
• 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: N/A
• 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. Yes
• 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:

• Delete No requests:

1. Go to offline
2. send a message
3. edit the message
4. add a emoji reaction
5. delete the message
6. Go to online
7. No requests should be sent

• No delete Threads

1. Go to offline
2. Send a message
3. create a thread
4. delete that message
5. Go to online
6. All the request must be sent

• Add + Update:

1. Go offline
2. Send a new message
3. Edit that new message
4. Go back online
5. Only AddComment request with the Edited text should be sent

• Multiple edit requests:

1. Turn off internet connection
2. Edit a single comment, save it.
3. Repeat step 2 multiple times.
4. Turn on internet connection
5. Observe a single UpdateCommentsent to BE.

Do we 👍 or 👎

[bfitzexpensify]

Thanks all - payments complete.