Address review feedback on UnassignedBannerService (#8680)

* Introduce `UnassignedItemsBannerApiService`

* Delete `WebUnassignedItemsBannerService`

apps/web/src/app/layouts/header/web-header.component.html

@@ -1,8 +1,8 @@
 <bit-banner
   class="-tw-m-6 tw-flex tw-flex-col tw-pb-6"
-  (onClose)="webUnassignedItemsBannerService.hideBanner()"
+  (onClose)="unassignedItemsBannerService.hideBanner()"
   *ngIf="
-    (unassignedItemsBannerEnabled$ | async) && (webUnassignedItemsBannerService.showBanner$ | async)
+    (unassignedItemsBannerEnabled$ | async) && (unassignedItemsBannerService.showBanner$ | async)
   "
 >
   {{ "unassignedItemsBanner" | i18n }}

apps/web/src/app/layouts/header/web-header.component.ts

@@ -2,6 +2,7 @@ import { Component, Input } from "@angular/core";
 import { ActivatedRoute } from "@angular/router";
 import { combineLatest, map, Observable } from "rxjs";
 
+import { UnassignedItemsBannerService } from "@bitwarden/angular/services/unassigned-items-banner.service";
 import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout-settings.service";
 import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
 import { VaultTimeoutAction } from "@bitwarden/common/enums/vault-timeout-action.enum";
@@ -11,8 +12,6 @@ import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/pl
 import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
 import { AccountProfile } from "@bitwarden/common/platform/models/domain/account";
 
-import { WebUnassignedItemsBannerService } from "./web-unassigned-items-banner.service";
-
 @Component({
   selector: "app-header",
   templateUrl: "./web-header.component.html",
@@ -43,7 +42,7 @@ export class WebHeaderComponent {
     private platformUtilsService: PlatformUtilsService,
     private vaultTimeoutSettingsService: VaultTimeoutSettingsService,
     private messagingService: MessagingService,
-    protected webUnassignedItemsBannerService: WebUnassignedItemsBannerService,
+    protected unassignedItemsBannerService: UnassignedItemsBannerService,
     private configService: ConfigService,
   ) {
     this.routeData$ = this.route.data.pipe(

apps/web/src/app/layouts/header/web-unassigned-items-banner.service.spec.ts

@@ -1,56 +0,0 @@
-import { MockProxy, mock } from "jest-mock-extended";
-import { firstValueFrom, skip } from "rxjs";
-
-import { ApiService } from "@bitwarden/common/abstractions/api.service";
-import { FakeStateProvider, mockAccountServiceWith } from "@bitwarden/common/spec";
-import { UserId } from "@bitwarden/common/types/guid";
-
-import {
-  SHOW_BANNER_KEY,
-  WebUnassignedItemsBannerService,
-} from "./web-unassigned-items-banner.service";
-
-describe("WebUnassignedItemsBanner", () => {
-  let stateProvider: FakeStateProvider;
-  let apiService: MockProxy<ApiService>;
-
-  const sutFactory = () => new WebUnassignedItemsBannerService(stateProvider, apiService);
-
-  beforeEach(() => {
-    const fakeAccountService = mockAccountServiceWith("userId" as UserId);
-    stateProvider = new FakeStateProvider(fakeAccountService);
-    apiService = mock();
-  });
-
-  it("shows the banner if showBanner local state is true", async () => {
-    const showBanner = stateProvider.activeUser.getFake(SHOW_BANNER_KEY);
-    showBanner.nextState(true);
-
-    const sut = sutFactory();
-    expect(await firstValueFrom(sut.showBanner$)).toBe(true);
-    expect(apiService.getShowUnassignedCiphersBanner).not.toHaveBeenCalled();
-  });
-
-  it("does not show the banner if showBanner local state is false", async () => {
-    const showBanner = stateProvider.activeUser.getFake(SHOW_BANNER_KEY);
-    showBanner.nextState(false);
-
-    const sut = sutFactory();
-    expect(await firstValueFrom(sut.showBanner$)).toBe(false);
-    expect(apiService.getShowUnassignedCiphersBanner).not.toHaveBeenCalled();
-  });
-
-  it("fetches from server if local state has not been set yet", async () => {
-    apiService.getShowUnassignedCiphersBanner.mockResolvedValue(true);
-
-    const showBanner = stateProvider.activeUser.getFake(SHOW_BANNER_KEY);
-    showBanner.nextState(undefined);
-
-    const sut = sutFactory();
-    // skip first value so we get the recomputed value after the server call
-    expect(await firstValueFrom(sut.showBanner$.pipe(skip(1)))).toBe(true);
-    // Expect to have updated local state
-    expect(await firstValueFrom(showBanner.state$)).toBe(true);
-    expect(apiService.getShowUnassignedCiphersBanner).toHaveBeenCalledTimes(1);
-  });
-});

apps/web/src/app/layouts/header/web-unassigned-items-banner.service.ts

@@ -1,46 +0,0 @@
-import { Injectable } from "@angular/core";
-import { EMPTY, concatMap } from "rxjs";
-
-import { ApiService } from "@bitwarden/common/abstractions/api.service";
-import {
-  StateProvider,
-  UNASSIGNED_ITEMS_BANNER_DISK,
-  UserKeyDefinition,
-} from "@bitwarden/common/platform/state";
-
-export const SHOW_BANNER_KEY = new UserKeyDefinition<boolean>(
-  UNASSIGNED_ITEMS_BANNER_DISK,
-  "showBanner",
-  {
-    deserializer: (b) => b,
-    clearOn: [],
-  },
-);
-
-/** Displays a banner that tells users how to move their unassigned items into a collection. */
-@Injectable({ providedIn: "root" })
-export class WebUnassignedItemsBannerService {
-  private _showBanner = this.stateProvider.getActive(SHOW_BANNER_KEY);
-
-  showBanner$ = this._showBanner.state$.pipe(
-    concatMap(async (showBanner) => {
-      // null indicates that the user has not seen or dismissed the banner yet - get the flag from server
-      if (showBanner == null) {
-        const showBannerResponse = await this.apiService.getShowUnassignedCiphersBanner();
-        await this._showBanner.update(() => showBannerResponse);
-        return EMPTY; // complete the inner observable without emitting any value; the update on the previous line will trigger another run
-      }
-
-      return showBanner;
-    }),
-  );
-
-  constructor(
-    private stateProvider: StateProvider,
-    private apiService: ApiService,
-  ) {}
-
-  async hideBanner() {
-    await this._showBanner.update(() => false);
-  }
-}

libs/angular/src/services/unassigned-items-banner.api.service.ts

@@ -0,0 +1,19 @@
+import { Injectable } from "@angular/core";
+
+import { ApiService } from "@bitwarden/common/abstractions/api.service";
+
+@Injectable({ providedIn: "root" })
+export class UnassignedItemsBannerApiService {
+  constructor(private apiService: ApiService) {}
+
+  async getShowUnassignedCiphersBanner(): Promise<boolean> {
+    const r = await this.apiService.send(
+      "GET",
+      "/ciphers/has-unassigned-ciphers",
+      null,
+      true,
+      true,
+    );
+    return r;
+  }
+}

libs/angular/src/services/unassigned-items-banner.service.spec.ts

@@ -1,15 +1,15 @@
 import { MockProxy, mock } from "jest-mock-extended";
 import { firstValueFrom, skip } from "rxjs";
 
-import { ApiService } from "@bitwarden/common/abstractions/api.service";
 import { FakeStateProvider, mockAccountServiceWith } from "@bitwarden/common/spec";
 import { UserId } from "@bitwarden/common/types/guid";
 
+import { UnassignedItemsBannerApiService } from "./unassigned-items-banner.api.service";
 import { SHOW_BANNER_KEY, UnassignedItemsBannerService } from "./unassigned-items-banner.service";
 
 describe("UnassignedItemsBanner", () => {
   let stateProvider: FakeStateProvider;
-  let apiService: MockProxy<ApiService>;
+  let apiService: MockProxy<UnassignedItemsBannerApiService>;
 
   const sutFactory = () => new UnassignedItemsBannerService(stateProvider, apiService);
 

libs/angular/src/services/unassigned-items-banner.service.ts

@@ -1,13 +1,14 @@
 import { Injectable } from "@angular/core";
 import { EMPTY, concatMap } from "rxjs";
 
-import { ApiService } from "@bitwarden/common/abstractions/api.service";
 import {
   StateProvider,
   UNASSIGNED_ITEMS_BANNER_DISK,
   UserKeyDefinition,
 } from "@bitwarden/common/platform/state";
 
+import { UnassignedItemsBannerApiService } from "./unassigned-items-banner.api.service";
+
 export const SHOW_BANNER_KEY = new UserKeyDefinition<boolean>(
   UNASSIGNED_ITEMS_BANNER_DISK,
   "showBanner",
@@ -37,7 +38,7 @@ export class UnassignedItemsBannerService {
 
   constructor(
     private stateProvider: StateProvider,
-    private apiService: ApiService,
+    private apiService: UnassignedItemsBannerApiService,
   ) {}
 
   async hideBanner() {

libs/common/src/abstractions/api.service.ts

@@ -207,7 +207,6 @@ export abstract class ApiService {
     emergencyAccessId?: string,
   ) => Promise<AttachmentResponse>;
   getCiphersOrganization: (organizationId: string) => Promise<ListResponse<CipherResponse>>;
-  getShowUnassignedCiphersBanner: () => Promise<boolean>;
   postCipher: (request: CipherRequest) => Promise<CipherResponse>;
   postCipherCreate: (request: CipherCreateRequest) => Promise<CipherResponse>;
   postCipherAdmin: (request: CipherCreateRequest) => Promise<CipherResponse>;

libs/common/src/services/api.service.ts

@@ -506,11 +506,6 @@ export class ApiService implements ApiServiceAbstraction {
     return new ListResponse(r, CipherResponse);
   }
 
-  async getShowUnassignedCiphersBanner(): Promise<boolean> {
-    const r = await this.send("GET", "/ciphers/has-unassigned-ciphers", null, true, true);
-    return r;
-  }
-
   async postCipher(request: CipherRequest): Promise<CipherResponse> {
     const r = await this.send("POST", "/ciphers", request, true, true);
     return new CipherResponse(r);