Fix bug in MDM command listing

[getvictor]

Fixes #32996

Checklist for submitter

• Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
  See Changes files for more information.

Testing

• Added/updated automated tests
• QA'd all new/changed functionality manually

Summary by CodeRabbit

• Bug Fixes

  • Corrected team filtering when listing MDM commands to ensure accurate results. Team-scoped and global commands now display correctly for users with appropriate access, resolving cases of missing or incorrect entries when filtering by team.

• Tests

  • Added comprehensive coverage for team-scoped MDM command listings, role-based visibility (team users vs. admins), and hostname ordering to prevent regressions.

[getvictor]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Walkthrough

Adjusted team filtering in ListMDMCommands to reference the correct derived-table alias from the combined MDM commands subquery. Added tests to validate team-scoped and global command visibility across user roles, including ordering by hostname.

Changes

Cohort / File(s)	Summary
MDM datastore query server/datastore/mysql/mdm.go	Updated team filter in ListMDMCommands to use the combined_commands alias produced by getCombinedMDMCommandsQuery; no other logic changes.
MDM tests server/datastore/mysql/mdm_test.go	Added TestListMDMCommandsWithTeamFilter covering team/global command visibility for team users and admins, and ordering by hostname; integrated into TestMDMShared.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as Caller
  participant DS as Datastore (MySQL)
  participant Q as ListMDMCommands
  participant SQL as getCombinedMDMCommandsQuery

  U->>DS: ListMDMCommands(ctx, TeamFilter, opts)
  DS->>Q: Build base query
  Q-->>DS: Subquery alias: combined_commands
  DS->>SQL: Apply team filter on combined_commands.team_id
  SQL-->>DS: Filtered result set
  DS-->>U: Commands list (with pagination/order)
  note over DS,SQL: Team-filter now targets combined_commands alias

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

• sgress454
• mostlikelee
• lukeheath

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch victor/mdm-commands-unknown-column

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

Pre-merge checks

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Description Check	❓ Inconclusive	The PR description only references an issue (Fixes #32996) and includes a minimal checklist with a couple of boxes checked, but it lacks the detailed sections expected by the repository template: a concise summary of the code changes and rationale, a list of modified files or key code paths, explicit testing steps and test results, and database/migration or compatibility notes. Because the description is too brief and does not follow the template structure, I cannot conclusively determine whether the PR meets the repository requirements. This makes the check inconclusive.	Please expand the PR description to include a clear summary of the bug and the implemented fix, enumerate the modified files and key code paths, and provide concrete testing instructions and observed results (including how the new test covers the regression). Also state whether any database migrations or schema changes are required (or confirm none), add the changes file path if applicable, and note any frontend/CLI or compatibility impacts; update the checklist to accurately reflect completed items so the description matches the repository template.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title "Fix bug in MDM command listing" succinctly identifies the primary change and aligns with the changeset summary (a fix to how team filters are applied when listing MDM commands). It is concise, clear, and meaningful for a teammate scanning PR history. It need not include low-level implementation details to be effective.

[coderabbitai]

Actionable comments posted: 1

Caution

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

⚠️ Outside diff range comments (1)

server/datastore/mysql/mdm.go (1)

96-105: Guard against nil listOpts to avoid nil pointer dereference

listOpts is dereferenced later (Filters/ListOptions); initialize it when nil to prevent a panic.

 func (ds *Datastore) ListMDMCommands(
   ctx context.Context,
   tmFilter fleet.TeamFilter,
   listOpts *fleet.MDMCommandListOptions,
 ) ([]*fleet.MDMCommand, error) {
-  if listOpts != nil && listOpts.Filters.HostIdentifier != "" {
+  if listOpts == nil {
+    listOpts = &fleet.MDMCommandListOptions{}
+  }
+  if listOpts.Filters.HostIdentifier != "" {
     // separate codepath for more performant query by host identifier
     return ds.listMDMCommandsByHostIdentifier(ctx, tmFilter, listOpts)
   }

🧹 Nitpick comments (1)

server/datastore/mysql/mdm.go (1)

83-86: Nit: avoid SELECT * from the derived table

Selecting explicit columns instead of SELECT * makes the query resilient to schema/column-order changes and can help the optimizer.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b7363ec and 5f1427c.

📒 Files selected for processing (2)

• server/datastore/mysql/mdm.go (1 hunks)
• server/datastore/mysql/mdm_test.go (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.go

⚙️ CodeRabbit configuration file

When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.

Files:

• server/datastore/mysql/mdm.go
• server/datastore/mysql/mdm_test.go

🔇 Additional comments (2)

server/datastore/mysql/mdm.go (1)

101-104: Correct alias for team scoping in derived table — good fix

Updating the team filter to use the combined_commands alias matches the derived-table alias and prevents the “unknown column h.team_id” error. This correctly scopes results across Apple and Windows rows.

server/datastore/mysql/mdm_test.go (1)

36-36: Wires new regression test into the shared suite — looks good.

No ordering dependencies detected; the shared TruncateTables call will isolate state between cases.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.79%. Comparing base (587eef0) to head (ec7c198).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #32992      +/-   ##
==========================================
- Coverage   63.79%   63.79%   -0.01%     
==========================================
  Files        2044     2044              
  Lines      201367   201371       +4     
  Branches     6686     6686              
==========================================
- Hits       128465   128459       -6     
- Misses      62741    62749       +8     
- Partials    10161    10163       +2     

Flag	Coverage Δ
backend	64.97% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.