[HOLD for payment 2024-10-11][$500] DeleteComment not working as expected

[roryabraham]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Issue reported by: @shubham1206agra @rayane-djouah
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1711678317008389?thread_ts=1711571474.268739&cid=C01GTK53T8Q

Action Performed:

1. Add a comment
2. Create a thread in the same comment
3. Add another comment inside the thread
4. Try to delete the comment in the thread
5. Observe that the DeleteComment API never runs inside the network tab
6. Try to go to the same thread after navigating away

Expected Result:

The comment deleted in step 4 should be gone.

Actual Result:

The comment deleted in step 4 remains.

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

https://expensify.slack.com/archives/C01GTK53T8Q/p1712034411209499?thread_ts=1711571474.268739&cid=C01GTK53T8Q

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014a6349d3b575268d
• Upwork Job ID: 1775247417597710336
• Last Price Increase: 2024-04-02
• Automatic offers:
• shubham1206agra | Contributor | 0
• bernhardoj | Contributor | 0

Issue Owner

Current Issue Owner: @slafortune

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~014a6349d3b575268d

[melvin-bot]

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

[bernhardoj]

Proposal

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

Delete comment request is never processed.

What is the root cause of that problem?

We include delete comment as part of the conflicting request and shouldIncludeCurrentRequest is also true (except thread parent)

App/src/libs/actions/Report.ts

Lines 1258 to 1267 in 879378f

	getConflictingRequests: (persistedRequests) => {
	const conflictingCommands = (
	isDeletedParentAction
	? [WRITE_COMMANDS.UPDATE_COMMENT]
	: [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.UPDATE_COMMENT, WRITE_COMMANDS.DELETE_COMMENT]
	) as string[];
	return persistedRequests.filter((request) => conflictingCommands.includes(request.command) && request.data?.reportActionID === reportActionID);
	},
	handleConflictingRequest: () => Onyx.update(successData),
	shouldIncludeCurrentRequest: !isDeletedParentAction,

so when we do a delete comment, it conflicts with itself and is removed from the request list.

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

From my understanding, the purpose of getConflictingRequests is:

1. Remove duplicate requests, such as reconnect app (which is achieved previously with idem key)
2. Remove both add (optionally edit) and delete request of a same action as they cancel out each other

The 1st point works fine, but not with the 2nd. There are 2 issues with the 2nd point:

1. The delete comment is always removed from the request list, but it should only be removed when there is an add request.
2. If we add a comment while online, then edit and delete it while offline, both edit and delete command is removed from the request list and we optimistically remove the message.

So, here is what I propose:

1. Replace shouldIncludeCurrentRequest with shouldExcludeCurrentRequestWhenConflict, the value stays the same
2. Remove DELETE from the conflicting request list and conditionally add UPDATE if the action.isOptimisticAction is true
   

   App/src/libs/actions/Report.ts

   Line 1262 in 879378f

   : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.UPDATE_COMMENT, WRITE_COMMANDS.DELETE_COMMENT]

3. In PersistedRequest.save, don't include the current request (requestToPersist) to the array yet, this means we don't need potentiallyConflictingRequests anymore. We will pass requests to getConflictingRequests
   

   App/src/libs/actions/PersistedRequests.ts

   Lines 20 to 21 in 879378f

   	function save(requestToPersist: Request) {
   	const requests = [...persistedRequests, requestToPersist];

4. Create a new variable called hasConflict with an initial value of false.
5. Set it to true if conflictingRequests is not empty
6. If shouldExcludeCurrentRequestWhenConflict is false or there is no conflict, add the current request to the request list

diff

diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts
index 8adb598246..7e671e8cce 100644
--- a/src/libs/actions/PersistedRequests.ts
+++ b/src/libs/actions/PersistedRequests.ts
@@ -18,16 +18,15 @@ function clear() {
 }
 
 function save(requestToPersist: Request) {
-    const requests = [...persistedRequests, requestToPersist];
+    const requests = [...persistedRequests];
+    let hasConflict = false;
 
     // identify and handle any existing requests that conflict with the new one
-    const {getConflictingRequests, handleConflictingRequest, shouldIncludeCurrentRequest} = requestToPersist;
+    const {getConflictingRequests, handleConflictingRequest, shouldExcludeCurrentRequestWhenConflict} = requestToPersist;
     if (getConflictingRequests) {
-        // Get all the requests, potentially including the one we're adding, which will always be at the end of the array
-        const potentiallyConflictingRequests = shouldIncludeCurrentRequest ? requests : requests.slice(0, requests.length - 1);
-
         // Identify conflicting requests according to logic bound to the new request
-        const conflictingRequests = getConflictingRequests(potentiallyConflictingRequests);
+        const conflictingRequests = getConflictingRequests(requests);
+        hasConflict = conflictingRequests.length > 0;
 
         conflictingRequests.forEach((conflictingRequest) => {
             // delete the conflicting request
@@ -41,11 +40,15 @@ function save(requestToPersist: Request) {
         });
     }
 
+    if (!shouldExcludeCurrentRequestWhenConflict || !hasConflict) {
+        requests.push(requestToPersist)
+    }
+
     // Don't try to serialize conflict resolution functions
     persistedRequests = requests.map((request) => {
         delete request.getConflictingRequests;
         delete request.handleConflictingRequest;
-        delete request.shouldIncludeCurrentRequest;
+        delete request.shouldExcludeCurrentRequestWhenConflict;
         return request;
     });
 
diff --git a/src/libs/actions/Report.ts b/src/libs/actions/Report.ts
index 775d2459b0..0ab400d523 100644
--- a/src/libs/actions/Report.ts
+++ b/src/libs/actions/Report.ts
@@ -1314,12 +1314,12 @@ function deleteReportComment(reportID: string, reportAction: ReportAction) {
                 const conflictingCommands = (
                     isDeletedParentAction
                         ? [WRITE_COMMANDS.UPDATE_COMMENT]
-                        : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.UPDATE_COMMENT, WRITE_COMMANDS.DELETE_COMMENT]
+                        : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, reportAction.isOptimisticAction ? WRITE_COMMANDS.UPDATE_COMMENT : '']
                 ) as string[];
                 return persistedRequests.filter((request) => conflictingCommands.includes(request.command) && request.data?.reportActionID === reportActionID);
             },
             handleConflictingRequest: () => Onyx.update(successData),
-            shouldIncludeCurrentRequest: !isDeletedParentAction,
+            shouldExcludeCurrentRequestWhenConflict: !isDeletedParentAction,
         },
     );
 }

[shubham1206agra]

@bernhardoj Can you confirm that your proposal fixes #39218?

[ntdiary]

Hi, @shubham1206agra, are you also interested in taking over this issue? I'll be offline for the next 4 days, and won't be able to handle new issues. :)

[shubham1206agra]

Ok no problem

[melvin-bot]

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

[slafortune]

Thanks all! I've update the assignments 👍

[roryabraham]

next step - waiting for an approved proposal. @bernhardoj's proposal LGTM, just want to answer @shubham1206agra's question first 👍🏼

I'd also love to see us add tests to cover the DeleteComment conflict resolution flow specifically 🙇🏼

[bernhardoj]

I have checked and it's a different issue unrelated to the conflict resolution logic. I have posted a proposal there explaining the issue.

[shubham1206agra]

@bernhardoj Sorry, but one last request. Can you check if any one of the proposal fixes #39307?

[bernhardoj]

Yes, it will fix #39307

Screen.Recording.2024-04-05.at.14.22.18.mov

[shubham1206agra]

@bernhardoj's proposal looks good.

🎀 👀 🎀 C+ Reviewed

[melvin-bot]

This issue has not been updated in over 15 days. @slafortune, @roryabraham, @bernhardoj, @shubham1206agra eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[slafortune]

This is waiting on Rory, so I don't feel the need to add another BZ person to this.
I'll be out until 8/21 and will check in on this then.

[slafortune]

@roryabraham I am not able to recreate this any longer - following the steps above I was able to successfully delete the comment on the thread - still gone after navigating away and back.

[shubham1206agra]

@slafortune Can I get paid for #39432 (comment)?

[slafortune]

Whoops, I didn't mean to close, I meant to clarify that this isn't reproducible and that we are still waiting on the linked PR to finalize payments as noted here - #39432 (comment)

[melvin-bot]

This issue has not been updated in over 15 days. @slafortune, @roryabraham, @bernhardoj, @shubham1206agra eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[roryabraham]

We've been actively developing/reviewing in #47913, seems we are close

[roryabraham]

#47913 was deployed to prod today. Not sure why this didn't get marked as awaiting payment, doing that manually