feat: add @ocom-verification coverage for member management (#293)#298
Open
aaron-rab wants to merge 3 commits into
Open
feat: add @ocom-verification coverage for member management (#293)#298aaron-rab wants to merge 3 commits into
aaron-rab wants to merge 3 commits into
Conversation
… headers to 'execute' cellix funciton
Contributor
Reviewer's GuideAdds end-to-end and acceptance coverage for community member creation and profile update flows across API and UI, wires GraphQL client to support per-request headers, and integrates event handlers and principal hints to drive member management scenarios. Sequence diagram for member profile update via API using per-request GraphQL headerssequenceDiagram
actor ApiActor
participant UpdateMember as UpdateMemberTask
participant UpdateMemberProfile as UpdateMemberProfileAbility
participant GraphQLClient
participant ApiServer as MemberAPI
ApiActor->>UpdateMember: performAs(details)
UpdateMember->>ApiActor: notes<MemberNotes>().get('lastMemberId')
UpdateMember->>ApiActor: notes<MemberNotes>().get('lastMemberCommunityId')
UpdateMember->>ApiActor: PrincipalMemberIdForCommunity.inCommunity(communityId)
UpdateMember->>UpdateMemberProfile: performAs(actor, { memberId, profile, communityId, principalMemberId })
UpdateMemberProfile->>GraphQLClient: execute(MEMBER_UPDATE_PROFILE_MUTATION, variables, { headers: { 'x-community-id', 'x-member-id' } })
GraphQLClient->>ApiServer: HTTP POST /graphql
ApiServer-->>GraphQLClient: memberUpdateProfile response
GraphQLClient-->>UpdateMemberProfile: { data, errors? }
UpdateMemberProfile-->>UpdateMember: { id, profile }
UpdateMember->>ApiActor: notes<MemberNotes>().set('lastMemberProfileEmail', profile.email)
UpdateMember->>ApiActor: notes<MemberNotes>().set('lastMemberStatus', 'SUCCESS')
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| await page.goto('/community/accounts', { waitUntil: 'networkidle' }); | ||
| await openAdminPortalFromCommunityRow(page, communityName); | ||
|
|
||
| await page.waitForURL(/\/community\/[^/]+\/admin\/[^/?#]+(?:\/)?(?:\?.*)?$/, { timeout: 15_000 }); |
![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 3 issues, and left some high level feedback:
- The global
eventHandlersRegisteredflag increateMockApplicationServicesFactorymay cause cross-test interference or issues under parallel execution; consider scoping event handler registration per test server or using a factory-level guard instead. - You’re repeatedly re-declaring
ExecuteWithRequestOptionsand castingGraphQLClienttoGraphQLClient & { execute: ExecuteWithRequestOptions }; it would be more robust to haveGraphQLClient.executeformally typed to acceptGraphQLExecuteOptionsor expose a reusable type so these casts aren’t needed. - Helpers like
hasGraphqlOperation,selectGraphqlPayload, andgraphqlErrorsare duplicated across several member tasks; consider extracting them into a shared utility to reduce repetition and keep future changes to GraphQL response handling in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The global `eventHandlersRegistered` flag in `createMockApplicationServicesFactory` may cause cross-test interference or issues under parallel execution; consider scoping event handler registration per test server or using a factory-level guard instead.
- You’re repeatedly re-declaring `ExecuteWithRequestOptions` and casting `GraphQLClient` to `GraphQLClient & { execute: ExecuteWithRequestOptions }`; it would be more robust to have `GraphQLClient.execute` formally typed to accept `GraphQLExecuteOptions` or expose a reusable type so these casts aren’t needed.
- Helpers like `hasGraphqlOperation`, `selectGraphqlPayload`, and `graphqlErrors` are duplicated across several member tasks; consider extracting them into a shared utility to reduce repetition and keep future changes to GraphQL response handling in one place.
## Individual Comments
### Comment 1
<location path="packages/ocom-verification/acceptance-api/src/shared/abilities/create-member.ts" line_range="55" />
<code_context>
+ throw new Error('principalMemberId is required to create a member');
+ }
+
+ const response = await graphql.execute(
+ MEMBER_CREATE_MUTATION,
+ {
</code_context>
<issue_to_address>
**issue (bug_risk):** The optional `role` field in `CreateMemberDetails` is never sent to the GraphQL API.
`CreateMemberDetails.role` is passed through `CreateMemberTaskDetails` but never included in the mutation input, so it is effectively ignored. If `role` should affect backend behavior, add it to the GraphQL input and schema handling; if not, remove it from `CreateMemberDetails` and the task to avoid confusing API consumers.
</issue_to_address>
### Comment 2
<location path="packages/ocom-verification/acceptance-api/src/mock-application-services.ts" line_range="23" />
<code_context>
type EndUserUpdateQueueMessage = Awaited<ReturnType<QueueStorageOperations['receiveFromEndUserUpdateQueue']>>;
const communityCreationMessages: RecordedCommunityCreationMessage[] = [];
+let eventHandlersRegistered = false;
function createMockTokenValidation(): TokenValidation {
</code_context>
<issue_to_address>
**issue (bug_risk):** Global `eventHandlersRegistered` flag may cause cross-test coupling and concurrency issues.
Because `eventHandlersRegistered` is module-level, only the first `createMockApplicationServicesFactory` call will register handlers; subsequent factories may skip registration even if they use different `serviceMongoose` instances or state. In parallel tests or multi-tenant environments this shared flag can cause inconsistent, hard-to-debug behavior. Consider scoping registration to the factory (e.g., per `serviceMongoose` or domainDataSource), or providing an explicit idempotent registration API on the event handler module rather than a global flag.
</issue_to_address>
### Comment 3
<location path="packages/ocom-verification/acceptance-ui/src/contexts/community/tasks/create-member.ts" line_range="8-10" />
<code_context>
+import { type Actor, Task } from '@serenity-js/core';
+import type { AcceptanceUiMemberCreatePage } from '../../../shared/page-contracts.ts';
+
+/** Let the form's async `onFinish` callback settle before assertions run. */
+async function flushPendingReactWork(): Promise<void> {
+ await new Promise<void>((resolve) => setTimeout(resolve, 0));
+ await new Promise<void>((resolve) => setTimeout(resolve, 0));
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Relying on two `setTimeout(0)` calls to flush React work can be brittle.
This helper waits for `onSave` to complete using two zero-delay timers, which is a timing-based approach and can become flaky if the component does more async work or the test runner’s scheduling changes. If there’s a more explicit signal to await (e.g. `waitFor` on a specific state/DOM change or a promise returned from `onSave`), using that instead would make these acceptance tests more reliable.
Suggested implementation:
```typescript
import { RenderInDom } from '@cellix/serenity-framework/dom/render-in-dom';
import { DomPageAdapter } from '@cellix/serenity-framework/pages/dom';
import { TaskStep } from '@cellix/serenity-framework/serenity';
import { MemberCreatePage } from '@ocom-verification/verification-shared/pages';
import { type Actor, Task } from '@serenity-js/core';
import type { AcceptanceUiMemberCreatePage } from '../../../shared/page-contracts.ts';
/**
* Wait for the member creation to complete using an explicit UI signal
* instead of timing-based `setTimeout(0)` calls.
*/
async function waitForMemberCreationToComplete(
page: AcceptanceUiMemberCreatePage,
memberName: string,
): Promise<void> {
await page.waitForMemberToBeCreated(memberName);
}
export const CreateMember = (memberName: string): Task =>
```
To fully implement this change and remove the brittle timing-based approach:
1. Replace any calls to `flushPendingReactWork()` in this file with:
```ts
await waitForMemberCreationToComplete(page, memberName);
```
right after `await page.clickCreateMember();` and before any assertions that depend on the member having been created.
2. In the `AcceptanceUiMemberCreatePage` contract and its concrete `MemberCreatePage` implementation, add a method such as:
```ts
waitForMemberToBeCreated(memberName: string): Promise<void>;
```
that explicitly waits for a reliable UI signal (e.g. new row appearing in a list, success toast, navigation completion, or a specific DOM state) indicating that `onFinish`/`onSave` has completed.
3. If the existing page objects already expose a more appropriate method (e.g. `waitUntilSaved`, `waitForCreateMemberSuccess`, etc.), adjust `waitForMemberCreationToComplete` to call that method instead of `waitForMemberToBeCreated`, and update the helper’s name/comment to reflect the actual signal being used.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| throw new Error('principalMemberId is required to create a member'); | ||
| } | ||
|
|
||
| const response = await graphql.execute( |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The optional role field in CreateMemberDetails is never sent to the GraphQL API.
CreateMemberDetails.role is passed through CreateMemberTaskDetails but never included in the mutation input, so it is effectively ignored. If role should affect backend behavior, add it to the GraphQL input and schema handling; if not, remove it from CreateMemberDetails and the task to avoid confusing API consumers.
| type EndUserUpdateQueueMessage = Awaited<ReturnType<QueueStorageOperations['receiveFromEndUserUpdateQueue']>>; | ||
|
|
||
| const communityCreationMessages: RecordedCommunityCreationMessage[] = []; | ||
| let eventHandlersRegistered = false; |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Global eventHandlersRegistered flag may cause cross-test coupling and concurrency issues.
Because eventHandlersRegistered is module-level, only the first createMockApplicationServicesFactory call will register handlers; subsequent factories may skip registration even if they use different serviceMongoose instances or state. In parallel tests or multi-tenant environments this shared flag can cause inconsistent, hard-to-debug behavior. Consider scoping registration to the factory (e.g., per serviceMongoose or domainDataSource), or providing an explicit idempotent registration API on the event handler module rather than a global flag.
Comment on lines
+8
to
+10
| /** Let the form's async `onFinish` callback settle before assertions run. */ | ||
| async function flushPendingReactWork(): Promise<void> { | ||
| await new Promise<void>((resolve) => setTimeout(resolve, 0)); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Relying on two setTimeout(0) calls to flush React work can be brittle.
This helper waits for onSave to complete using two zero-delay timers, which is a timing-based approach and can become flaky if the component does more async work or the test runner’s scheduling changes. If there’s a more explicit signal to await (e.g. waitFor on a specific state/DOM change or a promise returned from onSave), using that instead would make these acceptance tests more reliable.
Suggested implementation:
import { RenderInDom } from '@cellix/serenity-framework/dom/render-in-dom';
import { DomPageAdapter } from '@cellix/serenity-framework/pages/dom';
import { TaskStep } from '@cellix/serenity-framework/serenity';
import { MemberCreatePage } from '@ocom-verification/verification-shared/pages';
import { type Actor, Task } from '@serenity-js/core';
import type { AcceptanceUiMemberCreatePage } from '../../../shared/page-contracts.ts';
/**
* Wait for the member creation to complete using an explicit UI signal
* instead of timing-based `setTimeout(0)` calls.
*/
async function waitForMemberCreationToComplete(
page: AcceptanceUiMemberCreatePage,
memberName: string,
): Promise<void> {
await page.waitForMemberToBeCreated(memberName);
}
export const CreateMember = (memberName: string): Task =>To fully implement this change and remove the brittle timing-based approach:
-
Replace any calls to
flushPendingReactWork()in this file with:await waitForMemberCreationToComplete(page, memberName);
right after
await page.clickCreateMember();and before any assertions that depend on the member having been created. -
In the
AcceptanceUiMemberCreatePagecontract and its concreteMemberCreatePageimplementation, add a method such as:waitForMemberToBeCreated(memberName: string): Promise<void>;
that explicitly waits for a reliable UI signal (e.g. new row appearing in a list, success toast, navigation completion, or a specific DOM state) indicating that
onFinish/onSavehas completed. -
If the existing page objects already expose a more appropriate method (e.g.
waitUntilSaved,waitForCreateMemberSuccess, etc.), adjustwaitForMemberCreationToCompleteto call that method instead ofwaitForMemberToBeCreated, and update the helper’s name/comment to reflect the actual signal being used.
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
2 participants
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 verification coverage for community member management across API, UI and E2E flows, including member creation and profile update scenarios.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: