Add vitest and fix visual form builder bugs#7
Merged
matteius merged 5 commits intoJul 24, 2026
Conversation
addFieldAtPosition inserted at the raw position SortableJS reported via evt.newIndex, then reused that same raw position to look up the field for editField(). On an empty canvas, SortableJS's placeholder element isn't excluded by the `filter` option the way `.canvas-drop-zone` is, so the very first field dropped reports newIndex 1 instead of 0. Array.splice silently clamps that out-of-range index to fields.length (so the field itself lands in the right place) — but editField then looks up the *unclamped* index, which points past the end of the array, opening the property editor for the wrong field (or none at all). Clamp once with Math.min(position, this.fields.length) and reuse the clamped value for both the splice and the editField() call.
setupCanvas() had two independent code paths handling the same palette-drop
gesture: SortableJS's own onAdd callback (which reports the correct drop
position via evt.newIndex) and a hand-rolled canvas.addEventListener('drop',
...) handler that computed its own position and called addFieldAtPosition
again. Since this.draggingFieldType is still set at the point the manual
handler runs for an ordinary palette drop, both call sites fired for a
single drag gesture, silently inserting two fields instead of one. Each
insertion gets its own auto-generated field_name, so this wasn't visibly
wrong until two generated names collided — surfacing downstream as a
`UNIQUE constraint failed: form_definition_id, field_name` error from the
preview endpoint, far from the actual cause.
Delete the redundant position-computation and addFieldAtPosition call from
the manual drop handler; keep only its placeholder cleanup, which still
needs to run on every drop regardless of source.
buildConditionalLogicTab/buildValidationTab/buildDependenciesTab each ended with a literal <script> block, returned as part of the HTML string editField() assigns via `.innerHTML =`. Script tags inserted via innerHTML never execute (browser spec, not something specific to this code) — so none of this ever actually ran: the "Enable Conditional Logic" toggle didn't show/hide its own section, and none of the three tabs' interactive rule-builder lists (conditions/validation rules/dependencies) ever initialized. Add initializePropertyFormTabs(field), a real method wiring the same behavior — the propEnableConditional change listener, plus calling initializeConditionsList/initializeValidationRulesList/ initializeDependenciesList directly — called from editField() right after its innerHTML assignment. Delete the three dead <script> blocks.
There was a problem hiding this comment.
Pull request overview
Adds a JavaScript test harness (Vitest + jsdom) and fixes three regressions in the visual Form Builder’s drag/drop and property-editor initialization to improve correctness and support future JS refactoring.
Changes:
- Introduce Vitest tooling/configuration and a new
tests_js/suite for client-side builder code. - Fix duplicate field insertion on palette drop and an empty-canvas off-by-one when opening the new field’s editor.
- Replace inert
<script>-in-innerHTMLtab initialization with a realinitializePropertyFormTabs()method invoked fromeditField().
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.js | Configures Vitest to run browser-like tests via jsdom. |
| package.json | Adds dev-only JS test tooling scripts/dependencies. |
| package-lock.json | Locks the dependency tree for the new JS test tooling. |
| tests_js/setup.smoke.test.js | Smoke test to confirm Vitest + jsdom are wired correctly. |
| tests_js/form-builder/setupCanvas.test.js | Regression tests for canvas drop handling (no duplicate insertion; cleanup still occurs). |
| tests_js/form-builder/initializePropertyFormTabs.test.js | Tests property-editor tab initialization and conditional toggle wiring. |
| tests_js/form-builder/helpers/loadFormBuilderClass.js | Loads the real non-module form-builder.js class for tests by evaluating source. |
| tests_js/form-builder/addFieldAtPosition.test.js | Regression tests for clamped insertion index and correct editField() targeting. |
| django_forms_workflows/static/django_forms_workflows/js/form-builder.js | Fixes drop duplication, clamps insert index, and adds initializePropertyFormTabs(). |
| README.md | Documents how to run the new JavaScript test suite. |
| CONTRIBUTING.md | Adds contributor guidance for JS tests and JS coding style expectations. |
| .nvmrc | Pins a Node version for running the JS tooling. |
| .gitignore | Ignores node_modules/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
352
to
355
| - Optional: `django-auth-ldap` for LDAP/AD integration (`pip install django-forms-workflows[ldap]`) | ||
| - Optional: WeasyPrint for PDF export (`pip install django-forms-workflows[pdf]`) | ||
| - Optional: Node.js 20+ / npm, to run the JS test suite (`npm test`) covering the visual Form/Workflow Builder's client-side code | ||
|
|
Contributor
Author
There was a problem hiding this comment.
@matteius I've pushed up a commit to address these three issues. Let me know if that was not your preference and you would I prefer I rebase this into the first commit that adds the test runner. I wasn't sure if the plan was to squash merge, etc.
Contributor
There was a problem hiding this comment.
I am fine with the commit history representing reality. squash is fine, but since its your work I'll just merge as is.
Comment on lines
36
to
40
| 3. **Add tests** for new functionality | ||
| 4. **Update documentation** as needed | ||
| 5. **Ensure tests pass**: `pytest` | ||
| 5. **Ensure tests pass**: `pytest` (and `npm test` if you touched front-end js`) | ||
| 6. **Ensure code quality**: `black .`, `flake8`, `isort .` | ||
| 7. **Commit with clear messages** |
Comment on lines
+89
to
+93
| Only needed if you're changing the visual Form/Workflow Builder's client-side | ||
| JS (`django_forms_workflows/static/django_forms_workflows/js/`). Requires | ||
| Node.js 20+ / npm — an `.nvmrc` is committed at the repo root, so `nvm use` | ||
| picks the right version automatically if you have nvm installed. | ||
|
|
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
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.
Adds vitest in preparation for future js changes (splitting form-builder.js into modules).
Three independent bugs in form-builder.js, found via hands-on testing of the visual Form Builder. Landed as three separate commits.
Off-by-one on an empty canvas. addFieldAtPosition used SortableJS's raw evt.newIndex for both the insert and the follow-up editField() lookup. On an empty canvas, SortableJS counts a placeholder element and reports newIndex 1 instead of 0 — splice silently clamps this, but editField didn't, so it opened the wrong field. Fixed by clamping once and reusing that value for both calls.
Duplicate field insertion on palette drop. setupCanvas() had two handlers acting on one drop gesture — SortableJS's onAdd and a redundant manual drop listener — both calling addFieldAtPosition. Silently inserted two fields per drop; surfaced downstream as a UNIQUE constraint failed error when two generated names collided. Fixed by removing the redundant insertion logic.
Inert <script> tags in the property-editor tabs. Three tab-builder methods returned <script> blocks inside HTML assigned via innerHTML, which never execute (browser spec) — so the Conditional Logic toggle and all three tabs' rule-builder lists never actually initialized. Fixed with a real initializePropertyFormTabs() method called from editField().
Test plan
Added Vitest (none existed before). Each fix has its own test in tests_js/form-builder/, loading the real source via a small helper since the class isn't an ES module. All pass:
Test Files 4 passed (4)
Tests 8 passed (8)