[$500] Web – Assign Task - An error occurs when both User A and User B press the "Mark as done" button simultaneously #27570

lanitochka17 · 2023-09-15T23:02:29Z

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

Action Performed:

Open any chat labeled "Assign Task" between User A and User B. (The task was created by User A.)
Click "Mark as done" and "Mark as incomplete" from User A. Observe that it works correctly
Click "Mark as done" and "Mark as incomplete" from User B. Observe that it also works correctly
Ensure that the "Mark as done" option is present in the header for each user
Simultaneously press the "Mark as done" button from both User A and User B
An error message appears on User B's system, stating, "You do not have permission to perform the requested action.

Expected Result:

The error should not appear and the task should be closed regardless of which user performs the action

Actual Result:

An error message appears on User B's system, stating, "You do not have permission to perform the requested action."

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.70-5

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug.2.mp4

0-02-01-6ce24cc1ebb53856f7b92b9ede59b4d4774dbe3451ed70704747a1e8a05c7c25_bdeb7f22536b3c99.1.mp4

Expensify/Expensify Issue URL:

Issue reported by: @usmantariq96

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694588501046099

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0129a7b3f4233c245f
Upwork Job ID: 1702820221863018496
Last Price Increase: 2023-10-06
Automatic offers:

dukenv0307 | Contributor | 27120400

melvin-bot · 2023-09-15T23:02:34Z

Job added to Upwork: https://www.upwork.com/jobs/~0129a7b3f4233c245f

melvin-bot · 2023-09-15T23:02:34Z

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

melvin-bot · 2023-09-15T23:02:36Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot · 2023-09-15T23:02:38Z

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

melvin-bot · 2023-09-15T23:02:43Z

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

dukenv0307 · 2023-09-16T06:17:16Z

Proposal

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

An error message appears on User B's system, stating, "You do not have permission to perform the requested action."

What is the root cause of that problem?

At the time the CompleteTask API call from user B is made, the report is already in the completed state, so it throws an error.

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

We can check if the report action is CompleteTask, and the report is already in the completed state, then we clear the report action errors.

We can do it in an useEffect in ReportActionItem (or somewhere like ReportActionsList we can loop through the actions). This will create a graceful experience if in a team, multiple members complete a task at the same time (or in offline mode then comes online).

useEffect(() => {
        if (props.action.actionName === CONST.REPORT.ACTIONS.TYPE.TASKCOMPLETED 
          && !_.isEmpty(props.action.errors) 
          && props.report.stateNum === CONST.REPORT.STATE_NUM.SUBMITTED 
          && props.report.statusNum === CONST.REPORT.STATUS.APPROVED) {            
            ReportActions.clearReportActionErrors(props.report.reportID, props.action)
        }
}, [props]);

It can be fixed the same for the reopen/cancel case.

What alternative solutions did you explore? (Optional)

An alternative is to add a field in the back-end to clarify that the error is due to task is already completed, like errorCode: 'taskAlreadyCompleted', and clear the error right away if the report action is TASKCOMPLETED and it has that error code. Or we can depend on the 403 http status from the back-end.

ayazalavi · 2023-09-16T07:26:39Z

Proposal

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

In assign task when 2 users simulatenously complete task then 1 of 2 users gets an unwanted error.

What is the root cause of that problem?

Root cause is that we are hard coding error message like here:

App/src/libs/actions/Task.js

Lines 264 to 272 in 76c1559

    
           { 
        
               onyxMethod: Onyx.METHOD.MERGE, 
        
               key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${taskReportID}`, 
        
               value: { 
        
                   [completedTaskReportAction.reportActionID]: { 
        
                       errors: ErrorUtils.getMicroSecondOnyxError('task.messages.error'), 
        
                   }, 
        
               }, 
        
           },

Same is true for both completeTask and reopenTask methods.

This is not considering other reasons of error and have just 1 hard coded case of permissions issue which is not true in this specific case.

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

We need to add error handling for task errors and show error that is tailored to the problem that happened which in this case is e.g. "User 1 has already completed task.", "User 2 has already reopened task" or simply "Unable to complete/reopen task".

In following piece of code

App/src/languages/en.ts

Lines 1524 to 1539 in 76c1559

    
           task: { 
        
               task: 'Task', 
        
               title: 'Title', 
        
               description: 'Description', 
        
               assignee: 'Assignee', 
        
               completed: 'Completed', 
        
               messages: { 
        
                   completed: 'completed task', 
        
                   canceled: 'canceled task', 
        
                   reopened: 'reopened task', 
        
                   error: 'You do not have the permission to do the requested action.', 
        
               }, 
        
               markAsDone: 'Mark as done', 
        
               markAsIncomplete: 'Mark as incomplete', 
        
               assigneeError: 'There was an error assigning this task, please try another assignee.', 
        
           },

we need to add 2 more keys

completingError: "Unable to mark the task as completed. Please try again later."
reopeningError: "Unable to reopen the task. Please try again later."

then in completeTask method use task.completingError instead of task.messages.error

App/src/libs/actions/Task.js

Line 269 in 76c1559

errors: ErrorUtils.getMicroSecondOnyxError('task.messages.error'),

and in reopenTask use task.reopeningError instead of task.messages.error

App/src/libs/actions/Task.js

Line 356 in 76c1559

errors: ErrorUtils.getMicroSecondOnyxError('task.messages.error'),

Result:

Untitled.9.mp4

What alternative solutions did you explore? (Optional)

Just simply remove this:

App/src/libs/actions/Task.js

Lines 264 to 272 in 76c1559

    
           { 
        
               onyxMethod: Onyx.METHOD.MERGE, 
        
               key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${taskReportID}`, 
        
               value: { 
        
                   [completedTaskReportAction.reportActionID]: { 
        
                       errors: ErrorUtils.getMicroSecondOnyxError('task.messages.error'), 
        
                   }, 
        
               }, 
        
           },

App/src/libs/actions/Task.js

Lines 351 to 359 in 76c1559

    
           { 
        
               onyxMethod: Onyx.METHOD.MERGE, 
        
               key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${taskReportID}`, 
        
               value: { 
        
                   [reopenedTaskReportAction.reportActionID]: { 
        
                       errors: ErrorUtils.getMicroSecondOnyxError('task.messages.error'), 
        
                   }, 
        
               }, 
        
           },

That will stop showing error messages for tasks api call.

ayazalavi · 2023-09-16T13:31:56Z

Proposal

Updated

imranaalam · 2023-09-16T15:03:30Z

Problem Statement:
When two users simultaneously attempt to mark a task as done, one of them encounters an error message, "You do not have permission to perform the requested action."

Root Cause:
The system doesn't handle concurrent task state modifications gracefully, leading to misleading error responses when race conditions emerge.

Proposed Solution:
Intelligent Error Handling Using Task State Locking and Enhanced Onyx Messaging:

Backend Task State Locking:
- Use a short-term locking mechanism to handle task state changes.
- If another user tries to modify the task during this lock, return an appropriate concurrency error code.
Enhanced Onyx Error Management on the Frontend:
- Incorporate specific error codes to provide clearer feedback.
- Update the Onyx store with user-friendly error messages based on the error code received.

Implementation:

Backend:

if (task.isLocked()) {
    return { success: false, errorCode: 'CONCURRENCY_ISSUE', message: 'Another user is currently updating the task. Please try again in a moment.' };
}
task.lock();
// Carry out the task state modification
task.unlock();

Frontend:

function changeTaskState(taskID) {
    API.ChangeTaskState(taskID)
        .then((response) => {
            if (response.errorCode === 'CONCURRENCY_ISSUE') {
                Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${taskID}`, {
                    errors: {
                        concurrencyIssue: "Another user is updating this task. Kindly wait a moment."
                    }
                });
            } else if (!response.success) {
                // Manage other potential errors
            }
        });
}

Incorporate enhanced error messages in App/src/languages/en.ts:

task: {
    // ... existing keys
    messages: {
        // ... existing messages
        concurrencyIssue: "Another user is updating this task. Kindly wait a moment.",
        completingError: "Unable to finalize the task as completed. Please retry shortly.",
        reopeningError: "Unable to revert the task. Please try again after a while."
    }
}

Advantages:

Efficiently handles race conditions using a backend locking mechanism.
Delivers clearer feedback to users through enhanced Onyx error messaging, improving UX.
Centralizes error messages with Onyx, streamlining error management.

Disadvantages:

The locking mechanism can introduce a minor delay in processing task updates.
Requires frontend and backend modifications to manage and display the new error messages.

Alternative Solution:
Optimistic UI Updates with Timestamp-based Validation and Onyx Messaging:

Assume success on the frontend (optimistically) when a user tries to change the task state.
Use timestamps to determine the most recent task state change on the backend.
If there's a conflict, return a specific error code.
Display a more descriptive error message using Onyx on the frontend.

Advantages:

Provides an immediate response, enhancing user experience.
Reduces the chance of race conditions using timestamp validation.

Disadvantages:

Potential for race conditions if timestamps are identical, though very unlikely.
Users might still experience rejection messages, but they will be more descriptive.

michaelhaxhiu · 2023-09-20T18:17:57Z

This was marked External prematurely, and I'm just getting caught up now. I just tested on my local accountant this is reproducible so we should indeed fix it.

@sobitneupane inviting your input on the proposals above.

melvin-bot · 2023-09-22T16:00:13Z

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

sobitneupane · 2023-09-25T07:38:50Z

@michaelhaxhiu It is better to handle this issue internally. I think it's possible that we may need to make some adjustments to the backend in order to address the issue.

ayazalavi · 2023-09-25T13:25:02Z

@sobitneupane @michaelhaxhiu in the frontend showing simple error message in case of a failure should be enough as per my thinking. Handling it in the backend with semaphores and other simultaneous access jargon can make things more complicated and will cause more errors. Simply showing message unable to mark completed/reopen should be enough. @michaelhaxhiu what do you think?

dukenv0307 · 2023-09-26T14:59:44Z

@michaelhaxhiu It is better to handle this issue internally. I think it's possible that we may need to make some adjustments to the backend in order to address the issue.

@sobitneupane what behavior do you think should happen here? If the Expected Result in the issue is what we want, we can have seamless user experience with just this front-end fix

cc @michaelhaxhiu

melvin-bot · 2023-09-27T14:43:12Z

Current assignee @sobitneupane is eligible for the Internal assigner, not assigning anyone new.

neil-marcellini · 2023-10-18T17:56:00Z

Rodrigo is taking over for me to implement the backend change.

rlinoz · 2023-10-20T01:35:56Z

This PR solves it in the backend if we are ok with showing the marked as complete twice for the client who would get the error

What do you think @neil-marcellini ?

ayazalavi · 2023-10-20T13:31:10Z

any room for new proposal here?

sobitneupane · 2023-10-27T12:32:47Z

@ayazalavi It is decided to fix the issue in the backend. It is no longer available for new proposals.

MitchExpensify · 2023-10-28T23:00:14Z

So we are suggesting showing one action, marked as complete, as taken by two people? I'm not sure that's ideal but curious what @neil-marcellini thinks too

rlinoz · 2023-10-30T08:45:18Z

In the end only one person actually completed the task, since we don't save a new completed action for a task that was already completed, that is why I think this is ok.

MitchExpensify · 2023-11-01T18:14:14Z

In the end only one person actually completed the task, since we don't save a new completed action for a task that was already completed, that is why I think this is ok.

Just to be sure, you think showing the task that one person actually completed to two people as completed by both of them to be OK?

rlinoz · 2023-11-02T10:03:37Z

Oh no sorry, I think we should show only one completed message by the correct person, and that is what we are actually doing, showing only one complete action for this situation.

I guess the screenshot I took made things a little bit confusing, but we fixed that screenshot in the backend PR.

If two people complete a task simultaneously we are erasing the completed message from the person that didn't actually complete it.

MitchExpensify · 2023-11-03T23:20:33Z

Awesome!

melvin-bot · 2023-11-27T13:08:05Z

This issue has not been updated in over 15 days. @rlinoz, @sobitneupane, @MitchExpensify, @dukenv0307 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!

rlinoz · 2023-11-27T14:20:42Z

I think we can close this issue, but I am not sure whether payment is due or not.

@MitchExpensify

MitchExpensify · 2023-11-28T19:22:41Z

Payment summary:

usmantariq96 Reporter: $50, Upwork
C @dukenv0307 Upwork - $500
C+ @sobitneupane Newdot - $500

sobitneupane · 2023-11-29T09:15:48Z

@MitchExpensify A proposal was selected and while implementing the PR, it was decided that the issue would be best solved on the backend. So, we ended of closing the PR without merging.

dukenv0307 · 2023-11-29T12:24:50Z

Should we process payment for both C and C+ since we spent lots of effort in the PR and it's almost done. We have the same case in the past #17188 (comment)

MitchExpensify · 2023-12-05T20:28:38Z

That seems fair, can you both please apply to this job for payment? https://www.upwork.com/jobs/~01314f533571a41c4f

MitchExpensify · 2023-12-05T20:29:10Z

And same for you usmantariq96, thanks!

usmantariq96 · 2023-12-06T03:42:31Z

@MitchExpensify applied for the job
thanks

dukenv0307 · 2023-12-06T06:55:35Z

That seems fair, can you both please apply to this job for payment? https://www.upwork.com/jobs/~01314f533571a41c4f

@MitchExpensify applied, thank you!

sobitneupane · 2023-12-08T08:48:31Z

@MitchExpensify I will request payment on newDot once you post the payment summary.

MitchExpensify · 2023-12-11T23:23:40Z

Updated the payment summary - #27570 (comment)

MitchExpensify · 2023-12-11T23:27:04Z

Do we need to add a regression test here @sobitneupane ?

sobitneupane · 2023-12-15T09:29:28Z

Regression Test Proposal

Create a task from User A assigning it to User B.
Ensure that the "Mark as done" option is present in the header for each user
Simultaneously press the "Mark as done" button from both User A and User B
Verify that the app works fine and no error is displayed
Ensure that the task is marked as Done
Simultaneously press the green box to re-open the task from both User A and User B
Verify that the app works fine and no error is displayed
Simultaneously press Cancel the task from both User A and User B
Verify that the app works fine and no error is displayed

Do we agree 👍 or 👎

sobitneupane · 2023-12-15T09:52:01Z

#27570 (comment)

Requested payment on newDot.

JmillsExpensify · 2023-12-15T23:14:32Z

$500 payment approved for @sobitneupane based on this comment.

lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 15, 2023

melvin-bot bot changed the title ~~Web – Assign Task - An error occurs when both User A and User B press the "Mark as done" button simultaneously~~ [$500] Web – Assign Task - An error occurs when both User A and User B press the "Mark as done" button simultaneously Sep 15, 2023

melvin-bot bot assigned michaelhaxhiu Sep 15, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 15, 2023

melvin-bot bot assigned sakluger Sep 15, 2023

melvin-bot bot assigned sobitneupane Sep 15, 2023

sakluger removed their assignment Sep 18, 2023

melvin-bot bot added the Overdue label Sep 20, 2023

melvin-bot bot removed the Overdue label Sep 20, 2023

melvin-bot bot added the Overdue label Sep 27, 2023

michaelhaxhiu added the Internal Requires API changes or must be handled by Expensify staff label Sep 27, 2023

michaelhaxhiu added Engineering and removed Internal Requires API changes or must be handled by Expensify staff labels Sep 27, 2023

neil-marcellini removed their assignment Oct 18, 2023

rlinoz added the Reviewing Has a PR in review label Oct 23, 2023

melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Nov 27, 2023

MitchExpensify closed this as completed Dec 15, 2023

dukenv0307 mentioned this issue Jan 17, 2024

[$1000] Web - Delete Button Fails to Delete Changed Description in IOU #25997

Closed

6 tasks

[$500] Web – Assign Task - An error occurs when both User A and User B press the "Mark as done" button simultaneously #27570

[$500] Web – Assign Task - An error occurs when both User A and User B press the "Mark as done" button simultaneously #27570

Comments

lanitochka17 commented Sep 15, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023 • edited by michaelhaxhiu Loading

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Sep 15, 2023

melvin-bot bot commented Sep 15, 2023

dukenv0307 commented Sep 16, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

ayazalavi commented Sep 16, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

ayazalavi commented Sep 16, 2023

Proposal

imranaalam commented Sep 16, 2023

michaelhaxhiu commented Sep 20, 2023

melvin-bot bot commented Sep 22, 2023

sobitneupane commented Sep 25, 2023

ayazalavi commented Sep 25, 2023

dukenv0307 commented Sep 26, 2023

melvin-bot bot commented Sep 27, 2023

neil-marcellini commented Oct 18, 2023

rlinoz commented Oct 20, 2023

ayazalavi commented Oct 20, 2023

sobitneupane commented Oct 27, 2023

MitchExpensify commented Oct 28, 2023

rlinoz commented Oct 30, 2023

MitchExpensify commented Nov 1, 2023

rlinoz commented Nov 2, 2023

MitchExpensify commented Nov 3, 2023

melvin-bot bot commented Nov 27, 2023

rlinoz commented Nov 27, 2023

MitchExpensify commented Nov 28, 2023 • edited Loading

sobitneupane commented Nov 29, 2023 • edited Loading

dukenv0307 commented Nov 29, 2023

MitchExpensify commented Dec 5, 2023

MitchExpensify commented Dec 5, 2023

usmantariq96 commented Dec 6, 2023

dukenv0307 commented Dec 6, 2023

sobitneupane commented Dec 8, 2023

MitchExpensify commented Dec 11, 2023

MitchExpensify commented Dec 11, 2023

sobitneupane commented Dec 15, 2023

sobitneupane commented Dec 15, 2023

JmillsExpensify commented Dec 15, 2023

lanitochka17 commented Sep 15, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Sep 15, 2023 •

edited by michaelhaxhiu

Loading

dukenv0307 commented Sep 16, 2023 •

edited

Loading

ayazalavi commented Sep 16, 2023 •

edited

Loading

MitchExpensify commented Nov 28, 2023 •

edited

Loading

sobitneupane commented Nov 29, 2023 •

edited

Loading