Skip to content

feat: team creation & team switcher functionality#37

Merged
harshtandiya merged 11 commits into
developfrom
team-switcher
Feb 7, 2026
Merged

feat: team creation & team switcher functionality#37
harshtandiya merged 11 commits into
developfrom
team-switcher

Conversation

@harshtandiya
Copy link
Copy Markdown
Collaborator

@harshtandiya harshtandiya commented Feb 7, 2026

image image

Summary by CodeRabbit

  • New Features
    • Create teams from the app and switch between teams; creators are added to their team and granted access.
  • UI/UX Improvements
    • Added "Create New Team" dialog and integrated team controls into the main layout/menu.
    • Avatar component gains a disabled state for improved UI control.
    • Team list updates and notifications shown when switching/creating teams.
  • Bug Fixes
    • Prevent duplicate team members and remove stray debug output.
  • Tests
    • Added tests verifying owner is added, permissions are created, and current-team behavior.

- Added create_team and switch_team API methods to manage team operations.
- Introduced CreateTeamDialog component for user interface to create new teams.
- Updated user store to handle team creation and switching with success/error notifications.
- Enhanced FPTeam document to automatically set the current team upon creation.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Feb 7, 2026

📝 Walkthrough

Walkthrough

Adds backend endpoints to create and switch teams, updates FPTeam to auto-add the owner and create DocShare on insert, removes automatic current-team assignment in roles, adds a CreateTeam dialog and UI menu integration, introduces frontend store actions/watchers, and adds tests and install updates for the test user.

Changes

Cohort / File(s) Summary
Backend API
forms_pro/api/team.py
Added create_team(team_name: str) and switch_team(team_id: str) POST endpoints; permission checks and current-team update calls.
Domain Model (FPTeam)
forms_pro/forms_pro/doctype/fp_team/fp_team.py
Changed team_members to @property; added after_insert() and add_to_team(user) to auto-add owner, create DocShare, and call set_current_team.
Role Initialization
forms_pro/overrides/roles.py
Removed automatic set_current_team call during default team creation.
Tests & Install
forms_pro/forms_pro/doctype/fp_team/test_fp_team.py, forms_pro/install.py, forms_pro/tests/__init__.py
Added FORMS_PRO_TEST_USER constant, updated install to use it, and added integration test verifying owner membership, DocShare creation, and current_team default.
Frontend — Components
frontend/src/components/team/CreateTeamDialog.vue, frontend/src/components/ui/Avatar.vue, frontend/src/layouts/BaseLayout.vue
Added CreateTeamDialog component; Avatar gains disabled prop; BaseLayout integrates team menu, CreateTeamDialog, avatar/footer popover, and adjusts sidebar props.
Frontend — Stores
frontend/src/stores/user.ts, frontend/src/stores/team.ts
Added createTeam() and switchTeam() actions, CreateTeamResponse type, toast handling, and a watcher in team store to fetch members when currentTeam changes.

Sequence Diagram(s)

sequenceDiagram
    participant UI as Frontend UI
    participant Store as User Store
    participant API as Team API
    participant DB as FPTeam Model

    UI->>Store: createTeam(teamName)
    Store->>API: POST /api/method/forms_pro.api.team.create_team {team_name}
    API->>DB: insert FPTeam document
    DB-->>API: after_insert -> add_to_team(owner), add_docshare, set_current_team
    API-->>Store: 200 {team_id, name}
    Store->>Store: reload userTeamsResource
    Store-->>UI: update currentTeam and menu
Loading
sequenceDiagram
    participant UI as Frontend UI
    participant Store as User Store
    participant API as Team API
    participant Defaults as User Defaults

    UI->>Store: switchTeam(team)
    Store->>API: POST /api/method/forms_pro.api.team.switch_team {team_id}
    API->>Defaults: set_current_team(user, team_id)
    API-->>Store: 200 OK
    Store->>Store: reload userTeamsResource -> fetch members
    Store-->>UI: update header/menu
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • feat: teams v1 #5: Introduced team utilities and initial teams feature; this PR extends that surface by adding create/switch endpoints and FPTeam lifecycle integrations.

Poem

🐰 I hopped in code to stitch a team,

I seeded names and set the stream,
A dialog blooms, a watcher sings,
Owner snug with permission wings,
Small paws, big teamwork dream.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: team creation & team switcher functionality' accurately describes the main changes: new team creation endpoints/UI and team switching capabilities throughout the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch team-switcher

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
forms_pro/overrides/roles.py (1)

38-50: ⚠️ Potential issue | 🔴 Critical

Duplicate team member: after_insert already adds the owner.

team.insert() on Line 43 triggers FPTeam.after_insert (see forms_pro/forms_pro/doctype/fp_team/fp_team.py Lines 67-70), which calls self.add_to_team(self.owner) and self.save(). Lines 44-49 then append the same user again and save, creating a duplicate member entry.

Proposed fix — remove the redundant append+save
 def create_default_team_for_user(user: User) -> None:
     from forms_pro.forms_pro.doctype.fp_team.fp_team import FPTeam
 
     team: FPTeam = frappe.new_doc("FP Team")
     team.team_name = f"{user.first_name}'s Team"
     team.insert(ignore_permissions=True)
-    team.append(
-        "users",
-        {
-            "user": user.name,
-        },
-    )
-    team.save(ignore_permissions=True)
forms_pro/forms_pro/doctype/fp_team/fp_team.py (1)

55-65: ⚠️ Potential issue | 🟠 Major

is_team_member always returns False.

self.team_members returns a list of dicts (from model_dump()), but user is a plain string. The in check on Line 65 compares a string against dicts, which never matches. This should compare against the email field of each member.

Proposed fix
 def is_team_member(self, user: str) -> bool:
-    return user in self.team_members
+    return any(m["email"] == user for m in self.team_members)
🤖 Fix all issues with AI agents
In `@forms_pro/api/team.py`:
- Around line 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.

In `@forms_pro/forms_pro/doctype/fp_team/fp_team.py`:
- Around line 72-91: The add_to_team method currently appends a user and creates
a docshare unconditionally; modify add_to_team (in fp_team.FPTeam) to first
check the team child-table (self.users) for an existing entry matching the user
argument and do nothing (or return) if found, otherwise append and call
add_docshare; ensure you reference the users child table lookup (self.users) and
the add_docshare call so both the duplicate-row append and duplicate docshare
are prevented.
- Around line 79-80: The thrown error message is not wrapped for translation:
update the frappe.throw call that uses the variable user and the literal message
in fp_team.py so the user-facing string is wrapped with Frappe's translation
function (use _(...)) — keep the comparison to the user variable (user ==
"Administrator") as-is but change the frappe.throw invocation (the
frappe.throw(...) that currently contains "Administrator cannot be added to a
team") to pass a translated string using _(...) so the linter passes.

In `@frontend/src/components/team/CreateTeamDialog.vue`:
- Around line 42-51: The createTeam function currently calls
user.createTeam(form.team_name) and closes the dialog (model.value = false) even
when validation fails; update createTeam (the function using
formSchema.safeParse, formErrors.value, user.createTeam and model.value) to only
invoke user.createTeam and set model.value = false when result.success is
true—either by moving those two statements inside the else block or returning
early after populating formErrors.value when validation fails, so invalid forms
do not trigger the API call or close the dialog.
- Around line 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.

In `@frontend/src/stores/user.ts`:
- Around line 30-35: The onSuccess handler currently returns early when
_currTeam is undefined, leaving currentTeam null; update onSuccess (the function
handling UserTeam[] responses) to fall back to a sensible team and/or surface
feedback: if data.find(...) yields no _currTeam then pick data[0] (if any) and
call setCurrentTeam with that fallback, otherwise trigger a user
notification/Toast indicating no current team exists so the UI isn’t silently
unchanged; ensure you still handle the empty-array case (leave null and notify)
and keep references to onSuccess, _currTeam, setCurrentTeam and currentTeam for
locating the change.
🧹 Nitpick comments (5)
frontend/src/stores/team.ts (1)

34-36: Guard against null currentTeam and potential double-fetch with initialize().

The watcher fires unconditionally, even when currentTeam.value is null (e.g., during initial load before user teams resolve). This sends a request with team_id: undefined. Additionally, initialize() on Line 30 already calls fetch(), so the watcher may trigger a redundant second fetch when currentTeam is first set.

Suggested fix
 watch(currentTeam, () => {
-    teamMembersResource.fetch();
+    if (currentTeam.value) {
+      teamMembersResource.fetch();
+    }
 });
forms_pro/api/team.py (1)

63-77: Consider using throw=True for consistency with get_team_members.

get_team_members (Line 31) uses frappe.has_permission(..., throw=True) directly, while switch_team manually checks the return value and raises. Using the same pattern would be more consistent and reduce boilerplate.

Proposed fix
 `@frappe.whitelist`(methods=["POST"])
 def switch_team(team_id: str) -> None:
     """
     Switch to a new team
     """
-
-    if not frappe.has_permission(
+    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")
+        throw=True,
+    )
 
     set_current_team(team_id, frappe.session.user)
frontend/src/stores/user.ts (1)

65-84: switchTeam and createTeam are fire-and-forget — callers can't await completion.

Both functions create a one-shot resource and call .submit() without returning a promise. This means callers (e.g., CreateTeamDialog.vue) have no way to wait for success/failure before taking follow-up actions like closing a dialog. Consider returning the promise so callers can await the result:

Sketch for `createTeam` (apply similarly to `switchTeam`)
-  function createTeam(teamName: string) {
-    createResource({
+  function createTeam(teamName: string) {
+    return createResource({
       url: "forms_pro.api.team.create_team",
       ...
     }).submit();
   }

Also applies to: 86-105

frontend/src/layouts/BaseLayout.vue (2)

68-77: Props shadow computed properties with identical names — consider renaming.

sidebarHeader and sidebarSections exist as both props (lines 69, 73) and computed properties (lines 79, 113). Vue's <script setup> resolves the template bindings to the local computed (which internally reads props.sidebarHeader / props.sidebarSections), so this works — but the name collision is a readability trap.

A simple rename of the props (e.g., headerOverride / sectionsOverride) or the computed properties (e.g., resolvedHeader / resolvedSections) would make the intent self-documenting.

Also applies to: 79-111, 113-133


118-130: isActive: true is hardcoded — Dashboard will appear selected on every page.

Once additional sidebar items are introduced, this will be visually misleading. Consider deriving isActive from the current route:

♻️ Suggested fix
+import { useRoute } from "vue-router";
+
+const route = useRoute();
                 {
                     label: "Dashboard",
                     to: "/",
-                    isActive: true,
+                    isActive: route.path === "/",
                     icon: LayoutDashboard,
                 },

Comment thread forms_pro/api/team.py
Comment on lines +45 to +60
@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
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.

Comment thread forms_pro/forms_pro/doctype/fp_team/fp_team.py
Comment thread forms_pro/forms_pro/doctype/fp_team/fp_team.py Outdated
Comment on lines +71 to +73
<Button @click="createTeam" :disabled="formErrors !== ''" variant="subtle">
Create Team
</Button>
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.

Comment thread frontend/src/stores/user.ts
- Added a check to ensure that a user cannot be added to the team if they are already a member, throwing a DuplicateEntryError if the condition is met.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@forms_pro/forms_pro/doctype/fp_team/test_fp_team.py`:
- Around line 32-39: The test sets the user with frappe.set_user(self.test_user)
but does not guarantee restoration if team.insert() or team.reload() raises;
wrap the user change in a safety mechanism such as using self.addCleanup(lambda:
frappe.set_user("Administrator")) immediately after setting the user, or convert
the block to a try/finally where frappe.set_user("Administrator") is called in
the finally; ensure you reference the existing calls (frappe.set_user,
team.insert, team.reload) so the restore always runs even on exceptions.
🧹 Nitpick comments (4)
forms_pro/install.py (1)

4-5: Production module imports from test package.

install.py runs after_install in production, yet it unconditionally imports from forms_pro.tests at module level. Today the test module is trivial (a single constant), so this won't break anything, but it creates an undesirable production → test dependency. Consider either moving the constant to a shared non-test module, or deferring the import inside before_tests/create_test_user.

forms_pro/forms_pro/doctype/fp_team/fp_team.py (2)

66-69: self.save() inside after_insert — consider using db_update or flags to avoid re-triggering the full save lifecycle.

Calling self.save() within after_insert re-runs validate, before_save, on_update, and any associated hooks. This works today but is fragile — future hooks added to on_update or before_save will silently run a second time on every team creation. A lighter approach is to directly insert the child row and skip the full lifecycle:

Proposed alternative
 def after_insert(self) -> None:
     self.add_to_team(self.owner)
     set_current_team(self.name, self.owner)
-    self.save()
+    self.save(ignore_permissions=True)

Or even better, insert the child record and docshare directly (avoiding a second full save cycle) and call self.db_update() if only field-level changes need persisting.


34-35: cached_property for team_members may return stale data after add_to_team mutates self.users.

team_members is a @cached_property, so once accessed it is frozen for the lifetime of the instance. If any code path accesses team_members (or is_team_member) before the team is saved/reloaded, then calls add_to_team, subsequent is_team_member checks on the same instance will miss the newly added member. This is safe today only because the test explicitly calls reload(), but it's a latent trap.

Consider either:

  • Switching to a regular @property (small perf cost, always correct), or
  • Invalidating the cache in add_to_team by deleting the cached attribute.
Option B: Invalidate cache in add_to_team
 def add_to_team(self, user: str) -> None:
     ...
     self.append("users", {"user": user})
+    # Invalidate cached team_members so is_team_member sees the new user
+    self.__dict__.pop("team_members", None)
     add_docshare(

Also applies to: 64-64

forms_pro/forms_pro/doctype/fp_team/test_fp_team.py (1)

44-44: Copy-paste comment: line 44 duplicates line 41.

Line 44 says "Check that the user is added to the team" but the assertion on lines 45–55 actually verifies the FP Team Member child record exists. Update the comment to reflect what's being checked.

-        # Check that the user is added to the team
+        # Check that the FP Team Member record exists for the user

Comment thread forms_pro/forms_pro/doctype/fp_team/test_fp_team.py
- Updated createTeam function to return early on validation failure, preventing unnecessary team creation attempts.
- Ensured formErrors are cleared appropriately based on validation results.
- Added a check to set the first team as the current team if no current team is found in the user store.
@harshtandiya harshtandiya merged commit b756fa0 into develop Feb 7, 2026
3 of 4 checks passed
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
forms_pro/forms_pro/doctype/fp_team/fp_team.py (1)

54-64: ⚠️ Potential issue | 🟠 Major

is_team_member triggers N+1 queries unnecessarily.

self.team_members calls get_user() for every member (one DB query each) just to build a list of emails. Since self.users already contains the user email in the child-table rows, the membership check can be done without any extra queries:

Proposed fix
     def is_team_member(self, user: str) -> bool:
-        return user in [member["email"] for member in self.team_members]
+        return any(member.user == user for member in self.users)

This also eliminates the issue where team_members returns early with [] for a team with no persisted members yet (e.g., during after_insert before save()), since self.users reflects the in-memory child-table state.

🤖 Fix all issues with AI agents
In `@frontend/src/components/team/CreateTeamDialog.vue`:
- 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.
- Around line 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.

In `@frontend/src/stores/user.ts`:
- Around line 68-87: The switchTeam and createTeam functions currently call
createResource(...).submit() and don't return its promise, making them
fire-and-forget; change both functions (switchTeam and createTeam) to return the
promise from createResource(...).submit() so callers can await the result,
handle success/failure (e.g., close dialog only on success), and optionally
dedupe/cancel in caller code; locate the createResource(...) usages in
switchTeam and createTeam and ensure their submit() return value is propagated
(so user.createTeam can be awaited from CreateTeamDialog.vue).
🧹 Nitpick comments (5)
frontend/src/stores/user.ts (2)

60-62: getCurrentTeamFromAllTeams is not exported and appears unused.

This function is defined but not included in the store's return object (lines 124–135), making it dead code.


98-98: Unused data parameter in onSuccess callback.

The data: CreateTeamResponse parameter is declared but never referenced. Remove it or use it (e.g., to auto-switch to the newly created team).

forms_pro/forms_pro/doctype/fp_team/fp_team.py (3)

81-85: Duplicate member check throws instead of returning silently — verify this is intentional.

In after_insert, the owner is always added via add_to_team. If, for any reason, the owner is already present in self.users at that point, this will throw DuplicateEntryError and abort the entire insert flow. The Administrator check on Line 78 returns silently — consider whether the duplicate case should also return silently (or log a warning) rather than hard-fail with frappe.throw.

Option: silent return for duplicates
         if self.is_team_member(user):
-            frappe.throw(
-                frappe._("User is already a member of the team"),
-                frappe.DuplicateEntryError,
-            )
+            return

If the hard error is intentional for external callers (e.g., an explicit "add member" API), consider splitting the internal after_insert path from the public-facing path.


71-96: add_to_team modifies the document in-memory but doesn't persist — relies on caller to save().

self.append(...) modifies the child table in memory; add_docshare(...) writes directly to the DB. If the caller forgets to save (or save() fails), you get a docshare without the corresponding team membership row. This asymmetry could leave data in an inconsistent state.

This is currently safe because after_insert calls self.save() afterwards, but any future caller of add_to_team must remember to also persist. Consider documenting this contract or having add_to_team call self.save() itself.


66-69: Child table persistence requires self.save()—this call is necessary, not avoidable.

The self.append("users", {"user": user}) in add_to_team() creates an in-memory child row that is not persisted to the database until self.save() is called. The test at test_fp_team.py:31-59 confirms this is working correctly.

That said, calling save() in after_insert() is unconventional and can mask permission issues. Since no hooks (before_save, validate, on_update, etc.) currently exist in this doctype, the immediate risk of side effects is low. However, for better safety and clarity, consider:

  • Using self.save(ignore_permissions=True) if the owner is guaranteed to have write access post-insert, or
  • Using self.db_update() combined with explicit db_insert() for child rows to avoid re-triggering the full save lifecycle.

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.

Comment on lines +42 to +51
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;
}
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.

Comment on lines +68 to +87
function switchTeam(team: UserTeam) {
createResource({
url: "forms_pro.api.team.switch_team",
method: "POST",
makeParams() {
return {
team_id: team.name,
};
},
onSuccess() {
userTeamsResource.reload();
toast.success("Team switched successfully");
},
onError(error: Error) {
toast.error("Failed to switch team", {
description: error.message,
});
},
}).submit();
}
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 | 🟠 Major

switchTeam and createTeam are fire-and-forget — callers cannot await completion.

Both functions create an ephemeral resource via createResource(...).submit() without returning a promise. This means:

  1. In CreateTeamDialog.vue, the dialog closes immediately (model.value = false) before the API responds — if creation fails, the user sees a closed dialog + an error toast, which is a confusing UX.
  2. Rapid successive calls create orphaned resource instances with no way to cancel or deduplicate them.

Consider returning the promise from .submit() so callers can await the result and react accordingly (e.g., close the dialog only on success).

Sketch
- function createTeam(teamName: string) {
-   createResource({
+ async function createTeam(teamName: string) {
+   return createResource({
      url: "forms_pro.api.team.create_team",
      ...
-   }).submit();
+   }).submit();
  }

Then in CreateTeamDialog.vue:

async function createTeam() {
    // ...validation...
    await user.createTeam(form.team_name);
    model.value = false;
}

Also applies to: 89-108

🤖 Prompt for AI Agents
In `@frontend/src/stores/user.ts` around lines 68 - 87, The switchTeam and
createTeam functions currently call createResource(...).submit() and don't
return its promise, making them fire-and-forget; change both functions
(switchTeam and createTeam) to return the promise from
createResource(...).submit() so callers can await the result, handle
success/failure (e.g., close dialog only on success), and optionally
dedupe/cancel in caller code; locate the createResource(...) usages in
switchTeam and createTeam and ensure their submit() return value is propagated
(so user.createTeam can be awaited from CreateTeamDialog.vue).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant