chore!: refactor code for better maintainability#75
Merged
Conversation
Introduced new TypeScript type definitions for forms and teams, including `Form`, `FormField`, `FPTeam`, and `FPTeamMember`. These definitions enhance type safety and structure for managing forms and team data within the application.
Introduce `frontend/src/config/fieldTypes.ts` as the single source of truth for field-type-specific metadata (component, layout, Frappe fieldtype mapping, boolean/date semantics). Everything that previously required touching 5+ files now lives in one place. - Replace scattered `FormFieldTypes` enum with the auto-generated `Fieldtype` enum from `form_field.types.ts` (frappe-types output), so `form_field.json` is the ground truth for valid names. - Rewrite `form_fields.ts` to re-export from the registry; remove the unused individual named field exports. - `mapDoctypeFieldForForm` now maps to typed `Fieldtype` values. - Replace the 8-branch `to_frappe_field` if/elif chain in `form_field.py` with a declarative `FORM_TO_FRAPPE_FIELDTYPE` dict — adding a new field type is one line. - Fieldtype dropdown in `FieldPropertiesForm` is derived from the registry, so only implemented types are offered.
…eldLabel Replace the 6 hardcoded v-if/v-else-if branches in FieldRenderer (Switch, Checkbox, Text Editor, Attach, Table, default) with 4 layout-driven branches derived from the field type registry. The repeated label/ required-asterisk block is extracted into a FieldLabel sub-component. - Add FieldLabel.vue — editable label input + required asterisk - FieldRenderer branches on registry layout: "inline", "description-first", "custom" (Attach + Table), "default" - Update FieldLayout type: rename "below-label" → "description-first" - Attach keeps its original :value/@update:value binding (custom layout)
Replace hardcoded field type arrays and string comparisons in conditionals.ts and SubmissionFieldValue.vue with registry lookups. - conditionals.ts: getFieldValue uses isBoolean flag instead of explicit Switch/Checkbox check; Number coercion uses Fieldtype enum - SubmissionFieldValue: isDateField and classNames derived from typeDef.isDate and typeDef.isBoolean — new field types with these flags set get correct behaviour automatically
Mirror frontend conditional logic server-side so required-field checks cannot be bypassed via direct API calls. Extracts shared field type constants into utils/constants.py and covers all new helpers with pure-unit tests (no DB required).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds server-side submission coercion and conditional-logic validation; centralizes frontend field-type metadata and types; refactors field rendering and label editing to use layout-driven components; introduces constants to filter unsupported/system fields and tightens form lookup error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "forms_pro API"
participant Validator as "Validation System"
participant DB as "Frappe DB"
Client->>API: POST submit_form_response(form_id, form_data, submission_status)
API->>API: parse submission_status -> SubmissionStatus (or raise)
API->>Validator: filter form_data to declared fields
Validator->>Validator: coerce values by fieldtype (_coerce_field_value)
Validator->>Validator: evaluate conditional_logic (_evaluate_conditions)
Validator->>Validator: validate required/visible rules (_validate_form_response)
alt Validation fails
Validator-->>API: ValidationError
API-->>Client: HTTP 400 (validation error)
else Validation succeeds
API->>DB: create/update submission (fp_submission_status set)
DB-->>API: success
API-->>Client: HTTP 200 (submission saved)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
…c endpoints Both get_doctype_fields and submit_form_response are intentionally guest-accessible — the former for form builder field introspection, the latter for public form submissions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/form/submissions/SubmissionFieldValue.vue (1)
77-84:⚠️ Potential issue | 🟠 MajorValidate attachment URLs before rendering them as links.
This branch turns raw submission data into a clickable
hrefwithtarget="_blank". A craftedjavascript:or other unsafe URL here becomes a phishing/XSS vector for anyone reviewing submissions. Restrict this to trusted file/http(s) URLs, and addrel="noopener noreferrer"for the new tab.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/form/submissions/SubmissionFieldValue.vue` around lines 77 - 84, SubmissionFieldValue.vue renders raw attachment URLs for Fieldtype.ATTACH using the value variable, which allows unsafe schemes like javascript: and missing rel attributes; fix by validating/sanitizing value before rendering: use the URL constructor or a safe helper to parse value and only allow http:, https: (and file: if explicitly trusted) protocols, reject or render as plain text otherwise, and add rel="noopener noreferrer" to the <a> (and keep target="_blank") to prevent tab-napping; implement this validation in the component (e.g., a computed property or method like isSafeAttachmentUrl(value) used where Fieldtype.ATTACH is checked) so only validated URLs become clickable links.
🧹 Nitpick comments (1)
frontend/src/stores/editForm.ts (1)
154-159: TightenaddFieldtyping to acceptFieldtypedirectly and remove the unsafe cast.
addField(fieldtype: string)+as Fieldtypebypasses type safety. Since all callers pass keys fromFIELD_TYPE_DEFINITIONS(which are guaranteedFieldtypeenum values), acceptFieldtypeas the parameter type and remove the cast.♻️ Proposed refactor
- function addField(fieldtype: string) { + function addField(fieldtype: Fieldtype) { if (formResource.value?.doc) { const newField: FormField = { idx: formResource.value.doc.fields.length + 1, - fieldtype: fieldtype as Fieldtype, + fieldtype, label: "", fieldname: "", options: "",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/editForm.ts` around lines 154 - 159, The addField function currently accepts a string and uses an unsafe cast to Fieldtype; change its signature to addField(fieldtype: Fieldtype) and remove the "as Fieldtype" cast when constructing the new FormField (newField.fieldtype = fieldtype), ensuring you still use formResource.value?.doc and FormField.idx logic as-is; update any call sites that pass keys from FIELD_TYPE_DEFINITIONS to pass them as Fieldtype (they should already be valid) so the typing is preserved and no unsafe cast remains.
🤖 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/api/submission.py`:
- Around line 111-112: The current required check uses generic falsiness (if
is_required and not form_data.get(field.fieldname)) which treats valid values
like 0 as missing; update the condition to detect absence or empty-string only
by explicitly checking for None or "" (or missing key) instead of truthiness —
locate the check using is_required and form_data.get(field.fieldname) and
replace it with an explicit absence/empty-string test so numeric/falsey but
valid answers aren’t rejected.
- Around line 153-160: The code currently writes every client-supplied key from
form_data_dict into the linked DocType via submission.set, so reject unexpected
fieldnames by building an allowed set from the declared form fields (e.g.,
allowed = {f.fieldname for f in form.fields}) and compare form_data_dict.keys()
against it; if there are any extras, raise/return an error before creating or
populating the submission (do not call submission.set for unknown fields) and
otherwise only iterate over the filtered keys when assigning values; apply this
check before or immediately after the _validate_form_response call when status
== SubmissionStatus.SUBMITTED to ensure guest requests cannot inject fields into
linked_doctype.
In `@forms_pro/tests/test_submission_validation.py`:
- Around line 30-183: Change the four test classes (TestCoerceFieldValue,
TestEvaluateConditions, TestValidateFormResponse, TestConstants) to inherit from
IntegrationTestCase instead of unittest.TestCase and add the required import for
IntegrationTestCase (e.g. import IntegrationTestCase from the frappe test
utilities) so the tests run under Frappe's test infrastructure.
In `@frontend/src/components/builder/FieldRenderer.vue`:
- Around line 103-105: The template uses an invalid type assertion in v-model
("v-model=\"modelValue as undefined\""); replace that by removing the assertion
and binding a proper assignable ref/computed: create a computed property in the
FieldRenderer component script (with a getter that returns the narrowed value
and a setter that updates the original modelValue/ref) or perform type-narrowing
before binding, then change the Table usage to v-model="yourComputedModel" (or
v-model="modelValue" if you narrow earlier) and keep :in-edit-mode="inEditMode"
unchanged; make sure the computed's setter writes back to the original
modelValue so two-way binding works with the Table component.
In `@frontend/src/config/fieldTypes.ts`:
- Around line 72-256: FIELD_TYPE_DEFINITIONS is non-exhaustive so lookup like
getFieldTypeDef(Fieldtype.MULTISELECT) can return undefined; change the registry
to an exhaustive mapping (e.g., export const FIELD_TYPE_DEFINITIONS:
Record<Fieldtype, FieldTypeDefinition>) and add a missing entry for
Fieldtype.MULTISELECT (use the appropriate component, e.g., Select with
multi/multiple props or the app’s multiselect component, and set
frappeFieldtype/frappeOptions/isBoolean/isDate accordingly) so the compiler
enforces coverage and getFieldTypeDef no longer returns undefined.
In `@frontend/src/types/FormsPro/fpteam_member.types.ts`:
- Around line 1-14: Remove the unused FPTeamMember interface from the file since
team member data is already typed as TeamMember (see TeamMember in
frontend/src/stores/team.ts); either delete the FPTeamMember interface entirely
or, if you intend to keep it for future use, replace it with a one-line TODO
comment above the declaration explaining its planned purpose and referencing
TeamMember so reviewers know why it exists. Ensure no other modules import or
reference FPTeamMember before removal.
In `@frontend/src/types/FormsPro/fpteam.types.ts`:
- Around line 3-20: The FPTeam interface and FPTeamMember type are declared but
unused; either remove both declarations (FPTeam and FPTeamMember) from
frontend/src/types/FormsPro/fpteam.types.ts and any related unused imports, or
add a clear JSDoc comment above each declaration explaining their intended
purpose, fields that map to backend ERPNext docs, and why they are kept for
future use (so reviewers know they aren’t dead code); update any module
exports/index files accordingly to reflect the removal or the documented
retention.
In `@frontend/src/utils/form_fields.ts`:
- Around line 26-28: The mapDoctypeFieldForForm function currently returns
undefined for unmapped DocType fieldtypes which leads to invalid form field
objects downstream (see editForm.ts usage); update mapDoctypeFieldForForm to
stop silently emitting undefined by either (A) making the mapping exhaustive
(add all DocType fieldtype keys to the map and return the concrete form type) or
(B) failing fast: throw a descriptive Error (e.g., `Unknown DocType fieldtype:
${fieldtype}`) or return a clearly handled sentinel and ensure callers (e.g.,
the code in editForm.ts that assigns fieldtype) filter/handle unknowns before
building form objects; locate and update mapDoctypeFieldForForm (and the similar
mapping function at the other referenced spot) so unknown types are not
propagated as undefined.
---
Outside diff comments:
In `@frontend/src/components/form/submissions/SubmissionFieldValue.vue`:
- Around line 77-84: SubmissionFieldValue.vue renders raw attachment URLs for
Fieldtype.ATTACH using the value variable, which allows unsafe schemes like
javascript: and missing rel attributes; fix by validating/sanitizing value
before rendering: use the URL constructor or a safe helper to parse value and
only allow http:, https: (and file: if explicitly trusted) protocols, reject or
render as plain text otherwise, and add rel="noopener noreferrer" to the <a>
(and keep target="_blank") to prevent tab-napping; implement this validation in
the component (e.g., a computed property or method like
isSafeAttachmentUrl(value) used where Fieldtype.ATTACH is checked) so only
validated URLs become clickable links.
---
Nitpick comments:
In `@frontend/src/stores/editForm.ts`:
- Around line 154-159: The addField function currently accepts a string and uses
an unsafe cast to Fieldtype; change its signature to addField(fieldtype:
Fieldtype) and remove the "as Fieldtype" cast when constructing the new
FormField (newField.fieldtype = fieldtype), ensuring you still use
formResource.value?.doc and FormField.idx logic as-is; update any call sites
that pass keys from FIELD_TYPE_DEFINITIONS to pass them as Fieldtype (they
should already be valid) so the typing is preserved and no unsafe cast remains.
🪄 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: 32971035-07be-4d0f-8846-22950d15bee9
📒 Files selected for processing (18)
forms_pro/api/form.pyforms_pro/api/submission.pyforms_pro/forms_pro/doctype/form_field/form_field.pyforms_pro/tests/test_submission_validation.pyforms_pro/utils/constants.pyfrontend/src/components/builder/FieldLabel.vuefrontend/src/components/builder/FieldRenderer.vuefrontend/src/components/builder/field-editor/FieldPropertiesForm.vuefrontend/src/components/form/submissions/SubmissionFieldValue.vuefrontend/src/config/fieldTypes.tsfrontend/src/stores/editForm.tsfrontend/src/types/FormsPro/form.types.tsfrontend/src/types/FormsPro/form_field.types.tsfrontend/src/types/FormsPro/fpteam.types.tsfrontend/src/types/FormsPro/fpteam_member.types.tsfrontend/src/types/formfield.tsfrontend/src/utils/conditionals.tsfrontend/src/utils/form_fields.ts
Comment on lines
+153
to
+160
| form_data_dict = {item["fieldname"]: item["value"] for item in form_data} | ||
|
|
||
| if status == SubmissionStatus.SUBMITTED: | ||
| _validate_form_response(form, form_data_dict) | ||
|
|
||
| submission = frappe.new_doc(linked_doctype) | ||
| for data in form_data: | ||
| submission.set(data["fieldname"], data["value"]) | ||
| for fieldname, value in form_data_dict.items(): | ||
| submission.set(fieldname, value) |
There was a problem hiding this comment.
Reject unexpected field names before calling submission.set().
This endpoint is allow_guest=True, but it currently writes every client-supplied fieldname into the linked DocType. A direct API caller can populate fields that are not part of the form at all. Filter against {f.fieldname for f in form.fields} and throw on extras before inserting.
🔒 Suggested hardening
- form_data_dict = {item["fieldname"]: item["value"] for item in form_data}
+ allowed_fieldnames = {field.fieldname for field in form.fields}
+ unexpected_fieldnames = [
+ item["fieldname"] for item in form_data if item["fieldname"] not in allowed_fieldnames
+ ]
+ if unexpected_fieldnames:
+ frappe.throw(
+ _("Invalid field(s): {0}").format(", ".join(unexpected_fieldnames)),
+ frappe.ValidationError,
+ )
+
+ form_data_dict = {
+ item["fieldname"]: item["value"]
+ for item in form_data
+ if item["fieldname"] in allowed_fieldnames
+ }📝 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
| form_data_dict = {item["fieldname"]: item["value"] for item in form_data} | |
| if status == SubmissionStatus.SUBMITTED: | |
| _validate_form_response(form, form_data_dict) | |
| submission = frappe.new_doc(linked_doctype) | |
| for data in form_data: | |
| submission.set(data["fieldname"], data["value"]) | |
| for fieldname, value in form_data_dict.items(): | |
| submission.set(fieldname, value) | |
| allowed_fieldnames = {field.fieldname for field in form.fields} | |
| unexpected_fieldnames = [ | |
| item["fieldname"] for item in form_data if item["fieldname"] not in allowed_fieldnames | |
| ] | |
| if unexpected_fieldnames: | |
| frappe.throw( | |
| _("Invalid field(s): {0}").format(", ".join(unexpected_fieldnames)), | |
| frappe.ValidationError, | |
| ) | |
| form_data_dict = { | |
| item["fieldname"]: item["value"] | |
| for item in form_data | |
| if item["fieldname"] in allowed_fieldnames | |
| } | |
| if status == SubmissionStatus.SUBMITTED: | |
| _validate_form_response(form, form_data_dict) | |
| submission = frappe.new_doc(linked_doctype) | |
| for fieldname, value in form_data_dict.items(): | |
| submission.set(fieldname, value) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forms_pro/api/submission.py` around lines 153 - 160, The code currently
writes every client-supplied key from form_data_dict into the linked DocType via
submission.set, so reject unexpected fieldnames by building an allowed set from
the declared form fields (e.g., allowed = {f.fieldname for f in form.fields})
and compare form_data_dict.keys() against it; if there are any extras,
raise/return an error before creating or populating the submission (do not call
submission.set for unknown fields) and otherwise only iterate over the filtered
keys when assigning values; apply this check before or immediately after the
_validate_form_response call when status == SubmissionStatus.SUBMITTED to ensure
guest requests cannot inject fields into linked_doctype.
Comment on lines
+30
to
+183
| class TestCoerceFieldValue(unittest.TestCase): | ||
| def test_empty_string_returns_none(self): | ||
| self.assertIsNone(_coerce_field_value("", "Data")) | ||
|
|
||
| def test_none_returns_none(self): | ||
| self.assertIsNone(_coerce_field_value(None, "Data")) | ||
|
|
||
| def test_switch_coerces_to_bool(self): | ||
| self.assertTrue(_coerce_field_value(1, "Switch")) | ||
| self.assertFalse(_coerce_field_value(0, "Switch")) | ||
| self.assertFalse(_coerce_field_value("", "Switch")) | ||
|
|
||
| def test_checkbox_coerces_to_bool(self): | ||
| self.assertTrue(_coerce_field_value("true", "Checkbox")) | ||
| self.assertFalse(_coerce_field_value(0, "Checkbox")) | ||
|
|
||
| def test_number_coerces_to_int(self): | ||
| self.assertEqual(_coerce_field_value("42", "Number"), 42) | ||
| self.assertEqual(_coerce_field_value(7, "Number"), 7) | ||
|
|
||
| def test_number_invalid_returns_none(self): | ||
| self.assertIsNone(_coerce_field_value("abc", "Number")) | ||
|
|
||
| def test_data_returns_string(self): | ||
| self.assertEqual(_coerce_field_value("hello", "Data"), "hello") | ||
| self.assertEqual(_coerce_field_value(123, "Data"), "123") | ||
|
|
||
|
|
||
| class TestEvaluateConditions(unittest.TestCase): | ||
| def _field_map(self, *fields): | ||
| return {f.fieldname: f for f in fields} | ||
|
|
||
| def test_is_operator_match(self): | ||
| fields = self._field_map(_field("status", "Data")) | ||
| conditions = [{"fieldname": "status", "operator": "is", "value": "Active"}] | ||
| self.assertTrue(_evaluate_conditions(conditions, {"status": "Active"}, fields)) | ||
| self.assertFalse(_evaluate_conditions(conditions, {"status": "Inactive"}, fields)) | ||
|
|
||
| def test_is_not_operator(self): | ||
| fields = self._field_map(_field("status", "Data")) | ||
| conditions = [{"fieldname": "status", "operator": "is_not", "value": "Active"}] | ||
| self.assertTrue(_evaluate_conditions(conditions, {"status": "Inactive"}, fields)) | ||
| self.assertFalse(_evaluate_conditions(conditions, {"status": "Active"}, fields)) | ||
|
|
||
| def test_is_empty_operator(self): | ||
| fields = self._field_map(_field("note", "Data")) | ||
| conditions = [{"fieldname": "note", "operator": "is_empty", "value": None}] | ||
| self.assertTrue(_evaluate_conditions(conditions, {"note": ""}, fields)) | ||
| self.assertTrue(_evaluate_conditions(conditions, {"note": None}, fields)) | ||
| self.assertFalse(_evaluate_conditions(conditions, {"note": "some text"}, fields)) | ||
|
|
||
| def test_is_not_empty_operator(self): | ||
| fields = self._field_map(_field("note", "Data")) | ||
| conditions = [{"fieldname": "note", "operator": "is_not_empty", "value": None}] | ||
| self.assertTrue(_evaluate_conditions(conditions, {"note": "hi"}, fields)) | ||
| self.assertFalse(_evaluate_conditions(conditions, {"note": ""}, fields)) | ||
|
|
||
| def test_all_conditions_must_pass(self): | ||
| """AND logic — all conditions must be true.""" | ||
| fields = self._field_map(_field("a", "Data"), _field("b", "Data")) | ||
| conditions = [ | ||
| {"fieldname": "a", "operator": "is", "value": "yes"}, | ||
| {"fieldname": "b", "operator": "is", "value": "yes"}, | ||
| ] | ||
| self.assertTrue(_evaluate_conditions(conditions, {"a": "yes", "b": "yes"}, fields)) | ||
| self.assertFalse(_evaluate_conditions(conditions, {"a": "yes", "b": "no"}, fields)) | ||
|
|
||
| def test_unknown_field_returns_false(self): | ||
| conditions = [{"fieldname": "nonexistent", "operator": "is", "value": "x"}] | ||
| self.assertFalse(_evaluate_conditions(conditions, {}, {})) | ||
|
|
||
|
|
||
| class TestValidateFormResponse(unittest.TestCase): | ||
| def test_required_field_missing_raises(self): | ||
| import frappe | ||
|
|
||
| form = _form(_field("name", reqd=1)) | ||
| with self.assertRaises(frappe.ValidationError): | ||
| _validate_form_response(form, {}) | ||
|
|
||
| def test_required_field_present_passes(self): | ||
| form = _form(_field("name", reqd=1)) | ||
| _validate_form_response(form, {"name": "John"}) # must not raise | ||
|
|
||
| def test_hidden_required_field_is_skipped(self): | ||
| """A hidden required field must not be validated.""" | ||
| form = _form(_field("secret", reqd=1, hidden=1)) | ||
| _validate_form_response(form, {}) # must not raise | ||
|
|
||
| def test_conditional_show_makes_field_required(self): | ||
| """A field conditionally required via require_answer must be checked.""" | ||
| import frappe | ||
|
|
||
| logic = json.dumps( | ||
| { | ||
| "target_field": "phone", | ||
| "action": "require_answer", | ||
| "conditions": [{"fieldname": "contact_method", "operator": "is", "value": "Phone"}], | ||
| } | ||
| ) | ||
| contact = _field("contact_method") | ||
| phone = _field("phone", reqd=0) | ||
| phone_with_logic = _field("contact_method", conditional_logic=logic) | ||
|
|
||
| form = _form(contact, phone, phone_with_logic) | ||
| with self.assertRaises(frappe.ValidationError): | ||
| _validate_form_response(form, {"contact_method": "Phone", "phone": ""}) | ||
|
|
||
| def test_conditional_hide_skips_required_field(self): | ||
| """A conditionally hidden field must not be validated even if reqd=1.""" | ||
| logic = json.dumps( | ||
| { | ||
| "target_field": "extra", | ||
| "action": "hide_field", | ||
| "conditions": [{"fieldname": "flag", "operator": "is", "value": "yes"}], | ||
| } | ||
| ) | ||
| flag = _field("flag") | ||
| extra = _field("extra", reqd=1) | ||
| flag_with_logic = _field("flag", conditional_logic=logic) | ||
|
|
||
| form = _form(flag, extra, flag_with_logic) | ||
| # extra is required but should be hidden when flag=yes — no error expected | ||
| _validate_form_response(form, {"flag": "yes", "extra": ""}) | ||
|
|
||
| def test_multiple_missing_fields_all_reported(self): | ||
| """All missing required fields should appear in the error, not just the first.""" | ||
| import frappe | ||
|
|
||
| form = _form( | ||
| _field("first_name", reqd=1), | ||
| _field("last_name", reqd=1), | ||
| ) | ||
| with self.assertRaises(frappe.ValidationError) as ctx: | ||
| _validate_form_response(form, {}) | ||
|
|
||
| error_msg = str(ctx.exception) | ||
| self.assertIn("First Name", error_msg) | ||
| self.assertIn("Last Name", error_msg) | ||
|
|
||
|
|
||
| class TestConstants(unittest.TestCase): | ||
| def test_system_fieldnames_derived_from_form_generator(self): | ||
| """Constants must stay in sync with form_generator field definitions.""" | ||
| self.assertIn(SUBMISSION_STATUS_FIELDOPTIONS["fieldname"], FORMS_PRO_SYSTEM_FIELDNAMES) | ||
| self.assertIn(LINKED_FORM_FIELDOPTIONS["fieldname"], FORMS_PRO_SYSTEM_FIELDNAMES) | ||
|
|
||
| def test_unsupported_fieldtypes_contains_layout_types(self): | ||
| for fieldtype in ("Section Break", "Column Break", "Tab Break", "Fold"): | ||
| self.assertIn(fieldtype, UNSUPPORTED_FRAPPE_FIELDTYPES) | ||
|
|
||
| def test_unsupported_fieldtypes_contains_non_input_types(self): | ||
| for fieldtype in ("HTML", "Button", "Barcode", "Dynamic Link"): | ||
| self.assertIn(fieldtype, UNSUPPORTED_FRAPPE_FIELDTYPES) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use IntegrationTestCase for test modules under tests/.
These classes currently inherit from unittest.TestCase, which diverges from the repository convention for **/tests/test_*.py. Please switch to Frappe's IntegrationTestCase here so the module runs inside the expected test infrastructure. As per coding guidelines, "Use Frappe's IntegrationTestCase for tests and ensure test infrastructure is set up in install.py:before_tests()".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forms_pro/tests/test_submission_validation.py` around lines 30 - 183, Change
the four test classes (TestCoerceFieldValue, TestEvaluateConditions,
TestValidateFormResponse, TestConstants) to inherit from IntegrationTestCase
instead of unittest.TestCase and add the required import for IntegrationTestCase
(e.g. import IntegrationTestCase from the frappe test utilities) so the tests
run under Frappe's test infrastructure.
Comment on lines
+1
to
+14
| export interface FPTeamMember { | ||
| name: string; | ||
| creation: string; | ||
| modified: string; | ||
| owner: string; | ||
| modified_by: string; | ||
| docstatus: 0 | 1 | 2; | ||
| parent?: string; | ||
| parentfield?: string; | ||
| parenttype?: string; | ||
| idx?: number; | ||
| /** User : Link - User */ | ||
| user?: string; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for usage of FPTeamMember and TeamMember types to identify duplication or unused types
echo "=== Searching for FPTeamMember usage ==="
rg -n --type=ts --type=tsx 'FPTeamMember' -C 2
echo -e "\n=== Searching for TeamMember type definition ==="
rg -n --type=ts --type=tsx 'interface TeamMember|type TeamMember' -C 3
echo -e "\n=== Searching for TeamMember usage in stores ==="
rg -n --type=ts 'TeamMember' frontend/src/stores/ -C 2Repository: BuildWithHussain/forms_pro
Length of output: 927
Remove the unused FPTeamMember interface.
FPTeamMember has no usage anywhere in the codebase. The team member data is already typed as TeamMember in frontend/src/stores/team.ts:18. Delete this interface unless it's intended for future use—if so, add a TODO comment explaining its purpose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/types/FormsPro/fpteam_member.types.ts` around lines 1 - 14,
Remove the unused FPTeamMember interface from the file since team member data is
already typed as TeamMember (see TeamMember in frontend/src/stores/team.ts);
either delete the FPTeamMember interface entirely or, if you intend to keep it
for future use, replace it with a one-line TODO comment above the declaration
explaining its planned purpose and referencing TeamMember so reviewers know why
it exists. Ensure no other modules import or reference FPTeamMember before
removal.
Comment on lines
+3
to
+20
| export interface FPTeam { | ||
| name: string; | ||
| creation: string; | ||
| modified: string; | ||
| owner: string; | ||
| modified_by: string; | ||
| docstatus: 0 | 1 | 2; | ||
| parent?: string; | ||
| parentfield?: string; | ||
| parenttype?: string; | ||
| idx?: number; | ||
| /** Logo : Attach Image */ | ||
| logo?: string; | ||
| /** Team Name : Data */ | ||
| team_name: string; | ||
| /** Users : Table MultiSelect - FP Team Member */ | ||
| users?: FPTeamMember[]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for FPTeam and UserTeam usage to identify duplication or distinct purposes
echo "=== Searching for FPTeam usage ==="
rg -n --type=ts --type=tsx 'FPTeam' -C 2
echo -e "\n=== Searching for UserTeam type definition and usage ==="
rg -n --type=ts 'interface UserTeam|type UserTeam' -C 3
echo -e "\n=== Searching for team-related API calls ==="
rg -n --type=ts 'forms_pro\.api\.(user|team)\.' frontend/src/stores/ -C 2Repository: BuildWithHussain/forms_pro
Length of output: 3146
🏁 Script executed:
# Check if fpteam.types.ts is imported anywhere
rg -n 'from.*fpteam|import.*FPTeam' --type ts --type tsx
# Check for exports of FPTeam
rg -n 'export.*FPTeam|export \*.*fpteam' --type ts --type tsx
# Check if there are index files that might export FPTeam
find frontend/src/types -name 'index.ts' -o -name 'index.tsx' | head -5
# Get complete UserTeam definition
rg -n -A 10 'export type UserTeam' frontend/src/stores/user.tsRepository: BuildWithHussain/forms_pro
Length of output: 508
🏁 Script executed:
# Search for any imports using file path (direct or relative)
rg -n 'fpteam' --type ts --type tsx -i
# Check types directory for index or barrel files
find frontend/src/types -type f \( -name 'index.ts' -o -name 'index.tsx' \)
# Check if fpteam.types.ts exists
find . -path '*fpteam.types.ts'
# List all files in FormsPro types directory
find frontend/src/types/FormsPro -type f 2>/dev/null | head -20Repository: BuildWithHussain/forms_pro
Length of output: 345
🏁 Script executed:
# Check for barrel/index files that might export FPTeam
find frontend/src/types -name 'index.ts' -exec grep -l 'FPTeam\|fpteam' {} \;
# Check if types are exported from any index.ts files
find frontend/src/types -name 'index.ts' -exec cat {} \;
# Final verification: search for any actual usage of fpteam or FPTeam
rg -i 'fpteam' frontend/src --type tsRepository: BuildWithHussain/forms_pro
Length of output: 390
Remove unused FPTeam and FPTeamMember types or document their intended purpose.
The FPTeam interface and FPTeamMember type are defined but never imported or used anywhere in the codebase. All team-related operations use the simpler UserTeam type from frontend/src/stores/user.ts (4 fields: name, team_name, logo, is_current). The comprehensive metadata fields in FPTeam (creation, modified, owner, modified_by, docstatus, hierarchy fields) are not utilized by the frontend.
Either remove these unused types or clearly document their intended use case if they are meant for future features. Keeping dead code without documentation creates confusion and maintenance burden.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/types/FormsPro/fpteam.types.ts` around lines 3 - 20, The FPTeam
interface and FPTeamMember type are declared but unused; either remove both
declarations (FPTeam and FPTeamMember) from
frontend/src/types/FormsPro/fpteam.types.ts and any related unused imports, or
add a clear JSDoc comment above each declaration explaining their intended
purpose, fields that map to backend ERPNext docs, and why they are kept for
future use (so reviewers know they aren’t dead code); update any module
exports/index files accordingly to reflect the removal or the documented
retention.
Comment on lines
+26
to
+28
| export const mapDoctypeFieldForForm = ( | ||
| fieldtype: string | ||
| ): string | undefined => { |
There was a problem hiding this comment.
Don't silently emit undefined field types from the importer.
frontend/src/stores/editForm.ts:46-60 writes this return value straight into fieldtype, so any unmapped DocType field produces an invalid form field object that later registry lookups cannot resolve. Either make this mapping exhaustive or throw/filter when a fieldtype is unknown instead of returning undefined.
🧭 One way to make this fail fast
-export const mapDoctypeFieldForForm = (
- fieldtype: string
-): string | undefined => {
+export const mapDoctypeFieldForForm = (fieldtype: string): Fieldtype => {
const FRAPPE_TO_FORM_TYPE: Partial<Record<string, Fieldtype>> = {
Autocomplete: Fieldtype.DATA,
@@
- return FRAPPE_TO_FORM_TYPE[fieldtype];
+ const mapped = FRAPPE_TO_FORM_TYPE[fieldtype];
+ if (!mapped) {
+ throw new Error(`Unsupported DocType fieldtype: ${fieldtype}`);
+ }
+ return mapped;
};Also applies to: 56-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/utils/form_fields.ts` around lines 26 - 28, The
mapDoctypeFieldForForm function currently returns undefined for unmapped DocType
fieldtypes which leads to invalid form field objects downstream (see editForm.ts
usage); update mapDoctypeFieldForForm to stop silently emitting undefined by
either (A) making the mapping exhaustive (add all DocType fieldtype keys to the
map and return the concrete form type) or (B) failing fast: throw a descriptive
Error (e.g., `Unknown DocType fieldtype: ${fieldtype}`) or return a clearly
handled sentinel and ensure callers (e.g., the code in editForm.ts that assigns
fieldtype) filter/handle unknowns before building form objects; locate and
update mapDoctypeFieldForForm (and the similar mapping function at the other
referenced spot) so unknown types are not propagated as undefined.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
forms_pro/api/submission.py (2)
153-160:⚠️ Potential issue | 🔴 CriticalReject unexpected field names before writing to submission.
This
allow_guest=Trueendpoint writes every client-suppliedfieldnameto the linked DocType without validation. A direct API caller can inject fields not defined in the form. Filter against{f.fieldname for f in form.fields}before setting values.,
🔒 Proposed hardening
- form_data_dict = {item["fieldname"]: item["value"] for item in form_data} + allowed_fieldnames = {field.fieldname for field in form.fields} + form_data_dict = { + item["fieldname"]: item["value"] + for item in form_data + if item["fieldname"] in allowed_fieldnames + } + unexpected = [ + item["fieldname"] for item in form_data if item["fieldname"] not in allowed_fieldnames + ] + if unexpected: + frappe.throw( + _("Invalid field(s): {0}").format(", ".join(unexpected)), + frappe.ValidationError, + ) if status == SubmissionStatus.SUBMITTED: _validate_form_response(form, form_data_dict)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/api/submission.py` around lines 153 - 160, Client-supplied form_data_dict is written directly into the new linked DocType (submission) allowing injection of unexpected fields; before calling submission.set you must validate and reject any fieldnames not defined on the form by computing allowed = {f.fieldname for f in form.fields} and filtering form_data_dict (or raising an error) so only allowed keys are set; apply this check prior to calling _validate_form_response and before the loop that calls submission.set to ensure only whitelisted fields are written to the linked_doctype.
111-112:⚠️ Potential issue | 🟠 MajorTruthiness check rejects valid falsy values like
0.
not form_data.get(field.fieldname)treats0,False, and empty string as missing. A required Number field answered with0would incorrectly fail validation. Check explicitly for absence.,
🩹 Proposed fix
- if is_required and not form_data.get(field.fieldname): + value = form_data.get(field.fieldname) + if is_required and (value is None or value == ""): errors.append(_("{0} is required").format(field.label))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/api/submission.py` around lines 111 - 112, The current required check uses "not form_data.get(field.fieldname)" which rejects valid falsy answers like 0 or False; update the condition to explicitly treat only missing/empty values as invalid, e.g. replace the check with one that verifies whether the key is absent or the value is None or an empty string (use: if is_required and (field.fieldname not in form_data or form_data[field.fieldname] is None or form_data[field.fieldname] == "")), keeping references to form_data.get(field.fieldname), is_required, errors, and field.label for the error append.
🧹 Nitpick comments (2)
forms_pro/api/form.py (1)
235-243: Return type annotation is inaccurate.
get_doctype_fieldsis annotated as-> dictbut returns a list. Please align the signature with the actual return type to avoid type drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/api/form.py` around lines 235 - 243, The function get_doctype_fields is annotated as returning dict but actually returns a list of field objects; update its return type annotation from -> dict to -> list (or a more specific typing like -> List[frappe.Field] or -> List[Dict[str, Any]] if available) and import typing List/Dict as needed; ensure the function signature in forms_pro/api/form.py reflects this change and adjust any callers/annotations if they rely on the previous dict type.forms_pro/api/submission.py (1)
90-93: Silently ignoring malformed conditional logic may hide corruption.Catching
JSONDecodeErrorand continuing without logging makes debugging difficult if conditional logic fields become corrupted. Consider adding a warning log.📝 Suggested improvement
try: logic = json.loads(other.conditional_logic) except (json.JSONDecodeError, TypeError): + frappe.log_error( + title="Invalid conditional_logic JSON", + message=f"Field {other.fieldname} has malformed conditional_logic", + ) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/api/submission.py` around lines 90 - 93, The current loop in submission processing silently skips malformed conditional logic when json.loads(other.conditional_logic) raises JSONDecodeError or TypeError; update the except block to log a warning including the exception and identifying context (e.g., the offending object’s id or form/field identifiers such as other.id or other.field_name) before continuing so corrupted conditional_logic is visible in logs; keep the continue behavior but ensure the log message includes the exception text and enough context to locate the bad record.
🤖 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/api/submission.py`:
- Around line 43-47: The Number coercion in the submission.py branch that checks
fieldtype == "Number" uses int(value) which rejects decimal strings; update that
block to use float(value) (and keep the existing exception handling for
TypeError/ValueError) so backend coercion matches the frontend's
Number(fieldValue) behavior; locate the fieldtype == "Number" handling in the
relevant function in submission.py and replace int(...) with float(...) while
returning None on parse failure.
- Around line 61-64: The comparison logic currently stringifies actual/expected
and loses type info; instead, call _coerce_field_value(expected,
field.fieldtype) to coerce expected to the field's type and then perform strict
comparisons for the "is" and "is_not" operators (use the already-coerced actual
variable), so comparisons between booleans and numbers match frontend behavior;
update the branch handling operator == "is" and "is_not" to use the
coerced_expected and strict equality checks rather than str conversions.
---
Duplicate comments:
In `@forms_pro/api/submission.py`:
- Around line 153-160: Client-supplied form_data_dict is written directly into
the new linked DocType (submission) allowing injection of unexpected fields;
before calling submission.set you must validate and reject any fieldnames not
defined on the form by computing allowed = {f.fieldname for f in form.fields}
and filtering form_data_dict (or raising an error) so only allowed keys are set;
apply this check prior to calling _validate_form_response and before the loop
that calls submission.set to ensure only whitelisted fields are written to the
linked_doctype.
- Around line 111-112: The current required check uses "not
form_data.get(field.fieldname)" which rejects valid falsy answers like 0 or
False; update the condition to explicitly treat only missing/empty values as
invalid, e.g. replace the check with one that verifies whether the key is absent
or the value is None or an empty string (use: if is_required and
(field.fieldname not in form_data or form_data[field.fieldname] is None or
form_data[field.fieldname] == "")), keeping references to
form_data.get(field.fieldname), is_required, errors, and field.label for the
error append.
---
Nitpick comments:
In `@forms_pro/api/form.py`:
- Around line 235-243: The function get_doctype_fields is annotated as returning
dict but actually returns a list of field objects; update its return type
annotation from -> dict to -> list (or a more specific typing like ->
List[frappe.Field] or -> List[Dict[str, Any]] if available) and import typing
List/Dict as needed; ensure the function signature in forms_pro/api/form.py
reflects this change and adjust any callers/annotations if they rely on the
previous dict type.
In `@forms_pro/api/submission.py`:
- Around line 90-93: The current loop in submission processing silently skips
malformed conditional logic when json.loads(other.conditional_logic) raises
JSONDecodeError or TypeError; update the except block to log a warning including
the exception and identifying context (e.g., the offending object’s id or
form/field identifiers such as other.id or other.field_name) before continuing
so corrupted conditional_logic is visible in logs; keep the continue behavior
but ensure the log message includes the exception text and enough context to
locate the bad record.
🪄 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: 46daf265-98fc-42db-bfc7-dffa85c86e47
📒 Files selected for processing (2)
forms_pro/api/form.pyforms_pro/api/submission.py
Comment on lines
+234
to
242
| @frappe.whitelist(allow_guest=True) # nosemgrep: frappe-semgrep-rules.rules.security.guest-whitelisted-method | ||
| def get_doctype_fields(doctype: str) -> dict: | ||
| doctype = frappe.get_doc("DocType", doctype) | ||
| fields = doctype.fields | ||
|
|
||
| FIELDTYPES_TO_REMOVE = [ | ||
| "Section Break", | ||
| "HTML", | ||
| "Button", | ||
| "Column Break", | ||
| "Tab Break", | ||
| "Barcode", | ||
| "Dynamic Link", | ||
| "Fold", | ||
| fields = [ | ||
| field | ||
| for field in doctype.fields | ||
| if field.fieldtype not in UNSUPPORTED_FRAPPE_FIELDTYPES | ||
| and field.fieldname not in FORMS_PRO_SYSTEM_FIELDNAMES | ||
| ] |
There was a problem hiding this comment.
Guest access here can expose schema for arbitrary DocTypes.
get_doctype_fields is guest-accessible and accepts any doctype name, but it does not enforce a permission or publish-scope check before returning field metadata. That enables unauthenticated schema enumeration outside intended form contexts.
🔒 Suggested guard (limit guest access to published-form doctypes)
`@frappe.whitelist`(allow_guest=True) # nosemgrep: frappe-semgrep-rules.rules.security.guest-whitelisted-method
def get_doctype_fields(doctype: str) -> dict:
+ if frappe.session.user == "Guest":
+ is_public_doctype = frappe.db.exists(
+ "Form",
+ {"linked_doctype": doctype, "is_published": 1},
+ )
+ if not is_public_doctype:
+ frappe.throw(_("You do not have access to this doctype"), frappe.PermissionError)
+ elif not frappe.has_permission("DocType", "read", doctype):
+ frappe.throw(_("You do not have read access to this doctype"), frappe.PermissionError)
+
doctype = frappe.get_doc("DocType", doctype)
fields = [
field
for field in doctype.fields
if field.fieldtype not in UNSUPPORTED_FRAPPE_FIELDTYPES
and field.fieldname not in FORMS_PRO_SYSTEM_FIELDNAMES
]
return fields- Validate attachment URLs before rendering as links; reject javascript: and other unsafe schemes, add rel="noopener noreferrer" - Fix required-field check to allow falsy-but-valid values (0, False) - Whitelist form_data_dict keys to declared form fields only, preventing injection of system fields into the linked DocType - Tighten addField() signature to Fieldtype, removing the unsafe string cast - Remove invalid `as undefined` type assertion from Table v-model - Add two new tests covering the falsy-value edge cases
Number fields now coerce via float() matching frontend Number() behaviour, so decimal values like 3.14 no longer cause conditions to silently fail. Condition comparison now coerces expected through the same function as actual, eliminating str(True)=="True" vs "true" and "3.0" vs "3" mismatches. Adds five new tests covering float parity and type-aware boolean/number cases.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
frontend/src/components/builder/FieldRenderer.vue (2)
80-105: Move the last field-type-specific custom branch into the registry.
fieldData.fieldtype === "Attach"plus the hard-coded<Table>path puts custom-type knowledge back intoFieldRenderer.vue, so the next custom widget still needs edits here. Consider storing the custom renderer/binding mode inFieldTypeDefinition, and at least replace the raw"Attach"literal withFieldtype.ATTACHso the generated enum stays the source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/builder/FieldRenderer.vue` around lines 80 - 105, Replace the hard-coded custom branch in FieldRenderer.vue that checks fieldData.fieldtype === 'Attach' and renders Table with a registry-driven solution: add a renderer/binding mode property to your FieldTypeDefinition (e.g., rendererComponent or bindingMode) and register the custom renderer for the Attach type in the field type registry, then update FieldRenderer.vue to look up the renderer from the registry and render it instead of special-casing "Attach"; also replace the raw "Attach" string with the enum constant Fieldtype.ATTACH wherever that type is referenced so the generated enum remains the single source of truth (use the registry lookup symbol and the FieldTypeDefinition fields to locate the change).
26-27: Replace with Biome-native suppression syntax or tighten the model type.The
eslint-disable-next-linecomment uses ESLint syntax in a Biome-managed frontend path. Per the coding guidelines (frontend/src/**/*.{vue,ts,tsx,scss}: Use BiomeJS for linting and formatting), use// biome-ignore lint/suspicious/noExplicitAny:instead, or consider tightening thedefineModeltype to avoid the suppression altogether.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/builder/FieldRenderer.vue` around lines 26 - 27, The file uses an ESLint suppression for an any type on the modelValue declaration; replace the ESLint suppression with the Biome-native suppression or tighten the type: locate the const modelValue = defineModel<any>() declaration and either change the suppression comment to // biome-ignore lint/suspicious/noExplicitAny: or replace the any by a concrete type for defineModel (e.g., the actual field model/interface) to remove the need for suppression. Ensure the change targets the modelValue/defineModel usage so Biome linting passes.
🤖 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/api/submission.py`:
- Around line 51-73: The decoded conditional payload must be validated before
use and _evaluate_conditions must defensively handle unexpected shapes and
operators: ensure the caller verifies json.loads(...) yields a dict/expected
shape and that the "conditions" value is a list of dicts (or bail/return False)
before calling _evaluate_conditions; inside _evaluate_conditions validate that
each condition is a dict, that fieldname/operator exist and are the correct
types, and treat any unrecognized operator as failing the condition (return
False) instead of falling through as satisfied so unsupported operators don't
silently pass.
- Around line 36-48: The coercion currently converts empty collections (e.g., []
for Table) to a string, so required checks and empty/ non-empty conditionals
misclassify them; update _coerce_field_value to detect iterable/collection types
(list, tuple, set, dict, etc.) and return None when their length is zero (treat
zero-length collections as empty answers) while preserving non-empty
collections, and also update the required-answer check logic used by
is_empty/is_not_empty (the code that currently only treats None/"" as empty) to
consider zero-length collections as empty as well; reference _coerce_field_value
and the is_empty / is_not_empty / required-check code paths so both coercion and
validation consistently treat empty collections as empty.
- Around line 158-163: The code builds form_data_dict from form_data without
validating each item shape, causing TypeError/KeyError for malformed guest
payloads; before the dict comprehension that creates form_data_dict (and before
filtering against allowed_fieldnames), iterate/validate each item in form_data
to ensure it's a mapping with both "fieldname" and "value" keys (and that
"fieldname" is a string), and if any item is malformed raise a ValidationError;
then proceed to build form_data_dict and filter by allowed_fieldnames (from
form.fields) as existing code does.
---
Nitpick comments:
In `@frontend/src/components/builder/FieldRenderer.vue`:
- Around line 80-105: Replace the hard-coded custom branch in FieldRenderer.vue
that checks fieldData.fieldtype === 'Attach' and renders Table with a
registry-driven solution: add a renderer/binding mode property to your
FieldTypeDefinition (e.g., rendererComponent or bindingMode) and register the
custom renderer for the Attach type in the field type registry, then update
FieldRenderer.vue to look up the renderer from the registry and render it
instead of special-casing "Attach"; also replace the raw "Attach" string with
the enum constant Fieldtype.ATTACH wherever that type is referenced so the
generated enum remains the single source of truth (use the registry lookup
symbol and the FieldTypeDefinition fields to locate the change).
- Around line 26-27: The file uses an ESLint suppression for an any type on the
modelValue declaration; replace the ESLint suppression with the Biome-native
suppression or tighten the type: locate the const modelValue =
defineModel<any>() declaration and either change the suppression comment to //
biome-ignore lint/suspicious/noExplicitAny: or replace the any by a concrete
type for defineModel (e.g., the actual field model/interface) to remove the need
for suppression. Ensure the change targets the modelValue/defineModel usage so
Biome linting passes.
🪄 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: 13e2b146-9e9d-407e-b07e-9305416d6384
📒 Files selected for processing (7)
forms_pro/api/submission.pyforms_pro/tests/test_submission_validation.pyfrontend/src/components/builder/FieldRenderer.vuefrontend/src/components/builder/sidebar/AddFieldsSection.vuefrontend/src/components/form/submissions/SubmissionFieldValue.vuefrontend/src/stores/editForm.tsfrontend/src/types/FormsPro/form_field.types.ts
✅ Files skipped from review due to trivial changes (2)
- forms_pro/tests/test_submission_validation.py
- frontend/src/types/FormsPro/form_field.types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/form/submissions/SubmissionFieldValue.vue
Comment on lines
+36
to
+48
| def _coerce_field_value(value: Any, fieldtype: str) -> Any: | ||
| """Coerce a submitted value to its comparable type, matching frontend conditionals.ts logic.""" | ||
| if value is None or value == "": | ||
| return None | ||
| # Matches isBoolean types in the frontend registry (Switch, Checkbox) | ||
| if fieldtype in ("Switch", "Checkbox"): | ||
| return bool(value) | ||
| if fieldtype == "Number": | ||
| try: | ||
| return float(value) | ||
| except (TypeError, ValueError): | ||
| return None | ||
| return str(value) |
There was a problem hiding this comment.
Treat zero-length collections as empty answers.
forms_pro/forms_pro/doctype/form_field/form_field.py includes Table as a supported field type, but [] currently falls through to str(value) and the required check on Line 116 only rejects None/"". That lets submitted responses bypass required table rows, and the same root cause also makes is_empty / is_not_empty misclassify empty collections.
🩹 Suggested fix
+def _is_empty_value(value: Any) -> bool:
+ return value is None or value == "" or (
+ isinstance(value, (list, tuple, dict, set)) and len(value) == 0
+ )
+
+
def _coerce_field_value(value: Any, fieldtype: str) -> Any:
"""Coerce a submitted value to its comparable type, matching frontend conditionals.ts logic."""
- if value is None or value == "":
+ if _is_empty_value(value):
return None
+ if fieldtype == "Table":
+ return value
# Matches isBoolean types in the frontend registry (Switch, Checkbox)
if fieldtype in ("Switch", "Checkbox"):
return bool(value)
if fieldtype == "Number":
try:
return float(value)
except (TypeError, ValueError):
return None
return str(value)
...
- if is_required and (value is None or value == ""):
+ if is_required and _is_empty_value(value):
errors.append(_("{0} is required").format(field.label))Also applies to: 115-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forms_pro/api/submission.py` around lines 36 - 48, The coercion currently
converts empty collections (e.g., [] for Table) to a string, so required checks
and empty/ non-empty conditionals misclassify them; update _coerce_field_value
to detect iterable/collection types (list, tuple, set, dict, etc.) and return
None when their length is zero (treat zero-length collections as empty answers)
while preserving non-empty collections, and also update the required-answer
check logic used by is_empty/is_not_empty (the code that currently only treats
None/"" as empty) to consider zero-length collections as empty as well;
reference _coerce_field_value and the is_empty / is_not_empty / required-check
code paths so both coercion and validation consistently treat empty collections
as empty.
Comment on lines
+51
to
+73
| def _evaluate_conditions(conditions: list[dict], form_data: dict, field_map: dict) -> bool: | ||
| """Evaluate AND-joined conditions against submitted form data.""" | ||
| for condition in conditions: | ||
| fieldname = condition.get("fieldname") | ||
| operator = condition.get("operator") | ||
| expected = condition.get("value") | ||
| field = field_map.get(fieldname) | ||
| if not field: | ||
| return False | ||
| actual = _coerce_field_value(form_data.get(fieldname), field.fieldtype) | ||
| # Coerce the condition's expected value through the same function so | ||
| # types match — avoids str(True)=="True" vs "true" mismatches and | ||
| # str(3.0)=="3.0" vs "3" mismatches for Number fields. | ||
| expected_coerced = _coerce_field_value(expected, field.fieldtype) | ||
| if operator == "is" and actual != expected_coerced: | ||
| return False | ||
| if operator == "is_not" and actual == expected_coerced: | ||
| return False | ||
| if operator == "is_empty" and actual is not None and actual != "": | ||
| return False | ||
| if operator == "is_not_empty" and (actual is None or actual == ""): | ||
| return False | ||
| return True |
There was a problem hiding this comment.
Validate decoded conditional logic before evaluating it.
Line 99 assumes json.loads() returned a dict, and _evaluate_conditions() assumes conditions is a list of dicts. Valid JSON like [] or {"conditions": "oops"} will blow up here, and an unsupported operator currently falls through as satisfied because none of these branches return False.
🛡️ Suggested fix
def _evaluate_conditions(conditions: list[dict], form_data: dict, field_map: dict) -> bool:
"""Evaluate AND-joined conditions against submitted form data."""
+ if not isinstance(conditions, list):
+ return False
+
for condition in conditions:
+ if not isinstance(condition, dict):
+ return False
+
fieldname = condition.get("fieldname")
operator = condition.get("operator")
expected = condition.get("value")
field = field_map.get(fieldname)
if not field:
return False
actual = _coerce_field_value(form_data.get(fieldname), field.fieldtype)
expected_coerced = _coerce_field_value(expected, field.fieldtype)
- if operator == "is" and actual != expected_coerced:
- return False
- if operator == "is_not" and actual == expected_coerced:
- return False
- if operator == "is_empty" and actual is not None and actual != "":
- return False
- if operator == "is_not_empty" and (actual is None or actual == ""):
- return False
+ if operator == "is":
+ if actual != expected_coerced:
+ return False
+ elif operator == "is_not":
+ if actual == expected_coerced:
+ return False
+ elif operator == "is_empty":
+ if actual is not None and actual != "":
+ return False
+ elif operator == "is_not_empty":
+ if actual is None or actual == "":
+ return False
+ else:
+ return False
return True
...
try:
logic = json.loads(other.conditional_logic)
except (json.JSONDecodeError, TypeError):
continue
+ if not isinstance(logic, dict):
+ continueAlso applies to: 94-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forms_pro/api/submission.py` around lines 51 - 73, The decoded conditional
payload must be validated before use and _evaluate_conditions must defensively
handle unexpected shapes and operators: ensure the caller verifies
json.loads(...) yields a dict/expected shape and that the "conditions" value is
a list of dicts (or bail/return False) before calling _evaluate_conditions;
inside _evaluate_conditions validate that each condition is a dict, that
fieldname/operator exist and are the correct types, and treat any unrecognized
operator as failing the condition (return False) instead of falling through as
satisfied so unsupported operators don't silently pass.
Comment on lines
+158
to
+163
| form_data_dict = {item["fieldname"]: item["value"] for item in form_data} | ||
|
|
||
| # Whitelist to declared form fields only — prevents injecting system fields | ||
| # like owner, docstatus, etc. into the linked DocType. | ||
| allowed_fieldnames = {f.fieldname for f in form.fields} | ||
| form_data_dict = {k: v for k, v in form_data_dict.items() if k in allowed_fieldnames} |
There was a problem hiding this comment.
Validate form_data item shape before building the dict.
Because this endpoint is allow_guest=True, payloads like [1] or [{"fieldname": "x"}] currently raise TypeError / KeyError here and bubble out as 500s. Convert malformed items into a ValidationError before building form_data_dict.
🧪 Suggested fix
- form_data_dict = {item["fieldname"]: item["value"] for item in form_data}
+ form_data_dict: dict[str, Any] = {}
+ for item in form_data:
+ if (
+ not isinstance(item, dict)
+ or not isinstance(item.get("fieldname"), str)
+ or "value" not in item
+ ):
+ frappe.throw(_("Invalid form data payload"), frappe.ValidationError)
+
+ form_data_dict[item["fieldname"]] = item["value"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forms_pro/api/submission.py` around lines 158 - 163, The code builds
form_data_dict from form_data without validating each item shape, causing
TypeError/KeyError for malformed guest payloads; before the dict comprehension
that creates form_data_dict (and before filtering against allowed_fieldnames),
iterate/validate each item in form_data to ensure it's a mapping with both
"fieldname" and "value" keys (and that "fieldname" is a string), and if any item
is malformed raise a ValidationError; then proceed to build form_data_dict and
filter by allowed_fieldnames (from form.fields) as existing code does.
…bugs
Operator and action values stored in conditional_logic JSON use the
frontend ConditionalOperators/Actions enum strings ("Is", "Is Not",
"Show Field", "Hide Field", "Require Answer"). The backend was checking
snake_case equivalents ("is", "hide_field", etc.) that never matched,
making server-side conditional validation completely inert. All operator
and action comparisons are updated to match the stored values, and all
tests are updated to use real-world JSON values rather than impossible
snake_case strings.
Also:
- get_form_by_route: throw DoesNotExistError when route not found
instead of passing None to get_form and getting a confusing error
- get_doctype_fields: fix return type annotation dict → list
- saveAndPublish: return the save() promise instead of fire-and-forget
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
forms_pro/api/form.py (1)
236-244:⚠️ Potential issue | 🟠 MajorGuest-accessible
get_doctype_fieldsstill allows arbitrary schema enumeration.Line 236 allows guest access, and Lines 238-244 return DocType field metadata without publish-scope or permission checks. This remains an exposure.
🔒 Suggested guard
`@frappe.whitelist`(allow_guest=True) # nosemgrep: frappe-semgrep-rules.rules.security.guest-whitelisted-method def get_doctype_fields(doctype: str) -> list: + if frappe.session.user == "Guest": + is_public_doctype = frappe.db.exists( + "Form", + {"linked_doctype": doctype, "is_published": 1}, + ) + if not is_public_doctype: + frappe.throw(_("You do not have access to this doctype"), frappe.PermissionError) + elif not frappe.has_permission("DocType", "read", doctype): + frappe.throw(_("You do not have read access to this doctype"), frappe.PermissionError) + doctype = frappe.get_doc("DocType", doctype) fields = [ field for field in doctype.fields if field.fieldtype not in UNSUPPORTED_FRAPPE_FIELDTYPES and field.fieldname not in FORMS_PRO_SYSTEM_FIELDNAMES ] return fields🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/api/form.py` around lines 236 - 244, The guest-exposed get_doctype_fields function returns DocType field metadata without permission checks; remove or stop using allow_guest=True on the `@frappe.whitelist` decorator and add an explicit permission guard in get_doctype_fields: fetch the DocType using frappe.get_doc or frappe.get_meta, then call frappe.has_permission(doctype_name, "read") (or raise frappe.PermissionError) and only proceed to build the fields list (filtering by UNSUPPORTED_FRAPPE_FIELDTYPES and FORMS_PRO_SYSTEM_FIELDNAMES) if the permission check passes; alternatively, allow anonymous access only for DocTypes explicitly marked public by checking a dedicated allow_guest/public flag on the DocType before returning fields.
🤖 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/api/form.py`:
- Around line 40-45: The endpoint get_form_by_route currently allows guests to
fetch any form; before returning get_form(form_id) add a publish-scope guard:
after resolving form_id, fetch the form's published flag (e.g.,
frappe.db.get_value("Form", form_id, "published") or the appropriate field) and
if frappe.session.user == "Guest" and the form is not published, raise a
permission error (frappe.throw(..., frappe.PermissionError) or similar). Keep
the check inside get_form_by_route so authenticated users still get
get_form(form_id) but guests can only access forms where published is truthy.
---
Duplicate comments:
In `@forms_pro/api/form.py`:
- Around line 236-244: The guest-exposed get_doctype_fields function returns
DocType field metadata without permission checks; remove or stop using
allow_guest=True on the `@frappe.whitelist` decorator and add an explicit
permission guard in get_doctype_fields: fetch the DocType using frappe.get_doc
or frappe.get_meta, then call frappe.has_permission(doctype_name, "read") (or
raise frappe.PermissionError) and only proceed to build the fields list
(filtering by UNSUPPORTED_FRAPPE_FIELDTYPES and FORMS_PRO_SYSTEM_FIELDNAMES) if
the permission check passes; alternatively, allow anonymous access only for
DocTypes explicitly marked public by checking a dedicated allow_guest/public
flag on the DocType before returning fields.
🪄 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: 82ef01b6-1ac9-47de-888d-db8d31dfdf37
📒 Files selected for processing (4)
forms_pro/api/form.pyforms_pro/api/submission.pyforms_pro/tests/test_submission_validation.pyfrontend/src/stores/editForm.ts
✅ Files skipped from review due to trivial changes (1)
- forms_pro/tests/test_submission_validation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/stores/editForm.ts
Comment on lines
40
to
45
| @frappe.whitelist(allow_guest=True) | ||
| def get_form_by_route(route: str) -> dict: | ||
| form_id = frappe.db.get_value("Form", {"route": route}, pluck="name") | ||
| if not form_id: | ||
| frappe.throw(_("Form not found"), frappe.DoesNotExistError) | ||
| return get_form(form_id) |
There was a problem hiding this comment.
Restrict guest reads to published forms in get_form_by_route.
Line 40 exposes this endpoint to guests, but Lines 42-45 return full form metadata for any known route. Add a publish-scope/permission check before returning get_form(...).
🔒 Suggested guard
`@frappe.whitelist`(allow_guest=True)
def get_form_by_route(route: str) -> dict:
- form_id = frappe.db.get_value("Form", {"route": route}, pluck="name")
- if not form_id:
+ form_meta = frappe.db.get_value(
+ "Form",
+ {"route": route},
+ ["name", "is_published"],
+ as_dict=True,
+ )
+ if not form_meta:
frappe.throw(_("Form not found"), frappe.DoesNotExistError)
- return get_form(form_id)
+
+ if frappe.session.user == "Guest" and not form_meta.is_published:
+ frappe.throw(_("You do not have access to this form"), frappe.PermissionError)
+
+ if frappe.session.user != "Guest" and not frappe.has_permission("Form", "read", form_meta.name):
+ frappe.throw(_("You do not have read access to this form"), frappe.PermissionError)
+
+ return get_form(form_meta.name)🧰 Tools
🪛 GitHub Actions: Linters
[error] 40-45: Semgrep blocking finding: frappe-semgrep-rules.rules.security.guest-whitelisted-method. Whitelisted method accessible to guest requires manual security review. Rule triggered at @frappe.whitelist(allow_guest=True) in get_form_by_route(route: str).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forms_pro/api/form.py` around lines 40 - 45, The endpoint get_form_by_route
currently allows guests to fetch any form; before returning get_form(form_id)
add a publish-scope guard: after resolving form_id, fetch the form's published
flag (e.g., frappe.db.get_value("Form", form_id, "published") or the appropriate
field) and if frappe.session.user == "Guest" and the form is not published,
raise a permission error (frappe.throw(..., frappe.PermissionError) or similar).
Keep the check inside get_form_by_route so authenticated users still get
get_form(form_id) but guests can only access forms where published is truthy.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
forms_pro/api/form.py (2)
40-45:⚠️ Potential issue | 🟠 MajorGuest route lookup still exposes unpublished forms.
Line 40 allows guest access, but Line 45 returns full form data without checking publish scope/permissions first. This still permits unauthenticated access to unpublished form metadata.
🔒 Suggested guard
`@frappe.whitelist`(allow_guest=True) # nosemgrep: frappe-semgrep-rules.rules.security.guest-whitelisted-method def get_form_by_route(route: str) -> dict: - form_id = frappe.db.get_value("Form", {"route": route}, pluck="name") - if not form_id: + form_meta = frappe.db.get_value( + "Form", + {"route": route}, + ["name", "is_published"], + as_dict=True, + ) + if not form_meta: frappe.throw(_("Form not found"), frappe.DoesNotExistError) - return get_form(form_id) + + if frappe.session.user == "Guest" and not form_meta.is_published: + frappe.throw(_("You do not have access to this form"), frappe.PermissionError) + + if frappe.session.user != "Guest" and not frappe.has_permission("Form", "read", form_meta.name): + frappe.throw(_("You do not have read access to this form"), frappe.PermissionError) + + return get_form(form_meta.name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/api/form.py` around lines 40 - 45, get_form_by_route currently returns full form data to guests without verifying publish state; before calling get_form(form_id) add a publish/permission guard: after retrieving form_id from frappe.db.get_value, load the Form document (or query for a published flag) and ensure it is published or that the requesting user has permission (e.g., check a Form.published boolean or use frappe.has_permission/permission_query to verify read access for guests); if not published/allowed, raise frappe.DoesNotExistError or frappe.PermissionError so unpublished forms are not exposed to unauthenticated callers.
236-244:⚠️ Potential issue | 🟠 MajorGuest-accessible DocType field enumeration is still unrestricted.
Line 236 is guest-whitelisted, and Lines 238-244 return schema fields for arbitrary doctypes without any publish-scope or read-permission gate. This is an information disclosure risk.
🔒 Suggested guard
`@frappe.whitelist`(allow_guest=True) # nosemgrep: frappe-semgrep-rules.rules.security.guest-whitelisted-method def get_doctype_fields(doctype: str) -> list: + if frappe.session.user == "Guest": + is_public_doctype = frappe.db.exists( + "Form", + {"linked_doctype": doctype, "is_published": 1}, + ) + if not is_public_doctype: + frappe.throw(_("You do not have access to this doctype"), frappe.PermissionError) + elif not frappe.has_permission("DocType", "read", doctype): + frappe.throw(_("You do not have read access to this doctype"), frappe.PermissionError) + doctype = frappe.get_doc("DocType", doctype) fields = [ field for field in doctype.fields if field.fieldtype not in UNSUPPORTED_FRAPPE_FIELDTYPES and field.fieldname not in FORMS_PRO_SYSTEM_FIELDNAMES ] return fields🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forms_pro/api/form.py` around lines 236 - 244, get_doctype_fields is currently guest-whitelisted and returns arbitrary DocType fields; restrict access by checking read/publish scope before enumerating fields. Update get_doctype_fields to: validate the DocType exists, call a permission check such as frappe.has_permission(doctype, "read", user=frappe.session.user) or use the doc's can_read() and also require a published flag or explicit allow-list for guest access; if the check fails return an empty list or raise a PermissionError. Keep references to UNSUPPORTED_FRAPPE_FIELDTYPES and FORMS_PRO_SYSTEM_FIELDNAMES for filtering only after permissions are verified. Ensure guest access is removed or gated behind an explicit published/allow-list check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@forms_pro/api/form.py`:
- Around line 40-45: get_form_by_route currently returns full form data to
guests without verifying publish state; before calling get_form(form_id) add a
publish/permission guard: after retrieving form_id from frappe.db.get_value,
load the Form document (or query for a published flag) and ensure it is
published or that the requesting user has permission (e.g., check a
Form.published boolean or use frappe.has_permission/permission_query to verify
read access for guests); if not published/allowed, raise
frappe.DoesNotExistError or frappe.PermissionError so unpublished forms are not
exposed to unauthenticated callers.
- Around line 236-244: get_doctype_fields is currently guest-whitelisted and
returns arbitrary DocType fields; restrict access by checking read/publish scope
before enumerating fields. Update get_doctype_fields to: validate the DocType
exists, call a permission check such as frappe.has_permission(doctype, "read",
user=frappe.session.user) or use the doc's can_read() and also require a
published flag or explicit allow-list for guest access; if the check fails
return an empty list or raise a PermissionError. Keep references to
UNSUPPORTED_FRAPPE_FIELDTYPES and FORMS_PRO_SYSTEM_FIELDNAMES for filtering only
after permissions are verified. Ensure guest access is removed or gated behind
an explicit published/allow-list check.
3 tasks
4 tasks
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>
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.
What changed and why
Adding a new field type previously required touching many files across the codebase — the frontend field list, the renderer, the submission display, the Python doctype mapping, and more. This PR consolidates all of that into a single source of truth and adds server-side validation that was previously missing.
Changes
Frontend: field type registry (
src/config/fieldTypes.ts)All per-field-type metadata now lives in one place. Adding a new field type means editing one file instead of ~6. The registry stores:
default,inline,description-first,custom)Frontend: auto-generated types as ground truth
The
Fieldtypeenum insrc/types/FormsPro/form_field.types.tsis auto-generated from the doctype JSON. The old hand-writtenFormFieldTypesenum has been removed — it had already drifted (missingMultiselect). All frontend code now importsFieldtypefrom the auto-generated file.Frontend:
FieldRenderersimplifiedFieldRenderer.vuenow branches onlayoutfrom the registry rather than a chain ofv-if fieldtype === "..."checks. A newFieldLabel.vuecomponent was extracted to avoid duplication.Backend: data-driven fieldtype mapping (
form_field.py)The 8-branch
if/elifinto_frappe_field()is replaced with a dictionary lookup — same logic, easier to extend.Backend: shared constants (
utils/constants.py)UNSUPPORTED_FRAPPE_FIELDTYPESandFORMS_PRO_SYSTEM_FIELDNAMESare now defined once and imported wherever needed, derived fromform_generator.pyso they stay in sync automatically.Backend: server-side form validation (
api/submission.py)Required-field and conditional-logic checks now run on the server, mirroring the frontend
conditionals.tslogic. Previously, a direct API call could bypass all required-field validation. Covered by 22 pure-unit tests (no DB required).Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests