feat: manage form pages#8
Conversation
…agement - Added error handling in create_form to log and throw exceptions. - Updated DocType module name to "User Forms". - Implemented permissions management for DocType and Form documents using DocShare. - Added tests to verify DocShare creation with correct permissions for both DocType and Form.
- Introduced a new utility module for date formatting and manipulation. - Added functions for formatting dates, checking if dates are today/past/future, and calculating date differences. - Enhanced user experience with human-readable date formats and relative time displays.
…ation - Introduced a new composable function to sync a reactive value with a URL query parameter. - Supports default values, validation against a list of valid values, and options for URL update behavior. - Enhances user experience by keeping the application state in sync with the URL.
- Added AccessSection, Avatar, DescriptionSection, and ShareAccessModal to the global components interface in Vue. - Enhances component accessibility throughout the application.
- Converted tailwind configuration from JavaScript to TypeScript for improved type safety and maintainability. - Updated import path for the frappe UI preset.
- Introduced a new Avatar component that fetches and displays user information. - Utilizes TypeScript for improved type safety and integrates with the frappe-ui library. - Enhances user interface by providing a tooltip with the user's full name and an avatar image.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds form access sharing: backend APIs and models for shared users and team members; Form generator now creates DocShare and handles permission errors; frontend introduces manage form pages, components (access UI, avatars, modals), stores, utilities, and build/tooling updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant ManageFormPage as ManageForm Page
participant ManageFormStore as useManageForm
participant TeamAPI as Team API
participant ShareAPI as Frappe Share API
participant UserAPI as User API
rect rgb(220,235,255)
User->>Browser: Navigate to /manage/:id
Browser->>ManageFormPage: mount/init with id
ManageFormPage->>ManageFormStore: initialize(formId)
ManageFormStore->>TeamAPI: GET team members (forms_pro.api.team.get_team_members)
ManageFormStore->>ShareAPI: GET shared users (get_form_shared_with)
ShareAPI-->>ManageFormStore: shared list
TeamAPI-->>ManageFormStore: team members
ManageFormStore->>UserAPI: get_user per user
UserAPI-->>ManageFormStore: user basic info
ManageFormStore-->>ManageFormPage: formData + sharedAccessUsers
end
rect rgb(220,255,230)
User->>ManageFormPage: open ShareAccessModal
ManageFormPage->>ManageFormStore: addAccess(userId)
ManageFormStore->>ShareAPI: POST add share (frappe.share.add)
ShareAPI-->>ManageFormStore: success
ManageFormStore->>ShareAPI: reload shared users
ManageFormStore-->>ManageFormPage: update UI / toast
end
rect rgb(255,235,230)
User->>ManageFormPage: remove user -> confirm
ManageFormPage->>ManageFormStore: removeAccess(userEmail)
ManageFormStore->>FormAPI: DELETE remove access (forms_pro.api.form.remove_form_access)
FormAPI-->>ManageFormStore: success
ManageFormStore->>ShareAPI: reload shared users
ManageFormStore-->>ManageFormPage: update UI / toast
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
forms_pro/utils/form_generator.py (1)
148-164: Module name mismatch will cause incorrect counter and potential name collisions.Line 78 sets
module = "User Forms"but line 154 queries formodule: "Forms Pro". This means the count will never reflect newly created DocTypes, causingnext_numberto potentially generate duplicate names.🔎 Proposed fix
def _generate_doctype_name(self) -> str: """ Generate a unique DocType name with format: formspro_XXXXXX_YYYYYY where XXXXXX is a random 6-character string and YYYYYY is a serialized number """ - # Count existing formspro_* DocTypes from Forms Pro module - count = frappe.db.count("DocType", filters={"module": "Forms Pro", "name": ["like", "formspro_%"]}) + # Count existing formspro_* DocTypes from User Forms module + count = frappe.db.count("DocType", filters={"module": "User Forms", "name": ["like", "formspro_%"]}) # Next number is count + 1 next_number = count + 1forms_pro/forms_pro/doctype/fp_team/fp_team.py (1)
53-63: is_team_member will always return False: team_members returns dicts, not emails.The
is_team_membermethod checksuser in self.team_members, butteam_members(line 34) returnslist[GetTeamMembersResponse]—a list of dictionaries with keysfull_name,user_image, and.model_dump()on line 49). Theuser incheck compares a string against dictionaries, which will always evaluate toFalse.Fix
def is_team_member(self, user: str) -> bool: """ Check if a user is a member of the team Args: user: The user email address Returns: bool - True if the user is a member of the team, False otherwise """ - return user in self.team_members + return any(member["email"] == user for member in self.team_members)
🟡 Minor comments (4)
frontend/package.json-16-18 (1)
16-18: Updatefrappe-uito the latest version.The
dayjsversion (^1.11.19) is at the latest stable release, butfrappe-uican be updated from ^0.1.244 to ^0.1.248, which includes the latest patch releases.frontend/src/components/ui/Avatar.vue-1-40 (1)
1-40: Consider adding caching to thegetUserutility to avoid redundant API calls.Each Avatar instance independently fetches user data on mount. When multiple Avatar components render (e.g., in the AccessSection ListView with multiple users), this results in separate API calls for the same data.
Since
frappe-ui'screateResourcesupports client-side caching via acacheoption, add a cache key to thegetUserutility:Example fix
export async function getUser(userId: string) { const userData = createResource({ url: "forms_pro.api.user.get_user", params: { user: userId, }, cache: `user_${userId}`, }); await userData.fetch(); return userData.data; }Alternatively, refactor the component to accept user data as a prop and fetch at the parent level, allowing data to be shared across multiple Avatar instances.
forms_pro/api/team.py-38-38 (1)
38-38: Remove debugDebug print statements should not be committed to production code. Use logging if needed.
🔎 Proposed fix
- print("team_id", team_id)frontend/src/stores/form/manageForm.ts-38-43 (1)
38-43: Owner may not appear in shared access list, causinguserHasShareAccessto be false.The form owner likely has implicit share access but might not be in
formAccessResource.data. Consider also checking if the current user is the owner.🔎 Proposed fix
const userHasShareAccess = computed<boolean>( - () => - formAccessResource.data?.find( - (user: SharedAccessUser) => user.email === sessionUser() - )?.share ?? false + () => { + const currentUser = sessionUser(); + // Owner implicitly has share access + if (formOwner.value === currentUser) return true; + return formAccessResource.data?.find( + (user: SharedAccessUser) => user.email === currentUser + )?.share ?? false; + } );Committable suggestion skipped: line range outside the PR's diff.
🧹 Nitpick comments (18)
pyproject.toml (1)
19-19: LGTM! Repository URL added.The repository URL addition is a good practice for package metadata and helps with discoverability.
Note: GitHub URLs are case-insensitive, but for consistency, you may want to match the casing used in the actual repository URL (the PR shows "BuildWithHussain" with capital letters).
frontend/src/stores/editForm.ts (1)
118-118: Consider retaining trailing commas for modern JavaScript/TypeScript conventions.Trailing commas in object and array literals are considered best practice in modern JavaScript/TypeScript as they produce cleaner diffs and improve consistency. Removing them (lines 118 and 149) goes against common style guides like Airbnb and Prettier defaults.
🔎 Proposed adjustment
Line 118:
- } + },Line 149:
- } + },Also applies to: 149-149
frontend/src/utils/form_fields.ts (1)
103-104: Formatting change looks fine, but duplicate Tailwind class detected.The string reflow is acceptable, but the
editorClasscontainsrounded-btwice, which is redundant.🔎 Proposed fix to remove duplicate class
editorClass: - "bg-surface-white w-full rounded-b form-description border rounded-b min-h-24", + "bg-surface-white w-full rounded-b form-description border min-h-24", fixedMenu: true,frontend/tsconfig.json (1)
23-23: Redundant include entry.The explicit
"src/main.ts"entry is already covered by the"src/**/*.ts"glob pattern. While harmless, it's unnecessary.🔎 Proposed fix to remove redundancy
- "include": ["src/**/*.ts", "src/**/*.d.ts", "src/**/*.tsx", "src/**/*.vue", "src/main.ts"], + "include": ["src/**/*.ts", "src/**/*.d.ts", "src/**/*.tsx", "src/**/*.vue"],frontend/src/utils/user.ts (1)
8-18: Add return type annotation and consider error handling.The
getUserfunction lacks a return type annotation and doesn't handle potential fetch errors. Consider adding a return type and documenting error handling expectations.🔎 Proposed improvements
-export async function getUser(userId: string) { +export async function getUser(userId: string): Promise<GetUserBasicResponse> { const userData = createResource({ url: "forms_pro.api.user.get_user", params: { user: userId, }, }); await userData.fetch(); - return userData.data; + return userData.data as GetUserBasicResponse; }Note: Error handling can be left to the caller, or you can wrap the fetch in a try-catch block depending on your error handling strategy.
frontend/src/components/ui/Avatar.vue (1)
31-40: Consider adding a loading state.The component renders nothing while fetching user data. Adding a loading skeleton would improve the user experience.
🔎 Optional improvement
+const loading = ref(true); + onMounted(async () => { if (props.userId) { - user.value = await getUser(props.userId); + try { + user.value = await getUser(props.userId); + } finally { + loading.value = false; + } } });<template> + <div v-if="loading" class="animate-pulse"> + <div :class="['rounded-full bg-gray-200', `w-${size} h-${size}`]"></div> + </div> <Tooltip v-if="user?.full_name" :text="user?.full_name" placement="bottom"> <FrappeAvatar :size="size" :shape="shape" :image="user?.user_image" :label="user?.full_name" /> </Tooltip> </template>frontend/src/layouts/BaseLayout.vue (1)
36-39: Use a factory function for array/object prop defaults.In Vue, using a mutable literal like
[]as a default can cause shared state across component instances. Use a factory function instead.🔎 Proposed fix
sidebarSections: { type: Array, - default: [], + default: () => [], },forms_pro/utils/form_generator.py (1)
17-24: Consider catching more specific exceptions.Catching bare
Exceptionmakes debugging harder by obscuring the root cause. Consider catching specific Frappe exceptions (e.g.,frappe.ValidationError,frappe.DuplicateEntryError) or at minimum re-raising with the original traceback preserved.Also applies to: 41-48
frontend/src/stores/form/manageForm.ts (1)
69-95: Consider exposing loading state for mutation actions.Creating resources inside functions works, but the loading state isn't exposed to consumers. For better UX (e.g., disabling buttons during submission), consider returning the resource or exposing a shared loading ref.
frontend/src/utils/date.ts (1)
5-11:customParseFormatplugin is imported but not used.The plugin is extended but none of the functions use custom parsing formats. Consider removing it if unused, or document its intended use case.
frontend/src/pages/manage/ManageForm.vue (2)
46-68: Consider adding type definitions for better type safety.The computed property uses
anyforsectionanditemparameters. Consider defining interfaces for sidebar sections and items to improve type safety and IDE support.Example type definitions
interface SidebarItem { label: string; icon?: Component; to?: string; isActive?: boolean; onClick?: () => void; } interface SidebarSection { label?: string; items: SidebarItem[]; } // Then update the computed signature: const sidebarSections = computed<SidebarSection[]>(() => [ // ... ]);
27-29: Consider usingrouter.pushinstead ofrouter.replacefor better navigation history.Using
router.replaceremoves the current entry from the browser history, preventing users from using the back button to return to the manage form view. Consider usingrouter.pushto preserve navigation history.onClick: () => { - router.replace("/"); + router.push("/"); },frontend/src/components/form/manage/RemoveAccessModal.vue (1)
17-26: Add null safety to the dialog message.The message interpolates
selectedUserToRemovedirectly without null checking. While the dialog might not open when the value is null, adding optional chaining improves defensive coding.-message: `Are you sure you want to remove access for ${selectedUserToRemove}?`, +message: `Are you sure you want to remove access for ${selectedUserToRemove.value ?? 'this user'}?`,frontend/src/components/form/manage/DescriptionSection.vue (1)
7-10: Simplify conditional rendering logic.Both the
v-ifand thedivelement check for description existence, but they render in mutually exclusive scenarios. Consider usingv-elsefor clarity.-<p class="prose-sm text-ink-gray-5" v-if="!manageFormStore.formData?.description"> +<p class="prose-sm text-ink-gray-5" v-if="!manageFormStore.formData?.description"> No description added for this form. </p> -<div class="prose-sm mb-6" v-html="manageFormStore.formData?.description"></div> +<div v-else class="prose-sm mb-6" v-html="manageFormStore.formData?.description"></div>frontend/src/composables/useQueryParam.ts (1)
62-68: Avoid spreading entire route object when updating query params.Spreading
...route(lines 62, 102) copies all route properties, which may include internal properties or cause unintended side effects. Use only the necessary properties.const updateMethod = replace ? router.replace : router.push; updateMethod({ - ...route, + path: route.path, + params: route.params, + hash: route.hash, query: { ...route.query, [paramName]: newValue, }, });Also applies to: 102-108
frontend/src/components/form/manage/ShareAccessModal.vue (1)
9-11: MoveteamStore.initialize()toonMountedhook.Calling
initialize()at the module level (line 10) means it runs every time the component is created, even if the store is already initialized. This could cause unnecessary API calls or state resets.+import { computed, ref, onMounted } from "vue"; + const teamStore = useTeam(); -teamStore.initialize(); const manageFormStore = useManageForm(); + +onMounted(() => { + if (!teamStore.isInitialized) { + teamStore.initialize(); + } +});Note: This assumes the store has an
isInitializedflag. If not, consider adding one to prevent redundant initialization.forms_pro/forms_pro/doctype/fp_team/fp_team.py (1)
41-42: Use Pythonic truthiness check instead of len().Checking
if not len(self.users):is less idiomatic thanif not self.users:. Empty lists are falsy in Python.🔎 Simplify the check
- if not len(self.users): + if not self.users: return []frontend/src/components/form/manage/AccessSection.vue (1)
48-52: Add proper types instead of usingany.The
setPermissionfunction usesanyfor parametersrowandcolumn, which loses type safety and IDE support.🔎 Add proper parameter types
-function setPermission(row: any, column: any, value: boolean) { +function setPermission( + row: { email: string; [key: string]: any }, + column: { key: string; [key: string]: any }, + value: boolean +) { const userId = row.email; const permission = column.key as PermissionTypes; manageFormStore.setPermission(userId, permission, value); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (43)
.github/workflows/ci.yml.pre-commit-config.yamlforms_pro/api/form.pyforms_pro/api/team.pyforms_pro/api/user.pyforms_pro/forms_pro/doctype/form/form.pyforms_pro/forms_pro/doctype/fp_team/fp_team.pyforms_pro/modules.txtforms_pro/user_forms/__init__.pyforms_pro/utils/form_generator.pyforms_pro/utils/test_form_generator.pyfrontend/.gitignorefrontend/auto-imports.d.tsfrontend/components.d.tsfrontend/index.htmlfrontend/package.jsonfrontend/src/components/FormBuilderContent.vuefrontend/src/components/dashboard/FormPreviewCard.vuefrontend/src/components/form/manage/AccessSection.vuefrontend/src/components/form/manage/DescriptionSection.vuefrontend/src/components/form/manage/RemoveAccessModal.vuefrontend/src/components/form/manage/ShareAccessModal.vuefrontend/src/components/ui/Avatar.vuefrontend/src/composables/useQueryParam.tsfrontend/src/index.cssfrontend/src/layouts/BaseLayout.vuefrontend/src/main.tsfrontend/src/pages/manage/ManageForm.vuefrontend/src/pages/manage/overview/Overview.vuefrontend/src/router.jsfrontend/src/stores/editForm.tsfrontend/src/stores/form/manageForm.tsfrontend/src/stores/submissionForm.tsfrontend/src/stores/team.tsfrontend/src/stores/user.tsfrontend/src/utils/date.tsfrontend/src/utils/form_fields.tsfrontend/src/utils/form_generator.tsfrontend/src/utils/user.tsfrontend/tailwind.config.jsfrontend/tailwind.config.tsfrontend/tsconfig.jsonpyproject.toml
💤 Files with no reviewable changes (1)
- frontend/tailwind.config.js
🧰 Additional context used
🧬 Code graph analysis (9)
frontend/src/utils/user.ts (1)
forms_pro/api/user.py (1)
GetUserBasicResponse(33-35)
frontend/src/composables/useQueryParam.ts (1)
frontend/src/router.js (1)
router(36-39)
frontend/src/stores/form/manageForm.ts (1)
frontend/src/data/session.js (1)
sessionUser(7-14)
forms_pro/forms_pro/doctype/fp_team/fp_team.py (1)
forms_pro/api/user.py (1)
get_user(39-44)
forms_pro/api/team.py (1)
forms_pro/forms_pro/doctype/fp_team/fp_team.py (3)
FPTeam(18-63)GetTeamMembersResponse(12-15)team_members(34-51)
forms_pro/api/form.py (2)
forms_pro/api/user.py (1)
get_user(39-44)forms_pro/forms_pro/doctype/form/form.py (2)
Form(11-117)shared_with(41-46)
forms_pro/api/user.py (1)
frontend/src/utils/user.ts (1)
GetUserBasicResponse(3-6)
forms_pro/utils/test_form_generator.py (1)
forms_pro/utils/form_generator.py (2)
FormGenerator(56-165)generate(67-70)
frontend/src/stores/team.ts (1)
frontend/src/stores/user.ts (1)
useUser(13-74)
🪛 actionlint (1.7.9)
.github/workflows/ci.yml
52-52: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 GitHub Actions: Linters
forms_pro/api/form.py
[error] 54-54: frappe-missing-translate-function-python: All user facing text must be wrapped in translate function.
[error] 83-83: frappe-missing-translate-function-python: All user facing text must be wrapped in translate function.
[error] 91-91: frappe-missing-translate-function-python: All user facing text must be wrapped in translate function.
🪛 Ruff (0.14.10)
forms_pro/utils/form_generator.py
20-20: Do not catch blind exception: Exception
(BLE001)
44-44: Do not catch blind exception: Exception
(BLE001)
⏰ 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 (40)
frontend/src/utils/form_generator.ts (1)
3-6: LGTM!The trailing comma removal is a minor formatting improvement that aligns with clean code practices. The function signatures and implementation remain correct and functional.
Also applies to: 35-38
frontend/.gitignore (1)
9-9: LGTM!Adding generated type definition files to
.gitignoreis standard practice. You could optionally make this more general (e.g.,src/types/**/*.d.ts) if other generated type files are expected, but being specific works fine.frontend/auto-imports.d.ts (1)
8-8: Whitespace cleanup in auto-generated file.This is a cosmetic change to remove blank lines in the ambient global block of an auto-generated file. Since
auto-imports.d.tsis regenerated by unplugin-auto-import, this formatting may be reset on the next tool run. If tooling respects existing formatting, this cleanup is harmless; otherwise, it may be overwritten and is not worth preserving manually.frontend/src/components/FormBuilderContent.vue (1)
3-3: This review comment is based on incorrect assumptions about the change.The file
FormBuilderContent.vueis newly created, not a modification of existing code. TheButtonimport was never on a deep path in this file—it's been imported directly from the mainfrappe-uipackage from the start. No consolidation or refactoring occurred. TheButtoncomponent usage at lines 86-91 withicon="x"andvariant="outline"props is consistent with howButtonis imported and used throughout the codebase.Likely an incorrect or invalid review comment.
pyproject.toml (3)
4-4: LGTM! Author name correction.The author name update is a straightforward metadata correction.
15-15: No action needed. The pinned version2.3.0is the latest stable version available on PyPI, valid, and free from identified security advisories.
14-14: No issues identified. The pinned versiondnspython==2.8.0is the latest stable release (published September 7, 2025), is available on PyPI with provenance attestations, and has no known security vulnerabilities..github/workflows/ci.yml (2)
49-49: Python 3.14 is released and stable as of January 2026 (3.14.0 released October 2025, with bugfix releases in December 2025). No action needed.Likely an incorrect or invalid review comment.
52-55: Update actions/setup-node to v4 or later.
actions/setup-node@v3is outdated and should be updated. However, as of January 2026, the current major version is v6 (latest v6.1.0 as of December 2025).Node.js 24 is released and actively supported: it was released May 6, 2025, entered LTS in October 2025, and is currently in Active LTS with support through April 30, 2028.
🔎 Proposed fix for the action version
- name: Setup Node - uses: actions/setup-node@v3 + uses: actions/setup-node@v4 with: node-version: 24 check-latest: trueLikely an incorrect or invalid review comment.
frontend/src/index.css (1)
15-38: LGTM! Well-structured typography refactor.The refactor to use Tailwind's
@applydirective with nested typography rules improves maintainability and follows best practices. The semantic hierarchy (h1-h6) with appropriate text sizes is correct, and the new.is-emptystate provides consistent styling for placeholder content.frontend/src/stores/editForm.ts (1)
42-42: The explicitmethod: "GET"specification is valid and already supported by frappe-ui 0.1.244. This pattern is already used consistently in other parts of the codebase (e.g.,manageForm.ts), confirming that the parameter is a supported option forcreateResourcein this version.frontend/src/main.ts (1)
9-21: LGTM! Formatting improvements with no functional changes.The reformatting of imports, object properties, and loop body improves readability without altering behavior.
Also applies to: 26-33, 50-50
forms_pro/modules.txt (1)
1-2: LGTM! Module registration for User Forms feature.The addition of the "User Forms" module aligns with the PR's form management objectives.
frontend/index.html (1)
13-13: Entry point migration to TypeScript is correctly implemented.The change from
main.jstomain.tsis properly completed with:
- main.ts exists and properly imports Vue and Pinia
- main.js has been removed
- TypeScript configuration (tsconfig.json) is present
- Vite configuration supports TypeScript
- TypeScript dependencies are installed
No further action needed.
frontend/src/stores/submissionForm.ts (1)
12-14: LGTM!The formatting cleanup of the
allowIncompleteFormscomputed property is correct and improves code readability.forms_pro/forms_pro/doctype/form/form.py (1)
4-8: LGTM!The added imports (
typing.Anyandfrappe.share.get_users) are appropriate for the newshared_with()method implementation.forms_pro/utils/test_form_generator.py (3)
77-77: LGTM!The module name assertions have been correctly updated to expect
"User Forms"to align with the implementation changes informs_pro/utils/form_generator.py.Also applies to: 136-136
161-186: LGTM!The test correctly verifies that DocShare is created for the DocType with appropriate permissions (read, write, share enabled; submit disabled). The test properly sets user context and validates the expected permission structure.
188-213: LGTM!The test correctly verifies that DocShare is created for the Form document with the same permission structure as the DocType (read, write, share enabled; submit disabled). Consistent test structure with the DocType test.
frontend/tailwind.config.ts (1)
1-17: LGTM!The Tailwind configuration is well-structured. The frappe-ui preset integration and content paths are properly configured for Vue 3 components.
frontend/src/stores/user.ts (1)
7-11: LGTM!Exporting the
UserTeamtype enables its reuse across the codebase, which is a good TypeScript practice.forms_pro/api/user.py (2)
38-44: LGTM!The
get_userfunction properly handles the case when a user is not found and uses Pydantic for validation and serialization.
48-59: LGTM!Renaming to
get_current_userimproves clarity and distinguishes it from the newget_user(user: str)function.forms_pro/api/form.py (3)
9-16: LGTM!The
FormSharedWithResponsemodel is well-structured and properly uses field aliasing for the email field.
85-85: Verify thatignore_permissionsis necessary.The
ignore_permissionsflag bypasses Frappe's permission system. While you've validated write permissions beforehand (line 82), ensure this flag is necessary for theremove()function to work correctly.Please confirm whether the
ignore_permissionsflag is required for thefrappe.share.removefunction or if permissions are properly handled without it.
88-99: LGTM! (after addressing translate wrapper)The
get_doctype_listfunction properly checks permissions and fetches non-table doctypes. The largelimit_page_lengthappears intentional to retrieve all doctypes.frontend/src/layouts/BaseLayout.vue (1)
50-82: LGTM!The computed properties correctly provide fallback values when props are not supplied, enabling flexible sidebar customization while maintaining sensible defaults.
forms_pro/utils/form_generator.py (2)
67-70: LGTM!Adding
frappe.clear_cache()after DocType initialization is appropriate to ensure the newly created DocType is immediately available.
113-125: Theadd_docsharecalls do not explicitly specify auserparameter. In Frappe's implementation, whenuseris omitted, it defaults tofrappe.session.user(the current session user). This is standard Frappe behavior and the code is correct; however, consider whether explicitly passinguser=frappe.session.userwould improve code clarity to make the intent more obvious to future readers.frontend/src/stores/form/manageForm.ts (1)
55-62: LGTM!The initialize function correctly sets up the form resource and triggers the access data fetch. The non-awaited
fetch()is fine here as the reactiveformAccessResource.datawill update the UI when ready.frontend/src/utils/date.ts (2)
47-73: LGTM!The
formatPrettyDatefunction provides excellent progressive date formatting—showing "Today"/"Yesterday" for recent dates, relative time for the past week, and formatted dates for older entries.
144-151: Defaultdate2 = dayjs()is evaluated at function definition, not call time.The default value
dayjs()captures "now" when the module loads, not when the function is called. For long-running apps, this could return stale differences.🔎 Proposed fix
export function getDateDiff( date1: string | Date | dayjs.Dayjs | null | undefined, - date2: string | Date | dayjs.Dayjs | null | undefined = dayjs(), + date2?: string | Date | dayjs.Dayjs | null | undefined, unit: dayjs.ManipulateType = "day" ): number { if (!date1) return 0; - return dayjs(date1).diff(dayjs(date2), unit); + return dayjs(date1).diff(date2 ? dayjs(date2) : dayjs(), unit); }Likely an incorrect or invalid review comment.
frontend/src/components/dashboard/FormPreviewCard.vue (1)
18-19: LGTM!Navigation correctly updated to point to the new "Manage Form" route, aligning with the routing changes in this PR.
frontend/src/router.js (1)
11-23: LGTM!The nested route structure is well-designed with automatic redirect to the overview child route. Lazy loading the components is appropriate for code splitting.
forms_pro/api/team.py (1)
23-43: LGTM!The endpoint correctly checks permissions before returning team members. The type annotations and docstring provide good documentation.
frontend/src/components/form/manage/ShareAccessModal.vue (1)
100-102: Rename Avatar prop fromuserIdtouserEmailfor clarity.The Avatar component's
userIdprop is intentionally designed to receive email addresses, as evidenced by AccessSection.vue (which explicitly uses:userId="row.email"), and ShareAccessModal.vue (which passesitem.valuecontaining member emails). The prop name should beuserEmailto reflect what it actually accepts.frontend/src/pages/manage/overview/Overview.vue (1)
12-29: Excellent type-safe tab configuration.The pattern of extracting
TabValuefrom the tabs array ensures compile-time validation of tab values, preventing typos in conditional checks.frontend/src/stores/team.ts (1)
16-19: Good defensive programming with Array.isArray check.The fallback to empty array prevents runtime errors if the API returns unexpected data.
frontend/components.d.ts (1)
6-31: Auto-generated component declarations look correct.The new component type declarations (AccessSection, Avatar, DescriptionSection, RemoveAccessModal, ShareAccessModal) properly align with the components introduced in this PR.
frontend/src/components/form/manage/AccessSection.vue (1)
88-109: Well-implemented owner protection logic.The UI correctly prevents removing the owner and provides clear visual feedback through disabled buttons and tooltips. The consistent checks across multiple interaction points (removal button, checkboxes) prevent accidental permission changes.
| hooks: | ||
| - id: prettier | ||
| types_or: [javascript, vue, scss] | ||
| types_or: [javascript, vue, scss, ts] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the .pre-commit-config.yaml file around the mentioned lines
cat -n .pre-commit-config.yaml | sed -n '25,55p'Repository: BuildWithHussain/forms_pro
Length of output: 1160
🏁 Script executed:
#!/bin/bash
# Check if TypeScript configuration files exist
echo "=== Checking for TypeScript configuration files ==="
ls -la tsconfig.json 2>&1
ls -la .eslintrc* 2>&1
ls -la eslint.config.* 2>&1
echo -e "\n=== Checking for TypeScript in package.json ==="
if [ -f "package.json" ]; then
cat package.json | jq '.devDependencies // {} | to_entries[] | select(.key | contains("typescript") or contains("eslint")) | "\(.key): \(.value)"'
fiRepository: BuildWithHussain/forms_pro
Length of output: 353
🏁 Script executed:
#!/bin/bash
# Check the overall structure of .pre-commit-config.yaml
echo "=== Full .pre-commit-config.yaml content ==="
cat .pre-commit-config.yamlRepository: BuildWithHussain/forms_pro
Length of output: 1781
Add tsconfig.json before enabling TypeScript support in pre-commit hooks.
The hook changes themselves are syntactically correct, but verification reveals that tsconfig.json is missing from the project. Adding TypeScript file processing to prettier and eslint without a TypeScript configuration will lead to inconsistent behavior and potential build failures. Create a tsconfig.json in the repository root before these changes take effect. Alternatively, confirm whether TypeScript is actually intended to be used in this project—if not, these hook changes should not be merged.
| for user in shared_with: | ||
| _user = get_user(user["user"]) | ||
| user.update(_user) | ||
| shared_with_responses.append(FormSharedWithResponse.model_validate(user).model_dump()) |
There was a problem hiding this comment.
Handle None return value from get_user.
The get_user function can return None (as defined in forms_pro/api/user.py lines 38-44), but this code calls user.update(_user) without checking. This will cause a TypeError when trying to update with None.
🔎 Proposed fix
for user in shared_with:
_user = get_user(user["user"])
- user.update(_user)
+ if _user:
+ user.update(_user)
shared_with_responses.append(FormSharedWithResponse.model_validate(user).model_dump())📝 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.
| for user in shared_with: | |
| _user = get_user(user["user"]) | |
| user.update(_user) | |
| shared_with_responses.append(FormSharedWithResponse.model_validate(user).model_dump()) | |
| for user in shared_with: | |
| _user = get_user(user["user"]) | |
| if _user: | |
| user.update(_user) | |
| shared_with_responses.append(FormSharedWithResponse.model_validate(user).model_dump()) |
🤖 Prompt for AI Agents
In forms_pro/api/form.py around lines 61 to 64, the code calls
user.update(_user) but get_user(...) can return None causing a TypeError; change
the logic to check if _user is None before calling update — if _user is None
either skip that shared_with entry (optionally log a warning with the user id)
or populate required fallback/default fields so
FormSharedWithResponse.model_validate receives a dict, then only append when you
have a non-None dict to pass to model_validate.
| class GetUserBasicResponse(BaseModel): | ||
| full_name: str | ||
| user_image: str | None = None |
There was a problem hiding this comment.
Critical type mismatch: user_image nullability inconsistency.
The Python model defines user_image as str | None, but the frontend type in frontend/src/utils/user.ts defines it as a required string without null handling:
export type GetUserBasicResponse = {
full_name: string;
user_image: string; // ❌ Not nullable
};This mismatch will cause TypeScript type errors when user_image is None.
🔎 Fix the frontend type definition
Update frontend/src/utils/user.ts to match the backend model:
export type GetUserBasicResponse = {
full_name: string;
- user_image: string;
+ user_image: string | null;
};🤖 Prompt for AI Agents
In forms_pro/api/user.py around lines 33 to 35 the Pydantic model declares
user_image as nullable (str | None) but the frontend type in
frontend/src/utils/user.ts marks it as a required string; update the frontend
type to match the backend by changing GetUserBasicResponse.user_image to allow
null (e.g., string | null or string | undefined) or make it optional, then
rebuild/typecheck the frontend to ensure no TypeScript errors remain.
| @frappe.whitelist() | ||
| def shared_with(self) -> list[dict[str, Any]]: | ||
| """ | ||
| Get list of users with which this form is shared | ||
| """ | ||
| users_shared_with = get_users(self.doctype, self.name) | ||
| return users_shared_with |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for frappe.share.get_users usage patterns and permission checks
rg -n -A5 -B5 'frappe\.share\.get_users|from frappe\.share import get_users' --type=pyRepository: BuildWithHussain/forms_pro
Length of output: 794
🏁 Script executed:
# Search for other whitelisted methods that check permissions
rg -n -A10 '@frappe\.whitelist\(\)' --type=py | head -100Repository: BuildWithHussain/forms_pro
Length of output: 5439
🏁 Script executed:
# Examine the specific file
cat -n forms_pro/forms_pro/doctype/form/form.py | head -60Repository: BuildWithHussain/forms_pro
Length of output: 2195
🏁 Script executed:
# Search for permission checks in whitelisted methods more broadly
rg -B2 -A8 '@frappe\.whitelist\(\)' --type=py | rg -B8 -A2 'check_perm|has_perm|get_permission' | head -80Repository: BuildWithHussain/forms_pro
Length of output: 1617
🏁 Script executed:
# Search for frappe.share documentation or get_users usage in similar contexts
rg -n 'get_users' --type=py -B3 -A3 | head -50Repository: BuildWithHussain/forms_pro
Length of output: 1023
🏁 Script executed:
# Check if there are other document methods with whitelist that don't check permissions
rg -B2 -A8 '@frappe\.whitelist\(\)' --type=py forms_pro/forms_pro/doctype/ -A8Repository: BuildWithHussain/forms_pro
Length of output: 881
🏁 Script executed:
# Look at the full get_form_shared_with implementation for comparison
sed -n '43,68p' forms_pro/api/form.pyRepository: BuildWithHussain/forms_pro
Length of output: 782
🏁 Script executed:
# Search for any comments or docstrings about frappe.whitelist() permission handling
rg -B5 -A5 'frappe.whitelist' --type=py | rg -B5 -A5 'permission|auth|check' | head -60Repository: BuildWithHussain/forms_pro
Length of output: 2856
🏁 Script executed:
# Check if there are any permission decorators or checks in Frappe that might auto-validate
rg -n 'allow_guest|check_permission|require_login' --type=py | head -20Repository: BuildWithHussain/forms_pro
Length of output: 257
🏁 Script executed:
# Look for the actual Form doctype definition to see if there are document-level permission rules
fd -name '*.json' forms_pro/forms_pro/doctype/formRepository: BuildWithHussain/forms_pro
Length of output: 300
🏁 Script executed:
# Find the Form doctype JSON definition
fd form.json forms_pro/forms_pro/doctype/form/Repository: BuildWithHussain/forms_pro
Length of output: 112
🏁 Script executed:
# Check the JSON file if it exists
cat forms_pro/forms_pro/doctype/form/form.json 2>/dev/null | head -100Repository: BuildWithHussain/forms_pro
Length of output: 1931
🏁 Script executed:
# Check if there are permissions defined in the doctype JSON for the shared_with method
cat forms_pro/forms_pro/doctype/form/form.json | grep -A50 'permission' 2>/dev/null | head -80Repository: BuildWithHussain/forms_pro
Length of output: 357
🏁 Script executed:
# Examine whether Frappe automatically enforces permissions on document methods
rg -n 'def shared_with' -A20 forms_pro/forms_pro/doctype/form/form.pyRepository: BuildWithHussain/forms_pro
Length of output: 849
Add explicit permission validation before returning sensitive sharing information.
The shared_with() whitelisted method returns sensitive form sharing data without authorization checks. While a similar API endpoint (get_form_shared_with() in forms_pro/api/form.py) correctly validates read access with frappe.has_permission("Form", "read", form_id), this document method is exposed directly without checks. Add the same permission validation to prevent unauthorized access to form sharing lists:
@frappe.whitelist()
def shared_with(self) -> list[dict[str, Any]]:
"""
Get list of users with which this form is shared
"""
if not frappe.has_permission("Form", "read", self.name):
frappe.throw(_("You do not have read access to this form"))
users_shared_with = get_users(self.doctype, self.name)
return users_shared_with🤖 Prompt for AI Agents
In forms_pro/forms_pro/doctype/form/form.py around lines 40 to 46, the
whitelisted shared_with method returns sharing details without authorization;
add an explicit read-permission check using frappe.has_permission("Form",
"read", self.name) and raise a permission error (frappe.throw with an
appropriate message) when the check fails before calling get_users, so only
authorized users can retrieve the shared-with list.
| const sidebarSections = ref<any[]>([ | ||
| { | ||
| items: [ | ||
| { | ||
| label: "Go Back", | ||
| icon: ArrowLeft, | ||
| onClick: () => { | ||
| router.replace("/"); | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| label: "", | ||
| items: [ | ||
| { | ||
| label: "Overview", | ||
| to: `/manage/${manageFormStore.currentFormId}/overview`, | ||
| isActive: true, | ||
| icon: LayoutGrid, | ||
| }, | ||
| ], | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
Convert to computed property and fix hardcoded isActive.
The sidebarSections is defined as a ref with static data, and isActive: true on line 39 is hardcoded. This means the Overview item will always appear active regardless of the actual route. Additionally, the sidebar won't react to changes in currentFormId.
🔎 Proposed fix
-const sidebarSections = ref<any[]>([
+const sidebarSections = computed(() => [
{
items: [
{
label: "Go Back",
icon: ArrowLeft,
onClick: () => {
router.replace("/");
},
},
],
},
{
label: "",
items: [
{
label: "Overview",
to: `/manage/${manageFormStore.currentFormId}/overview`,
- isActive: true,
+ isActive: route.path === `/manage/${manageFormStore.currentFormId}/overview`,
icon: LayoutGrid,
},
],
},
-]);
+]);📝 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.
| const sidebarSections = ref<any[]>([ | |
| { | |
| items: [ | |
| { | |
| label: "Go Back", | |
| icon: ArrowLeft, | |
| onClick: () => { | |
| router.replace("/"); | |
| }, | |
| }, | |
| ], | |
| }, | |
| { | |
| label: "", | |
| items: [ | |
| { | |
| label: "Overview", | |
| to: `/manage/${manageFormStore.currentFormId}/overview`, | |
| isActive: true, | |
| icon: LayoutGrid, | |
| }, | |
| ], | |
| }, | |
| ]); | |
| const sidebarSections = computed(() => [ | |
| { | |
| items: [ | |
| { | |
| label: "Go Back", | |
| icon: ArrowLeft, | |
| onClick: () => { | |
| router.replace("/"); | |
| }, | |
| }, | |
| ], | |
| }, | |
| { | |
| label: "", | |
| items: [ | |
| { | |
| label: "Overview", | |
| to: `/manage/${manageFormStore.currentFormId}/overview`, | |
| isActive: route.path === `/manage/${manageFormStore.currentFormId}/overview`, | |
| icon: LayoutGrid, | |
| }, | |
| ], | |
| }, | |
| ]); |
🤖 Prompt for AI Agents
In frontend/src/pages/manage/ManageForm.vue around lines 21 to 44,
sidebarSections is a ref with static data and has isActive hardcoded to true;
change it to a computed property that reads manageFormStore.currentFormId and
router.currentRoute (or route.path/name) so the overview link uses the
up-to-date formId and computes isActive dynamically (e.g., compare currentRoute
to the overview path or route name). Return the sections array from the computed
and remove the ref usage so the sidebar updates reactively when currentFormId or
the route changes.
| <script setup lang="ts"> | ||
| import AccessSection from "@/components/form/manage/AccessSection.vue"; | ||
| import { useManageForm } from "@/stores/form/manageForm"; | ||
| import { FileText, CaseLower, Lock } from "lucide-vue-next"; | ||
| import { formatPrettyDate } from "@/utils/date"; | ||
| import { TabButtons, LoadingText } from "frappe-ui"; | ||
| import Avatar from "@/components/ui/Avatar.vue"; | ||
| import { useQueryParam } from "@/composables/useQueryParam"; | ||
|
|
There was a problem hiding this comment.
Missing imports for Button and DescriptionSection.
The template uses Button (line 43) and DescriptionSection (line 67), but neither component is imported in the script section.
🔎 Add missing imports
<script setup lang="ts">
import AccessSection from "@/components/form/manage/AccessSection.vue";
+import DescriptionSection from "@/components/form/manage/DescriptionSection.vue";
import { useManageForm } from "@/stores/form/manageForm";
import { FileText, CaseLower, Lock } from "lucide-vue-next";
import { formatPrettyDate } from "@/utils/date";
-import { TabButtons, LoadingText } from "frappe-ui";
+import { TabButtons, LoadingText, Button } from "frappe-ui";
import Avatar from "@/components/ui/Avatar.vue";
import { useQueryParam } from "@/composables/useQueryParam";🤖 Prompt for AI Agents
In frontend/src/pages/manage/overview/Overview.vue lines 1-9, the template
references Button (line 43) and DescriptionSection (line 67) but they are not
imported; add imports for Button and DescriptionSection at the top of the script
setup using the appropriate component paths (e.g. import Button from
"@/components/ui/Button.vue" and import DescriptionSection from
"@/components/form/manage/DescriptionSection.vue") so the components are
available to the template.
| import { computed } from "vue"; | ||
| import { useCall } from "frappe-ui"; | ||
|
|
||
| const user = useUser(); |
There was a problem hiding this comment.
Move store initialization inside the factory function.
Calling useUser() at module level (outside defineStore) breaks Pinia's reactivity system and can lead to circular dependency issues or stale references.
🔎 Refactor to call useUser inside the store
-const user = useUser();
-
export const useTeam = defineStore("team", () => {
+ const user = useUser();
const currentTeam = computed(() => user.currentTeam);📝 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.
| const user = useUser(); | |
| export const useTeam = defineStore("team", () => { | |
| const user = useUser(); | |
| const currentTeam = computed(() => user.currentTeam); |
🤖 Prompt for AI Agents
In frontend/src/stores/team.ts around line 6, the module-level call const user =
useUser() must be removed and moved inside the store factory (the function
passed to defineStore) so Pinia reactivity and dependency order are preserved;
update the store to call useUser() within the setup/return scope (or inside
actions/getters) so the user ref is created when the store is initialized rather
than at module import time, and replace any top-level references with the new
in-store user variable.
|
|
||
| const userResource = createResource({ | ||
| url: "forms_pro.api.user.get_user", | ||
| url: "forms_pro.api.user.get_current_user", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "forms_pro\.api\.user\.get_user[^_]" --type=ts --type=js --type=vueRepository: BuildWithHussain/forms_pro
Length of output: 97
🏁 Script executed:
rg -n "forms_pro\.api\.user\.get_user[^_]" --type=ts --type=jsRepository: BuildWithHussain/forms_pro
Length of output: 139
🏁 Script executed:
# Also search for any references to get_user in the codebase more broadly
rg -n "get_user" --type=ts --type=js -A 2 -B 2Repository: BuildWithHussain/forms_pro
Length of output: 632
🏁 Script executed:
cat -n frontend/src/stores/user.ts | head -30Repository: BuildWithHussain/forms_pro
Length of output: 1212
Update the endpoint reference in frontend/src/utils/user.ts to match the renamed backend API.
The endpoint rename to forms_pro.api.user.get_current_user is correct in stores/user.ts, but frontend/src/utils/user.ts:10 still references the old endpoint name forms_pro.api.user.get_user. Both files must be updated consistently to align with the backend API rename.
🤖 Prompt for AI Agents
In frontend/src/utils/user.ts around line 10, update the API endpoint string
from "forms_pro.api.user.get_user" to the renamed backend endpoint
"forms_pro.api.user.get_current_user" so it matches frontend/src/stores/user.ts;
search for any other occurrences of the old endpoint in this file and replace
them, then rebuild/test the frontend to verify no runtime API errors.
| export type GetUserBasicResponse = { | ||
| full_name: string; | ||
| user_image: string; | ||
| }; |
There was a problem hiding this comment.
Fix type mismatch for user_image field.
The user_image field is typed as string, but the server-side GetUserBasicResponse model (forms_pro/api/user.py) defines it as str | None. This mismatch could lead to runtime errors when the backend returns null.
🔎 Proposed fix
export type GetUserBasicResponse = {
full_name: string;
- user_image: string;
+ user_image: string | null;
};📝 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.
| export type GetUserBasicResponse = { | |
| full_name: string; | |
| user_image: string; | |
| }; | |
| export type GetUserBasicResponse = { | |
| full_name: string; | |
| user_image: string | null; | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/utils/user.ts around lines 3 to 6, the GetUserBasicResponse type
declares user_image as string but the backend can return null; change the
user_image type to allow null (e.g., string | null) and update any call sites or
consumers to handle a possible null value (fallback logic or conditional
rendering) to prevent runtime errors.
|
Add screenshot / demo? 👀 |
2 participants
Summary by CodeRabbit
New Features
Dependencies
Chores
✏️ Tip: You can customize this high-level summary in your review settings.