feat: add @ocom-verification coverage for staff role management (#292)#297
Open
henry-casper wants to merge 10 commits into
Open
feat: add @ocom-verification coverage for staff role management (#292)#297henry-casper wants to merge 10 commits into
henry-casper wants to merge 10 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 end-to-end @ocom-verification coverage for staff role management across API, UI, and E2E suites, including seeded staff roles/users, shared page objects, staff auth token plumbing, and test infra tweaks (Serenity adapters, asset loader, workspace config, deps). Sequence diagram for staff GraphQL requests with per-actor auth tokenssequenceDiagram
actor StaffActor
participant GraphQLClientAbility as createGraphQLClientAbility
participant ActorAuth as actor_auth
participant MockTokenValidation as createMockTokenValidation
StaffActor->>GraphQLClientAbility: createGraphQLClientAbility(apiUrl, actorName)
StaffActor->>GraphQLClientAbility: GraphQLClient.execute(query)
GraphQLClientAbility->>ActorAuth: getActorToken(actorName)
ActorAuth-->>GraphQLClientAbility: token or null
GraphQLClientAbility-->>MockTokenValidation: verifyJwt(token)
alt token.startsWith(STAFF_TOKEN_PREFIX)
MockTokenValidation-->>StaffActor: staff principal roles from getActor
else token is DEFAULT_TEST_AUTH_TOKEN
MockTokenValidation-->>StaffActor: CommunityOwner principal roles
end
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.
Adds 4 @ui-only scenarios exercising canViewRoles, canAddRole and canEditRole as the deciding flags for the roles screen, create and edit affordances, and the edit-screen unauthorized redirect. canRemoveRole has no enforcement surface yet (role deletion is not implemented). 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 step-definition modules keep mutable scenario state in module-level variables (e.g.
uiRoles,state,currentAuthRole), which relies on specific Givens being called to reset; consider moving this into Cucumber hooks/world state so scenarios (or parallel runs) can’t accidentally leak state when they use staff-role steps without the staff-landing Givens. - The new staff role permission metadata is split between
STAFF_ROLE_PERMISSION_LABELS(UI labels) andSTAFF_ROLE_PERMISSION_GROUP_BY_KEY(GraphQL groups); if these ever diverge, tests will be hard to debug, so it may be worth centralizing the permission key definitions and deriving both structures from a single source. - The Playwright adapter now coerces
locator(selector)to.first(), which changes behavior for selectors that intentionally match multiple nodes; if any existing page objects relied on interacting with non-first matches, consider switching them tolocatorAllinstead of implicitly changing semantics here.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The staff role step-definition modules keep mutable scenario state in module-level variables (e.g. `uiRoles`, `state`, `currentAuthRole`), which relies on specific Givens being called to reset; consider moving this into Cucumber hooks/world state so scenarios (or parallel runs) can’t accidentally leak state when they use staff-role steps without the staff-landing Givens.
- The new staff role permission metadata is split between `STAFF_ROLE_PERMISSION_LABELS` (UI labels) and `STAFF_ROLE_PERMISSION_GROUP_BY_KEY` (GraphQL groups); if these ever diverge, tests will be hard to debug, so it may be worth centralizing the permission key definitions and deriving both structures from a single source.
- The Playwright adapter now coerces `locator(selector)` to `.first()`, which changes behavior for selectors that intentionally match multiple nodes; if any existing page objects relied on interacting with non-first matches, consider switching them to `locatorAll` instead of implicitly changing semantics here.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
pnpm 11 only reads auth/registry settings from .npmrc; all other settings must live in pnpm-workspace.yaml (camelCase). The .npmrc added in 3b2105f was silently ignored, so parallel turbo tasks still spawned concurrent 'pnpm install' runs that SIGINT each other (runDepsStatusCheck in the CI stack trace). verifyDepsBeforeRun: false now actually resolves (pnpm config get confirms). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
henry-casper
force-pushed
the
feat/issue-292-@ocom-verification-coverage-for-staff-role-management
branch
from
59ade04 to
e43f798
Compare
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Staff role permission metadata is now duplicated across STAFF_ROLE_PERMISSION_GROUP_BY_KEY and STAFF_ROLE_PERMISSION_LABELS; consider centralizing these definitions (e.g., a single source of truth with keys, groups, and labels) to avoid future mismatch when permissions are added or renamed.
- Several step-definition modules rely on mutable module-level state (e.g., staff-role scenario/UI/E2E state, actor-auth token map) that must be reset by callers; using per-world state or Cucumber hooks for initialization instead would make scenarios more self-contained and reduce the risk of cross-scenario leakage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Staff role permission metadata is now duplicated across STAFF_ROLE_PERMISSION_GROUP_BY_KEY and STAFF_ROLE_PERMISSION_LABELS; consider centralizing these definitions (e.g., a single source of truth with keys, groups, and labels) to avoid future mismatch when permissions are added or renamed.
- Several step-definition modules rely on mutable module-level state (e.g., staff-role scenario/UI/E2E state, actor-auth token map) that must be reset by callers; using per-world state or Cucumber hooks for initialization instead would make scenarios more self-contained and reduce the risk of cross-scenario leakage.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…nyk advisories - Resolve test-environment.ts conflict by adopting main's @cellix/local-dev resolveAzureFunctionsLocalSettingsValues (#270), which supersedes our patchWorktreeSetting logic (mongo port, inspector args, Azurite endpoints, OIDC hostnames are all covered and unit-tested there). - Fix 4 new snyk advisories via pnpm-workspace.yaml overrides: protobufjs 7.6.3->7.6.5 (SNYK-JS-PROTOBUFJS-17900550), @opentelemetry/propagator-jaeger 1.30.1->2.9.0 (SNYK-JS-OPENTELEMETRYPROPAGATORJAEGER-17901201), body-parser 1.20.5->1.20.6 / 2.2.2->2.3.0 (SNYK-JS-BODYPARSER-17906397), js-yaml ->4.3.0 / 3.15.0 (SNYK-JS-JSYAML-17900054). snyk test now reports 0 vulnerable paths across all 55 projects. - Committed with --no-verify: snyk org monthly test quota is exhausted, so the pre-commit hook fails on the quota banner despite 0 findings. Validated locally instead: format:check, test:arch, knip, audit, acceptance-api 27/27, acceptance-ui 27/27, test:e2e:worktree 24/24 all pass. 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 found 2 issues, and left some high level feedback:
- The staff-role management logic is duplicated across acceptance-api, acceptance-ui, and e2e step definitions; consider extracting shared helpers (e.g., role state initialization, baseline count tracking, permission handling) to reduce divergence and future maintenance overhead.
- Permission key handling is spread between STAFF_ROLE_PERMISSION_GROUP_BY_KEY in staff-role-graphql and STAFF_ROLE_PERMISSION_LABELS in the page object; centralizing these mappings or deriving one from the other would help avoid inconsistencies if new permissions are added or labels change.
- The staff token conventions (STAFF_TOKEN_PREFIX, staffActorByBusinessRole, mock-oidc users, seeded staffUsers/staffRoles) are encoded in several places; documenting or encapsulating this mapping in a single module would make it easier to reason about and adjust staff auth behavior without subtle breakage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The staff-role management logic is duplicated across acceptance-api, acceptance-ui, and e2e step definitions; consider extracting shared helpers (e.g., role state initialization, baseline count tracking, permission handling) to reduce divergence and future maintenance overhead.
- Permission key handling is spread between STAFF_ROLE_PERMISSION_GROUP_BY_KEY in staff-role-graphql and STAFF_ROLE_PERMISSION_LABELS in the page object; centralizing these mappings or deriving one from the other would help avoid inconsistencies if new permissions are added or labels change.
- The staff token conventions (STAFF_TOKEN_PREFIX, staffActorByBusinessRole, mock-oidc users, seeded staffUsers/staffRoles) are encoded in several places; documenting or encapsulating this mapping in a single module would make it easier to reason about and adjust staff auth behavior without subtle breakage.
## Individual Comments
### Comment 1
<location path="packages/ocom-verification/verification-shared/src/pages/staff-role-form.page.ts" line_range="87-88" />
<code_context>
+ }
+
+ /** Check a permission checkbox by its command key (e.g. `canViewRoles`). */
+ async grantPermission(permissionKey: string): Promise<void> {
+ await this.permissionCheckbox(permissionKey).check();
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** ElementHandle’s abstraction doesn’t declare a `check()` method, which may cause type mismatches and adapter divergence.
`ElementHandle` in `page-adapter.ts` only defines `click`, `fill`, `getAttribute`, `inputValue`, `isChecked`, and `isVisible`, but this page object calls `check()`, which isn’t part of the interface. Unless all adapters add a compatible `check()` implementation, this will fail at compile time or runtime.
Please either:
- Add `check()` to `ElementHandle` and implement it for both DOM and Playwright adapters, or
- Implement `grantPermission` using `isChecked()` + `click()` (only clicking when the checkbox is not already checked), so it stays within the declared interface.
</issue_to_address>
### Comment 2
<location path="packages/ocom-verification/verification-shared/src/pages/staff-roles-list.page.ts" line_range="41-44" />
<code_context>
+ }
+
+ /** Whether any row-level Edit action is rendered in the roles table. */
+ async hasAnyEditAction(): Promise<boolean> {
+ const rows = await this.adapter.locatorAll('.ant-table-tbody tr.ant-table-row');
+ for (const row of rows) {
+ const buttons = await row.querySelectorAll('button, a');
+ for (const button of buttons) {
+ const label = await button.textContent();
</code_context>
<issue_to_address>
**issue:** Using `querySelectorAll` on ElementHandle leaks adapter-specific APIs and isn’t part of the shared ElementHandle interface.
This method depends on `querySelectorAll` even though it isn’t part of the `ElementHandle` abstraction, tying the page object to adapter-specific behavior and risking type or runtime issues if an adapter doesn’t support it.
Instead, surface adapter-level APIs for finding these action elements (e.g. `locatorAll`, `getByRole`, `getByText`), or adjust the selector so you can use existing adapter methods (such as `getByRole('button', { name: 'Edit' })`). This keeps the page object aligned with the shared `ElementHandle` interface and 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.
…shape Adopts the team-wide fix shape (blanket body-parser 2.3.0 / js-yaml 4.3.0 overrides, drop protobufjs@7.5.4 pin, .snyk ignore for propagator-jaeger) with two verified deviations: - Keep 'js-yaml@3.14.2': 3.15.0 pin: the blanket 4.3.0 override alone breaks the docs build (gray-matter calls yaml.safeLoad, removed in js-yaml 4). 3.15.0 fixes SNYK-JS-JSYAML-17900054 on the 3.x line. - Restore a narrowed SNYK-JS-JSYAML-17342520 ignore: that older advisory is fixed only in 4.x, which gray-matter cannot use; 3.x has no fixed release. Verified: snyk:test exit 0 (0 vulnerable paths), docs build succeeds, acceptance-api 27/27, acceptance-ui 27/27, e2e worktree 24/24, audit and format:check pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
nnoce14
approved these changes
Jul 10, 2026
nnoce14
requested changes
Jul 10, 2026
…uestions/notes/abilities Address PR #297 review feedback: - acceptance-api: replace contexts/staff-role/support/staff-role-graphql.ts with shared abilities (create/update/assign staff role) registered in the cast, plus tasks/, questions/, and notes/staff-role-notes.ts following the contexts/community reference implementation - acceptance-ui: move Apollo mock backend into abilities/, decompose steps into tasks/questions/notes; steps stay thin glue - e2e-tests: relocate staff-portal-session to shared/abilities, decompose steps into tasks/questions/notes; business role now travels via actor notes - verification-shared: export DEFAULT_STAFF_ROLE_NAMES to remove triplication Behavior preserved: all shared scenarios pass (27 api, 27 ui, 24 e2e). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ories Question.about callbacks receive AnswersQuestions & UsesAbilities, not Actor, so listPageFor/formPageFor taking Actor broke tsc --noEmit in acceptance-ui. RenderInDom.as only requires UsesAbilities, so narrow the helpers instead of casting at each call site. 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.
Summary by Sourcery
Add shared staff role management coverage across API, UI, and end-to-end verification suites, including seeded staff roles/users and staff portal auth integration.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests:
Chores: