feat: expand @ocom-verification coverage for community management#299
Open
henry-casper wants to merge 4 commits into
Open
feat: expand @ocom-verification coverage for community management#299henry-casper wants to merge 4 commits into
henry-casper wants to merge 4 commits into
Conversation
Add shared community settings scenarios (create/read/update + permission checks) in verification-shared and implement them across acceptance-api, acceptance-ui, and e2e-tests following the Screenplay notes/tasks/questions and e2e interactions patterns. Port PR #297 dependency overrides and add websocket-driver/ws overrides so audit and snyk scans pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Reviewer's GuideExpands community management verification to cover community settings read/update flows with role- and permission-aware scenarios across API, UI, and E2E suites, refactors existing create-community flows into Screenplay-aligned tasks/interactions, seeds MongoDB with community/membership/staff data for auth-sensitive tests, wires actor-specific auth tokens and principal hints into the GraphQL client/mock services, and updates pnpm workspace overrides to satisfy audit/Snyk/CI constraints. Sequence diagram for API community settings update with actor-aware authsequenceDiagram
actor TestActor
participant UpdateCommunitySettingsTask as UpdateCommunitySettings
participant UpdateCommunityAbility as UpdateCommunity
participant GraphQLClient as GraphQLClient
participant ActorAuth as actor_auth
participant ApiFactory as createMockApplicationServicesFactory
participant TokenValidation
participant QueryCommunity
TestActor->>UpdateCommunitySettingsTask: performAs(actor)
UpdateCommunitySettingsTask->>UpdateCommunityAbility: UpdateCommunity.performAs(actor, details)
UpdateCommunityAbility->>GraphQLClient: GraphQLClient.as(actor)
UpdateCommunityAbility->>GraphQLClient: execute(COMMUNITY_UPDATE_SETTINGS_MUTATION, input)
GraphQLClient->>ActorAuth: getActorToken(actor.name)
GraphQLClient->>ActorAuth: getActorHints(actor.name)
GraphQLClient->>ApiFactory: forRequest(rawAuthHeader, hints)
ApiFactory->>TokenValidation: verifyJwt(token)
TokenValidation-->>ApiFactory: TokenValidationResult (roles, principal)
ApiFactory-->>GraphQLClient: communityUpdateSettings response
UpdateCommunityAbility-->>UpdateCommunitySettingsTask: UpdateCommunityResult
UpdateCommunitySettingsTask->>QueryCommunity: QueryCommunity.as(actor).byId(actor, communityId)
QueryCommunity-->>UpdateCommunitySettingsTask: CommunityReadResult
UpdateCommunitySettingsTask->>TestActor: notes.set(lastCommunityStatus, "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}
![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:
Fixed security issues:
-
websocket-driver (link)
-
The actor auth plumbing (prefix constants, token construction, hints) is split between
shared/abilities/actor-auth.tsandmock-application-services.ts; consider centralizing this logic so the mock token verification and GraphQL client headers stay aligned in one place. -
The new
CommunitySettingsPagerelies on hard-coded CSS selectors and input IDs (e.g.#name,.ant-form-item-explain-error), which will make the settings tests brittle against UI refactors; you may want to introduce dedicated test attributes or a small selector helper to decouple tests from Ant Design internals.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The actor auth plumbing (prefix constants, token construction, hints) is split between `shared/abilities/actor-auth.ts` and `mock-application-services.ts`; consider centralizing this logic so the mock token verification and GraphQL client headers stay aligned in one place.
- The new `CommunitySettingsPage` relies on hard-coded CSS selectors and input IDs (e.g. `#name`, `.ant-form-item-explain-error`), which will make the settings tests brittle against UI refactors; you may want to introduce dedicated test attributes or a small selector helper to decouple tests from Ant Design internals.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Resolves CodeQL js/polynomial-redos warning by matching the post-create redirect URL with an exact pathname check instead of a regular expression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Fixed security issues:
- websocket-driver (link)
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
Fixed security issues:
-
websocket-driver (link)
-
The
CommunitySettingsFormDetails/CommunitySettingsDetailstypes are defined separately in multiple packages (acceptance-api, acceptance-ui, e2e); consider extracting a shared type to avoid divergence and keep the settings field contract consistent across suites. -
The seeded test data uses many hard-coded string IDs across communities, roles, members, and staff; centralizing these in a single module or builder helpers would reduce the risk of mismatched references and make future changes to the seed graph easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `CommunitySettingsFormDetails`/`CommunitySettingsDetails` types are defined separately in multiple packages (acceptance-api, acceptance-ui, e2e); consider extracting a shared type to avoid divergence and keep the settings field contract consistent across suites.
- The seeded test data uses many hard-coded string IDs across communities, roles, members, and staff; centralizing these in a single module or builder helpers would reduce the risk of mismatched references and make future changes to the seed graph easier.
## Individual Comments
### Comment 1
<location path="packages/ocom-verification/acceptance-api/src/contexts/community/questions/community-field.ts" line_range="21-26" />
<code_context>
+ }
+
+ override async answeredBy(actor: AnswersQuestions & UsesAbilities): Promise<string> {
+ const communityId = await this.readNote(actor, 'lastCommunityId');
+ if (!communityId) {
+ throw new Error(`No community id found in actor notes; cannot read the community ${this.fieldName}. Did the actor act on a community first?`);
+ }
+
+ const community = await QueryCommunity.as(actor).byId(actor, communityId);
+ if (!community) {
+ throw new Error(`Community "${communityId}" was not found through the API`);
</code_context>
<issue_to_address>
**suggestion:** The note-reading helper uses a generic `Record<typeof key, string>` that obscures the intended `CommunityNotes` shape.
In `readNote` the use of `notes<Record<typeof key, string>>()` makes the note type opaque and doesn’t add meaningful type safety. It also hides the actual `CommunityNotes` shape from consumers.
Prefer:
```ts
return await actor.answer(notes<CommunityNotes>().get(key));
```
so the helper stays aligned with the `CommunityNotes` interface and the available keys and their semantics remain discoverable and easier to evolve.
Suggested implementation:
```typescript
private async readNote(actor: AnswersQuestions & UsesAbilities, key: keyof CommunityNotes): Promise<string | undefined> {
try {
return await actor.answer(notes<CommunityNotes>().get(key));
```
If `CommunityNotes` is not yet imported in this file, ensure you add or fix the import so the interface is available:
- Add an import such as `import type { CommunityNotes } from '...';` from the module where `CommunityNotes` is defined.
</issue_to_address>
### Comment 2
<location path="packages/ocom-verification/e2e-tests/src/contexts/community/step-definitions/update-community-settings.steps.ts" line_range="118-126" />
<code_context>
+ }
+});
+
+Then('the community should not be modified', async () => {
+ // Verify through an independently authenticated actor so the assertion works
+ // even when the scenario actor is unauthenticated or under-privileged.
</code_context>
<issue_to_address>
**suggestion (testing):** E2E "community should not be modified" only checks name; consider asserting other settings fields too
This step only revalidates the `name` against `SEEDED_COMMUNITY`. Because the update flows can also change `whiteLabelDomain`, `domain`, and `handle`, a bug that wrongly persists those fields would go unnoticed.
After the reload, consider also reading `SavedWhiteLabelDomain`, `SavedDomain`, and `SavedHandle` and asserting they match the seeded baseline (likely empty/null). That way, the "not modified" check covers all settings, not just the name.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Union-resolve overlapping acceptance-api harness (community + staff-role abilities, principal hints, per-scenario re-seed), shared test data, and acceptance-ui tsconfig includes. Keep the stricter dependency overrides (ws 8.21.1, split body-parser, websocket-driver) and extended .snyk waivers; adopt main's refined page-adapter implementations. Bump fast-uri override to ^4.1.1 for SNYK-JS-FASTURI-18021349. 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.
Add shared community settings scenarios (create/read/update + permission checks) in verification-shared and implement them across acceptance-api, acceptance-ui, and e2e-tests following the Screenplay notes/tasks/questions and e2e interactions patterns. Port PR #297 dependency overrides and add websocket-driver/ws overrides so audit and snyk scans pass.
Summary by Sourcery
Expand community management verification scenarios to cover viewing and updating community settings with permission checks across API, UI, and E2E suites, backed by new seeded community/membership data and shared page/GraphQL abstractions.
New Features:
Enhancements:
Build:
Tests: