[SM-1743] feature flag implementation for disableSmAdsForUsers
đī¸ Tracking
https://bitwarden.atlassian.net/jira/software/c/projects/SM/boards/74?assignee=625cb516fd06270069beaf5d&selectedIssue=SM-1743
đ Objective
Add feature flag for disableSmAdsForUsers
đ¸ 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
Checkmarx One â Scan Summary & Details â 66f586c9-485f-4cce-8cb7-a784f668ccae
Great job! No new security vulnerabilities introduced in this pull request
Claude finished @cd-bitwarden's task in 6m 37s ââ View job
Code Review Complete â
I've completed a comprehensive review of this PR. The feature flag implementation follows Bitwarden's patterns correctly and properly propagates through all layers (API â Data â Domain â Service). However, there are some important items to address:
đ´ Must Fix Before Merge
-
Missing Test Coverage - The new
shouldDisableSMAdslogic inproduct-switcher.service.tshas no unit tests. This is critical functionality that needs test coverage for:- User with no organizations with the flag
- Regular User in org with flag enabled (should hide ad)
- Admin/Owner in org with flag enabled (should show ad)
- Edge cases with multiple organizations
-
Revert Tools Formatting Changes - As @harr1424 noted, please revert the formatting-only changes to the 11 files in
libs/tools/generator/. These are unrelated to your feature and make the PR harder to review.
â ī¸ Logic Clarification Needed
The current implementation uses orgs.some() which means the SM ad will be hidden if the user is a regular User type in ANY organization with the flag enabled.
Question: Is this the intended behavior, or should it only hide the ad if:
- ALL organizations have the flag enabled?
- The user is a regular User in their primary/current organization?
Please verify this with product/business requirements and add a code comment documenting the decision.
â What's Working Well
- Feature flag properly propagated through all layers
- Consistent naming convention with existing flags
- Clean, readable implementation
- Test data properly updated
Codecov Report
:x: Patch coverage is 82.92683% with 7 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 41.85%. Comparing base (27d82aa) to head (82ef39b).
:warning: Report is 3 commits behind head on main.
:white_check_mark: All tests successful. No failed tests found.
Additional details and impacted files
@@ Coverage Diff @@
## main #17525 +/- ##
=======================================
Coverage 41.84% 41.85%
=======================================
Files 3589 3589
Lines 104193 104199 +6
Branches 15719 15720 +1
=======================================
+ Hits 43603 43610 +7
+ Misses 58734 58731 -3
- Partials 1856 1858 +2
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Should there be so many formatting changes here? Looks like a bunch of teams are pinged due to formatting changes?
Yeah, if the formatting is intentional, it should be a separate concern, but my guess is an outdated branch basis with a different version of prettier (which has a commit hook to run in our default setups and version-bumped recently-ish).
@cd-bitwarden try running nvm install && npm ci on your local branch, then npm run lint:fix && npm run prettier