feat: add @ocom-verification coverage for staff role management (#292)#297
Open
henry-casper wants to merge 4 commits into
Open
feat: add @ocom-verification coverage for staff role management (#292)#297henry-casper wants to merge 4 commits into
henry-casper wants to merge 4 commits into
Conversation
- verification-shared: 12 shared Gherkin scenarios for staff role creation, listing, viewing, updates, permission enforcement, and error cases (duplicates, insufficient privileges, validation); staff-role/staff-user seed data, page objects, and test actors - acceptance-api: staff-role step definitions and GraphQL glue covering backend behavior, persistence, and error handling (27/27) - acceptance-ui: staff portal role management step definitions with component wrappers (23/23) - e2e-tests: end-to-end staff role scenarios combining UI actions with API verification (24/24) - serenity-framework: isChecked support and first-match locator handling in page adapters - e2e-tests: fix worktree env leak in test-environment.ts where local-settings.e2e.json values overrode worktree-scoped ports and OIDC hosts in spawned app servers (test:e2e:worktree now passes) - security: bump brace-expansion@5 to 5.0.7 and webpack-dev-server to ^5.2.6 via overrides; add .snyk ignore for SNYK-JS-BRACEEXPANSION-17706650 on minimatch@3 paths (only fixed in 5.0.7, which has an incompatible API; Docusaurus tooling only) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Reviewer's GuideAdds comprehensive staff role management verification across shared Gherkin, API, UI, and E2E suites, including seeded staff roles/users, shared staff role page objects, staff-auth aware token routing, and Serenity adapter enhancements, plus fixes for worktree env leakage and Apollo ESM resolution and small security/build config updates. Sequence diagram for staff-actor GraphQL call with role-based tokensequenceDiagram
actor StaffActor
participant SerenityCast as SerenityCast
participant GraphQLClient as GraphQLClientAbility
participant HttpApi as GraphQL_HTTP_API
participant TokenValidation as MockTokenValidation
StaffActor->>SerenityCast: execute GraphQL step
SerenityCast->>GraphQLClient: createGraphQLClientAbility(apiUrl, actor.name)
StaffActor->>GraphQLClient: execute(query, variables)
GraphQLClient->>GraphQLClient: getActorToken(actor.name)
GraphQLClient-->>HttpApi: HTTP request
Note over GraphQLClient,HttpApi: headers(): { Authorization: token || {} }
HttpApi->>TokenValidation: verifyJwt(token)
alt token startsWith STAFF_TOKEN_PREFIX
TokenValidation-->>HttpApi: StaffPortal principal with roles from getActor
else default token or null
TokenValidation-->>HttpApi: CommunityOwner or unauthorized
end
HttpApi-->>StaffActor: GraphQL response (authorized/denied)
File-Level Changes
Assessment against linked issues
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 left some high level feedback:
- The staff-role UI acceptance steps keep mutable scenario state (e.g.
uiRoles,baselineRoleCount,lastMutation) in module-level variables; if Cucumber ever runs scenarios in parallel within the same process this will race—consider scoping this state to the world/actor instead of module globals. - In
StaffRoleFormPagethe permission key → label mapping is hard-coded and separate from the GraphQL permission grouping logic instaff-role-graphql.ts; consider centralising these permission definitions so changes to permissions or labels can’t drift between layers. - The
PlaywrightPageAdapter.locator()now unconditionally calls.first(), which silently changes behaviour for existing selectors; you might want to either keeplocator()returning the full locator and introduce a separate helper forfirst(), or audit current call sites and document thatlocator()is now intentionally first-match-only.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The staff-role UI acceptance steps keep mutable scenario state (e.g. `uiRoles`, `baselineRoleCount`, `lastMutation`) in module-level variables; if Cucumber ever runs scenarios in parallel within the same process this will race—consider scoping this state to the world/actor instead of module globals.
- In `StaffRoleFormPage` the permission key → label mapping is hard-coded and separate from the GraphQL permission grouping logic in `staff-role-graphql.ts`; consider centralising these permission definitions so changes to permissions or labels can’t drift between layers.
- The `PlaywrightPageAdapter.locator()` now unconditionally calls `.first()`, which silently changes behaviour for existing selectors; you might want to either keep `locator()` returning the full locator and introduce a separate helper for `first()`, or audit current call sites and document that `locator()` is now intentionally first-match-only.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
This PR expands the @ocom-verification suites to cover staff role management end-to-end (shared Gherkin scenarios + API/UI/E2E step definitions), adds supporting seeded staff-role/staff-user test data, and makes small framework/environment updates needed to execute those scenarios reliably.
Changes:
- Add shared staff-role management feature scenarios, page objects, and seeded staff roles/users for verification runs.
- Implement staff-role step definitions across acceptance-api, acceptance-ui, and e2e-tests; update API mock token validation to support per-actor staff principals.
- Update serenity framework adapters (checkbox/value helpers + Playwright first-match locator) and patch E2E worktree env leakage; bump/override vulnerable transitive deps and add a scoped Snyk ignore.
Review Notes (actionable)
- pnpm override risk (semver break):
pnpm-workspace.yamlchangesbrace-expansion@1→2.0.3. This is a major-version jump for any dependency that explicitly asked for@1, which can violate semver expectations and break tooling at runtime. Consider narrowing the override to the specific transitive paths that need it (e.g., minimatch@3 chains) rather than blanket-upgrading all@1requests. - Behavior change in Playwright adapter:
PlaywrightPageAdapter.locator()now forces.first(). This silently “picks one” when a selector matches multiple elements, potentially masking ambiguous selectors that would otherwise fail loudly (useful signal in tests). If the intent is to support “first match” semantics, consider a separate API (or making this opt-in) instead of changinglocator()globally. - Doc/route mismatch:
StaffRolesListPage’s docstring references/staff/user-management/roles, while the E2E step defs navigate to/staff/user-management/staff-roles. Aligning the page-object documentation with the exercised route will prevent confusion and future misnavigation in tests.
Reviewed changes
Copilot reviewed 46 out of 48 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Updates overrides for brace-expansion and webpack-dev-server. |
| pnpm-lock.yaml | Lockfile updates reflecting overrides and added deps. |
| packages/ocom-verification/verification-shared/src/test-data/test-actors.ts | Adds staff actor roles and additional staff actors. |
| packages/ocom-verification/verification-shared/src/test-data/seed/staff-users.ts | New seeded staff-user documents for verification suites. |
| packages/ocom-verification/verification-shared/src/test-data/seed/staff-roles.ts | New seeded staff-role documents + permission shape. |
| packages/ocom-verification/verification-shared/src/test-data/seed/seed.ts | Extends MongoDB seeding to include staff users/roles. |
| packages/ocom-verification/verification-shared/src/test-data/seed/index.ts | Re-exports staff seed data/types/IDs. |
| packages/ocom-verification/verification-shared/src/test-data/index.ts | Re-exports staff seed artifacts from test-data barrel. |
| packages/ocom-verification/verification-shared/src/scenarios/staff/staff-role-management.feature | Adds shared Gherkin scenarios for staff role management. |
| packages/ocom-verification/verification-shared/src/pages/staff-roles-list.page.ts | New page object for staff roles list UI. |
| packages/ocom-verification/verification-shared/src/pages/staff-role-form.page.ts | New page object + permission label mapping for role form. |
| packages/ocom-verification/verification-shared/src/pages/index.ts | Exposes new staff-role page objects from pages barrel. |
| packages/ocom-verification/e2e-tests/src/step-definitions/index.ts | Registers staff-role E2E step definitions. |
| packages/ocom-verification/e2e-tests/src/shared/environment/test-environment.ts | Fixes worktree env leakage by patching local-settings values. |
| packages/ocom-verification/e2e-tests/src/contexts/staff/step-definitions/staff-landing.steps.ts | Syncs staff-role E2E state resets with staff auth givens. |
| packages/ocom-verification/e2e-tests/src/contexts/staff-role/support/staff-portal-session.ts | Adds staff-portal session reuse + role-based login mapping. |
| packages/ocom-verification/e2e-tests/src/contexts/staff-role/step-definitions/staff-role-management.steps.ts | Implements staff-role E2E steps using shared page objects. |
| packages/ocom-verification/e2e-tests/src/contexts/staff-role/step-definitions/index.ts | Index to load staff-role E2E steps. |
| packages/ocom-verification/e2e-tests/cucumber.js | Filters tags to exclude api-only/ui-only/skip-e2e scenarios. |
| packages/ocom-verification/acceptance-ui/tsconfig.json | Expands include paths for staff user-management UI sources. |
| packages/ocom-verification/acceptance-ui/src/step-definitions/index.ts | Registers staff-role UI step definitions. |
| packages/ocom-verification/acceptance-ui/src/shared/ocom-component-wrapper.ts | Adds MemoryRouter + StaffAuthContext support; popup container fix. |
| packages/ocom-verification/acceptance-ui/src/contexts/staff/step-definitions/create-staff-landing.steps.ts | Syncs staff-role UI state resets with staff auth givens. |
| packages/ocom-verification/acceptance-ui/src/contexts/staff-role/step-definitions/staff-role-management.steps.tsx | Implements staff-role component-acceptance steps with Apollo mocks. |
| packages/ocom-verification/acceptance-ui/src/contexts/staff-role/step-definitions/index.ts | Index to load staff-role UI steps. |
| packages/ocom-verification/acceptance-ui/package.json | Adds react-router-dom dependency for UI acceptance tests. |
| packages/ocom-verification/acceptance-ui/cucumber.js | Filters tags to exclude api-only/e2e-only/skip-ui scenarios. |
| packages/ocom-verification/acceptance-api/src/world.ts | Makes GraphQL client ability actor-aware for auth routing. |
| packages/ocom-verification/acceptance-api/src/step-definitions/index.ts | Registers staff-role API step definitions. |
| packages/ocom-verification/acceptance-api/src/shared/graphql/staff-role-operations.ts | Adds GraphQL operations for staff roles/users mutations/queries. |
| packages/ocom-verification/acceptance-api/src/shared/abilities/graphql-client.ts | Lazy per-actor Authorization header resolution. |
| packages/ocom-verification/acceptance-api/src/shared/abilities/actor-auth.ts | Introduces per-actor token registry + staff token prefix helper. |
| packages/ocom-verification/acceptance-api/src/mock-application-services.ts | Passes raw auth header through; adds staff-token verification path. |
| packages/ocom-verification/acceptance-api/src/cucumber-lifecycle-hooks.ts | Clears per-actor tokens between scenarios. |
| packages/ocom-verification/acceptance-api/src/contexts/staff/step-definitions/staff-landing.steps.ts | Registers staff tokens + resets staff-role scenario state. |
| packages/ocom-verification/acceptance-api/src/contexts/staff-role/support/staff-role-graphql.ts | Adds staff-role API helper functions (query/mutate + permissions mapping). |
| packages/ocom-verification/acceptance-api/src/contexts/staff-role/step-definitions/staff-role-management.steps.ts | Implements staff-role API step definitions and scenario state. |
| packages/ocom-verification/acceptance-api/src/contexts/staff-role/step-definitions/index.ts | Index to load staff-role API steps. |
| packages/ocom-verification/acceptance-api/cucumber.js | Filters tags to exclude ui-only/e2e-only/skip-api scenarios. |
| packages/cellix/server-oauth2-mock-seedwork/tests/portal-discovery.test.ts | Formatting-only test adjustment. |
| packages/cellix/server-oauth2-mock-seedwork/tests/file-user-store.test.ts | Formatting-only test adjustments. |
| packages/cellix/serenity-framework/src/pages/page-adapter.ts | Extends ElementHandle API (inputValue/isChecked). |
| packages/cellix/serenity-framework/src/pages/adapters/playwright-adapter.ts | Implements new ElementHandle APIs; changes locator() to first match. |
| packages/cellix/serenity-framework/src/pages/adapters/dom-adapter.ts | Implements new ElementHandle APIs; improves click sequence for widgets. |
| packages/cellix/serenity-framework/src/dom/asset-loader-hooks.ts | Adds ESM redirect for @apollo/client subpath imports. |
| apps/ui-staff/mock-oidc.users.json | Adds tech-admin mock OIDC user; fixes indentation. |
| apps/ui-community/mock-oidc.users.json | Fixes indentation in mock OIDC users. |
| .snyk | Adds ignore entry for brace-expansion ReDoS advisory on minimatch@3 path. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
…install conflicts in CI pnpm 11 defaults to verify-deps-before-run=install, causing each concurrent turbo task to trigger pnpm install when workspace state is stale. Parallel workers SIGINT each other, killing tasks like @ocom/service-queue-storage#gen. CI already runs 'pnpm install --frozen-lockfile' before turbo tasks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…verification-coverage-for-staff-role-management # Conflicts: # pnpm-lock.yaml # pnpm-workspace.yaml
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
STAFF_ROLE_PERMISSION_LABELSmap tightly couples tests to the exact checkbox label text; consider using stable data attributes or test-ids for permissions to avoid brittleness when copy changes occur in the UI. - The shared UI staff-role mocks (
uiRoles,currentAuthRole, etc.) are maintained in module-level state; if Cucumber is ever run with parallel workers this will cause cross-scenario interference, so it may be worth guarding against parallel execution or scoping the state per-world.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `STAFF_ROLE_PERMISSION_LABELS` map tightly couples tests to the exact checkbox label text; consider using stable data attributes or test-ids for permissions to avoid brittleness when copy changes occur in the UI.
- The shared UI staff-role mocks (`uiRoles`, `currentAuthRole`, etc.) are maintained in module-level state; if Cucumber is ever run with parallel workers this will cause cross-scenario interference, so it may be worth guarding against parallel execution or scoping the state per-world.
## Individual Comments
### Comment 1
<location path="packages/cellix/serenity-framework/src/pages/adapters/playwright-adapter.ts" line_range="33-34" />
<code_context>
return Promise.resolve(this.element?.getAttribute(name) ?? null);
}
+ inputValue(): Promise<string | null> {
+ if (this.element instanceof HTMLInputElement || this.element instanceof HTMLTextAreaElement || this.element instanceof HTMLSelectElement) {
+ return Promise.resolve(this.element.value);
</code_context>
<issue_to_address>
**issue (bug_risk):** Align Playwright inputValue/isChecked semantics with DomElementHandle to avoid runtime errors.
DomElementHandle.inputValue/isChecked return null/false for non‑inputs, but the Playwright adapter currently delegates straight to locator.inputValue()/isChecked, which will throw for unsupported elements. Because shared page objects (e.g. StaffRoleFormPage.permissionCheckbox) call these via the ElementHandle interface and may use either adapter, this creates adapter‑dependent runtime failures. Please align the Playwright behavior with the DOM adapter (e.g. guard via locator.evaluate() before calling, or catch and map errors to null/false) so callers can remain adapter‑agnostic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- serenity-framework: align PlaywrightElementHandle inputValue()/isChecked() with the DomElementHandle contract — resolve null/false for unsupported elements instead of throwing, so shared page objects behave identically across adapters; document the contract on the ElementHandle interface - acceptance-ui: make the TS project typecheckable (tsc --noEmit passes) while keeping the include globs tsx needs for the react-jsx runtime transform: exclude UI stories/tests, disable exactOptionalPropertyTypes to match how the included UI packages typecheck themselves, and add a plain-css ambient module declaration for side-effect stylesheet imports Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The staff role defaults (
DEFAULT_STAFF_ROLE_NAMES) are duplicated across the API, UI, E2E step definitions and the feature file; consider moving this list into a shared verification-shared module to avoid drift when the defaults change. - Both
STAFF_ROLE_PERMISSION_LABELSandSTAFF_ROLE_PERMISSION_GROUP_BY_KEYdepend on hard-coded string keys and throw when a key is missing; it may be safer to centralize permission keys in a shared typed enum/source of truth so adding a new permission can't silently break tests at runtime. - The staff-role step implementations maintain mutable module-level state (
state,uiRoles,idCounter, etc.); if Cucumber is ever configured to run scenarios in parallel or a scenario omits the resetting Givens, this can leak state between tests, so consider tightening initialization/reset (e.g. usingBeforehooks or per-scenario world state) to make isolation explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The staff role defaults (`DEFAULT_STAFF_ROLE_NAMES`) are duplicated across the API, UI, E2E step definitions and the feature file; consider moving this list into a shared verification-shared module to avoid drift when the defaults change.
- Both `STAFF_ROLE_PERMISSION_LABELS` and `STAFF_ROLE_PERMISSION_GROUP_BY_KEY` depend on hard-coded string keys and throw when a key is missing; it may be safer to centralize permission keys in a shared typed enum/source of truth so adding a new permission can't silently break tests at runtime.
- The staff-role step implementations maintain mutable module-level state (`state`, `uiRoles`, `idCounter`, etc.); if Cucumber is ever configured to run scenarios in parallel or a scenario omits the resetting Givens, this can leak state between tests, so consider tightening initialization/reset (e.g. using `Before` hooks or per-scenario world state) to make isolation explicit.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Summary by Sourcery
Add end-to-end verification coverage for staff role management across API, UI, and browser flows, including shared scenarios, test data, and portal wiring.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Chores: