Fix for ONC profiles losing their details when adding another profile - #45299
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/agentic_review |
Code Review by Qodo
1.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR makes the Android host-profile bulk upsert optionally preserve the 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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.
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/android.go (1)
1463-1474:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the reset to the current team.
This subquery matches incoming profiles by
nameonly, so updating team A can resethost_mdm_android_profilesrows for team B if both teams have a profile with the same name. Please include the currentprofileTeamID/no-team scope in the subquery. As per coding guidelines: "ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity."Suggested fix
WHERE - profile_uuid IN (SELECT profile_uuid FROM mdm_android_configuration_profiles WHERE name IN (?)) + profile_uuid IN ( + SELECT profile_uuid + FROM mdm_android_configuration_profiles + WHERE team_id = ? AND name IN (?) + ) ` - stmt, args, err = sqlx.In(updateIncludedInPolicyVersionStmt, incomingNames) + stmt, args, err = sqlx.In(updateIncludedInPolicyVersionStmt, profileTeamID, incomingNames)🤖 Prompt for 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. In `@server/datastore/mysql/android.go` around lines 1463 - 1474, The UPDATE's subquery (in updateIncludedInPolicyVersionStmt) only matches profiles by name and can touch other teams; restrict the subquery on mdm_android_configuration_profiles to the current team's scope by adding a filter on the profile team identifier (use the same profileTeamID/no-team logic the codebase uses) so the subquery becomes something like "SELECT profile_uuid FROM mdm_android_configuration_profiles WHERE name IN (?) AND (team_id = ? OR (team_id IS NULL AND ? IS NULL))" and ensure the caller passes profileTeamID (or equivalent) as the additional parameter(s) so only profiles for the current team (or no-team when appropriate) are affected.
🤖 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.
Outside diff comments:
In `@server/datastore/mysql/android.go`:
- Around line 1463-1474: The UPDATE's subquery (in
updateIncludedInPolicyVersionStmt) only matches profiles by name and can touch
other teams; restrict the subquery on mdm_android_configuration_profiles to the
current team's scope by adding a filter on the profile team identifier (use the
same profileTeamID/no-team logic the codebase uses) so the subquery becomes
something like "SELECT profile_uuid FROM mdm_android_configuration_profiles
WHERE name IN (?) AND (team_id = ? OR (team_id IS NULL AND ? IS NULL))" and
ensure the caller passes profileTeamID (or equivalent) as the additional
parameter(s) so only profiles for the current team (or no-team when appropriate)
are affected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 49f29d6a-21cf-4ad2-83fb-109194aa016c
📒 Files selected for processing (2)
server/datastore/mysql/android.goserver/service/integration_android_certificate_templates_test.go
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds coverage and datastore changes to prevent Android ONC profiles from losing their detail message (e.g. “Waiting for certificate ...”) when additional profiles are added to the same team.
Changes:
- Adds an integration regression test asserting ONC profile
detailis preserved when adding another Android profile. - Adjusts Android host profile upsert/reset logic to preserve
detailwhile resettingstatustoNULL. - Stops clearing
detailin the “mark for reprocessing” update path so reconciler-owned messages survive state resets.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/service/integration_android_certificate_templates_test.go | Adds an integration test to ensure ONC withheld detail is not wiped when adding another profile. |
| server/datastore/mysql/android.go | Preserves detail during “pending reset” / reprocessing updates and adds an internal upsert helper to control whether detail is updated. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45299 +/- ##
==========================================
+ Coverage 66.72% 66.74% +0.02%
==========================================
Files 2732 2732
Lines 218476 218486 +10
Branches 10706 10706
==========================================
+ Hits 145770 145824 +54
+ Misses 59496 59461 -35
+ Partials 13210 13201 -9
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:
|
![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.
ksykulev
left a comment
ksykulev
left a comment
There was a problem hiding this comment.
Don't love the trailing boolean flag, but the other way to fix this would be to distinguish between ONC-withheld profiles and regular profiles and only preserve detail for the former? Which seems much more complicated.
Related issue: Resolves #42405
Unreleased bug fix.
Checklist for submitter
Testing
Summary by CodeRabbit
Bug Fixes
Tests