feat: allow authorized staff users to delete non-default staff roles#302
Open
henry-casper wants to merge 10 commits into
Open
feat: allow authorized staff users to delete non-default staff roles#302henry-casper wants to merge 10 commits into
henry-casper wants to merge 10 commits into
Conversation
Staff users with staffRolePermissions.canRemoveRole can delete non-default StaffRoles from the staff-role details page. Deletion raises a StaffRoleDeletedEvent whose integration handler reassigns every staff user holding the deleted role to the default StaffRole matching the deleted role's enterpriseAppRole (idempotent; missing default role is logged and surfaced as a processing failure). - domain: StaffRole.requestDelete() (visa canRemoveRole, default roles never deletable), StaffRoleDeletedEvent carrying enterpriseAppRole, StaffRoleDeletedReassignmentService, canRemoveRole in UserDomainPermissions and passports; remove legacy deleteAndReassignTo - persistence: StaffUserRepository.getAllAssignedToRole - application-services: StaffRole.delete (replaces delete-and-reassign) - event-handler: staff-role-deleted--reassign-staff-users wired into integration handler registration - graphql: staffRoleDelete mutation + command mapper + resolver tests; isDefault exposed on staff-role detail fragment - ui: red Delete button with Popconfirm on staff-role details page, gated by canRemoveRole and non-default role; progress/failure feedback; navigate back and refetch list on success; Storybook coverage for deletable/default/no-permission/confirmation/pending/failure states - verification: 8 shared Gherkin scenarios implemented across acceptance-api (31/31), acceptance-ui (32/32), and e2e (28/28) with tasks/questions/notes and e2e interactions per contexts/community reference pattern - fix: staff-user.resolvers.test.ts step/feature mismatches (file failed at collection on main, masked by aggregate test counts) - security: clear pnpm audit/snyk gate failures — bump vitest family to 4.1.10, svgo ^3.3.4, fast-uri ^4.1.1, @opentelemetry/propagator-jaeger ^2.9.0, websocket-driver ^0.7.5, axios 1.18.0, ws 8.21.1; extend expired SNYK-JS-OPENTELEMETRYCORE-17373280 ignore to 2026-10-31 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Reviewer's GuideImplements domain-driven, end-to-end support for deleting non-default staff roles by authorized staff, emitting a StaffRoleDeletedEvent and asynchronously reassigning affected staff users to matching default roles, with full GraphQL, UI, verification, and security dependency coverage and removal of the legacy delete-and-reassign flow. Sequence diagram for staff role deletion and default-role reassignmentsequenceDiagram
actor StaffUser
participant StaffRoleEditUI
participant GraphQLAPI
participant StaffRoleAppService
participant StaffRoleDomain
participant EventBus
participant StaffRoleDeletedReassignmentService
participant StaffUserRepository
participant StaffUserDomain
StaffUser->>StaffRoleEditUI: Click "Delete Role" and confirm
StaffRoleEditUI->>GraphQLAPI: staffRoleDelete(input: { id })
GraphQLAPI->>StaffRoleAppService: User.StaffRole.delete({ roleId })
StaffRoleAppService->>StaffRoleDomain: requestDelete()
StaffRoleDomain->>EventBus: publish StaffRoleDeletedEvent
GraphQLAPI-->>StaffRoleEditUI: { status: { success: true | false } }
rect rgb(240,240,240)
EventBus-->>StaffRoleDeletedReassignmentService: StaffRoleDeletedEvent
StaffRoleDeletedReassignmentService->>StaffUserRepository: getAllAssignedToRole(deletedRoleId)
StaffUserRepository-->>StaffRoleDeletedReassignmentService: staffUsers[]
loop for each staff user not already in default role
StaffRoleDeletedReassignmentService->>StaffUserDomain: requestRoleAssignment(defaultRole, description, staffUserId)
StaffRoleDeletedReassignmentService->>StaffUserRepository: save(staffUser)
end
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
Fixed security issues:
-
@opentelemetry/propagator-jaeger (link)
-
@vitest/browser (link)
-
axios (link)
-
fast-uri (link)
-
svgo (link)
-
websocket-driver (link)
-
The new
StaffRoleDeletedReassignmentServicelogs and then rethrows (and in one branch recreates) errors for missing default roles, which leads to duplicated, slightly inconsistent error messages; consider centralizing this into a single error construction/logging path so downstream handlers see a consistent error type/message. -
The new
StaffUserAssignedRoleE2E question implements its own 30-second polling loop withsetTimeout; it might be more maintainable and less flaky to use a shared wait helper (similar towaitUntil) with a shorter default timeout and configurable interval instead of hand-rolled sleeps. -
Both the UI component tests and the new page objects rely on specific antd class names/selectors (e.g.
.ant-select-selection-item, .ant-select-content,.ant-popconfirm); to reduce fragility on library upgrades, consider encapsulating these selectors in a single utility or page-object layer and avoiding duplicated selector strings across tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `StaffRoleDeletedReassignmentService` logs and then rethrows (and in one branch recreates) errors for missing default roles, which leads to duplicated, slightly inconsistent error messages; consider centralizing this into a single error construction/logging path so downstream handlers see a consistent error type/message.
- The new `StaffUserAssignedRole` E2E question implements its own 30-second polling loop with `setTimeout`; it might be more maintainable and less flaky to use a shared wait helper (similar to `waitUntil`) with a shorter default timeout and configurable interval instead of hand-rolled sleeps.
- Both the UI component tests and the new page objects rely on specific antd class names/selectors (e.g. `.ant-select-selection-item, .ant-select-content`, `.ant-popconfirm`); to reduce fragility on library upgrades, consider encapsulating these selectors in a single utility or page-object layer and avoiding duplicated selector strings across tests.
## Individual Comments
### Comment 1
<location path="packages/ocom/domain/src/domain/services/user/staff-role-deleted-reassignment.service.ts" line_range="19" />
<code_context>
+ export class StaffRoleDeletedReassignmentService {
</code_context>
<issue_to_address>
**suggestion:** Error handling for missing default role is a bit redundant and could be simplified while still logging context
`reassignStaffUsersToDefaultRole` both logs/rethrows in the `catch` and then has a separate `if (!defaultRole)` that logs/throws again. Given `getDefaultRoleByEnterpriseAppRole` currently rejects when not found, the `if (!defaultRole)` branch is unreachable and could lead to duplicate logging if the repo is later changed. It would be clearer to keep a single error path: either rely on the repo throwing and handle/log/throw only in the `catch`, or change the repo to return `null` and handle only the `!defaultRole` case.
</issue_to_address>
### Comment 2
<location path="packages/ocom/ui-staff-route-user-management/src/components/staff-role-edit.container.graphql" line_range="19" />
<code_context>
}
}
+mutation StaffRoleDelete($input: StaffRoleDeleteInput!) {
+ staffRoleDelete(input: $input) {
+ status {
</code_context>
<issue_to_address>
**issue (review_instructions):** This container GraphQL file does not follow the required directory structure under `layouts/<AreaName>/components/`.
Per the conventions, container GraphQL files should live under `ui-<PortalName>/src/components/layouts/<AreaName>/components/<componentName>.container.graphql`. This file is currently at `ui-staff-route-user-management/src/components/staff-role-edit.container.graphql`, which is missing the `layouts/<AreaName>/components` segments. Please relocate it (and its corresponding container component, if applicable) to match the required path pattern.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.container.graphql`
**Instructions:**
container graphql files should be found in the following path pattern: ui-<PortalName>/src/components/layouts/<AreaName>/components/<componentName>.container.graphql
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
Implements an end-to-end “delete non-default staff role” flow across domain, persistence, application services, GraphQL, UI, and verification suites. The deletion emits a StaffRoleDeletedEvent and an integration handler reassigns staff users from the deleted role to the matching default role by enterpriseAppRole.
Changes:
- Adds
StaffRole.requestDelete()+StaffRoleDeletedEvent+ reassignment domain service + integration handler wiring. - Introduces
staffRoleDeleteGraphQL mutation and wires it through application services to the domain, plus UI delete action (Popconfirm + loading/error feedback). - Expands repositories and verification coverage (acceptance-api/acceptance-ui/e2e) and updates security-related dependency overrides/Snyk ignore.
Blocking review notes (requires changes)
packages/ocom/ui-staff-route-user-management/tsconfig.vitest.jsonuses"extends": [...](an array). TypeScripttsconfig.jsonextendsexpects a single string, so this is very likely to fail config parsing in Vitest/tsc. Use a singleextendstarget (and merge the other config’s options) or introduce an intermediate config file that extends one base and is then extended by the other.
Reviewed changes
Copilot reviewed 80 out of 81 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Bumps Vitest and security-related overrides (axios/svgo/ws/fast-uri/etc.) to clear audit/Snyk gates. |
| packages/ocom/ui-staff-route-user-management/tsconfig.vitest.json | Adds Vitest TS config (currently uses invalid extends array). |
| packages/ocom/ui-staff-route-user-management/src/components/staff-user-detail.test.tsx | Makes role-display assertion resilient to antd markup differences. |
| packages/ocom/ui-staff-route-user-management/src/components/staff-role-edit.stories.tsx | Adds Storybook scenarios for deletable/default/no-permission/confirm/loading states. |
| packages/ocom/ui-staff-route-user-management/src/components/staff-role-edit.container.tsx | Wires delete mutation, gating by canRemoveRole and !isDefault, with success/error navigation & feedback. |
| packages/ocom/ui-staff-route-user-management/src/components/staff-role-edit.container.stories.tsx | Adds container-level Storybook coverage for delete failure path via Apollo mocks. |
| packages/ocom/ui-staff-route-user-management/src/components/staff-role-edit.container.graphql | Adds StaffRoleDelete mutation and exposes isDefault in edit fragment. |
| packages/ocom/ui-staff-route-user-management/src/components/staff-role-create.tsx | Adds delete button + Popconfirm + loading state to the edit header of the form component. |
| packages/ocom/ui-staff-route-user-management/src/components/staff-role-create.test.tsx | Adds unit tests for delete button visibility, confirmation behavior, and loading state. |
| packages/ocom/ui-staff-route-user-management/.storybook/preview.tsx | Adjusts MockedProvider setup for Storybook decorators. |
| packages/ocom/persistence/src/datasources/domain/user/staff-user/staff-user.repository.ts | Adds getAllAssignedToRole for bulk role reassignment. |
| packages/ocom/persistence/src/datasources/domain/user/staff-user/staff-user.repository.test.ts | Adds Cucumber/Vitest scenarios covering getAllAssignedToRole. |
| packages/ocom/persistence/src/datasources/domain/user/staff-user/features/staff-user.repository.feature | Adds feature scenarios for getAllAssignedToRole. |
| packages/ocom/graphql/src/schema/types/staff-user.resolvers.test.ts | Fixes resolver test expectations/mocking for role update/permission paths. |
| packages/ocom/graphql/src/schema/types/staff-role.resolvers.ts | Adds staffRoleDelete resolver mapping to application service delete command. |
| packages/ocom/graphql/src/schema/types/staff-role.resolvers.test.ts | Adds resolver tests for success/unauthorized/error handling for staffRoleDelete. |
| packages/ocom/graphql/src/schema/types/staff-role.graphql | Adds StaffRoleDeleteInput, result type, and staffRoleDelete mutation. |
| packages/ocom/graphql/src/schema/types/staff-role.command-mapper.ts | Adds command mapper for delete (roleId). |
| packages/ocom/graphql/src/schema/types/features/staff-user.resolvers.feature | Updates Gherkin scenario naming/expectations to match resolver behavior. |
| packages/ocom/graphql/src/schema/types/features/staff-role.resolvers.feature | Adds Gherkin coverage for staff role delete resolver behavior. |
| packages/ocom/event-handler/src/handlers/integration/staff-role-deleted--reassign-staff-users.ts | Registers integration handler to reassign staff users after role deletion. |
| packages/ocom/event-handler/src/handlers/integration/index.ts | Wires new staff-role-deleted integration handler into registration. |
| packages/ocom/domain/src/domain/services/user/staff-role-deleted-reassignment.service.ts | Implements reassignment to matching default role; logs and fails when default role is missing. |
| packages/ocom/domain/src/domain/services/user/staff-role-deleted-reassignment.service.test.ts | Adds Cucumber/Vitest tests for reassignment success/idempotency/missing-default failure. |
| packages/ocom/domain/src/domain/services/user/index.ts | Exposes StaffRoleDeletedReassignmentService via Domain.Services.User. |
| packages/ocom/domain/src/domain/services/user/index.test.ts | Tests Domain.Services.User export shape. |
| packages/ocom/domain/src/domain/services/user/features/staff-role-deleted-reassignment.service.feature | Gherkin coverage for reassignment service behavior. |
| packages/ocom/domain/src/domain/services/user/features/index.feature | Gherkin coverage for user services index export. |
| packages/ocom/domain/src/domain/services/index.ts | Exports User services alongside Community. |
| packages/ocom/domain/src/domain/services/index.test.ts | Tests services index exports include User. |
| packages/ocom/domain/src/domain/services/features/index.feature | Updates Gherkin for services index export coverage. |
| packages/ocom/domain/src/domain/iam/user/staff-user/contexts/staff-user.user.passport.ts | Propagates canRemoveRole into user-domain permissions for staff users. |
| packages/ocom/domain/src/domain/iam/user/staff-user/contexts/staff-user.user.passport.test.ts | Adds passport tests for canRemoveRole mapping. |
| packages/ocom/domain/src/domain/iam/user/staff-user/contexts/features/staff-user.user.passport.feature | Adds Gherkin scenarios for canRemoveRole mapping. |
| packages/ocom/domain/src/domain/iam/member/contexts/member.user.vendor-user.visa.ts | Extends member visa permissions with canRemoveRole: false. |
| packages/ocom/domain/src/domain/iam/member/contexts/member.user.staff-user.visa.ts | Extends member visa permissions with canRemoveRole: false. |
| packages/ocom/domain/src/domain/iam/member/contexts/member.user.staff-role.visa.ts | Extends member visa permissions with canRemoveRole: false. |
| packages/ocom/domain/src/domain/iam/member/contexts/member.user.end-user.visa.ts | Extends member visa permissions with canRemoveRole: false. |
| packages/ocom/domain/src/domain/events/types/staff-role-deleted.ts | Introduces StaffRoleDeletedEvent payload carrying enterpriseAppRole. |
| packages/ocom/domain/src/domain/events/types/index.ts | Exports StaffRoleDeletedEvent. |
| packages/ocom/domain/src/domain/contexts/user/user.domain-permissions.ts | Adds canRemoveRole to shared UserDomainPermissions. |
| packages/ocom/domain/src/domain/contexts/user/staff-user/staff-user.repository.ts | Extends domain repository contract with getAllAssignedToRole. |
| packages/ocom/domain/src/domain/contexts/user/staff-user/staff-user-activity-log.entity.additional.test.ts | Updates test permissions object to include canRemoveRole. |
| packages/ocom/domain/src/domain/contexts/user/staff-role/staff-role.ts | Replaces legacy delete-and-reassign with requestDelete() that emits StaffRoleDeletedEvent and enforces permission/default-role rules. |
| packages/ocom/domain/src/domain/contexts/user/staff-role/staff-role.test.ts | Updates unit tests/features for requestDelete, idempotency, and new default-role helpers. |
| packages/ocom/domain/src/domain/contexts/user/staff-role/features/staff-role.feature | Updates Gherkin for delete semantics (requestDelete) and idempotency. |
| packages/ocom/application-services/src/contexts/user/staff-role/index.ts | Replaces deleteAndReassign with delete service entry point. |
| packages/ocom/application-services/src/contexts/user/staff-role/index.test.ts | Updates application service delegation tests for delete. |
| packages/ocom/application-services/src/contexts/user/staff-role/features/index.feature | Updates Gherkin for application service delete delegation. |
| packages/ocom/application-services/src/contexts/user/staff-role/features/delete.feature | Adds Gherkin for delete service behavior. |
| packages/ocom/application-services/src/contexts/user/staff-role/features/delete-and-reassign.feature | Removes legacy delete-and-reassign Gherkin. |
| packages/ocom/application-services/src/contexts/user/staff-role/delete.ts | Implements delete app-service calling requestDelete() and saving. |
| packages/ocom/application-services/src/contexts/user/staff-role/delete.test.ts | Adds tests for delete service success/not-found/domain-refusal paths. |
| packages/ocom/application-services/src/contexts/user/staff-role/delete-and-reassign.ts | Removes legacy delete-and-reassign implementation. |
| packages/ocom/application-services/src/contexts/user/staff-role/delete-and-reassign.test.ts | Removes legacy delete-and-reassign tests. |
| packages/ocom-verification/verification-shared/src/scenarios/staff/staff-role-management.feature | Adds shared Gherkin scenarios for delete flow, permission gating, cancel, and failure behavior. |
| packages/ocom-verification/verification-shared/src/pages/staff-users-list.page.ts | Adds page object for staff users list (used by acceptance/e2e). |
| packages/ocom-verification/verification-shared/src/pages/staff-user-detail.page.ts | Adds page object for staff user detail screen (role select + displayed role name). |
| packages/ocom-verification/verification-shared/src/pages/staff-role-form.page.ts | Adds delete-action elements and helpers to staff role form page object. |
| packages/ocom-verification/verification-shared/src/pages/index.ts | Exports new staff user page objects. |
| packages/ocom-verification/e2e-tests/src/contexts/staff-role/tasks/delete-staff-role.ts | Adds e2e tasks for delete / start delete / cancel delete. |
| packages/ocom-verification/e2e-tests/src/contexts/staff-role/tasks/assign-staff-role-to-user.ts | Adds e2e task to assign staff role to user via staff user detail flow. |
| packages/ocom-verification/e2e-tests/src/contexts/staff-role/step-definitions/staff-role-management.steps.ts | Adds step definitions for delete flow and reassignment assertion. |
| packages/ocom-verification/e2e-tests/src/contexts/staff-role/questions/staff-user-screen.ts | Adds polling question for post-delete reassignment completion. |
| packages/ocom-verification/e2e-tests/src/contexts/staff-role/questions/staff-role-screen.ts | Adds questions for delete action visibility/confirmation text/list exclusion. |
| packages/ocom-verification/e2e-tests/src/contexts/staff-role/interactions/delete-staff-role-actions.ts | Adds low-level delete/cancel/confirm interactions. |
| packages/ocom-verification/e2e-tests/src/contexts/staff-role/abilities/staff-user-page.ts | Adds Playwright ability helpers to navigate users list and open detail screen. |
| packages/ocom-verification/acceptance-ui/src/contexts/staff-role/tasks/delete-staff-role.ts | Adds DOM-acceptance UI tasks for delete flow (start/confirm/cancel). |
| packages/ocom-verification/acceptance-ui/src/contexts/staff-role/step-definitions/staff-role-management.steps.tsx | Adds acceptance-ui step definitions for delete/cancel/failure gating. |
| packages/ocom-verification/acceptance-ui/src/contexts/staff-role/questions/staff-role-screen.ts | Adds acceptance-ui questions for delete action/confirmation/list exclusion. |
| packages/ocom-verification/acceptance-ui/src/contexts/staff-role/abilities/mock-staff-role-backend.ts | Extends mock backend with delete mutation mocking and isDefault handling. |
| packages/ocom-verification/acceptance-api/src/world.ts | Registers delete staff role ability for acceptance-api actors. |
| packages/ocom-verification/acceptance-api/src/shared/graphql/staff-role-operations.ts | Adds delete mutation string for API verification client. |
| packages/ocom-verification/acceptance-api/src/shared/abilities/index.ts | Exports delete ability. |
| packages/ocom-verification/acceptance-api/src/shared/abilities/delete-staff-role.ts | Adds Serenity ability to delete staff roles via GraphQL mutation. |
| packages/ocom-verification/acceptance-api/src/mock-application-services.ts | Registers production event handlers once so integration events run in acceptance-api environment. |
| packages/ocom-verification/acceptance-api/src/contexts/staff-role/tasks/delete-staff-role.ts | Adds acceptance-api task to delete a staff role and record outcome notes. |
| packages/ocom-verification/acceptance-api/src/contexts/staff-role/step-definitions/staff-role-management.steps.ts | Adds API steps for delete flow and polling for async reassignment completion. |
| packages/ocom-verification/acceptance-api/package.json | Adds dependency on @ocom/event-handler for acceptance-api handler registration. |
| .snyk | Extends ignore expiry for SNYK-JS-OPENTELEMETRYCORE-17373280. |
Apply focused fixes for failure propagation, assignment races, permission consistency, and audit attribution on top of the original staff-role deletion implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
henry-casper
force-pushed
the
feat/Allow-Authorized-Staff-Users-to-Delete-Non-Default-Staff-Roles
branch
from
July 23, 2026 02:34
3c24594 to
0c2f3b8
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reassign deleted-role users in bounded transactions so later failures preserve completed batches, and decouple deletion success from list refresh failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Treat post-commit reassignment failures as a successful deletion with a recovery warning so the UI clears stale role data and navigates away. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Retry the remote pipeline after an unrelated acceptance UI timing failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Staff users with staffRolePermissions.canRemoveRole can delete non-default StaffRoles from the staff-role details page. Deletion raises a StaffRoleDeletedEvent whose integration handler reassigns every staff user holding the deleted role to the default StaffRole matching the deleted role's enterpriseAppRole (idempotent; missing default role is logged and surfaced as a processing failure).
Summary by Sourcery
Introduce a full stack flow for deleting non-default staff roles, including permission-gated UI, GraphQL mutation, application service, domain event, and integration handler that reassigns affected staff users to matching default roles.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Chores: