BIT-2284: Display unassigned ciphers banner #638
Conversation
Bitwarden code coverageTotal coverage:
|
| File | Coverage |
|---|---|
| BitwardenShared/Core/Auth/Repositories/AuthRepository.swift | 97.13% |
| BitwardenShared/Core/Platform/Services/ConfigService.swift | 85.45% |
| BitwardenShared/Core/Platform/Services/ServiceContainer.swift | 97.20% |
| BitwardenShared/Core/Platform/Services/StateService.swift | 97.40% |
| BitwardenShared/Core/Platform/Services/Stores/AppSettingsStore.swift | 95.58% |
| BitwardenShared/Core/Vault/Repositories/VaultRepository.swift | 96.31% |
| BitwardenShared/Core/Vault/Services/API/Cipher/CipherAPIService.swift | 91.84% |
| BitwardenShared/Core/Vault/Services/CipherService.swift | 98.29% |
| BitwardenShared/UI/Auth/Extensions/Alert+Auth.swift | 100.00% |
| BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift | 96.39% |
| BitwardenShared/UI/Vault/Vault/VaultList/VaultListState.swift | 100.00% |
Powered by Slather
Generated by 🚫 Danger
|
No New Or Fixed Issues Found |
| func hasUnassignedCiphers() async throws -> Bool { | ||
| try await cipherAPIService.hasUnassignedCiphers() | ||
| } |
There was a problem hiding this comment.
🎨 Just in case on the MAUI app we have an improvement to this that is to actually check if the user has any organizations before checking if it has unassigned ciphers so we can save resources and time making a request that will always be false when the user doesn't have organizations, check here.
There was a problem hiding this comment.
ℹ️ Additionally, you'd need to check when the response returns BadRequest, given that I had to add this so it returns false for that case; although not sure if it's indeed needed on native.
There was a problem hiding this comment.
With moving the logic into VaultListProcessor I might be able to key the check/alert off of state.loadingState, but I'm not sure what else other way we'd have to know whether or not the sync has finished. It's possible I'm missing some property, but I'm also reluctant to build out a whole mechanism for it for a temporary thing. I'll keep looking, though, to see if there's some way to check for that.
|
I've moved things into I do also have it check that the user is in an organization. Because it happens after the With duplicate notifications, I looked a bit into implementing some workaround, and it's possible I missed something, but other solutions started feeling a little obfuscated or overly complicated, especially compared to the likely frequency of it happening. I can still go down one of those paths (or find a new one) if we want to, but I wanted to double-check that before hacking something together for that. |
There was a problem hiding this comment.
Nice work 🎉
Regarding duplicate notifications, I agree that a queue would be awesome but feels a bit out of scope for this.
One workaround I can think of is to have a flag that says if a notification permission request is in progress, e.g. isNotificationPermissionRequestInProgress.
Then, we can do the await checkUnassignedCiphers() in .appeared when such flag is false to avoid alerts overlapping.
For the true case we'd go to the onDismiss callback Matt mentioned of showAlert and call checkUnassignedCiphers() (If this has already run then the guard will catch it and prevent from running again.
What do you think?
|
That's probably the best way of doing it, yeah. I just don't know how much I like adding a flag to the |
| @@ -31,6 +31,8 @@ final class VaultListProcessor: StateProcessor< | |||
| /// The services used by this processor. | |||
| private let services: Services | |||
|
|
|||
| private var isShowingNotificationPermissions = false | |||
There was a problem hiding this comment.
⛏️ Could you add docs for this?
KatherineInCode
deleted the
katherine/BIT-2284-display-unassigned-ciphers-banner
branch
🎟️ Tracking
BIT-2284
📔 Objective
This adds a check when a user logs in or unlocks their vault to see if they're an org owner or admin with unassigned ciphers, and if so, show an alert about it.
📸 Screenshots
⏰ Reminders before review
🦮 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