[PM-14219] Add service for new device verification notice #11988
Conversation
Jingo88
requested review from
coroiu,
cd-bitwarden,
shane-melton and
gbubemismith
| noticeState$: Observable<NewDeviceVerificationNotice>; | ||
|
|
||
| constructor(private stateProvider: StateProvider) { | ||
| this.noticeState = this.stateProvider.getActive(NEW_DEVICE_VERIFICATION_NOTICE_KEY); |
There was a problem hiding this comment.
ActiveUserState and should require a userId for service methods/state. Something like
noticeState$(userId: userId) {
return this.stateProvider.getUser(userId, NEW_DEVICE_VERIFICATION_NOTICE_KEY).state$;
}
| } from "@bitwarden/common/platform/state"; | ||
|
|
||
| export type NewDeviceVerificationNotice = { | ||
| last_dismissal: string; |
There was a problem hiding this comment.
❓ What type of values will last_dismissal hold? Is it a date?
| NEW_DEVICE_VERIFICATION_NOTICE, | ||
| } from "@bitwarden/common/platform/state"; | ||
|
|
||
| export type NewDeviceVerificationNotice = { |
There was a problem hiding this comment.
🎨 We should add some documentation on what these values represent / how they're expected to be used.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #11988 +/- ##
=======================================
Coverage 33.48% 33.49%
=======================================
Files 2846 2847 +1
Lines 89214 89231 +17
Branches 17006 17009 +3
=======================================
+ Hits 29872 29886 +14
- Misses 56989 56991 +2
- Partials 2353 2354 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
Fixed Issues
|
| return this.stateProvider.getUser(userId, NEW_DEVICE_VERIFICATION_NOTICE_KEY); | ||
| } | ||
|
|
||
| noticeState$(userId$: Observable<UserId>): Observable<NewDeviceVerificationNotice> { |
There was a problem hiding this comment.
💭 I think we may want to just accept a single UserId instead of Observable<UserId>. For reference there is some recent discussion on this here.
But, from my understanding, by accepting an observable, there is a greater chance we run into the same issue we're trying to avoid by moving away from ActiveUserState (race conditions with different userIds during account switching or lock/logout).
cc: @justindbaur In case you have any more insight here.
🎟️ Tracking
PM-14219
📔 Objective
Added service for New Device Verification Notice. Components and Routeguards that will use this are being implement next.
📸 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