feat(e2e): Playwright E2E test suite (#57)#79
Merged
Conversation
Install @playwright/test, add test:e2e scripts, create playwright.config.ts with Chromium project, global-setup for Frappe session auth, and a smoke spec that asserts an authenticated user can reach the forms dashboard. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add data-testid to 7 Vue components (form-card, btn-new-form, field-type-*, btn-save-form, btn-publish, btn-submit-form, submission-success, btn-send-invite, input-invite-email). Restructure InviteMemberDialog to use #actions slot so the send button is directly targetable. Create Page Object Models (dashboard, form-builder, submission, team) and a Playwright fixture with API helpers for form create/publish/teardown. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ests)
Remove all data-testid attributes from Vue components (reverted to original).
Rewrite POMs with semantic selectors: getByRole/getByLabel/getByText, scoped
to pre-existing data-form-builder-component attributes. Add form-creation.spec.ts
covering dashboard listing, builder load, field canvas, publish, and unpublish.
Tests avoid triggering Frappe validation by never saving a field with an empty
label; publish/unpublish uses setValue({is_published}) which is always valid.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Specs:
- form-submission.spec.ts — guest context submission + success message
- submission-view.spec.ts — empty state and post-submission row visibility
- team-invite.spec.ts — invite dialog open/fill/send flow
Fixture:
- submitForm fixture for creating a guest submission via API
Reliability fixes:
- smoke.spec.ts: reload on Frappe 500 under parallel startup load
- TeamPage.goto(): waitForLoadState("networkidle") so Vue API calls
complete before assertions run
- TeamPage.openInviteDialog(): wait for email input to be visible (not
just the dialog shell) before returning
- playwright.config.ts: retries=1 everywhere; github reporter added
CI:
- .github/workflows/ui-tests.yml: full Playwright E2E workflow running
against a real Frappe bench on every PR and push to develop
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Playwright end-to-end tests and CI: a GitHub Actions UI Tests workflow, Playwright config/scripts, global auth setup, typed fixtures, page-object helpers, multiple E2E test suites, .gitignore updates, and small backend changes for admin team setup and narrower cache invalidation. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions"
participant Dock as "Containers (MariaDB, Redis)"
participant Bench as "Bench / setup"
participant App as "App (forms.test:8000)"
participant PW as "Playwright Runner"
participant Art as "Artifacts"
GH->>Dock: start MariaDB & Redis containers
GH->>Bench: checkout, install deps, init bench, install apps
Bench->>Dock: health-check DB (mariadb-admin ping)
Bench->>App: start server (bench start)
GH->>App: poll until http reachable
GH->>PW: run tests (uses storageState, BASE_URL)
PW->>App: browser/API interactions (create/publish/submit/etc.)
PW->>Art: upload html report & failure artifacts
GH->>Bench: on failure fetch logs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
frontend/e2e/helpers/submission.ts (1)
6-8: Normalizeroutebefore building the public URL.This avoids edge-case double slashes when API-provided routes include a leading
/.♻️ Suggested fix
async goto(route: string) { - await this.page.goto(`/forms/p/${route}`); + const normalizedRoute = route.replace(/^\/+/, ""); + await this.page.goto(`/forms/p/${normalizedRoute}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/helpers/submission.ts` around lines 6 - 8, The goto helper builds a public URL using the passed route but doesn't normalize it, which can produce double slashes when route starts with '/'; update the async goto(route: string) function to normalize the route (e.g., strip any leading slashes or use a safe join) before calling this.page.goto(`/forms/p/${route}`) so the resulting URL never contains `//`; ensure the change is applied to the goto method and any callers expecting the same behavior.frontend/e2e/specs/smoke.spec.ts (1)
4-8: Make startup 500 handling bounded-retry instead of single-retry.A single reload can still leave this smoke test flaky under parallel cold starts.
♻️ Suggested reliability tweak
- const response = await page.goto("/forms"); + let response = await page.goto("/forms"); // Frappe dev server can 500 on the very first request under parallel test startup - if (response?.status() === 500) { - await page.reload({ waitUntil: "networkidle" }); + for (let attempt = 0; attempt < 3 && response?.status() === 500; attempt++) { + response = await page.reload({ waitUntil: "networkidle" }); + } + if (response?.status() === 500) { + throw new Error("Dashboard remained unavailable after retries"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/specs/smoke.spec.ts` around lines 4 - 8, Replace the single conditional reload with a bounded-retry loop around the initial navigation: call page.goto("/forms") up to a small maxRetries (e.g., 3-5) while checking response?.status() === 500, and on 500 perform page.reload({ waitUntil: "networkidle" }) with a short delay/backoff between attempts; if all retries are exhausted still returning 500, fail the test by throwing an error or asserting the status. Use variables like maxRetries, attempt, and the existing response variable to implement the loop so page.goto and page.reload logic is preserved but retried deterministically.frontend/e2e/specs/form-submission.spec.ts (1)
24-40: Guarantee guest context cleanup on failure paths.If an assertion fails before Line 40, the guest context may remain open for the worker.
♻️ Suggested fix
- const guestCtx = await browser.newContext(); - const guestPage = await guestCtx.newPage(); - const submissionPage = new SubmissionPage(guestPage); - - await submissionPage.goto(route); - await expect(guestPage.getByRole("button", { name: "Submit" })).toBeVisible( - { timeout: 10000 } - ); - - await submissionPage.submit(); - - // Success section appears with the default thank-you copy - await expect(submissionPage.successMessage()).toBeVisible({ - timeout: 10000, - }); - - await guestCtx.close(); + const guestCtx = await browser.newContext(); + try { + const guestPage = await guestCtx.newPage(); + const submissionPage = new SubmissionPage(guestPage); + + await submissionPage.goto(route); + await expect( + guestPage.getByRole("button", { name: "Submit" }) + ).toBeVisible({ timeout: 10000 }); + + await submissionPage.submit(); + + // Success section appears with the default thank-you copy + await expect(submissionPage.successMessage()).toBeVisible({ + timeout: 10000, + }); + } finally { + await guestCtx.close(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/specs/form-submission.spec.ts` around lines 24 - 40, The test creates a browser context (guestCtx) and page (guestPage) and may leak the context if an assertion fails before the explicit guestCtx.close(); wrap the interaction and assertions that use guestCtx/guestPage (e.g., the calls to new SubmissionPage(guestPage), submissionPage.goto(route), expect(...).toBeVisible, submissionPage.submit(), and submissionPage.successMessage()) in a try/finally and always call guestCtx.close() in the finally block so the context is closed on both success and failure.frontend/e2e/helpers/team.ts (1)
29-31: Consider scopingmemberRows()to exclude header rows.
getByRole("row")will match all table rows including any header row. If this method is intended to return only member data rows, you may want to scope it more precisely (e.g., withintbodyor filter by content).This is minor since the method isn't currently used in the test specs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/helpers/team.ts` around lines 29 - 31, The memberRows() helper currently returns all rows via getByRole("row"), which includes header rows; update memberRows() to scope to only data rows by querying within the table body or filtering out header rows—for example, use a locator like this.page.locator("tbody").getByRole("row") or call getByRole("row") and filter by excluding rows that contain header cells (th) or known header text; modify the memberRows() function accordingly so it returns only member data rows.frontend/e2e/fixtures/test-data.fixture.ts (3)
15-22: Consider adding response status validation.
fetchTeamIddoesn't verify the response status before parsing JSON. If the API returns an error (4xx/5xx), the JSON parsing or property access may fail with an unclear error.Proposed fix
async function fetchTeamId(apiContext: APIRequestContext): Promise<string> { const res = await apiContext.get( "/api/method/forms_pro.api.user.get_user_teams" ); + if (!res.ok()) { + throw new Error(`Failed to fetch teams: ${res.status()}`); + } const { message } = await res.json(); if (!message?.length) throw new Error("No team found for test user"); return message[0].name as string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/fixtures/test-data.fixture.ts` around lines 15 - 22, The function fetchTeamId should validate the HTTP response before parsing JSON: check res.ok (or res.status) after apiContext.get and, if not OK, read/res.text() or res.json() for details and throw a descriptive Error including the status and response body; then proceed to parse JSON and validate message length as currently implemented. Update fetchTeamId to use the APIRequestContext response (res) for the status check and include the status/response content in the thrown error to make failures clear.
87-95: Add response validation tosubmitFormfixture.Similar to other fixtures,
submitFormdoesn't validate the response. A failed submission would go unnoticed, potentially causing confusing test failures downstream.Proposed fix
submitForm: async ({ apiContext }, use) => { await use(async (formId: string) => { - await apiContext.post( + const res = await apiContext.post( "/api/method/forms_pro.api.submission.submit_form_response", { data: { form_id: formId, form_data: [] } } ); + if (!res.ok()) { + throw new Error(`Failed to submit form: ${res.status()}`); + } }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/fixtures/test-data.fixture.ts` around lines 87 - 95, The submitForm fixture currently fires apiContext.post in submitForm but does not validate the response; update the submitForm implementation to capture the response from apiContext.post, assert the HTTP status is successful (e.g., response.ok()/status 2xx) and validate the returned JSON payload contains the expected success/identifier fields for a submission before returning; reference the submitForm fixture function and the apiContext.post call and ensure tests fail fast when submission fails by throwing or using the test runner assertion on bad status or unexpected response shape.
38-57: Add response validation to prevent silent failures during test setup.The
createFormfixture doesn't validate the API response status. If form creation fails (e.g., permission error, server error), the test will proceed with invalid data and fail with a confusing error later.Proposed fix
await use(async () => { const teamId = await fetchTeamId(apiContext); const res = await apiContext.post( "/api/method/forms_pro.utils.form_generator.create_form", { data: { team_id: teamId } } ); + if (!res.ok()) { + throw new Error(`Failed to create form: ${res.status()}`); + } const { message } = await res.json(); const formId = message.form_document as string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/fixtures/test-data.fixture.ts` around lines 38 - 57, The createForm fixture currently assumes the POST succeeded; add response validation after the apiContext.post call in the createForm async fixture: check res.ok (or status) and throw or fail the fixture with a clear error including status and body if not ok, then validate that the parsed response contains message.form_document and that it's a string before pushing to the created array and returning formId; ensure the teardown loop remains but only runs for IDs actually pushed into created so you don't attempt to delete undefined IDs (refer to symbols createForm, apiContext.post, res, message, formId, created and the endpoint "/api/method/forms_pro.utils.form_generator.create_form")..github/workflows/ui-tests.yml (1)
86-86: Pinfrappe_factory_botto a specific commit SHA for reproducibility.The workflow references an external repository using the
mainbranch, which can change unexpectedly and cause CI failures. Use a commit SHA instead of--branch mainto ensure deterministic builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ui-tests.yml at line 86, The workflow currently installs the external repo using the mutable branch flag in the bench command (bench get-app frappe_factory_bot ... --branch main); replace the branch reference with a specific commit SHA to pin the dependency (e.g., use the bench get-app invocation with a --commit <COMMIT_SHA> or equivalent option) so the CI uses a deterministic, reproducible version of frappe_factory_bot; update the command that references frappe_factory_bot to target that commit SHA and remove or replace the --branch main argument.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ui-tests.yml:
- Around line 9-11: The concurrency group currently uses ui-tests-${{
github.event.number }} which is undefined for push events and causes collisions;
update the concurrency group expression (the concurrency: group value where
ui-tests-${{ github.event.number }} is used) to use a stable identifier that
exists for all events, e.g. prefer github.run_id or a fallback to pull_request
number (use an expression like using github.event.pull_request.number with
fallback to github.run_id) so each workflow run gets a unique concurrency group
across push and pull_request events.
In `@frontend/e2e/global-setup.ts`:
- Around line 19-27: Ensure the login POST via page.request.post is validated
before persisting session: check the response (e.g., response.ok or
response.status === 200 and any expected body fields) from page.request.post and
throw or exit on failure so context.storageState({ path: AUTH_FILE }) and
browser.close() only run for a successful login; include failure context
(status/text) in the thrown error or log to make the cause clear.
---
Nitpick comments:
In @.github/workflows/ui-tests.yml:
- Line 86: The workflow currently installs the external repo using the mutable
branch flag in the bench command (bench get-app frappe_factory_bot ... --branch
main); replace the branch reference with a specific commit SHA to pin the
dependency (e.g., use the bench get-app invocation with a --commit <COMMIT_SHA>
or equivalent option) so the CI uses a deterministic, reproducible version of
frappe_factory_bot; update the command that references frappe_factory_bot to
target that commit SHA and remove or replace the --branch main argument.
In `@frontend/e2e/fixtures/test-data.fixture.ts`:
- Around line 15-22: The function fetchTeamId should validate the HTTP response
before parsing JSON: check res.ok (or res.status) after apiContext.get and, if
not OK, read/res.text() or res.json() for details and throw a descriptive Error
including the status and response body; then proceed to parse JSON and validate
message length as currently implemented. Update fetchTeamId to use the
APIRequestContext response (res) for the status check and include the
status/response content in the thrown error to make failures clear.
- Around line 87-95: The submitForm fixture currently fires apiContext.post in
submitForm but does not validate the response; update the submitForm
implementation to capture the response from apiContext.post, assert the HTTP
status is successful (e.g., response.ok()/status 2xx) and validate the returned
JSON payload contains the expected success/identifier fields for a submission
before returning; reference the submitForm fixture function and the
apiContext.post call and ensure tests fail fast when submission fails by
throwing or using the test runner assertion on bad status or unexpected response
shape.
- Around line 38-57: The createForm fixture currently assumes the POST
succeeded; add response validation after the apiContext.post call in the
createForm async fixture: check res.ok (or status) and throw or fail the fixture
with a clear error including status and body if not ok, then validate that the
parsed response contains message.form_document and that it's a string before
pushing to the created array and returning formId; ensure the teardown loop
remains but only runs for IDs actually pushed into created so you don't attempt
to delete undefined IDs (refer to symbols createForm, apiContext.post, res,
message, formId, created and the endpoint
"/api/method/forms_pro.utils.form_generator.create_form").
In `@frontend/e2e/helpers/submission.ts`:
- Around line 6-8: The goto helper builds a public URL using the passed route
but doesn't normalize it, which can produce double slashes when route starts
with '/'; update the async goto(route: string) function to normalize the route
(e.g., strip any leading slashes or use a safe join) before calling
this.page.goto(`/forms/p/${route}`) so the resulting URL never contains `//`;
ensure the change is applied to the goto method and any callers expecting the
same behavior.
In `@frontend/e2e/helpers/team.ts`:
- Around line 29-31: The memberRows() helper currently returns all rows via
getByRole("row"), which includes header rows; update memberRows() to scope to
only data rows by querying within the table body or filtering out header
rows—for example, use a locator like this.page.locator("tbody").getByRole("row")
or call getByRole("row") and filter by excluding rows that contain header cells
(th) or known header text; modify the memberRows() function accordingly so it
returns only member data rows.
In `@frontend/e2e/specs/form-submission.spec.ts`:
- Around line 24-40: The test creates a browser context (guestCtx) and page
(guestPage) and may leak the context if an assertion fails before the explicit
guestCtx.close(); wrap the interaction and assertions that use
guestCtx/guestPage (e.g., the calls to new SubmissionPage(guestPage),
submissionPage.goto(route), expect(...).toBeVisible, submissionPage.submit(),
and submissionPage.successMessage()) in a try/finally and always call
guestCtx.close() in the finally block so the context is closed on both success
and failure.
In `@frontend/e2e/specs/smoke.spec.ts`:
- Around line 4-8: Replace the single conditional reload with a bounded-retry
loop around the initial navigation: call page.goto("/forms") up to a small
maxRetries (e.g., 3-5) while checking response?.status() === 500, and on 500
perform page.reload({ waitUntil: "networkidle" }) with a short delay/backoff
between attempts; if all retries are exhausted still returning 500, fail the
test by throwing an error or asserting the status. Use variables like
maxRetries, attempt, and the existing response variable to implement the loop so
page.goto and page.reload logic is preserved but retried deterministically.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06329ea7-e563-4ddd-bc76-3ef2332433a7
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
.github/workflows/ui-tests.yml.gitignorefrontend/e2e/fixtures/test-data.fixture.tsfrontend/e2e/global-setup.tsfrontend/e2e/helpers/dashboard.tsfrontend/e2e/helpers/form-builder.tsfrontend/e2e/helpers/submission.tsfrontend/e2e/helpers/team.tsfrontend/e2e/specs/form-creation.spec.tsfrontend/e2e/specs/form-submission.spec.tsfrontend/e2e/specs/smoke.spec.tsfrontend/e2e/specs/submission-view.spec.tsfrontend/e2e/specs/team-invite.spec.tsfrontend/package.jsonfrontend/playwright.config.ts
Comment on lines
+19
to
+27
| await page.request.post(`${baseURL}/api/method/login`, { | ||
| form: { | ||
| usr: "Administrator", | ||
| pwd: process.env.TEST_ADMIN_PASSWORD ?? "admin", | ||
| }, | ||
| }); | ||
|
|
||
| await context.storageState({ path: AUTH_FILE }); | ||
| await browser.close(); |
There was a problem hiding this comment.
Fail fast if login request fails before writing storage state.
Right now, Line 19-Line 27 can persist an unauthenticated state when login fails, which turns into opaque downstream test failures.
🛠️ Suggested fix
- await page.request.post(`${baseURL}/api/method/login`, {
+ const loginRes = await page.request.post(`${baseURL}/api/method/login`, {
form: {
usr: "Administrator",
pwd: process.env.TEST_ADMIN_PASSWORD ?? "admin",
},
});
+ if (!loginRes.ok()) {
+ throw new Error(
+ `Global setup login failed: ${loginRes.status()} ${loginRes.statusText()}`
+ );
+ }
await context.storageState({ path: AUTH_FILE });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/e2e/global-setup.ts` around lines 19 - 27, Ensure the login POST via
page.request.post is validated before persisting session: check the response
(e.g., response.ok or response.status === 200 and any expected body fields) from
page.request.post and throw or exit on failure so context.storageState({ path:
AUTH_FILE }) and browser.close() only run for a successful login; include
failure context (status/text) in the thrown error or log to make the cause
clear.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e, fix action versions - Replace bench serve (silent crash) with bench start + Procfile (disable watch/schedule) - Add forms.test to /etc/hosts and set host_name config; rename site test_site → forms.test - Swap npx wait-on for a curl loop (no install overhead, same pattern as buzz CI) - Fix non-existent action versions: checkout@v6→@v4, setup-python@v6→@v5, setup-node@v6→@v4 - Add timeout-minutes: 60, fix concurrency group for push events, add workflow_dispatch - Add Playwright browser cache step and bench logs dump on failure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ault team `before_tests()` already added the role to Administrator, but the `on_update` hook that creates the default team checks `has_value_changed` on a freshly-fetched doc (which is always False), so no team was ever created. `_ensure_admin_has_default_team()` closes that gap explicitly. A new "Setup E2E test data" CI step calls `bench execute forms_pro.install.before_tests` before the server starts, so the test user has a team available when fixtures call `get_user_teams`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e clear
frappe.clear_cache() flushes Redis entirely, including session and CSRF
token data. In the E2E test suite this causes all POST API calls after
the first form creation to fail with CSRFTokenError, because the token
captured during global-setup is invalidated server-side.
Replace with frappe.clear_document_cache("DocType", name) which only
evicts the specific newly-created DocType from the document cache —
sufficient for the new schema to be reloaded on next access, and does
not touch sessions or CSRF tokens.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Frappe's DocType.on_update() calls frappe.clear_cache() internally whenever a DocType is saved. This flushes session/CSRF data from Redis, invalidating the CSRF token captured in Playwright's storageState.json. All POST API calls after the first form creation then fail with CSRFTokenError. ignore_csrf=1 is the standard Frappe configuration for CI/test sites and skips CSRF validation entirely, which is correct here. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sets mute_emails=1 in site config so Frappe returns a dummy outgoing account instead of throwing OutgoingEmailError when no real account is configured, allowing the team-invite E2E test to succeed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
forms_pro/install.py (1)
28-35: Guaranteecurrent_teaminitialization after team creation.
create_default_team_for_user()may set defaults for the creator/owner context, not necessarilyAdministrator. To make this helper’s contract deterministic, re-fetch teams after creation soget_user_teams("Administrator")can initializecurrent_teamwhen needed.Proposed hardening
def _ensure_admin_has_default_team(): from forms_pro.overrides.roles import create_default_team_for_user from forms_pro.utils.teams import get_user_teams if get_user_teams("Administrator"): return admin = frappe.get_doc("User", "Administrator") create_default_team_for_user(admin) + # Ensure current_team default is initialized for Administrator. + get_user_teams("Administrator")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/install.py` around lines 28 - 35, The helper _ensure_admin_has_default_team can leave Administrator without an initialized current_team because create_default_team_for_user may set defaults on the creator context; after calling create_default_team_for_user(admin) re-call get_user_teams("Administrator") (or otherwise reload the Administrator user's teams) to re-fetch and ensure current_team is initialized deterministically; update _ensure_admin_has_default_team to fetch teams again after team creation so get_user_teams initializes current_team reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@forms_pro/install.py`:
- Around line 28-35: The helper _ensure_admin_has_default_team can leave
Administrator without an initialized current_team because
create_default_team_for_user may set defaults on the creator context; after
calling create_default_team_for_user(admin) re-call
get_user_teams("Administrator") (or otherwise reload the Administrator user's
teams) to re-fetch and ensure current_team is initialized deterministically;
update _ensure_admin_has_default_team to fetch teams again after team creation
so get_user_teams initializes current_team reliably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa097140-d4e3-43f7-8fff-29647248a12e
📒 Files selected for processing (3)
.github/workflows/ui-tests.ymlforms_pro/install.pyforms_pro/utils/form_generator.py
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ui-tests.yml
- Switch global-setup login from Administrator to FORMS_PRO_TEST_USER (test_forms_pro_user@example.com / testforms123) so E2E tests run under a least-privileged Forms Pro User, matching real-user conditions - Fix create_test_user() to strip the System User role Frappe auto-assigns on insert, leaving only the Forms Pro User role - Set a known password via update_password() so Playwright can log in - Add frontend/e2e/tsconfig.json with @types/node so process.env references in global-setup.ts type-check correctly - Install @types/node dev dependency Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forms_pro/install.py (1)
47-61:⚠️ Potential issue | 🟠 Major
create_test_user()is not idempotent for existing users.The early return at Line 47-48 skips role cleanup and password reset for pre-existing users, so reused sites can retain stale privileges/credentials and break deterministic E2E auth.
Suggested change
def create_test_user(): from frappe.utils.password import update_password - if frappe.db.exists("User", FORMS_PRO_TEST_USER): - return - - user: User = frappe.new_doc("User") - user.email = FORMS_PRO_TEST_USER - user.first_name = "Test" - user.last_name = "Forms Pro User" - user.insert(ignore_permissions=True) + if frappe.db.exists("User", FORMS_PRO_TEST_USER): + user: User = frappe.get_doc("User", FORMS_PRO_TEST_USER) + else: + user = frappe.new_doc("User") + user.email = FORMS_PRO_TEST_USER + user.first_name = "Test" + user.last_name = "Forms Pro User" + user.insert(ignore_permissions=True) # Frappe auto-assigns System User on insert; replace with only Forms Pro User user.roles = [] user.append("roles", {"role": FORMS_PRO_ROLE}) user.save(ignore_permissions=True) update_password(FORMS_PRO_TEST_USER, "testforms123")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/install.py` around lines 47 - 61, The current create_test_user() exits early when the User exists, skipping role normalization and password reset; instead, if frappe.db.exists("User", FORMS_PRO_TEST_USER) is True, load the existing user doc (frappe.get_doc("User", FORMS_PRO_TEST_USER)), clear its roles (set user.roles = [] or remove non-FORMS_PRO_ROLE entries), append the Forms Pro role via user.append("roles", {"role": FORMS_PRO_ROLE}), save with ignore_permissions=True, and still call update_password(FORMS_PRO_TEST_USER, "testforms123") so both newly created and pre-existing users are normalized and reset idempotently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@forms_pro/install.py`:
- Around line 29-33: get_user_teams is a helper that can create defaults and
must not be called for a simple existence check in the install hook; replace the
predicate usage of get_user_teams("Administrator") with a read-only existence
check (e.g., a new helper like team_role_exists("Administrator") or a direct
non-mutating DB query against the Team/Role model) so the install hook doesn't
trigger side effects, update imports accordingly, and leave
create_default_team_for_user for actual creation logic only.
---
Outside diff comments:
In `@forms_pro/install.py`:
- Around line 47-61: The current create_test_user() exits early when the User
exists, skipping role normalization and password reset; instead, if
frappe.db.exists("User", FORMS_PRO_TEST_USER) is True, load the existing user
doc (frappe.get_doc("User", FORMS_PRO_TEST_USER)), clear its roles (set
user.roles = [] or remove non-FORMS_PRO_ROLE entries), append the Forms Pro role
via user.append("roles", {"role": FORMS_PRO_ROLE}), save with
ignore_permissions=True, and still call update_password(FORMS_PRO_TEST_USER,
"testforms123") so both newly created and pre-existing users are normalized and
reset idempotently.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea3d8528-d019-49fd-a0c7-9b491cfada62
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.github/workflows/ui-tests.ymlforms_pro/install.pyfrontend/e2e/global-setup.tsfrontend/e2e/tsconfig.jsonfrontend/package.json
✅ Files skipped from review due to trivial changes (2)
- frontend/e2e/tsconfig.json
- .github/workflows/ui-tests.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/e2e/global-setup.ts
- frontend/package.json
Comment on lines
+29
to
+33
| from forms_pro.overrides.roles import create_default_team_for_user | ||
| from forms_pro.utils.teams import get_user_teams | ||
|
|
||
| if get_user_teams("Administrator"): | ||
| return |
There was a problem hiding this comment.
Avoid side-effecting helper calls for existence checks in install hooks.
Line 32 uses get_user_teams("Administrator") as a predicate, but that helper can mutate defaults. For setup hooks, prefer a read-only existence check.
Suggested change
def _ensure_admin_has_default_team():
from forms_pro.overrides.roles import create_default_team_for_user
- from forms_pro.utils.teams import get_user_teams
- if get_user_teams("Administrator"):
+ if frappe.db.exists("FP Team Member", {"user": "Administrator"}):
return
admin = frappe.get_doc("User", "Administrator")
create_default_team_for_user(admin)📝 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
| from forms_pro.overrides.roles import create_default_team_for_user | |
| from forms_pro.utils.teams import get_user_teams | |
| if get_user_teams("Administrator"): | |
| return | |
| from forms_pro.overrides.roles import create_default_team_for_user | |
| if frappe.db.exists("FP Team Member", {"user": "Administrator"}): | |
| return |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forms_pro/install.py` around lines 29 - 33, get_user_teams is a helper that
can create defaults and must not be called for a simple existence check in the
install hook; replace the predicate usage of get_user_teams("Administrator")
with a read-only existence check (e.g., a new helper like
team_role_exists("Administrator") or a direct non-mutating DB query against the
Team/Role model) so the install hook doesn't trigger side effects, update
imports accordingly, and leave create_default_team_for_user for actual creation
logic only.
harshtandiya
added a commit
that referenced
this pull request
Apr 19, 2026
* feat(e2e): Phase 1 — Playwright foundation
Install @playwright/test, add test:e2e scripts, create playwright.config.ts
with Chromium project, global-setup for Frappe session auth, and a smoke
spec that asserts an authenticated user can reach the forms dashboard.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* feat(e2e): Phase 2 — data-testid attributes, POMs, and test data fixture
Add data-testid to 7 Vue components (form-card, btn-new-form, field-type-*,
btn-save-form, btn-publish, btn-submit-form, submission-success,
btn-send-invite, input-invite-email). Restructure InviteMemberDialog to use
#actions slot so the send button is directly targetable. Create Page Object
Models (dashboard, form-builder, submission, team) and a Playwright fixture
with API helpers for form create/publish/teardown.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* feat(e2e): Phase 2+3 — semantic selectors and form-creation spec (5 tests)
Remove all data-testid attributes from Vue components (reverted to original).
Rewrite POMs with semantic selectors: getByRole/getByLabel/getByText, scoped
to pre-existing data-form-builder-component attributes. Add form-creation.spec.ts
covering dashboard listing, builder load, field canvas, publish, and unpublish.
Tests avoid triggering Frappe validation by never saving a field with an empty
label; publish/unpublish uses setValue({is_published}) which is always valid.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* feat(e2e): Phase 3+4 — remaining specs, flake fixes, and CI workflow
Specs:
- form-submission.spec.ts — guest context submission + success message
- submission-view.spec.ts — empty state and post-submission row visibility
- team-invite.spec.ts — invite dialog open/fill/send flow
Fixture:
- submitForm fixture for creating a guest submission via API
Reliability fixes:
- smoke.spec.ts: reload on Frappe 500 under parallel startup load
- TeamPage.goto(): waitForLoadState("networkidle") so Vue API calls
complete before assertions run
- TeamPage.openInviteDialog(): wait for email input to be visible (not
just the dialog shell) before returning
- playwright.config.ts: retries=1 everywhere; github reporter added
CI:
- .github/workflows/ui-tests.yml: full Playwright E2E workflow running
against a real Frappe bench on every PR and push to develop
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* ci(ui-tests): log bench serve output on startup timeout
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* ci(ui-tests): fix server startup — switch to bench start, add hostname, fix action versions
- Replace bench serve (silent crash) with bench start + Procfile (disable watch/schedule)
- Add forms.test to /etc/hosts and set host_name config; rename site test_site → forms.test
- Swap npx wait-on for a curl loop (no install overhead, same pattern as buzz CI)
- Fix non-existent action versions: checkout@v6→@v4, setup-python@v6→@v5, setup-node@v6→@v4
- Add timeout-minutes: 60, fix concurrency group for push events, add workflow_dispatch
- Add Playwright browser cache step and bench logs dump on failure
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* ci(ui-tests): setup E2E test data — give Admin Forms Pro role and default team
`before_tests()` already added the role to Administrator, but the
`on_update` hook that creates the default team checks `has_value_changed`
on a freshly-fetched doc (which is always False), so no team was ever
created. `_ensure_admin_has_default_team()` closes that gap explicitly.
A new "Setup E2E test data" CI step calls `bench execute
forms_pro.install.before_tests` before the server starts, so the
test user has a team available when fixtures call `get_user_teams`.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(form-generator): replace clear_cache() with targeted doctype cache clear
frappe.clear_cache() flushes Redis entirely, including session and CSRF
token data. In the E2E test suite this causes all POST API calls after
the first form creation to fail with CSRFTokenError, because the token
captured during global-setup is invalidated server-side.
Replace with frappe.clear_document_cache("DocType", name) which only
evicts the specific newly-created DocType from the document cache —
sufficient for the new schema to be reloaded on next access, and does
not touch sessions or CSRF tokens.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* ci(ui-tests): set ignore_csrf=1 for test site
Frappe's DocType.on_update() calls frappe.clear_cache() internally
whenever a DocType is saved. This flushes session/CSRF data from Redis,
invalidating the CSRF token captured in Playwright's storageState.json.
All POST API calls after the first form creation then fail with
CSRFTokenError.
ignore_csrf=1 is the standard Frappe configuration for CI/test sites
and skips CSRF validation entirely, which is correct here.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* ci(ui-tests): mute emails so invite test does not need an email account
Sets mute_emails=1 in site config so Frappe returns a dummy outgoing
account instead of throwing OutgoingEmailError when no real account is
configured, allowing the team-invite E2E test to succeed.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* ci(ui-tests): run E2E tests as the shared Forms Pro test user
- Switch global-setup login from Administrator to FORMS_PRO_TEST_USER
(test_forms_pro_user@example.com / testforms123) so E2E tests run
under a least-privileged Forms Pro User, matching real-user conditions
- Fix create_test_user() to strip the System User role Frappe auto-assigns
on insert, leaving only the Forms Pro User role
- Set a known password via update_password() so Playwright can log in
- Add frontend/e2e/tsconfig.json with @types/node so process.env
references in global-setup.ts type-check correctly
- Install @types/node dev dependency
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
harshtandiya
added a commit
that referenced
this pull request
Apr 19, 2026
* fix: route handling for /form * feat: child table support v1 (#42) * chore: bump frappe-ui to `0.1.262` * feat: add 'Table' option to fieldtype selection - Updated form_field.json and form_field.py to include 'Table' in the list of selectable field types for enhanced functionality. - Adjusted the modified timestamp in form_field.json to reflect recent changes. * feat: implement Table field component and enhance field options handling - Added a new Table component for rendering tabular data input. - Updated RenderField and FieldRenderer components to support dynamic options for Select and Link fields. - Introduced useFieldOptions composable for improved option management. - Enhanced form_fields utility to include Table as a selectable field type. - Updated auto-imports and TypeScript definitions to accommodate new functionality. * feat: enhance Table component fieldtype mapping - Integrated utility function to map fieldtype for Table component, ensuring proper handling of field types. - Defaulted unmapped fieldtypes to "Data" for improved consistency in table rendering. * fix: update TableField component to use the correct Table component - Replaced ListView with Table in the TableField definition to ensure proper rendering of tabular data. - Adjusted imports in form_fields utility to reflect the change in component usage. * refactor: remove onMounted hook from useFieldOptions - Eliminated the onMounted lifecycle hook from the useFieldOptions composable to streamline the loading process of options. - Adjusted imports to reflect the removal of the unused onMounted function. * fix: child table fields fetch * refactor: update TextEditor component bindings - Changed `:model-value` to `:content` for TextEditor components across multiple files to ensure consistency in prop usage. - Added an empty declaration block in auto-imports.d.ts for improved TypeScript support. * feat: team management pages (#46) * chore: bump lucide-icons * feat: restructure home dashboard and sidebar - Introduced a new home dashboard layout with a dedicated Dashboard.vue component. - Migrated existing dashboard functionality from the previous Dashboard.vue to the new home structure. - Added sidebar items management through a new sidebarItems.ts file for better organization. - Updated TeamSwitcher component to conditionally render based on the current team. - Removed unused code and components to streamline the application. * refactor: enhance layout structure in BaseLayout and Dashboard components - Wrapped the slot in BaseLayout with a div for improved layout consistency. - Removed unnecessary padding class from the Dashboard component for a cleaner design. - Ensured consistent formatting in watch options for better readability. * fix: manageform page layout * feat: team invitations via User Invitations * feat: implement team member removal and permission toggling - Added functionality to remove members from a team and toggle their edit permissions. - Introduced `remove_member_from_team` and `toggle_can_edit_team` API endpoints. - Created `RemoveMemberDialog` component for user confirmation before removal. - Updated `TeamMemberList` to include removal actions and permission toggling. - Enhanced `FPTeam` model to manage team member permissions effectively. * feat: add team details update functionality - Implemented a new API endpoint `save` to update team fields, allowing modifications to `team_name` and `logo`. - Created `ManageTeamHeader` component for editing team name and logo upload functionality. - Integrated the new save functionality into the team store for seamless updates. - Updated `ManageTeam` page to include the new header component for enhanced team management. * refactor: clean up imports in team store - Removed unused `call` import from the team store file to streamline dependencies. * chore: better code - Added checks to prevent duplicate team member invitations and ensure team owners cannot have their permissions toggled. - Updated `FPTeam` model to prevent removal of the team owner from the team. - Modified `RemoveMemberDialog` and `TeamMemberList` components to improve member removal functionality and user experience. * fix: tests * chore: better code * fix: team avatar uploader (#47) * feat: Image uploader component with crop * feat: integrate ImageUploader for team logo management - Replaced the existing logo upload button with an ImageUploader component for enhanced functionality. - Added error handling and upload progress display within the ImageUploader. - Updated the button label dynamically based on the upload state and existing logo presence. * feat: enhance TeamSwitcher with search functionality - Added a search input to the TeamSwitcher component, allowing users to filter teams by name. - Updated the team options computation to include search query handling. - Integrated a new search icon and improved layout for better user experience. - Refactored dropdown item templates to accommodate the search input and maintain consistent styling. * refactor: update CreateTeamDialog to use ImageUploader for logo uploads - Replaced FileUploader with ImageUploader component for improved logo management. - Updated type handling for uploaded files to align with new component. - Enhanced error handling and upload progress display in the logo upload section. - Adjusted button label to reflect upload status dynamically. * refactor: update BaseLayout styling and icon usage - Modified the layout classes for improved design consistency and responsiveness. - Changed the logout button icon from a string to a component reference for better integration with the icon library. * feat: add Breadcrumbs component to Dashboard - Integrated Breadcrumbs component into the Dashboard for improved navigation. - Defined breadcrumb items to enhance user context within the application. * feat: submission list pages (#59) * feat: add submissions page and update sidebar navigation - Introduced a new "Manage Form Submissions" page for viewing form submissions. - Updated the router to include a route for submissions. - Refactored sidebar items to include navigation to the new submissions page. - Enhanced the overview page with breadcrumb navigation for better user context. * feat: submissions page v1 * feat: add Drawer component for UI enhancement - Introduced a new Drawer component to provide a sliding panel interface. - Implemented customizable properties for size, position, title, and actions. - Added keyboard accessibility with Escape key support for closing the drawer. - Included transition effects for smooth opening and closing animations. * feat: extend Drawer component with additional size options - Added "xl" size option to the Drawer component for enhanced customization. - Updated size classes for both horizontal and vertical orientations to include new "xl" dimensions. * feat: add submission details view and field value component - Introduced SubmissionDetails component to display detailed information about a specific submission. - Created SubmissionFieldValue component to render individual field values with appropriate formatting based on field type. - Enhanced SubmissionList component to include a drawer for viewing submission details upon row click. - Updated API integration to fetch submission data securely based on user permissions. * feat: enhance SubmissionFieldValue component with dynamic class handling - Added computed property to determine class names based on field type, improving layout for Switch and Checkbox types. - Updated template structure to utilize dynamic classes for better styling and responsiveness. * fix: validate doctype against linked_doctype in get_submission_response A client could pass an arbitrary doctype to access submissions from a different form. Now we fetch both linked_team_id and linked_doctype from the Form doc and reject requests where the supplied doctype doesn't match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: throw when form not found in get_all_submissions If form_id is invalid, frappe.db.get_value returns None and calling has_permission(doc=None) has undefined behaviour. Explicitly throw DoesNotExistError before the permission check. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: show dash placeholder for null textarea value in SubmissionFieldValue Textarea was rendering blank when value is null/undefined, inconsistent with the default case which uses value ?? "–". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: remove unused isLoading ref in SubmissionList isLoading was declared and set but never bound in the template, so the loading state had no effect and was also never cleared on fetch error. Removed it entirely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: show loading state in drawer when formData not yet available If a row is clicked before formData loads, the drawer was empty. Now shows a Loading... placeholder until linked_doctype is available. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: add aria-labelledby to Drawer dialog element Adds id="drawer-title" to the title heading and wires aria-labelledby on the dialog div so assistive tech can announce the drawer title. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: form sharing methods (#58) * fix: form sharing methods - Introduced `add_form_access` and `set_form_permission` functions to manage user permissions on forms. - Updated the frontend to utilize the new API endpoints for adding access and setting permissions. - Enhanced permission validation to ensure only authorized users can share forms. * fix: validate permission_to allowlist and add docstrings to sharing API - Validate `permission_to` against an explicit allowlist in `set_form_permission` to prevent unexpected kwargs from being forwarded to `add_docshare` - Use `int(bool(value))` for safe coercion of the permission value - Expand docstrings on `add_form_access` and `set_form_permission` with full Args/Raises sections and inline comments Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: hygine (#60) * chore: update gitignore and pre-commit config for current project structure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: replace license.txt with LICENSE Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: add community health files and GitHub templates Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: update README with contributing and security references Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: update Python version references to 3.14 across all configs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: set ruff target-version to py313 (py314 not yet supported) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor: move vulnerable dependency check to a separate workflow The vulnerable dependency check has been extracted from the linter workflow into its own dedicated workflow file. This change improves organization and allows for more focused execution of dependency checks during pull requests and manual triggers. * chore: update dependabot configuration to use npm instead of yarn * chore(deps): bump actions/cache from 4 to 5 (#61) Bumps [actions/cache](https://github.com/actions/cache) from 4 to 5. - [Release notes](https://github.com/actions/cache/releases) - [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md) - [Commits](actions/cache@v4...v5) --- updated-dependencies: - dependency-name: actions/cache dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump actions/setup-node from 3 to 6 (#62) Bumps [actions/setup-node](https://github.com/actions/setup-node) from 3 to 6. - [Release notes](https://github.com/actions/setup-node/releases) - [Commits](actions/setup-node@v3...v6) --- updated-dependencies: - dependency-name: actions/setup-node dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump actions/setup-python from 4 to 6 (#63) Bumps [actions/setup-python](https://github.com/actions/setup-python) from 4 to 6. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v4...v6) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump pre-commit/action from 3.0.0 to 3.0.1 (#65) Bumps [pre-commit/action](https://github.com/pre-commit/action) from 3.0.0 to 3.0.1. - [Release notes](https://github.com/pre-commit/action/releases) - [Commits](pre-commit/action@v3.0.0...v3.0.1) --- updated-dependencies: - dependency-name: pre-commit/action dependency-version: 3.0.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump actions/checkout from 3 to 6 (#66) Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 6. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3...v6) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump jsdom from 25.0.1 to 29.0.2 in /frontend (#67) Bumps [jsdom](https://github.com/jsdom/jsdom) from 25.0.1 to 29.0.2. - [Release notes](https://github.com/jsdom/jsdom/releases) - [Commits](jsdom/jsdom@v25.0.1...v29.0.2) --- updated-dependencies: - dependency-name: jsdom dependency-version: 29.0.2 dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump vue from 3.5.21 to 3.5.32 in /frontend (#68) Bumps [vue](https://github.com/vuejs/core) from 3.5.21 to 3.5.32. - [Release notes](https://github.com/vuejs/core/releases) - [Changelog](https://github.com/vuejs/core/blob/main/CHANGELOG.md) - [Commits](vuejs/core@v3.5.21...v3.5.32) --- updated-dependencies: - dependency-name: vue dependency-version: 3.5.32 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump zod from 4.1.12 to 4.3.6 in /frontend (#69) Bumps [zod](https://github.com/colinhacks/zod) from 4.1.12 to 4.3.6. - [Release notes](https://github.com/colinhacks/zod/releases) - [Commits](colinhacks/zod@v4.1.12...v4.3.6) --- updated-dependencies: - dependency-name: zod dependency-version: 4.3.6 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump socket.io-client from 4.8.1 to 4.8.3 in /frontend (#70) Bumps [socket.io-client](https://github.com/socketio/socket.io) from 4.8.1 to 4.8.3. - [Release notes](https://github.com/socketio/socket.io/releases) - [Changelog](https://github.com/socketio/socket.io/blob/main/CHANGELOG.md) - [Commits](https://github.com/socketio/socket.io/compare/socket.io-client@4.8.1...socket.io-client@4.8.3) --- updated-dependencies: - dependency-name: socket.io-client dependency-version: 4.8.3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump postcss from 8.5.6 to 8.5.8 in /frontend (#71) Bumps [postcss](https://github.com/postcss/postcss) from 8.5.6 to 8.5.8. - [Release notes](https://github.com/postcss/postcss/releases) - [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md) - [Commits](postcss/postcss@8.5.6...8.5.8) --- updated-dependencies: - dependency-name: postcss dependency-version: 8.5.8 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): update frappe-ui to 0.1.272 * chore(deps): update vite to version 5.4.21 and adjust auto-imports and Vite config * refactor(tests): streamline email handling in TestTeamInvitations Removed the use of patching for the sendmail function and replaced it with a flag to mute emails during tests. This change simplifies the setup and teardown process while ensuring that email notifications are suppressed during test execution. Additionally, cleaned up the tearDown method to delete the Email Queue after tests. * refactor(tests): enhance email handling in TestTeamInvitations Reintroduced patching for the sendmail function in the TestTeamInvitations class to ensure email notifications are suppressed during tests. Updated the setUp and tearDown methods to manage the patching process effectively, ensuring compatibility with different Frappe versions. Removed the deletion of the Email Queue in tearDown as it is no longer necessary. * feat: test framework with test factory (#72) * feat(tests): add UserFactory and FPTeamFactory via frappe_factory_bot Introduces a factories package under forms_pro/tests/factories/ with: - UserFactory: generates random User docs; with_forms_pro_role trait includes the role in the initial doc dict so on_update fires once with the role already set - FPTeamFactory: generates FP Team docs with a random team name default Both factories implement __del_override__ as a safety-net cleanup guard. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(tests): use UserFactory in test_roles Replaces manual frappe.get_doc + Faker boilerplate with UserFactory.create(). Role assignment is kept as a post-insert step since the test specifically exercises the on_update hook that fires when a role is added after creation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(tests): use factories in TestTeamInvitations Replaces _create_user() and _create_team() helpers and the Faker/string tracking lists with UserFactory and FPTeamFactory. Key changes: - _user(*traits) and _team(owner) thin wrappers call the factories and track Document objects instead of email/name strings - Helper signatures updated to accept Document args throughout - UserFactory.build().email used where only a random email is needed (no DB insert required) - All 11 tests pass unchanged in behaviour Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(hooks): add required_apps for frappe_factory_bot Introduces a new required_apps list in hooks.py to include the frappe_factory_bot repository, enabling its integration into the application. This change enhances the app's capabilities by allowing the use of factory methods for testing and development. * ci: fetch frappe_factory_bot before installing forms_pro bench get-app with a local path skips required_apps resolution, so frappe_factory_bot was never fetched. bench install-app then failed with ModuleNotFoundError when Frappe tried to satisfy the required_apps entry. Explicitly get the app before installing forms_pro. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: update README with local test instructions and installation steps for frappe_factory_bot Added a new section to the README detailing how to run tests locally using frappe_factory_bot. Included installation instructions and commands for running all tests as well as specific test modules. * chore: remove del_override * refactor: tests * chore: refactor test_fp_team --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(deps): update devDependencies for Vite and Vitest (#73) Updated the versions of @vitejs/plugin-vue, @vitest/coverage-v8, vite, and vitest in package.json to their latest compatible releases. Removed deprecated dependencies from yarn.lock and updated @bcoe/v8-coverage to version 1.0.2. * feat(e2e): Playwright E2E test suite (#57) (#79) * feat(e2e): Phase 1 — Playwright foundation Install @playwright/test, add test:e2e scripts, create playwright.config.ts with Chromium project, global-setup for Frappe session auth, and a smoke spec that asserts an authenticated user can reach the forms dashboard. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(e2e): Phase 2 — data-testid attributes, POMs, and test data fixture Add data-testid to 7 Vue components (form-card, btn-new-form, field-type-*, btn-save-form, btn-publish, btn-submit-form, submission-success, btn-send-invite, input-invite-email). Restructure InviteMemberDialog to use #actions slot so the send button is directly targetable. Create Page Object Models (dashboard, form-builder, submission, team) and a Playwright fixture with API helpers for form create/publish/teardown. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(e2e): Phase 2+3 — semantic selectors and form-creation spec (5 tests) Remove all data-testid attributes from Vue components (reverted to original). Rewrite POMs with semantic selectors: getByRole/getByLabel/getByText, scoped to pre-existing data-form-builder-component attributes. Add form-creation.spec.ts covering dashboard listing, builder load, field canvas, publish, and unpublish. Tests avoid triggering Frappe validation by never saving a field with an empty label; publish/unpublish uses setValue({is_published}) which is always valid. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(e2e): Phase 3+4 — remaining specs, flake fixes, and CI workflow Specs: - form-submission.spec.ts — guest context submission + success message - submission-view.spec.ts — empty state and post-submission row visibility - team-invite.spec.ts — invite dialog open/fill/send flow Fixture: - submitForm fixture for creating a guest submission via API Reliability fixes: - smoke.spec.ts: reload on Frappe 500 under parallel startup load - TeamPage.goto(): waitForLoadState("networkidle") so Vue API calls complete before assertions run - TeamPage.openInviteDialog(): wait for email input to be visible (not just the dialog shell) before returning - playwright.config.ts: retries=1 everywhere; github reporter added CI: - .github/workflows/ui-tests.yml: full Playwright E2E workflow running against a real Frappe bench on every PR and push to develop Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ci(ui-tests): log bench serve output on startup timeout Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ci(ui-tests): fix server startup — switch to bench start, add hostname, fix action versions - Replace bench serve (silent crash) with bench start + Procfile (disable watch/schedule) - Add forms.test to /etc/hosts and set host_name config; rename site test_site → forms.test - Swap npx wait-on for a curl loop (no install overhead, same pattern as buzz CI) - Fix non-existent action versions: checkout@v6→@v4, setup-python@v6→@v5, setup-node@v6→@v4 - Add timeout-minutes: 60, fix concurrency group for push events, add workflow_dispatch - Add Playwright browser cache step and bench logs dump on failure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ci(ui-tests): setup E2E test data — give Admin Forms Pro role and default team `before_tests()` already added the role to Administrator, but the `on_update` hook that creates the default team checks `has_value_changed` on a freshly-fetched doc (which is always False), so no team was ever created. `_ensure_admin_has_default_team()` closes that gap explicitly. A new "Setup E2E test data" CI step calls `bench execute forms_pro.install.before_tests` before the server starts, so the test user has a team available when fixtures call `get_user_teams`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(form-generator): replace clear_cache() with targeted doctype cache clear frappe.clear_cache() flushes Redis entirely, including session and CSRF token data. In the E2E test suite this causes all POST API calls after the first form creation to fail with CSRFTokenError, because the token captured during global-setup is invalidated server-side. Replace with frappe.clear_document_cache("DocType", name) which only evicts the specific newly-created DocType from the document cache — sufficient for the new schema to be reloaded on next access, and does not touch sessions or CSRF tokens. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ci(ui-tests): set ignore_csrf=1 for test site Frappe's DocType.on_update() calls frappe.clear_cache() internally whenever a DocType is saved. This flushes session/CSRF data from Redis, invalidating the CSRF token captured in Playwright's storageState.json. All POST API calls after the first form creation then fail with CSRFTokenError. ignore_csrf=1 is the standard Frappe configuration for CI/test sites and skips CSRF validation entirely, which is correct here. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ci(ui-tests): mute emails so invite test does not need an email account Sets mute_emails=1 in site config so Frappe returns a dummy outgoing account instead of throwing OutgoingEmailError when no real account is configured, allowing the team-invite E2E test to succeed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ci(ui-tests): run E2E tests as the shared Forms Pro test user - Switch global-setup login from Administrator to FORMS_PRO_TEST_USER (test_forms_pro_user@example.com / testforms123) so E2E tests run under a least-privileged Forms Pro User, matching real-user conditions - Fix create_test_user() to strip the System User role Frappe auto-assigns on insert, leaving only the Forms Pro User role - Set a known password via update_password() so Playwright can log in - Add frontend/e2e/tsconfig.json with @types/node so process.env references in global-setup.ts type-check correctly - Install @types/node dev dependency Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: update .gitignore to include Playwright report and skills-lock.json * feat(multiselect): add Multiselect field with registry-driven builder extras (#85) * feat(form-field): add Multiselect fieldtype to doctype and backend mapping * fix(submission): serialize list values to JSON string before DocType insert * feat(multiselect): wire up Multiselect field across the frontend * docs: add add-field skill and register it in CLAUDE.md * chore: move skills to .claude/skills, gitignore .agents, document npx skills workflow * docs: add userinterface-wiki skill reference to CLAUDE.md * feat(fieldTypes): add optional builderExtras component slot to FieldTypeDefinition * refactor(fields): move Multiselect into multiselect/ subfolder, add MultiselectBuilderExtras * feat(fieldTypes): update Multiselect import path and register MultiselectBuilderExtras * refactor(builder): replace hardcoded Multiselect extras with registry-driven builderExtras * feat(multiselect): enhance option addition with error handling and unique validation * feat(multiselect): change layout to description-first (label → desc → checkboxes) * refactor(builder): wrap template in single root div so builderExtras stacks below field * test(e2e): add Playwright spec for Multiselect field builder and submission flow * test(e2e): wait for canvas render and set label before adding options * test(e2e): fix strict mode violation on No options defined locator * test(e2e): wait for Add Option button to confirm field rendered on canvas * test(e2e): ensure Add Option button is visible before adding options * fix(e2e): ensure form is saved before publishing to enable Publish button * fix(e2e): set form title and wait on Save button state Multiselect spec clicked Save while form title was blank — Frappe rejected the save with MandatoryError, Save button never hid, and the publish helper timed out. Fill a title before save so the request succeeds, and in the helper wait for the Save button to disappear (same render cycle as Publish appearing) instead of polling for Publish. * fix(ci): resolve all CI failures on backport branch - Node 18 → 20 in ci.yml and typecheck.yml (@vitejs/plugin-vue@6 requires ≥20.19) - nosemgrep comment on allow_guest=True in get_doctype_fields (intentional for public form rendering) - Migrate form_fields.ts to re-export FIELD_TYPE_DEFINITIONS, fixing Multiselect missing from sidebar - Add Multiselect to FormFieldTypes enum; fix TS2367 comparison in SubmissionFieldValue.vue * fix(tests): use FrappeTestCase for version-15 compatibility IntegrationTestCase was introduced in Frappe v16; version-15 exposes FrappeTestCase from frappe.tests.utils. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
9 tasks
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
1 participant
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.
Summary
Closes #57. Implements the full Playwright E2E test suite for the four critical user flows, plus the CI workflow that gates merges on test passage.
UI Tests / Playwright E2EWhat's tested
smoke.spec.tsform-creation.spec.tsform-submission.spec.tssubmission-view.spec.tsteam-invite.spec.tsKey design decisions
getByRole,getByLabel,getByTextusing pre-existingdata-form-builder-componentattributes for scoping; no newdata-testidpollution in production componentsbrowser.newContext()(no storageState) simulates a real anonymous user for public form submissionTeamPage.goto()waits fornetworkidleso Vue API calls finish;openInviteDialog()waits for the email input (not just the dialog shell); smoke test reloads once on Frappe 500 under parallel startup loadTest plan
yarn test:e2e— 13/13 pass locally with 5 parallel workers🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores
Bug Fixes