[PM-33861] Display organization notification banner on vault list#2798
Conversation
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the addition of the Code Review Details
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2798 +/- ##
==========================================
+ Coverage 80.82% 80.85% +0.02%
==========================================
Files 1018 1018
Lines 64941 64996 +55
==========================================
+ Hits 52490 52551 +61
+ Misses 12451 12445 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /// Dismisses the organization user notification banner. | ||
| /// | ||
| private func dismissOrganizationBanner() async { | ||
| // TODO: PM-33861 Persist banner dismissal data |
There was a problem hiding this comment.
⛏️ This is referencing the same ticket as this PR. Will this be added as part of this PR?
There was a problem hiding this comment.
I'll have a follow-up PR for this work, as there was already a lot here. The dismissal logic was easy to break off from this work.
There was a problem hiding this comment.
Roger, I'll approve, although the TODO should reference the ticket in which the work will be done, but I guess it doesn't matter much here as it will be worked on soon.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| case .sendOptions: self = .sendOptions | ||
| case .twoFactorAuthentication: self = .twoFactorAuthentication | ||
| // TODO: PM-39144 Add SDK mapping for `organizationUserNotification` PolicyType | ||
| case .organizationUserNotification: return nil |
There was a problem hiding this comment.
❓ QUESTION: This nil mapping silently disables the new banner when the policiesInAcceptedState flag is enabled.
Details
getOrganizationUserNotificationBannerData() calls policiesApplyingToUser(.organizationUserNotification). When policiesInAcceptedState is enabled, that routes through sdkFilterPolicies, which returns [] early because BitwardenSdk.PolicyType(.organizationUserNotification) is nil here. The result is that the banner never appears for any user once policiesInAcceptedState rolls out, even though organizationUserNotificationBanner is on.
The getOrganizationUserNotificationBannerData tests only exercise the legacy native-filter path (the policiesInAcceptedState flag defaults to false in the mock), so this interaction isn't covered.
Is the intent that the banner only ships after PM-39144 adds the SDK mapping, or should this be handled before enabling the banner flag? A note tying PM-39144 to the banner rollout (or test coverage for the SDK path) would make the dependency explicit.
2 participants
🎟️ Tracking
PM-33861
📔 Objective
Add support for the
organizationUserNotificationorganization policy and surface it as a dismissible banner (action card) on the vault list.Changes:
pm-31948-org-user-notification-bannerfeature flag.organizationUserNotificationPolicyTypeand its associatedPolicyOptionTypekeys (header,description,buttonText,showAfterEveryLogin).OrganizationUserNotificationBannerDataandPolicyService.getOrganizationUserNotificationBannerData(), which (behind the feature flag) returns the banner data for the earliest-revision policy applying to the user, ornilwhen no policy applies or the policy has nodescription.ActionCardon the vault list (empty and populated states). When the policy supplies both a header and button text, an acknowledgement button is shown; otherwise a standard X dismiss button is shown. Dismissing clears the banner.extensionand share apolicyWithEarliestRevisionDatehelper inPolicyService.Currently this just dismisses the banner in memory. There's additional logic for persisting the dismissal of the banner coming in a future PR.
📸 Screenshots