refactor: update roles immediately without waiting for full sync (#2294)

packages/snjs/lib/Services/Features/FeaturesService.spec.ts

@@ -170,7 +170,9 @@ describe('featuresService', () => {
       featuresService.getExperimentalFeatures = jest.fn().mockReturnValue([FeatureIdentifier.PlusEditor])
 
       featuresService.initializeFromDisk()
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+      await featuresService.updateOnlineRoles(newRoles)
+      await featuresService.fetchFeatures('123')
+
       expect(itemManager.createItem).not.toHaveBeenCalled()
     })
 
@@ -191,7 +193,9 @@ describe('featuresService', () => {
       featuresService.getEnabledExperimentalFeatures = jest.fn().mockReturnValue([FeatureIdentifier.PlusEditor])
 
       featuresService.initializeFromDisk()
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+      await featuresService.updateOnlineRoles(newRoles)
+      await featuresService.fetchFeatures('123')
+
       expect(itemManager.createItem).toHaveBeenCalled()
     })
   })
@@ -226,9 +230,10 @@ describe('featuresService', () => {
       const spy = jest.spyOn(featuresService, 'notifyEvent' as never)
 
       const newRoles = [...roles, RoleName.NAMES.ProUser]
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+      await featuresService.updateOnlineRoles(newRoles)
+      await featuresService.fetchFeatures('123')
 
-      expect(spy.mock.calls[2][0]).toEqual(FeaturesEvent.DidPurchaseSubscription)
+      expect(spy.mock.calls[0][0]).toEqual(FeaturesEvent.DidPurchaseSubscription)
     })
 
     it('should not notify of subscription purchase on initial roles load after sign in', async () => {
@@ -240,7 +245,8 @@ describe('featuresService', () => {
       const spy = jest.spyOn(featuresService, 'notifyEvent' as never)
 
       const newRoles = [...roles, RoleName.NAMES.ProUser]
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+      await featuresService.updateOnlineRoles(newRoles)
+      await featuresService.fetchFeatures('123')
 
       const triggeredEvents = spy.mock.calls.map((call) => call[0])
       expect(triggeredEvents).not.toContain(FeaturesEvent.DidPurchaseSubscription)
@@ -252,8 +258,11 @@ describe('featuresService', () => {
       storageService.getValue = jest.fn().mockReturnValue(roles)
       const featuresService = createService()
       featuresService.initializeFromDisk()
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+
+      await featuresService.updateOnlineRoles(newRoles)
       expect(storageService.setValue).toHaveBeenCalledWith(StorageKey.UserRoles, newRoles)
+
+      await featuresService.fetchFeatures('123')
       expect(apiService.getUserFeatures).toHaveBeenCalledWith('123')
     })
 
@@ -263,7 +272,9 @@ describe('featuresService', () => {
       storageService.getValue = jest.fn().mockReturnValue(roles)
       const featuresService = createService()
       featuresService.initializeFromDisk()
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+      await featuresService.updateOnlineRoles(newRoles)
+      await featuresService.fetchFeatures('123')
+
       expect(storageService.setValue).toHaveBeenCalledWith(StorageKey.UserRoles, newRoles)
       expect(apiService.getUserFeatures).toHaveBeenCalledWith('123')
     })
@@ -274,7 +285,9 @@ describe('featuresService', () => {
       storageService.getValue = jest.fn().mockReturnValue(roles)
       const featuresService = createService()
       featuresService.initializeFromDisk()
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+      await featuresService.updateOnlineRoles(newRoles)
+      await featuresService.fetchFeatures('123')
+
       expect(storageService.setValue).toHaveBeenCalledWith(StorageKey.UserFeatures, features)
     })
 
@@ -284,7 +297,9 @@ describe('featuresService', () => {
       storageService.getValue = jest.fn().mockReturnValue(roles)
       const featuresService = createService()
       featuresService.initializeFromDisk()
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+      await featuresService.updateOnlineRoles(newRoles)
+      await featuresService.fetchFeatures('123')
+
       expect(itemManager.createItem).toHaveBeenCalledTimes(2)
       expect(itemManager.createItem).toHaveBeenCalledWith(
         ContentType.Theme,
@@ -328,7 +343,8 @@ describe('featuresService', () => {
       itemManager.getItems = jest.fn().mockReturnValue([existingItem])
       const featuresService = createService()
       featuresService.initializeFromDisk()
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+      await featuresService.updateOnlineRoles(newRoles)
+      await featuresService.fetchFeatures('123')
 
       expect(itemManager.changeComponent).toHaveBeenCalledWith(existingItem, expect.any(Function))
     })
@@ -354,7 +370,9 @@ describe('featuresService', () => {
 
       const featuresService = createService()
       featuresService.initializeFromDisk()
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+      await featuresService.updateOnlineRoles(newRoles)
+      await featuresService.fetchFeatures('123')
+
       expect(itemManager.createItem).toHaveBeenCalledWith(
         ContentType.Component,
         expect.objectContaining({
@@ -401,7 +419,9 @@ describe('featuresService', () => {
 
       const featuresService = createService()
       featuresService.initializeFromDisk()
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+      await featuresService.updateOnlineRoles(newRoles)
+      await featuresService.fetchFeatures('123')
+
       expect(itemManager.setItemsToBeDeleted).toHaveBeenCalledWith([existingItem])
     })
 
@@ -424,7 +444,9 @@ describe('featuresService', () => {
       storageService.getValue = jest.fn().mockReturnValue(roles)
       const featuresService = createService()
       featuresService.initializeFromDisk()
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+      await featuresService.updateOnlineRoles(newRoles)
+      await featuresService.fetchFeatures('123')
+
       expect(itemManager.createItem).not.toHaveBeenCalled()
     })
 
@@ -447,7 +469,9 @@ describe('featuresService', () => {
       storageService.getValue = jest.fn().mockReturnValue(roles)
       const featuresService = createService()
       featuresService.initializeFromDisk()
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+      await featuresService.updateOnlineRoles(newRoles)
+      await featuresService.fetchFeatures('123')
+
       expect(itemManager.createItem).not.toHaveBeenCalled()
     })
 
@@ -455,10 +479,14 @@ describe('featuresService', () => {
       storageService.getValue = jest.fn().mockReturnValue(roles)
       const featuresService = createService()
       featuresService.initializeFromDisk()
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', roles)
+      await featuresService.updateOnlineRoles(roles)
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', roles)
+      await featuresService.fetchFeatures('123')
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', roles)
+      await featuresService.updateOnlineRoles(roles)
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', roles)
+      await featuresService.fetchFeatures('123')
+      await featuresService.updateOnlineRoles(roles)
+      await featuresService.fetchFeatures('123')
+      await featuresService.updateOnlineRoles(roles)
+      await featuresService.fetchFeatures('123')
       expect(storageService.setValue).toHaveBeenCalledTimes(2)
     })
 
@@ -482,7 +510,9 @@ describe('featuresService', () => {
       const nativeFeature = featuresService['mapRemoteNativeFeatureToStaticFeature'](remoteFeature)
       featuresService['mapRemoteNativeFeatureToItem'] = jest.fn()
       featuresService.initializeFromDisk()
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', newRoles)
+      await featuresService.updateOnlineRoles(newRoles)
+      await featuresService.fetchFeatures('123')
+
       expect(featuresService['mapRemoteNativeFeatureToItem']).toHaveBeenCalledWith(
         nativeFeature,
         expect.anything(),
@@ -522,7 +552,8 @@ describe('featuresService', () => {
 
       sessionManager.isSignedIntoFirstPartyServer = jest.fn().mockReturnValue(true)
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.CoreUser, RoleName.NAMES.PlusUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.CoreUser, RoleName.NAMES.PlusUser])
+      await featuresService.fetchFeatures('123')
 
       expect(featuresService.getFeatureStatus(FeatureIdentifier.MidnightTheme)).toBe(FeatureStatus.Entitled)
       expect(featuresService.getFeatureStatus(FeatureIdentifier.SuperEditor)).toBe(FeatureStatus.Entitled)
@@ -554,7 +585,8 @@ describe('featuresService', () => {
 
       sessionManager.isSignedIntoFirstPartyServer = jest.fn().mockReturnValue(true)
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.CoreUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.CoreUser])
+      await featuresService.fetchFeatures('123')
 
       expect(featuresService.getFeatureStatus(FeatureIdentifier.MidnightTheme)).toBe(FeatureStatus.NoUserSubscription)
       expect(featuresService.getFeatureStatus(FeatureIdentifier.PlusEditor)).toBe(FeatureStatus.NoUserSubscription)
@@ -566,7 +598,8 @@ describe('featuresService', () => {
 
       sessionManager.isSignedIntoFirstPartyServer = jest.fn().mockReturnValue(false)
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.ProUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.ProUser])
+      await featuresService.fetchFeatures('123')
 
       expect(featuresService.getFeatureStatus(FeatureIdentifier.SuperEditor)).toBe(FeatureStatus.NotInCurrentPlan)
     })
@@ -615,7 +648,8 @@ describe('featuresService', () => {
         } as never),
       ])
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.CoreUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.CoreUser])
+      await featuresService.fetchFeatures('123')
 
       expect(featuresService.getFeatureStatus(themeFeature.identifier)).toBe(FeatureStatus.Entitled)
       expect(featuresService.getFeatureStatus(editorFeature.identifier)).toBe(FeatureStatus.InCurrentPlanButExpired)
@@ -627,7 +661,8 @@ describe('featuresService', () => {
     it('feature status should be not entitled if no account or offline repo', async () => {
       const featuresService = createService()
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.CoreUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.CoreUser])
+      await featuresService.fetchFeatures('123')
 
       sessionManager.isSignedIntoFirstPartyServer = jest.fn().mockReturnValue(false)
 
@@ -652,7 +687,8 @@ describe('featuresService', () => {
     it('feature status for offline subscription', async () => {
       const featuresService = createService()
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.CoreUser, RoleName.NAMES.PlusUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.CoreUser, RoleName.NAMES.PlusUser])
+      await featuresService.fetchFeatures('123')
 
       sessionManager.isSignedIntoFirstPartyServer = jest.fn().mockReturnValue(false)
       featuresService.onlineRolesIncludePaidSubscription = jest.fn().mockReturnValue(false)
@@ -680,7 +716,8 @@ describe('featuresService', () => {
         FeatureStatus.NoUserSubscription,
       )
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.CoreUser, RoleName.NAMES.PlusUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.CoreUser, RoleName.NAMES.PlusUser])
+      await featuresService.fetchFeatures('123')
 
       expect(featuresService.getFeatureStatus(FeatureIdentifier.DeprecatedFileSafe as FeatureIdentifier)).toBe(
         FeatureStatus.Entitled,
@@ -690,12 +727,14 @@ describe('featuresService', () => {
     it('has paid subscription', async () => {
       const featuresService = createService()
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.CoreUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.CoreUser])
+      await featuresService.fetchFeatures('123')
       sessionManager.isSignedIntoFirstPartyServer = jest.fn().mockReturnValue(true)
 
       expect(featuresService.hasPaidAnyPartyOnlineOrOfflineSubscription()).toBeFalsy
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.CoreUser, RoleName.NAMES.PlusUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.CoreUser, RoleName.NAMES.PlusUser])
+      await featuresService.fetchFeatures('123')
 
       expect(featuresService.hasPaidAnyPartyOnlineOrOfflineSubscription()).toEqual(true)
     })
@@ -703,7 +742,8 @@ describe('featuresService', () => {
     it('has paid subscription should be true if offline repo and signed into third party server', async () => {
       const featuresService = createService()
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.CoreUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.CoreUser])
+      await featuresService.fetchFeatures('123')
 
       featuresService.hasOfflineRepo = jest.fn().mockReturnValue(true)
       sessionManager.isSignedIntoFirstPartyServer = jest.fn().mockReturnValue(false)
@@ -725,7 +765,11 @@ describe('featuresService', () => {
 
       const featuresService = createService()
       await featuresService.migrateFeatureRepoToUserSetting([extensionRepoItem])
-      expect(settingsService.updateSetting).toHaveBeenCalledWith(SettingName.create(SettingName.NAMES.ExtensionKey).getValue(), extensionKey, true)
+      expect(settingsService.updateSetting).toHaveBeenCalledWith(
+        SettingName.create(SettingName.NAMES.ExtensionKey).getValue(),
+        extensionKey,
+        true,
+      )
     })
   })
 
@@ -789,7 +833,8 @@ describe('featuresService', () => {
     it('should be false if core user checks for plus role', async () => {
       const featuresService = createService()
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.CoreUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.CoreUser])
+      await featuresService.fetchFeatures('123')
 
       const hasPlusUserRole = featuresService.hasMinimumRole(RoleName.NAMES.PlusUser)
 
@@ -801,7 +846,8 @@ describe('featuresService', () => {
 
       sessionManager.isSignedIntoFirstPartyServer = jest.fn().mockReturnValue(true)
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.PlusUser, RoleName.NAMES.CoreUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.PlusUser, RoleName.NAMES.CoreUser])
+      await featuresService.fetchFeatures('123')
 
       const hasProUserRole = featuresService.hasMinimumRole(RoleName.NAMES.ProUser)
 
@@ -813,7 +859,8 @@ describe('featuresService', () => {
 
       sessionManager.isSignedIntoFirstPartyServer = jest.fn().mockReturnValue(true)
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.ProUser, RoleName.NAMES.PlusUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.ProUser, RoleName.NAMES.PlusUser])
+      await featuresService.fetchFeatures('123')
 
       const hasCoreUserRole = featuresService.hasMinimumRole(RoleName.NAMES.CoreUser)
 
@@ -825,7 +872,8 @@ describe('featuresService', () => {
 
       sessionManager.isSignedIntoFirstPartyServer = jest.fn().mockReturnValue(true)
 
-      await featuresService.updateOnlineRolesAndFetchFeatures('123', [RoleName.NAMES.ProUser, RoleName.NAMES.PlusUser])
+      await featuresService.updateOnlineRoles([RoleName.NAMES.ProUser, RoleName.NAMES.PlusUser])
+      await featuresService.fetchFeatures('123')
 
       const hasProUserRole = featuresService.hasMinimumRole(RoleName.NAMES.ProUser)
 

packages/snjs/lib/Services/Features/FeaturesService.ts

@@ -88,7 +88,8 @@ export class SNFeaturesService
         const {
           payload: { userUuid, currentRoles },
         } = data as UserRolesChangedEvent
-        await this.updateOnlineRolesAndFetchFeatures(userUuid, currentRoles)
+        await this.updateOnlineRoles(currentRoles)
+        await this.fetchFeatures(userUuid)
       }
     })
 
@@ -143,6 +144,9 @@ export class SNFeaturesService
         return
       }
 
+      const { userUuid, userRoles } = event.payload as MetaReceivedData
+      await this.updateOnlineRoles(userRoles.map((role) => role.name))
+
       /**
        * All user data must be downloaded before we map features. Otherwise, feature mapping
        * may think a component doesn't exist and create a new one, when in reality the component
@@ -152,11 +156,7 @@ export class SNFeaturesService
         return
       }
 
-      const { userUuid, userRoles } = event.payload as MetaReceivedData
+      await this.fetchFeatures(userUuid)
-      await this.updateOnlineRolesAndFetchFeatures(
-        userUuid,
-        userRoles.map((role) => role.name),
-      )
     }
   }
 
@@ -400,7 +400,7 @@ export class SNFeaturesService
     return hasFirstPartyOfflineSubscription || new URL(offlineRepo.content.offlineFeaturesUrl).hostname === 'localhost'
   }
 
-  async updateOnlineRolesAndFetchFeatures(userUuid: UuidString, roles: string[]): Promise<void> {
+  async updateOnlineRoles(roles: string[]): Promise<void> {
     const previousRoles = this.onlineRoles
 
     const userRolesChanged =
@@ -412,9 +412,21 @@ export class SNFeaturesService
       return
     }
 
-    this.needsInitialFeaturesUpdate = false
+    if (userRolesChanged && !isInitialLoadRolesChange) {
+      if (this.onlineRolesIncludePaidSubscription()) {
+        await this.notifyEvent(FeaturesEvent.DidPurchaseSubscription)
+      }
+    }
 
     await this.setOnlineRoles(roles)
+  }
+
+  async fetchFeatures(userUuid: UuidString): Promise<void> {
+    if (!this.needsInitialFeaturesUpdate) {
+      return
+    }
+
+    this.needsInitialFeaturesUpdate = false
 
     const shouldDownloadRoleBasedFeatures = !this.hasOfflineRepo()
 
@@ -426,12 +438,6 @@ export class SNFeaturesService
         await this.didDownloadFeatures(features)
       }
     }
-
-    if (userRolesChanged && !isInitialLoadRolesChange) {
-      if (this.onlineRolesIncludePaidSubscription()) {
-        await this.notifyEvent(FeaturesEvent.DidPurchaseSubscription)
-      }
-    }
   }
 
   async setOnlineRoles(roles: string[]): Promise<void> {