Skip to content

[PM-24788] Remove Org-Based Policy Requirements Queries#7038

Merged
sven-bitwarden merged 7 commits intomainfrom
ac/pm-24788/refactor-policies-confirm-org-user
Mar 3, 2026
Merged

[PM-24788] Remove Org-Based Policy Requirements Queries#7038
sven-bitwarden merged 7 commits intomainfrom
ac/pm-24788/refactor-policies-confirm-org-user

Conversation

@sven-bitwarden
Copy link
Copy Markdown
Contributor

@sven-bitwarden sven-bitwarden commented Feb 18, 2026

🎟️ Tracking

PM-24788

📔 Objective

Previously within the policy requirements feature, we could only gather enforcement data for individual users, or for organizations. #6876 enabled retrieving policy requirements for multiple users, which is much more efficient in scenarios where out of an entire organization, we need policy requirements for a handful of users.

This PR adjusts the areas that had previously used the by-org query to load by the user IDs on hand. It also changes the OrgDataOwnershipPolicyRequirement to return a helpful (pre-existing) model that gives back the org user ID it's applicable to.

@sven-bitwarden sven-bitwarden requested a review from a team as a code owner February 18, 2026 23:30
@sven-bitwarden sven-bitwarden added needs-qa ai-review Request a Claude code review labels Feb 18, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Feb 18, 2026

Claude finished @bnagawiecki's task in 3m 58s —— View job


Re-Review: PM-24788 Remove Org-Based Policy Requirements Queries

Previous Finding Status

Previous Severity Finding Status
⛔ CRITICAL 4 test methods referencing removed GetManyByOrganizationIdAsync method RESOLVED in 333a06ec9

Incremental Changes Reviewed (since last review at 333a06ec9)

Commit Description Assessment
14270061d Merge from main Clean merge; brought formatting fix in constructor and DefaultUserCollectionRestore FF removal (from PR #7071)

Verification

  • No remaining references to GetManyByOrganizationIdAsync<T>() or RequiresDefaultCollectionOnConfirm anywhere in the codebase
  • Merge from main cleanly applied: DefaultUserCollectionRestore feature flag removal in RestoreOrganizationUserCommand now runs default collection logic unconditionally, consistent with the rest of this PR's approach
  • Minor constructor formatting fix in ConfirmOrganizationUserCommand is cosmetic only
  • All test mocks correctly use GetAsync<T>(IEnumerable<Guid>) with appropriate Arg.Is matchers
  • UseMyItemsDisabled tests correctly omit stale mock setups (code returns early before policy query)

Result

Approved -- All previous findings resolved. Merge from main introduces no new issues.

.ToList();

if (confirmedOrganizationUsers.Count == 1)
if (confirmedOrganizationUsers.Count > 0)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The difference between these two methods (== 1 vs > 1) was whether or not it used the by-org or by-user policy requirements query.

Since we removed the by-org query, and the by-user query delegates to the by-multiple-users query, these methods don't need to be separate anymore.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 18, 2026

Logo
Checkmarx One – Scan Summary & Details08b0db9d-dd3e-4153-b645-ca2afc8a178f

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 19, 2026

Codecov Report

❌ Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.44%. Comparing base (07ead26) to head (1427006).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...s/RestoreUser/v1/RestoreOrganizationUserCommand.cs 90.90% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7038      +/-   ##
==========================================
- Coverage   56.45%   56.44%   -0.01%     
==========================================
  Files        2013     2013              
  Lines       88109    88089      -20     
  Branches     7862     7855       -7     
==========================================
- Hits        49739    49719      -20     
- Misses      36554    36555       +1     
+ Partials     1816     1815       -1     

☔ 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.

r-tome
r-tome previously approved these changes Feb 19, 2026
Copy link
Copy Markdown
Contributor

@r-tome r-tome left a comment

Choose a reason for hiding this comment

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

Looks great!

r-tome
r-tome previously approved these changes Feb 20, 2026
# Conflicts:
#	src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs
#	src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RestoreUser/v1/RestoreOrganizationUserCommand.cs
@sven-bitwarden
Copy link
Copy Markdown
Contributor Author

@r-tome fixed some merge conflicts and got the tests restored

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Mar 2, 2026

@sven-bitwarden sven-bitwarden merged commit 5be9ccd into main Mar 3, 2026
42 of 43 checks passed
@sven-bitwarden sven-bitwarden deleted the ac/pm-24788/refactor-policies-confirm-org-user branch March 3, 2026 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants