[PM-38269] refactor: remove redundant permissions round-trip in OrganizationUserUserDetailsQuery - #8119
[PM-38269] refactor: remove redundant permissions round-trip in OrganizationUserUserDetailsQuery#8119r-tome wants to merge 1 commit into
Conversation
r-tome
added
ai-review
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the removal of the Code Review Details
|
…izationUserUserDetailsQuery For each Custom-role user the query was deserializing the Permissions JSON string then immediately re-serializing it back — a no-op before the string reaches the response model, which deserializes it once anyway. Remove the three identical if (Type == Custom) blocks and the now-unused CoreHelpers import. Add an API integration test to verify permissions survive the full pipeline end-to-end.
0f4bebe to
e0ba63b
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| return o; | ||
| }); | ||
| return organizationUsers; |
There was a problem hiding this comment.
♻️ DEBT: With the round-trip removed, GetOrganizationUserUserDetails is a bare pass-through with no production callers.
Details
The permissions block was the only logic in this method; it now just forwards to _organizationUserRepository.GetManyDetailsByOrganizationAsync. A repo-wide search for GetOrganizationUserUserDetails finds only the interface declaration, this implementation, and test setup — no caller in src/ or bitwarden_license/ (OrganizationUsersController.GetAll uses Get / GetAccountRecoveryEnrolledUsers).
Related stale tests:
test/Api.Test/AdminConsole/Queries/OrganizationUserUserDetailsQueryTests.cs—Get_HandlesNullPermissionsObjectexisted to prove the removedGetPermissions()/ClassToJsonDatapath tolerated a"null"permissions string; it now asserts only that IDs round-trip through a mocked repository.test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs:452—GetMany_Setupstill stubsGetOrganizationUserUserDetails.
Consider dropping the method plus its interface member and these tests in this PR, or leave a note if it is being kept intentionally for an in-flight consumer.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8119 +/- ##
===========================================
+ Coverage 14.99% 62.95% +47.95%
===========================================
Files 1409 2301 +892
Lines 61097 100187 +39090
Branches 4864 9016 +4152
===========================================
+ Hits 9161 63068 +53907
+ Misses 51780 34937 -16843
- Partials 156 2182 +2026 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-38269
📔 Objective
For each Custom-role user,
OrganizationUserUserDetailsQuerywas deserializing thePermissionsJSON string from the database and immediately re-serializing it back to a string — a no-op round-trip before the string reaches the API response model, which deserializes it once anyway.This PR removes the three identical
if (Type == Custom)blocks fromGetOrganizationUserUserDetails,Get, andGetAccountRecoveryEnrolledUsers, and drops the now-unusedusing Bit.Core.Utilitiesimport.An API integration test is added to
OrganizationUsersControllerGetTeststo verify that Custom-user permissions survive the full pipeline (DB → query → response model → JSON) correctly.