From 891ecb798f287d209b3ebec9a58f46a88cea322b Mon Sep 17 00:00:00 2001 From: Milorad Filipovic Date: Tue, 4 Mar 2025 14:22:57 +0100 Subject: [PATCH 1/8] =?UTF-8?q?=E2=9C=85=20Adding=20RLC=20unit=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../ResourceLocator.test.constants.ts | 84 ++++++++ .../ResourceLocator/ResourceLocator.test.ts | 189 ++++++++++++++++++ 2 files changed, 273 insertions(+) create mode 100644 packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.test.constants.ts create mode 100644 packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.test.ts diff --git a/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.test.constants.ts b/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.test.constants.ts new file mode 100644 index 0000000000..57d161b752 --- /dev/null +++ b/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.test.constants.ts @@ -0,0 +1,84 @@ +import type { INode, INodeParameterResourceLocator, INodeProperties } from 'n8n-workflow'; + +export const TEST_MODEL_VALUE: INodeParameterResourceLocator = { + __rl: true, + value: 'test', + mode: 'list', + cachedResultName: 'table', + cachedResultUrl: 'https://test.com/test', +}; + +export const TEST_PARAMETER_MULTI_MODE: INodeProperties = { + displayName: 'Test Parameter', + name: 'testParamMultiMode', + type: 'resourceLocator', + default: { mode: 'list', value: '' }, + required: true, + modes: [ + { + displayName: 'From List', + name: 'list', + type: 'list', + typeOptions: { searchListMethod: 'testSearch', searchable: true }, + }, + { + displayName: 'By URL', + name: 'url', + type: 'string', + placeholder: 'https://test.com/test', + }, + { + displayName: 'ID', + name: 'id', + type: 'string', + placeholder: 'id', + }, + ], +}; + +export const TEST_PARAMETER_SINGLE_MODE: INodeProperties = { + ...TEST_PARAMETER_MULTI_MODE, + name: 'testParameterSingleMode', + modes: [ + { + displayName: 'From List', + name: 'list', + type: 'list', + typeOptions: { searchListMethod: 'testSearch', searchable: true }, + }, + ], +}; + +export const TEST_NODE_MULTI_MODE: INode = { + type: 'n8n-nodes-base.airtable', + typeVersion: 2.1, + position: [80, -260], + id: '377e4287-b1e0-44cc-ba0f-7bb3d676d60c', + name: 'Test Node - Multi Mode', + parameters: { + authentication: 'testAuth', + resource: 'test', + operation: 'get', + testParamMultiMode: TEST_MODEL_VALUE, + id: '', + options: {}, + }, + credentials: { + testAuth: { + id: '1234', + name: 'Test Account', + }, + }, +}; + +export const TEST_NODE_SINGLE_MODE: INode = { + ...TEST_NODE_MULTI_MODE, + parameters: { + authentication: 'testAuth', + resource: 'test', + operation: 'get', + testParameterSingleMode: TEST_MODEL_VALUE, + id: '', + options: {}, + }, +}; diff --git a/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.test.ts b/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.test.ts new file mode 100644 index 0000000000..c8f5a8de01 --- /dev/null +++ b/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.test.ts @@ -0,0 +1,189 @@ +import { createComponentRenderer } from '@/__tests__/render'; +import { useNodeTypesStore } from '@/stores/nodeTypes.store'; +import ResourceLocator from './ResourceLocator.vue'; +import { createTestingPinia } from '@pinia/testing'; +import userEvent from '@testing-library/user-event'; +import { screen, waitFor } from '@testing-library/vue'; +import { mockedStore } from '@/__tests__/utils'; +import { + TEST_MODEL_VALUE, + TEST_NODE_MULTI_MODE, + TEST_NODE_SINGLE_MODE, + TEST_PARAMETER_MULTI_MODE, + TEST_PARAMETER_SINGLE_MODE, +} from './ResourceLocator.test.constants'; + +vi.mock('vue-router', async () => { + const actual = await vi.importActual('vue-router'); + const params = {}; + const location = {}; + return { + ...actual, + useRouter: () => ({ + push: vi.fn(), + }), + useRoute: () => ({ + params, + location, + }), + }; +}); +vi.mock('@/composables/useWorkflowHelpers', () => { + return { + useWorkflowHelpers: vi.fn(() => ({ + resolveExpression: vi.fn().mockImplementation((val) => val), + resolveRequiredParameters: vi.fn().mockImplementation((_, params) => params), + })), + }; +}); +vi.mock('@/composables/useTelemetry', () => ({ + useTelemetry: () => ({ track: vi.fn() }), +})); + +let nodeTypesStore: ReturnType>; + +const renderComponent = createComponentRenderer(ResourceLocator, { + props: { + modelValue: TEST_MODEL_VALUE, + parameter: TEST_PARAMETER_MULTI_MODE, + path: `parameters.${TEST_PARAMETER_MULTI_MODE.name}`, + node: TEST_NODE_MULTI_MODE, + displayTitle: 'Test Resource Locator', + expressionComputedValue: '', + isValueExpression: false, + }, + global: { + stubs: { + ResourceLocatorDropdown: false, + ExpressionParameterInput: true, + ParameterIssues: true, + N8nCallout: true, + 'font-awesome-icon': true, + FromAiOverrideField: true, + FromAiOverrideButton: true, + ParameterOverrideSelectableList: true, + }, + }, +}); + +describe('ResourceLocator', () => { + beforeEach(() => { + createTestingPinia(); + nodeTypesStore = mockedStore(useNodeTypesStore); + nodeTypesStore.getNodeType = vi.fn().mockReturnValue({ displayName: 'Test Node' }); + }); + afterEach(() => { + vi.clearAllMocks(); + }); + + it('renders multi-mode correctly', async () => { + const { getByTestId } = renderComponent(); + expect(getByTestId(`resource-locator-${TEST_PARAMETER_MULTI_MODE.name}`)).toBeInTheDocument(); + // Should render mode selector with all available modes + expect(getByTestId('rlc-mode-selector')).toBeInTheDocument(); + await userEvent.click(getByTestId('rlc-mode-selector')); + TEST_PARAMETER_MULTI_MODE.modes?.forEach((mode) => { + expect(screen.getByTestId(`mode-${mode.name}`)).toBeInTheDocument(); + }); + }); + + it('renders single mode correctly', async () => { + const { getByTestId, queryByTestId } = renderComponent({ + props: { + modelValue: TEST_MODEL_VALUE, + parameter: TEST_PARAMETER_SINGLE_MODE, + path: `parameters.${TEST_PARAMETER_SINGLE_MODE.name}`, + node: TEST_NODE_SINGLE_MODE, + displayTitle: 'Test Resource Locator', + expressionComputedValue: '', + }, + }); + expect(getByTestId(`resource-locator-${TEST_PARAMETER_SINGLE_MODE.name}`)).toBeInTheDocument(); + // Should not render mode selector + expect(queryByTestId('rlc-mode-selector')).not.toBeInTheDocument(); + }); + + it('renders fetched resources correctly', async () => { + const TEST_ITEMS = [ + { name: 'Test Resource', value: 'test-resource', url: 'https://test.com/test-resource' }, + { + name: 'Test Resource 2', + value: 'test-resource-2', + url: 'https://test.com/test-resource-2', + }, + ]; + nodeTypesStore.getResourceLocatorResults.mockResolvedValue({ + results: TEST_ITEMS, + paginationToken: null, + }); + const { getByTestId, getByText, getAllByTestId } = renderComponent(); + + expect(getByTestId(`resource-locator-${TEST_PARAMETER_MULTI_MODE.name}`)).toBeInTheDocument(); + // Click on the input to fetch resources + await userEvent.click(getByTestId('rlc-input')); + // Wait for the resources to be fetched + await waitFor(() => { + expect(nodeTypesStore.getResourceLocatorResults).toHaveBeenCalled(); + }); + // Expect the items to be rendered + expect(getAllByTestId('rlc-item')).toHaveLength(TEST_ITEMS.length); + // We should be getting one item for each result + TEST_ITEMS.forEach((item) => { + expect(getByText(item.name)).toBeInTheDocument(); + }); + }); + + it('renders permission error correctly', async () => { + const TEST_401_ERROR = { + message: 'Failed to load resources', + httpCode: '401', + description: 'Authentication failed. Please check your credentials.', + }; + + nodeTypesStore.getResourceLocatorResults.mockRejectedValue(TEST_401_ERROR); + + const { getByTestId, findByTestId } = renderComponent(); + + expect(getByTestId(`resource-locator-${TEST_PARAMETER_MULTI_MODE.name}`)).toBeInTheDocument(); + + await userEvent.click(getByTestId('rlc-input')); + await waitFor(() => { + expect(nodeTypesStore.getResourceLocatorResults).toHaveBeenCalled(); + }); + + const errorContainer = await findByTestId('rlc-error-container'); + expect(errorContainer).toBeInTheDocument(); + + expect(errorContainer).toHaveTextContent(TEST_401_ERROR.httpCode); + expect(errorContainer).toHaveTextContent(TEST_401_ERROR.message); + + expect(getByTestId('permission-error-link')).toBeInTheDocument(); + }); + + it('renders generic error correctly', async () => { + const TEST_500_ERROR = { + message: 'Whoops', + httpCode: '500', + description: 'Something went wrong. Please try again later.', + }; + + nodeTypesStore.getResourceLocatorResults.mockRejectedValue(TEST_500_ERROR); + const { getByTestId, findByTestId, queryByTestId } = renderComponent(); + + expect(getByTestId(`resource-locator-${TEST_PARAMETER_MULTI_MODE.name}`)).toBeInTheDocument(); + + await userEvent.click(getByTestId('rlc-input')); + + await waitFor(() => { + expect(nodeTypesStore.getResourceLocatorResults).toHaveBeenCalled(); + }); + + const errorContainer = await findByTestId('rlc-error-container'); + expect(errorContainer).toBeInTheDocument(); + + expect(errorContainer).toHaveTextContent(TEST_500_ERROR.httpCode); + expect(errorContainer).toHaveTextContent(TEST_500_ERROR.message); + + expect(queryByTestId('permission-error-link')).not.toBeInTheDocument(); + }); +}); From 8c6a5eed8e19a11393f705ee77ee36c5640b7d0f Mon Sep 17 00:00:00 2001 From: Milorad Filipovic Date: Tue, 4 Mar 2025 14:54:21 +0100 Subject: [PATCH 2/8] =?UTF-8?q?=E2=9A=A1Implemented=20error=20notice?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../ResourceLocator/ResourceLocator.vue | 64 +++++++++++++------ .../ResourceLocator/resourceLocator.scss | 7 +- .../src/plugins/i18n/locales/en.json | 5 +- 3 files changed, 53 insertions(+), 23 deletions(-) diff --git a/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.vue b/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.vue index 649272a12a..779897efd2 100644 --- a/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.vue +++ b/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.vue @@ -53,11 +53,17 @@ import { updateFromAIOverrideValues, type FromAIOverride, } from '../../utils/fromAIOverrideUtils'; +import { N8nNotice } from '@n8n/design-system'; interface IResourceLocatorQuery { results: INodeListSearchItems[]; nextPageToken: unknown; error: boolean; + errorDetails?: { + message?: string; + description?: string; + httpCode?: string; + }; loading: boolean; } @@ -148,13 +154,9 @@ const selectedMode = computed(() => { const isListMode = computed(() => selectedMode.value === 'list'); -const hasCredential = computed(() => { - const node = ndvStore.activeNode; - if (!node) { - return false; - } - return !!(node?.credentials && Object.keys(node.credentials).length === 1); -}); +const hasPermissionError = computed(() => + ['401', '403'].includes(currentResponse.value?.errorDetails?.httpCode ?? ''), +); const credentialsNotSet = computed(() => { if (!props.node) return false; @@ -643,6 +645,11 @@ async function loadResources() { setResponse(paramsKey, { loading: false, error: true, + errorDetails: { + message: e.message, + description: e.description, + httpCode: e.httpCode, + }, }); } } @@ -777,19 +784,37 @@ function removeOverride() { @load-more="loadResourcesDebounced" >
Date: Tue, 4 Mar 2025 17:11:01 +0100 Subject: [PATCH 3/8] =?UTF-8?q?=F0=9F=91=8C=20Addressing=20latest=20design?= =?UTF-8?q?=20feedback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/components/ResourceLocator/ResourceLocator.vue | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.vue b/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.vue index 779897efd2..73e031e52e 100644 --- a/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.vue +++ b/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.vue @@ -789,10 +789,12 @@ function removeOverride() { :class="$style.error" data-test-id="rlc-error-container" > - - {{ i18n.baseText('resourceLocator.mode.list.error.title') }} - + + {{ i18n.baseText('resourceLocator.mode.list.error.title') }} + + - {{ currentResponse.errorDetails.httpCode }} + {{ currentResponse.errorDetails.httpCode }} - {{ currentResponse.errorDetails.message?.split('-')[0]?.trim() }} From 2e41075fc88a3a7cb17c03aa97cba4eb93c1c983 Mon Sep 17 00:00:00 2001 From: Milorad Filipovic Date: Wed, 5 Mar 2025 11:22:25 +0100 Subject: [PATCH 4/8] =?UTF-8?q?=E2=9A=A1Improve=20auth=20error=20detection?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../ResourceLocator/ResourceLocator.vue | 67 ++++++++++++------- .../ResourceLocator/resourceLocator.scss | 20 ++++-- 2 files changed, 60 insertions(+), 27 deletions(-) diff --git a/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.vue b/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.vue index 73e031e52e..3275938759 100644 --- a/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.vue +++ b/packages/frontend/editor-ui/src/components/ResourceLocator/ResourceLocator.vue @@ -63,6 +63,7 @@ interface IResourceLocatorQuery { message?: string; description?: string; httpCode?: string; + stackTrace?: string; }; loading: boolean; } @@ -154,9 +155,26 @@ const selectedMode = computed(() => { const isListMode = computed(() => selectedMode.value === 'list'); -const hasPermissionError = computed(() => - ['401', '403'].includes(currentResponse.value?.errorDetails?.httpCode ?? ''), -); +const hasCredentialError = computed(() => { + const PERMISSION_ERROR_CODES = ['401', '403']; + // Some of our NodeAPIErrors just return 500 without any details, + // so checking messages and stack traces for permission errors + const NODE_API_AUTH_ERROR_MESSAGES = [ + 'NodeApiError: Authorization failed', + 'NodeApiError: Unable to sign without access token', + 'secretOrPrivateKey must be an asymmetric key when using RS256', + ]; + // Check if error stack trace contains permission error messages + const stackTraceContainsCredentialError = (currentResponse.value?.errorDetails?.stackTrace ?? '') + .split('\n') + .some((line) => NODE_API_AUTH_ERROR_MESSAGES.includes(line.trim())); + + return ( + PERMISSION_ERROR_CODES.includes(currentResponse.value?.errorDetails?.httpCode ?? '') || + NODE_API_AUTH_ERROR_MESSAGES.includes(currentResponse.value?.errorDetails?.message ?? '') || + stackTraceContainsCredentialError + ); +}); const credentialsNotSet = computed(() => { if (!props.node) return false; @@ -784,28 +802,31 @@ function removeOverride() { @load-more="loadResourcesDebounced" >