App: [HOLD for payment 2024-04-25] [$500] DeleteComment not working as expected

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

About this issue

  • Original URL
  • State: open
  • Created 3 months ago
  • Comments: 34 (21 by maintainers)

Most upvoted comments

No worries, I don’t think you or @bernhardoj did anything wrong - you’ve both been great. ❤️ I just decided to bring this internal because we have a fireroom. I agree you should both be compensated fully, and I’d appreciate your help reviewing the new PR to ensure that it ends up fixing the problem(s) without introducing new ones. :teamwork:

Taking this internal over here for urgency, but @bernhardoj should still be paid since they provided much of the solution and opened a PR, which might have been merged already if I had reviewed faster.

I should also be compensated fully as it was not blocking from my side but @bernhardoj’s side.

reviewed the PR

PR is ready

cc: @shubham1206agra

@ishpaul777 Just put it on hold. We can retest once this issue is fixed.

Thanks all! I’ve update the assignments 👍

Ok no problem

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) https://github.com/Expensify/App/blob/879378ff68dc026c1b4ba6af30b70051b99c6d35/src/libs/actions/Report.ts#L1258-L1267

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 https://github.com/Expensify/App/blob/879378ff68dc026c1b4ba6af30b70051b99c6d35/src/libs/actions/Report.ts#L1262
  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 https://github.com/Expensify/App/blob/879378ff68dc026c1b4ba6af30b70051b99c6d35/src/libs/actions/PersistedRequests.ts#L20-L21
  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,
         },
     );
 }