[PM-33861] Display organization notification banner on vault list#2798
[PM-33861] Display organization notification banner on vault list#2798matt-livefront wants to merge 4 commits into
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:
|
|
|
||
| /// Returns the policy with the earliest revision date from the given list, or `nil` if the list is empty. | ||
| /// | ||
| /// - Parameter policies: The list of policies to search. | ||
| /// - Returns: The policy with the earliest revision date. | ||
| /// | ||
| private func policyWithEarliestRevisionDate(from policies: [Policy]) -> Policy? { | ||
| policies.min { ($0.revisionDate ?? .distantFuture) < ($1.revisionDate ?? .distantFuture) } | ||
| } |
There was a problem hiding this comment.
❓ I know this comes from the organization logic, but I'm a bit hesitant as this is getting the policy that has been updated least recently, i.e. the oldest one. Maybe it's correct as is, but I would think the most recently updated policy should prevail instead as it would have the latest changes.
Would this be to display the cards ordered from earliest to latest? Would the shown card policy be removed in a future PR?
There was a problem hiding this comment.
In practice, there can only be a single notification banner. This policy requires the "Single organization" policy, so you can't be a member of multiple organizations and you can only specify a single banner policy. Also, once the dismissal logic is implemented, which is based on on revision date, if there were multiple, dismissing an older one would then cause the newer one to be shown.
| /// 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.
![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