feat: teams v1 #5
Conversation
* add user retrieval and user teams functionality in user.py * enhance team retrieval with ordering in teams.py * create user store in frontend for managing user data and theme preferences * update theme utility to support setting specific themes
* introduce linked_team_id as a required link field in form.json * update Form class in form.py to include linked_team_id attribute * modify metadata timestamp for recent changes
WalkthroughAdds team-scoped forms: backend APIs and Pydantic schemas for user, teams, and team-forms; Form DocType and generator threaded with linked_team_id; FPTeam helpers and team utilities for current-team persistence; frontend Pinia user store, Dashboard refactor to team-scoped flows, theme API changes, and test/install setup. Changes
Sequence DiagramsequenceDiagram
participant App as App.vue
participant Store as useUser (Pinia)
participant Dashboard as Dashboard.vue
participant API as Backend API
participant DB as Database / DocType
App->>Store: initialize()
activate Store
Store->>API: get_user()
Store->>API: get_user_teams()
API-->>Store: user + teams (with is_current)
Store->>Store: set currentTeam
deactivate Store
Dashboard->>Store: read currentTeam
Dashboard->>API: get_team_forms(team_id)
API->>DB: query Forms by linked_team_id
DB-->>API: forms list
API-->>Dashboard: list[GetTeamFormsResponseSchema]
Dashboard->>Dashboard: render teamForms
Dashboard->>API: create_form_with_doctype(team_id, doctype,...)
API->>DB: insert Form with linked_team_id
DB-->>API: created form
API-->>Dashboard: created form
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/pages/Dashboard.vue (1)
12-17: Add missingdoctypesresource definition.The
Comboboxon line 14 referencesdoctypes.data, but nodoctypesresource is defined in the script. This will cause a runtime error when opening the "Create from Existing DocType" dialog. Add a resource definition using the backend API endpointforms_pro.api.form.get_doctype_list:const doctypes = createResource({ url: "forms_pro.api.form.get_doctype_list", }); onMounted(() => { teamForms.fetch(); doctypes.fetch(); });Also define
selectedDoctypeas a ref to store the selected value.
🧹 Nitpick comments (7)
frontend/src/utils/theme.ts (1)
3-8: Consider removing debug console.log statements.The function contains two
console.logstatements that appear to be for debugging. Consider removing these before merging to production.🔎 Proposed cleanup
export default function setTheme(theme: ThemePreferenceType) { - console.log("setTheme", theme); const root = document.documentElement; root.setAttribute("data-theme", theme.toLowerCase()); - console.log("root", root.getAttribute("data-theme")); }frontend/src/App.vue (1)
4-7: Consider moving store initialization to a lifecycle hook.Calling
userStore.initialize()directly in the script setup top-level executes immediately during component creation. This could cause issues with SSR, race conditions with router initialization, or make error handling more difficult. Consider wrapping it inonMountedfor more predictable initialization timing.🔎 Proposed refactor
<script setup lang="ts"> import "vue-sonner/style.css"; import { Toaster } from "vue-sonner"; import { useUser } from "@/stores/user"; +import { onMounted } from "vue"; const userStore = useUser(); -userStore.initialize(); + +onMounted(() => { + userStore.initialize(); +}); </script>frontend/src/layouts/BaseLayout.vue (1)
28-29: Unused store instance.
userStoreis declared but never used in this component. If the store is intended for the commented-out "Switch Teams" feature, consider removing it until the feature is implemented.import { useUser } from "@/stores/user"; -const userStore = useUser();frontend/src/stores/user.ts (2)
50-62: Redundantawaitwith.then()chaining.Using both
awaitand.then()is redundant. Consider using one or the other for cleaner async flow:🔎 Suggested refactor
async function toggleThemePreference(theme: ThemePreferenceType) { setTheme(theme); const switchTheme = createResource({ url: "frappe.core.doctype.user.user.switch_theme", params: { theme, }, }); - await switchTheme.fetch().then(() => { - userResource.value?.reload(); - }); + await switchTheme.fetch(); + userResource.reload(); }
14-24: Consider reordering declarations for clarity.The computed properties
useranduserTeamsreferenceuserResourceanduserTeamsResourcebefore they are defined. While this works due to JavaScript's temporal behavior within the function scope, reordering the declarations improves readability:+ const userResource = createResource({ + url: "forms_pro.api.user.get_user", + }); + + const userTeamsResource = createResource({ + url: "forms_pro.api.user.get_user_teams", + }); + const user = computed(() => userResource.data); const userTeams = computed<UserTeam[]>(() => userTeamsResource.data); const currentTeam = ref<UserTeam | null>(null); - - const userResource = createResource({ - url: "forms_pro.api.user.get_user", - }); - - const userTeamsResource = createResource({ - url: "forms_pro.api.user.get_user_teams", - });forms_pro/utils/teams.py (1)
90-116: Return type annotation mismatch.The function signature declares
list[GetTeamFormsResponseSchema], but line 115 returns a list of dictionaries viamodel_dump(). Consider correcting the type hint:-def get_team_forms(team_id: str) -> list[GetTeamFormsResponseSchema]: +def get_team_forms(team_id: str) -> list[dict]:Alternatively, return the Pydantic models directly if the caller can handle them.
frontend/src/pages/Dashboard.vue (1)
109-127: Form creation may fail silently when no team is selected.Both
handleCreateDraftFormWithDoctypeandhandleCreateDraftFormpassuser.currentTeam?.namewhich could beundefined. Consider disabling the create buttons or showing an error when no team is available:const handleCreateDraftForm = async () => { + if (!user.currentTeam?.name) { + // Handle missing team - show toast or error + return; + } const data = await createNewForm(user.currentTeam?.name);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
forms_pro/api/team.py(1 hunks)forms_pro/api/user.py(1 hunks)forms_pro/forms_pro/doctype/form/form.json(2 hunks)forms_pro/forms_pro/doctype/form/form.py(1 hunks)forms_pro/forms_pro/doctype/fp_team/fp_team.py(1 hunks)forms_pro/overrides/roles.py(3 hunks)forms_pro/utils/form_generator.py(4 hunks)forms_pro/utils/teams.py(2 hunks)frontend/src/App.vue(1 hunks)frontend/src/layouts/BaseLayout.vue(1 hunks)frontend/src/pages/Dashboard.vue(5 hunks)frontend/src/stores/user.ts(1 hunks)frontend/src/utils/form_generator.ts(2 hunks)frontend/src/utils/theme.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
forms_pro/api/team.py (1)
forms_pro/utils/teams.py (2)
GetTeamFormsResponseSchema(10-52)get_team_forms(90-116)
forms_pro/utils/teams.py (1)
forms_pro/api/team.py (1)
get_team_forms(8-19)
forms_pro/api/user.py (1)
forms_pro/utils/teams.py (1)
get_user_teams(55-83)
forms_pro/overrides/roles.py (1)
forms_pro/utils/teams.py (2)
get_user_teams(55-83)set_current_team(86-87)
🪛 Ruff (0.14.8)
forms_pro/utils/teams.py
32-32: Avoid specifying long messages outside the exception class
(TRY003)
44-44: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (10)
forms_pro/forms_pro/doctype/fp_team/fp_team.py (2)
23-31: LGTM!The
team_membersproperty implementation is clean and correctly handles the case whenself.usersis empty or None.
33-43: LGTM!The
is_team_membermethod correctly leverages theteam_membersproperty for membership checking.forms_pro/api/user.py (2)
8-11: LGTM!The
GetUserTeamsResponseSchemais well-structured with clear field descriptions.
14-30: LGTM!The
GetUserResponseSchemacorrectly extracts role names fromHasRoleobjects using a field validator, with proper handling of empty lists.forms_pro/forms_pro/doctype/form/form.py (1)
24-24: LGTM!The type annotation for
linked_team_idcorrectly aligns with the new field added inform.json.forms_pro/forms_pro/doctype/form/form.json (1)
112-118: LGTM!The
linked_team_idfield configuration is correct, properly linking to the FP Team doctype with the required constraint.frontend/src/utils/form_generator.ts (2)
3-19: LGTM!The function correctly accepts and passes
team_idto the backend API.
21-33: LGTM!The function correctly accepts and passes
team_idto the backend API.forms_pro/overrides/roles.py (1)
30-43: LGTM!Good addition to set the current team immediately after creating the default team. This ensures new users have a proper team context from the start.
frontend/src/pages/Dashboard.vue (1)
39-70: Good conditional UI based on user permissions.The pattern of showing a
Dropdownfor desk users and a simpleButtonfor others is a clean approach to differentiate capabilities based onuser.has_desk_access.
| def __init__( | ||
| self, | ||
| team_id: str, | ||
| linked_doctype: str | None = None, | ||
| ) -> None: | ||
| self.doctype = None | ||
| self.team_id = team_id |
There was a problem hiding this comment.
Missing validation for team_id parameter.
The team_id parameter is required but there's no validation. If a caller passes None (e.g., when user.currentTeam?.name is undefined in the frontend), the form will be created with linked_team_id = None, which may cause issues when querying team forms later.
Consider adding validation:
🔎 Suggested fix
def __init__(
self,
team_id: str,
linked_doctype: str | None = None,
) -> None:
+ if not team_id:
+ frappe.throw("Team ID is required to create a form")
self.doctype = None
self.team_id = team_id🤖 Prompt for AI Agents
In forms_pro/utils/form_generator.py around lines 37 to 43, the __init__ method
accepts a required team_id but performs no validation; add a guard to ensure
team_id is provided and is a non-empty string (e.g., if team_id is None or
team_id == "" then raise ValueError with a clear message), optionally
coerce/check type and document the expectation in the method docstring so
callers (and tests) must pass a valid team_id rather than None.
| def get_team_forms(team_id: str) -> list[GetTeamFormsResponseSchema]: | ||
| """ | ||
| Get the list of forms for a team | ||
| Args: | ||
| team_id: ID of the team | ||
| Returns: | ||
| list[GetTeamFormsResponseSchema] - List of forms for the team | ||
| """ | ||
| forms = frappe.get_all( | ||
| "Form", | ||
| filters={"linked_team_id": team_id}, | ||
| fields=[ | ||
| "name", | ||
| "title", | ||
| "description", | ||
| "route", | ||
| "is_published", | ||
| "creation", | ||
| "modified", | ||
| "linked_team_id", | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Missing authorization check for team membership.
get_team_forms fetches forms for any team_id without verifying that the requesting user is a member of that team. This could allow users to view forms belonging to teams they're not part of.
Consider adding a membership check:
🔎 Suggested fix
def get_team_forms(team_id: str) -> list[GetTeamFormsResponseSchema]:
"""
Get the list of forms for a team
...
"""
+ # Verify user is a member of the team
+ is_member = frappe.db.exists(
+ "FP Team Member",
+ {"parent": team_id, "user": frappe.session.user}
+ )
+ if not is_member:
+ frappe.throw("You are not a member of this team", frappe.PermissionError)
+
forms = frappe.get_all(
"Form",
filters={"linked_team_id": team_id},
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
forms_pro/utils/teams.py (1)
90-116: Authorization check still missing (previously flagged).This function was previously flagged for lacking an authorization check to verify that the requesting user is a member of the team before returning its forms. This security issue remains unaddressed and allows users to view forms belonging to teams they're not part of.
🔎 Recommended fix to add membership verification
def get_team_forms(team_id: str) -> list[GetTeamFormsResponseSchema]: """ Get the list of forms for a team Args: team_id: ID of the team Returns: list[GetTeamFormsResponseSchema] - List of forms for the team """ + # Verify user is a member of the team + is_member = frappe.db.exists( + "FP Team Member", + {"parent": team_id, "user": frappe.session.user} + ) + if not is_member: + frappe.throw("You are not a member of this team", frappe.PermissionError) + forms = frappe.get_all( "Form", filters={"linked_team_id": team_id},
🧹 Nitpick comments (1)
forms_pro/install.py (1)
18-30: Consider creating a default team for the test user.The test user is created but no team is assigned. This will cause an
IndexErrorinforms_pro/utils/test_form_generator.pyline 15, where the test setup assumes the test user has at least one team.Should
create_test_useralso create a default team for the test user? This would prevent theIndexErrorin test setup.🔎 Suggested enhancement to create default team
def create_test_user(): test_user = "test_forms_pro_user@example.com" if frappe.db.exists("User", test_user): return user: User = frappe.new_doc("User") user.email = test_user user.first_name = "Test" user.last_name = "Forms Pro User" user.insert(ignore_permissions=True) user.append("roles", {"role": FORMS_PRO_ROLE}) user.save(ignore_permissions=True) + + # Create a default team for the test user + if not frappe.db.exists("FP Team", {"team_name": "Test Team"}): + team = frappe.new_doc("FP Team") + team.team_name = "Test Team" + team.append("members", {"user": test_user}) + team.insert(ignore_permissions=True)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
forms_pro/hooks.py(1 hunks)forms_pro/install.py(1 hunks)forms_pro/utils/teams.py(2 hunks)forms_pro/utils/test_form_generator.py(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- forms_pro/hooks.py
🧰 Additional context used
🧬 Code graph analysis (2)
forms_pro/utils/test_form_generator.py (2)
forms_pro/utils/teams.py (1)
get_user_teams(55-83)forms_pro/utils/form_generator.py (1)
FormGenerator(36-109)
forms_pro/utils/teams.py (1)
forms_pro/api/team.py (1)
get_team_forms(8-19)
🪛 Ruff (0.14.8)
forms_pro/utils/teams.py
32-32: Avoid specifying long messages outside the exception class
(TRY003)
44-44: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (6)
forms_pro/utils/teams.py (4)
1-8: LGTM!The imports are appropriate and necessary for the team management functionality.
10-52: LGTM!The Pydantic schema is well-structured with appropriate validators for type conversions. The field validators correctly handle datetime parsing, boolean conversion, and None string handling.
55-83: LGTM!The IndexError issue from the previous review has been properly addressed with the
len(teams) > 0check on line 79. The function now safely handles cases where users have no teams.
86-88: LGTM!Clean wrapper function that appropriately abstracts the user default persistence.
forms_pro/utils/test_form_generator.py (1)
21-159: LGTM!All test methods have been consistently updated to pass
team_idto theFormGeneratorconstructor. The changes properly align with the new team-scoped FormGenerator API while maintaining test coverage across all scenarios.forms_pro/install.py (1)
1-10: LGTM!The imports are appropriate, and the
before_testsfunction provides a clean orchestration of test setup tasks.
| def give_admin_forms_pro_role(): | ||
| admin = frappe.get_doc("User", "Administrator") | ||
| admin.append("roles", {"role": FORMS_PRO_ROLE}) | ||
| admin.save() |
There was a problem hiding this comment.
Check for existing role before appending.
Line 14 appends the Forms Pro role without checking if it already exists. If before_tests is called multiple times, this will create duplicate role entries for the Administrator user.
🔎 Recommended fix to prevent duplicate roles
def give_admin_forms_pro_role():
admin = frappe.get_doc("User", "Administrator")
- admin.append("roles", {"role": FORMS_PRO_ROLE})
+ if not any(r.role == FORMS_PRO_ROLE for r in admin.roles):
+ admin.append("roles", {"role": FORMS_PRO_ROLE})
admin.save()🤖 Prompt for AI Agents
In forms_pro/install.py around lines 12 to 15, the code appends the
FORMS_PRO_ROLE to the Administrator user unconditionally which can create
duplicate role entries; change it to first check whether the role already exists
on the admin document (e.g., inspect admin.get("roles") or admin.roles) and only
append and save if the role is not present, ensuring admin.save() is called only
when a new role was actually added.
| def create_test_user(): | ||
| test_user = "test_forms_pro_user@example.com" | ||
|
|
||
| if frappe.db.exists("User", test_user): | ||
| return | ||
|
|
||
| user: User = frappe.new_doc("User") | ||
| user.email = test_user | ||
| user.first_name = "Test" | ||
| user.last_name = "Forms Pro User" | ||
| user.insert(ignore_permissions=True) | ||
| user.append("roles", {"role": FORMS_PRO_ROLE}) | ||
| user.save(ignore_permissions=True) |
There was a problem hiding this comment.
Check for existing role before appending.
Line 29 appends the Forms Pro role without checking if it already exists. While this is less critical than in give_admin_forms_pro_role due to the existence check on line 21, it could still cause issues if the user document is partially created in a previous failed run.
🔎 Recommended fix to prevent duplicate roles
user: User = frappe.new_doc("User")
user.email = test_user
user.first_name = "Test"
user.last_name = "Forms Pro User"
user.insert(ignore_permissions=True)
- user.append("roles", {"role": FORMS_PRO_ROLE})
+ if not any(r.role == FORMS_PRO_ROLE for r in user.roles):
+ user.append("roles", {"role": FORMS_PRO_ROLE})
user.save(ignore_permissions=True)🤖 Prompt for AI Agents
In forms_pro/install.py around lines 18 to 30, the code appends the
FORMS_PRO_ROLE to the new user unconditionally, which can create a duplicate
role entry if the user doc was partially created in a prior run; before
appending, check whether the user already has a role entry for FORMS_PRO_ROLE
(e.g., inspect user.get("roles") or query existing UserRole) and only append/add
the role when it's not present, then save the document as before.
| def setUp(self): | ||
| super().setUp() | ||
| self.test_user = "test_forms_pro_user@example.com" | ||
| self.test_team = get_user_teams(self.test_user)[0]["name"] | ||
|
|
||
| def tearDown(self): | ||
| frappe.set_user("Administrator") | ||
| super().tearDown() |
There was a problem hiding this comment.
Potential IndexError in test setup.
Line 15 assumes the test user has at least one team. If get_user_teams(self.test_user) returns an empty list, accessing [0] will raise an IndexError and cause all tests to fail during setup.
🔎 Recommended fix to handle empty teams
def setUp(self):
super().setUp()
self.test_user = "test_forms_pro_user@example.com"
- self.test_team = get_user_teams(self.test_user)[0]["name"]
+ teams = get_user_teams(self.test_user)
+ if not teams:
+ # Create a test team if none exists
+ test_team = frappe.new_doc("FP Team")
+ test_team.team_name = "Test Team"
+ test_team.append("members", {"user": self.test_user})
+ test_team.insert(ignore_permissions=True)
+ teams = get_user_teams(self.test_user)
+ self.test_team = teams[0]["name"]Alternatively, ensure that install.py:create_test_user creates a default team for the test user.
🤖 Prompt for AI Agents
In forms_pro/utils/test_form_generator.py around lines 12 to 19, the setUp
assumes get_user_teams(self.test_user)[0] exists which can raise IndexError if
the user has no teams; modify setUp to handle an empty list by checking the
result of get_user_teams(self.test_user) before indexing and either (a) create
or assign a default team for the test user (via the existing
install.py:create_test_user helper or a small helper that creates a team and
adds the user), or (b) fail the setup with a clear, actionable error message
when no teams are found; ensure teardown still resets the user.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/pages/Dashboard.vue (2)
3-33: Missing resource declarations break DocType dialog.Lines 13-14 and 24, 27 reference
selectedDoctypeanddoctypes.datain the template and functionhandleCreateDraftFormWithDoctype, but neither variable is declared in the script section. This will cause runtime errors when the dialog is opened or the Create button is clicked—attempting to accessdoctypes.datawill fail with undefined property access, and the v-model binding onselectedDoctypewill not function.
109-127: Add frontend validation to prevent creating forms without an assigned team.Both
handleCreateDraftFormWithDoctypeandhandleCreateDraftFormpassuser.currentTeam?.nameto backend APIs, which could beundefinedif no team is assigned. The backend has no validation to prevent forms from being created withlinked_team_id = None. Either validate thatcurrentTeamexists before calling these functions, or implement backend validation to reject requests with missingteam_id.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/pages/Dashboard.vue(5 hunks)frontend/src/stores/user.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/stores/user.ts
⏰ 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 (2)
frontend/src/pages/Dashboard.vue (2)
129-138: Team forms resource configuration looks good.The resource is properly configured with the team-scoped API endpoint and dynamic
team_idparameter. Theloading: trueinitial state and cache key are appropriate.
140-148: Watcher implementation properly addresses the race condition.The watcher with
immediate: trueand conditional check correctly handles the team initialization race condition mentioned in the past review comment. It will:
- Run immediately on mount if
currentTeamis already set- Catch when
currentTeamis set after mount (from undefined to defined)- Only fetch when a valid team is available
This resolves the previous concern about
teamForms.fetch()being called withteam_id: undefined.
| <template v-if="user.has_desk_access"> | ||
| <Dropdown | ||
| class="w-fit" | ||
| :button="{ | ||
| label: 'Create', | ||
| iconLeft: 'plus', | ||
| variant: 'solid', | ||
| }" | ||
| :options="[ | ||
| { | ||
| label: 'Create New', | ||
| onClick: handleCreateDraftForm, | ||
| }, | ||
| }, | ||
| ]" | ||
| /> | ||
| { | ||
| label: 'Create from Existing DocType', | ||
| onClick: () => { | ||
| showSelectDoctypeDialog = true; | ||
| }, | ||
| }, | ||
| ]" | ||
| /> | ||
| </template> | ||
| <template v-else> | ||
| <Button | ||
| class="w-fit" | ||
| variant="solid" | ||
| iconLeft="plus" | ||
| @click="handleCreateDraftForm" | ||
| > | ||
| Create New | ||
| </Button> | ||
| </template> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for has_desk_access in frontend
echo "=== Frontend usage of has_desk_access ==="
rg -n "has_desk_access" frontend/src || echo "No frontend matches"
# Search for has_desk_access in backend
echo "=== Backend usage of has_desk_access ==="
rg -n "has_desk_access" forms_pro/api || echo "No backend API matches"
# Broader search in backend
echo "=== All backend matches for has_desk_access ==="
rg -n "has_desk_access" . --type py || echo "No Python matches"Repository: BuildWithHussain/forms_pro
Length of output: 557
🏁 Script executed:
#!/bin/bash
# Get full context around has_desk_access in user.py
echo "=== forms_pro/api/user.py context ==="
cat -n forms_pro/api/user.py | head -50
# Search for the user_doc class definition and has_desk_access method
echo ""
echo "=== Search for has_desk_access method definition ==="
rg -n "def has_desk_access" . --type py -B 2 -A 5
# Look for imports and base class
echo ""
echo "=== Check where user_doc comes from ==="
rg -n "user_doc" forms_pro/api/user.py -B 3 -A 1Repository: BuildWithHussain/forms_pro
Length of output: 2126
🏁 Script executed:
#!/bin/bash
# Search for "desk_access" or "Desk" in the codebase to understand its purpose
echo "=== Search for desk_access or Desk references ==="
rg -n -i "desk" . --type py | head -20
echo ""
echo "=== Check if there's any documentation or comments about desk access ==="
rg -n -i "desk.*access|access.*desk" . -B 2 -A 2 | head -30
echo ""
echo "=== Look for DocType creation related code ==="
rg -n "DocType\|doctype" frontend/src/pages/Dashboard.vue -B 2 -A 2
echo ""
echo "=== Check handleCreateDraftForm implementation ==="
rg -n "handleCreateDraftForm" frontend/src -B 2 -A 10 | head -40Repository: BuildWithHussain/forms_pro
Length of output: 5064
🏁 Script executed:
#!/bin/bash
# Get the full Dashboard.vue to understand the context better
echo "=== Full Dashboard.vue script section ==="
cat frontend/src/pages/Dashboard.vue | tail -100
echo ""
echo "=== Search for what handleCreateDraftForm does ==="
rg -n "const handleCreateDraftForm\|function handleCreateDraftForm" frontend/src -A 15
echo ""
echo "=== Check Frappe documentation or codebase for User.has_desk_access() ==="
echo "This is a Frappe built-in method, let's verify its purpose"
rg -n "has_desk_access" . --type py -B 5 -A 5Repository: BuildWithHussain/forms_pro
Length of output: 4377
🏁 Script executed:
#!/bin/bash
# Look at the role.json fixture to understand desk_access better
echo "=== Role fixture with desk_access ==="
cat forms_pro/fixtures/role.json
echo ""
echo "=== Check if there are any role-based permissions related to form creation ==="
rg -n "create.*form\|form.*create" forms_pro/ --type py -i | head -20Repository: BuildWithHussain/forms_pro
Length of output: 469
🏁 Script executed:
#!/bin/bash
# Check if there's backend enforcement of this permission when creating forms
echo "=== Check createNewForm backend implementation ==="
rg -n "createNewForm\|create_new_form" forms_pro/ --type py -A 10
echo ""
echo "=== Check createNewFormWithDoctype backend implementation ==="
rg -n "createNewFormWithDoctype\|create_new_form_with_doctype" forms_pro/ --type py -A 10
echo ""
echo "=== Check for permission checks in API endpoints ==="
rg -n "@frappe.whitelist\|@frappe.permission_required\|has_permission" forms_pro/api/ --type py -B 1 -A 3Repository: BuildWithHussain/forms_pro
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Search for form_generator file
echo "=== Find form_generator file ==="
find . -name "*form_generator*" -type f
echo ""
echo "=== Search for where form creation API is implemented ==="
rg -n "def.*form" forms_pro/api/ --type py | head -20
echo ""
echo "=== Check all files in forms_pro/api ==="
ls -la forms_pro/api/
echo ""
echo "=== Search for form creation in entire codebase ==="
rg -n "createNewForm" . -B 2 -A 5 | head -40Repository: BuildWithHussain/forms_pro
Length of output: 3714
🏁 Script executed:
#!/bin/bash
# Check the form_generator.py file for permission checks
echo "=== forms_pro/utils/form_generator.py ==="
cat forms_pro/utils/form_generator.py
echo ""
echo "=== Check if create_form or create_form_with_doctype check has_desk_access ==="
rg -n "has_desk_access" forms_pro/utils/form_generator.pyRepository: BuildWithHussain/forms_pro
Length of output: 3723
Fix permission check mismatch in conditional rendering.
The frontend uses has_desk_access to gate the "Create from Existing DocType" feature, but the backend (create_form and create_form_with_doctype) enforces the FORMS_PRO_ROLE permission. These are semantically different:
has_desk_access= user can access Frappe Desk (admin interface)FORMS_PRO_ROLE= user can create forms
Use FORMS_PRO_ROLE instead of has_desk_access on the frontend to match the backend permission model and ensure consistency.
🤖 Prompt for AI Agents
In frontend/src/pages/Dashboard.vue around lines 39 to 70, the template uses
user.has_desk_access to gate the "Create from Existing DocType" flow but the
backend requires the FORMS_PRO_ROLE permission; update the conditional to check
the actual forms permission instead of has_desk_access (e.g., replace
user.has_desk_access with a check for FORMS_PRO_ROLE via
user.roles.includes(FORMS_PRO_ROLE) or user.has_role('FORMS_PRO_ROLE'), and
import or reference the FORMS_PRO_ROLE constant if available) so the frontend
permission model matches the backend.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
forms_pro/forms_pro/doctype/form/test_form.py(2 hunks)
⏰ 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 (2)
forms_pro/forms_pro/doctype/form/test_form.py (2)
28-35: Addition oflinked_team_idaligns with team-scoped forms.The inclusion of
linked_team_idin the Form document creation correctly implements the team-scoped forms feature described in the PR objectives. The test now properly exercises team-linked form creation.Note: This depends on the proper setup of
self.test_teamin lines 23-25. Once the IndexError concern is addressed, this test will reliably validate team-scoped form behavior.
7-8: Theget_user_teamsutility is properly defined and available for import fromforms_pro.utils.teams.
| self.test_user = "test_forms_pro_user@example.com" | ||
| self.test_team = get_user_teams(self.test_user)[0]["name"] | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "install.py" -o -name "*fixture*" -o -name "*setup*" | head -20Repository: BuildWithHussain/forms_pro
Length of output: 113
🏁 Script executed:
rg -n "test_forms_pro_user@example.com" -A 5 -B 5Repository: BuildWithHussain/forms_pro
Length of output: 2308
🏁 Script executed:
rg -n "def get_user_teams" -A 15Repository: BuildWithHussain/forms_pro
Length of output: 1637
🏁 Script executed:
rg -n "def setUp|def setUpClass" -B 2 -A 10 forms_pro/forms_pro/doctype/form/test_form.pyRepository: BuildWithHussain/forms_pro
Length of output: 564
🏁 Script executed:
cat -n forms_pro/forms_pro/doctype/form/test_form.py | head -40Repository: BuildWithHussain/forms_pro
Length of output: 1679
🏁 Script executed:
cat -n forms_pro/install.pyRepository: BuildWithHussain/forms_pro
Length of output: 1019
🏁 Script executed:
rg -n "FP Team" -B 3 -A 10 forms_pro/install.pyRepository: BuildWithHussain/forms_pro
Length of output: 52
🏁 Script executed:
cat -n forms_pro/utils/test_form_generator.py | head -30Repository: BuildWithHussain/forms_pro
Length of output: 1331
🏁 Script executed:
rg -n "FP Team" forms_pro/ -A 5 | head -50Repository: BuildWithHussain/forms_pro
Length of output: 3097
🏁 Script executed:
ls -la forms_pro/fixtures/Repository: BuildWithHussain/forms_pro
Length of output: 239
🏁 Script executed:
cat forms_pro/fixtures/*.json 2>/dev/null | head -100Repository: BuildWithHussain/forms_pro
Length of output: 352
🏁 Script executed:
cat -n forms_pro/overrides/roles.pyRepository: BuildWithHussain/forms_pro
Length of output: 1820
🏁 Script executed:
rg -n "def.*after_insert|def.*on_update" forms_pro/overrides/roles.py -B 2 -A 15Repository: BuildWithHussain/forms_pro
Length of output: 52
Guard against IndexError when retrieving test team.
The test user is created with the "Forms Pro User" role during test setup, which should automatically create a default team through the handle_forms_pro_role_change hook in forms_pro/overrides/roles.py. However, accessing [0] on get_user_teams() without bounds checking creates an unnecessary risk—if team creation fails or is skipped, an IndexError will crash the test. Add defensive checks to ensure the user has at least one team before accessing it.
Suggested fix:
self.test_user = "test_forms_pro_user@example.com"
-self.test_team = get_user_teams(self.test_user)[0]["name"]
+user_teams = get_user_teams(self.test_user)
+self.assertGreater(len(user_teams), 0, f"User {self.test_user} has no teams")
+self.test_team = user_teams[0]["name"]📝 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.
| self.test_user = "test_forms_pro_user@example.com" | |
| self.test_team = get_user_teams(self.test_user)[0]["name"] | |
| self.test_user = "test_forms_pro_user@example.com" | |
| user_teams = get_user_teams(self.test_user) | |
| self.assertGreater(len(user_teams), 0, f"User {self.test_user} has no teams") | |
| self.test_team = user_teams[0]["name"] | |
🤖 Prompt for AI Agents
In forms_pro/forms_pro/doctype/form/test_form.py around lines 23 to 25, guard
against IndexError when retrieving the test user's team: call
get_user_teams(self.test_user) into a variable, check that the returned list is
non-empty before indexing, and if it is empty either (a) create or assign a
default team for the test user (e.g., invoke the team-creation path used by the
Forms Pro role hook) or (b) raise an explicit AssertionError with a clear
message so the test fails fast and clearly; then set self.test_team to the first
team's name only after this defensive check.
Summary by CodeRabbit
New Features
Improvements
Tests / Chores
✏️ Tip: You can customize this high-level summary in your review settings.