Skip to content

[PM-32699] Utilize cache to check ability of useMyItems if available#7922

Merged
JaredScar merged 2 commits into
mainfrom
ac/pm-32699-remove-temp-workarounds-in-consuming-code
Jul 7, 2026
Merged

[PM-32699] Utilize cache to check ability of useMyItems if available#7922
JaredScar merged 2 commits into
mainfrom
ac/pm-32699-remove-temp-workarounds-in-consuming-code

Conversation

@JaredScar

Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

📔 Objective

Uses useMyItems flag from cache if available, otherwise if null/miss, falls back to checking on the Organization model

…ization ability cache

- Added IOrganizationAbilityCacheService to the constructor for improved performance.
- Replaced direct organization repository calls with cache lookups in UpsertDefaultCollectionsForUsersAsync.
- Introduced OrganizationUsesMyItemsAsync method to streamline cache access and fallback logic.
- Updated tests to accommodate the new cache service and ensure correct behavior under various scenarios.
@JaredScar JaredScar requested a review from a team as a code owner July 2, 2026 22:06
@JaredScar JaredScar requested a review from eliykat July 2, 2026 22:06
@JaredScar JaredScar added ai-review Request a Claude code review t:tech-debt Change Type - Tech debt labels Jul 2, 2026
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the refactor of OrganizationDataOwnershipPolicyEventHandler to source the UseMyItems flag from IOrganizationAbilityCacheService instead of reading the full Organization via IOrganizationRepository. The new OrganizationUsesMyItemsAsync helper throws NotFoundException on a cache miss, which matches the prior reviewer's guidance to rely on the now-fixed ability cache. Tests were updated to mock the cache service and cover cache-hit-enabled, cache-hit-disabled, and cache-miss paths. The new constructor dependency resolves cleanly since IOrganizationAbilityCacheService is DI-registered.

Code Review Details

No blocking findings.

  • ❓ : PR description says the handler "falls back to checking on the Organization model" on a cache miss, but the implementation throws NotFoundException instead (per the earlier review discussion). Consider updating the description to reflect the final behavior.
    • src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyEventHandlers/OrganizationDataOwnershipPolicyEventHandler.cs:73

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.33%. Comparing base (e899a9c) to head (5f75d75).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7922      +/-   ##
==========================================
- Coverage   61.35%   61.33%   -0.03%     
==========================================
  Files        2236     2242       +6     
  Lines       98547    98633      +86     
  Branches     8911     8911              
==========================================
+ Hits        60468    60493      +25     
- Misses      35943    36004      +61     
  Partials     2136     2136              

☔ View full report in Codecov by Harness.
📢 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 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Per above

@JaredScar JaredScar requested a review from eliykat July 6, 2026 14:58
… handling and remove organization repository dependency

- Removed IOrganizationRepository from the constructor and refactored OrganizationUsesMyItemsAsync to throw NotFoundException when the organization is not found in the ability cache.
- Updated tests to reflect changes in exception handling and removed unnecessary organization repository calls.
@JaredScar JaredScar merged commit 6a71ed0 into main Jul 7, 2026
43 checks passed
@JaredScar JaredScar deleted the ac/pm-32699-remove-temp-workarounds-in-consuming-code branch July 7, 2026 15:27
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 t:tech-debt Change Type - Tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants