Repository navigation
[Payment due @huult] [No QA] Let workspace admins delete tasks in workspace rooms - #102661
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@hoangzinh Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e77e6c609
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
|
||
| // Setup tasks are completed automatically by later setup flows, which fails once the task is deleted | ||
| const isTaskOwnedByGuideOrConcierge = report.ownerAccountID === CONST.ACCOUNT_ID.CONCIERGE || (!!report.ownerAccountID && !!guideAccountIDs?.includes(report.ownerAccountID)); | ||
| const canDeleteTaskAsPolicyAdmin = isPolicyAdmin && policy?.type !== CONST.POLICY.TYPE.PERSONAL && !isParentReportArchived && !isTaskOwnedByGuideOrConcierge; |
There was a problem hiding this comment.
@lakchote will we only allow admins to be able to delete a task in workspace rooms? Or all other chat reports? (i.e employees' workspace chats, expense reports...)
There was a problem hiding this comment.
Only tasks in workspace rooms @hoangzinh
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@hoangzinh can you finish your review please? |
|
Reassigning to @huult, @hoangzinh got sick |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-10-08.at.08.57.39.movAndroid: mWeb ChromeScreen.Recording.2026-10-08.at.08.54.20.moviOS: HybridAppScreen.Recording.2026-10-08.at.09.04.18.moviOS: mWeb SafariScreen.Recording.2026-10-08.at.09.05.32.movMacOS: Chrome / SafariScreen.Recording.2026-10-08.at.08.48.27.mov |
|
Unrelated changes Five files in this PR have nothing to do with the task Delete change. They are whitespace-only (trailing spaces removed), and they came in through the "merge main" commits:
Please revert these. The git checkout origin/main -- <the five files above> |
| const isTaskOwnedByGuideOrConcierge = report?.ownerAccountID === CONST.ACCOUNT_ID.CONCIERGE || (!!report?.ownerAccountID && !!guideAccountIDs?.includes(report.ownerAccountID)); | ||
|
|
||
| // Setup flows later complete these tasks, including when another admin has no matching personal onboarding data | ||
| const isSetupTask = isAdminRoom(parentReport) && isTaskOwnedByGuideOrConcierge; | ||
| const canDeleteTaskAsPolicyAdmin = | ||
| isPolicyAdmin(policy) && | ||
| policy?.type !== CONST.POLICY.TYPE.PERSONAL && | ||
| (isUserCreatedPolicyRoom(parentReport) || isDefaultRoom(parentReport)) && | ||
| !isParentReportArchived && | ||
| !isSetupTask; |
There was a problem hiding this comment.
@lakchote I think we should move this logic into src/libs/actions/Task.ts by adding a canDeleteTaskAsPolicyAdmin function there. Then the component would only need one line to call it. This would make the code easier to read and easier to unit test.
There was a problem hiding this comment.
Good call, moved it to canDeleteTaskAsPolicyAdmin in Task.ts next to canModifyTask and added a test.
|
|
||
| const isTaskOwnedByGuideOrConcierge = report?.ownerAccountID === CONST.ACCOUNT_ID.CONCIERGE || (!!report?.ownerAccountID && !!guideAccountIDs?.includes(report.ownerAccountID)); | ||
|
|
||
| // Setup flows later complete these tasks, including when another admin has no matching personal onboarding data |
There was a problem hiding this comment.
| // Setup flows later complete these tasks, including when another admin has no matching personal onboarding data | |
| // Guide/Concierge setup tasks in #admins are completed by the onboarding flow, so admins should not delete them |
@lakchote can we update like this?
|
Good catch, reverted those 5 files. |
huult
left a comment
There was a problem hiding this comment.
LGTM.
The code changes are working as described.
|
🎯 @huult, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 MariaHCD has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/MariaHCD in version: 9.5.6-0 🚀
|
|
No help site update is needed. No article says who can delete a task, so letting workspace admins delete other people's tasks in workspace rooms doesn't make any existing sentence wrong. The closest article, Assign a Task via Chat, only lists editing actions for the creator and assignee, and those are unchanged. |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.6-6 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Workspace admins couldn't delete a task that someone else created in one of their workspace rooms. The task details page only showed Delete to the task creator, so an admin had no way to remove spam tasks posted by other people in a public room.
Now the task details page also shows Delete to an admin of the task's workspace, as long as the workspace isn't a personal one and the parent room isn't archived. It uses the same Delete button and confirmation as for the creator.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/681309
PROPOSAL:
Tests
Prerequisite: Account A is a new account whose email has no
+. During onboarding it picked "Manage my team's expenses", which creates a workspace with setup tasks in its #admins room.Screen.Recording.2026-09-30.at.18.54.12.mov
Offline tests
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari