Skip to content

Added UserSummary type for UsersByIDs. - #38710

Merged
getvictor merged 2 commits into
mainfrom
victor/38232-UsersByIDs
Jan 23, 2026
Merged

Added UserSummary type for UsersByIDs.#38710
getvictor merged 2 commits into
mainfrom
victor/38232-UsersByIDs

Conversation

@getvictor

@getvictor getvictor commented Jan 23, 2026

Copy link
Copy Markdown
Member

Related issue: Resolves #38234

Addresses Ian's suggestion from activity bounded context code review.

Checklist for submitter

Testing

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

Summary by CodeRabbit

  • Refactor
    • Updated user lookup functionality across the system to return minimal user information instead of full user objects. Changes affect multiple system interfaces and data access layers to optimize performance and reduce data payload for user-related operations throughout the application.

✏️ Tip: You can customize this high-level summary in your review settings.

@getvictor

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jan 23, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

Comment thread server/fleet/users.go Outdated
@coderabbitai

coderabbitai Bot commented Jan 23, 2026

Copy link
Copy Markdown
Contributor

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

A new UserSummary type is introduced as a lightweight user data representation, and the UsersByIDs method signature is updated across interfaces, implementations, and mocks to return user summaries instead of full user objects. The activity ACL adapter's user conversion logic is adjusted accordingly.

Changes

Cohort / File(s) Summary
Core Type Definition
server/fleet/users.go
New exported type UserSummary introduced as a subset view of User, containing essential user fields (ID, Name, Email, APIOnly, GravatarURL, etc.) with JSON and database tags for serialization and persistence. Password and Salt fields are excluded from JSON output.
Interface Updates
server/fleet/datastore.go, server/fleet/service.go
The UsersByIDs method signature updated in both the Datastore interface and the UserLookupService interface to return []*fleet.UserSummary instead of []*fleet.User, with documentation adjusted to indicate minimal user information is returned.
Implementation Updates
server/datastore/mysql/users.go, server/service/users.go
The UsersByIDs method implementations updated to return []*fleet.UserSummary in alignment with interface changes; MySQL implementation adjusts query result type and function documentation.
Activity ACL Adapter
server/acl/activityacl/fleet_adapter.go
The convertUser helper now accepts *fleet.UserSummary instead of *fleet.User, and the returned activity.User now includes Email, Gravatar, and APIOnly fields.
Mock Implementations
server/mock/datastore_mock.go, server/mock/service/service_mock.go
Mock datastore and service implementations updated to align with the new return type; UsersByIDsFunc functional types and UsersByIDs methods now return []*fleet.UserSummary instead of []*fleet.User.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • iansltx
  • lucasmrod
  • JordanMontgomery
🚥 Pre-merge checks | ❌ 5
❌ Failed checks (3 warnings, 2 inconclusive)
Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title describes only a peripheral change (adding UserSummary type) that is a refactoring detail, not the main objective of resolving issue #38232 about removing osquery verification for Windows profiles. Update the title to reflect the primary goal from issue #38232, such as 'Remove osquery verification for Windows profiles' or similar, which represents the actual business objective.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The pull request description is incomplete and missing most required checklist sections from the repository template. Complete the description by adding: changes file documentation (if applicable), data validation notes, backwards compatibility verification, and database migration checks. Provide more context about what this UserSummary change accomplishes.
Linked Issues check ❓ Inconclusive The PR changes focus on refactoring user data structures (UserSummary type and UsersByIDs return types) but the linked issue #38232 concerns Windows profile osquery verification. The connection between these changes and the stated issue objective is unclear. Clarify in the PR description how these UserSummary changes relate to resolving the Windows profile osquery verification issue, or link the correct issue if this refactoring supports a separate objective.
Out of Scope Changes check ❓ Inconclusive All code changes are consistent with introducing a UserSummary type and updating UsersByIDs signatures across the codebase, but the relevance of these changes to issue #38232 about Windows profile osquery verification is unclear, making it difficult to assess scope. Document the relationship between the UserSummary refactoring and the Windows profile issue resolution to clarify whether these changes are in-scope or supporting work.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch victor/38232-UsersByIDs

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/users.go (1)

161-175: Use a summary-specific column list for UsersByIDs.

Line 167 still selects userSelectColumns, which includes password/salt. For UserSummary, fetch only the required fields to reduce sensitive data exposure and I/O.

♻️ Suggested refactor
 const userSelectColumns = `id, created_at, updated_at, password, salt, name, email,
 	admin_forced_password_reset, gravatar_url, position, sso_enabled, global_role,
 	api_only, mfa_enabled, invite_id`
+
+const userSummarySelectColumns = `id, created_at, updated_at, name, email,
+	admin_forced_password_reset, gravatar_url, position, sso_enabled, global_role,
+	api_only, mfa_enabled, invite_id`
@@
-	query, args, err := sqlx.In(
-		fmt.Sprintf("SELECT %s FROM users WHERE id IN (?)", userSelectColumns), ids)
+	query, args, err := sqlx.In(
+		fmt.Sprintf("SELECT %s FROM users WHERE id IN (?)", userSummarySelectColumns), ids)
🤖 Fix all issues with AI agents
In `@server/fleet/users.go`:
- Around line 15-31: Remove the sensitive Password and Salt fields from the
UserSummary struct so credential material is not carried in the minimal summary
type (modify the type declaration for UserSummary to delete the Password and
Salt fields); ensure only the full User struct retains password/salt storage;
update any code that constructs or scans into UserSummary (e.g., functions that
map/scan from DB rows or convert User -> UserSummary) to stop copying or
selecting those columns (remove password/salt from SELECTs or mapping logic) and
run tests to confirm no compile or query errors.

Comment thread server/fleet/users.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new UserSummary type to replace the full User type in the UsersByIDs method. The change is primarily aimed at optimizing data retrieval by returning only the essential user fields needed for enriching activities.

Changes:

  • Introduced UserSummary type containing a subset of User fields (excluding Teams, Settings, and Deleted fields)
  • Updated UsersByIDs method signature across all layers (service, datastore, mocks, and interfaces) to return []*fleet.UserSummary instead of []*fleet.User
  • Updated the activity ACL adapter's convertUser function to accept *fleet.UserSummary instead of *fleet.User

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
server/fleet/users.go Defines new UserSummary struct with essential user fields
server/fleet/service.go Updates UsersByIDs interface method signature to return UserSummary
server/fleet/datastore.go Updates UsersByIDs datastore interface to return UserSummary
server/service/users.go Updates service implementation to return UserSummary
server/datastore/mysql/users.go Updates MySQL implementation to query and return UserSummary
server/acl/activityacl/fleet_adapter.go Updates adapter to convert UserSummary to activity.User
server/mock/service/service_mock.go Updates service mock function signature
server/mock/datastore_mock.go Updates datastore mock function signature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread server/fleet/users.go Outdated
Comment thread server/fleet/users.go Outdated
@codecov

codecov Bot commented Jan 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.07%. Comparing base (af2d8a2) to head (e26dcf6).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #38710      +/-   ##
==========================================
- Coverage   66.07%   66.07%   -0.01%     
==========================================
  Files        2414     2414              
  Lines      192683   192683              
  Branches     8529     8529              
==========================================
- Hits       127321   127320       -1     
  Misses      53799    53799              
- Partials    11563    11564       +1     
Flag Coverage Δ
backend 67.92% <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.

@getvictor
getvictor marked this pull request as ready for review January 23, 2026 19:31
@getvictor
getvictor requested a review from a team as a code owner January 23, 2026 19:31
@getvictor

Copy link
Copy Markdown
Member Author

@iansltx I'm assigning this PR to you, since it addresses your previous PR review comment.

@getvictor
getvictor merged commit 8e68173 into main Jan 23, 2026
48 checks passed
@getvictor
getvictor deleted the victor/38232-UsersByIDs branch January 23, 2026 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Activity bounded context clean up for next PRs

3 participants