Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions forms_pro/api/team.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import frappe

from forms_pro.forms_pro.doctype.fp_team.fp_team import FPTeam, GetTeamMembersResponse
from forms_pro.utils.teams import GetTeamFormsResponseSchema
from forms_pro.utils.teams import GetTeamFormsResponseSchema, set_current_team
from forms_pro.utils.teams import get_team_forms as get_team_forms_utils


Expand Down Expand Up @@ -35,9 +35,43 @@ def get_team_members(team_id: str) -> list[GetTeamMembersResponse]:
user=frappe.session.user,
throw=True,
)
print("team_id", team_id)

team: FPTeam = frappe.get_doc("FP Team", team_id)
members = team.team_members

return members


@frappe.whitelist(methods=["POST"])
def create_team(team_name: str) -> FPTeam:
"""
Create a new team

Args:
team_name: Name of the team

Returns:
FPTeam - The created team
"""

team: FPTeam = frappe.new_doc("FP Team")
team.team_name = team_name
team.insert()
return team
Comment on lines +45 to +60
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

No server-side validation on team_name.

create_team accepts any string, including empty or whitespace-only names. The frontend enforces min(2) via Zod, but the API endpoint is publicly callable and should validate independently.

Proposed fix
 def create_team(team_name: str) -> FPTeam:
+    team_name = team_name.strip()
+    if not team_name:
+        frappe.throw("Team name is required")
+
     team: FPTeam = frappe.new_doc("FP Team")
     team.team_name = team_name
     team.insert()
     return team
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@frappe.whitelist(methods=["POST"])
def create_team(team_name: str) -> FPTeam:
"""
Create a new team
Args:
team_name: Name of the team
Returns:
FPTeam - The created team
"""
team: FPTeam = frappe.new_doc("FP Team")
team.team_name = team_name
team.insert()
return team
`@frappe.whitelist`(methods=["POST"])
def create_team(team_name: str) -> FPTeam:
"""
Create a new team
Args:
team_name: Name of the team
Returns:
FPTeam - The created team
"""
team_name = team_name.strip()
if not team_name:
frappe.throw("Team name is required")
team: FPTeam = frappe.new_doc("FP Team")
team.team_name = team_name
team.insert()
return team
🤖 Prompt for AI Agents
In `@forms_pro/api/team.py` around lines 45 - 60, create_team currently accepts
any string for team_name (including empty or whitespace-only) and lacks
server-side validation; update the create_team function to validate team_name
before creating the FP Team: trim whitespace, verify length >= 2, and raise a
Frappe validation error (e.g., frappe.throw or frappe.ValidationError) if
validation fails; keep the existing flow using frappe.new_doc("FP Team"), set
team.team_name only after validation, and only call team.insert() when the name
passes validation.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this won't be needed, since we have the validation coming in from frappe. team_name is a mandatory field in doctype

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harshtandiya, you're absolutely right! I appreciate the clarification. Frappe's DocType mandatory field validation will handle this automatically when team.insert() is called, so the explicit validation is indeed redundant. Thank you for pointing that out! 👍


✏️ Learnings added
Learnt from: harshtandiya
Repo: BuildWithHussain/forms_pro PR: 37
File: forms_pro/api/team.py:45-60
Timestamp: 2026-02-07T20:33:14.636Z
Learning: In Frappe framework (Python backend), when a field is marked as mandatory in a DocType definition, the framework automatically validates it during the `.insert()` operation. This validation includes checking for empty values, so explicit server-side validation for mandatory fields is typically redundant in API endpoints.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.



@frappe.whitelist(methods=["POST"])
def switch_team(team_id: str) -> None:
"""
Switch to a new team
"""

if not frappe.has_permission(
doctype="FP Team",
ptype="read",
doc=team_id,
user=frappe.session.user,
):
raise frappe.PermissionError("You do not have permission to switch to this team")

set_current_team(team_id, frappe.session.user)
41 changes: 37 additions & 4 deletions forms_pro/forms_pro/doctype/fp_team/fp_team.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
# Copyright (c) 2025, harsh@buildwithhussain.com and contributors
# For license information, please see license.txt

# import frappe
import frappe
from frappe.model.document import Document
from frappe.utils import cached_property
from frappe.share import add_docshare
from pydantic import BaseModel, EmailStr

from forms_pro.api.user import get_user
from forms_pro.utils.teams import set_current_team


class GetTeamMembersResponse(BaseModel):
Expand All @@ -30,7 +31,7 @@ class FPTeam(Document):
users: DF.TableMultiSelect[FPTeamMember]
# end: auto-generated types

@cached_property
@property
def team_members(self) -> list[GetTeamMembersResponse]:
"""
Get the list of team members
Expand Down Expand Up @@ -60,4 +61,36 @@ def is_team_member(self, user: str) -> bool:
Returns:
bool - True if the user is a member of the team, False otherwise
"""
return user in self.team_members
return user in [member["email"] for member in self.team_members]

def after_insert(self) -> None:
self.add_to_team(self.owner)
set_current_team(self.name, self.owner)
self.save()

def add_to_team(self, user: str) -> None:
"""
Add a user to the team

Args:
user: The user email address
"""
if user == "Administrator":
return

if self.is_team_member(user):
frappe.throw(
frappe._("User is already a member of the team"),
frappe.DuplicateEntryError,
)

self.append("users", {"user": user})
add_docshare(
self.doctype,
self.name,
user,
read=1,
write=1,
share=1,
flags={"ignore_share_permission": True},
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
61 changes: 59 additions & 2 deletions forms_pro/forms_pro/doctype/fp_team/test_fp_team.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
# Copyright (c) 2025, harsh@buildwithhussain.com and Contributors
# See license.txt

# import frappe
import frappe
from frappe.defaults import get_user_default
from frappe.tests import IntegrationTestCase

from forms_pro.tests import FORMS_PRO_TEST_USER

# On IntegrationTestCase, the doctype test records and all
# link-field test record dependencies are recursively loaded
# Use these module variables to add/remove to/from that list
Expand All @@ -17,4 +20,58 @@ class IntegrationTestFPTeam(IntegrationTestCase):
Use this class for testing interactions between multiple components.
"""

pass
def setUp(self):
super().setUp()
self.test_user = FORMS_PRO_TEST_USER

def tearDown(self):
frappe.set_user("Administrator")
super().tearDown()

def test_add_owner_to_team(self):
"""
Test that after a user creates a team, that owner user is added to the team and the team is shared with the owner user
"""

frappe.set_user(self.test_user)

team = frappe.new_doc("FP Team")
team.team_name = "Test Team"
team.insert()
team.reload()

frappe.set_user("Administrator")
Comment thread
coderabbitai[bot] marked this conversation as resolved.

# Check that the user is added to the team
self.assertTrue(team.is_team_member(self.test_user))

# Check that the user is added to the team
self.assertIsNotNone(
frappe.db.exists(
"FP Team Member",
{
"parent": team.name,
"parentfield": "users",
"parenttype": "FP Team",
"user": self.test_user,
},
)
)

# Check that the user is added to the team's docshare
self.assertTrue(
frappe.db.exists(
"DocShare",
{
"share_doctype": "FP Team",
"share_name": team.name,
"user": self.test_user,
"read": 1,
"write": 1,
"share": 1,
},
)
)

# Check that the user's current team is set to the team
self.assertEqual(get_user_default("current_team", self.test_user), team.name)
7 changes: 3 additions & 4 deletions forms_pro/install.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from frappe.core.doctype.user.user import User

from forms_pro.roles import FORMS_PRO_ROLE
from forms_pro.tests import FORMS_PRO_TEST_USER


def after_install():
Expand Down Expand Up @@ -30,13 +31,11 @@ def give_admin_forms_pro_role():


def create_test_user():
test_user = "test_forms_pro_user@example.com"

if frappe.db.exists("User", test_user):
if frappe.db.exists("User", FORMS_PRO_TEST_USER):
return

user: User = frappe.new_doc("User")
user.email = test_user
user.email = FORMS_PRO_TEST_USER
user.first_name = "Test"
user.last_name = "Forms Pro User"
user.insert(ignore_permissions=True)
Expand Down
3 changes: 1 addition & 2 deletions forms_pro/overrides/roles.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from frappe.core.doctype.user.user import User

from forms_pro.roles import FORMS_PRO_ROLE
from forms_pro.utils.teams import get_user_teams, set_current_team
from forms_pro.utils.teams import get_user_teams


def has_forms_pro_permission() -> bool:
Expand Down Expand Up @@ -48,4 +48,3 @@ def create_default_team_for_user(user: User) -> None:
},
)
team.save(ignore_permissions=True)
set_current_team(team.name, user.name)
5 changes: 5 additions & 0 deletions forms_pro/public/images/logo_300.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 2 additions & 0 deletions forms_pro/tests/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Test user email address
FORMS_PRO_TEST_USER = "test_forms_pro_user@example.com"
77 changes: 77 additions & 0 deletions frontend/src/components/team/CreateTeamDialog.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
<script setup lang="ts">
import { Dialog, ErrorMessage, FormControl } from "frappe-ui";
import * as z from "zod";
import { reactive, ref, watch } from "vue";
import { useUser } from "@/stores/user";

const user = useUser();

const model = defineModel<boolean>({
type: Boolean,
required: true,
});

const formErrors = ref<string>("");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Button is initially enabled despite empty (invalid) input.

formErrors starts as "", and the watcher hasn't fired yet on mount, so the "Create Team" button appears enabled even though team_name is empty. Clicking it triggers the guard in createTeam() (which is safe), but the UX is misleading.

A simple fix: initialize formErrors by running validation eagerly, or start with a non-empty error string.

Option: validate on mount
-const formErrors = ref<string>("");
+const formErrors = ref<string>("");
+
+// Validate immediately so button starts disabled
+const initialResult = formSchema.safeParse(form);
+if (!initialResult.success) {
+    formErrors.value = initialResult.error.issues.map((i) => i.message).join(", ");
+}

Also applies to: 29-40

🤖 Prompt for AI Agents
In `@frontend/src/components/team/CreateTeamDialog.vue` at line 14, The button is
enabled because formErrors is initialized to an empty string before validation
runs; change initialization so validation runs eagerly: either set formErrors to
a non-empty error or call the existing validation function on mount (or
immediately after defining the watcher) to populate formErrors based on
team_name. Specifically, update the CreateTeamDialog component to invoke the
validation routine (the same logic the watcher uses) during setup/mounted (so
formErrors reflects the current validity of team_name on render) or initialize
formErrors with a suitable error message; keep the guard in createTeam()
unchanged.


const formSchema = z.object({
team_name: z
.string()
.min(2, "Team name must be at least 2 characters long")
.max(140, "Team name must be less than 140 characters long"),
});

type Form = z.infer<typeof formSchema>;

const form = reactive<Form>({
team_name: "",
});

watch(
() => form,
(newVal) => {
const result = formSchema.safeParse(newVal);
if (!result.success) {
formErrors.value = result.error.issues.map((issue) => issue.message).join(", ");
} else {
formErrors.value = "";
}
},
{ deep: true }
);

function createTeam() {
const result = formSchema.safeParse(form);
if (!result.success) {
formErrors.value = result.error.issues.map((issue) => issue.message).join(", ");
return;
}
formErrors.value = "";
user.createTeam(form.team_name);
model.value = false;
}
Comment on lines +42 to +51
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Dialog closes before API call completes and form state is never reset.

Two related UX issues:

  1. model.value = false (Line 50) runs immediately after the fire-and-forget user.createTeam() call — the dialog closes before the server responds. If the request fails, the user sees an error toast with no dialog context. This ties into the store-level issue where createTeam doesn't return a promise.

  2. form.team_name is never cleared when the dialog closes or re-opens. A user who opens the dialog a second time will see the previously entered team name.

Suggested reset on close
 function createTeam() {
     const result = formSchema.safeParse(form);
     if (!result.success) {
         formErrors.value = result.error.issues.map((issue) => issue.message).join(", ");
         return;
     }
     formErrors.value = "";
     user.createTeam(form.team_name);
     model.value = false;
+    form.team_name = "";
 }

Or better yet, watch model and reset on close:

watch(model, (open) => {
    if (!open) {
        form.team_name = "";
        formErrors.value = "";
    }
});
🤖 Prompt for AI Agents
In `@frontend/src/components/team/CreateTeamDialog.vue` around lines 42 - 51, The
createTeam handler closes the dialog immediately and never resets the form;
change createTeam to await the store call (ensure user.createTeam returns a
Promise) and only set model.value = false after the awaited call succeeds
(handle errors to keep the dialog open and show toast), add a loading/disabled
state while awaiting, and reset form.team_name and formErrors when the dialog
closes (preferably by adding a watcher on model that clears form.team_name and
formErrors when model becomes false); reference createTeam, user.createTeam,
model, form.team_name, and formErrors when making these changes.

</script>
<template>
<Dialog
v-model="model"
:options="{
title: 'Create New Team',
icon: 'users',
}"
>
<template #body-content>
<div class="flex flex-col gap-4">
<FormControl
v-model="form.team_name"
label="Team Name"
variant="outline"
type="text"
required
/>
<ErrorMessage :message="formErrors" />
<Button @click="createTeam" :disabled="formErrors !== ''" variant="subtle">
Create Team
</Button>
Comment on lines +71 to +73
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Button is used but not imported.

Button is referenced in the template but is not listed in the imports on Line 2. This works only if Button is globally registered by frappe-ui. Add it to the import for explicitness and to avoid breakage if global registration changes.

-import { Dialog, ErrorMessage, FormControl } from "frappe-ui";
+import { Button, Dialog, ErrorMessage, FormControl } from "frappe-ui";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<Button @click="createTeam" :disabled="formErrors !== ''" variant="subtle">
Create Team
</Button>
import { Button, Dialog, ErrorMessage, FormControl } from "frappe-ui";
🤖 Prompt for AI Agents
In `@frontend/src/components/team/CreateTeamDialog.vue` around lines 71 - 73, The
template uses the Button component (see <Button `@click`="createTeam"
:disabled="formErrors !== ''"> in CreateTeamDialog.vue) but Button is not
imported; update the component imports at the top of CreateTeamDialog.vue to
explicitly import Button from frappe-ui (alongside any existing imports) and add
it to the component registration/definition so the CreateTeamDialog component
can reference Button reliably when createTeam is invoked.

</div>
</template>
</Dialog>
</template>
11 changes: 9 additions & 2 deletions frontend/src/components/ui/Avatar.vue
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@ const props = withDefaults(
variant?: "default" | "outline" | "filled";
shape?: "circle" | "square";
className?: string;
disabled?: boolean;
}>(),
{
size: "md",
variant: "default",
shape: "circle",
disabled: false,
}
);

Expand All @@ -29,11 +31,16 @@ onMounted(async () => {
</script>

<template>
<Tooltip v-if="user?.full_name" :text="user?.full_name" placement="bottom">
<Tooltip
v-if="user?.full_name"
:disabled="disabled"
:text="user?.full_name"
placement="bottom"
>
<FrappeAvatar
:size="size"
:shape="shape"
:image="user?.user_image"
:image="user?.user_image ?? undefined"
:label="user?.full_name"
/>
</Tooltip>
Expand Down
Loading