WEB-1037: Playwright — client action page objects, seeded factory with state overrides, and ApiSetupManager.getClientTemplate#3722
Conversation
|
Warning Review limit reached
Next review available in: 22 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 (14)
Note
|
| Layer / File(s) | Summary |
|---|---|
Client workflow contracts and API commands playwright/factories/client.ts, playwright/fixtures/fineract-api.ts |
Client states, seeded-client return types, reason helpers, and reject/withdraw commands are defined. |
Seeded client orchestration playwright/factories/client.ts, playwright/factories/index.ts |
Factories build payloads, create clients, apply lifecycle transitions, expose cleanup, and re-export the new API. |
Factory workflow validation playwright/factories/client.spec.ts |
Unit tests cover payload overrides, state mapping, response identifiers, cleanup ordering, failures, and returned values. |
Client action page objects
| Layer / File(s) | Summary |
|---|---|
Client action selector contracts playwright/config/selectors.ts |
Typed selector maps define controls for activate, reject, withdraw, reactivate, and transfer forms. |
Client action page workflows playwright/pages/client-actions/*, playwright/pages/index.ts |
Page objects implement loading, form submission, option selection, cancellation navigation, and barrel exports for each action. |
Playwright infrastructure
| Layer / File(s) | Summary |
|---|---|
Client template caching playwright/fixtures/fineract-api.ts, playwright/utils/api-setup-manager.ts, playwright/utils/api-setup-manager.spec.ts |
Client templates support global or office-scoped retrieval, deduplication, keyed caching, and retry behavior after failures. |
Test project discovery rules playwright.config.ts |
The unit project includes the client factory spec, while integration discovery targets *.factory.spec.ts files. |
Estimated code review effort: 4 (Complex) | ~60 minutes
Sequence Diagram(s)
sequenceDiagram
participant Test as Factory test
participant Factory as createSeededClient
participant API as FineractApiClient
participant Cleanup as cleanup
Test->>Factory: request client state
Factory->>API: create client
Factory->>API: apply lifecycle command
Factory-->>Test: return SeededTestClient
Test->>Cleanup: invoke cleanup
Cleanup->>API: delete family members
Cleanup->>API: delete client
Possibly related PRs
- openMF/web-app#3684: Adds the caching primitive used by
ApiSetupManager.getClientTemplate(). - openMF/web-app#3686: Related Playwright factory setup and factory-spec discovery changes.
- openMF/web-app#3699: Related client test-factory changes around
createTestClient.
Suggested reviewers: alberto-art3ch
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the main changes: new client action page objects, seeded client factory state overrides, and the new ApiSetupManager client template wrapper. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
| 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. |
✨ 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.
b920077 to
eec1702
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: 4
🧹 Nitpick comments (2)
playwright/factories/client.spec.ts (2)
92-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
satisfiesto validate the stub before casting.
as unknown as FineractApiClientbypasses the compatibility checking claimed by the comment, allowing API signature drift to compile unnoticed. Validate the object against aPick<FineractApiClient, ...>first, retaining the cast only at the final handoff.🤖 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.spec.ts` around lines 92 - 190, Update buildStub so the stub object is first validated with satisfies against a Pick<FineractApiClient, ...> containing the implemented methods, ensuring signature drift is caught; retain the final as unknown as FineractApiClient cast only when assigning the validated stub to the returned api field.
364-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that cleanup still attempts
deleteClient.This test only checks that cleanup resolves, so it also passes if cleanup stops after the family-member lookup failure. Record the deleted ID and assert that
deleteClient(99)was reached.Proposed assertion
+ let deletedClientId: number | undefined; const failingStub = { getFirstOfficeId: async (): Promise<number> => 7, createPendingClient: async (): Promise<{ resourceId: number }> => ({ resourceId: 99 }), getClientFamilyMembers: async (): Promise<never> => { throw new Error('boom'); }, - deleteClient: async (): Promise<void> => undefined + deleteClient: async (clientId: number): Promise<void> => { + deletedClientId = clientId; + } } as unknown as FineractApiClient; const seeded = await createSeededClient(failingStub); await expect(seeded.cleanup()).resolves.toBeUndefined(); + expect(deletedClientId).toBe(99);🤖 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.spec.ts` around lines 364 - 379, Update the cleanup rejection test around createSeededClient to track calls to the failingStub.deleteClient method, recording the client ID received. After awaiting seeded.cleanup(), assert that deleteClient was called with ID 99, while retaining the existing resolution assertion.
🤖 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/factories/client.ts`:
- Around line 51-57: Update CreateTestClientOverrides and createSeededClient so
callers cannot provide GeneralStepData fields that the seeded API path ignores;
either narrow the override type to the fields actually mapped into the request
or resolve and include every supported override, including office selection and
persisted client fields. Ensure the returned payload reflects the values saved
by createSeededClient.
- Around line 137-184: Update createSeededClient around the lifecycle transition
switch and cleanup function so cleanup is registered immediately after client
creation, before reason lookups or state commands can fail, and invoke it when
any transition rejects. Ensure non-pending clients also receive state-aware or
suite-level teardown when hard deletion is unavailable, while preserving cleanup
of family members and pending clients.
In `@playwright/fixtures/fineract-api.ts`:
- Around line 410-453: Remove the duplicate declarations of
ensureClientRejectionReason and ensureClientWithdrawalReason in this section,
and remove the second declarations of rejectClient and withdrawClient later in
the class. Preserve one complete implementation of each lifecycle method and
leave their behavior unchanged.
In `@playwright/utils/api-setup-manager.ts`:
- Around line 189-204: Remove the duplicated getClientTemplate method and its
associated JSDoc block, preserving the single existing definition and its
deduplication behavior.
---
Nitpick comments:
In `@playwright/factories/client.spec.ts`:
- Around line 92-190: Update buildStub so the stub object is first validated
with satisfies against a Pick<FineractApiClient, ...> containing the implemented
methods, ensuring signature drift is caught; retain the final as unknown as
FineractApiClient cast only when assigning the validated stub to the returned
api field.
- Around line 364-379: Update the cleanup rejection test around
createSeededClient to track calls to the failingStub.deleteClient method,
recording the client ID received. After awaiting seeded.cleanup(), assert that
deleteClient was called with ID 99, while retaining the existing resolution
assertion.
🪄 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: 14154b4c-6c83-4335-9a92-a103cc6a415b
📒 Files selected for processing (14)
playwright.config.tsplaywright/config/selectors.tsplaywright/factories/client.spec.tsplaywright/factories/client.tsplaywright/factories/index.tsplaywright/fixtures/fineract-api.tsplaywright/pages/client-actions/activate-client.page.tsplaywright/pages/client-actions/reactivate-client.page.tsplaywright/pages/client-actions/reject-client.page.tsplaywright/pages/client-actions/transfer-client.page.tsplaywright/pages/client-actions/withdraw-client.page.tsplaywright/pages/index.tsplaywright/utils/api-setup-manager.spec.tsplaywright/utils/api-setup-manager.ts
eec1702 to
c792443
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/fixtures/fineract-api.ts`:
- Around line 129-135: Update the officeId conditional in getClientTemplate to
check specifically for undefined, ensuring officeId=0 includes the query
parameter while omitted officeId retains the unscoped URL.
🪄 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: 8b7a8d85-3f11-43c6-941c-8a9ab1144533
📒 Files selected for processing (14)
playwright.config.tsplaywright/config/selectors.tsplaywright/factories/client.spec.tsplaywright/factories/client.tsplaywright/factories/index.tsplaywright/fixtures/fineract-api.tsplaywright/pages/client-actions/activate-client.page.tsplaywright/pages/client-actions/reactivate-client.page.tsplaywright/pages/client-actions/reject-client.page.tsplaywright/pages/client-actions/transfer-client.page.tsplaywright/pages/client-actions/withdraw-client.page.tsplaywright/pages/index.tsplaywright/utils/api-setup-manager.spec.tsplaywright/utils/api-setup-manager.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- playwright/factories/index.ts
- playwright/utils/api-setup-manager.ts
- playwright/pages/index.ts
- playwright/factories/client.spec.ts
- playwright/config/selectors.ts
- playwright/factories/client.ts
| async getClientTemplate(officeId?: number): Promise<any> { | ||
| const url = officeId | ||
| ? `/fineract-provider/api/v1/clients/template?officeId=${officeId}` | ||
| : '/fineract-provider/api/v1/clients/template'; | ||
| const res = await this.ctx.get(url); | ||
| return this.validateResponse(res, 'getClientTemplate'); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a strict undefined check for officeId.
A falsy check (officeId ? ... : ...) drops the query parameter if officeId is 0. While Fineract IDs are typically positive integers, using a strict !== undefined check is more robust and correctly aligns with the test suite, which explicitly expects officeId=0 to be treated as a distinct, scoped request (as seen in the sentinel isolation test in api-setup-manager.spec.ts).
🛡️ Proposed fix
async getClientTemplate(officeId?: number): Promise<any> {
- const url = officeId
+ const url = officeId !== undefined
? `/fineract-provider/api/v1/clients/template?officeId=${officeId}`
: '/fineract-provider/api/v1/clients/template';
const res = await this.ctx.get(url);
return this.validateResponse(res, 'getClientTemplate');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getClientTemplate(officeId?: number): Promise<any> { | |
| const url = officeId | |
| ? `/fineract-provider/api/v1/clients/template?officeId=${officeId}` | |
| : '/fineract-provider/api/v1/clients/template'; | |
| const res = await this.ctx.get(url); | |
| return this.validateResponse(res, 'getClientTemplate'); | |
| } | |
| async getClientTemplate(officeId?: number): Promise<any> { | |
| const url = officeId !== undefined | |
| ? `/fineract-provider/api/v1/clients/template?officeId=${officeId}` | |
| : '/fineract-provider/api/v1/clients/template'; | |
| const res = await this.ctx.get(url); | |
| return this.validateResponse(res, 'getClientTemplate'); | |
| } |
🤖 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/fixtures/fineract-api.ts` around lines 129 - 135, Update the
officeId conditional in getClientTemplate to check specifically for undefined,
ensuring officeId=0 includes the query parameter while omitted officeId retains
the unscoped URL.
…verrides, and ApiSetupManager.getClientTemplate - Add 5 Layer-2 selector maps (activate/reject/withdraw/reactivate/transfer) - Add 5 client-action Page Objects under playwright/pages/client-actions/ - Extend client factory with ClientState union, CreateTestClientOverrides, and createSeededClient covering pending/active/rejected/withdrawn/closed with FK-safe cleanup deleters - Extend FineractApiClient with getClientTemplate, rejectClient, withdrawClient, ensureClientRejectionReason, ensureClientWithdrawalReason - Add ApiSetupManager.getClientTemplate domain wrapper with 'none' sentinel - Add 15 factory unit tests + 5 getClientTemplate wrapper tests (95 unit tests total, all passing) - Route factories/client.spec.ts through the 'unit' project; keep .factory.spec.ts under 'integration'
8cfdd0a to
89c3c4a
Compare
1 participant
Description
Extends the Playwright E2E infrastructure to unblock client-lifecycle specs (activate / reject / withdraw / reactivate / transfer / close) with reusable Page Objects, a seeded factory that owns state-specific setup + cleanup, and a deduplicated client-template API wrapper.
1. Layer-2 selector contracts —
playwright/config/selectors.tsAdded five typed selector maps mirroring
CloseClientSelectors:ACTIVATE_CLIENT_SELECTORS,REJECT_CLIENT_SELECTORS,WITHDRAW_CLIENT_SELECTORS,REACTIVATE_CLIENT_SELECTORS,TRANSFER_CLIENT_SELECTORSgetByRole('button', { name })(same code path for Angular Material and the React port).2. Page Objects —
playwright/pages/client-actions/Five thin parallels of
close-client.page.ts:activate-client.page.ts—submitActivation({ activationDate })reject-client.page.ts—submitRejection({ rejectionDate, reasonName })withdraw-client.page.ts—submitWithdrawal({ withdrawalDate, reasonName })reactivate-client.page.ts—submitReactivation({ reactivationDate })transfer-client.page.ts—submitTransfer({ destinationOfficeName, transferDate, note? })(URL regex tolerates bothTransfer%20ClientandTransfer Client)Barrel re-exports added in
playwright/pages/index.ts.3. Factory extension —
playwright/factories/client.tsClientStateunion:'pending' | 'active' | 'rejected' | 'withdrawn' | 'closed'CreateTestClientOverrideswidens the existing overrides withstate,actionDate,rejectionReasonName,withdrawalReasonName,closureReasonNamecreateTestClientstrips seeded-factory-only fields viaextractPayloadOverridessoTestClientPayloadstays cleancreateSeededClient(fineractApi, overrides?)— single owner of "make me a client in state X with a cleanup deleter that works for state X":Cleanup purges family members first (FK-safe), then deletes; non-pending deletion errors are swallowed and logged.
4. FineractApiClient extension —
playwright/fixtures/fineract-api.tsgetClientTemplate(officeId?)rejectClient(id, reasonId, date)/withdrawClient(id, reasonId, date)ensureClientRejectionReason(name?)/ensureClientWithdrawalReason(name?)(backed byClientRejectReason/ClientWithdrawReasonsystem codes).5. ApiSetupManager domain wrapper —
playwright/utils/api-setup-manager.tsgetClientTemplate(officeId?)delegates todedupe('clientTemplate:' + (officeId ?? 'none'), …). The'none'sentinel keeps the no-officeId form on its own key so it never collides withclientTemplate:0.6. Tests
playwright/utils/api-setup-manager.spec.ts— 5 new tests: concurrent dedup, serial cache hits, distinct keys per officeId, sentinel isolation from officeId=0, reject eviction.playwright/factories/client.spec.ts— 15 new unit tests (stubbedFineractApiClient): payload synthesis, state→command mapping for all 5 states, actionDate fallback, missing resourceId error, cleanup FK-safe order + best-effort swallow, family-member fetch resilience, return-shape.playwright.config.ts—unitproject includesfactories/client.spec.ts;integrationnarrowed to.factory.spec.tsonly.Validation
Related issues and discussion
https://mifosforge.jira.com/browse/WEB-1037
Screenshots, if any
No TypeScript errors across touched files or downstream consumer specs.
Checklist
30d56abf8)web-app/.github/CONTRIBUTING.mdSummary by CodeRabbit