[CHECKLIST] [$250] Tasks - Task stays on the previously assigned user after assignee has been changed

[lanitochka17]

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

---

Version Number: 9.0.16
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): biruknew45+22s9sj@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to [staging.new.expensify.com]
2. Go to any chat
3. Click on the plus icon in the compose box
4. Select "Assign Task."
5. Give it a title, then click "Next."
6. Add an assignee, then confirm the task
7. Open the task
8. Click on the assignee and reassign it to another person
9. Open the task and reassign it to another person again

Expected Result:

When reassigning to the third person, the chat created when reassigning to the second person should be cleared or removed

Actual Result:

When reassigning to the third person, the chat created for the second person assigned still exists/persists, and they can see it and take some actions

Workaround:

Unknown

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

Add any screenshot/video evidence

Bug6554782_1722094115952.1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0116c365af04938c86
• Upwork Job ID: 1821259987888071161
• Last Price Increase: 2024-08-14
• Automatic offers:
• jjcoffee | Reviewer | 103541888
• dominictb | Contributor | 103541890

Issue Owner

Current Issue Owner: @garrettmknight

[melvin-bot]

Triggered auto assignment to @garrettmknight (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@garrettmknight FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[kpadmanabhan]

@lanitochka17 : unable to reproduce this in staging. is there a separate setting that should be enabled in account settings?
i do not see the PM chat popping up with assignee. see screenshot below.

[nyomanjyotisa]

Proposal

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

Task stays on the previously assigned user after assignee has been changed

What is the root cause of that problem?

Problem 1: OpenReport API call response still provide the reportAction_ onyxData for the task that no longer assigned to the user

Problem 2: canModifyTask here returns true for the user that no longer assigned to the task, so that user still can update the task and mark as completed. And gives an error when the previously assigned user marks the task as completed:

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

Problem 1:
I think this is expected, the previously assigned user is still able to view the task and leave comments, although they are not assigned anymore. But if this needs to be fixed, it needs to be fixed on BE, and some optimistic data fix after the BE fix

Problem 2:
We need to return false on canModifyTask function here if the current user is not task owner and also not task assignee, so the previously assigned user cannot update the task and cannot mark the task as completed
Change this code to the following:

    if (sessionAccountID !== getTaskOwnerAccountID(taskReport) && sessionAccountID !== getTaskAssigneeAccountID(taskReport)) {
        return false;
    }

What alternative solutions did you explore? (Optional)

[melvin-bot]

@garrettmknight Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0116c365af04938c86

[melvin-bot]

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

[jjcoffee]

The expected result is a bit unclear here, do we actually expect the created chat to be hidden/deleted? @garrettmknight

What's happening is a 1:1 DM is being created with the assigned user and the task shared there. Once re-assigned, the previously assigned user still has write access to the task report (i.e. they can add comments) because the task is shared in their DM, which is why canModifyTask returns true here:

App/src/libs/actions/Task.ts

Line 1116 in 53eee53

return !isEmptyObject(taskReport) && ReportUtils.isAllowedToComment(taskReport);

The previously assigned user can also edit the task, but the BE throws an error if the previously assigned user tries to complete the task (even though strangely the task will actually get marked complete). So we could tidy that up so that the FE matches the BE so that the previously assigned user can't complete the task but still can modify it as it's shared with them.

Dealing with the other part sounds like it could be complicated, especially if there's an existing DM between the (previously) assigned user and the task owner.

[kpadmanabhan]

Proposal

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

With clarity of requirement from @jjcoffee previous task owner is shown ability to complete task, which is unexpected.

What is the root cause of that problem?

Lack of clarity of requirement.
BE allows user to save comments to previously assigned task owner. FE allows at the moment and should continue to do the same. However BE allows only current task owner to complete the task. FE should behave the same.

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

Complete task option to be shown ONLY to task owner and current assignee. If user is a previous owner of the task, complete task checkbox and "mark as complete" button should be hidden.
Visibility of task completion checkbox and button to be connected to task owner / assignee.

What alternative solutions did you explore? (Optional)

N.A.

[garrettmknight]

Thanks for checking out @jjcoffee - I think the expected behavior was that the chat for the second assignee is removed. That likely means that the assignee is removed from the task room if it's not shared in a chat they'd have access to normally; however, I'd agree that it's better to allow them access to that chat and block their ability to complete the task they're not assigned to.

[dominictb]

Proposal

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

User is not task's author or task's assignee has ability to complete the task.

What is the root cause of that problem?

The condition to allow user to complete the task is:

App/src/components/ReportActionItem/TaskView.tsx

Line 116 in 98e7536

disabled={!canModifyTask}

with:

App/src/components/ReportActionItem/TaskView.tsx

Line 58 in 98e7536

const canModifyTask = Task.canModifyTask(report, props.currentUserPersonalDetails.accountID);

In this case, canModifyTask function return true because of:

App/src/libs/actions/Task.ts

Line 1116 in 98e7536

return !isEmptyObject(taskReport) && ReportUtils.isAllowedToComment(taskReport);

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

We can create:

    const isOwnerOrAssignee = sessionAccountID === getTaskOwnerAccountID(report) || sessionAccountID === getTaskAssigneeAccountID(report)

and then update:

App/src/components/ReportActionItem/TaskView.tsx

Line 116 in 98e7536

disabled={!canModifyTask}

                                            disabled={!canModifyTask || !isOwnerOrAssignee}

The same should be applied to:

App/src/components/TaskHeaderActionButton.tsx

Line 37 in 98e7536

isDisabled={!Task.canModifyTask(report, session?.accountID ?? -1)}

App/src/components/ReportActionItem/TaskPreview.tsx

Line 102 in 98e7536

disabled={!Task.canModifyTask(taskReport, currentUserPersonalDetails.accountID)}

Can create the util function for it.

[jjcoffee]

@dominictb's proposal is something along the lines of what I was thinking, but just to check @garrettmknight are you able to confirm if this would be the expected behaviour. Essentially there would be two separate groups:

1. canModifyTask - users that can edit the description or title of the task only (current behaviour).
2. canActionTask - users that can actually change the status of the task (mark complete or incomplete), which currently would be the owner or assignee of the task (which I think matches current BE expectations).

[garrettmknight]

Could we just disable canModifyTask when the assignee is changed instead of adding new function like canActionTask? Would that still allow them to comment on the task, but not change the description, title, or complete the task? That feels like a really simple, clean solution if so.

[dominictb]

Would that still allow them to comment on the task, but not change the description, title, or complete the task?

@garrettmknight We had a PR that "allow members to edit task, if users can comment on reports they also can edit task" here. If we implement what you said above, it will break the change in that PR.

[melvin-bot]

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

[garrettmknight]

Thanks @dominictb - In that case it does seem like we need a different function canActionTask. @jjcoffee let's move forward with the expected behavior you explained here

[jjcoffee]

Thanks for confirming @garrettmknight! Let's move forward with @dominictb's proposal then, with a util function canActionTask (unless someone can come up with a neater name!).

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @bondydaa, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[jjcoffee]

Well we have two similar cases:

1. Task is shared automatically to a 1:1 DM when you create a task and assign a user
2. You can choose to share a task with a user without assigning them

It's clear that in (1) we want to block the user from being able to complete/incomplete the task after they're no longer assigned, but it's not clear in (2) whether we want the same behaviour.

[dominictb]

It's clear that in (1) we want to block the user from being able to complete/incomplete the task after they're no longer assigned, but it's not clear in (2) whether we want the same behaviour.

IMO, I think both cases can have the same behavior. In (2), user still cannot complete/incomplete the task unless the task is assigned to them.

What do you think @puneetlath?

[jjcoffee]

@garrettmknight Do you have any thoughts on this? Or do you know who to bring in to get a consensus on this so we can move forward with the PR?

[jjcoffee]

@puneetlath @garrettmknight Do you guys have thoughts on what should happen in these two cases?

[garrettmknight]

I'd agree that both cases should share the same behavior.

[jjcoffee]

@garrettmknight Sweet, thanks for clearing that up! So we'll proceed with the PR under the assumption that only task assignees and the task owner can complete the task - anyone the task was shared with (who are not either of those) won't be able to complete/incomplete the task.

That means the PR is basically ready, just waiting for one small tweak from @dominictb.

[jjcoffee]

@garrettmknight Could we get a new engineer assigned to review the PR as @bondydaa is OOO?

[bondydaa]

merged that PR earlier, looks like it was included in the latest deploy to staging.

[bondydaa]

looks like it was deploy to production #47519 (comment) but the automation must have failed

[jjcoffee]

@garrettmknight This one should be due payment 2024-09-24.

[garrettmknight]

Payment Summary:

• Contributor: @dominictb $250 paid via upwork
• Reveiwer: @jjcoffee $250 paid via upwork

[garrettmknight]

@jjcoffee can you complete the checklist when you get a chance?

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: Update Task Permissions #23971
• 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: https://github.com/Expensify/App/pull/23971/files#r1774811254
• 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 - a fairly specific case that was missed
• 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 Proposal

1. Go to FAB -> Assign Task
2. Give the task a title and assign it to User A
3. Open the task, click on the assignee and reassign it to User B
4. [User A] Verify that you cannot mark the task as complete/incomplete

Do we agree 👍 or 👎

[melvin-bot]

@garrettmknight, @bondydaa, @jjcoffee, @dominictb Whoops! This issue is 2 days overdue. Let's get this updated quick!

[dominictb]

@garrettmknight We're good to pay.