From 8b99384367161a47b3de13b7e83bcf6d07e3bf19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milorad=20FIlipovi=C4=87?= Date: Wed, 29 Nov 2023 10:51:15 +0100 Subject: [PATCH] fix(editor): Fix cloud plan data loading on instance (#7841) Moving cloud hooks and store initialization logic after users are authenticated. This will ensure user local account is available when their cloud plan data is being fetched. This PR also adds the following error handling improvements: - Added error handling to the same initialization logic - Fixed empty `catch` clauses inside the cloud store which caused it to silently fail and complicated debugging of this bug --- packages/editor-ui/src/__tests__/init.test.ts | 77 +++++++++++++++++++ packages/editor-ui/src/hooks/register.ts | 11 ++- packages/editor-ui/src/init.ts | 17 +++- .../editor-ui/src/stores/__tests__/ui.test.ts | 9 +++ .../stores/__tests__/utils/cloudStoreUtils.ts | 2 +- .../editor-ui/src/stores/cloudPlan.store.ts | 25 ++++-- packages/editor-ui/src/views/SigninView.vue | 8 +- 7 files changed, 136 insertions(+), 13 deletions(-) create mode 100644 packages/editor-ui/src/__tests__/init.test.ts diff --git a/packages/editor-ui/src/__tests__/init.test.ts b/packages/editor-ui/src/__tests__/init.test.ts new file mode 100644 index 0000000000..02cf81bced --- /dev/null +++ b/packages/editor-ui/src/__tests__/init.test.ts @@ -0,0 +1,77 @@ +import { useUsersStore } from '@/stores/users.store'; +import { useCloudPlanStore } from '@/stores/cloudPlan.store'; +import { useSourceControlStore } from '@/stores/sourceControl.store'; +import { useNodeTypesStore } from '@/stores/nodeTypes.store'; +import { useRootStore } from '@/stores/n8nRoot.store'; +import { initializeAuthenticatedFeatures } from '@/init'; +import type { SpyInstance } from 'vitest'; +import { createTestingPinia } from '@pinia/testing'; +import { setActivePinia } from 'pinia'; +import { useSettingsStore } from '@/stores/settings.store'; + +vi.mock('@/stores/users.store', () => ({ + useUsersStore: vi.fn(), +})); + +vi.mock('@/stores/n8nRoot.store', () => ({ + useRootStore: vi.fn(), +})); + +describe('Init', () => { + describe('Authenticated Features', () => { + let settingsStore: ReturnType; + let cloudPlanStore: ReturnType; + let sourceControlStore: ReturnType; + let nodeTypesStore: ReturnType; + let cloudStoreSpy: SpyInstance<[], Promise>; + let templatesTestSpy: SpyInstance<[], Promise>; + let sourceControlSpy: SpyInstance<[], Promise>; + let nodeTranslationSpy: SpyInstance<[], Promise>; + + beforeAll(() => { + setActivePinia(createTestingPinia()); + settingsStore = useSettingsStore(); + cloudPlanStore = useCloudPlanStore(); + sourceControlStore = useSourceControlStore(); + nodeTypesStore = useNodeTypesStore(); + vi.spyOn(settingsStore, 'isCloudDeployment', 'get').mockReturnValue(true); + vi.spyOn(settingsStore, 'isTemplatesEnabled', 'get').mockReturnValue(true); + vi.spyOn(sourceControlStore, 'isEnterpriseSourceControlEnabled', 'get').mockReturnValue(true); + vi.mocked(useRootStore).mockReturnValue({ defaultLocale: 'es' } as ReturnType< + typeof useRootStore + >); + vi.mock('@/hooks/register', () => ({ + initializeCloudHooks: vi.fn(), + })); + cloudStoreSpy = vi.spyOn(cloudPlanStore, 'initialize'); + templatesTestSpy = vi.spyOn(settingsStore, 'testTemplatesEndpoint'); + sourceControlSpy = vi.spyOn(sourceControlStore, 'getPreferences'); + nodeTranslationSpy = vi.spyOn(nodeTypesStore, 'getNodeTranslationHeaders'); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('should not init authenticated features if user is not logged in', async () => { + vi.mocked(useUsersStore).mockReturnValue({ currentUser: null } as ReturnType< + typeof useUsersStore + >); + await initializeAuthenticatedFeatures(); + expect(cloudStoreSpy).not.toHaveBeenCalled(); + expect(templatesTestSpy).not.toHaveBeenCalled(); + expect(sourceControlSpy).not.toHaveBeenCalled(); + expect(nodeTranslationSpy).not.toHaveBeenCalled(); + }); + it('should init authenticated features if user is not logged in', async () => { + vi.mocked(useUsersStore).mockReturnValue({ currentUser: { id: '123' } } as ReturnType< + typeof useUsersStore + >); + await initializeAuthenticatedFeatures(); + expect(cloudStoreSpy).toHaveBeenCalled(); + expect(templatesTestSpy).toHaveBeenCalled(); + expect(sourceControlSpy).toHaveBeenCalled(); + expect(nodeTranslationSpy).toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/editor-ui/src/hooks/register.ts b/packages/editor-ui/src/hooks/register.ts index fb937c61a9..7d15486363 100644 --- a/packages/editor-ui/src/hooks/register.ts +++ b/packages/editor-ui/src/hooks/register.ts @@ -6,7 +6,12 @@ export async function initializeCloudHooks() { return; } - const { n8nCloudHooks } = await import('@/hooks/cloud'); - extendExternalHooks(n8nCloudHooks); - cloudHooksInitialized = true; + try { + const { n8nCloudHooks } = await import('@/hooks/cloud'); + extendExternalHooks(n8nCloudHooks); + } catch (error) { + throw new Error(`Failed to extend external hooks: ${error.message}`); + } finally { + cloudHooksInitialized = true; + } } diff --git a/packages/editor-ui/src/init.ts b/packages/editor-ui/src/init.ts index bc30972e4f..b4495f9969 100644 --- a/packages/editor-ui/src/init.ts +++ b/packages/editor-ui/src/init.ts @@ -19,13 +19,17 @@ export async function initializeCore() { } const settingsStore = useSettingsStore(); - const cloudPlanStore = useCloudPlanStore(); const usersStore = useUsersStore(); await settingsStore.initialize(); await usersStore.initialize(); + if (settingsStore.isCloudDeployment) { - await Promise.all([cloudPlanStore.initialize(), initializeCloudHooks()]); + try { + await initializeCloudHooks(); + } catch (e) { + console.error('Failed to initialize cloud hooks:', e); + } } coreInitialized = true; @@ -48,6 +52,7 @@ export async function initializeAuthenticatedFeatures() { const settingsStore = useSettingsStore(); const rootStore = useRootStore(); const nodeTypesStore = useNodeTypesStore(); + const cloudPlanStore = useCloudPlanStore(); if (sourceControlStore.isEnterpriseSourceControlEnabled) { await sourceControlStore.getPreferences(); @@ -63,5 +68,13 @@ export async function initializeAuthenticatedFeatures() { await nodeTypesStore.getNodeTranslationHeaders(); } + if (settingsStore.isCloudDeployment) { + try { + await cloudPlanStore.initialize(); + } catch (e) { + console.error('Failed to initialize cloud plan store:', e); + } + } + authenticatedFeaturesInitialized = true; } diff --git a/packages/editor-ui/src/stores/__tests__/ui.test.ts b/packages/editor-ui/src/stores/__tests__/ui.test.ts index 996e67057c..39ff24cf76 100644 --- a/packages/editor-ui/src/stores/__tests__/ui.test.ts +++ b/packages/editor-ui/src/stores/__tests__/ui.test.ts @@ -165,11 +165,15 @@ describe('UI store', () => { const fetchUserCloudAccountSpy = vi .spyOn(cloudPlanApi, 'getCloudUserInfo') .mockResolvedValue(getUserCloudInfo(true)); + const getCurrentUsageSpy = vi + .spyOn(cloudPlanApi, 'getCurrentUsage') + .mockResolvedValue({ executions: 1000, activeWorkflows: 100 }); setupOwnerAndCloudDeployment(); await cloudPlanStore.checkForCloudPlanData(); await cloudPlanStore.fetchUserCloudAccount(); expect(fetchCloudSpy).toHaveBeenCalled(); expect(fetchUserCloudAccountSpy).toHaveBeenCalled(); + expect(getCurrentUsageSpy).toHaveBeenCalled(); expect(uiStore.bannerStack).toContain('TRIAL'); // There should be no email confirmation banner for trialing users expect(uiStore.bannerStack).not.toContain('EMAIL_CONFIRMATION'); @@ -183,10 +187,15 @@ describe('UI store', () => { .spyOn(cloudPlanApi, 'getCloudUserInfo') .mockResolvedValue(getUserCloudInfo(true)); setupOwnerAndCloudDeployment(); + const getCurrentUsageSpy = vi + .spyOn(cloudPlanApi, 'getCurrentUsage') + .mockResolvedValue({ executions: 1000, activeWorkflows: 100 }); + setupOwnerAndCloudDeployment(); await cloudPlanStore.checkForCloudPlanData(); await cloudPlanStore.fetchUserCloudAccount(); expect(fetchCloudSpy).toHaveBeenCalled(); expect(fetchUserCloudAccountSpy).toHaveBeenCalled(); + expect(getCurrentUsageSpy).toHaveBeenCalled(); expect(uiStore.bannerStack).toContain('TRIAL_OVER'); // There should be no email confirmation banner for trialing users expect(uiStore.bannerStack).not.toContain('EMAIL_CONFIRMATION'); diff --git a/packages/editor-ui/src/stores/__tests__/utils/cloudStoreUtils.ts b/packages/editor-ui/src/stores/__tests__/utils/cloudStoreUtils.ts index 35010b2898..a74e7db78e 100644 --- a/packages/editor-ui/src/stores/__tests__/utils/cloudStoreUtils.ts +++ b/packages/editor-ui/src/stores/__tests__/utils/cloudStoreUtils.ts @@ -10,7 +10,7 @@ function getUserPlanData(trialExpirationDate: Date, isTrial = true): Cloud.PlanD isActive: true, displayName: 'Trial', metadata: { - group: isTrial ? 'trial' : 'pro', + group: isTrial ? 'trial' : 'opt-in', slug: 'trial-1', trial: { gracePeriod: 3, diff --git a/packages/editor-ui/src/stores/cloudPlan.store.ts b/packages/editor-ui/src/stores/cloudPlan.store.ts index 1d89ed6556..2278ad4116 100644 --- a/packages/editor-ui/src/stores/cloudPlan.store.ts +++ b/packages/editor-ui/src/stores/cloudPlan.store.ts @@ -55,7 +55,7 @@ export const useCloudPlanStore = defineStore(STORES.CLOUD_PLAN, () => { const hasCloudPlan = computed(() => { const cloudUserId = settingsStore.settings.n8nMetadata?.userId; - return usersStore.currentUser?.isOwner && settingsStore.isCloudDeployment && cloudUserId; + return usersStore.isInstanceOwner && settingsStore.isCloudDeployment && cloudUserId; }); const getUserCloudAccount = async () => { @@ -68,7 +68,7 @@ export const useCloudPlanStore = defineStore(STORES.CLOUD_PLAN, () => { } } } catch (error) { - throw new Error(error); + throw new Error(error.message); } }; @@ -143,13 +143,17 @@ export const useCloudPlanStore = defineStore(STORES.CLOUD_PLAN, () => { if (!userIsTrialing.value) return; await getInstanceCurrentUsage(); startPollingInstanceUsageData(); - } catch {} + } catch (e) { + throw new Error(e.message); + } }; const fetchUserCloudAccount = async () => { try { await getUserCloudAccount(); - } catch {} + } catch (e) { + throw new Error(e.message); + } }; const redirectToDashboard = async () => { @@ -163,8 +167,17 @@ export const useCloudPlanStore = defineStore(STORES.CLOUD_PLAN, () => { return; } - await checkForCloudPlanData(); - await fetchUserCloudAccount(); + try { + await checkForCloudPlanData(); + } catch (error) { + console.warn('Error checking for cloud plan data:', error); + } + + try { + await fetchUserCloudAccount(); + } catch (error) { + console.warn('Error fetching user cloud account:', error); + } state.initialized = true; }; diff --git a/packages/editor-ui/src/views/SigninView.vue b/packages/editor-ui/src/views/SigninView.vue index c30f930088..5d6e0406ca 100644 --- a/packages/editor-ui/src/views/SigninView.vue +++ b/packages/editor-ui/src/views/SigninView.vue @@ -126,7 +126,13 @@ export default defineComponent({ mfaRecoveryCode: form.recoveryCode, }); this.loading = false; - await this.cloudPlanStore.checkForCloudPlanData(); + if (this.settingsStore.isCloudDeployment) { + try { + await this.cloudPlanStore.checkForCloudPlanData(); + } catch (error) { + console.warn('Failed to check for cloud plan data', error); + } + } await this.settingsStore.getSettings(); this.clearAllStickyNotifications(); this.checkRecoveryCodesLeft();