From 3be393078460d90e19fb8f6ce5c429a68f57dd96 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Wed, 10 Apr 2024 08:42:53 +1000 Subject: [PATCH 01/14] Boostrap basic banner, show for all admins --- .../layouts/header/web-header.component.html | 15 +++++++ .../layouts/header/web-header.component.ts | 2 + .../web-unassigned-items-banner.service.ts | 40 +++++++++++++++++++ apps/web/src/locales/en/messages.json | 3 ++ .../src/platform/state/state-definitions.ts | 4 ++ 5 files changed, 64 insertions(+) create mode 100644 apps/web/src/app/layouts/header/web-unassigned-items-banner.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 514e5deebd9..c50d451baf5 100644 --- a/apps/web/src/app/layouts/header/web-header.component.html +++ b/apps/web/src/app/layouts/header/web-header.component.html @@ -13,6 +13,21 @@ >{{ "releaseBlog" | i18n }} + + {{ "unassignedItemsBanner" | i18n }} + {{ "learnMore" | i18n }} +
{ 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 new file mode 100644 index 00000000000..c8493df34b7 --- /dev/null +++ b/apps/web/src/app/layouts/header/web-unassigned-items-banner.service.ts @@ -0,0 +1,40 @@ +import { Injectable } from "@angular/core"; +import { combineLatest, map } from "rxjs"; + +import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; +import { + GlobalStateProvider, + KeyDefinition, + UNASSIGNED_ITEMS_BANNER_DISK, +} from "@bitwarden/common/platform/state"; + +const SHOW_BANNER_KEY = new KeyDefinition(UNASSIGNED_ITEMS_BANNER_DISK, "showBanner", { + deserializer: (b) => { + if (b === null) { + return true; + } + return b; + }, +}); + +/** Displays a banner that tells users how to move their unassigned items into a collection. */ +@Injectable({ providedIn: "root" }) +export class WebUnassignedItemsBannerService { + private _showBannerState = this.globalStateProvider.get(SHOW_BANNER_KEY); + private _adminOrganizations = this.organizationService.organizations$.pipe( + map((orgs) => orgs.filter((o) => o.isAdmin)), + ); + + showBanner$ = combineLatest([this._showBannerState.state$, this._adminOrganizations]).pipe( + map(([showBanner, adminOrganizations]) => showBanner && adminOrganizations?.length > 1), + ); + + constructor( + private globalStateProvider: GlobalStateProvider, + private organizationService: OrganizationService, + ) {} + + async hideBanner() { + await this._showBannerState.update(() => false); + } +} diff --git a/apps/web/src/locales/en/messages.json b/apps/web/src/locales/en/messages.json index 307c5be70cb..3ce99f68328 100644 --- a/apps/web/src/locales/en/messages.json +++ b/apps/web/src/locales/en/messages.json @@ -7899,5 +7899,8 @@ }, "machineAccountAccessUpdated": { "message": "Machine account access updated" + }, + "unassignedItemsBanner": { + "message": "Unassigned organization items are no longer visible in your All Vaults view across devices and are now only accessible via the Admin Console. Assign these items to a collection from the Admin Console to make them visible." } } diff --git a/libs/common/src/platform/state/state-definitions.ts b/libs/common/src/platform/state/state-definitions.ts index d9265cf10cb..86ac0ff7104 100644 --- a/libs/common/src/platform/state/state-definitions.ts +++ b/libs/common/src/platform/state/state-definitions.ts @@ -74,6 +74,10 @@ export const NEW_WEB_LAYOUT_BANNER_DISK = new StateDefinition("newWebLayoutBanne web: "disk-local", }); +export const UNASSIGNED_ITEMS_BANNER_DISK = new StateDefinition("unassignedItemsBanner", "disk", { + web: "disk-local", +}); + // Platform export const APPLICATION_ID_DISK = new StateDefinition("applicationId", "disk", { From 7b3ab859a76208b9f1f6fe5f2c2e6adc42fc217b Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Wed, 10 Apr 2024 09:29:24 +1000 Subject: [PATCH 02/14] Remove UI banner, fix method calls --- .../layouts/header/web-header.component.html | 19 ++----------------- .../layouts/header/web-header.component.ts | 2 -- 2 files changed, 2 insertions(+), 19 deletions(-) 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 c50d451baf5..f69c60dc786 100644 --- a/apps/web/src/app/layouts/header/web-header.component.html +++ b/apps/web/src/app/layouts/header/web-header.component.html @@ -1,22 +1,7 @@ - {{ "newWebApp" | i18n }} - {{ "releaseBlog" | i18n }} - - {{ "unassignedItemsBanner" | i18n }} Date: Wed, 10 Apr 2024 10:05:09 +1000 Subject: [PATCH 03/14] Invert showBanner -> hideBanner --- .../header/web-unassigned-items-banner.service.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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 index c8493df34b7..8b8bb2c7d70 100644 --- 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 @@ -8,10 +8,10 @@ import { UNASSIGNED_ITEMS_BANNER_DISK, } from "@bitwarden/common/platform/state"; -const SHOW_BANNER_KEY = new KeyDefinition(UNASSIGNED_ITEMS_BANNER_DISK, "showBanner", { +const HIDE_BANNER_KEY = new KeyDefinition(UNASSIGNED_ITEMS_BANNER_DISK, "hideBanner", { deserializer: (b) => { if (b === null) { - return true; + return false; } return b; }, @@ -20,13 +20,13 @@ const SHOW_BANNER_KEY = new KeyDefinition(UNASSIGNED_ITEMS_BANNER_DISK, /** Displays a banner that tells users how to move their unassigned items into a collection. */ @Injectable({ providedIn: "root" }) export class WebUnassignedItemsBannerService { - private _showBannerState = this.globalStateProvider.get(SHOW_BANNER_KEY); + private _hideBannerState = this.globalStateProvider.get(HIDE_BANNER_KEY); private _adminOrganizations = this.organizationService.organizations$.pipe( map((orgs) => orgs.filter((o) => o.isAdmin)), ); - showBanner$ = combineLatest([this._showBannerState.state$, this._adminOrganizations]).pipe( - map(([showBanner, adminOrganizations]) => showBanner && adminOrganizations?.length > 1), + showBanner$ = combineLatest([this._hideBannerState.state$, this._adminOrganizations]).pipe( + map(([hideBanner, adminOrganizations]) => !hideBanner && adminOrganizations?.length > 1), ); constructor( @@ -35,6 +35,6 @@ export class WebUnassignedItemsBannerService { ) {} async hideBanner() { - await this._showBannerState.update(() => false); + await this._hideBannerState.update(() => true); } } From 6dbb6e9c031a7ed9dc44d89b06dfcc85c6326fb7 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Wed, 10 Apr 2024 10:22:04 +1000 Subject: [PATCH 04/14] Add api call --- .../web-unassigned-items-banner.service.ts | 52 +++++++++++++------ libs/common/src/abstractions/api.service.ts | 1 + libs/common/src/services/api.service.ts | 5 ++ 3 files changed, 41 insertions(+), 17 deletions(-) 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 index 8b8bb2c7d70..8b3d2254856 100644 --- 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 @@ -1,40 +1,58 @@ import { Injectable } from "@angular/core"; -import { combineLatest, map } from "rxjs"; +import { EMPTY, combineLatest, concatMap } from "rxjs"; -import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; +import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { GlobalStateProvider, KeyDefinition, UNASSIGNED_ITEMS_BANNER_DISK, } from "@bitwarden/common/platform/state"; -const HIDE_BANNER_KEY = new KeyDefinition(UNASSIGNED_ITEMS_BANNER_DISK, "hideBanner", { - deserializer: (b) => { - if (b === null) { - return false; - } - return b; +const DISMISS_BANNER_KEY = new KeyDefinition( + UNASSIGNED_ITEMS_BANNER_DISK, + "dismissBanner", + { + deserializer: (b) => { + if (b === null) { + return false; + } + return b; + }, + }, +); + +const SHOW_BANNER_KEY = new KeyDefinition( + UNASSIGNED_ITEMS_BANNER_DISK, + "showBanner", + { + deserializer: (b) => b ?? null, }, -}); +); /** Displays a banner that tells users how to move their unassigned items into a collection. */ @Injectable({ providedIn: "root" }) export class WebUnassignedItemsBannerService { - private _hideBannerState = this.globalStateProvider.get(HIDE_BANNER_KEY); - private _adminOrganizations = this.organizationService.organizations$.pipe( - map((orgs) => orgs.filter((o) => o.isAdmin)), - ); + private _dismissBannerState = this.globalStateProvider.get(DISMISS_BANNER_KEY); + private _showBannerState = this.globalStateProvider.get(SHOW_BANNER_KEY); + + showBanner$ = combineLatest([this._dismissBannerState.state$, this._showBannerState.state$]).pipe( + concatMap(async ([dismissBanner, showBanner]) => { + if (!dismissBanner && showBanner == null) { + const showBannerResponse = await this.apiService.getShowUnassignedCiphersBanner(); + await this._showBannerState.update(() => showBannerResponse); + return EMPTY; // to test, we could also just emit false and let the value update the next time around + } - showBanner$ = combineLatest([this._hideBannerState.state$, this._adminOrganizations]).pipe( - map(([hideBanner, adminOrganizations]) => !hideBanner && adminOrganizations?.length > 1), + return !dismissBanner && showBanner; + }), ); constructor( private globalStateProvider: GlobalStateProvider, - private organizationService: OrganizationService, + private apiService: ApiService, ) {} async hideBanner() { - await this._hideBannerState.update(() => true); + await this._dismissBannerState.update(() => true); } } diff --git a/libs/common/src/abstractions/api.service.ts b/libs/common/src/abstractions/api.service.ts index 20ed3216a54..811cca8638f 100644 --- a/libs/common/src/abstractions/api.service.ts +++ b/libs/common/src/abstractions/api.service.ts @@ -207,6 +207,7 @@ 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 6306eb1e288..07cc1132485 100644 --- a/libs/common/src/services/api.service.ts +++ b/libs/common/src/services/api.service.ts @@ -506,6 +506,11 @@ export class ApiService implements ApiServiceAbstraction { return new ListResponse(r, CipherResponse); } + async getShowUnassignedCiphersBanner(): Promise { + const r = await this.send("GET", "/ciphers/unassigned-banner", null, true, true); + return r; + } + async postCipher(request: CipherRequest): Promise { const r = await this.send("POST", "/ciphers", request, true, true); return new CipherResponse(r); From 29dd728664ef3288a3dd9e3a50086966d5bcce64 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Wed, 10 Apr 2024 11:04:49 +1000 Subject: [PATCH 05/14] Minor tweaks and wording --- .../web-unassigned-items-banner.service.ts | 31 ++++++++----------- libs/common/src/services/api.service.ts | 2 +- 2 files changed, 14 insertions(+), 19 deletions(-) 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 index 8b3d2254856..50bba137a64 100644 --- 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 @@ -12,18 +12,13 @@ const DISMISS_BANNER_KEY = new KeyDefinition( UNASSIGNED_ITEMS_BANNER_DISK, "dismissBanner", { - deserializer: (b) => { - if (b === null) { - return false; - } - return b; - }, + deserializer: (b) => b ?? false, }, ); -const SHOW_BANNER_KEY = new KeyDefinition( +const HAS_UNASSIGNED_ITEMS = new KeyDefinition( UNASSIGNED_ITEMS_BANNER_DISK, - "showBanner", + "hasUnassignedItems", { deserializer: (b) => b ?? null, }, @@ -32,18 +27,18 @@ const SHOW_BANNER_KEY = new KeyDefinition( /** Displays a banner that tells users how to move their unassigned items into a collection. */ @Injectable({ providedIn: "root" }) export class WebUnassignedItemsBannerService { - private _dismissBannerState = this.globalStateProvider.get(DISMISS_BANNER_KEY); - private _showBannerState = this.globalStateProvider.get(SHOW_BANNER_KEY); - - showBanner$ = combineLatest([this._dismissBannerState.state$, this._showBannerState.state$]).pipe( - concatMap(async ([dismissBanner, showBanner]) => { - if (!dismissBanner && showBanner == null) { - const showBannerResponse = await this.apiService.getShowUnassignedCiphersBanner(); - await this._showBannerState.update(() => showBannerResponse); + private _hasUnassignedItems = this.globalStateProvider.get(DISMISS_BANNER_KEY); + private _showBanner = this.globalStateProvider.get(HAS_UNASSIGNED_ITEMS); + + showBanner$ = combineLatest([this._hasUnassignedItems.state$, this._showBanner.state$]).pipe( + concatMap(async ([dismissBanner, hasUnassignedItems]) => { + if (!dismissBanner && hasUnassignedItems == null) { + const hasUnassignedItemsResponse = await this.apiService.getShowUnassignedCiphersBanner(); + await this._showBanner.update(() => hasUnassignedItemsResponse); return EMPTY; // to test, we could also just emit false and let the value update the next time around } - return !dismissBanner && showBanner; + return !dismissBanner && hasUnassignedItems; }), ); @@ -53,6 +48,6 @@ export class WebUnassignedItemsBannerService { ) {} async hideBanner() { - await this._dismissBannerState.update(() => true); + await this._hasUnassignedItems.update(() => true); } } diff --git a/libs/common/src/services/api.service.ts b/libs/common/src/services/api.service.ts index 07cc1132485..501b924e5b9 100644 --- a/libs/common/src/services/api.service.ts +++ b/libs/common/src/services/api.service.ts @@ -507,7 +507,7 @@ export class ApiService implements ApiServiceAbstraction { } async getShowUnassignedCiphersBanner(): Promise { - const r = await this.send("GET", "/ciphers/unassigned-banner", null, true, true); + const r = await this.send("GET", "/ciphers/has-unassigned-ciphers", null, true, true); return r; } From 13f54ffc64781a66d9aa56b487e488313408462c Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Wed, 10 Apr 2024 11:10:04 +1000 Subject: [PATCH 06/14] Change to active user state --- .../web-unassigned-items-banner.service.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) 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 index 50bba137a64..db73abc14a6 100644 --- 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 @@ -3,32 +3,34 @@ import { EMPTY, combineLatest, concatMap } from "rxjs"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { - GlobalStateProvider, - KeyDefinition, + StateProvider, UNASSIGNED_ITEMS_BANNER_DISK, + UserKeyDefinition, } from "@bitwarden/common/platform/state"; -const DISMISS_BANNER_KEY = new KeyDefinition( +const DISMISS_BANNER_KEY = new UserKeyDefinition( UNASSIGNED_ITEMS_BANNER_DISK, "dismissBanner", { deserializer: (b) => b ?? false, + clearOn: ["logout"], }, ); -const HAS_UNASSIGNED_ITEMS = new KeyDefinition( +const HAS_UNASSIGNED_ITEMS = new UserKeyDefinition( UNASSIGNED_ITEMS_BANNER_DISK, "hasUnassignedItems", { deserializer: (b) => b ?? null, + clearOn: ["logout"], }, ); /** Displays a banner that tells users how to move their unassigned items into a collection. */ @Injectable({ providedIn: "root" }) export class WebUnassignedItemsBannerService { - private _hasUnassignedItems = this.globalStateProvider.get(DISMISS_BANNER_KEY); - private _showBanner = this.globalStateProvider.get(HAS_UNASSIGNED_ITEMS); + private _hasUnassignedItems = this.stateProvider.getActive(DISMISS_BANNER_KEY); + private _showBanner = this.stateProvider.getActive(HAS_UNASSIGNED_ITEMS); showBanner$ = combineLatest([this._hasUnassignedItems.state$, this._showBanner.state$]).pipe( concatMap(async ([dismissBanner, hasUnassignedItems]) => { @@ -43,7 +45,7 @@ export class WebUnassignedItemsBannerService { ); constructor( - private globalStateProvider: GlobalStateProvider, + private stateProvider: StateProvider, private apiService: ApiService, ) {} From 976c11ac75a802d68d4c235cfd6579e356ae88c9 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Wed, 10 Apr 2024 11:59:29 +1000 Subject: [PATCH 07/14] Add tests --- ...eb-unassigned-items-banner.service.spec.ts | 76 +++++++++++++++++++ .../web-unassigned-items-banner.service.ts | 4 +- 2 files changed, 78 insertions(+), 2 deletions(-) create mode 100644 apps/web/src/app/layouts/header/web-unassigned-items-banner.service.spec.ts 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 new file mode 100644 index 00000000000..b71c476cc29 --- /dev/null +++ b/apps/web/src/app/layouts/header/web-unassigned-items-banner.service.spec.ts @@ -0,0 +1,76 @@ +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 { + DISMISS_BANNER_KEY, + HAS_UNASSIGNED_ITEMS, + 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(); + }); + + describe("given stored values only", () => { + it("shows the banner if has unassigned ciphers and has not been dismissed", async () => { + const dismissBanner = stateProvider.activeUser.getFake(DISMISS_BANNER_KEY); + dismissBanner.nextState(undefined); + const hasUnassignedItems = stateProvider.activeUser.getFake(HAS_UNASSIGNED_ITEMS); + hasUnassignedItems.nextState(true); + + const sut = sutFactory(); + expect(await firstValueFrom(sut.showBanner$)).toBe(true); + expect(apiService.getShowUnassignedCiphersBanner).not.toHaveBeenCalled(); + }); + + it("does not show the banner if does not have unassigned ciphers", async () => { + const dismissBanner = stateProvider.activeUser.getFake(DISMISS_BANNER_KEY); + dismissBanner.nextState(undefined); + const hasUnassignedItems = stateProvider.activeUser.getFake(HAS_UNASSIGNED_ITEMS); + hasUnassignedItems.nextState(false); + + const sut = sutFactory(); + expect(await firstValueFrom(sut.showBanner$)).toBe(false); + expect(apiService.getShowUnassignedCiphersBanner).not.toHaveBeenCalled(); + }); + + it("does not show the banner if it has already been dismissed", async () => { + const dismissBanner = stateProvider.activeUser.getFake(DISMISS_BANNER_KEY); + dismissBanner.nextState(true); + const hasUnassignedItems = stateProvider.activeUser.getFake(HAS_UNASSIGNED_ITEMS); + hasUnassignedItems.nextState(true); + + const sut = sutFactory(); + expect(await firstValueFrom(sut.showBanner$)).toBe(false); + expect(apiService.getShowUnassignedCiphersBanner).not.toHaveBeenCalled(); + }); + }); + + it("fetches unassigned ciphers value from server", async () => { + apiService.getShowUnassignedCiphersBanner.mockResolvedValue(true); + + const dismissBanner = stateProvider.activeUser.getFake(DISMISS_BANNER_KEY); + dismissBanner.nextState(false); + const hasUnassignedItems = stateProvider.activeUser.getFake(HAS_UNASSIGNED_ITEMS); + hasUnassignedItems.nextState(null); + + 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(hasUnassignedItems.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 index db73abc14a6..fe435e728d5 100644 --- 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 @@ -8,7 +8,7 @@ import { UserKeyDefinition, } from "@bitwarden/common/platform/state"; -const DISMISS_BANNER_KEY = new UserKeyDefinition( +export const DISMISS_BANNER_KEY = new UserKeyDefinition( UNASSIGNED_ITEMS_BANNER_DISK, "dismissBanner", { @@ -17,7 +17,7 @@ const DISMISS_BANNER_KEY = new UserKeyDefinition( }, ); -const HAS_UNASSIGNED_ITEMS = new UserKeyDefinition( +export const HAS_UNASSIGNED_ITEMS = new UserKeyDefinition( UNASSIGNED_ITEMS_BANNER_DISK, "hasUnassignedItems", { From 2992943b05d6aa8d9140dd8b7e9f7ad2b8633fda Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Wed, 10 Apr 2024 12:08:26 +1000 Subject: [PATCH 08/14] Fix mixed up names --- .../header/web-unassigned-items-banner.service.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 index fe435e728d5..32f66f09258 100644 --- 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 @@ -29,14 +29,14 @@ export const HAS_UNASSIGNED_ITEMS = new UserKeyDefinition( /** Displays a banner that tells users how to move their unassigned items into a collection. */ @Injectable({ providedIn: "root" }) export class WebUnassignedItemsBannerService { - private _hasUnassignedItems = this.stateProvider.getActive(DISMISS_BANNER_KEY); - private _showBanner = this.stateProvider.getActive(HAS_UNASSIGNED_ITEMS); + private _dismissBanner = this.stateProvider.getActive(DISMISS_BANNER_KEY); + private _hasUnassignedItems = this.stateProvider.getActive(HAS_UNASSIGNED_ITEMS); - showBanner$ = combineLatest([this._hasUnassignedItems.state$, this._showBanner.state$]).pipe( + showBanner$ = combineLatest([this._dismissBanner.state$, this._hasUnassignedItems.state$]).pipe( concatMap(async ([dismissBanner, hasUnassignedItems]) => { if (!dismissBanner && hasUnassignedItems == null) { const hasUnassignedItemsResponse = await this.apiService.getShowUnassignedCiphersBanner(); - await this._showBanner.update(() => hasUnassignedItemsResponse); + await this._hasUnassignedItems.update(() => hasUnassignedItemsResponse); return EMPTY; // to test, we could also just emit false and let the value update the next time around } @@ -50,6 +50,6 @@ export class WebUnassignedItemsBannerService { ) {} async hideBanner() { - await this._hasUnassignedItems.update(() => true); + await this._dismissBanner.update(() => true); } } From ac76328e05b9ae7c728791a9a7e8c61d31567a3e Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Wed, 10 Apr 2024 13:16:25 +1000 Subject: [PATCH 09/14] Simplify logic --- ...eb-unassigned-items-banner.service.spec.ts | 56 ++++++------------- .../web-unassigned-items-banner.service.ts | 39 +++++-------- 2 files changed, 33 insertions(+), 62 deletions(-) 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 index b71c476cc29..a9db11a2013 100644 --- 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 @@ -6,8 +6,7 @@ import { FakeStateProvider, mockAccountServiceWith } from "@bitwarden/common/spe import { UserId } from "@bitwarden/common/types/guid"; import { - DISMISS_BANNER_KEY, - HAS_UNASSIGNED_ITEMS, + SHOW_BANNER_KEY, WebUnassignedItemsBannerService, } from "./web-unassigned-items-banner.service"; @@ -23,54 +22,35 @@ describe("WebUnassignedItemsBanner", () => { apiService = mock(); }); - describe("given stored values only", () => { - it("shows the banner if has unassigned ciphers and has not been dismissed", async () => { - const dismissBanner = stateProvider.activeUser.getFake(DISMISS_BANNER_KEY); - dismissBanner.nextState(undefined); - const hasUnassignedItems = stateProvider.activeUser.getFake(HAS_UNASSIGNED_ITEMS); - hasUnassignedItems.nextState(true); + 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 does not have unassigned ciphers", async () => { - const dismissBanner = stateProvider.activeUser.getFake(DISMISS_BANNER_KEY); - dismissBanner.nextState(undefined); - const hasUnassignedItems = stateProvider.activeUser.getFake(HAS_UNASSIGNED_ITEMS); - hasUnassignedItems.nextState(false); - - const sut = sutFactory(); - expect(await firstValueFrom(sut.showBanner$)).toBe(false); - expect(apiService.getShowUnassignedCiphersBanner).not.toHaveBeenCalled(); - }); + const sut = sutFactory(); + expect(await firstValueFrom(sut.showBanner$)).toBe(true); + expect(apiService.getShowUnassignedCiphersBanner).not.toHaveBeenCalled(); + }); - it("does not show the banner if it has already been dismissed", async () => { - const dismissBanner = stateProvider.activeUser.getFake(DISMISS_BANNER_KEY); - dismissBanner.nextState(true); - const hasUnassignedItems = stateProvider.activeUser.getFake(HAS_UNASSIGNED_ITEMS); - hasUnassignedItems.nextState(true); + 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(); - }); + const sut = sutFactory(); + expect(await firstValueFrom(sut.showBanner$)).toBe(false); + expect(apiService.getShowUnassignedCiphersBanner).not.toHaveBeenCalled(); }); - it("fetches unassigned ciphers value from server", async () => { + it("fetches from server if local state has not been set yet", async () => { apiService.getShowUnassignedCiphersBanner.mockResolvedValue(true); - const dismissBanner = stateProvider.activeUser.getFake(DISMISS_BANNER_KEY); - dismissBanner.nextState(false); - const hasUnassignedItems = stateProvider.activeUser.getFake(HAS_UNASSIGNED_ITEMS); - hasUnassignedItems.nextState(null); + 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(hasUnassignedItems.state$)).toBe(true); + 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 index 32f66f09258..58de80276fc 100644 --- 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 @@ -1,5 +1,5 @@ import { Injectable } from "@angular/core"; -import { EMPTY, combineLatest, concatMap } from "rxjs"; +import { EMPTY, concatMap } from "rxjs"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { @@ -8,20 +8,11 @@ import { UserKeyDefinition, } from "@bitwarden/common/platform/state"; -export const DISMISS_BANNER_KEY = new UserKeyDefinition( +export const SHOW_BANNER_KEY = new UserKeyDefinition( UNASSIGNED_ITEMS_BANNER_DISK, - "dismissBanner", + "showBanner", { - deserializer: (b) => b ?? false, - clearOn: ["logout"], - }, -); - -export const HAS_UNASSIGNED_ITEMS = new UserKeyDefinition( - UNASSIGNED_ITEMS_BANNER_DISK, - "hasUnassignedItems", - { - deserializer: (b) => b ?? null, + deserializer: (b) => b, clearOn: ["logout"], }, ); @@ -29,18 +20,18 @@ export const HAS_UNASSIGNED_ITEMS = new UserKeyDefinition( /** Displays a banner that tells users how to move their unassigned items into a collection. */ @Injectable({ providedIn: "root" }) export class WebUnassignedItemsBannerService { - private _dismissBanner = this.stateProvider.getActive(DISMISS_BANNER_KEY); - private _hasUnassignedItems = this.stateProvider.getActive(HAS_UNASSIGNED_ITEMS); - - showBanner$ = combineLatest([this._dismissBanner.state$, this._hasUnassignedItems.state$]).pipe( - concatMap(async ([dismissBanner, hasUnassignedItems]) => { - if (!dismissBanner && hasUnassignedItems == null) { - const hasUnassignedItemsResponse = await this.apiService.getShowUnassignedCiphersBanner(); - await this._hasUnassignedItems.update(() => hasUnassignedItemsResponse); - return EMPTY; // to test, we could also just emit false and let the value update the next time around + 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 !dismissBanner && hasUnassignedItems; + return showBanner; }), ); @@ -50,6 +41,6 @@ export class WebUnassignedItemsBannerService { ) {} async hideBanner() { - await this._dismissBanner.update(() => true); + await this._showBanner.update(() => false); } } From 8d43a74b4a3ec0d183fdee414813c91193c24e17 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Wed, 10 Apr 2024 13:25:09 +1000 Subject: [PATCH 10/14] Add feature flag --- apps/web/src/app/layouts/header/web-header.component.html | 4 +++- apps/web/src/app/layouts/header/web-header.component.ts | 6 ++++++ libs/common/src/enums/feature-flag.enum.ts | 1 + 3 files changed, 10 insertions(+), 1 deletion(-) 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 f69c60dc786..1555726e2b6 100644 --- a/apps/web/src/app/layouts/header/web-header.component.html +++ b/apps/web/src/app/layouts/header/web-header.component.html @@ -1,7 +1,9 @@ {{ "unassignedItemsBanner" | i18n }} ; protected selfHosted: boolean; protected hostname = location.hostname; + protected unassignedItemsBannerEnabled$ = this.configService.getFeatureFlag$( + FeatureFlag.UnassignedItemsBanner, + ); constructor( private route: ActivatedRoute, @@ -39,6 +44,7 @@ export class WebHeaderComponent { private vaultTimeoutSettingsService: VaultTimeoutSettingsService, private messagingService: MessagingService, protected webUnassignedItemsBannerService: WebUnassignedItemsBannerService, + private configService: ConfigService, ) { this.routeData$ = this.route.data.pipe( map((params) => { diff --git a/libs/common/src/enums/feature-flag.enum.ts b/libs/common/src/enums/feature-flag.enum.ts index 9d427034bd5..b937e6c462d 100644 --- a/libs/common/src/enums/feature-flag.enum.ts +++ b/libs/common/src/enums/feature-flag.enum.ts @@ -9,6 +9,7 @@ export enum FeatureFlag { ShowPaymentMethodWarningBanners = "show-payment-method-warning-banners", EnableConsolidatedBilling = "enable-consolidated-billing", AC1795_UpdatedSubscriptionStatusSection = "AC-1795_updated-subscription-status-section", + UnassignedItemsBanner = "unassigned-items-banner", } // Replace this with a type safe lookup of the feature flag values in PM-2282 From 06d70ff1fac92cd17aa1564ba490db1f89937267 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Wed, 10 Apr 2024 14:18:53 +1000 Subject: [PATCH 11/14] Do not clear on logout --- .../app/layouts/header/web-unassigned-items-banner.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 58de80276fc..8f09b685479 100644 --- 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 @@ -13,7 +13,7 @@ export const SHOW_BANNER_KEY = new UserKeyDefinition( "showBanner", { deserializer: (b) => b, - clearOn: ["logout"], + clearOn: [], }, ); From bfc039cd15555cb003954afd9e1df261e15e46ed Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Wed, 10 Apr 2024 14:40:33 +1000 Subject: [PATCH 12/14] Show banner in browser as well --- apps/browser/src/_locales/en/messages.json | 3 +++ .../vault/current-tab.component.html | 25 ++++++++++++++----- .../components/vault/current-tab.component.ts | 10 ++++++++ .../layouts/header/web-header.component.html | 4 +-- .../layouts/header/web-header.component.ts | 4 +-- .../unassigned-items-banner.service.spec.ts | 9 +++---- .../unassigned-items-banner.service.ts | 2 +- 7 files changed, 40 insertions(+), 17 deletions(-) rename apps/web/src/app/layouts/header/web-unassigned-items-banner.service.spec.ts => libs/angular/src/services/unassigned-items-banner.service.spec.ts (88%) rename apps/web/src/app/layouts/header/web-unassigned-items-banner.service.ts => libs/angular/src/services/unassigned-items-banner.service.ts (96%) diff --git a/apps/browser/src/_locales/en/messages.json b/apps/browser/src/_locales/en/messages.json index d802d277001..0d30f695545 100644 --- a/apps/browser/src/_locales/en/messages.json +++ b/apps/browser/src/_locales/en/messages.json @@ -3005,5 +3005,8 @@ }, "passkeyRemoved": { "message": "Passkey removed" + }, + "unassignedItemsBanner": { + "message": "Unassigned organization items are no longer visible in your All Vaults view across devices and are now only accessible via the Admin Console. Assign these items to a collection from the Admin Console to make them visible." } } diff --git a/apps/browser/src/vault/popup/components/vault/current-tab.component.html b/apps/browser/src/vault/popup/components/vault/current-tab.component.html index c971f6c9371..1a42f707010 100644 --- a/apps/browser/src/vault/popup/components/vault/current-tab.component.html +++ b/apps/browser/src/vault/popup/components/vault/current-tab.component.html @@ -36,19 +36,32 @@

{{ "currentTab" | i18n }}

- -

{{ autofillCalloutText }}

+ +

+ {{ "unassignedItemsBanner" | i18n }} + {{ "learnMore" | i18n }} +

-

diff --git a/apps/browser/src/vault/popup/components/vault/current-tab.component.ts b/apps/browser/src/vault/popup/components/vault/current-tab.component.ts index d9cf6550fa5..c7cc67e8018 100644 --- a/apps/browser/src/vault/popup/components/vault/current-tab.component.ts +++ b/apps/browser/src/vault/popup/components/vault/current-tab.component.ts @@ -3,11 +3,14 @@ import { Router } from "@angular/router"; import { Subject, firstValueFrom } from "rxjs"; import { debounceTime, takeUntil } from "rxjs/operators"; +import { UnassignedItemsBannerService } from "@bitwarden/angular/services/unassigned-items-banner.service"; import { SearchService } from "@bitwarden/common/abstractions/search.service"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { AutofillOverlayVisibility } from "@bitwarden/common/autofill/constants"; import { AutofillSettingsServiceAbstraction } from "@bitwarden/common/autofill/services/autofill-settings.service"; +import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; import { BroadcasterService } from "@bitwarden/common/platform/abstractions/broadcaster.service"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { Utils } from "@bitwarden/common/platform/misc/utils"; @@ -24,6 +27,7 @@ import { BrowserApi } from "../../../../platform/browser/browser-api"; import BrowserPopupUtils from "../../../../platform/popup/browser-popup-utils"; import { VaultFilterService } from "../../../services/vault-filter.service"; + const BroadcasterSubscriptionId = "CurrentTabComponent"; @Component({ @@ -54,6 +58,10 @@ export class CurrentTabComponent implements OnInit, OnDestroy { private loadedTimeout: number; private searchTimeout: number; + protected unassignedItemsBannerEnabled$ = this.configService.getFeatureFlag$( + FeatureFlag.UnassignedItemsBanner, + ); + constructor( private platformUtilsService: PlatformUtilsService, private cipherService: CipherService, @@ -70,6 +78,8 @@ export class CurrentTabComponent implements OnInit, OnDestroy { private organizationService: OrganizationService, private vaultFilterService: VaultFilterService, private vaultSettingsService: VaultSettingsService, + private configService: ConfigService, + protected unassignedItemsBannerService: UnassignedItemsBannerService, ) {} async ngOnInit() { 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 1555726e2b6..e1cda607c01 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 6016463ebbf..622f92a8fbd 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,7 +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", @@ -43,7 +43,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/libs/angular/src/services/unassigned-items-banner.service.spec.ts similarity index 88% rename from apps/web/src/app/layouts/header/web-unassigned-items-banner.service.spec.ts rename to libs/angular/src/services/unassigned-items-banner.service.spec.ts index a9db11a2013..eedfbf3429f 100644 --- a/apps/web/src/app/layouts/header/web-unassigned-items-banner.service.spec.ts +++ b/libs/angular/src/services/unassigned-items-banner.service.spec.ts @@ -5,16 +5,13 @@ 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"; +import { SHOW_BANNER_KEY, UnassignedItemsBannerService } from "./unassigned-items-banner.service"; -describe("WebUnassignedItemsBanner", () => { +describe("UnassignedItemsBanner", () => { let stateProvider: FakeStateProvider; let apiService: MockProxy; - const sutFactory = () => new WebUnassignedItemsBannerService(stateProvider, apiService); + const sutFactory = () => new UnassignedItemsBannerService(stateProvider, apiService); beforeEach(() => { const fakeAccountService = mockAccountServiceWith("userId" as UserId); diff --git a/apps/web/src/app/layouts/header/web-unassigned-items-banner.service.ts b/libs/angular/src/services/unassigned-items-banner.service.ts similarity index 96% rename from apps/web/src/app/layouts/header/web-unassigned-items-banner.service.ts rename to libs/angular/src/services/unassigned-items-banner.service.ts index 8f09b685479..dd374fe5cef 100644 --- a/apps/web/src/app/layouts/header/web-unassigned-items-banner.service.ts +++ b/libs/angular/src/services/unassigned-items-banner.service.ts @@ -19,7 +19,7 @@ export const SHOW_BANNER_KEY = new UserKeyDefinition( /** Displays a banner that tells users how to move their unassigned items into a collection. */ @Injectable({ providedIn: "root" }) -export class WebUnassignedItemsBannerService { +export class UnassignedItemsBannerService { private _showBanner = this.stateProvider.getActive(SHOW_BANNER_KEY); showBanner$ = this._showBanner.state$.pipe( From c2f1650a0b9c8a1a496bfd5a2c905199eb734220 Mon Sep 17 00:00:00 2001 From: Addison Beck Date: Wed, 10 Apr 2024 09:15:49 -0500 Subject: [PATCH 13/14] Update apps/browser/src/_locales/en/messages.json --- apps/browser/src/_locales/en/messages.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/browser/src/_locales/en/messages.json b/apps/browser/src/_locales/en/messages.json index 0d30f695545..6e57c05be13 100644 --- a/apps/browser/src/_locales/en/messages.json +++ b/apps/browser/src/_locales/en/messages.json @@ -3007,6 +3007,6 @@ "message": "Passkey removed" }, "unassignedItemsBanner": { - "message": "Unassigned organization items are no longer visible in your All Vaults view across devices and are now only accessible via the Admin Console. Assign these items to a collection from the Admin Console to make them visible." + "message": "Notice: Unassigned organization items are no longer visible in your All Vaults view across devices and are now only accessible via the Admin Console. Assign these items to a collection from the Admin Console to make them visible." } } From 61bb7f522469417b13ba0e683466eda21d0d1481 Mon Sep 17 00:00:00 2001 From: Addison Beck Date: Wed, 10 Apr 2024 12:56:37 -0500 Subject: [PATCH 14/14] Update copy --- apps/browser/src/_locales/en/messages.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/browser/src/_locales/en/messages.json b/apps/browser/src/_locales/en/messages.json index 6e57c05be13..9d444ce40ef 100644 --- a/apps/browser/src/_locales/en/messages.json +++ b/apps/browser/src/_locales/en/messages.json @@ -3007,6 +3007,6 @@ "message": "Passkey removed" }, "unassignedItemsBanner": { - "message": "Notice: Unassigned organization items are no longer visible in your All Vaults view across devices and are now only accessible via the Admin Console. Assign these items to a collection from the Admin Console to make them visible." + "message": "Notice: Unassigned organization items are no longer visible in the All Vaults view and only accessible via the Admin Console. Assign these items to a collection from the Admin Console to make them visible." } }