[PM-9442] Add tests for undefined state values and proper emulation of ElectronStorageService in tests (#9910)

* fix: handle undefined value in migration 66

* fix: the if-statement was typo

* feat: duplicate error behavior in fake storage service

* feat: fix all migrations that were setting undefined values

* feat: add test for disabled fingrint in migration 66

* fix: default single user state saving undefined value to state

* revert: awaiting floating promise

gonna fix this in a separate PR

* Revert "feat: fix all migrations that were setting undefined values"

This reverts commit 034713256c.

* feat: automatically convert save to remove

* Revert "fix: default single user state saving undefined value to state"

This reverts commit 6c36da6ba5.

apps/desktop/src/platform/services/electron-storage.service.ts

@@ -82,9 +82,14 @@ export class ElectronStorageService implements AbstractStorageService {
   }
 
   save(key: string, obj: unknown): Promise<void> {
+    if (obj === undefined) {
+      return this.remove(key);
+    }
+
     if (obj instanceof Set) {
       obj = Array.from(obj);
     }
+
     this.store.set(key, obj);
     this.updatesSubject.next({ key, updateType: "save" });
     return Promise.resolve();

libs/common/spec/fake-storage.service.ts

@@ -8,6 +8,8 @@ import {
 } from "../src/platform/abstractions/storage.service";
 import { StorageOptions } from "../src/platform/models/domain/storage-options";
 
+const INTERNAL_KEY = "__internal__";
+
 export class FakeStorageService implements AbstractStorageService, ObservableStorageService {
   private store: Record<string, unknown>;
   private updatesSubject = new Subject<StorageUpdate>();
@@ -63,13 +65,32 @@ export class FakeStorageService implements AbstractStorageService, ObservableSto
     this.mock.has(key, options);
     return Promise.resolve(this.store[key] != null);
   }
-  save<T>(key: string, obj: T, options?: StorageOptions): Promise<void> {
+  async save<T>(key: string, obj: T, options?: StorageOptions): Promise<void> {
+    // These exceptions are copied from https://github.com/sindresorhus/conf/blob/608adb0c46fb1680ddbd9833043478367a64c120/source/index.ts#L193-L203
+    // which is a library that is used by `ElectronStorageService`. We add them here to ensure that the behavior in our testing mirrors the real world.
+    if (typeof key !== "string" && typeof key !== "object") {
+      throw new TypeError(
+        `Expected \`key\` to be of type \`string\` or \`object\`, got ${typeof key}`,
+      );
+    }
+
+    // We don't throw this error because ElectronStorageService automatically detects this case
+    // and calls `delete()` instead of `set()`.
+    // if (typeof key !== "object" && obj === undefined) {
+    //   throw new TypeError("Use `delete()` to clear values");
+    // }
+
+    if (this._containsReservedKey(key)) {
+      throw new TypeError(
+        `Please don't use the ${INTERNAL_KEY} key, as it's used to manage this module internal operations.`,
+      );
+    }
+
     // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
     // eslint-disable-next-line @typescript-eslint/no-floating-promises
     this.mock.save(key, obj, options);
     this.store[key] = obj;
     this.updatesSubject.next({ key: key, updateType: "save" });
-    return Promise.resolve();
   }
   remove(key: string, options?: StorageOptions): Promise<void> {
     // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
@@ -79,4 +100,20 @@ export class FakeStorageService implements AbstractStorageService, ObservableSto
     this.updatesSubject.next({ key: key, updateType: "remove" });
     return Promise.resolve();
   }
+
+  private _containsReservedKey(key: string | Partial<unknown>): boolean {
+    if (typeof key === "object") {
+      const firsKey = Object.keys(key)[0];
+
+      if (firsKey === INTERNAL_KEY) {
+        return true;
+      }
+    }
+
+    if (typeof key !== "string") {
+      return false;
+    }
+
+    return false;
+  }
 }

libs/common/src/state-migrations/migrations/66-move-final-desktop-settings.spec.ts

@@ -92,6 +92,45 @@ describe("MoveDesktopSettings", () => {
         global_desktopSettings_browserIntegrationFingerprintEnabled: false,
       },
     },
+    {
+      it: "migrates browser integration without fingerprint enabled",
+      preMigration: {
+        global_account_accounts: {
+          user1: {},
+          otherUser: {},
+        },
+        user1: {
+          settings: {
+            minimizeOnCopyToClipboard: false,
+          },
+        },
+        otherUser: {
+          settings: {
+            random: "stuff",
+          },
+        },
+        global: {
+          enableBrowserIntegration: true,
+        },
+      },
+      postMigration: {
+        global_account_accounts: {
+          user1: {},
+          otherUser: {},
+        },
+        global: {},
+        user1: {
+          settings: {},
+        },
+        otherUser: {
+          settings: {
+            random: "stuff",
+          },
+        },
+        global_desktopSettings_browserIntegrationEnabled: true,
+        user_user1_desktopSettings_minimizeOnCopy: false,
+      },
+    },
     {
       it: "does not move non-existant values",
       preMigration: {