Fleet UI: Allow users with edit access to add automation from policy details page#45239
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR makes PolicyAutomations' onAddAutomation and isAddingAutomation props optional, updates PolicyAutomations to require canEditPolicy for the CTA card, adds a fully wired onAddPatchAutomation handler in PolicyDetailsPage that validates storedPolicy fields, calls teamPoliciesAPI.update, invalidates the ["policy", policyId] React Query cache, shows flash messages, and tracks isAddingAutomation, and adds tests covering CTA visibility, interaction, loading state, and eligibility (including team_id === 0). It also changes PolicyForm to use a nullish check for team_id and passes canEditPolicy={isEditMode}. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/pages/policies/edit/components/PolicyAutomations/PolicyAutomations.tests.tsx`:
- Around line 63-65: Format the multiline expect assertions in
PolicyAutomations.tests (e.g., the expect(screen.getByText(/Automatically patch
Zoom/)).toBeInTheDocument() pattern and the similar expects at the other
locations) so each expect fits on a single line according to Prettier/ESLint
rules; the simplest fix is to run Prettier (npx prettier --write) on
frontend/pages/policies/edit/components/PolicyAutomations/PolicyAutomations.tests.tsx
or manually collapse the broken expect calls into single-line statements for the
assertions referencing screen.getByText and toBeInTheDocument.
In
`@frontend/pages/policies/edit/components/PolicyAutomations/PolicyAutomations.tsx`:
- Line 44: The single-line conditional expression using isPatchPolicy,
hasPatchSoftware, hasSoftwareAutomation, and onAddAutomation violates Prettier
formatting; reformat that expression across multiple lines for readability
(breaking after each &&) so it conforms to prettier/prettier rules and remains
functionally identical, keeping the same short-circuit semantics where
onAddAutomation is only evaluated when the preceding conditions are truthy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f38ebac1-c67b-4060-a951-7f3ef201785b
📒 Files selected for processing (3)
frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsxfrontend/pages/policies/edit/components/PolicyAutomations/PolicyAutomations.tests.tsxfrontend/pages/policies/edit/components/PolicyAutomations/PolicyAutomations.tsx
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45239 +/- ##
========================================
Coverage 66.80% 66.81%
========================================
Files 2721 2722 +1
Lines 218927 218932 +5
Branches 10753 10630 -123
========================================
+ Hits 146263 146269 +6
Misses 59512 59512
+ Partials 13152 13151 -1
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:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes the “Add automation” CTA on the policy details page by wiring it up to actually attach the patch policy’s Fleet-maintained app as an install automation (and hiding the CTA when the user can’t edit the policy).
Changes:
- Make
PolicyAutomations’ add-automation props optional and only show the CTA when a realonAddAutomationhandler is provided. - Add an
onAddPatchAutomationhandler toPolicyDetailsPagethat calls the team policies update API, invalidates the policy query, and shows success/error flash messages. - Add unit tests covering the
PolicyAutomationsCTA rendering/click behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/pages/policies/edit/components/PolicyAutomations/PolicyAutomations.tsx | Make CTA conditional on presence of an add handler; relax prop requirements. |
| frontend/pages/policies/edit/components/PolicyAutomations/PolicyAutomations.tests.tsx | Add tests validating CTA visibility/click and “Adding…” state. |
| frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsx | Implement add-automation behavior on details page (API call + cache invalidation + flash). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const onAddPatchAutomation = async () => { | ||
| if ( | ||
| !storedPolicy?.patch_software?.software_title_id || | ||
| !storedPolicy?.team_id |
There was a problem hiding this comment.
Sorry, I think you're right, we want this available for no team
There was a problem hiding this comment.
Yes — this was broken before your PR too. The onAddPatchAutomation in PolicyForm.tsx had the same !storedPolicy?.team_id guard, so team_id === 0 would have bailed out early and silently done nothing when you clicked "Add automation" for a no-team policy. Your PR carried the same pattern over to PolicyDetailsPage.tsx.
|
@claude review |
There was a problem hiding this comment.
LGTM — straightforward UI change with tests; team_id === 0 fix and reviewer feedback addressed.
Extended reasoning...
Overview
This PR enables the "Add automation" CTA on the policy details page for patch policies (previously only available on the edit page). Changes span 4 frontend files: PolicyDetailsPage.tsx (adds onAddPatchAutomation handler with loading state, flash messages, and query cache invalidation), PolicyAutomations.tsx (makes onAddAutomation/isAddingAutomation optional and hides CTA when no handler is passed), PolicyForm.tsx (fixes the !storedPolicy?.team_id guard to use == null so team_id === 0 no-team policies work), and a new test file with good coverage of CTA visibility conditions including team_id === 0.
Security risks
None. Frontend-only change. The new API call (teamPoliciesAPI.update) is the same one already invoked from the edit page, gated by the same canEditPolicy permission check.
Level of scrutiny
Low — this is a small, scoped UI change with no auth/crypto/permissions impact. The new handler is a direct mirror of the existing one in PolicyForm.tsx, the bug fix (team_id === 0) is a well-understood nullish-vs-falsy correction that the author confirmed was broken pre-PR, and tests cover the eligibility conditions.
Other factors
- Reviewer feedback from Copilot (
team_id === 0) and CodeRabbit (Prettier formatting) was addressed in commits c7d5b22 and 4a7d69e. - Minor untouched nit:
showCtaCard = ... && onAddAutomationmakes the value function-or-falsy rather than a strict boolean, but it's only used in{showCtaCard && ...}JSX where this is harmless. - The
onAddPatchAutomationbody is duplicated betweenPolicyDetailsPage.tsxandPolicyForm.tsx; not worth blocking on but a future refactor candidate. - No bugs found by the bug hunting system.
| if ( | ||
| !storedPolicy?.patch_software?.software_title_id || | ||
| !storedPolicy?.team_id | ||
| storedPolicy?.team_id == null |
There was a problem hiding this comment.
fixes to work on "Unassigned"
There was a problem hiding this comment.
The change looks clean — bug fix for team_id===0 (Unassigned) policies is correct, the canEditPolicy guard is properly threaded through, and test coverage covers the key cases. Deferring since this touches frontend code owned by @fleetdm/frontend.
Extended reasoning...
Overview
This PR enables the 'Add automation' CTA on the policy details page for users with edit access (previously the handler was a no-op). It touches 4 files in frontend/pages/policies/:
PolicyDetailsPage.tsx: wires up a realonAddPatchAutomationhandler that callsteamPoliciesAPI.update, invalidates the policy query cache, and shows flash messagesPolicyAutomations.tsx: adds a requiredcanEditPolicyprop and includes it in theshowCtaCardconditionPolicyForm.tsx: passescanEditPolicy={isEditMode}and fixes the same team_id===0 bug as in details pagePolicyAutomations.tests.tsx: new test file covering CTA visibility, button behavior, loading state, click handling, team_id===0 case
Security risks
None apparent. The change uses an existing API endpoint (teamPoliciesAPI.update) with backend-enforced authorization. The frontend canEditPolicy gate is a UX affordance — actual permission checks live in the API. No new auth, crypto, or data exposure paths.
Level of scrutiny
Light scrutiny appropriate. This is a frontend UI handler wiring + a one-line bug fix (!team_id → team_id == null) duplicated to a second file. The bug fix is correct: team_id of 0 represents 'No team' / Unassigned and was being incorrectly treated as missing due to falsy coercion. Rachel acknowledged this in the timeline and applied the fix to both call sites.
Other factors
- Both Copilot reviewer comments (the boolean coercion for
showCtaCardwas solved via the canEditPolicy guard semantics; the team_id===0 issue was fixed) were addressed in commits c7d5b22 / 4a7d69e / 926754c - Test coverage explicitly includes a
team_id === 0case (line 142-156 of the test file) - No backend changes
- Frontend code is owned by @fleetdm/frontend via CODEOWNERS, so human review is appropriate even though the change is small
Issue
Closes #44379 (and duplicate #44891 )
Description
Screenrecordings
Before behavior
https://fleetdm.zoom.us/clips/share/QqoTqDvgQEeg2pQSBCX6Og
After fix behavior
https://fleetdm.zoom.us/clips/share/xBwNYndgRiO2FiaHao4ABQ
Checklist for submitter
Testing
Summary by CodeRabbit
New Features
UI
Bug Fixes
Tests