Skip to content

Conversation

@voommen-livefront
Copy link
Collaborator

🎟️ Tracking

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

📔 Objective

On the server side PR (bitwarden/server#6625) we add the UsePhishingBlocker flag to dbo.Organization
In this PR, we ensure that the UsePhishingBlocker property is available in the Organization entity on the client side.

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@voommen-livefront voommen-livefront requested a review from a team as a code owner November 26, 2025 18:29
@claude
Copy link

claude bot commented Nov 26, 2025

Claude finished @voommen-livefront's task —— View job


Code Review Complete

I've reviewed the changes to add the UsePhishingBlocker property to the Organization entity. The implementation is consistent and follows the established patterns in the codebase.

Summary

The PR successfully adds the usePhishingBlocker boolean property across all Organization model layers (response models, data models, and domain models). The property is properly initialized with a default value of false using the nullish coalescing operator (?? false) in the response classes, and the test data has been updated accordingly.

Findings

Finding 1: Missing usePhishingBlocker property in test fixture data

💭 The test file organization.spec.ts (line 88) contains a beforeEach block that creates test OrganizationData, but it's missing the new usePhishingBlocker property. While the tests still pass (TypeScript isn't enforcing this due to the @ts-strict-ignore directive), adding it would improve test data completeness and consistency.

Suggested fix

In libs/common/src/admin-console/models/domain/organization.spec.ts, add to the beforeEach data object around line 87:

ssoEnabled: false,
ssoMemberDecryptionType: MemberDecryptionType.MasterPassword,
usePhishingBlocker: false,

Positive Observations

  • ✅ Consistent implementation across all model layers
  • ✅ Proper use of nullish coalescing for default values in response classes
  • ✅ Test data updated in organization.data.spec.ts
  • ✅ Previous review feedback addressed (test was fixed in commit a12a320)
  • ✅ Property placement follows alphabetical/logical ordering with other feature flags

PR Metadata Assessment

The PR title and description are clear and well-structured:

  • Title clearly describes what's being added
  • Objective section explains both the server-side context and client-side goal
  • Proper Jira reference included

@github-actions
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Detailsac3fd7f0-9da7-411d-ad3e-7e73a0296c3b

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

@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.55%. Comparing base (ae0eff5) to head (a12a320).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...n-console/models/response/organization.response.ts 0.00% 1 Missing ⚠️
...e/models/response/profile-organization.response.ts 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #17681      +/-   ##
==========================================
- Coverage   41.56%   41.55%   -0.01%     
==========================================
  Files        3553     3553              
  Lines      102367   102371       +4     
  Branches    15355    15357       +2     
==========================================
- Hits        42547    42542       -5     
- Misses      58027    58035       +8     
- Partials     1793     1794       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants