WEB-1019 Add Playwright E2E spec for create-client CRUD workflow#3699
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
Note
|
| Layer / File(s) | Summary |
|---|---|
Playwright config slowMo option playwright.config.ts |
Adds slowMo to chromium launchOptions, set to 0 in CI or E2E_SLOW_MO/500 otherwise. |
Client test data factory playwright/factories/client.ts, playwright/factories/index.ts |
Adds TestClientPayload type and createTestClient factory generating deterministic payloads with unique firstname suffixes, plus a barrel re-export. |
Fineract API deletion methods playwright/fixtures/fineract-api.ts |
Adds deleteClient, getClientFamilyMembers, and deleteClientFamilyMember methods to FineractApiClient for test cleanup. |
Create-client CRUD spec playwright/tests/clients/create-client.spec.ts |
Adds a new spec with teardown hooks, a happy-path stepper test asserting created client and family member data, and a negative-path test validating required-field enforcement. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Sequence Diagram(s)
sequenceDiagram
participant Spec as create-client.spec.ts
participant Factory as createTestClient
participant UI as Create Client Stepper
participant API as FineractApiClient
Spec->>Factory: createTestClient(overrides)
Spec->>UI: fill General/Family/Address steps
UI->>API: submit client payload
API-->>UI: created client + id
Spec->>API: getClientFamilyMembers(clientId)
API-->>Spec: family member data
Spec->>API: deleteClientFamilyMember(clientId, memberId)
Spec->>API: deleteClient(clientId)
Suggested reviewers: alberto-art3ch
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately summarizes the main change: adding a Playwright E2E spec for the create-client CRUD workflow. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
5b85e40 to
6b85088
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
playwright/factories/client.ts (1)
64-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUniqueness guarantee can collide across parallel workers.
MODULE_LOAD_EPOCH+testClientSequenceare process-local; if two Playwright workers load this module within the same millisecond, their firstcreateTestClient()calls produce identicalfirstnamesuffixes, contradicting the "uniquely-named payload" guarantee documented above. No current test submits this payload to the API, but future specs relying on this factory for API creation could hit name collisions under parallel execution.♻️ Suggested fix: mix in worker index or randomness
-let testClientSequence = 0; -const MODULE_LOAD_EPOCH = Date.now(); +let testClientSequence = 0; +const MODULE_LOAD_EPOCH = Date.now(); +const WORKER_SALT = Math.random().toString(36).slice(2, 8); ... - const uniqueSuffix = `${MODULE_LOAD_EPOCH}${testClientSequence}`; + const uniqueSuffix = `${MODULE_LOAD_EPOCH}${WORKER_SALT}${testClientSequence}`;Also applies to: 93-102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/factories/client.ts` around lines 64 - 73, The `createTestClient` naming scheme in `playwright/factories/client.ts` can still collide across parallel Playwright workers because `MODULE_LOAD_EPOCH` and `testClientSequence` are process-local. Update the firstname/payload generation in `createTestClient` to mix in a worker-unique value such as the Playwright worker index or a random component, while keeping the existing `Test...` prefix and uniqueness guarantees for all callers.playwright.config.ts (1)
172-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard against non-numeric
E2E_SLOW_MOvalues.
Number(process.env.E2E_SLOW_MO ?? 500)yieldsNaNif the env var is set to a non-numeric string, silently passing an invalidslowMovalue to Playwright's launch options.🛠️ Optional fallback
- slowMo: process.env.CI ? 0 : Number(process.env.E2E_SLOW_MO ?? 500) + slowMo: process.env.CI ? 0 : (Number(process.env.E2E_SLOW_MO) || 500)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright.config.ts` around lines 172 - 175, Guard the Playwright launchOptions slowMo handling against invalid E2E_SLOW_MO values, since Number(process.env.E2E_SLOW_MO ?? 500) can produce NaN. Update the slowMo logic in playwright.config.ts so the value is parsed and validated before being passed to launchOptions, and fall back to the default delay when E2E_SLOW_MO is missing or non-numeric. Use the launchOptions block and the slowMo setting as the main place to fix this.playwright/tests/clients/create-client.spec.ts (1)
152-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConditional contradicts its own comment.
Comment states the Family Members step is "always present in the wizard," yet the code still gates on
.count() > 0. If it's truly always present, the guard is dead code masking a template regression; if it isn't always present, the comment should say so (like the Address-step comment does).As per path instructions for
**/*.spec.ts: "encourage clear Arrange-Act-Assert structure ... and minimal brittle timing dependencies" — an unconditional step assertion would make the happy-path flow more deterministic and easier to follow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/tests/clients/create-client.spec.ts` around lines 152 - 157, The Family Members step handling in create-client.spec.ts contradicts its own comment and hides template regressions. Update the happy-path flow around createClientPage.stepHeader('Family Members'), goToStep('Family Members'), and fillFamilyStep(...) to assert the step is present unconditionally if it is truly always part of the wizard; otherwise, revise the comment to match the optional guard. Keep the spec in a clear Arrange-Act-Assert shape and avoid the conditional count() check unless the step is genuinely optional.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@playwright/tests/clients/create-client.spec.ts`:
- Around line 135-140: The manually aligned property colons in the
create-client.spec.ts fixture are causing the Prettier check to fail. Update the
FamilyMemberData object in the create-client test to use standard Prettier
formatting with normal spacing/indentation, and let Prettier reflow the object
consistently without manual colon alignment.
---
Nitpick comments:
In `@playwright.config.ts`:
- Around line 172-175: Guard the Playwright launchOptions slowMo handling
against invalid E2E_SLOW_MO values, since Number(process.env.E2E_SLOW_MO ?? 500)
can produce NaN. Update the slowMo logic in playwright.config.ts so the value is
parsed and validated before being passed to launchOptions, and fall back to the
default delay when E2E_SLOW_MO is missing or non-numeric. Use the launchOptions
block and the slowMo setting as the main place to fix this.
In `@playwright/factories/client.ts`:
- Around line 64-73: The `createTestClient` naming scheme in
`playwright/factories/client.ts` can still collide across parallel Playwright
workers because `MODULE_LOAD_EPOCH` and `testClientSequence` are process-local.
Update the firstname/payload generation in `createTestClient` to mix in a
worker-unique value such as the Playwright worker index or a random component,
while keeping the existing `Test...` prefix and uniqueness guarantees for all
callers.
In `@playwright/tests/clients/create-client.spec.ts`:
- Around line 152-157: The Family Members step handling in create-client.spec.ts
contradicts its own comment and hides template regressions. Update the
happy-path flow around createClientPage.stepHeader('Family Members'),
goToStep('Family Members'), and fillFamilyStep(...) to assert the step is
present unconditionally if it is truly always part of the wizard; otherwise,
revise the comment to match the optional guard. Keep the spec in a clear
Arrange-Act-Assert shape and avoid the conditional count() check unless the step
is genuinely optional.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 847036c5-6387-4348-8876-b843186b5007
📒 Files selected for processing (5)
playwright.config.tsplaywright/factories/client.tsplaywright/factories/index.tsplaywright/fixtures/fineract-api.tsplaywright/tests/clients/create-client.spec.ts
484ccb2 to
e3ffae9
Compare
- Happy path: drives full mat-stepper (General → Family Members → Preview → Submit), asserts redirect to /#/clients/:id/general, verifies API echoes displayName, officeId, status=Pending, and persists the family member via GET /clients/:id/familymembers - Negative path: empty lastname blocks Preview step (areFormvalids()), mat-error is visible, recovery restores the Preview step header - playwright/factories/client.ts: createTestClient() payload factory with unique-suffix generation and TestClientPayload type - playwright/factories/index.ts: barrel export - playwright/fixtures/fineract-api.ts: adds deleteClient(), getClientFamilyMembers(), deleteClientFamilyMember() — cascade- delete guard in afterEach prevents 403 FK violations on cleanup - playwright.config.ts: slowMo (default 500ms, CI=0, override via E2E_SLOW_MO env var) added to chromium launchOptions 3 tests passing (auth + 2 create-client specs) in ~27s
e3ffae9 to
4b530e2
Compare
2 participants
Description
Adds the first Playwright E2E spec for the Create Client workflow as part of the broader WA-3.2 test coverage initiative. The spec validates the complete client creation journey through the Angular mat-stepper UI — from the clients list CTA through General, Family Members, and Preview steps to final submission — and then cross-checks the result directly against the Fineract API to confirm the server stored exactly what the form sent.
A negative-path test covers the required-field validation gate: the Preview step is hidden (
areFormvalids()) when a required field is empty, and a mat-error message is shown. Recovery by filling the field restores the Previewstep, proving the guard is driven by the specific validator and not an unrelated form issue.
Three supporting pieces of infrastructure are added that every future spec in
this suite will reuse:
createTestClient()payload factory for deterministic, unique test datadeleteClient(),getClientFamilyMembers(),deleteClientFamilyMember()fixture methods for reliable teardown (Fineract enforces a FK constraint
that blocks client deletion when family member records exist — the cascade
guard handles this)
slowMoin chromium launchOptions so the UI flow is visible during localreview without changing test logic
Related issues and discussion
https://mifosforge.jira.com/browse/WEB-1019
Screenshots, if any
UI MODE-
npx playwright test --ui playwright/tests/clients/create-client.spec.tsclient-crud-playwright.mp4
Checklist
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
New Features
Bug Fixes