feat:support link fields #12
Conversation
📝 WalkthroughWalkthroughThis PR adds support for the Link field type across the forms application stack. It introduces a new guest-accessible API endpoint to fetch link field options, updates the Form Field DocType configuration and type hints to include Link as a fieldtype option, and implements reactive option loading in the frontend to dynamically fetch and render link field options. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant FE as FieldRenderer.vue
participant API as forms_pro.api.form
participant DB as Database
User->>FE: Render form with Link field
FE->>FE: Mount component / Watch fieldtype/options change
FE->>API: getOptions(doctype, filters, page_length)
Note over API: Resolve title field from doctype meta
API->>DB: Query documents (value, label fields)
DB-->>API: Return document list
API-->>FE: Return formatted options list
FE->>FE: Update selectOptions ref
FE->>User: Display Link field with populated options
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In @forms_pro/api/form.py:
- Around line 63-78: get_link_field_options currently exposes any doctype to
unauthenticated callers because of @frappe.whitelist(allow_guest=True); remove
or disable allow_guest and enforce permission checks inside
get_link_field_options by verifying the caller has read access with
frappe.has_permission(doctype, "read", user) (or frappe.has_permission for each
record if needed) before calling frappe.get_all, and if guest access must be
kept implement a strict allowlist of permitted doctypes and validate the
requested doctype against that list; ensure the function raises/returns an error
when permission fails and do not return results for unauthorized doctypes.
- Around line 64-78: The return type annotation on get_link_field_options is
wrong: frappe.get_all with fields returns a list of dicts (each with "value" and
"label"), not list[str]; update the function signature's return type from
list[str] to list[dict[str, str]] (or list[dict]) and adjust any related typing
imports or docstring if present to reflect that the function returns a list of
dictionaries with "value" and "label".
In @frontend/src/components/builder/FieldRenderer.vue:
- Around line 95-106: The component is calling loadOptions twice on mount
(watcher with immediate: true and onMounted), causing duplicate API calls;
remove the redundant onMounted(() => { loadOptions(); }) block so the watcher
(watch(() => [fieldData.value.fieldtype, fieldData.value.options], () => {
loadOptions(); }, { immediate: true })) uniquely handles initial and subsequent
loads for loadOptions tied to fieldData changes.
🧹 Nitpick comments (5)
forms_pro/api/form.py (1)
63-78: Add error handling and documentation.The function lacks error handling for invalid inputs and has no docstring. Consider adding:
- Validation for the doctype parameter
- Error handling if the title_field doesn't exist
- Input sanitization for the filters parameter
- Documentation explaining the function's purpose and parameters
📚 Suggested improvements
@frappe.whitelist(allow_guest=True) def get_link_field_options( doctype: str, filters: dict | None = None, page_length: int = 20, ) -> list[str]: + """ + Fetch options for a Link field from the specified doctype. + + Args: + doctype: The doctype to fetch options from + filters: Optional filters to apply to the query + page_length: Maximum number of options to return + + Returns: + List of dictionaries with 'value' and 'label' keys + """ + # Validate doctype exists + if not frappe.db.exists("DocType", doctype): + frappe.throw(_("DocType {0} does not exist").format(doctype)) + meta = frappe.get_meta(doctype) title_field = meta.title_field or "name" + + # Validate title_field exists in doctype + if title_field != "name" and not meta.has_field(title_field): + title_field = "name"frontend/src/components/builder/FieldRenderer.vue (4)
6-8: Remove unused import and initialization.The
useSubmissionFormstore is imported and initialized but never used in this component.🧹 Remove unused code
-import { createResource } from "frappe-ui"; -import { useSubmissionForm } from "@/stores/submissionForm"; - -const submissionFormStore = useSubmissionForm(); +import { createResource } from "frappe-ui";
72-84: Add error handling for API failures.The API call to fetch Link field options has no error handling. If the request fails, it could leave the component in an inconsistent state or crash.
🛡️ Add error handling
if (fieldData.value.fieldtype === "Link") { const _options = createResource({ url: "forms_pro.api.form.get_link_field_options", makeParams: () => { return { doctype: fieldData.value.options, filters: {}, page_length: 999, }; }, }); - await _options.fetch(); - return _options.data; + try { + await _options.fetch(); + return _options.data || []; + } catch (error) { + console.error("Failed to fetch link options:", error); + return []; + } }
74-79: Consider pagination or search for large datasets.The hard-coded
page_length: 999may be insufficient for doctypes with thousands of records and could cause performance issues. Consider implementing:
- A higher page limit or no limit for comprehensive options
- Search/autocomplete functionality for large datasets
- Lazy loading with infinite scroll
77-77: Consider exposing the filters parameter.The
filtersparameter is always passed as an empty object, but the backend API supports filtering. Consider exposing this as a prop to enable contextual filtering of Link field options based on form state or other criteria.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
forms_pro/api/form.pyforms_pro/forms_pro/doctype/form_field/form_field.jsonforms_pro/forms_pro/doctype/form_field/form_field.pyfrontend/src/components/builder/FieldRenderer.vuefrontend/src/utils/form_fields.ts
🧰 Additional context used
🪛 Ruff (0.14.10)
forms_pro/forms_pro/doctype/form_field/form_field.py
33-33: Undefined name Link
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (3)
forms_pro/forms_pro/doctype/form_field/form_field.py (1)
33-33: LGTM! Static analysis warning is a false positive.The addition of "Link" to the fieldtype Literal is correct. The static analysis warning about an undefined name can be safely ignored—"Link" is a string literal within the TYPE_CHECKING block, not a reference to a class.
Note that
to_frappe_fielddoesn't include special handling for "Link", which is appropriate since Link is a native Frappe fieldtype that doesn't require transformation.forms_pro/forms_pro/doctype/form_field/form_field.json (1)
38-38: LGTM!The addition of "Link" to the fieldtype options correctly extends the Form Field DocType to support the new field type.
frontend/src/utils/form_fields.ts (1)
132-132: LGTM!Reusing the SelectField configuration for Link fields is appropriate since Link fields render as dropdowns with dynamically loaded options.
| @frappe.whitelist(allow_guest=True) | ||
| def get_link_field_options( | ||
| doctype: str, | ||
| filters: dict | None = None, | ||
| page_length: int = 20, | ||
| ) -> list[str]: | ||
| meta = frappe.get_meta(doctype) | ||
| title_field = meta.title_field or "name" | ||
|
|
||
| results = frappe.get_all( | ||
| doctype=doctype, | ||
| filters=filters or {}, | ||
| page_length=page_length, | ||
| fields=["name as value", f"{title_field} as label"], | ||
| ) | ||
| return results |
There was a problem hiding this comment.
CRITICAL: Guest access without permission checks exposes sensitive data.
This endpoint allows unauthenticated users to query any doctype without permission validation. This is a serious security vulnerability that could expose:
- Sensitive business data (customers, invoices, employees, etc.)
- System metadata useful for reconnaissance attacks
- Data that should be restricted by Frappe's permission system
🔒 Recommended security fixes
Option 1: Remove guest access and add permission checks (recommended)
-@frappe.whitelist(allow_guest=True)
+@frappe.whitelist()
def get_link_field_options(
doctype: str,
filters: dict | None = None,
page_length: int = 20,
) -> list[str]:
+ # Validate user has read permission for the doctype
+ if not frappe.has_permission(doctype, "read"):
+ frappe.throw(_("You do not have permission to access this doctype"))
+
meta = frappe.get_meta(doctype)
title_field = meta.title_field or "name"Option 2: If guest access is required, implement a whitelist of allowed doctypes
@frappe.whitelist(allow_guest=True)
def get_link_field_options(
doctype: str,
filters: dict | None = None,
page_length: int = 20,
) -> list[str]:
+ # Only allow specific doctypes for guest access
+ ALLOWED_DOCTYPES = ["Country", "State", "City"] # Add safe doctypes here
+ if doctype not in ALLOWED_DOCTYPES:
+ frappe.throw(_("Access to this doctype is not allowed"))
+
meta = frappe.get_meta(doctype)
title_field = meta.title_field or "name"🤖 Prompt for AI Agents
In @forms_pro/api/form.py around lines 63 - 78, get_link_field_options currently
exposes any doctype to unauthenticated callers because of
@frappe.whitelist(allow_guest=True); remove or disable allow_guest and enforce
permission checks inside get_link_field_options by verifying the caller has read
access with frappe.has_permission(doctype, "read", user) (or
frappe.has_permission for each record if needed) before calling frappe.get_all,
and if guest access must be kept implement a strict allowlist of permitted
doctypes and validate the requested doctype against that list; ensure the
function raises/returns an error when permission fails and do not return results
for unauthorized doctypes.
| def get_link_field_options( | ||
| doctype: str, | ||
| filters: dict | None = None, | ||
| page_length: int = 20, | ||
| ) -> list[str]: | ||
| meta = frappe.get_meta(doctype) | ||
| title_field = meta.title_field or "name" | ||
|
|
||
| results = frappe.get_all( | ||
| doctype=doctype, | ||
| filters=filters or {}, | ||
| page_length=page_length, | ||
| fields=["name as value", f"{title_field} as label"], | ||
| ) | ||
| return results |
There was a problem hiding this comment.
Incorrect return type annotation.
The function is annotated to return list[str] but actually returns list[dict] since frappe.get_all with the fields parameter returns dictionaries with "value" and "label" keys.
📝 Fix the return type
+from typing import TypedDict
+
+class LinkOption(TypedDict):
+ value: str
+ label: str
+
@frappe.whitelist(allow_guest=True)
def get_link_field_options(
doctype: str,
filters: dict | None = None,
page_length: int = 20,
-) -> list[str]:
+) -> list[LinkOption]:Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @forms_pro/api/form.py around lines 64 - 78, The return type annotation on
get_link_field_options is wrong: frappe.get_all with fields returns a list of
dicts (each with "value" and "label"), not list[str]; update the function
signature's return type from list[str] to list[dict[str, str]] (or list[dict])
and adjust any related typing imports or docstring if present to reflect that
the function returns a list of dictionaries with "value" and "label".
| watch( | ||
| () => [fieldData.value.fieldtype, fieldData.value.options], | ||
| () => { | ||
| loadOptions(); | ||
| }, | ||
| { immediate: true } | ||
| ); | ||
| // Also load on mount | ||
| onMounted(() => { | ||
| loadOptions(); | ||
| }); |
There was a problem hiding this comment.
Duplicate option loading on mount.
The loadOptions function is called twice when the component mounts: once by the watcher with immediate: true and once by onMounted. This results in unnecessary duplicate API calls for Link fields.
⚡ Remove duplicate call
// Watch for changes to field type or options
watch(
() => [fieldData.value.fieldtype, fieldData.value.options],
() => {
loadOptions();
},
{ immediate: true }
);
-
-// Also load on mount
-onMounted(() => {
- loadOptions();
-});The watcher with immediate: true already handles the initial load, so onMounted is redundant.
📝 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.
| watch( | |
| () => [fieldData.value.fieldtype, fieldData.value.options], | |
| () => { | |
| loadOptions(); | |
| }, | |
| { immediate: true } | |
| ); | |
| // Also load on mount | |
| onMounted(() => { | |
| loadOptions(); | |
| }); | |
| watch( | |
| () => [fieldData.value.fieldtype, fieldData.value.options], | |
| () => { | |
| loadOptions(); | |
| }, | |
| { immediate: true } | |
| ); |
🤖 Prompt for AI Agents
In @frontend/src/components/builder/FieldRenderer.vue around lines 95 - 106, The
component is calling loadOptions twice on mount (watcher with immediate: true
and onMounted), causing duplicate API calls; remove the redundant onMounted(()
=> { loadOptions(); }) block so the watcher (watch(() =>
[fieldData.value.fieldtype, fieldData.value.options], () => { loadOptions(); },
{ immediate: true })) uniquely handles initial and subsequent loads for
loadOptions tied to fieldData changes.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.