WEB-1029: [Playwright] Add create-client form-validation E2E spec#3717
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
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
|
@devvaansh needs update to base branch.. If I do it I cant then approve for merge |
30d56ab to
b2d4490
Compare
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
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
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
80b24ed to
374b364
Compare
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
a8e8ab8 to
c9f88fc
Compare
d238297 to
cdca011
Compare
cdca011 to
f6ada20
Compare
…lidation-e-2-e-spec
4 participants
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