From 41a5b1415a9d2da07fdf7a5e924bc2dbd4b526e7 Mon Sep 17 00:00:00 2001 From: Aman Harwara Date: Wed, 5 Apr 2023 21:34:06 +0530 Subject: [PATCH] refactor: update roles immediately without waiting for full sync (#2294) --- .../Services/Features/FeaturesService.spec.ts | 116 +++++++++++++----- .../lib/Services/Features/FeaturesService.ts | 34 ++--- 2 files changed, 102 insertions(+), 48 deletions(-) diff --git a/packages/snjs/lib/Services/Features/FeaturesService.spec.ts b/packages/snjs/lib/Services/Features/FeaturesService.spec.ts index 12c32a2a8..43905acab 100644 --- a/packages/snjs/lib/Services/Features/FeaturesService.spec.ts +++ b/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.updateOnlineRolesAndFetchFeatures('123', roles) - await featuresService.updateOnlineRolesAndFetchFeatures('123', roles) - await featuresService.updateOnlineRolesAndFetchFeatures('123', roles) + await featuresService.updateOnlineRoles(roles) + await featuresService.fetchFeatures('123') + await featuresService.updateOnlineRoles(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) diff --git a/packages/snjs/lib/Services/Features/FeaturesService.ts b/packages/snjs/lib/Services/Features/FeaturesService.ts index aa419b9d0..6c38da600 100644 --- a/packages/snjs/lib/Services/Features/FeaturesService.ts +++ b/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.updateOnlineRolesAndFetchFeatures( - userUuid, - userRoles.map((role) => role.name), - ) + await this.fetchFeatures(userUuid) } } @@ -400,7 +400,7 @@ export class SNFeaturesService return hasFirstPartyOfflineSubscription || new URL(offlineRepo.content.offlineFeaturesUrl).hostname === 'localhost' } - async updateOnlineRolesAndFetchFeatures(userUuid: UuidString, roles: string[]): Promise { + async updateOnlineRoles(roles: string[]): Promise { 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 { + 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 {