From ab83a367dd5e0cdcca73f8881b305ea4cb484985 Mon Sep 17 00:00:00 2001 From: Addison Beck Date: Wed, 10 Apr 2024 16:13:41 -0500 Subject: [PATCH] Address review feedback on `UnassignedBannerService` (#8680) * Introduce `UnassignedItemsBannerApiService` * Delete `WebUnassignedItemsBannerService` --- .../layouts/header/web-header.component.html | 4 +- .../layouts/header/web-header.component.ts | 5 +- ...eb-unassigned-items-banner.service.spec.ts | 56 ------------------- .../web-unassigned-items-banner.service.ts | 46 --------------- .../unassigned-items-banner.api.service.ts | 19 +++++++ .../unassigned-items-banner.service.spec.ts | 4 +- .../unassigned-items-banner.service.ts | 5 +- libs/common/src/abstractions/api.service.ts | 1 - libs/common/src/services/api.service.ts | 5 -- 9 files changed, 28 insertions(+), 117 deletions(-) delete mode 100644 apps/web/src/app/layouts/header/web-unassigned-items-banner.service.spec.ts delete mode 100644 apps/web/src/app/layouts/header/web-unassigned-items-banner.service.ts create mode 100644 libs/angular/src/services/unassigned-items-banner.api.service.ts diff --git a/apps/web/src/app/layouts/header/web-header.component.html b/apps/web/src/app/layouts/header/web-header.component.html index 1555726e2b..e1cda607c0 100644 --- a/apps/web/src/app/layouts/header/web-header.component.html +++ b/apps/web/src/app/layouts/header/web-header.component.html @@ -1,8 +1,8 @@ {{ "unassignedItemsBanner" | i18n }} diff --git a/apps/web/src/app/layouts/header/web-header.component.ts b/apps/web/src/app/layouts/header/web-header.component.ts index 6016463ebb..1f012e52dd 100644 --- a/apps/web/src/app/layouts/header/web-header.component.ts +++ b/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( diff --git a/apps/web/src/app/layouts/header/web-unassigned-items-banner.service.spec.ts b/apps/web/src/app/layouts/header/web-unassigned-items-banner.service.spec.ts deleted file mode 100644 index a9db11a201..0000000000 --- a/apps/web/src/app/layouts/header/web-unassigned-items-banner.service.spec.ts +++ /dev/null @@ -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; - - 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); - }); -}); diff --git a/apps/web/src/app/layouts/header/web-unassigned-items-banner.service.ts b/apps/web/src/app/layouts/header/web-unassigned-items-banner.service.ts deleted file mode 100644 index 8f09b68547..0000000000 --- a/apps/web/src/app/layouts/header/web-unassigned-items-banner.service.ts +++ /dev/null @@ -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( - 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); - } -} diff --git a/libs/angular/src/services/unassigned-items-banner.api.service.ts b/libs/angular/src/services/unassigned-items-banner.api.service.ts new file mode 100644 index 0000000000..69b74f8c7f --- /dev/null +++ b/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 { + const r = await this.apiService.send( + "GET", + "/ciphers/has-unassigned-ciphers", + null, + true, + true, + ); + return r; + } +} diff --git a/libs/angular/src/services/unassigned-items-banner.service.spec.ts b/libs/angular/src/services/unassigned-items-banner.service.spec.ts index eedfbf3429..ac80f7d651 100644 --- a/libs/angular/src/services/unassigned-items-banner.service.spec.ts +++ b/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; + let apiService: MockProxy; const sutFactory = () => new UnassignedItemsBannerService(stateProvider, apiService); diff --git a/libs/angular/src/services/unassigned-items-banner.service.ts b/libs/angular/src/services/unassigned-items-banner.service.ts index dd374fe5ce..bc567aa44e 100644 --- a/libs/angular/src/services/unassigned-items-banner.service.ts +++ b/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( UNASSIGNED_ITEMS_BANNER_DISK, "showBanner", @@ -37,7 +38,7 @@ export class UnassignedItemsBannerService { constructor( private stateProvider: StateProvider, - private apiService: ApiService, + private apiService: UnassignedItemsBannerApiService, ) {} async hideBanner() { diff --git a/libs/common/src/abstractions/api.service.ts b/libs/common/src/abstractions/api.service.ts index 811cca8638..20ed3216a5 100644 --- a/libs/common/src/abstractions/api.service.ts +++ b/libs/common/src/abstractions/api.service.ts @@ -207,7 +207,6 @@ export abstract class ApiService { emergencyAccessId?: string, ) => Promise; getCiphersOrganization: (organizationId: string) => Promise>; - getShowUnassignedCiphersBanner: () => Promise; postCipher: (request: CipherRequest) => Promise; postCipherCreate: (request: CipherCreateRequest) => Promise; postCipherAdmin: (request: CipherCreateRequest) => Promise; diff --git a/libs/common/src/services/api.service.ts b/libs/common/src/services/api.service.ts index 501b924e5b..6306eb1e28 100644 --- a/libs/common/src/services/api.service.ts +++ b/libs/common/src/services/api.service.ts @@ -506,11 +506,6 @@ export class ApiService implements ApiServiceAbstraction { return new ListResponse(r, CipherResponse); } - async getShowUnassignedCiphersBanner(): Promise { - const r = await this.send("GET", "/ciphers/has-unassigned-ciphers", null, true, true); - return r; - } - async postCipher(request: CipherRequest): Promise { const r = await this.send("POST", "/ciphers", request, true, true); return new CipherResponse(r);