Gsn ocom coverage staff user - #304
Conversation
Reviewer's GuideAdds full BDD-style coverage for staff user management (provisioning, role assignment, access control, activity logs, list and detail views) across acceptance UI, acceptance API, and E2E layers, backed by shared page objects and a mock staff-user backend integrated with the existing staff-role mock/auth plumbing. Sequence diagram for viewing a staff user’s details via the new UI taskssequenceDiagram
actor StaffAdmin
participant ViewStaffUserDetails as ViewStaffUserDetails.performAs
participant RenderStaffUsersScreen as RenderStaffUsersScreen
participant StaffUsersListPage as StaffUsersListPage
participant StaffUserDetailPage as StaffUserDetailPage
participant StaffUserMocks as buildStaffUserMocks
StaffAdmin->>ViewStaffUserDetails: performAs(actor)
activate ViewStaffUserDetails
ViewStaffUserDetails->>StaffAdmin: attemptsTo(RenderStaffUsersScreen())
activate RenderStaffUsersScreen
RenderStaffUsersScreen->>StaffUserMocks: resetStaffUserUiState()
RenderStaffUsersScreen->>StaffUserMocks: setCurrentStaffUser(Alice, finance)
RenderStaffUsersScreen->>StaffUserMocks: setStaffUserUiState(Bob, finance)
RenderStaffUsersScreen->>StaffUserMocks: buildStaffUserMocks()
RenderStaffUsersScreen-->>StaffAdmin: UI rendered with StaffUsersPage
deactivate RenderStaffUsersScreen
ViewStaffUserDetails->>StaffUsersListPage: listedUserNames()
StaffUsersListPage-->>ViewStaffUserDetails: [Alice, Bob]
ViewStaffUserDetails->>StaffUsersListPage: clickRowForUser(Bob)
StaffUsersListPage-->>ViewStaffUserDetails: row clicked
ViewStaffUserDetails->>StaffUserDetailPage: heading.isVisible()
StaffUserDetailPage-->>ViewStaffUserDetails: true
ViewStaffUserDetails->>StaffAdmin: attemptsTo(notes.set(staffUserName, Bob))
ViewStaffUserDetails->>StaffAdmin: attemptsTo(notes.set(result, details-visible))
deactivate ViewStaffUserDetails
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}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/ocom-verification/acceptance-ui/src/contexts/staff/abilities/mock-staff-user-backend.ts" line_range="158" />
<code_context>
+ {
+ request: { query: StaffUserAssignRoleDocument },
</code_context>
<issue_to_address>
**issue (testing):** The StaffUserAssignRole mock reports success but doesn’t update the in‑memory user state, so subsequent queries will still return the old role.
This mock returns success but never updates `uiStaffUsers` (or `activityLog`), so later queries via `StaffUsersListDocument`/`StaffUserDetailDocument` will still show the old role. Please update the relevant `MockStaffUser` (and optionally `activityLog`) so the mock state matches the assigned role and better reflects real backend behavior.
</issue_to_address>
### Comment 2
<location path="packages/ocom-verification/e2e-tests/src/contexts/staff/step-definitions/staff-user-management.steps.ts" line_range="10-11" />
<code_context>
+import { ViewStaffUserDetails } from '../tasks/view-staff-user-details.ts';
+import { ViewStaffUsersList } from '../tasks/view-staff-users-list.ts';
+
+const userState = new Map<string, { role: string; activityLog: string[] }>();
+let currentActorName = '';
+
+const ensureUser = (name: string, role: string) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Shared `userState` map is never reset between scenarios, which can cause cross-scenario pollution in E2E tests.
Since `userState` and `currentActorName` are module-level and never cleared, data (e.g. users created in one scenario) can persist into later scenarios, making tests order-dependent and flaky. Add a `Before`/`After` hook in this file to reset `userState.clear()` and `currentActorName = ''` for each scenario, or move this state into a Cucumber World/Serenity-scoped object so it’s naturally isolated per scenario.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| { | ||
| request: { query: StaffUserAssignRoleDocument }, | ||
| variableMatcher: () => true, | ||
| result: () => ({ |
There was a problem hiding this comment.
issue (testing): The StaffUserAssignRole mock reports success but doesn’t update the in‑memory user state, so subsequent queries will still return the old role.
This mock returns success but never updates uiStaffUsers (or activityLog), so later queries via StaffUsersListDocument/StaffUserDetailDocument will still show the old role. Please update the relevant MockStaffUser (and optionally activityLog) so the mock state matches the assigned role and better reflects real backend behavior.
| const userState = new Map<string, { role: string; activityLog: string[] }>(); | ||
| let currentActorName = ''; |
There was a problem hiding this comment.
issue (bug_risk): Shared userState map is never reset between scenarios, which can cause cross-scenario pollution in E2E tests.
Since userState and currentActorName are module-level and never cleared, data (e.g. users created in one scenario) can persist into later scenarios, making tests order-dependent and flaky. Add a Before/After hook in this file to reset userState.clear() and currentActorName = '' for each scenario, or move this state into a Cucumber World/Serenity-scoped object so it’s naturally isolated per scenario.
* fix: update task dependencies to ensure proper build order for e2e test environment; update dependency overrides in pnpm-workspace.yaml to resolve vulnerabilities * fix: update @azure/storage-queue dependency to pin catalog version in pnpm-workspace.yaml * fix: keep react router versions aligned Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: normalize mock seed object ids Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: preserve mongoose readonly document types Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use explicit object id string conversion Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: avoid implicit mongodb id stringification Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Bot <devnull@example.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lixJs/cellixjs into gsn_ocom_coverage_staff_user
There was a problem hiding this comment.
This comment applies to all of the tasks under staff context for acceptance-api.
The overall setup and structure is aligned but the implementation is not fully exercising the system to actually give us confidence that the logic in the API is behaving as the scenarios expect.
Currently, the tasks just record in-memory state in the form of those Serenity notes, when the step definitions execute them during the scenarios at runtime. However, this doesn't actually hit anywhere in our application code for the relevant functionality we're trying to test. Without this, tests passing or failing does not tell us anything meaningful about the state of the code related to the functionality.
For acceptance-api in particular, you will need to exercise the GraphlQL API on our backend and perform the relevant queries and mutations in your tasks which are then verified in the appropriate scenarios for that context and then we will have the confidence the system works as expected.
Since the queries/mutations are typically reused across multiple tasks for different scenarios, we abstract them into Abilities which can be shared across multiple tasks.
Take reference from the other existing abilities used in tasks for other contexts in acceptance-api as needed.
(src/shared/abilities/)
There was a problem hiding this comment.
This comment applies to the new tasks and step-definitions under the staff context in @ocom-verification/acceptance-ui.
The overall setup and structure is aligned but the implementation is not fully exercising the actual staff UI to give us confidence that the application behaves as the scenarios expect.
Right now, several of the tasks and step definitions are just recording in-memory state (Serenity notes plus the local userState map) when the steps execute at runtime. In particular, tasks like ProvisionStaffUser, UpdateStaffUserRole and RecordAccessResult only manipulate StaffUserManagementUiNotes, and the step definitions maintain their own userState/activityLog. These flows can all “pass” without ever rendering StaffUsersPage, using the shared page objects, or going through the mock staff user backend.
For acceptance-ui, the expectation is that scenarios render the real components and drive them via the shared page objects and mock GraphQL backend, using notes only for cross-step state. The staff-role context is a good reference: it renders StaffRolesPage via RenderStaffRolesScreen, uses StaffRolesListPage/StaffRoleFormPage, and relies on the mock-staff-role-backend and Questions for assertions.
For the staff user context in acceptance-ui, please refactor so that:
-
the “When … provisions/updates …” steps actually go through StaffUsersPage (via RenderStaffUsersScreen) and the existing staff user page objects, instead of just mutating userState and notes
-
assertions like “user X should be created/updated with role Y” and “activity log includes …” are based on UI/mocked-backend state rather than the in-memory map
-
the result note (used in “Alice should be blocked with …”) is derived from observed UI behaviour (routing, visible messages, etc.) rather than being pre-set in Given/When steps
That keeps backend logic coverage in acceptance-api and ensures acceptance-ui is consistently testing the real UI for these staff scenarios.
There was a problem hiding this comment.
This comment applies to the new tasks and step-definitions under the staff context in @ocom-verification/e2e-tests.
The overall setup and structure is aligned but the implementation is not exercising the actual staff portal UI in the same way as the community and staff-role e2e contexts.
Right now, the staff user step definitions and supporting tasks are just recording in-memory state (userState + StaffUserManagementE2ENotes) when the steps execute. Tasks like ProvisionStaffUser, UpdateStaffUserRole, ViewStaffUsersList, ViewStaffUserDetails and RecordAccessResult only manipulate notes; they never go through staffPortalPageOf, navigate to the staff users screens, or use the StaffUsersListPage / StaffUserDetailPage page objects.
By contrast, the community and staff-role e2e contexts always drive the real browser via Playwright page adapters and shared page objects, and use notes only to capture outcomes.
For the staff context in e2e-tests, please refactor so that:
-
Given/When steps use staffPortalPageOf + the existing staff page objects to navigate to /staff/user-management/staff-users and interact with the UI
-
userState is not the source of truth; the staff portal UI (and underlying API) should determine whether a user exists and what role they have
-
assertions like “user X should be created/updated with role Y” and “activity log includes …” are based on observed UI / backend behaviour rather than an in-memory map
-
the result note used in “Alice should be blocked with …” is derived from actual routing / visible UI feedback instead of being pre-set in the Given/When steps
This will bring the staff e2e tests in line with the existing community and staff-role patterns and ensure we’re actually testing the live staff portal rather than simulating it in step definitions.
2 participants
Summary by Sourcery
Add staff user management coverage across verification layers, including list and detail views, provisioning, role updates, and access control rules.
New Features:
Enhancements:
Tests: