[PM-24146] Remove stateProvider.activeUserId from ProviderService (#16258)

* Refactor provider service calls to include userId parameter

- Updated multiple components and services to pass userId when fetching provider data.
- Adjusted the ProviderService interface to require userId for get, get$, and getAll methods.
- Ensured consistent handling of userId across various components, enhancing data retrieval based on active user context.

* Remove deprecated type safety comments and use the getById utility for fetching providers.

* Update ProviderService methods to return undefined for non-existent providers

- Modified the return types of get$ and get methods in ProviderService to allow for undefined values, enhancing type safety.
- Adjusted the providers$ method to return only defined Provider arrays, ensuring consistent handling of provider data.

* Enhance provider permissions guard tests to include userId parameter

- Updated test cases in provider-permissions.guard.spec.ts to pass userId when calling ProviderService methods.
- Mocked AccountService to provide active account details for improved test coverage.
- Ensured consistent handling of userId across all relevant test scenarios.

* remove promise based api's from provider service, continue refactor

* cleanup observable logic

* cleanup

---------

Co-authored-by: Brandon <btreston@bitwarden.com>

libs/angular/src/billing/components/add-account-credit-dialog/add-account-credit-dialog.component.ts

@@ -123,7 +123,9 @@ export class AddAccountCreditDialogComponent implements OnInit {
       this.formGroup.patchValue({
         creditAmount: 20.0,
       });
-      this.provider = await this.providerService.get(this.dialogParams.providerId);
+      this.provider = await firstValueFrom(
+        this.providerService.get$(this.dialogParams.providerId, this.user.id),
+      );
       payPalCustomField = "provider_id:" + this.provider.id;
       this.payPalConfig.subject = this.provider.name;
     } else {

libs/common/src/admin-console/abstractions/provider.service.ts

@@ -5,8 +5,7 @@ import { ProviderData } from "../models/data/provider.data";
 import { Provider } from "../models/domain/provider";
 
 export abstract class ProviderService {
-  abstract get$(id: string): Observable<Provider>;
-  abstract get(id: string): Promise<Provider>;
-  abstract getAll(): Promise<Provider[]>;
-  abstract save(providers: { [id: string]: ProviderData }, userId?: UserId): Promise<any>;
+  abstract providers$(userId: UserId): Observable<Provider[]>;
+  abstract get$(id: string, userId: UserId): Observable<Provider | undefined>;
+  abstract save(providers: { [id: string]: ProviderData }, userId: UserId): Promise<any>;
 }

libs/common/src/admin-console/services/provider.service.spec.ts

@@ -1,7 +1,7 @@
 import { firstValueFrom } from "rxjs";
 
 import { FakeAccountService, FakeStateProvider, mockAccountServiceWith } from "../../../spec";
-import { FakeActiveUserState, FakeSingleUserState } from "../../../spec/fake-state";
+import { FakeSingleUserState } from "../../../spec/fake-state";
 import { Utils } from "../../platform/misc/utils";
 import { UserId } from "../../types/guid";
 import {
@@ -20,11 +20,11 @@ import { PROVIDERS, ProviderService } from "./provider.service";
  * in state. This helper methods lets us build provider arrays in tests
  * and easily map them to records before storing them in state.
  */
-function arrayToRecord(input: ProviderData[]): Record<string, ProviderData> {
-  if (input == null) {
-    return undefined;
+function arrayToRecord(input: ProviderData[] | undefined): Record<string, ProviderData> | null {
+  if (input == null || input.length < 1) {
+    return null;
   }
-  return Object.fromEntries(input?.map((i) => [i.id, i]));
+  return Object.fromEntries(input.map((i) => [i.id, i]));
 }
 
 /**
@@ -39,7 +39,7 @@ function arrayToRecord(input: ProviderData[]): Record<string, ProviderData> {
  */
 function buildMockProviders(count = 1, suffix?: string): ProviderData[] {
   if (count < 1) {
-    return undefined;
+    return [];
   }
 
   function buildMockProvider(id: string, name: string): ProviderData {
@@ -87,30 +87,28 @@ describe("ProviderService", () => {
   let fakeAccountService: FakeAccountService;
   let fakeStateProvider: FakeStateProvider;
   let fakeUserState: FakeSingleUserState<Record<string, ProviderData>>;
-  let fakeActiveUserState: FakeActiveUserState<Record<string, ProviderData>>;
 
   beforeEach(async () => {
     fakeAccountService = mockAccountServiceWith(fakeUserId);
     fakeStateProvider = new FakeStateProvider(fakeAccountService);
     fakeUserState = fakeStateProvider.singleUser.getFake(fakeUserId, PROVIDERS);
-    fakeActiveUserState = fakeStateProvider.activeUser.getFake(PROVIDERS);
 
     providerService = new ProviderService(fakeStateProvider);
   });
 
-  describe("getAll()", () => {
+  describe("providers$()", () => {
     it("Returns an array of all providers stored in state", async () => {
-      const mockData: ProviderData[] = buildMockProviders(5);
+      const mockData = buildMockProviders(5);
       fakeUserState.nextState(arrayToRecord(mockData));
-      const providers = await providerService.getAll();
+      const providers = await firstValueFrom(providerService.providers$(fakeUserId));
       expect(providers).toHaveLength(5);
       expect(providers).toEqual(mockData.map((x) => new Provider(x)));
     });
 
     it("Returns an empty array if no providers are found in state", async () => {
-      const mockData: ProviderData[] = undefined;
+      let mockData;
       fakeUserState.nextState(arrayToRecord(mockData));
-      const result = await providerService.getAll();
+      const result = await firstValueFrom(providerService.providers$(fakeUserId));
       expect(result).toEqual([]);
     });
   });
@@ -119,50 +117,38 @@ describe("ProviderService", () => {
     it("Returns an observable of a single provider from state that matches the specified id", async () => {
       const mockData = buildMockProviders(5);
       fakeUserState.nextState(arrayToRecord(mockData));
-      const result = providerService.get$(mockData[3].id);
+      const result = providerService.get$(mockData[3].id, fakeUserId);
       const provider = await firstValueFrom(result);
       expect(provider).toEqual(new Provider(mockData[3]));
     });
 
     it("Returns an observable of undefined if the specified provider is not found", async () => {
-      const result = providerService.get$("this-provider-does-not-exist");
+      const result = providerService.get$("this-provider-does-not-exist", fakeUserId);
       const provider = await firstValueFrom(result);
       expect(provider).toBe(undefined);
     });
   });
 
-  describe("get()", () => {
-    it("Returns a single provider from state that matches the specified id", async () => {
-      const mockData = buildMockProviders(5);
-      fakeUserState.nextState(arrayToRecord(mockData));
-      const result = await providerService.get(mockData[3].id);
-      expect(result).toEqual(new Provider(mockData[3]));
-    });
-
-    it("Returns undefined if the specified provider id is not found", async () => {
-      const result = await providerService.get("this-provider-does-not-exist");
-      expect(result).toBe(undefined);
-    });
-  });
-
   describe("save()", () => {
-    it("replaces the entire provider list in state for the active user", async () => {
+    it("replaces the entire provider list in state for the specified user", async () => {
       const originalData = buildMockProviders(10);
       fakeUserState.nextState(arrayToRecord(originalData));
 
       const newData = arrayToRecord(buildMockProviders(10, "newData"));
-      await providerService.save(newData);
+      if (newData) {
+        await providerService.save(newData, fakeUserId);
+      }
 
-      expect(fakeActiveUserState.nextMock).toHaveBeenCalledWith([fakeUserId, newData]);
+      expect(fakeUserState.nextMock).toHaveBeenCalledWith(newData);
     });
 
     // This is more or less a test for logouts
     it("can replace state with null", async () => {
       const originalData = buildMockProviders(2);
-      fakeActiveUserState.nextState(arrayToRecord(originalData));
-      await providerService.save(null);
+      fakeUserState.nextState(arrayToRecord(originalData));
+      await providerService.save(null, fakeUserId);
 
-      expect(fakeActiveUserState.nextMock).toHaveBeenCalledWith([fakeUserId, null]);
+      expect(fakeUserState.nextMock).toHaveBeenCalledWith(null);
     });
   });
 });

libs/common/src/admin-console/services/provider.service.ts

@@ -1,7 +1,6 @@
-// FIXME: Update this file to be type safe and remove this and next line
-// @ts-strict-ignore
-import { firstValueFrom, map, Observable, of, switchMap, take } from "rxjs";
+import { map, Observable } from "rxjs";
 
+import { getById } from "../../platform/misc";
 import { PROVIDERS_DISK, StateProvider, UserKeyDefinition } from "../../platform/state";
 import { UserId } from "../../types/guid";
 import { ProviderService as ProviderServiceAbstraction } from "../abstractions/provider.service";
@@ -13,46 +12,26 @@ export const PROVIDERS = UserKeyDefinition.record<ProviderData>(PROVIDERS_DISK,
   clearOn: ["logout"],
 });
 
-function mapToSingleProvider(providerId: string) {
-  return map<Provider[], Provider>((providers) => providers?.find((p) => p.id === providerId));
-}
-
 export class ProviderService implements ProviderServiceAbstraction {
   constructor(private stateProvider: StateProvider) {}
 
-  private providers$(userId?: UserId): Observable<Provider[] | undefined> {
-    // FIXME: Can be replaced with `getUserStateOrDefault$` if we weren't trying to pick this.
-    return (
-      userId != null
-        ? this.stateProvider.getUser(userId, PROVIDERS).state$
-        : this.stateProvider.activeUserId$.pipe(
-            take(1),
-            switchMap((userId) =>
-              userId != null ? this.stateProvider.getUser(userId, PROVIDERS).state$ : of(null),
-            ),
-          )
-    ).pipe(this.mapProviderRecordToArray());
+  providers$(userId: UserId): Observable<Provider[]> {
+    return this.stateProvider
+      .getUser(userId, PROVIDERS)
+      .state$.pipe(this.mapProviderRecordToArray());
   }
 
   private mapProviderRecordToArray() {
-    return map<Record<string, ProviderData>, Provider[]>((providers) =>
-      Object.values(providers ?? {})?.map((o) => new Provider(o)),
+    return map<Record<string, ProviderData> | null, Provider[]>((providers) =>
+      Object.values(providers ?? {}).map((o) => new Provider(o)),
     );
   }
 
-  get$(id: string): Observable<Provider> {
-    return this.providers$().pipe(mapToSingleProvider(id));
+  get$(id: string, userId: UserId): Observable<Provider | undefined> {
+    return this.providers$(userId).pipe(getById(id));
   }
 
-  async get(id: string): Promise<Provider> {
-    return await firstValueFrom(this.providers$().pipe(mapToSingleProvider(id)));
-  }
-
-  async getAll(): Promise<Provider[]> {
-    return await firstValueFrom(this.providers$());
-  }
-
-  async save(providers: { [id: string]: ProviderData }, userId?: UserId) {
+  async save(providers: { [id: string]: ProviderData }, userId: UserId) {
     await this.stateProvider.setUserState(PROVIDERS, providers, userId);
   }
 }