feat: add Heading 1/2/3 field types#103
Merged
Merged
Conversation
… mapping Maps heading fieldtypes to Frappe HTML, generates h1/h2/h3 options content for the CustomField, and skips heading fields in server-side required validation.
Adds heading layout type, Heading component with h2/h3/h4 tag rendering, FieldRenderer branch for edit/view modes, isHeading util, and submission display handling.
- Unit tests for heading fields skipped in validation - Integration tests for get_options() and to_frappe_field - E2E tests for builder, public form rendering, and submission - Fix missing Heading imports in FieldRenderer and SubmissionFieldValue
📝 WalkthroughWalkthroughAdds three heading field types (Heading 1/2/3), maps them to Frappe HTML, renders them via a new Heading component in the builder and submissions UI, and treats these heading types as display-only so server-side validation skips required/conditional checks for them. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 3
🧹 Nitpick comments (9)
forms_pro/forms_pro/doctype/form_field/form_field.py (1)
95-95: Eager evaluation ofget_options()default.
dict.get("options", self.get_options())evaluatesself.get_options()on every call regardless of whether"options"is present inmapping. Minor; switch to a conditional if profiling flags this on form-generation hot paths.- "options": mapping.get("options", self.get_options()), + "options": mapping["options"] if "options" in mapping else self.get_options(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/forms_pro/doctype/form_field/form_field.py` at line 95, The current line building the dict uses mapping.get("options", self.get_options()) which eagerly calls self.get_options() even when "options" exists; change it to check for the key first (e.g. use mapping.get("options") and if that returns None/Falsey call self.get_options(), or use an if/else/ternary) so get_options() is only invoked when mapping lacks "options"; update the code around the "options": mapping.get(... ) entry in form_field.py inside the method that constructs the field dict (referencing mapping and self.get_options).forms_pro/api/submission.py (1)
11-11: Cross-module import of underscore-prefixed symbol breaks privacy convention.
_DISPLAY_ONLY_FIELDTYPESis conventionally module-private (per PEP 8), but it's imported here and informs_pro/tests/test_submission_validation.py. Either drop the leading underscore or relocate it to a shared module likeforms_pro/utils/constants.py(which already housesFORMS_PRO_SYSTEM_FIELDNAMES,UNSUPPORTED_FRAPPE_FIELDTYPES).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/api/submission.py` at line 11, The module-private constant _DISPLAY_ONLY_FIELDTYPES is being imported across modules; move it into the shared constants module (add DISPLAY_ONLY_FIELDTYPES to the existing constants file), remove the leading underscore (public name), update all imports (replace _DISPLAY_ONLY_FIELDTYPES imports in submission.py and tests with DISPLAY_ONLY_FIELDTYPES from the shared constants), and optionally keep a one-line import alias in the original form_field module (e.g., _DISPLAY_ONLY_FIELDTYPES = DISPLAY_ONLY_FIELDTYPES) for backward compatibility before removing it.frontend/src/components/fields/Heading.vue (2)
17-23:HeadingFieldtypeused before itstypedeclaration — works, but reorder for readability.
headingClasseson line 17 references the type aliasHeadingFieldtypedeclared on line 23. TypeScript hoists type aliases so this compiles fine, but it reads awkwardly and trips up casual readers. Move thetype HeadingFieldtype = ...(andHeadingTag) aboveheadingClasses.♻️ Proposed reorder
+type HeadingFieldtype = Fieldtype.HEADING_1 | Fieldtype.HEADING_2 | Fieldtype.HEADING_3; +type HeadingTag = "h2" | "h3" | "h4"; + const headingClasses: Record<HeadingFieldtype, string> = { [Fieldtype.HEADING_1]: "text-xl font-bold", [Fieldtype.HEADING_2]: "text-lg font-semibold", [Fieldtype.HEADING_3]: "text-base font-semibold", }; - -type HeadingFieldtype = Fieldtype.HEADING_1 | Fieldtype.HEADING_2 | Fieldtype.HEADING_3; -type HeadingTag = "h2" | "h3" | "h4";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/fields/Heading.vue` around lines 17 - 23, Move the type aliases so they appear before the usage: place the declaration type HeadingFieldtype = Fieldtype.HEADING_1 | Fieldtype.HEADING_2 | Fieldtype.HEADING_3 (and any related alias like HeadingTag) above the headingClasses constant; update the file so headingClasses: Record<HeadingFieldtype, string> references the already-declared HeadingFieldtype to improve readability.
26-30: Document the deliberate offset between heading level and HTML tag.
HEADING_1 → h2,HEADING_2 → h3,HEADING_3 → h4is non-obvious and looks like a bug at first glance. I assume the rationale is that the page already owns theh1(form title), so the form body must start ath2to keep the document outline valid for a11y/SEO. A short comment here will save future readers (and code reviewers) from "fixing" it.📝 Proposed comment
+// Intentional offset: the form page already renders an h1 (form title), +// so user-authored "Heading 1" maps to <h2> to preserve a valid heading outline. const headingTypeToTag: Record<HeadingFieldtype, HeadingTag> = { [Fieldtype.HEADING_1]: "h2", [Fieldtype.HEADING_2]: "h3", [Fieldtype.HEADING_3]: "h4", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/fields/Heading.vue` around lines 26 - 30, The mapping in headingTypeToTag (mapping HeadingFieldtype → HeadingTag using Fieldtype.HEADING_1→"h2", HEADING_2→"h3", HEADING_3→"h4") is intentionally offset to reserve the document's h1 for the page/form title; add a short inline comment above the headingTypeToTag declaration explaining this rationale (mentioning accessibility/SEO and that form headings start at h2 to preserve document outline) so future readers won't treat the mapping as a bug.forms_pro/tests/test_form_field.py (2)
62-66: Consider asserting closing tag too, to lock in well-formed HTML.
assertIn("<h1>", ...)andassertIn("My Title", ...)would pass on<h1>My Title(no closing tag), which would render broken HTML in Frappe. A singleassertEqual(result["options"], "<h1>My Title</h1>")is stricter and matches the existing style inTestFormFieldGetOptions.♻️ Proposed tightening
def test_heading_options_contains_html_tag(self): field = _build_field("Heading 1", label="My Title", fieldname="my_title") - result = field.to_frappe_field - self.assertIn("<h1>", result["options"]) - self.assertIn("My Title", result["options"]) + self.assertEqual(field.to_frappe_field["options"], "<h1>My Title</h1>")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/tests/test_form_field.py` around lines 62 - 66, The test test_heading_options_contains_html_tag currently only asserts the opening tag and text, so change it to assert the full well-formed HTML: get the field via _build_field(...), compute result = field.to_frappe_field, then assert that result["options"] equals the exact string "<h1>My Title</h1>" (or at minimum assertIn("</h1>", result["options"])); update the assertion in test_heading_options_contains_html_tag to use assertEqual(result["options"], "<h1>My Title</h1>") to match the style used in TestFormFieldGetOptions.
72-76: Also assert the generatedoptionscarry the right tag inFORM_TO_FRAPPE_FIELDTYPE.This currently checks the
fieldtypemapping but not whether each heading level's mapping yields the expected HTML wrapper (h1/h2/h3). That assertion is what would catch an off-by-one in the mapping table, which is the most likely place a bug would be introduced. Worth tightening since the loop already iterates the three levels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/tests/test_form_field.py` around lines 72 - 76, The test test_form_to_frappe_fieldtype_has_all_heading_levels currently only checks the mapped "fieldtype" but not the HTML wrapper; update it to also verify FORM_TO_FRAPPE_FIELDTYPE[fieldtype]["options"] contains the correct heading tags for each level (e.g., for "Heading 1" assert options contains "<h1>" and "</h1>", for "Heading 2" assert "<h2>" and "</h2>", etc.). Compute the heading level from the fieldtype string (or loop index) and assert both opening and closing tags are present in the "options" value to catch off-by-one mapping errors.frontend/src/config/fieldTypes.ts (2)
280-306: Consider deduplicating the three near-identical heading entries.The three
HEADING_*definitions differ only inname. A small loop or.mapover the heading types keeps the registry shorter and removes the risk of one entry drifting from the others (e.g., a future change tofrappeFieldtypethat misses one). Optional — current style is consistent with the rest of the file.♻️ Possible refactor
- { - name: Fieldtype.HEADING_1, - component: Heading, - props: {}, - layout: "heading", - frappeFieldtype: "HTML", - isBoolean: false, - isDate: false, - }, - { - name: Fieldtype.HEADING_2, - ... - }, - { - name: Fieldtype.HEADING_3, - ... - }, + ...[Fieldtype.HEADING_1, Fieldtype.HEADING_2, Fieldtype.HEADING_3].map( + (name): FieldTypeDefinition => ({ + name, + component: Heading, + props: {}, + layout: "heading", + frappeFieldtype: "HTML", + isBoolean: false, + isDate: false, + }) + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/config/fieldTypes.ts` around lines 280 - 306, The three registry entries for Fieldtype.HEADING_1, Fieldtype.HEADING_2 and Fieldtype.HEADING_3 are duplicated; replace them with a generated set (e.g., map over [Fieldtype.HEADING_1, Fieldtype.HEADING_2, Fieldtype.HEADING_3]) that produces objects with component: Heading, props: {}, layout: "heading", frappeFieldtype: "HTML", isBoolean: false, isDate: false; ensure the resulting entries keep the same shape and ordering so any future change (e.g., to frappeFieldtype or layout) is made in one place.
46-59: Update theFieldLayoutJSDoc to document the new"heading"value.The JSDoc above the union enumerates
default,inline,description-first, andcustom, but"heading"was added to the union without a corresponding bullet. Keeping the doc in sync helps future contributors understand the intent (label-only, no input widget).📝 Proposed doc update
/** * Controls how FieldRenderer positions the label relative to the input widget. * * - "default" label on top, input below, description at the bottom * - "inline" input first, label to the right (Switch, Checkbox) * - "description-first" label on top, description below label, input at the bottom (Text Editor) * - "custom" the component handles its own full layout (Table) + * - "heading" field label is rendered as an h2/h3/h4 tag; no input widget, no description */ export type FieldLayout =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/config/fieldTypes.ts` around lines 46 - 59, Update the JSDoc for the FieldLayout union to include the new "heading" option: add a bullet describing that "heading" is a label-only layout (no input widget) so future readers understand its intent; modify the comment block above the FieldLayout type (where other bullets like "default", "inline", "description-first", and "custom" are listed) to add the "heading" line and a brief description.frontend/e2e/specs/heading-field.spec.ts (1)
80-85: Brittle coupling between E2E expectation andHeading.vuetag mapping.The assertion that "Heading 2" renders as
h3mirrors the offset mapping inHeading.vue(HEADING_2 → h3). The inline comment helps, but if anyone "fixes" the mapping to be 1:1 (Heading 2 → h2), this test will fail without making the bug obvious. Consider asserting against the heading text viagetByRole("heading", { name: "Your Details" })so the test stays valid regardless of whichh*level is chosen, and additionally asserting that no<input>exists for the heading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/specs/heading-field.spec.ts` around lines 80 - 85, The test is brittle because it asserts an h3 element (guestPage.locator("h3", { hasText: "Your Details" })) tied to Heading.vue's HEADING_2 → h3 mapping; change the assertion to use an accessible role-based query like getByRole("heading", { name: "Your Details" }) so the test passes regardless of which h* level Heading.vue maps to, and add an assertion that no <input> exists for the heading (e.g., ensure there's no input associated with that heading) to preserve the intent; update references in the test file where guestPage.locator("h3"... ) is used and mention Heading.vue / HEADING_2 in the commit message for context.
🤖 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/forms_pro/doctype/form_field/form_field.py`:
- Around line 100-110: get_options currently returns raw HTML for heading
fieldtypes and interpolates self.label unsafely and also claims to return str
while sometimes returning None; fix get_options in form_field.FormField: when
self.fieldtype in _DISPLAY_ONLY_FIELDTYPES, wrap the label with
escape_html(self.label or '') before interpolating into the heading tag (use the
existing escape_html util) to prevent stored XSS, and change the function's
return annotation from -> str to -> str | None (or coerce non-heading
self.options to ''/str) so the signature matches possible None values from
self.options.
In `@forms_pro/tests/test_form_field.py`:
- Around line 43-45: The get_options return type and behavior are inconsistent
with the test: update the Field.get_options method to return an optional string
(e.g., change signature to return str | None and/or Optional[str]) and normalize
empty-string options to None (i.e., if self.options == "" return None, otherwise
return self.options) so the test_non_heading_with_no_options_returns_none
passes; modify the method implementation referenced as get_options and ensure it
checks the field attribute options before returning, leaving the test name
test_non_heading_with_no_options_returns_none unchanged.
In `@frontend/src/components/fields/Heading.vue`:
- Around line 42-53: The edit-mode input in the Heading component lacks an
accessible name; update the <input> inside the inEditMode block to provide one
(e.g., add aria-label="Heading text" or wire it to a visually-hidden <label>
tied to the input) so screen readers get context; modify the input that uses
:value="field.label" and `@input`="emit('update:label', ...)" to include the
aria-label (or an id and corresponding <label for="...">) and ensure any
localization key is used if needed.
---
Nitpick comments:
In `@forms_pro/api/submission.py`:
- Line 11: The module-private constant _DISPLAY_ONLY_FIELDTYPES is being
imported across modules; move it into the shared constants module (add
DISPLAY_ONLY_FIELDTYPES to the existing constants file), remove the leading
underscore (public name), update all imports (replace _DISPLAY_ONLY_FIELDTYPES
imports in submission.py and tests with DISPLAY_ONLY_FIELDTYPES from the shared
constants), and optionally keep a one-line import alias in the original
form_field module (e.g., _DISPLAY_ONLY_FIELDTYPES = DISPLAY_ONLY_FIELDTYPES) for
backward compatibility before removing it.
In `@forms_pro/forms_pro/doctype/form_field/form_field.py`:
- Line 95: The current line building the dict uses mapping.get("options",
self.get_options()) which eagerly calls self.get_options() even when "options"
exists; change it to check for the key first (e.g. use mapping.get("options")
and if that returns None/Falsey call self.get_options(), or use an
if/else/ternary) so get_options() is only invoked when mapping lacks "options";
update the code around the "options": mapping.get(... ) entry in form_field.py
inside the method that constructs the field dict (referencing mapping and
self.get_options).
In `@forms_pro/tests/test_form_field.py`:
- Around line 62-66: The test test_heading_options_contains_html_tag currently
only asserts the opening tag and text, so change it to assert the full
well-formed HTML: get the field via _build_field(...), compute result =
field.to_frappe_field, then assert that result["options"] equals the exact
string "<h1>My Title</h1>" (or at minimum assertIn("</h1>", result["options"]));
update the assertion in test_heading_options_contains_html_tag to use
assertEqual(result["options"], "<h1>My Title</h1>") to match the style used in
TestFormFieldGetOptions.
- Around line 72-76: The test
test_form_to_frappe_fieldtype_has_all_heading_levels currently only checks the
mapped "fieldtype" but not the HTML wrapper; update it to also verify
FORM_TO_FRAPPE_FIELDTYPE[fieldtype]["options"] contains the correct heading tags
for each level (e.g., for "Heading 1" assert options contains "<h1>" and
"</h1>", for "Heading 2" assert "<h2>" and "</h2>", etc.). Compute the heading
level from the fieldtype string (or loop index) and assert both opening and
closing tags are present in the "options" value to catch off-by-one mapping
errors.
In `@frontend/e2e/specs/heading-field.spec.ts`:
- Around line 80-85: The test is brittle because it asserts an h3 element
(guestPage.locator("h3", { hasText: "Your Details" })) tied to Heading.vue's
HEADING_2 → h3 mapping; change the assertion to use an accessible role-based
query like getByRole("heading", { name: "Your Details" }) so the test passes
regardless of which h* level Heading.vue maps to, and add an assertion that no
<input> exists for the heading (e.g., ensure there's no input associated with
that heading) to preserve the intent; update references in the test file where
guestPage.locator("h3"... ) is used and mention Heading.vue / HEADING_2 in the
commit message for context.
In `@frontend/src/components/fields/Heading.vue`:
- Around line 17-23: Move the type aliases so they appear before the usage:
place the declaration type HeadingFieldtype = Fieldtype.HEADING_1 |
Fieldtype.HEADING_2 | Fieldtype.HEADING_3 (and any related alias like
HeadingTag) above the headingClasses constant; update the file so
headingClasses: Record<HeadingFieldtype, string> references the already-declared
HeadingFieldtype to improve readability.
- Around line 26-30: The mapping in headingTypeToTag (mapping HeadingFieldtype →
HeadingTag using Fieldtype.HEADING_1→"h2", HEADING_2→"h3", HEADING_3→"h4") is
intentionally offset to reserve the document's h1 for the page/form title; add a
short inline comment above the headingTypeToTag declaration explaining this
rationale (mentioning accessibility/SEO and that form headings start at h2 to
preserve document outline) so future readers won't treat the mapping as a bug.
In `@frontend/src/config/fieldTypes.ts`:
- Around line 280-306: The three registry entries for Fieldtype.HEADING_1,
Fieldtype.HEADING_2 and Fieldtype.HEADING_3 are duplicated; replace them with a
generated set (e.g., map over [Fieldtype.HEADING_1, Fieldtype.HEADING_2,
Fieldtype.HEADING_3]) that produces objects with component: Heading, props: {},
layout: "heading", frappeFieldtype: "HTML", isBoolean: false, isDate: false;
ensure the resulting entries keep the same shape and ordering so any future
change (e.g., to frappeFieldtype or layout) is made in one place.
- Around line 46-59: Update the JSDoc for the FieldLayout union to include the
new "heading" option: add a bullet describing that "heading" is a label-only
layout (no input widget) so future readers understand its intent; modify the
comment block above the FieldLayout type (where other bullets like "default",
"inline", "description-first", and "custom" are listed) to add the "heading"
line and a brief description.
🪄 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: a03306ac-7fcd-4b5d-8a2c-ea85d0c4da5f
📒 Files selected for processing (12)
forms_pro/api/submission.pyforms_pro/forms_pro/doctype/form_field/form_field.jsonforms_pro/forms_pro/doctype/form_field/form_field.pyforms_pro/tests/test_form_field.pyforms_pro/tests/test_submission_validation.pyfrontend/e2e/specs/heading-field.spec.tsfrontend/src/components/builder/FieldRenderer.vuefrontend/src/components/fields/Heading.vuefrontend/src/components/form/submissions/SubmissionFieldValue.vuefrontend/src/config/fieldTypes.tsfrontend/src/types/FormsPro/form_field.types.tsfrontend/src/utils/form_fields.ts
Comment on lines
+43
to
+45
| def test_non_heading_with_no_options_returns_none(self): | ||
| field = _build_field("Data", label="Name") | ||
| self.assertIsNone(field.get_options()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the options docfield definition on Form Field to confirm None vs "" semantics.
fd -t f 'form_field.json' forms_pro
rg -n -C2 '"fieldname"\s*:\s*"options"' forms_pro/forms_pro/doctype/form_field/form_field.json
# And confirm the runtime behavior of frappe.get_doc + missing options.
rg -nP --type=py -C3 '\bdef\s+get_options\s*\(' forms_pro/forms_pro/doctype/form_field/form_field.pyRepository: BuildWithHussain/forms_pro
Length of output: 453
🏁 Script executed:
#!/bin/bash
# Get full form_field.json to check default value for options field
cat forms_pro/forms_pro/doctype/form_field/form_field.json | jq '.[] | select(.fieldname == "options")' 2>/dev/null || rg -A5 '"fieldname"\s*:\s*"options"' forms_pro/forms_pro/doctype/form_field/form_field.jsonRepository: BuildWithHussain/forms_pro
Length of output: 189
🏁 Script executed:
#!/bin/bash
# Get the complete get_options() method implementation
rg -A20 'def get_options\(' forms_pro/forms_pro/doctype/form_field/form_field.pyRepository: BuildWithHussain/forms_pro
Length of output: 441
🏁 Script executed:
#!/bin/bash
# Check the test file to see the full test and helper function
cat forms_pro/forms_pro/tests/test_form_field.pyRepository: BuildWithHussain/forms_pro
Length of output: 147
🏁 Script executed:
#!/bin/bash
# Find the test file
fd -type f 'test_form_field.py' forms_proRepository: BuildWithHussain/forms_pro
Length of output: 241
🏁 Script executed:
#!/bin/bash
# Also check what test files exist
find forms_pro -name 'test_*.py' -o -name '*_test.py' | head -20Repository: BuildWithHussain/forms_pro
Length of output: 420
🏁 Script executed:
#!/bin/bash
# Check the doctype python file to understand the Model class
cat -n forms_pro/forms_pro/doctype/form_field/form_field.py | head -50Repository: BuildWithHussain/forms_pro
Length of output: 2161
🏁 Script executed:
#!/bin/bash
# Now get the test file
cat -n forms_pro/tests/test_form_field.pyRepository: BuildWithHussain/forms_pro
Length of output: 3839
🏁 Script executed:
#!/bin/bash
# Check if options field has a default value in form_field.json - get the full field definition
rg -B2 -A8 '"fieldname"\s*:\s*"options"' forms_pro/forms_pro/doctype/form_field/form_field.jsonRepository: BuildWithHussain/forms_pro
Length of output: 259
🏁 Script executed:
#!/bin/bash
# Check the _build_field helper function and how it's used in tests
rg -B5 -A10 '_build_field' forms_pro/tests/test_form_field.pyRepository: BuildWithHussain/forms_pro
Length of output: 3163
🏁 Script executed:
#!/bin/bash
# Check if Frappe Small Text fields have any default behavior and verify type annotation
rg -B5 -A15 'def get_options' forms_pro/forms_pro/doctype/form_field/form_field.pyRepository: BuildWithHussain/forms_pro
Length of output: 601
🏁 Script executed:
#!/bin/bash
# Check if there's a to_frappe_field property that might be relevant
rg -B5 -A20 'to_frappe_field' forms_pro/forms_pro/doctype/form_field/form_field.pyRepository: BuildWithHussain/forms_pro
Length of output: 965
get_options() has incorrect return type annotation and test assertion.
The method is declared as def get_options(self) -> str: (never None), but the test asserts self.assertIsNone(field.get_options()). When frappe.get_doc() constructs the doc with "options": None (or omitted), Frappe coerces the Small Text field to "" (empty string), not None. This means:
field.optionswill be"", notNoneget_options()returnsself.options, so it returns""- The test assertion will fail because
assertIsNone("")fails - The return type annotation
-> strcontradicts the test's expectation ofNone
Fix: Either change the return type to str | None and handle both cases, or change the test to assertFalsy(field.get_options()) / assertEqual(field.get_options(), "").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forms_pro/tests/test_form_field.py` around lines 43 - 45, The get_options
return type and behavior are inconsistent with the test: update the
Field.get_options method to return an optional string (e.g., change signature to
return str | None and/or Optional[str]) and normalize empty-string options to
None (i.e., if self.options == "" return None, otherwise return self.options) so
the test_non_heading_with_no_options_returns_none passes; modify the method
implementation referenced as get_options and ensure it checks the field
attribute options before returning, leaving the test name
test_non_heading_with_no_options_returns_none unchanged.
- Updated get_options method to use escape_html for heading labels to prevent potential HTML injection. - Adjusted return type annotation to allow for None in addition to str.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
forms_pro/forms_pro/doctype/form_field/form_field.py (1)
88-99: Avoid eager evaluation ofget_options()when mapping already provides options.
mapping.get("options", self.get_options())always evaluatesself.get_options(), even for fieldtypes whose mapping definesoptions(e.g.,♻️ Proposed change
- "options": mapping.get("options", self.get_options()), + "options": mapping["options"] if "options" in mapping else self.get_options(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/forms_pro/doctype/form_field/form_field.py` around lines 88 - 99, The to_frappe_field property eagerly calls self.get_options() even when FORM_TO_FRAPPE_FIELDTYPE mapping already defines "options"; change the logic in to_frappe_field to only call get_options() when the mapping does not contain an "options" key (e.g., check "options" in mapping and use mapping["options"] when present, otherwise call self.get_options()) so that get_options() is avoided for mapped fieldtypes like Email.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/fields/Heading.vue`:
- Around line 26-34: The frontend mapping in Heading.vue (headingTypeToTag and
computed headingTag) currently maps Fieldtype.HEADING_1→"h2", HEADING_2→"h3",
HEADING_3→"h4", which mismatches backend form_field.py::get_options that emits
h1/h2/h3; update headingTypeToTag (and ensure computed headingTag uses it) so
Fieldtype.HEADING_1 → "h1", Fieldtype.HEADING_2 → "h2", Fieldtype.HEADING_3 →
"h3" to match backend rendering and keep the unique symbols headingTypeToTag,
HeadingFieldtype, Fieldtype.HEADING_1/2/3 and computed headingTag consistent.
---
Nitpick comments:
In `@forms_pro/forms_pro/doctype/form_field/form_field.py`:
- Around line 88-99: The to_frappe_field property eagerly calls
self.get_options() even when FORM_TO_FRAPPE_FIELDTYPE mapping already defines
"options"; change the logic in to_frappe_field to only call get_options() when
the mapping does not contain an "options" key (e.g., check "options" in mapping
and use mapping["options"] when present, otherwise call self.get_options()) so
that get_options() is avoided for mapped fieldtypes like Email.
🪄 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: db8e871e-f24d-4e43-a8e8-4e8cb4c7ab8c
📒 Files selected for processing (2)
forms_pro/forms_pro/doctype/form_field/form_field.pyfrontend/src/components/fields/Heading.vue
Comment on lines
+26
to
+34
| const headingTypeToTag: Record<HeadingFieldtype, HeadingTag> = { | ||
| [Fieldtype.HEADING_1]: "h2", | ||
| [Fieldtype.HEADING_2]: "h3", | ||
| [Fieldtype.HEADING_3]: "h4", | ||
| }; | ||
|
|
||
| const headingTag = computed<HeadingTag>( | ||
| () => headingTypeToTag[props.field.fieldtype as HeadingFieldtype] ?? "h2" | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the Heading.vue file
fd -type f -name "Heading.vue" | head -20Repository: BuildWithHussain/forms_pro
Length of output: 241
🏁 Script executed:
# Find form_field.py to verify the backend mapping
fd -type f -name "form_field.py" | head -10Repository: BuildWithHussain/forms_pro
Length of output: 241
🏁 Script executed:
# Search for references to heading fieldtypes in the codebase
rg "HEADING_[123]|Heading [123]" --type ts --type tsx --type vue --type py | head -30Repository: BuildWithHussain/forms_pro
Length of output: 98
🏁 Script executed:
# Find Heading.vue file
fd "Heading.vue"Repository: BuildWithHussain/forms_pro
Length of output: 113
🏁 Script executed:
# Find form_field.py
fd "form_field.py"Repository: BuildWithHussain/forms_pro
Length of output: 158
🏁 Script executed:
# Search for HEADING fieldtype references
rg "HEADING_[123]" --max-count 20Repository: BuildWithHussain/forms_pro
Length of output: 1316
🏁 Script executed:
# Read Heading.vue to see the exact mapping and context
cat -n frontend/src/components/fields/Heading.vue | head -60Repository: BuildWithHussain/forms_pro
Length of output: 2103
🏁 Script executed:
# Read form_field.py to check the backend heading mapping
cat -n forms_pro/forms_pro/doctype/form_field/form_field.py | head -120Repository: BuildWithHussain/forms_pro
Length of output: 4371
🏁 Script executed:
# Find SubmissionFieldValue.vue to see how it uses Heading component
fd "SubmissionFieldValue.vue"Repository: BuildWithHussain/forms_pro
Length of output: 136
🏁 Script executed:
# Search for where to_frappe_field is used/called
rg "to_frappe_field" --max-count 20Repository: BuildWithHussain/forms_pro
Length of output: 809
🏁 Script executed:
# Search for where the backend heading HTML is generated/used
rg "get_options|HEADING_MAP" --max-count 20Repository: BuildWithHussain/forms_pro
Length of output: 970
🏁 Script executed:
# Read SubmissionFieldValue.vue to see how it handles Heading fields
cat -n frontend/src/components/form/submissions/SubmissionFieldValue.vue | head -80Repository: BuildWithHussain/forms_pro
Length of output: 3205
🏁 Script executed:
# Read form.py to see how to_frappe_field is used
cat -n forms_pro/forms_pro/doctype/form/form.py | grep -A 5 -B 5 "to_frappe_field" | head -40Repository: BuildWithHussain/forms_pro
Length of output: 681
🏁 Script executed:
# Check the test file to understand the expected behavior
cat -n forms_pro/tests/test_form_field.py | grep -A 10 "test_form_to_frappe_fieldtype_has_all_heading_levels"Repository: BuildWithHussain/forms_pro
Length of output: 449
🏁 Script executed:
# Search for any comments or context around the heading tag mapping
rg "h2|h3|h4" frontend/src/components/fields/Heading.vue -B 2 -A 2Repository: BuildWithHussain/forms_pro
Length of output: 490
🏁 Script executed:
# Check if there are any TODOs or FIXMEs related to heading levels
rg "TODO|FIXME|NOTE" frontend/src/components/fields/Heading.vueRepository: BuildWithHussain/forms_pro
Length of output: 52
🏁 Script executed:
# Look for any design/architectural docs mentioning heading level strategy
fd -e md -e txt | xargs grep -l "heading\|h1\|document outline" 2>/dev/null | head -5Repository: BuildWithHussain/forms_pro
Length of output: 52
Heading tag mapping is inconsistent between frontend and backend renderers.
Frontend renders Heading 1 → h2, Heading 2 → h3, Heading 3 → h4, while backend (form_field.py::get_options) produces <h1>, <h2>, <h3> for the same fieldtypes. Both paths surface to users: SubmissionFieldValue.vue uses this Heading.vue component for form submissions, while the synced CustomField's HTML options are rendered via Frappe's standard form view. This causes inconsistent document outline levels depending on the entry point.
Align the mappings: either change frontend to use h1/h2/h3 or change backend to use h2/h3/h4.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/fields/Heading.vue` around lines 26 - 34, The
frontend mapping in Heading.vue (headingTypeToTag and computed headingTag)
currently maps Fieldtype.HEADING_1→"h2", HEADING_2→"h3", HEADING_3→"h4", which
mismatches backend form_field.py::get_options that emits h1/h2/h3; update
headingTypeToTag (and ensure computed headingTag uses it) so Fieldtype.HEADING_1
→ "h1", Fieldtype.HEADING_2 → "h2", Fieldtype.HEADING_3 → "h3" to match backend
rendering and keep the unique symbols headingTypeToTag, HeadingFieldtype,
Fieldtype.HEADING_1/2/3 and computed headingTag consistent.
Collaborator
|
Why not markdown field? 👀 |
Collaborator
Author
|
@NagariaHussain didn't think too much into it, this felt more natural |
harshtandiya
added
backport/version-15
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin version-15
git worktree add -d .worktree/backport-103-to-version-15 origin/version-15
cd .worktree/backport-103-to-version-15
git switch --create backport-103-to-version-15
git cherry-pick -x 16989b1151e9de65b0962b4d98d35d013ca8950c |
7 tasks
harshtandiya
added a commit
that referenced
this pull request
Apr 29, 2026
…114) * fix(multiselect): reset option input and error message on startAddingOption (cherry picked from commit 96a0f94) * chore: better ui to remove options in multiselect (#87) * chore: make labels w-full in builder * chore(multiselect): better ui for removing options - Introduced `inEditMode` prop to `RenderField.vue` to control edit state. - Updated `FieldRenderer.vue` to pass `inEditMode` to `RenderField`. - Enhanced `Multiselect.vue` to utilize `inEditMode` for conditional rendering and option removal functionality. (cherry picked from commit 55c4fc0) * chore(deps-dev): bump postcss from 8.5.8 to 8.5.10 in /frontend (#95) Bumps [postcss](https://github.com/postcss/postcss) from 8.5.8 to 8.5.10. - [Release notes](https://github.com/postcss/postcss/releases) - [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md) - [Commits](postcss/postcss@8.5.8...8.5.10) --- updated-dependencies: - dependency-name: postcss dependency-version: 8.5.10 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> (cherry picked from commit 7a3aa8c) * chore(deps): bump dayjs from 1.11.19 to 1.11.20 in /frontend (#84) Bumps [dayjs](https://github.com/iamkun/dayjs) from 1.11.19 to 1.11.20. - [Release notes](https://github.com/iamkun/dayjs/releases) - [Changelog](https://github.com/iamkun/dayjs/blob/dev/CHANGELOG.md) - [Commits](iamkun/dayjs@v1.11.19...v1.11.20) --- updated-dependencies: - dependency-name: dayjs dependency-version: 1.11.20 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> (cherry picked from commit 222c73d) * chore(deps): bump @lottiefiles/dotlottie-vue in /frontend (#93) Bumps [@lottiefiles/dotlottie-vue](https://github.com/LottieFiles/dotlottie-web/tree/HEAD/packages/vue) from 0.10.4 to 0.11.11. - [Release notes](https://github.com/LottieFiles/dotlottie-web/releases) - [Changelog](https://github.com/LottieFiles/dotlottie-web/blob/main/packages/vue/CHANGELOG.md) - [Commits](https://github.com/LottieFiles/dotlottie-web/commits/@lottiefiles/dotlottie-vue@0.11.11/packages/vue) --- updated-dependencies: - dependency-name: "@lottiefiles/dotlottie-vue" dependency-version: 0.11.11 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> (cherry picked from commit 092d2e8) * chore(deps): bump actions/setup-node from 4 to 6 (#88) Bumps [actions/setup-node](https://github.com/actions/setup-node) from 4 to 6. - [Release notes](https://github.com/actions/setup-node/releases) - [Commits](actions/setup-node@v4...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> (cherry picked from commit 9aa693f) * chore(deps): bump actions/checkout from 4 to 6 (#90) Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 6. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...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> (cherry picked from commit d147e0d) * chore(deps): bump actions/setup-python from 5 to 6 (#91) Bumps [actions/setup-python](https://github.com/actions/setup-python) from 5 to 6. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v5...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> (cherry picked from commit 00a219c) * chore(deps): bump actions/upload-artifact from 4 to 7 (#89) Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 4 to 7. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v4...v7) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-version: '7' 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> (cherry picked from commit e95b934) * ci: add merge_group trigger to enable GitHub merge queue (#97) (cherry picked from commit cd63fa7) * fix(e2e): auto-fill form title in FormBuilderPage.goto() to prevent MandatoryError (#96) Forms created via e2e fixtures have title "Untitled Form" which the frontend transforms to "" on load. When tests modify the form and save, the blank title causes a MandatoryError. This was causing CI failures in multiselect-field.spec.ts. Changes: - FormBuilderPage.goto() now auto-fills a unique title and saves - Added skipTitleFill option for forms that already have a title set - createPublishedForm fixture now sets title alongside is_published - Removed manual title fill from multiselect-field.spec.ts (cherry picked from commit 26e2a74) * chore: update gitignore to ignore semgrep folder (cherry picked from commit a6f6b2a) * fix: resolve semgrep findings across codebase (#98) * fix: resolve semgrep findings across codebase - Add nosemgrep comments for reviewed guest-whitelisted endpoints - Fix format string injection by converting exceptions to str() - Remove redundant db.commit() in test teardown (DDL auto-commits) - Add explanation comment for required CSRF token commit * ci: remove paths from pull_request trigger in typecheck workflow - Simplified the typecheck workflow by removing specific paths from the pull_request trigger, allowing for broader checks on all changes. * fix: use correct semgrep rule ID prefix (frappe-semgrep-rules) (cherry picked from commit d5cde23) * feat: add Heading 1/2/3 field types (#103) * feat(form-field): add Heading 1/2/3 fieldtypes to doctype and backend mapping Maps heading fieldtypes to Frappe HTML, generates h1/h2/h3 options content for the CustomField, and skips heading fields in server-side required validation. * feat(heading): wire up Heading 1/2/3 field types across the frontend Adds heading layout type, Heading component with h2/h3/h4 tag rendering, FieldRenderer branch for edit/view modes, isHeading util, and submission display handling. * test(heading): add backend and E2E tests for heading field types - Unit tests for heading fields skipped in validation - Integration tests for get_options() and to_frappe_field - E2E tests for builder, public form rendering, and submission - Fix missing Heading imports in FieldRenderer and SubmissionFieldValue * fix(form-field): escape HTML in heading labels for get_options method - Updated get_options method to use escape_html for heading labels to prevent potential HTML injection. - Adjusted return type annotation to allow for None in addition to str. * chore: minor styling (cherry picked from commit 16989b1) * refactor: redesign the form builder layout (#105) * refactor: redesign the form builder layout - Introduced a new FieldActions component to handle field removal and drag functionality. - Integrated FieldActions into FormBuilderContent for improved user interaction with form fields. - Updated styles for better visibility and interaction feedback. * feat: enhance FieldActions component for improved drag-and-drop functionality - Updated FieldActions to include drag state handling, allowing for better user feedback during field manipulation. - Integrated the new FieldActions component into FormBuilderContent, enhancing the interaction experience with form fields. - Adjusted styles for visibility based on selection and drag state. (cherry picked from commit 4e9b9d1) * enhance(FieldActions): add tooltips for field actions buttons - Added tooltip text for the remove and drag buttons in the FieldActions component to improve user experience and accessibility. - Updated button structure for better readability and maintainability. (cherry picked from commit 66e04de) * fix(Form): correct initial route generation string (#106) - Updated the initial route generation method to remove the unnecessary 's/' prefix, ensuring the route is generated correctly as 'forms_pro_' followed by a random string. (cherry picked from commit 72f63f8) * fix: prevent duplicate fieldnames on form save (#107) * feat(frontend): add global dialog component for imperative dialogs Adds dialog utility with confirm/alert/show methods callable from anywhere via TypeScript, similar to vue-sonner pattern. * feat(dialog): add html support for rich message content * fix(editForm): prevent save when duplicate fieldnames detected Shows dialog listing conflicting Label(fieldname) pairs before save. (cherry picked from commit 9e7849d) * chore(deps): bump vue from 3.5.32 to 3.5.33 in /frontend (#110) Bumps [vue](https://github.com/vuejs/core) from 3.5.32 to 3.5.33. - [Release notes](https://github.com/vuejs/core/releases) - [Changelog](https://github.com/vuejs/core/blob/main/CHANGELOG.md) - [Commits](vuejs/core@v3.5.32...v3.5.33) --- updated-dependencies: - dependency-name: vue dependency-version: 3.5.33 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> (cherry picked from commit 0ffdad3) * chore(deps-dev): bump @vitejs/plugin-vue in /frontend (#109) Bumps [@vitejs/plugin-vue](https://github.com/vitejs/vite-plugin-vue/tree/HEAD/packages/plugin-vue) from 6.0.5 to 6.0.6. - [Release notes](https://github.com/vitejs/vite-plugin-vue/releases) - [Changelog](https://github.com/vitejs/vite-plugin-vue/blob/main/packages/plugin-vue/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite-plugin-vue/commits/plugin-vue@6.0.6/packages/plugin-vue) --- updated-dependencies: - dependency-name: "@vitejs/plugin-vue" dependency-version: 6.0.6 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> (cherry picked from commit 9035fdb) * fix(backport): adapt cherry-picked code to v15 lucide + dual-enum types - FieldActions.vue: use lucide-vue-next (v15 hasn't migrated to @lucide/vue) - SubmissionFieldValue.vue: cast FormFieldTypes prop to Fieldtype where the Heading helper / component expect the doctype-generated enum (refactor #75 consolidated these enums on develop; v15 still has both) * fix(test): use v15-compatible FrappeTestCase import in test_form_field frappe.tests.IntegrationTestCase doesn't exist on Frappe v15. Match the import pattern used by the other v15 tests (test_roles, test_invitations). --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
3 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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
headinglayout type for FieldRenderercloses #101
Test plan
Summary by CodeRabbit