Make all task titles/descriptions read-only to all but the author

[quinthar]

Background: When a new user signs up, Concierge assigns tasks to the user based on their onboarding intentions. Many of these tasks contain links. We see in Fullstory that many users who are attempting to click the link, accidentally click next to it -- and thus are shown an "editor" for the description.

Problem: When users accidentally edit the description of a task assigned to them, it unexpected and feels broken, undermining the perception of product quality and conversion.

Solution: Make all tasks read-only for everyone except the author. This involves:

1. Prevent non-author editing on the front end
2. When done, we will update the API to prevent non-author editing on the back end.

Issue Owner

Current Issue Owner: @suneox

[melvin-bot[bot]]

Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.

[melvin-bot[bot]]

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

[nkdengineer]

Edited by proposal-police: This proposal was edited at 2024-11-22 20:05:08 UTC.

Proposal

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

Make all task titles/descriptions read-only to all but the author

What is the root cause of that problem?

Improvment

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

Update canModifyTask function to return false if the sessionAccountID isn't the author of the task.

function canModifyTask(taskReport: OnyxEntry<OnyxTypes.Report>, sessionAccountID: number): boolean {
    if (ReportUtils.isCanceledTaskReport(taskReport)) {
        return false;
    }

    const parentReport = getParentReport(taskReport);
    const reportNameValuePairs = ReportUtils.getReportNameValuePairs(parentReport?.reportID);
    if (ReportUtils.isArchivedRoom(parentReport, reportNameValuePairs)) {
        return false;
    }

    return sessionAccountID === getTaskOwnerAccountID(taskReport);
}

https://github.com/Expensify/App/blob/ef0d341c7d15ce1b636cf5bb70b1f389293af297/src/libs/actions/Task.ts#L1179-L1182

What alternative solutions did you explore? (Optional)

[Krishna2323]

Edited by proposal-police: This proposal was edited at 2024-12-01 00:08:25 UTC.

Proposal


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

Make all task titles/descriptions read-only to all but the author

What is the root cause of that problem?

Improvement

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


• We can modify canModifyTask to only return true if sessionAccountID === getTaskOwnerAccountID(taskReport) is true.

function canModifyTask(taskReport: OnyxEntry<OnyxTypes.Report>, sessionAccountID: number): boolean {
    if (ReportUtils.isCanceledTaskReport(taskReport)) {
        return false;
    }

    const parentReport = getParentReport(taskReport);
    const reportNameValuePairs = ReportUtils.getReportNameValuePairs(parentReport?.reportID);
    if (ReportUtils.isArchivedRoom(parentReport, reportNameValuePairs)) {
        return false;
    }

    if (!ReportUtils.canWriteInReport(taskReport) || ReportUtils.isAuditor(taskReport)) {
        return false;
    }

    return sessionAccountID === getTaskOwnerAccountID(taskReport);
}

• Remove the disabled prop. https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/components/ReportActionItem/TaskView.tsx#L92
• Add disableState && styles.cursorDisabled, in the styles array. https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/components/ReportActionItem/TaskView.tsx#L86-L91
• For checkbox update the disabled prop to disabled={!canActionTask}. https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/components/ReportActionItem/TaskView.tsx#L117

What alternative solutions did you explore? (Optional)

• We should also hide the TaskHeaderActionButton instead of disabling when the user can't edit the task report (mark complete/incomplete). For this we can update the condition to render the TaskHeaderActionButton here and here, we can simply add a check canModifyTask before rendering the button.
• We also need to update the isDisabled prop value to !Task.canActionTask(report, session?.accountID ?? -1) in TaskHeaderActionButton. https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/components/TaskHeaderActionButton.tsx#L37
• We would also need to update canActionTask to also add additional checks like in canModifyTask. e.g. canWriteInReport, isArchivedRoom, isCanceledTaskReport. Or we can refactor both functions in one by adding additional prop shouldAllowAssignee for conditionally allowing assignee to do some action. By default the function will only check for the author but if the prop is passed as true we will return true for assignee also. We will use the second param for task complete/incomplete action.
• If we want to keep the current behaviour and only make this change for tasks created by Concierge, as mentioned in the main proposal.

Result

[suneox]

1. Prevent non-author editing on the front end

@quinthar I’d like to confirm if this behavior only prevents editing tasks created by Concierge while still allowing users to change the task status to complete/incomplete, or does it also prevent changing the task status (meaning users can only view tasks created by Concierge)?

[melvin-bot[bot]]

@quinthar, @suneox Whoops! This issue is 2 days overdue. Let's get this updated quick!

[suneox]

We're still waiting for confirmation on this comment. When we prevent non-authors from marking a task as complete, should we also consider the backend rule to ensure it does not send another task to Concierge?

https://github.com/user-attachments/assets/a4d00089-544f-40f4-bf19-259a97cb75a5

[quinthar]

correct, the assignee CAN mark it as done, this only constrains editing the title & description.

[Krishna2323]

PROPOSAL UPDATED

• Added alternative 2

[Krishna2323]

PROPOSAL UPDATED

• Added few minor points in alternative 2 solution.

[suneox]

correct, the assignee CAN mark it as done, this only constrains editing the title & description.

Based on this confirmation, we only prevent editing the title and description of tasks where the owner is Concierge. Both solutions from @nkdengineer and @Krishna2323 are do not meet the expected behavior. We are still waiting for updates from the contributors.

[!NOTE]
We only need solutions that match the confirmed behavior. Any solutions that do not align with this behavior should be removed to avoid cluttering the proposal with relevant options.

[Krishna2323]

Edited by proposal-police: This proposal was edited at 2024-12-01 18:10:41 UTC.

Proposal


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

Make all task titles/descriptions read-only to all but the author

What is the root cause of that problem?

Improvement

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


• We can modify canModifyTask to only return false if getTaskOwnerAccountID(taskReport) === CONST.ACCOUNT_ID.CONCIERGE is true.

NOTE: If we only want to change this behaviour for CONCIERGE then we can just add the first condition and let canModifyTask as it is now.

function canModifyTask(taskReport: OnyxEntry<OnyxTypes.Report>, sessionAccountID: number): boolean {
    if (getTaskOwnerAccountID(taskReport) === CONST.ACCOUNT_ID.CONCIERGE) {
        return false;
    }

    const parentReport = getParentReport(taskReport);
    const reportNameValuePairs = ReportUtils.getReportNameValuePairs(parentReport?.reportID);
    if (ReportUtils.isArchivedRoom(parentReport, reportNameValuePairs)) {
        return false;
    }

    if (!ReportUtils.canWriteInReport(taskReport) || ReportUtils.isAuditor(taskReport)) {
        return false;
    }

    return sessionAccountID === getTaskOwnerAccountID(taskReport);
}

• Remove the disabled prop. https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/components/ReportActionItem/TaskView.tsx#L92
• Add disableState && styles.cursorDisabled, in the styles array. https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/components/ReportActionItem/TaskView.tsx#L86-L91
• For checkbox update the disabled prop to disabled={!canActionTask}. https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/components/ReportActionItem/TaskView.tsx#L117
• We should also check all the places where we use canModifyTask & canActionTask, like we also need to make changes in TaskPreview and few other places.
• We would also need to update canActionTask to also add additional checks like in canModifyTask. e.g. canWriteInReport, isArchivedRoom, isCanceledTaskReport. Or we can refactor both functions in one by adding additional prop allowEditingConciergeTask for conditionally allowing assignee to do some action. By default the function will return false for task created by Concierge but when allowEditingConciergeTask is true we will surpass that check. We would only need to surpass Concierge check for complete/incomplete action.

NOTE: If we only want to change this behaviour for CONCIERGE then we can just add the first condition and let canModifyTask as it is now.

function canModifyTask(taskReport: OnyxEntry<OnyxTypes.Report>, sessionAccountID: number, allowEditingConciergeTask?: boolean): boolean {
    if (getTaskOwnerAccountID(taskReport) === CONST.ACCOUNT_ID.CONCIERGE && !allowEditingConciergeTask) {
        return false;
    }
    const parentReport = getParentReport(taskReport);
    const reportNameValuePairs = ReportUtils.getReportNameValuePairs(parentReport?.reportID);
    if (ReportUtils.isArchivedRoom(parentReport, reportNameValuePairs)) {
        return false;
    }

    if (!ReportUtils.canWriteInReport(taskReport) || ReportUtils.isAuditor(taskReport)) {
        return false;
    }

    return sessionAccountID === getTaskOwnerAccountID(taskReport) || sessionAccountID === getTaskAssigneeAccountID(taskReport)
}

    // In TaskView.tsx
    const canModifyTask = Task.canModifyTask(report, props.currentUserPersonalDetails.accountID);
    const canActionTask = Task.canModifyTask(report, props.currentUserPersonalDetails.accountID, true);

• In TaskHeaderActionButton we also need to update the isDisabled prop value to !Task.canActionTask(report, session?.accountID ?? -1) in TaskHeaderActionButton to make sure that user can also complete the task using header button when canActionTask is true. https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/components/TaskHeaderActionButton.tsx#L37

What alternative solutions did you explore? (Optional)

NA

Result

https://github.com/user-attachments/assets/e900f2fb-0337-481b-8316-451bf706eceb

[Krishna2323]

@suneox, I added a new proposal and hid the previous one due to multiple edits.

Edit: sorry for multiple edits on the new proposal again, I was confused on the expected behaviour.

[suneox]

@suneox, I added a new proposal and hid the previous one due to multiple edits.

@Krishna2323 Thank you for the update. Your solution looks good overall. However, we cannot update the task status directly from the Concierge chat.

https://github.com/user-attachments/assets/b56cab9c-d622-4b66-a3a1-39fa753cde76

Nice to have if you please share a test branch with new updates

[Krishna2323]

However, we cannot update the task status directly from the Concierge chat.

@suneox, this is likely because you might have missed the 5th step in my proposal. Test branch for reference.

We should also check all the places where we use canModifyTask & canActionTask, like we also need to make changes in TaskPreview and few other places.

EDIT: I updated the test branch, initially on the concierge page TaskReport is empty inside TaskPreview component, that's why we were unable to complete/incomplete the task. To fix that, we need to pass the taskAssigneeAccountID obtained from the action to canModifyTask and also pass the reportID to reopenTask & completeTask.

[suneox]

The solution to add a new parameter to the canModifyTask function to check for ConciergeTask and restrict editing tasks LGTM. The arrangement of logic inside canModifyTask function can be handled during the PR review process. We can proceed with @Krishna2323 proposal

🎀 👀 🎀 C+ reviewed

[melvin-bot[bot]]

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

[thienlnam]

Hmm, this was actually originally the way tasks were created but then we let the assignee update the task details and then we just let everyone who has the ability to comment on the task to edit it.

Since this problem is specific to Concierge, should we just restrict editing those tasks so that in other usages the assignee can still edit tasks? cc @quinthar

[quinthar]

In the long run we will likely make this configurable somehow. But for now, we're thinking the easiest is to just change the default for all tasks.

[thienlnam]

Cool, sounds good

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.78-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/53749

If no regressions arise, payment will be issued on 2025-01-02. :confetti_ball:

For reference, here are some details about the assignees on this issue:

• @suneox requires payment (Needs manual offer from BZ)
• @Krishna2323 requires payment (Needs manual offer from BZ)

[suneox]

BugZero Checklist:

• [x] [Contributor] Classify the bug:

Bug classification

Source of bug:

• [ ] 1a. Result of the original design (eg. a case wasn't considered)
• [ ] 1b. Mistake during implementation
• [ ] 1c. Backend bug
• [x] 1z. Other: New feature request.

Where bug was reported:

• [ ] 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• [ ] 2b. Reported on staging (eg. found during regression or PR testing)
• [ ] 2d. Reported on a PR
• [x] 2z. Other: New feature request.

Who reported the bug:

• [ ] 3a. Expensify user
• [ ] 3b. Expensify employee
• [ ] 3c. Contributor
• [ ] 3d. QA
• [x] 3z. Other: New feature request.

• [x] [Contributor] 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: N/A: This is a new feature request.

• [x] [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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

• [x] [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

  N/A: Due to it isn't an impactful bug

[melvin-bot[bot]]

Issue is ready for payment but no BZ is assigned. @mallenexpensify you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[melvin-bot[bot]]

Payment Summary

Upwork Job

• ROLE: @suneox paid $(AMOUNT) via Upwork (LINK)
• ROLE: @Krishna2323 paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@mallenexpensify)

• [ ] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• [ ] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• [ ] I have paid out the Upwork contracts or cancelled the ones that are incorrect
• [ ] I have verified the payment summary above is correct

[melvin-bot[bot]]

Current assignee @mallenexpensify is eligible for the Bug assigner, not assigning anyone new.

[mallenexpensify]

@suneox @Krishna2323 , can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~021875244713886279566

[suneox]

@suneox @Krishna2323 , can you please accept the job and reply here once you have?

@mallenexpensify Thank you, I've accepted the offer

[Krishna2323]

@mallenexpensify Thanks for the offer, I've accepted it.

[mallenexpensify]

Contributor: @Krishna2323 paid $250 via Upwork Contributor+: @suneox paid $250 via Upwork

For the regression test, I'm unsure if one is needed so I'm going with @suneox 's recommendation above.

N/A: Due to it isn't an impactful bug

If anyone disagrees, please comment and I can create one. Thx