Hello @deluan, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello team, gemini-code-assist here to provide a summary of this pull request. This PR primarily addresses a bug in the GetUser Subsonic endpoint where the jukeboxRole was incorrectly set to true for all users when the jukebox feature was enabled, regardless of the Jukebox.AdminOnly configuration setting. It also includes a refactoring to consolidate user response logic and adds comprehensive tests to cover the fix and ensure consistency between GetUser and GetUsers endpoints.

Highlights

• Bug Fix: JukeboxRole Logic: Corrected the logic in the GetUser endpoint to properly respect the conf.Server.Jukebox.AdminOnly setting when determining the jukeboxRole for a user. The logic is now !conf.Server.Jukebox.AdminOnly || user.IsAdmin.
• Code Refactoring: Extracted the common logic for building a user response object into a new helper function buildUserResponse. Both GetUser and GetUsers now utilize this function, reducing code duplication. This refactoring also fixes an inconsistency where GetUsers was using loggedUser.Name instead of loggedUser.UserName.
• Comprehensive Tests Added: Introduced a new test file (server/subsonic/users_test.go) with tests covering the happy path (consistency between GetUser and GetUsers) and specific DescribeTable tests to validate the JukeboxRole logic across all relevant configuration and user type scenarios.

Changelog

• server/subsonic/users.go
  • Added new helper function buildUserResponse (lines 12-29) to centralize user response creation logic.
  • Updated GetUser (lines 32-42) to use the new buildUserResponse helper.
  • Updated GetUsers (lines 46-53) to use the new buildUserResponse helper, fixing the Username field inconsistency and applying the correct JukeboxRole logic via the helper.
• server/subsonic/users_test.go
  • Added a new test file (users_test.go) for the Subsonic users API.
  • Included a 'Happy path' test (lines 33-78) to verify consistency between GetUser and GetUsers responses.
  • Added a DescribeTable test (lines 80-95) specifically for validating the JukeboxRole logic under different configurations and user types.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

A bug did hide,
Jukebox role defied.
Refactor came,
Tests played the game,
Now logic's applied.

Code Review

This is an excellent pull request that effectively addresses the JukeboxRole bug and significantly improves code maintainability through refactoring. The introduction of the buildUserResponse helper function is a great way to centralize user response logic and eliminate duplication. The fix for the username field inconsistency is also well-handled by this refactoring.

The added tests are comprehensive and very well-structured, particularly the use of DescribeTable for testing various jukebox permission scenarios. This ensures the fix is robust and covers all relevant cases. The "Happy path" test also provides good confidence in the consistency between GetUser and GetUsers.

The code adheres to common Go best practices and is clear and easy to understand. The PR description itself is also very thorough and clearly outlines the changes and their rationale.

Overall, this is a high-quality contribution.

Summary of Findings

• Global Configuration Access in Helper Function: The buildUserResponse function directly accesses the global conf.Server variable (e.g., conf.Server.Jukebox, conf.Server.EnableDownloads). While this is consistent with some existing patterns in the codebase and works correctly, an alternative approach for future consideration could be to pass the necessary configuration values as parameters to buildUserResponse. This would make the function more self-contained, improve its testability in complete isolation (without relying on global state setup like configtest.SetupConfig()), and make its dependencies more explicit. This is a minor point and considered low severity, thus no specific review comment was added per the review settings.

Merge Readiness

The pull request is in excellent shape. The bug fix is correct, the refactoring improves the codebase, and the tests are thorough. From my perspective, after considering the findings, the code is ready to be merged. As I am an AI assistant, I am not authorized to approve pull requests; please ensure it undergoes the standard review and approval process by project maintainers.