Add Targeted platforms column and platform filter to Policies page#44125
Add Targeted platforms column and platform filter to Policies page#44125allenhouchins wants to merge 21 commits into
Conversation
Mirrors the Reports page pattern: a new non-sortable "Targeted platforms" column (rendered via PlatformCell) and a platform filter dropdown (All/macOS/Windows/Linux/ChromeOS) that syncs with the URL's ?platform= query param and resets pagination. On the backend, ListGlobalPolicies, ListTeamPolicies, ListMergedTeamPolicies, CountPolicies, CountMergedTeamPolicies, and the corresponding service/request types now accept an optional platform filter. Filtering is done in SQL with (p.platforms = '' OR FIND_IN_SET(?, p.platforms)) so policies that target all platforms (empty string) always match.
allenhouchins
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44125 +/- ##
==========================================
- Coverage 66.73% 66.73% -0.01%
==========================================
Files 2731 2729 -2
Lines 218550 218712 +162
Branches 10808 10766 -42
==========================================
+ Hits 145858 145963 +105
- Misses 59487 59529 +42
- Partials 13205 13220 +15
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:
|
Expand single-line anonymous assignments of ds.ListGlobalPoliciesFunc into multi-line function literals in gitops_test.go and testing_utils.go for readability and consistent formatting. No behavioral changes — the mocked functions still return nil.
allenhouchins
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Wrap the custom control and platform dropdown in a div with class `${baseClass}__filter-dropdowns` to provide a consistent horizontal layout. Add SCSS for the new class to display items in a row, center-align them, and apply gap spacing ($pad-medium). Affects PoliciesTable.tsx and _styles.scss.
allenhouchins
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Pass the filter-alt iconName to the platform filter dropdown in both PoliciesTable and QueriesTable to show a filter icon, and add a .__platform-dropdown style (width: 200px) in PoliciesTable SCSS to control the dropdown width for consistent layout.
allenhouchins
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
allenhouchins
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
|
@noahtalerman filter box width now matches the width on Reports and the icon has been updated to be consistent with the platform filter on the Hosts page. This is ready for review pending the checks passing. |
allenhouchins
requested review from
a team,
lukeheath and
rachaelshaw
as code owners
![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.
There was a problem hiding this comment.
Pull request overview
Adds a “Targeted platforms” column and a platform filter dropdown to the Policies page, backed by new platform query params and MySQL filtering so policies with empty platforms (all platforms) always match.
Changes:
- Frontend: add non-sortable “Targeted platforms” column (
PlatformCell) and a platform filter dropdown wired to URL query params. - Backend: thread optional
platformthrough service/datastore APIs and apply platform filtering in policy list/count SQL. - Tests/docs: add MySQL integration coverage for platform filtering and document new REST query params.
Reviewed changes
Copilot reviewed 25 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/testing_client.go | Updates test teardown call site for new ListGlobalPolicies(..., platform) signature. |
| server/service/team_policies_test.go | Updates service auth tests for new platform parameter in team policies list. |
| server/service/team_policies.go | Threads platform through team policies list/count endpoints, service, and datastore calls. |
| server/service/global_policies_test.go | Updates service auth tests for new platform parameter in global policies list. |
| server/service/global_policies.go | Threads platform through global policies list/count endpoints, service, and datastore calls. |
| server/mock/service/service_mock.go | Updates mock service function types and methods to include platform. |
| server/mock/datastore_mock.go | Updates mock datastore function types and methods to include platform. |
| server/fleet/service.go | Extends Fleet Service interface with platform args for policies list/count APIs. |
| server/fleet/datastore.go | Extends Fleet Datastore interface with platform args for policies list/count APIs. |
| server/fleet/api_policies.go | Adds optional platform query param to global/team policies list/count requests. |
| server/datastore/mysql/software_installers_test.go | Updates MySQL tests for updated team policies list signature. |
| server/datastore/mysql/policies_test.go | Adds testPoliciesPlatformFilter and updates many call sites for new signatures. |
| server/datastore/mysql/policies.go | Implements platform filtering via platformFilterClause across list/count/team merge paths. |
| frontend/services/entities/team_policies.ts | Adds optional platform param to team policies list/count API calls (normalizes "all"). |
| frontend/services/entities/global_policies.ts | Adds optional platform param to global policies list/count API calls (normalizes "all"). |
| frontend/pages/queries/ManageQueriesPage/components/QueriesTable/QueriesTable.tsx | Adds iconName to platform filter dropdown for queries table. |
| frontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/_styles.scss | Adds layout styles for multiple filter dropdowns and platform dropdown sizing. |
| frontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTableConfig.tsx | Adds “Targeted platforms” column rendered via PlatformCell. |
| frontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTable.tsx | Adds platform dropdown, URL update logic, and combines custom controls. |
| frontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTable.tests.tsx | Adds tests for new column rendering and dropdown presence. |
| frontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsx | Threads platform from URL into react-query keys and API calls; adjusts hide logic to consider platform. |
| docs/REST API/rest-api.md | Documents platform query param for relevant policies list/count endpoints. |
| cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go | Updates gitops integration tests for updated merged policies list signature. |
| cmd/fleetctl/fleetctl/testing_utils/testing_utils.go | Updates fleetctl test mocks for updated policies list signatures. |
| cmd/fleetctl/fleetctl/testing_utils.go | Updates empty gitops mocks for updated policies list signatures. |
| cmd/fleetctl/fleetctl/gitops_test.go | Updates gitops unit tests for updated policies list signatures. |
| changes/policies-targeted-platforms-filter | Adds release note entry for UI + API changes. |
Comments suppressed due to low confidence (1)
server/datastore/mysql/policies.go:873
appendListOptionsToSQLSecurecan add a cursor placeholder (?) and returns the corresponding args, but this code discards the returned args (query, _, err := ...). IfInheritedAfter/cursor-based pagination is used, the generated SQL will include a?without a matching argument and the query will fail. Capture the returned args and append them toargsbefore executing the query.
// We must normalize the name for full Unicode support (Unicode equivalence).
match := norm.NFC.String(opts.MatchQuery)
query, args = searchLike(query, args, match, policySearchColumns...)
query, _, err := appendListOptionsToSQLSecure(query, &opts, policyAllowedOrderKeys)
if err != nil {
return nil, ctxerr.Wrap(ctx, err, "apply list options")
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const page = | ||
| queryParams && queryParams.page ? parseInt(queryParams?.page, 10) : 0; | ||
| const targetedPlatformParam = queryParams?.platform; | ||
| const initialAutomationFilter = (() => { |
There was a problem hiding this comment.
targetedPlatformParam is passed through to API calls and react-query keys without normalization/validation. This can cause inconsistent behavior:
platform=allis treated as “no filter” by the API clients (they omit it) and by the table UI, but it’s still truthy here for hide/isFiltered checks and creates a distinct cache key.- An invalid value (e.g.
platform=macos) will be sent to the API (filtering results) while the dropdown falls back to “All platforms”, making the UI disagree with the data.
Consider normalizing once here (e.g. if value is"all"or notisQueryablePlatform, set toundefined) and then use that normalized value for keys/conditions and props.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Applied in commit 28d57581. targetedPlatformParam is now normalized at the point of derivation using isQueryablePlatform:
const targetedPlatformParam = isQueryablePlatform(queryParams?.platform)
? queryParams?.platform
: undefined;This means:
platform=all→undefined(not in["darwin","windows","linux","chrome"])platform=macos(invalid) →undefined(also not in the list)platform=darwin(valid) →"darwin"passes through
All downstream uses (API calls, react-query cache keys, hide/isFiltered conditions, and props to PoliciesTable) now consistently use the normalized value.
|
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:
WalkthroughAdds a "Targeted platforms" column and a platform filter dropdown to the Policies UI. Introduces an optional 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 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsx (1)
1021-1040:⚠️ Potential issue | 🟡 MinorUpdate stale comment to mention platform filter.
The sibling
renderAutomationFilterhide-comment at line 1109 was updated to list the platform filter, but this comment still reads "no filters (search or automation dropdown)" even though!targetedPlatformParamis now part of the predicate. Worth a quick sync so the two match.📝 Proposed edit
- // Hide count if fetching count || there are errors OR there are no policy results with no filters (search or automation dropdown) + // Hide count if fetching count || there are errors OR there are no policy results with no filters (search, automation dropdown, or platform)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsx` around lines 1021 - 1040, Update the comment above renderPoliciesCountAndLastUpdated to reflect that the hide condition now includes the platform filter; specifically, change the wording "no filters (search or automation dropdown)" to mention the platform filter as well (e.g., "no filters (search, automation dropdown, or platform)"), so the comment stays consistent with the predicate that checks searchQuery, automationFilter, and targetedPlatformParam alongside policyResults and policiesErrors.
🧹 Nitpick comments (4)
frontend/services/entities/global_policies.ts (1)
28-29: Optional: tightenplatformto a union type.The JSDoc already enumerates the valid tokens (
"darwin" | "windows" | "linux" | "chrome"plus"all"for the UI normalization). Promoting those to a shared union type (exported from here and reused byteam_policies.tsandPoliciesTable) would catch typos at compile time and make refactors when platforms are added/removed safer. Not blocking.♻️ Sketch
+export type PolicyTargetedPlatform = "darwin" | "windows" | "linux" | "chrome"; +export type PolicyPlatformFilter = PolicyTargetedPlatform | "all"; + export interface IGlobalPoliciesApiQueryParams { page?: number; perPage?: number; orderKey?: string; orderDirection?: "asc" | "desc"; query?: string; automationType?: GlobalPoliciesAutomationType; - /** Targeted platform to filter policies by (e.g. "darwin"/"windows"/"linux"/"chrome"). */ - platform?: string; + /** Targeted platform to filter policies by. "all" is normalized to no filter. */ + platform?: PolicyPlatformFilter; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/services/entities/global_policies.ts` around lines 28 - 29, The platform field is currently typed as string; tighten it to a shared exported union type (e.g., export type Platform = "darwin" | "windows" | "linux" | "chrome" | "all") and replace the inline platform?: string with platform?: Platform in global_policies.ts; export that Platform type so team_policies.ts and PoliciesTable can import and reuse it to catch typos and simplify future platform changes.server/datastore/mysql/policies_test.go (1)
474-509: Consider asserting team-onlyCountPolicieswith platform filter.This test validates team list + merged count, but it doesn’t explicitly lock down team-only
CountPolicies(..., &team.ID, ..., platform)behavior. Adding those assertions would close a useful regression gap.♻️ Suggested test additions
@@ // ListTeamPolicies with windows: the team has no darwin-only policy, so // we expect team-all (empty platform) + cross-platform + win-linux inherited. teamPols, inherited, err := ds.ListTeamPolicies(ctx, team.ID, fleet.ListOptions{OrderKey: "name"}, fleet.ListOptions{OrderKey: "name"}, "", "windows") require.NoError(t, err) require.Len(t, teamPols, 1) assert.Equal(t, "team-all", teamPols[0].Name) require.Len(t, inherited, 2) inheritedNames := []string{inherited[0].Name, inherited[1].Name} assert.ElementsMatch(t, []string{"cross-platform", "win-linux"}, inheritedNames) + + // Team-only count should match teamPols filtering (no inherited globals). + teamCount, err := ds.CountPolicies(ctx, &team.ID, "", "", "windows") + require.NoError(t, err) + assert.Equal(t, 1, teamCount) // team-all only + + teamCount, err = ds.CountPolicies(ctx, &team.ID, "", "", "darwin") + require.NoError(t, err) + assert.Equal(t, 2, teamCount) // team-all + team-darwin // CountMergedTeamPolicies with platform filter mergedCount, err := ds.CountMergedTeamPolicies(ctx, team.ID, "", "", "darwin") require.NoError(t, err) assert.Equal(t, 4, mergedCount)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/policies_test.go` around lines 474 - 509, Add explicit assertions for team-scoped CountPolicies with platform filters: after creating the team policies (NewTeamPolicy) and before/after the ListTeamPolicies/CountMergedTeamPolicies checks, call CountPolicies for the team (pass the team.ID as the team parameter) with platform="darwin" and assert it returns 2 (team-darwin + team-all), and call CountPolicies with platform="windows" and assert it returns 1 (team-all only); use the existing ds.CountPolicies function name to locate the implementation and add the two require/assert checks analogous to the existing CountMergedTeamPolicies and ListTeamPolicies assertions.frontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTable.tsx (1)
194-201: Minor: memoizecombinedCustomControlfor consistency.
renderPlatformDropdownandhandlePlatformFilterDropdownChangeare bothuseCallback-memoized, butcombinedCustomControlis a new function on every render, which defeats those memos at theTableContainerboundary. Not a correctness issue — purely a consistency/perf nit.♻️ Proposed refactor
- const combinedCustomControl = () => { - return ( - <div className={`${baseClass}__filter-dropdowns`}> - {customControl?.()} - {renderPlatformDropdown()} - </div> - ); - }; + const combinedCustomControl = useCallback( + () => ( + <div className={`${baseClass}__filter-dropdowns`}> + {customControl?.()} + {renderPlatformDropdown()} + </div> + ), + [customControl, renderPlatformDropdown] + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTable.tsx` around lines 194 - 201, combinedCustomControl is recreated each render which defeats callers relying on memoized callbacks; wrap it in useCallback to memoize it. Update the function to const combinedCustomControl = useCallback(() => ( ... ), [customControl, renderPlatformDropdown, baseClass]) so it only changes when its inputs change (include customControl, renderPlatformDropdown and baseClass in the dependency array) and use that memoized callback where TableContainer expects it.server/fleet/datastore.go (1)
868-872: LGTM — consistent signature extension.
ListGlobalPolicies,CountPolicies, andCountMergedTeamPoliciesnow acceptplatformas the trailing parameter, matching the pattern previously used forautomationType. The semantics (empty/"all"→ no filter, otherwiseFIND_IN_SETagainstp.platforms) are implemented safely via parameter binding inserver/datastore/mysql/policies.go(seeplatformFilterClause), so there are no SQL-injection concerns introduced at this layer.One optional nit: since callers pass a bare string where
""and"all"both mean "no filter" and valid tokens are a closed set (e.g.darwin,windows,linux,chrome), a short doc comment on these methods documenting the accepted values would make the contract explicit for future implementers/mocks. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/fleet/datastore.go` around lines 868 - 872, Add brief doc comments to the affected datastore methods (ListGlobalPolicies, CountPolicies, CountMergedTeamPolicies) describing the platform parameter contract: valid tokens (e.g. "darwin", "windows", "linux", "chrome"), and that empty string or "all" means no platform filter is applied; also note that other values are matched against p.platforms via FIND_IN_SET semantics implemented in the MySQL layer. Keep comments short and placed immediately above each method signature so implementers and mocks can rely on the explicit parameter behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTable.tsx`:
- Line 80: The component currently declares router?: InjectedRouter which allows
silent no-ops when router is missing; make the router prop required (change
router?: InjectedRouter to router: InjectedRouter) on the PoliciesTable
component and update all internal uses (replace router?.push(...) with
router.push(...), specifically in the filter-change handlers around the existing
router?.push calls at lines ~126-143) so the TypeScript compiler will force
callers to pass a router; then update any parent/factory that renders
PoliciesTable to provide the InjectedRouter (or wrap PoliciesTable with the
router HOC) so callers compile.
In `@server/service/team_policies.go`:
- Line 234: Add an HTTP integration test that exercises the platform query
binding end-to-end by calling the ListTeamPolicies and CountTeamPolicies
endpoints with ?platform=darwin; create policies with varying Platform values
using existing test helpers, invoke the handlers via the integration test server
(add the test in integration_core_test.go or integration_enterprise_test.go),
assert that responses include only darwin policies and that CountTeamPolicies
returns the matching count, and name the test something like
TestPoliciesPlatformFilterHTTP to mirror the datastore test; this verifies
ListTeamPoliciesRequest.Platform and CountTeamPoliciesRequest.Platform are bound
and forwarded correctly.
---
Outside diff comments:
In `@frontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsx`:
- Around line 1021-1040: Update the comment above
renderPoliciesCountAndLastUpdated to reflect that the hide condition now
includes the platform filter; specifically, change the wording "no filters
(search or automation dropdown)" to mention the platform filter as well (e.g.,
"no filters (search, automation dropdown, or platform)"), so the comment stays
consistent with the predicate that checks searchQuery, automationFilter, and
targetedPlatformParam alongside policyResults and policiesErrors.
---
Nitpick comments:
In
`@frontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTable.tsx`:
- Around line 194-201: combinedCustomControl is recreated each render which
defeats callers relying on memoized callbacks; wrap it in useCallback to memoize
it. Update the function to const combinedCustomControl = useCallback(() => ( ...
), [customControl, renderPlatformDropdown, baseClass]) so it only changes when
its inputs change (include customControl, renderPlatformDropdown and baseClass
in the dependency array) and use that memoized callback where TableContainer
expects it.
In `@frontend/services/entities/global_policies.ts`:
- Around line 28-29: The platform field is currently typed as string; tighten it
to a shared exported union type (e.g., export type Platform = "darwin" |
"windows" | "linux" | "chrome" | "all") and replace the inline platform?: string
with platform?: Platform in global_policies.ts; export that Platform type so
team_policies.ts and PoliciesTable can import and reuse it to catch typos and
simplify future platform changes.
In `@server/datastore/mysql/policies_test.go`:
- Around line 474-509: Add explicit assertions for team-scoped CountPolicies
with platform filters: after creating the team policies (NewTeamPolicy) and
before/after the ListTeamPolicies/CountMergedTeamPolicies checks, call
CountPolicies for the team (pass the team.ID as the team parameter) with
platform="darwin" and assert it returns 2 (team-darwin + team-all), and call
CountPolicies with platform="windows" and assert it returns 1 (team-all only);
use the existing ds.CountPolicies function name to locate the implementation and
add the two require/assert checks analogous to the existing
CountMergedTeamPolicies and ListTeamPolicies assertions.
In `@server/fleet/datastore.go`:
- Around line 868-872: Add brief doc comments to the affected datastore methods
(ListGlobalPolicies, CountPolicies, CountMergedTeamPolicies) describing the
platform parameter contract: valid tokens (e.g. "darwin", "windows", "linux",
"chrome"), and that empty string or "all" means no platform filter is applied;
also note that other values are matched against p.platforms via FIND_IN_SET
semantics implemented in the MySQL layer. Keep comments short and placed
immediately above each method signature so implementers and mocks can rely on
the explicit parameter behavior.
🪄 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: d7e6b88a-8a44-4e7d-a0bc-1d6af92fbaa4
⛔ Files ignored due to path filters (1)
docs/REST API/rest-api.mdis excluded by!**/*.md
📒 Files selected for processing (26)
changes/policies-targeted-platforms-filtercmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/fleetctl/testing_utils.gocmd/fleetctl/fleetctl/testing_utils/testing_utils.gocmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.gofrontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsxfrontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTable.tests.tsxfrontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTable.tsxfrontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTableConfig.tsxfrontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/_styles.scssfrontend/pages/queries/ManageQueriesPage/components/QueriesTable/QueriesTable.tsxfrontend/services/entities/global_policies.tsfrontend/services/entities/team_policies.tsserver/datastore/mysql/policies.goserver/datastore/mysql/policies_test.goserver/datastore/mysql/software_installers_test.goserver/fleet/api_policies.goserver/fleet/datastore.goserver/fleet/service.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/global_policies.goserver/service/global_policies_test.goserver/service/team_policies.goserver/service/team_policies_test.goserver/service/testing_client.go
| } | ||
|
|
||
| tmPols, inheritedPols, err := svc.ListTeamPolicies(ctx, req.TeamID, req.Opts, inheritedListOptions, req.MergeInherited, req.AutomationType) | ||
| tmPols, inheritedPols, err := svc.ListTeamPolicies(ctx, req.TeamID, req.Opts, inheritedListOptions, req.MergeInherited, req.AutomationType, req.Platform) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for integration coverage of the new platform query param on the list/count endpoints
rg -nP -C3 '[?&]platform=' --type=go -g '!vendor/**'
rg -nP -C3 'testPoliciesPlatformFilter' --type=goRepository: fleetdm/fleet
Length of output: 8629
🏁 Script executed:
# Check the full testPoliciesPlatformFilter test implementation
sed -n '400,500p' server/datastore/mysql/policies_test.goRepository: fleetdm/fleet
Length of output: 3858
🏁 Script executed:
# Find the ListTeamPoliciesRequest and CountTeamPoliciesRequest struct definitions
rg -n 'type (ListTeamPoliciesRequest|CountTeamPoliciesRequest)' --type=go -A 10Repository: fleetdm/fleet
Length of output: 1939
🏁 Script executed:
# Find the endpoint handlers for list/count team policies
rg -n 'listTeamPoliciesHandler|countTeamPoliciesHandler|ListTeamPolicies|CountTeamPolicies' --type=go -A 5 | head -100Repository: fleetdm/fleet
Length of output: 8536
🏁 Script executed:
# Search for integration tests that call team policies endpoints with platform parameter
rg -n 'teams.*policies.*platform|platform.*teams.*policies' --type=go -A 5 -B 5Repository: fleetdm/fleet
Length of output: 1249
🏁 Script executed:
# Check if there are any integration tests in integration_core_test.go that test platform filtering
rg -n 'TestTeamPolicies.*' --type=go server/service/integration_core_test.goRepository: fleetdm/fleet
Length of output: 270
🏁 Script executed:
# Look for any test that constructs a URL with platform query param for team policies
rg -n '\/teams\/.*policies.*platform' --type=goRepository: fleetdm/fleet
Length of output: 153
🏁 Script executed:
# Search for integration tests with platform parameter on team policies endpoints
rg -n 'teams.*policies.*\?platform' --type=goRepository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Look for any recent tests that might test platform filtering on team policies
rg -n 'ListTeamPolicies|CountTeamPolicies' --type=go server/service/integration_core_test.go -A 3Repository: fleetdm/fleet
Length of output: 2065
🏁 Script executed:
# Check the test files in service directory for platform filtering tests on team endpoints
fd -e _test.go server/service | xargs grep -l 'platform.*team.*polic\|team.*polic.*platform' 2>/dev/null | head -5Repository: fleetdm/fleet
Length of output: 419
🏁 Script executed:
# Look for all places where platform filtering is tested for team policies
rg -n 'testPoliciesPlatformFilter|ListMergedTeamPolicies|ListTeamPolicies.*platform' --type=go -B 2 -A 2Repository: fleetdm/fleet
Length of output: 18642
🏁 Script executed:
# Check if there are integration tests that test platform parameter at HTTP level for any endpoint
rg -n 'DoJSON.*platform.*=|DoRaw.*platform.*=' --type=go server/service/integration_core_test.go | head -20Repository: fleetdm/fleet
Length of output: 39
Request DTO binding is correct, but add HTTP integration test for platform parameter.
The query:"platform,optional" tags are properly set on both ListTeamPoliciesRequest.Platform (server/fleet/api_policies.go:192) and CountTeamPoliciesRequest.Platform (server/fleet/api_policies.go:212), and both handlers correctly read and pass req.Platform to the service layer. However, there is no HTTP integration test that exercises the ?platform= query parameter on these endpoints. While testPoliciesPlatformFilter in server/datastore/mysql/policies_test.go provides datastore-level coverage of platform filtering for team policies (lines 489–495), add an HTTP-level integration test in integration_core_test.go or integration_enterprise_test.go that calls these endpoints with ?platform=darwin to verify end-to-end binding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/service/team_policies.go` at line 234, Add an HTTP integration test
that exercises the platform query binding end-to-end by calling the
ListTeamPolicies and CountTeamPolicies endpoints with ?platform=darwin; create
policies with varying Platform values using existing test helpers, invoke the
handlers via the integration test server (add the test in
integration_core_test.go or integration_enterprise_test.go), assert that
responses include only darwin policies and that CountTeamPolicies returns the
matching count, and name the test something like TestPoliciesPlatformFilterHTTP
to mirror the datastore test; this verifies ListTeamPoliciesRequest.Platform and
CountTeamPoliciesRequest.Platform are bound and forwarded correctly.
Sweet! FYI @sharon-fdm @rachaelshaw re this quick win: #23737 |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
allenhouchins
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
nulmete
left a comment
nulmete
left a comment
There was a problem hiding this comment.
LGTM. Test failure seems unrelated.
|
@copilot resolve the merge conflicts in this pull request |
…araya-d4e1ba # Conflicts: # frontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTable.tsx # server/fleet/service.go Co-authored-by: allenhouchins <32207388+allenhouchins@users.noreply.github.com>
Merge conflicts resolved in commit
Go build and TypeScript both pass cleanly. |
Adjust test call sites to match updated datastore API: ListGlobalPolicies and ListTeamPolicies gained additional string parameters. Pass empty string(s) in tests so they compile and keep existing assertions. Changes applied to server/datastore/mysql/policies_test.go and server/service/integration_enterprise_test.go.
ListTeamPoliciesFunc/ListMergedTeamPoliciesFunc mocks and svc.ListTeamPolicies calls were missing the new platform string parameter, causing golangci-lint typecheck failures. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@copilot resolve the merge conflicts in this pull request |
…araya-d4e1ba Co-authored-by: allenhouchins <32207388+allenhouchins@users.noreply.github.com>
Merge conflicts resolved in commit |
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.
nulmete
left a comment
nulmete
left a comment
There was a problem hiding this comment.
LGTM overall 👍 I've tested locally and found no issues.
Just two stylistic comments to address:
- Remove the entire .table-container__search-input css block in frontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/_styles.scss L57-68.
- Use t.Context() in server/datastore/mysql/policies_test.go L404.
|
|
||
| func testPoliciesPlatformFilter(t *testing.T, ds *Datastore) { | ||
| user1 := test.NewUser(t, ds, "Alice", "alice@example.com", true) | ||
| ctx := context.Background() |
There was a problem hiding this comment.
use t.Context() (it's test-scoped and ensures proper cleanup - context.Background() does not)
there are other similar occurrences in this file but it's from an older pattern, modifying this one is enough 👍
| width: 100%; | ||
|
|
||
| .table-container__search-input { | ||
| margin-left: 0; |
There was a problem hiding this comment.
I think margin-left: 0 is not needed.
| order: -2; | ||
|
|
||
| .policies-table__filter-dropdowns { | ||
| flex: 1; |
| .form-field--dropdown, | ||
| .policies-table__platform-dropdown { | ||
| flex: 1; | ||
| width: auto; |
There was a problem hiding this comment.
I believe width: auto is not needed as well.
|
|
||
| &__header-left { | ||
| order: 2; | ||
| display: flex; |
There was a problem hiding this comment.
nit: display: flex is already declared as part of the TableContainer styles (see @frontend/components/TableContainer/_styles.scss#L120)
| & .search-field__input-wrapper { | ||
| width: auto; | ||
| } |
There was a problem hiding this comment.
I believe this is not affecting any element. I did a quick check and it doesn't seem we have any elements with this class:
Therefore I'd remove this style block, and I'll put an item on the FE sync agenda to revisit this particular style, since it's also present on other files
| .input-with-icon { | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
not needed as well, width: 100% is also part of the TableContainer styles.
| .table-container__search { | ||
| order: 1; | ||
| width: 100%; | ||
|
|
||
| .table-container__search-input { | ||
| margin-left: 0; | ||
|
|
||
| .input-with-icon { | ||
| width: 100%; | ||
| } | ||
|
|
||
| & .search-field__input-wrapper { | ||
| width: auto; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Seems like this whole block is not needed.
| .table-container__search { | |
| order: 1; | |
| width: 100%; | |
| .table-container__search-input { | |
| margin-left: 0; | |
| .input-with-icon { | |
| width: 100%; | |
| } | |
| & .search-field__input-wrapper { | |
| width: auto; | |
| } | |
| } | |
| } |
Remove unused/duplicated CSS rules in the policies table component (.table-container__search and some flex/width overrides) and simplify filter dropdown styling. In tests, replace context.Background() with t.Context() in testPoliciesPlatformFilter so the test uses the test's context (enabling cancellation/timeouts).
Summary
Adds a "Targeted platforms" column and a platform filter dropdown to the Policies page (
/policies/manage), matching the pattern already used on the Reports page (/queries/manage,ManageQueriesPage).Frontend
PlatformCell, sourced from each policy's comma-separatedplatformfield.customControlon the Policies table, alongside the existing automation filter. Selecting a value pushes a new URL (not a replace), resetspageto 0, and updates theplatformquery param.ManagePoliciesPagereadslocation.query.platformand threads it through to bothglobalPoliciesAPI.loadAll/teamPoliciesAPI.loadAlland the react-query keys, plus the count endpoints. The automation-filter and count "hide" conditions now include the platform filter so they remain visible when only a platform filter is active.frontend/services/entities/global_policies.tsandteam_policies.tsaccept an optionalplatformparam (with"all"normalized toundefined).PoliciesTable.tests.tsx.Backend
Platform string+`query:"platform,optional"`toListGlobalPoliciesRequest,CountGlobalPoliciesRequest,ListTeamPoliciesRequest,CountTeamPoliciesRequest.ListGlobalPolicies,ListTeamPolicies,ListMergedTeamPolicies,CountPolicies,CountMergedTeamPolicies,ListGlobalPolicies/ListTeamPolicieson the service) to accept aplatform stringarg. Mocks and all call sites updated.platformFilterClause:platformsfield) always match regardless of the selected filter.FIND_IN_SETuses a bound parameter (no injection risk).testPoliciesPlatformFiltercovering empty-platform (match-all), per-platform filter, and team/merged paths.Docs
GET /api/v1/fleet/global/policies,GET /api/v1/fleet/fleets/:id/policies, and the corresponding/countendpoints now document theplatformquery param.changes/policies-targeted-platforms-filter.Behavior
platform=all(or missing) returns all policies.platformscolumn is empty OR contains the selected token.Checklist for submitter
Changes file added for user-visible changes in
changes/.Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Local verification:
go build ./...— cleango vet ./server/... ./cmd/... ./ee/...— cleanmake lint-go-incremental— 0 issuestestPoliciesPlatformFiltertest.Summary by CodeRabbit
New Features
Tests