Added frontend support for No team automations - #32507
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #32507 +/- ##
=======================================
Coverage 62.13% 62.13%
=======================================
Files 1985 1985
Lines 195470 195462 -8
Branches 6520 6512 -8
=======================================
Hits 121446 121446
+ Misses 64348 64340 -8
Partials 9676 9676
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsx (2)
754-761: Bug: responses.concat(...) doesn’t mutate; policy updates never enqueued.The calendar-events policy updates aren’t added to
responses, so they’re never awaited/executed.Apply:
- // update changed policies calendar events enabled - responses.concat( - formData.changedPolicies.map((changedPolicy) => { - return teamPoliciesAPI.update(changedPolicy.id, { - calendar_events_enabled: changedPolicy.calendar_events_enabled, - team_id: teamIdForApi, - }); - }) - ); + // update changed policies calendar events enabled + responses.push( + ...formData.changedPolicies.map((changedPolicy) => + teamPoliciesAPI.update(changedPolicy.id, { + calendar_events_enabled: changedPolicy.calendar_events_enabled, + team_id: teamIdForApi, + }) + ) + );
821-829: Bug: Same concat issue — conditional access policy updates not enqueued.
responses.concat(...)result is ignored; no policy updates run.- // handle any changed policies for no team or a team - responses.concat( - changedPolicies.map((changedPolicy) => { - return teamPoliciesAPI.update(changedPolicy.id, { - conditional_access_enabled: - changedPolicy.conditional_access_enabled, - team_id: teamIdForApi, - }); - }) - ); + // handle any changed policies for no team or a team + responses.push( + ...changedPolicies.map((changedPolicy) => + teamPoliciesAPI.update(changedPolicy.id, { + conditional_access_enabled: changedPolicy.conditional_access_enabled, + team_id: teamIdForApi, + }) + ) + );
🧹 Nitpick comments (2)
frontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsx (2)
525-538: Guard against undefined teamId before teamsAPI.update in “Other workflows”.Edge-case: if routing glitches leave
teamIdForApiundefined while not on All Teams, this would callupdatewithundefined. Add a quick guard.- } else { - // For any team including "No team" (team ID 0), use the teams API - await teamsAPI.update(requestBody, teamIdForApi); + } else { + // For any team including "No team" (team ID 0), use the teams API + if (teamIdForApi === undefined) { + renderFlash("error", "No team selected."); + return; + } + await teamsAPI.update(requestBody, teamIdForApi); }
666-673: Send updates only for changed policies (avoid unnecessary writes).You compute
changedPoliciesabove but then issue requests for allformData. Prefer updating just the changed subset to reduce load and noise.- const promises = formData.map((changedPolicy) => + const promises = changedPolicies.map((changedPolicy) => teamPoliciesAPI.update(changedPolicy.id, { // "script_id": null will unset running a script for the policy // "script_id": X will sets script X to run when the policy fails script_id: changedPolicy.scriptIdToRun || null, team_id: teamIdForApi, }) );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
frontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsx(7 hunks)frontend/services/entities/teams.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-21T21:10:00.844Z
Learnt from: getvictor
PR: fleetdm/fleet#32129
File: server/service/global_policies.go:398-421
Timestamp: 2025-08-21T21:10:00.844Z
Learning: In Fleet's codebase, there's a distinction between "global policies" and "No team policies": Global policies have TeamID = nil, while "No team" policies have TeamID = 0. This means "No team" policies are treated as belonging to a specific team (team ID 0) rather than being truly global.
Applied to files:
frontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsx
🔇 Additional comments (5)
frontend/services/entities/teams.ts (1)
84-89: Allowing 0 (No team) is correct; validation/readability LGTM.Good strict check (
teamId === undefined || teamId < API_NO_TEAM_ID) and clearer error. This unblocks GET /teams/0 while rejecting negatives.Please confirm BE PR #32387 returns the expected payload for GET /teams/0 and that downstream consumers handle team.id === 0 without special-casing. Also confirm that keeping addUsers/removeUsers rejecting 0 is intended.
frontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsx (4)
395-399: Enable team config query for “No team” (0) — LGTM.Gate now includes 0 via
teamIdForApi !== undefined, matching the new teams.load().QA path: with team_id=0 in URL, verify team config loads and renders automations without 404s once BE #32387 is merged.
958-968: Confirm intent: No team automations use teamConfig, but Conditional Access uses globalConfig for No team.Elsewhere (lines 1123–1126, 788–818) Conditional Access reads/writes globalConfig when teamId=0. If “No team ≙ team 0” should behave like a regular team (per our prior learning), consider moving CA toggles for No team to teamConfig for consistency, or confirm this exception is deliberate.
Note: Using retrieved learning about “No team = team ID 0” vs global.
1127-1224: Dropdown options refactor — LGTM; “Other” now admin-only (not maintainers).Paramless helper simplifies call sites. Visibility rule now effectively admins-only given the dropdown itself is limited to maintainers/admins.
1236-1237: Callsite updated to new signature — LGTM.
2 participants
Fixes #32061
Checklist for submitter
Testing
Summary by CodeRabbit
New Features
Bug Fixes