[PM-6658] Migrate disableFavicon to Domain Settings service and remove Settings service (#8333)

* add showFavicons to domain settings

* replace usages of disableFavicon with showFavicons via the domain settings service and remove/replace settings service

* create migration for disableFavicon

* cleanup

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

@@ -1,8 +0,0 @@
-import { Observable } from "rxjs";
-
-export abstract class SettingsService {
-  disableFavicon$: Observable<boolean>;
-
-  setDisableFavicon: (value: boolean) => Promise<any>;
-  getDisableFavicon: () => boolean;
-}

libs/common/src/autofill/services/domain-settings.service.ts

@@ -16,6 +16,10 @@ import {
   UserKeyDefinition,
 } from "../../platform/state";
 
+const SHOW_FAVICONS = new KeyDefinition(DOMAIN_SETTINGS_DISK, "showFavicons", {
+  deserializer: (value: boolean) => value ?? true,
+});
+
 const NEVER_DOMAINS = new KeyDefinition(DOMAIN_SETTINGS_DISK, "neverDomains", {
   deserializer: (value: NeverDomains) => value ?? null,
 });
@@ -34,6 +38,8 @@ const DEFAULT_URI_MATCH_STRATEGY = new KeyDefinition(
 );
 
 export abstract class DomainSettingsService {
+  showFavicons$: Observable<boolean>;
+  setShowFavicons: (newValue: boolean) => Promise<void>;
   neverDomains$: Observable<NeverDomains>;
   setNeverDomains: (newValue: NeverDomains) => Promise<void>;
   equivalentDomains$: Observable<EquivalentDomains>;
@@ -44,6 +50,9 @@ export abstract class DomainSettingsService {
 }
 
 export class DefaultDomainSettingsService implements DomainSettingsService {
+  private showFaviconsState: GlobalState<boolean>;
+  readonly showFavicons$: Observable<boolean>;
+
   private neverDomainsState: GlobalState<NeverDomains>;
   readonly neverDomains$: Observable<NeverDomains>;
 
@@ -54,6 +63,9 @@ export class DefaultDomainSettingsService implements DomainSettingsService {
   readonly defaultUriMatchStrategy$: Observable<UriMatchStrategySetting>;
 
   constructor(private stateProvider: StateProvider) {
+    this.showFaviconsState = this.stateProvider.getGlobal(SHOW_FAVICONS);
+    this.showFavicons$ = this.showFaviconsState.state$.pipe(map((x) => x ?? true));
+
     this.neverDomainsState = this.stateProvider.getGlobal(NEVER_DOMAINS);
     this.neverDomains$ = this.neverDomainsState.state$.pipe(map((x) => x ?? null));
 
@@ -66,6 +78,10 @@ export class DefaultDomainSettingsService implements DomainSettingsService {
     );
   }
 
+  async setShowFavicons(newValue: boolean): Promise<void> {
+    await this.showFaviconsState.update(() => newValue);
+  }
+
   async setNeverDomains(newValue: NeverDomains): Promise<void> {
     await this.neverDomainsState.update(() => newValue);
   }

libs/common/src/platform/abstractions/state.service.ts

@@ -170,14 +170,6 @@ export abstract class StateService<T extends Account = Account> {
    * @deprecated Do not call this directly, use SendService
    */
   setDecryptedSends: (value: SendView[], options?: StorageOptions) => Promise<void>;
-  /**
-   * @deprecated Do not call this, use SettingsService
-   */
-  getDisableFavicon: (options?: StorageOptions) => Promise<boolean>;
-  /**
-   * @deprecated Do not call this, use SettingsService
-   */
-  setDisableFavicon: (value: boolean, options?: StorageOptions) => Promise<void>;
   getDisableGa: (options?: StorageOptions) => Promise<boolean>;
   setDisableGa: (value: boolean, options?: StorageOptions) => Promise<void>;
   getDuckDuckGoSharedKey: (options?: StorageOptions) => Promise<string>;

libs/common/src/platform/models/domain/global-state.ts

@@ -10,7 +10,6 @@ export class GlobalState {
   theme?: ThemeType = ThemeType.System;
   window?: WindowState = new WindowState();
   twoFactorToken?: string;
-  disableFavicon?: boolean;
   biometricFingerprintValidated?: boolean;
   vaultTimeout?: number;
   vaultTimeoutAction?: string;

libs/common/src/platform/services/state.service.ts

@@ -710,27 +710,6 @@ export class StateService<
     );
   }
 
-  async getDisableFavicon(options?: StorageOptions): Promise<boolean> {
-    return (
-      (
-        await this.getGlobals(
-          this.reconcileOptions(options, await this.defaultOnDiskLocalOptions()),
-        )
-      )?.disableFavicon ?? false
-    );
-  }
-
-  async setDisableFavicon(value: boolean, options?: StorageOptions): Promise<void> {
-    const globals = await this.getGlobals(
-      this.reconcileOptions(options, await this.defaultOnDiskLocalOptions()),
-    );
-    globals.disableFavicon = value;
-    await this.saveGlobals(
-      globals,
-      this.reconcileOptions(options, await this.defaultOnDiskLocalOptions()),
-    );
-  }
-
   async getDisableGa(options?: StorageOptions): Promise<boolean> {
     return (
       (await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())))

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

@@ -1,40 +0,0 @@
-import { BehaviorSubject, concatMap } from "rxjs";
-
-import { SettingsService as SettingsServiceAbstraction } from "../abstractions/settings.service";
-import { StateService } from "../platform/abstractions/state.service";
-import { Utils } from "../platform/misc/utils";
-
-export class SettingsService implements SettingsServiceAbstraction {
-  protected _disableFavicon = new BehaviorSubject<boolean>(null);
-
-  disableFavicon$ = this._disableFavicon.asObservable();
-
-  constructor(private stateService: StateService) {
-    this.stateService.activeAccountUnlocked$
-      .pipe(
-        concatMap(async (unlocked) => {
-          if (Utils.global.bitwardenContainerService == null) {
-            return;
-          }
-
-          if (!unlocked) {
-            return;
-          }
-
-          const disableFavicon = await this.stateService.getDisableFavicon();
-
-          this._disableFavicon.next(disableFavicon);
-        }),
-      )
-      .subscribe();
-  }
-
-  async setDisableFavicon(value: boolean) {
-    this._disableFavicon.next(value);
-    await this.stateService.setDisableFavicon(value);
-  }
-
-  getDisableFavicon(): boolean {
-    return this._disableFavicon.getValue();
-  }
-}

libs/common/src/state-migrations/migrate.ts

@@ -37,6 +37,7 @@ import { MoveBillingAccountProfileMigrator } from "./migrations/39-move-billing-
 import { RemoveEverBeenUnlockedMigrator } from "./migrations/4-remove-ever-been-unlocked";
 import { OrganizationMigrator } from "./migrations/40-move-organization-state-to-state-provider";
 import { EventCollectionMigrator } from "./migrations/41-move-event-collection-to-state-provider";
+import { EnableFaviconMigrator } from "./migrations/42-move-enable-favicon-to-domain-settings-state-provider";
 import { AddKeyTypeToOrgKeysMigrator } from "./migrations/5-add-key-type-to-org-keys";
 import { RemoveLegacyEtmKeyMigrator } from "./migrations/6-remove-legacy-etm-key";
 import { MoveBiometricAutoPromptToAccount } from "./migrations/7-move-biometric-auto-prompt-to-account";
@@ -45,7 +46,7 @@ import { MoveBrowserSettingsToGlobal } from "./migrations/9-move-browser-setting
 import { MinVersionMigrator } from "./migrations/min-version";
 
 export const MIN_VERSION = 3;
-export const CURRENT_VERSION = 41;
+export const CURRENT_VERSION = 42;
 export type MinVersion = typeof MIN_VERSION;
 
 export function createMigrationBuilder() {
@@ -88,7 +89,8 @@ export function createMigrationBuilder() {
     .with(TokenServiceStateProviderMigrator, 37, 38)
     .with(MoveBillingAccountProfileMigrator, 38, 39)
     .with(OrganizationMigrator, 39, 40)
-    .with(EventCollectionMigrator, 40, CURRENT_VERSION);
+    .with(EventCollectionMigrator, 40, 41)
+    .with(EnableFaviconMigrator, 41, 42);
 }
 
 export async function currentVersion(

libs/common/src/state-migrations/migrations/42-move-enable-favicon-to-domain-settings-state-provider.spec.ts

@@ -0,0 +1,108 @@
+import { MockProxy } from "jest-mock-extended";
+
+import { KeyDefinitionLike, MigrationHelper } from "../migration-helper";
+import { mockMigrationHelper } from "../migration-helper.spec";
+
+import { EnableFaviconMigrator } from "./42-move-enable-favicon-to-domain-settings-state-provider";
+
+function exampleJSON() {
+  return {
+    global: {
+      otherStuff: "otherStuff1",
+      disableFavicon: true,
+    },
+    authenticatedAccounts: ["user-1", "user-2"],
+    "user-1": {
+      settings: {
+        otherStuff: "otherStuff2",
+      },
+      otherStuff: "otherStuff3",
+    },
+    "user-2": {
+      settings: {
+        otherStuff: "otherStuff4",
+      },
+      otherStuff: "otherStuff5",
+    },
+  };
+}
+
+function rollbackJSON() {
+  return {
+    global_domainSettings_showFavicons: false,
+    global: {
+      otherStuff: "otherStuff1",
+    },
+    authenticatedAccounts: ["user-1", "user-2"],
+    "user-1": {
+      settings: {
+        otherStuff: "otherStuff2",
+      },
+      otherStuff: "otherStuff3",
+    },
+    "user-2": {
+      settings: {
+        otherStuff: "otherStuff4",
+      },
+      otherStuff: "otherStuff5",
+    },
+  };
+}
+
+const showFaviconsKeyDefinition: KeyDefinitionLike = {
+  stateDefinition: {
+    name: "domainSettings",
+  },
+  key: "showFavicons",
+};
+
+describe("EnableFaviconMigrator", () => {
+  let helper: MockProxy<MigrationHelper>;
+  let sut: EnableFaviconMigrator;
+
+  describe("migrate", () => {
+    beforeEach(() => {
+      helper = mockMigrationHelper(exampleJSON(), 41);
+      sut = new EnableFaviconMigrator(41, 42);
+    });
+
+    it("should remove global disableFavicon", async () => {
+      await sut.migrate(helper);
+      expect(helper.set).toHaveBeenCalledTimes(1);
+      expect(helper.set).toHaveBeenCalledWith("global", {
+        otherStuff: "otherStuff1",
+      });
+    });
+
+    it("should set global showFavicons", async () => {
+      await sut.migrate(helper);
+
+      expect(helper.setToGlobal).toHaveBeenCalledTimes(1);
+      expect(helper.setToGlobal).toHaveBeenCalledWith(showFaviconsKeyDefinition, false);
+    });
+  });
+
+  describe("rollback", () => {
+    beforeEach(() => {
+      helper = mockMigrationHelper(rollbackJSON(), 42);
+      sut = new EnableFaviconMigrator(41, 42);
+    });
+
+    it("should null global showFavicons", async () => {
+      await sut.rollback(helper);
+
+      expect(helper.setToGlobal).toHaveBeenCalledTimes(1);
+      expect(helper.setToGlobal).toHaveBeenCalledWith(showFaviconsKeyDefinition, null);
+    });
+
+    it("should add global disableFavicon back", async () => {
+      await sut.rollback(helper);
+
+      expect(helper.set).toHaveBeenCalledTimes(1);
+      expect(helper.set).toHaveBeenCalledWith("global", {
+        disableFavicon: true,
+        otherStuff: "otherStuff1",
+      });
+    });
+  });
+});

libs/common/src/state-migrations/migrations/42-move-enable-favicon-to-domain-settings-state-provider.ts

@@ -0,0 +1,45 @@
+import { KeyDefinitionLike, MigrationHelper } from "../migration-helper";
+import { Migrator } from "../migrator";
+
+type ExpectedGlobalState = {
+  disableFavicon?: boolean;
+};
+
+const ShowFaviconDefinition: KeyDefinitionLike = {
+  stateDefinition: {
+    name: "domainSettings",
+  },
+  key: "showFavicons",
+};
+
+export class EnableFaviconMigrator extends Migrator<41, 42> {
+  async migrate(helper: MigrationHelper): Promise<void> {
+    // global state ("disableFavicon" -> "showFavicons")
+    const globalState = await helper.get<ExpectedGlobalState>("global");
+
+    if (globalState?.disableFavicon != null) {
+      await helper.setToGlobal(ShowFaviconDefinition, !globalState.disableFavicon);
+
+      // delete `disableFavicon` from state global
+      delete globalState.disableFavicon;
+
+      await helper.set<ExpectedGlobalState>("global", globalState);
+    }
+  }
+
+  async rollback(helper: MigrationHelper): Promise<void> {
+    // global state ("showFavicons" -> "disableFavicon")
+    const globalState = (await helper.get<ExpectedGlobalState>("global")) || {};
+    const showFavicons: boolean = await helper.getFromGlobal(ShowFaviconDefinition);
+
+    if (showFavicons != null) {
+      await helper.set<ExpectedGlobalState>("global", {
+        ...globalState,
+        disableFavicon: !showFavicons,
+      });
+
+      // remove the global state provider framework key for `showFavicons`
+      await helper.setToGlobal(ShowFaviconDefinition, null);
+    }
+  }
+}

libs/common/src/vault/icon/build-cipher-icon.ts

@@ -2,12 +2,7 @@ import { Utils } from "../../platform/misc/utils";
 import { CipherType } from "../enums/cipher-type";
 import { CipherView } from "../models/view/cipher.view";
 
-export function buildCipherIcon(
-  iconsServerUrl: string,
-  cipher: CipherView,
-  isFaviconDisabled: boolean,
-) {
-  const imageEnabled = !isFaviconDisabled;
+export function buildCipherIcon(iconsServerUrl: string, cipher: CipherView, showFavicon: boolean) {
   let icon;
   let image;
   let fallbackImage = "";
@@ -38,17 +33,17 @@ export function buildCipherIcon(
           icon = "bwi-apple";
           image = null;
         } else if (
-          imageEnabled &&
+          showFavicon &&
           hostnameUri.indexOf("://") === -1 &&
           hostnameUri.indexOf(".") > -1
         ) {
           hostnameUri = `http://${hostnameUri}`;
           isWebsite = true;
-        } else if (imageEnabled) {
+        } else if (showFavicon) {
           isWebsite = hostnameUri.indexOf("http") === 0 && hostnameUri.indexOf(".") > -1;
         }
 
-        if (imageEnabled && isWebsite) {
+        if (showFavicon && isWebsite) {
           try {
             image = `${iconsServerUrl}/${Utils.getHostname(hostnameUri)}/icon.png`;
             fallbackImage = "images/bwi-globe.png";
@@ -65,7 +60,7 @@ export function buildCipherIcon(
       break;
     case CipherType.Card:
       icon = "bwi-credit-card";
-      if (imageEnabled && cipher.card.brand in cardIcons) {
+      if (showFavicon && cipher.card.brand in cardIcons) {
         icon = `credit-card-icon ${cardIcons[cipher.card.brand]}`;
       }
       break;
@@ -77,7 +72,7 @@ export function buildCipherIcon(
   }
 
   return {
-    imageEnabled,
+    imageEnabled: showFavicon,
     image,
     fallbackImage,
     icon,