[PM-14366] Deprecated active user state from billing state service #12273

cturnbull-bitwarden · 2024-12-06T16:20:12Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-14366

📔 Objective

This PR updates the BillingAccountProfileStateService to remove its dependency on ActiveUserStateProvider by requiring explicit user IDs when checking premium status. This change:

Modifies the service's public observables to accept a userId parameter instead of relying on the active user state
Removes the internal state tracking of the active user
Updates all consumers of the service to pass the active user's ID by combining with AccountService.activeAccount$, often via a switchMap, or otherwise, firstValueFrom

📸 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

github-actions · 2024-12-06T16:28:42Z

Checkmarx One – Scan Summary & Details – c114f21d-eab8-4f96-805b-f5bfd158ae45

No New Or Fixed Issues Found

…e-provider

apps/web/src/app/layouts/user-layout.component.ts

Co-authored-by: ✨ Audrey ✨ <[email protected]>

gbubemismith

Just added a few suggestions and pointed out some failing tests. Thanks for working on this

libs/angular/src/vault/components/view.component.ts

apps/web/src/app/vault/individual-vault/vault.component.ts

apps/web/src/app/vault/individual-vault/vault-banners/services/vault-banners.service.ts

apps/web/src/app/vault/individual-vault/add-edit.component.ts

gbubemismith · 2024-12-11T16:49:55Z

apps/cli/src/vault/create.command.ts

-      cipher.organizationId == null &&
-      !(await firstValueFrom(this.accountProfileService.hasPremiumFromAnySource$))
-    ) {
+    const account = await firstValueFrom(this.accountService.activeAccount$);


I think we can delete this and move the activeUserId declared on L155 out of the try scope

...c/vault/popup/components/vault-v2/attachments/open-attachments/open-attachments.component.ts

…e-provider

…e-provider # Conflicts: # apps/browser/src/tools/popup/settings/about-page/more-from-bitwarden-page.component.ts

codecov · 2024-12-17T18:30:57Z

Codecov Report

Attention: Patch coverage is 31.17647% with 117 lines in your changes missing coverage. Please review.

Project coverage is 33.61%. Comparing base (8caadac) to head (d6c1494).

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...pp/billing/individual/premium/premium.component.ts	0.00%	7 Missing ⚠️
...bout-page/more-from-bitwarden-page-v2.component.ts	0.00%	6 Missing ⚠️
...billing/individual/premium/premium-v2.component.ts	0.00%	6 Missing ⚠️
apps/web/src/app/core/guards/has-premium.guard.ts	0.00%	6 Missing ⚠️
libs/angular/src/directives/premium.directive.ts	0.00%	6 Missing ⚠️
...c/new-send-dropdown/new-send-dropdown.component.ts	0.00%	6 Missing ⚠️
...vault/popup/components/action-buttons.component.ts	0.00%	5 Missing ⚠️
apps/cli/src/tools/send/commands/create.command.ts	0.00%	5 Missing ⚠️
...pps/desktop/src/vault/app/vault/vault.component.ts	0.00%	5 Missing ⚠️
...bs/angular/src/directives/not-premium.directive.ts	16.66%	5 Missing ⚠️
... and 23 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12273      +/-   ##
==========================================
+ Coverage   33.59%   33.61%   +0.02%     
==========================================
  Files        2926     2926              
  Lines       91441    91539      +98     
  Branches    17375    17399      +24     
==========================================
+ Hits        30717    30770      +53     
- Misses      58310    58341      +31     
- Partials     2414     2428      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

audreyality

👍🏻 Tools code LGTM!

🤔 I wonder whether there's places that of will fail because it completes. Where firstValueFrom or combineLatest is used it shouldn't cause any issues since those keep the "singularity" of the last (only) emitted value. If there are places that expect the observable to change value instead of completing, then there could be a lot of non-responsive UI.

I guess what I'm saying is, please run the regression suite on this before merge, if we can, since this PR affects so much.

willmartian · 2024-12-17T20:04:05Z

apps/web/src/app/layouts/user-layout.component.ts

+    private accountService: AccountService,
+  ) {
+    this.showSubscription$ = this.accountService.activeAccount$.pipe(
+      switchMap((account) =>
+        this.billingAccountProfileStateService.canViewSubscription$(account.id),
+      ),
+    );
+  }


Much better, thank you! Any reason to not move the AccountService dependency inside the service as well? Such that we could do:

this.showSubscription$ = this.billingAccountProfileStateService.activeAccountCanViewSubscription$;

or, the active account could be the default if no account ID is provided:

this.showSubscription$ = this.billingAccountProfileStateService.canViewSubscription()$;

There might be reasons to not do this! Curious what you think @cturnbull-bitwarden

It's a good question! I decided to keep BillingAccountProfileStateService independent of AccountService for a couple of reasons:

Simply speaking, I wanted to maintain a clear dependency direction and avoid introducing AccountService as a hard dependency

My thinking for BillingAccountProfileStateService was that it should have a single responsibility - managing the state of billing's premium flags using the StateProvider pattern

That said, after implementing this, I've still had to map AccountService.activeAccount$ in a bunch places to avoid using ActiveUserStateProvider. I'm open to revisiting this design in a follow-up PR.

avoid introducing AccountService as a hard dependency

🎨

IMO, AccountService is already a hard dependency of the service as written. Consumers can't use the methods unless they have an account ID (often, the active account ID). This is evident by the number of times we needed to import AccountService in the PR. My gut says: if we always need to import a peer dep, maybe that dep should be brought into the service.

Larger questions at play here: when is it appropriate to push dependencies onto the consumer of a service? Should services that act on accounts provide convenience methods that target the active account, since it is most often interacted with? Gonna share in Slack to see if others have strong opinions.

JaredSnider-Bitwarden

Auth changes look good.

BTreston

Just some non-blocking ⛏️'s. Otherwise looks good!

BTreston · 2024-12-17T20:23:18Z

apps/browser/src/vault/popup/components/action-buttons.component.ts

+        switchMap((account) =>
+          this.billingAccountProfileStateService.hasPremiumFromAnySource$(account.id),
+        ),
+        takeUntil(this.componentIsDestroyed$),


non blocking ⛏️ this pattern for unsubscribing is outdated and we should favor using takeUntilDestroyed()

Thanks for the suggestion! I created a tech debt task here for us to handle this in a follow-up PR

BTreston · 2024-12-17T20:39:20Z

apps/desktop/src/vault/app/vault/vault.component.ts

+        switchMap((account) =>
+          this.billingAccountProfileStateService.hasPremiumFromAnySource$(account.id),
+        ),
+        takeUntil(this.componentIsDestroyed$),


same ⛏️ here

BTreston · 2024-12-17T20:43:16Z

libs/angular/src/directives/premium.directive.ts

+            ? this.billingAccountProfileStateService.hasPremiumFromAnySource$(account.id)
+            : of(false),
+        ),
+        takeUntil(this.directiveIsDestroyed$),


same ⛏️ here too.

BTreston · 2024-12-17T20:43:43Z

libs/angular/src/tools/send/add-edit.component.ts

+        switchMap((account) =>
+          this.billingAccountProfileStateService.hasPremiumFromAnySource$(account.id),
+        ),
+        takeUntil(this.destroy$),


same ⛏️ here

gbubemismith

Changes look good. Thanks.

justindbaur · 2024-12-18T13:24:32Z

apps/web/src/app/core/guards/has-premium.guard.ts

+      switchMap((account) =>
+        account
+          ? billingAccountProfileStateService.hasPremiumFromAnySource$(account.id)
+          : of(false),
+      ),


No need to do this on this PR but I worry that some of the routes might navigate to a route protected with this right after logging in. Since active user state wouldn't emit until an account is set as active it would have delayed until that happened. I think we should give it a grace period to allow the account to come in and if it doesn't happen in a reasonable time throw an error because I think it would likely be a developer error.

Suggested change

switchMap((account) =>

account

? billingAccountProfileStateService.hasPremiumFromAnySource$(account.id)

: of(false),

),

filter((account) => account != null),

timeout({

first: 2000,

with: () => {

throw new Error("An active user is expected when checking if a user has premium.");

},

}),

switchMap((account) =>

billingAccountProfileStateService.hasPremiumFromAnySource$(account.id),

),

Please don't worry about doing this here though, you've got a lot of approvals and I can throw up a small PR doing it after you merge.

jprusik · 2024-12-18T14:34:17Z

apps/browser/src/autofill/browser/main-context-menu-handler.ts

+      const hasPremium = await firstValueFrom(
+        this.billingAccountProfileStateService.hasPremiumFromAnySource$(account.id),
+      );


oo, moving this out of the loop was a good idea; thanks 👍

jprusik

Approved for Autofill concerns

…e-provider

cturnbull-bitwarden added 2 commits December 6, 2024 11:08

Updated billing state provider to not rely on ActiveUserStateProvider

79f0f64

Updated usages

056fbfe

cturnbull-bitwarden requested review from a team as code owners December 6, 2024 16:20

cturnbull-bitwarden requested review from vleague2, justindbaur, shane-melton and ike-kottlowski December 6, 2024 16:20

cturnbull-bitwarden marked this pull request as draft December 6, 2024 16:21

cturnbull-bitwarden added 6 commits December 6, 2024 12:39

Merge branch 'main' into billing/PM-14366/deprectete-active-user-stat…

e943dc9

…e-provider

Resolved browser build

79e02e1

Resolved web build

5f5d38e

Resolved CLI build

4643a64

resolved desktop build

e9d9ec9

Merge branch 'main' into billing/PM-14366/deprectete-active-user-stat…

8232a72

…e-provider

cturnbull-bitwarden marked this pull request as ready for review December 6, 2024 19:38

cturnbull-bitwarden requested review from a team as code owners December 6, 2024 19:38

cturnbull-bitwarden requested a review from BTreston December 6, 2024 19:38

This comment was marked as resolved.

Sign in to view

willmartian requested changes Dec 9, 2024

View reviewed changes

apps/web/src/app/layouts/user-layout.component.ts Outdated Show resolved Hide resolved

shane-melton requested a review from gbubemismith December 9, 2024 16:59

Update apps/cli/src/tools/send/commands/create.command.ts

55e6f24

Co-authored-by: ✨ Audrey ✨ <[email protected]>

gbubemismith requested changes Dec 11, 2024

View reviewed changes

Merge branch 'main' into billing/PM-14366/deprectete-active-user-stat…

39c8a0d

…e-provider

cturnbull-bitwarden added 6 commits December 16, 2024 15:09

Move subscription visibility logic from component to service

f9f4ceb

Resolved unit test failures. Using existing userIds where present

ad0c18e

Merge branch 'main' into billing/PM-14366/deprectete-active-user-stat…

bc9dc37

…e-provider

Simplified activeUserId access

62662e3

Merge branch 'main' into billing/PM-14366/deprectete-active-user-stat…

85af1e6

…e-provider # Conflicts: # apps/browser/src/tools/popup/settings/about-page/more-from-bitwarden-page.component.ts

Resolved typescript strict errors

a9d072c

jonashendrickx approved these changes Dec 17, 2024

View reviewed changes

cturnbull-bitwarden requested review from audreyality, willmartian and gbubemismith December 17, 2024 19:50

audreyality approved these changes Dec 17, 2024

View reviewed changes

willmartian reviewed Dec 17, 2024

View reviewed changes

JaredSnider-Bitwarden approved these changes Dec 17, 2024

View reviewed changes

willmartian approved these changes Dec 17, 2024

View reviewed changes

BTreston approved these changes Dec 17, 2024

View reviewed changes

gbubemismith approved these changes Dec 17, 2024

View reviewed changes

dani-garcia approved these changes Dec 18, 2024

View reviewed changes

justindbaur reviewed Dec 18, 2024

View reviewed changes

jprusik reviewed Dec 18, 2024

View reviewed changes

jprusik approved these changes Dec 18, 2024

View reviewed changes

cturnbull-bitwarden added 3 commits December 18, 2024 10:45

Merge branch 'main' into billing/PM-14366/deprectete-active-user-stat…

5f03ba7

…e-provider

Merge branch 'main' into billing/PM-14366/deprectete-active-user-stat…

6a69690

…e-provider

Merge branch 'main' into billing/PM-14366/deprectete-active-user-stat…

d6c1494

…e-provider

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-14366] Deprecated active user state from billing state service #12273

[PM-14366] Deprecated active user state from billing state service #12273

cturnbull-bitwarden commented Dec 6, 2024

github-actions bot commented Dec 6, 2024 •

edited

Loading

This comment was marked as resolved.

gbubemismith left a comment

gbubemismith Dec 11, 2024

codecov bot commented Dec 17, 2024 •

edited

Loading

audreyality left a comment •

edited

Loading

willmartian Dec 17, 2024

cturnbull-bitwarden Dec 17, 2024

willmartian Dec 18, 2024

JaredSnider-Bitwarden left a comment

BTreston left a comment

BTreston Dec 17, 2024

cturnbull-bitwarden Dec 18, 2024

BTreston Dec 17, 2024

BTreston Dec 17, 2024

BTreston Dec 17, 2024

gbubemismith left a comment

justindbaur Dec 18, 2024

jprusik Dec 18, 2024

jprusik left a comment

[PM-14366] Deprecated active user state from billing state service #12273

Are you sure you want to change the base?

[PM-14366] Deprecated active user state from billing state service #12273

Conversation

cturnbull-bitwarden commented Dec 6, 2024

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Dec 6, 2024 • edited Loading

No New Or Fixed Issues Found

This comment was marked as resolved.

gbubemismith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

audreyality left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

BTreston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbubemismith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jprusik left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

audreyality left a comment •

edited

Loading