Skip to content

PM-31847 Wire up UseMyItems organization ability#7039

Merged
eliykat merged 7 commits intomainfrom
ac/pm-31847/server-add-option-to-disable-my-items-in-bitwarden-portal
Feb 27, 2026
Merged

PM-31847 Wire up UseMyItems organization ability#7039
eliykat merged 7 commits intomainfrom
ac/pm-31847/server-add-option-to-disable-my-items-in-bitwarden-portal

Conversation

@eliykat
Copy link
Copy Markdown
Member

@eliykat eliykat commented Feb 19, 2026

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-31847

📔 Objective

Follow on from #7014 - now actually wire up this new ability flag into domain code so it can enable/disable creation of My Items collections.

There is some code duplication between ~5 different locations which create My Items collections. However, I have not DRY'd this up because:

  • this is a time limited request and we need to avoid large refactors
  • it's not clear how to do that, particularly because different code paths use a different PolicyRequirements interface

📸 Screenshots

@eliykat eliykat added the ai-review Request a Claude code review label Feb 19, 2026
@bitwarden bitwarden deleted a comment from claude Bot Feb 19, 2026
@eliykat eliykat closed this Feb 19, 2026
@eliykat eliykat reopened this Feb 19, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Feb 19, 2026

Claude finished @eliykat's task in 2m 59s —— View job


Re-review: PM-31847 Wire up UseMyItems organization ability

Verdict: Approve -- Incremental changes since last review are clean improvements that address prior feedback.

What changed since last review (5 commits)

Commit Assessment
Pass down org instead of re-fetching Addresses the previous SUGGESTED finding. RestoreOrganizationUserCommand.CreateDefaultCollectionsForConfirmedUsersAsync now accepts Organization directly, eliminating a redundant GetByIdAsync call.
Avoid using orgability cache OrganizationDataOwnershipPolicyValidator switched from IApplicationCacheService (noted as flaky) to IOrganizationRepository with proper null check and InvalidOperationException. FIXME with ticket PM-32699 tracks reverting to cache once reliability is resolved.
DRY up and rearrange tests Test setup extracted into SetupRepositoryMocks helper in AutomaticallyConfirmOrganizationUserCommandTests. Removed unnecessary PlanType assignments. Consistent Arrange/Act/Assert ordering.
dotnet format Formatting only (BOM, unused using removal).
Update ticket link FIXME comment URL updated to PM-32699.

Key observations on incremental changes

  • Correctness improvement: The old IApplicationCacheService code silently returned when orgAbility == null, masking potential issues. The new code correctly throws InvalidOperationException if the organization is not found, since a policy update on a non-existent organization is an invalid state.
  • Guard clause reordering: In RestoreOrganizationUserCommand.CreateDefaultCollectionsForConfirmedUsersAsync, the UseMyItems check is now the first guard clause (before the collection name string check), which is a minor efficiency improvement and reads better logically.
  • Test coverage: New ExecuteSideEffectsAsync_OrganizationNotFound_ThrowsInvalidOperationException test covers the null organization path. ArrangeSut helper now accepts useMyItems parameter with true default, preserving existing test behavior.

No new findings.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 19, 2026

Logo
Checkmarx One – Scan Summary & Detailsb7e128ca-9c37-49d3-a2a0-30242b44fb1a


New Issues (1) Checkmarx found the following issues in this Pull Request
# Severity Issue Source File / Package Checkmarx Insight
1 MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 291
detailsMethod at line 291 of /src/Api/Auth/Controllers/AccountsController.cs gets a parameter from a user request from model. This parameter value flow...
Attack Vector

Fixed Issues (1) Great job! The following issues were fixed in this Pull Request
Severity Issue Source File / Package
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 293

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 19, 2026

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7039      +/-   ##
==========================================
- Coverage   60.75%   56.75%   -4.00%     
==========================================
  Files        2013     2013              
  Lines       88175    88195      +20     
  Branches     7851     7856       +5     
==========================================
- Hits        53570    50059    -3511     
- Misses      32698    36313    +3615     
+ Partials     1907     1823      -84     

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

@eliykat eliykat marked this pull request as ready for review February 20, 2026 07:41
@eliykat eliykat requested a review from a team as a code owner February 20, 2026 07:41
@eliykat eliykat requested a review from JimmyVo16 February 20, 2026 07:41

private async Task UpsertDefaultCollectionsForUsersAsync(PolicyUpdate policyUpdate, string defaultCollectionName)
{
// FIXME: we should use the organizationAbility cache here, but it is currently flaky
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.

Hey @eliykat , I’m curious, how are we tracking this? I totally agree with the decision here, but I want to make sure it doesn’t get lost. Maybe we should add this to the ability cache initiative. It doesn’t have to be included in epic PM-32104.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good question, I do think that epic is probably the best place to track this (and any other instance where we've sidestepped the cache). I'll add a ticket as follow-up work after release of that feature.

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.

Okay, I added it to the tech spec.

JimmyVo16
JimmyVo16 previously approved these changes Feb 23, 2026
Base automatically changed from ac/pm-32131/server-add-usemyitems-organization-ability to main February 25, 2026 02:52
@eliykat eliykat dismissed JimmyVo16’s stale review February 25, 2026 02:52

The base branch was changed.

@rkac-bw rkac-bw requested review from a team as code owners February 25, 2026 02:52
@rkac-bw rkac-bw requested a review from kdenney February 25, 2026 02:52
@eliykat eliykat force-pushed the ac/pm-31847/server-add-option-to-disable-my-items-in-bitwarden-portal branch from ab45516 to 5df160b Compare February 25, 2026 02:55
@eliykat eliykat removed request for a team and kdenney February 25, 2026 02:56
@eliykat
Copy link
Copy Markdown
Member Author

eliykat commented Feb 25, 2026

Some additional reviewers were automatically requested for review when the base branch changed. Sorry about that! If you're not a code owner, please ignore.

@eliykat eliykat force-pushed the ac/pm-31847/server-add-option-to-disable-my-items-in-bitwarden-portal branch from 5df160b to 8bf54b5 Compare February 25, 2026 02:58
@eliykat eliykat requested a review from JimmyVo16 February 25, 2026 05:28
@sonarqubecloud
Copy link
Copy Markdown

@eliykat eliykat merged commit 4158056 into main Feb 27, 2026
51 checks passed
@eliykat eliykat deleted the ac/pm-31847/server-add-option-to-disable-my-items-in-bitwarden-portal branch February 27, 2026 01:28
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.

2 participants