[PM-13422] Fix sync time (#16603)

Do not update the "last sync time" when an error occurs during the sync process, including a network error when retrieving the account's revision date/time from the server. Update the sync time when a sync fires automatically, or when forced, in order to make it clear to the user that the extension's data is current.

libs/common/src/platform/sync/default-sync.service.spec.ts

@@ -292,5 +292,100 @@ describe("DefaultSyncService", () => {
         expect(masterPasswordAbstraction.setMasterPasswordUnlockData).not.toHaveBeenCalled();
       });
     });
+
+    describe("mutate 'last update time'", () => {
+      let mockUserState: { update: jest.Mock };
+
+      const setupMockUserState = () => {
+        const mockUserState = { update: jest.fn() };
+        jest.spyOn(stateProvider, "getUser").mockReturnValue(mockUserState as any);
+        return mockUserState;
+      };
+
+      const setupSyncScenario = (revisionDate: Date, lastSyncDate: Date) => {
+        jest.spyOn(apiService, "getAccountRevisionDate").mockResolvedValue(revisionDate.getTime());
+        jest.spyOn(sut as any, "getLastSync").mockResolvedValue(lastSyncDate);
+      };
+
+      const expectUpdateCallCount = (
+        mockUserState: { update: jest.Mock },
+        expectedCount: number,
+      ) => {
+        if (expectedCount === 0) {
+          expect(mockUserState.update).not.toHaveBeenCalled();
+        } else {
+          expect(mockUserState.update).toHaveBeenCalledTimes(expectedCount);
+        }
+      };
+
+      const defaultSyncOptions = { allowThrowOnError: true, skipTokenRefresh: true };
+      const errorTolerantSyncOptions = { allowThrowOnError: false, skipTokenRefresh: true };
+
+      beforeEach(() => {
+        mockUserState = setupMockUserState();
+      });
+
+      it("uses the current time when a sync is forced", async () => {
+        // Mock the value of this observable because it's used in `syncProfile`. Without it, the test breaks.
+        keyConnectorService.convertAccountRequired$ = of(false);
+
+        // Baseline date/time to compare sync time to, in order to avoid needing to use some kind of fake date provider.
+        const beforeSync = Date.now();
+
+        // send it!
+        await sut.fullSync(true, defaultSyncOptions);
+
+        expectUpdateCallCount(mockUserState, 1);
+        // Get the first and only call to update(...)
+        const updateCall = mockUserState.update.mock.calls[0];
+        // Get the first argument to update(...) -- this will be the date callback that returns the date of the last successful sync
+        const dateCallback = updateCall[0];
+        const actualTime = dateCallback() as Date;
+
+        expect(Math.abs(actualTime.getTime() - beforeSync)).toBeLessThan(1);
+      });
+
+      it("updates last sync time when no sync is necessary", async () => {
+        const revisionDate = new Date(1);
+        setupSyncScenario(revisionDate, revisionDate);
+
+        const syncResult = await sut.fullSync(false, defaultSyncOptions);
+
+        // Sync should complete but return false since no sync was needed
+        expect(syncResult).toBe(false);
+        expectUpdateCallCount(mockUserState, 1);
+      });
+
+      it("updates last sync time when sync is successful", async () => {
+        setupSyncScenario(new Date(2), new Date(1));
+
+        const syncResult = await sut.fullSync(false, defaultSyncOptions);
+
+        expect(syncResult).toBe(true);
+        expectUpdateCallCount(mockUserState, 1);
+      });
+
+      describe("error scenarios", () => {
+        it("does not update last sync time when sync fails", async () => {
+          apiService.getSync.mockRejectedValue(new Error("not connected"));
+
+          const syncResult = await sut.fullSync(true, errorTolerantSyncOptions);
+
+          expect(syncResult).toBe(false);
+          expectUpdateCallCount(mockUserState, 0);
+        });
+
+        it("does not update last sync time when account revision check fails", async () => {
+          jest
+            .spyOn(apiService, "getAccountRevisionDate")
+            .mockRejectedValue(new Error("not connected"));
+
+          const syncResult = await sut.fullSync(false, errorTolerantSyncOptions);
+
+          expect(syncResult).toBe(false);
+          expectUpdateCallCount(mockUserState, 0);
+        });
+      });
+    });
   });
 });

libs/common/src/platform/sync/default-sync.service.ts

@@ -134,9 +134,11 @@ export class DefaultSyncService extends CoreSyncService {
 
     const now = new Date();
     let needsSync = false;
+    let needsSyncSucceeded = true;
     try {
       needsSync = await this.needsSyncing(forceSync);
     } catch (e) {
+      needsSyncSucceeded = false;
       if (allowThrowOnError) {
         this.syncCompleted(false, userId);
         throw e;
@@ -144,7 +146,9 @@ export class DefaultSyncService extends CoreSyncService {
     }
 
     if (!needsSync) {
-      await this.setLastSync(now, userId);
+      if (needsSyncSucceeded) {
+        await this.setLastSync(now, userId);
+      }
       return this.syncCompleted(false, userId);
     }