WEB-1029: [Playwright] Add create-client form-validation E2E spec#3717
Open
devvaansh wants to merge 2 commits into
Open
WEB-1029: [Playwright] Add create-client form-validation E2E spec#3717devvaansh wants to merge 2 commits into
devvaansh wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Layer / File(s) | Summary |
|---|---|
Proxy handler wiring proxy.conf.js, proxy.localhost.conf.js |
Updates proxy callbacks, upstream logging, HTTPS handling, path rewriting, API key injection, and 502 error responses. |
Local Playwright startup package.json, playwright.config.ts |
Adds start:local and uses it for non-CI Playwright runs. |
Create Client test harness playwright/tests/clients/create-client-form-validation.spec.ts |
Adds mocked resolver responses, credentials initialization, shared test data, and request interception. |
Create Client validation scenarios playwright/tests/clients/create-client-form-validation.spec.ts |
Covers required office, firstname, lastname, activation-date ordering, and dynamic PERSON/ENTITY control switching. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Possibly related PRs
- openMF/web-app#3697: Adds related Playwright Create Client form validation coverage.
- openMF/web-app#3699: Extends related Create Client Playwright coverage and test setup.
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 matches the main change: adding a Playwright create-client form-validation E2E spec. |
| 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. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Warning
There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
🔧 Checkov (3.3.8)
package.json
Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
playwright/tests/clients/create-client-form-validation.spec.ts (2)
374-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an accessible locator instead of a CSS attribute selector.
As per path instructions, we should encourage the use of stable selectors. Playwright recommends using user-facing attributes like roles and labels over implementation-tied CSS selectors like
formcontrolname. Consider using a role-based locator to improve test stability and ensure accessibility behavior is validated.♻️ Proposed refactor
- const constitutionDropdown = page.locator('mat-select[formcontrolname="constitutionId"]'); + const constitutionDropdown = page.getByRole('combobox', { name: 'Constitution', exact: false });🤖 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-form-validation.spec.ts` at line 374, Replace the constitutionDropdown CSS locator with an accessible user-facing locator, preferably the appropriate select role and accessible name, while preserving the existing interactions and ensuring the test validates the control’s accessibility semantics.Source: Path instructions
209-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant explicit waits.
Playwright's action methods (
click,fill) and web-first assertions (expect(locator).toBeVisible()) automatically wait for the target element to become visible, enabled, and stable. Explicitly callingwaitFor({ state: 'visible' })beforehand is redundant, adds boilerplate, and can introduce brittle timing dependencies. As per path instructions, we should minimize brittle timing dependencies by leveraging Playwright's built-in auto-waiting mechanisms.
playwright/tests/clients/create-client-form-validation.spec.ts#L209-L209: Remove thewaitFor()call;click()natively handles waiting.playwright/tests/clients/create-client-form-validation.spec.ts#L250-L250: Remove thewaitFor()call;fill()natively handles waiting.playwright/tests/clients/create-client-form-validation.spec.ts#L289-L289: Remove thewaitFor()call;fill()natively handles waiting.playwright/tests/clients/create-client-form-validation.spec.ts#L306-L306: Remove thewaitFor()call;fill()natively handles waiting.playwright/tests/clients/create-client-form-validation.spec.ts#L365-L365: Remove thewaitFor()call, and optionally pass{ timeout: 10_000 }directly to the subsequenttoBeVisible()assertion if an extended timeout is needed.playwright/tests/clients/create-client-form-validation.spec.ts#L382-L382: Remove thewaitFor()call, and optionally pass{ timeout: 10_000 }directly to the subsequenttoBeVisible()assertion.🤖 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-form-validation.spec.ts` at line 209, Remove the redundant explicit waitFor calls in playwright/tests/clients/create-client-form-validation.spec.ts at lines 209, 250, 289, 306, 365, and 382. Rely on the subsequent click, fill, or toBeVisible actions for Playwright auto-waiting; preserve the existing interactions and, where needed at lines 365 and 382, apply the 10-second timeout directly to toBeVisible.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.
Nitpick comments:
In `@playwright/tests/clients/create-client-form-validation.spec.ts`:
- Line 374: Replace the constitutionDropdown CSS locator with an accessible
user-facing locator, preferably the appropriate select role and accessible name,
while preserving the existing interactions and ensuring the test validates the
control’s accessibility semantics.
- Line 209: Remove the redundant explicit waitFor calls in
playwright/tests/clients/create-client-form-validation.spec.ts at lines 209,
250, 289, 306, 365, and 382. Rely on the subsequent click, fill, or toBeVisible
actions for Playwright auto-waiting; preserve the existing interactions and,
where needed at lines 365 and 382, apply the 10-second timeout directly to
toBeVisible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d86e749b-f258-4266-92bb-e6f6ca232916
📒 Files selected for processing (4)
package.jsonplaywright.config.tsplaywright/tests/clients/create-client-form-validation.spec.tsproxy.localhost.conf.js
Contributor
|
@devvaansh needs update to base branch.. If I do it I cant then approve for merge |
devvaansh
force-pushed
the
WEB-1029-playwright-add-create-client-form-validation-e-2-e-spec
branch
from
July 14, 2026 15:43
30d56ab to
b2d4490
Compare
Member
Author
Updated - rebased onto the latest dev tip |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
playwright/tests/clients/create-client-form-validation.spec.ts (1)
209-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid hardcoded timeouts and rely on Playwright's auto-waiting.
Playwright's action methods (like
fillandclick) automatically wait for the target element to become visible and actionable. CallingwaitForwith a hardcoded timeout introduces a brittle timing dependency, which contradicts the path instructions for*.spec.tsfiles. If a longer timeout is genuinely needed for this action due to slow rendering, consider extending the action or expect timeouts in the Playwright configuration instead.♻️ Proposed refactor (can be applied to similar occurrences)
- await createClientPage.firstnameInput.waitFor({ state: 'visible', timeout: 10_000 });🤖 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-form-validation.spec.ts` at line 209, Remove the hardcoded waitFor call before interacting with firstnameInput in the create-client form validation test, and rely on the subsequent Playwright action’s built-in auto-waiting. Do not add another per-action timeout; use the configured Playwright timeout only if broader adjustment is required.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.
Nitpick comments:
In `@playwright/tests/clients/create-client-form-validation.spec.ts`:
- Line 209: Remove the hardcoded waitFor call before interacting with
firstnameInput in the create-client form validation test, and rely on the
subsequent Playwright action’s built-in auto-waiting. Do not add another
per-action timeout; use the configured Playwright timeout only if broader
adjustment is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e311dd6-1c53-4c27-9ef5-59e2d6a3903d
📒 Files selected for processing (4)
package.jsonplaywright.config.tsplaywright/tests/clients/create-client-form-validation.spec.tsproxy.localhost.conf.js
🚧 Files skipped from review as they are similar to previous changes (1)
- playwright.config.ts
devvaansh
force-pushed
the
WEB-1029-playwright-add-create-client-form-validation-e-2-e-spec
branch
from
July 15, 2026 09:59
8bf299d to
50cdcf4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
playwright/tests/clients/create-client-form-validation.spec.ts (1)
183-183: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope each validation-error assertion to its target field.
validationErrors.first()can match an unrelated error. Locatemat-errorinside the corresponding office, firstname, or lastname wrapper instead.Proposed adjustment
- await expect(createClientPage.validationErrors.first()).toBeVisible(); + await expect(officeWrapper.locator('mat-error')).toBeVisible(); - await expect(createClientPage.validationErrors.first()).toBeVisible(); + await expect(firstnameWrapper.locator('mat-error')).toBeVisible(); - await expect(createClientPage.validationErrors.first()).toBeVisible(); + await expect(lastnameWrapper.locator('mat-error')).toBeVisible();As per path instructions, “For tests: encourage clear Arrange-Act-Assert structure, stable selectors, and minimal brittle timing dependencies.”
Also applies to: 229-229, 265-265
🤖 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-form-validation.spec.ts` at line 183, Update the validation assertions in the create-client form tests to scope each error lookup to its corresponding office, firstname, or lastname field wrapper instead of using validationErrors.first(). Preserve the existing visibility checks while using the field-specific mat-error selector for all three affected cases.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-form-validation.spec.ts`:
- Around line 20-24: Update the documentation comment for the create-client form
validation spec to remove the claim that it reads the seeded “Head Office” from
live Fineract. Describe the actual prerequisite: the live backend is required,
while resolver requests and office data are mocked through MOCK_CLIENT_TEMPLATE.
In `@proxy.localhost.conf.js`:
- Around line 19-24: Update the proxy configuration around the middleware
options to replace the top-level onProxyReq and onError handlers with an on
object containing proxyReq and error callbacks. Preserve the existing request
logging and custom 502 response behavior within those callbacks.
---
Nitpick comments:
In `@playwright/tests/clients/create-client-form-validation.spec.ts`:
- Line 183: Update the validation assertions in the create-client form tests to
scope each error lookup to its corresponding office, firstname, or lastname
field wrapper instead of using validationErrors.first(). Preserve the existing
visibility checks while using the field-specific mat-error selector for all
three affected cases.
🪄 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: 115cb90b-c5ec-46ea-b6d3-fb1aa8a2adbd
📒 Files selected for processing (4)
package.jsonplaywright.config.tsplaywright/tests/clients/create-client-form-validation.spec.tsproxy.localhost.conf.js
🚧 Files skipped from review as they are similar to previous changes (2)
- playwright.config.ts
- package.json
Comment on lines
+20
to
+24
| * The spec runs as a real E2E test against a live Fineract backend | ||
| * (spun up via `npm run e2e:docker:up` or a local Fineract on | ||
| * `https://localhost:8443`). It reuses the shared auth setup and | ||
| * only reads the seeded `Head Office` name to keep every case | ||
| * self-explanatory. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the live-backend requirement documentation.
This says the spec reads a seeded office from live Fineract, but Lines 140-147 mock both resolver requests and MOCK_CLIENT_TEMPLATE supplies the office. Align the comment with the actual test prerequisites.
🤖 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-form-validation.spec.ts` around lines
20 - 24, Update the documentation comment for the create-client form validation
spec to remove the claim that it reads the seeded “Head Office” from live
Fineract. Describe the actual prerequisite: the live backend is required, while
resolver requests and office data are mocked through MOCK_CLIENT_TEMPLATE.
devvaansh
force-pushed
the
WEB-1029-playwright-add-create-client-form-validation-e-2-e-spec
branch
from
July 15, 2026 10:16
50cdcf4 to
c1fc31d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
playwright/tests/clients/create-client-form-validation.spec.ts (1)
183-183: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope validation errors to the control under test.
Using
validationErrors.first()can produce false positives when another control displays an error.
playwright/tests/clients/create-client-form-validation.spec.ts#L183-L183: locate the office form-field wrapper and assert itsmat-error.playwright/tests/clients/create-client-form-validation.spec.ts#L229-L229: assertfirstnameWrapper.locator('mat-error').playwright/tests/clients/create-client-form-validation.spec.ts#L265-L265: assertlastnameWrapper.locator('mat-error').Proposed changes
- await expect(createClientPage.validationErrors.first()).toBeVisible(); + const officeWrapper = page.locator('mat-form-field').filter({ has: createClientPage.officeDropdown }); + await expect(officeWrapper.locator('mat-error')).toBeVisible(); - await expect(createClientPage.validationErrors.first()).toBeVisible(); + await expect(firstnameWrapper.locator('mat-error')).toBeVisible(); - await expect(createClientPage.validationErrors.first()).toBeVisible(); + await expect(lastnameWrapper.locator('mat-error')).toBeVisible();As per path instructions, tests should use stable selectors.
🤖 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-form-validation.spec.ts` at line 183, Scope each validation assertion in create-client-form-validation.spec.ts to the control being tested: at lines 183, 229, and 265, locate the corresponding office, firstnameWrapper, and lastnameWrapper form-field wrappers and assert visibility on their mat-error locator instead of validationErrors.first(). Use stable selectors throughout.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.
Nitpick comments:
In `@playwright/tests/clients/create-client-form-validation.spec.ts`:
- Line 183: Scope each validation assertion in
create-client-form-validation.spec.ts to the control being tested: at lines 183,
229, and 265, locate the corresponding office, firstnameWrapper, and
lastnameWrapper form-field wrappers and assert visibility on their mat-error
locator instead of validationErrors.first(). Use stable selectors throughout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 477e0f71-3ab4-42e7-af58-b8984dbcc2c5
📒 Files selected for processing (5)
package.jsonplaywright.config.tsplaywright/tests/clients/create-client-form-validation.spec.tsproxy.conf.jsproxy.localhost.conf.js
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
devvaansh
force-pushed
the
WEB-1029-playwright-add-create-client-form-validation-e-2-e-spec
branch
from
July 16, 2026 13:43
80b24ed to
374b364
Compare
…lidation-e-2-e-spec
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
proxy.conf.js (2)
26-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpstream base URL is now duplicated between
target:and the log strings.Each rule's
on.proxyReq/on.errorhandlers hardcode the same upstream base URL (https://demo.mifos.community,https://apis.mifos.community×2) that presumably already exists in this same block'starget:property. Previously the log usedthis.target; now it's a separate literal, so a future change totarget:won't be reflected in the logs, silently producing misleading debug output.This finding is likely shared with the equivalent pattern in `proxy.localhost.conf.js`; see the consolidated comment.♻️ Suggested fix (illustrated for the fineract-provider rule)
+const FINERACT_PROVIDER_TARGET = 'https://demo.mifos.community'; + { context: '/fineract-provider', - target: 'https://demo.mifos.community', + target: FINERACT_PROVIDER_TARGET, ... on: { proxyReq: function (proxyReq, req, res) { - console.log('[Proxy] Proxying:', req.method, req.url, '->', 'https://demo.mifos.community' + req.url); + console.log('[Proxy] Proxying:', req.method, req.url, '->', FINERACT_PROVIDER_TARGET + req.url); }, error: function (err, req, res) { console.error( '[Proxy] Error while proxying request:', req && req.method, req && req.url, - '-> https://demo.mifos.community -', + '-> ' + FINERACT_PROVIDER_TARGET + ' -', err && err.message ); ...Same pattern applies to the
/external-nationalidand/remittance-apirules.Also applies to: 57-74, 89-106
🤖 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 `@proxy.conf.js` around lines 26 - 42, Update the proxyReq and error handlers for each affected proxy rule to derive the upstream base URL from that rule’s target configuration instead of hardcoding URL literals in log messages. Apply the same change to the fineract-provider, external-nationalid, and remittance-api handlers, preserving the existing logging and error-response behavior.
26-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpstream base URL duplicated between
target:config and newon.*log strings, in both proxy files. Migrating offthis.target-based logging replaced it with hardcoded literal URLs insideconsole.log/console.error, so a future change to a rule'starget:won't be reflected in its own logs.
proxy.conf.js#L26-L106: for/fineract-provider,/external-nationalid, and/remittance-api, extract each rule's upstream base URL into a localconst(e.g.FINERACT_PROVIDER_TARGET) declared alongsidetarget:, and reference that constant from theproxyReq/errorlog calls instead of repeating the literal string.proxy.localhost.conf.js#L49-L103: apply the same fix for/external-nationalid(https://apis.mifos.community) and/remittance-api(http://54.225.231.146:8080).🤖 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 `@proxy.conf.js` around lines 26 - 106, The proxy configurations duplicate upstream URLs in target settings and proxyReq/error logs. In proxy.conf.js lines 26-106, define a local target constant for each /fineract-provider, /external-nationalid, and /remittance-api rule beside target and use it in both logging callbacks; apply the same change in proxy.localhost.conf.js lines 49-103 for /external-nationalid and /remittance-api, preserving each rule’s existing URL.
🤖 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-form-validation.spec.ts`:
- Around line 338-387: Complete the round-trip assertion in the test around
createClientPage by filling the required PERSON fields, including firstname and
lastname, after establishing the selected office; then, after switching back
from ENTITY to PERSON, assert the form reaches its valid next-step state (such
as the Preview step being visible). Preserve the existing subtree
presence/absence assertions and keep the test organized as setup, toggles, and
final validity verification.
---
Nitpick comments:
In `@proxy.conf.js`:
- Around line 26-42: Update the proxyReq and error handlers for each affected
proxy rule to derive the upstream base URL from that rule’s target configuration
instead of hardcoding URL literals in log messages. Apply the same change to the
fineract-provider, external-nationalid, and remittance-api handlers, preserving
the existing logging and error-response behavior.
- Around line 26-106: The proxy configurations duplicate upstream URLs in target
settings and proxyReq/error logs. In proxy.conf.js lines 26-106, define a local
target constant for each /fineract-provider, /external-nationalid, and
/remittance-api rule beside target and use it in both logging callbacks; apply
the same change in proxy.localhost.conf.js lines 49-103 for /external-nationalid
and /remittance-api, preserving each rule’s existing 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: fc15f322-2195-4866-aa58-2d97b11ebca9
📒 Files selected for processing (5)
package.jsonplaywright.config.tsplaywright/tests/clients/create-client-form-validation.spec.tsproxy.conf.jsproxy.localhost.conf.js
🚧 Files skipped from review as they are similar to previous changes (1)
- playwright.config.ts
Comment on lines
+338
to
+387
| test('switches the name-control subtree when the legal form toggles between Person and Entity', async ({ page }) => { | ||
| // The legalForm switch is a pure client-side re-render. The office | ||
| // is filled with the seeded default so the form reaches a valid | ||
| // baseline for the final round-trip assertion. | ||
| const officeName = SEEDED_HEAD_OFFICE; | ||
| const createClientPage = new CreateClientPage(page); | ||
|
|
||
| await createClientPage.navigate(); | ||
| await createClientPage.waitForLoad(); | ||
|
|
||
| await createClientPage.officeDropdown.click(); | ||
| await page.getByRole('option', { name: officeName, exact: false }).first().click(); | ||
|
|
||
| // Default legal form is PERSON (`setClientForm()` patches | ||
| // `LegalFormId.PERSON` on init). PERSON should show first/last | ||
| // name controls; ENTITY-only controls must be absent. | ||
| await expect(createClientPage.firstnameInput).toBeVisible(); | ||
| await expect(createClientPage.lastnameInput).toBeVisible(); | ||
| await expect(createClientPage.fullnameInput).toHaveCount(0); | ||
|
|
||
| // Switch to ENTITY. `buildDependencies()` removes firstname / | ||
| // middlename / lastname and adds fullname + a | ||
| // `clientNonPersonDetails` FormGroup containing a required | ||
| // `constitutionId`. | ||
| await createClientPage.legalFormDropdown.click(); | ||
| await page.getByRole('option', { name: 'Entity', exact: false }).first().click(); | ||
|
|
||
| await createClientPage.fullnameInput.waitFor({ state: 'visible', timeout: 10_000 }); | ||
| await expect(createClientPage.fullnameInput).toBeVisible(); | ||
| await expect(createClientPage.firstnameInput).toHaveCount(0); | ||
| await expect(createClientPage.lastnameInput).toHaveCount(0); | ||
| await expect(createClientPage.middlenameInput).toHaveCount(0); | ||
|
|
||
| // The clientNonPersonDetails subtree contains a `constitutionId` | ||
| // mat-select that only exists in ENTITY mode. Assert it renders | ||
| // so a regression that stops swapping the subtree is caught here. | ||
| const constitutionDropdown = page.locator('mat-select[formcontrolname="constitutionId"]'); | ||
| await expect(constitutionDropdown).toBeVisible(); | ||
|
|
||
| // Switch back to PERSON — the name-control subtree must return | ||
| // and the ENTITY-only controls must be torn down. | ||
| await createClientPage.legalFormDropdown.click(); | ||
| await page.getByRole('option', { name: 'Person', exact: false }).first().click(); | ||
|
|
||
| await createClientPage.firstnameInput.waitFor({ state: 'visible', timeout: 10_000 }); | ||
| await expect(createClientPage.firstnameInput).toBeVisible(); | ||
| await expect(createClientPage.lastnameInput).toBeVisible(); | ||
| await expect(createClientPage.fullnameInput).toHaveCount(0); | ||
| await expect(constitutionDropdown).toHaveCount(0); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing final assertion for the "round-trip" the docstring claims to verify.
The docstring (lines 339-341) says office is filled "so the form reaches a valid baseline for the final round-trip assertion," but firstname/lastname are never filled in this test, and after switching back to PERSON (lines 379-386) there's no assertion that the form is actually valid (e.g., Preview step visible). As written, the test can't catch a regression where switching back to PERSON fails to restore a genuinely valid form state.
✅ Proposed fix to complete the round-trip assertion
await createClientPage.firstnameInput.waitFor({ state: 'visible', timeout: 10_000 });
await expect(createClientPage.firstnameInput).toBeVisible();
await expect(createClientPage.lastnameInput).toBeVisible();
await expect(createClientPage.fullnameInput).toHaveCount(0);
await expect(constitutionDropdown).toHaveCount(0);
+
+ // Complete the required PERSON fields to prove the round trip
+ // actually restores a valid, submittable form.
+ await createClientPage.firstnameInput.fill('Devansh');
+ await createClientPage.lastnameInput.fill('Client');
+ await createClientPage.submittedOnDateInput.fill(SUBMITTED_ON_DATE);
+ await createClientPage.submittedOnDateInput.blur();
+ await expect(createClientPage.stepHeader('Preview')).toHaveCount(1);📝 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.
Suggested change
| test('switches the name-control subtree when the legal form toggles between Person and Entity', async ({ page }) => { | |
| // The legalForm switch is a pure client-side re-render. The office | |
| // is filled with the seeded default so the form reaches a valid | |
| // baseline for the final round-trip assertion. | |
| const officeName = SEEDED_HEAD_OFFICE; | |
| const createClientPage = new CreateClientPage(page); | |
| await createClientPage.navigate(); | |
| await createClientPage.waitForLoad(); | |
| await createClientPage.officeDropdown.click(); | |
| await page.getByRole('option', { name: officeName, exact: false }).first().click(); | |
| // Default legal form is PERSON (`setClientForm()` patches | |
| // `LegalFormId.PERSON` on init). PERSON should show first/last | |
| // name controls; ENTITY-only controls must be absent. | |
| await expect(createClientPage.firstnameInput).toBeVisible(); | |
| await expect(createClientPage.lastnameInput).toBeVisible(); | |
| await expect(createClientPage.fullnameInput).toHaveCount(0); | |
| // Switch to ENTITY. `buildDependencies()` removes firstname / | |
| // middlename / lastname and adds fullname + a | |
| // `clientNonPersonDetails` FormGroup containing a required | |
| // `constitutionId`. | |
| await createClientPage.legalFormDropdown.click(); | |
| await page.getByRole('option', { name: 'Entity', exact: false }).first().click(); | |
| await createClientPage.fullnameInput.waitFor({ state: 'visible', timeout: 10_000 }); | |
| await expect(createClientPage.fullnameInput).toBeVisible(); | |
| await expect(createClientPage.firstnameInput).toHaveCount(0); | |
| await expect(createClientPage.lastnameInput).toHaveCount(0); | |
| await expect(createClientPage.middlenameInput).toHaveCount(0); | |
| // The clientNonPersonDetails subtree contains a `constitutionId` | |
| // mat-select that only exists in ENTITY mode. Assert it renders | |
| // so a regression that stops swapping the subtree is caught here. | |
| const constitutionDropdown = page.locator('mat-select[formcontrolname="constitutionId"]'); | |
| await expect(constitutionDropdown).toBeVisible(); | |
| // Switch back to PERSON — the name-control subtree must return | |
| // and the ENTITY-only controls must be torn down. | |
| await createClientPage.legalFormDropdown.click(); | |
| await page.getByRole('option', { name: 'Person', exact: false }).first().click(); | |
| await createClientPage.firstnameInput.waitFor({ state: 'visible', timeout: 10_000 }); | |
| await expect(createClientPage.firstnameInput).toBeVisible(); | |
| await expect(createClientPage.lastnameInput).toBeVisible(); | |
| await expect(createClientPage.fullnameInput).toHaveCount(0); | |
| await expect(constitutionDropdown).toHaveCount(0); | |
| }); | |
| test('switches the name-control subtree when the legal form toggles between Person and Entity', async ({ page }) => { | |
| // The legalForm switch is a pure client-side re-render. The office | |
| // is filled with the seeded default so the form reaches a valid | |
| // baseline for the final round-trip assertion. | |
| const officeName = SEEDED_HEAD_OFFICE; | |
| const createClientPage = new CreateClientPage(page); | |
| await createClientPage.navigate(); | |
| await createClientPage.waitForLoad(); | |
| await createClientPage.officeDropdown.click(); | |
| await page.getByRole('option', { name: officeName, exact: false }).first().click(); | |
| // Default legal form is PERSON (`setClientForm()` patches | |
| // `LegalFormId.PERSON` on init). PERSON should show first/last | |
| // name controls; ENTITY-only controls must be absent. | |
| await expect(createClientPage.firstnameInput).toBeVisible(); | |
| await expect(createClientPage.lastnameInput).toBeVisible(); | |
| await expect(createClientPage.fullnameInput).toHaveCount(0); | |
| // Switch to ENTITY. `buildDependencies()` removes firstname / | |
| // middlename / lastname and adds fullname + a | |
| // `clientNonPersonDetails` FormGroup containing a required | |
| // `constitutionId`. | |
| await createClientPage.legalFormDropdown.click(); | |
| await page.getByRole('option', { name: 'Entity', exact: false }).first().click(); | |
| await createClientPage.fullnameInput.waitFor({ state: 'visible', timeout: 10_000 }); | |
| await expect(createClientPage.fullnameInput).toBeVisible(); | |
| await expect(createClientPage.firstnameInput).toHaveCount(0); | |
| await expect(createClientPage.lastnameInput).toHaveCount(0); | |
| await expect(createClientPage.middlenameInput).toHaveCount(0); | |
| // The clientNonPersonDetails subtree contains a `constitutionId` | |
| // mat-select that only exists in ENTITY mode. Assert it renders | |
| // so a regression that stops swapping the subtree is caught here. | |
| const constitutionDropdown = page.locator('mat-select[formcontrolname="constitutionId"]'); | |
| await expect(constitutionDropdown).toBeVisible(); | |
| // Switch back to PERSON — the name-control subtree must return | |
| // and the ENTITY-only controls must be torn down. | |
| await createClientPage.legalFormDropdown.click(); | |
| await page.getByRole('option', { name: 'Person', exact: false }).first().click(); | |
| await createClientPage.firstnameInput.waitFor({ state: 'visible', timeout: 10_000 }); | |
| await expect(createClientPage.firstnameInput).toBeVisible(); | |
| await expect(createClientPage.lastnameInput).toBeVisible(); | |
| await expect(createClientPage.fullnameInput).toHaveCount(0); | |
| await expect(constitutionDropdown).toHaveCount(0); | |
| // Complete the required PERSON fields to prove the round trip | |
| // actually restores a valid, submittable form. | |
| await createClientPage.firstnameInput.fill('Devansh'); | |
| await createClientPage.lastnameInput.fill('Client'); | |
| await createClientPage.submittedOnDateInput.fill(SUBMITTED_ON_DATE); | |
| await createClientPage.submittedOnDateInput.blur(); | |
| await expect(createClientPage.stepHeader('Preview')).toHaveCount(1); | |
| }); |
🤖 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-form-validation.spec.ts` around lines
338 - 387, Complete the round-trip assertion in the test around createClientPage
by filling the required PERSON fields, including firstname and lastname, after
establishing the selected office; then, after switching back from ENTITY to
PERSON, assert the form reaches its valid next-step state (such as the Preview
step being visible). Preserve the existing subtree presence/absence assertions
and keep the test organized as setup, toggles, and final validity verification.
Source: Path instructions
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
3 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.
Description
Adds a dedicated Playwright spec that exhaustively covers the client-side validator surface of the create-client form (
ClientGeneralStepComponent). The existing CRUD spec (WEB-1019) exercises the happy path end-to-end and a single missing-lastname case; this spec fills the gap by targeting every individual validator in isolation without ever submitting data to Fineract.Five tests, all pure form-behaviour:
requiredonmat-select;areFormvalids()gateng-invalid+mat-form-field-invalid+ mat-errorrequiredon firstnamerequiredon lastnameactivationDate<submittedOnDate→ wrappermat-form-field-invalid; Preview absentmatDatepickerMincross-field validatorlegalFormIdPERSON↔ENTITY toggle → name-control subtree swappedbuildDependencies()tearing downfirstname/lastname, addingfullname + constitutionId(and back)Each test includes a recovery assertion: fix the bad input → Preview step must reappear, proving the failure was caused by that specific input alone.
Supporting changes:
target: http → https;pathRewriteremoved so Fineract receives the full/fineract-provider/…pathstart:localscript (ng serve --proxy-config proxy.localhost.conf.js)webServer.commandupdated tonpm run start:localDependencies: Requires the Fineract Docker stack for the shared
setupproject auth step (npm run e2e:docker:up). Individual test bodies mock/clients/templateand/fieldconfiguration/ADDRESSviapage.route()so they are independent of Fineract for the form behaviour they actually test.Related issues and discussion
#WEB-1029
Screenshots, if any
Screen.Recording.2026-07-15.at.3.07.54.PM.mov
Checklist
30d56abf8)web-app/.github/CONTRIBUTING.mdSummary by CodeRabbit
Summary by CodeRabbit
Development
Tests