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