From 228d95a2f8f01b4692f02adb3bcd334d11b76053 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milorad=20FIlipovi=C4=87?= Date: Fri, 22 Dec 2023 08:41:20 +0100 Subject: [PATCH] fix(editor): Fix operation change failing in certain conditions (#8114) ## Summary This PR handles the case when there are multiple parameters with the same name but different `options` and `displayOptions`. In this case, if one of such fields is set, changing the dependent parameter value so the other should be shown causes an error in case their options are not compatible (this [check](https://github.com/n8n-io/n8n/blob/7806a65229878a473f5526bad0b94614e8bfa8aa/packages/workflow/src/NodeHelpers.ts#L786)). #### Example: LDAP node has two `options` properties with the same name: 1. `attributes` with predefined options (`add`, `replace`, `delete`). Shown when **Update** operation is selected 2. `attributes` with a collection of `attribute` objects. Shows for the **Create** operation Setting one of these parameter values and switching operation so the other is shown breaks the app. This PR checks if there is a value saved for such parameter and removes it before calling `getNodeParameters` in `valueChanged` handler. ## Related tickets and issues Fixes ADO-1589 ## Review / Merge checklist - [x] PR title and summary are descriptive. **Remember, the title automatically goes into the changelog. Use `(no-changelog)` otherwise.** ([conventions](https://github.com/n8n-io/n8n/blob/master/.github/pull_request_title_conventions.md)) - [ ] [Docs updated](https://github.com/n8n-io/n8n-docs) or follow-up ticket created. - [x] Tests included. > A bug is not considered fixed, unless a test is added to prevent it from happening again. > A feature is not complete without tests. --- cypress/e2e/5-ndv.cy.ts | 15 +++++++ cypress/pages/ndv.ts | 6 ++- .../editor-ui/src/components/NodeSettings.vue | 44 +++++++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/cypress/e2e/5-ndv.cy.ts b/cypress/e2e/5-ndv.cy.ts index b7711b36e81d5..8089a1240733a 100644 --- a/cypress/e2e/5-ndv.cy.ts +++ b/cypress/e2e/5-ndv.cy.ts @@ -490,4 +490,19 @@ describe('NDV', () => { ndv.getters.nodeVersion().should('have.text', 'Function node version 1 (Deprecated)'); ndv.actions.close(); }); + it('Should handle mismatched option attributes', () => { + workflowPage.actions.addInitialNodeToCanvas('LDAP', { keepNdvOpen: true, action: 'Create a new entry' }); + // Add some attributes in Create operation + cy.getByTestId('parameter-item').contains('Add Attributes').click(); + ndv.actions.changeNodeOperation('Update'); + // Attributes should be empty after operation change + cy.getByTestId('parameter-item').contains('Currently no items exist').should('exist'); + }); + it('Should keep RLC values after operation change', () => { + const TEST_DOC_ID = '1111'; + workflowPage.actions.addInitialNodeToCanvas('Google Sheets', { keepNdvOpen: true, action: 'Append row in sheet' }); + ndv.actions.setRLCValue('documentId', TEST_DOC_ID); + ndv.actions.changeNodeOperation('Update Row'); + ndv.getters.resourceLocatorInput('documentId').find('input').should('have.value', TEST_DOC_ID); + }); }); diff --git a/cypress/pages/ndv.ts b/cypress/pages/ndv.ts index 0eaa1361cfa82..8d8f5297e9e81 100644 --- a/cypress/pages/ndv.ts +++ b/cypress/pages/ndv.ts @@ -246,10 +246,14 @@ export class NDV extends BasePage { }); this.actions.validateExpressionPreview(fieldName, `node doesn't exist`); }, - openSettings: () => { this.getters.nodeSettingsTab().click(); }, + changeNodeOperation: (operation: string) => { + this.getters.parameterInput('operation').click(); + cy.get('.el-select-dropdown__item').contains(new RegExp(`^${operation}$`)).click({ force: true }); + this.getters.parameterInput('operation').find('input').should('have.value', operation); + }, }; } diff --git a/packages/editor-ui/src/components/NodeSettings.vue b/packages/editor-ui/src/components/NodeSettings.vue index b1d70fb972ec2..7c95ba1805d66 100644 --- a/packages/editor-ui/src/components/NodeSettings.vue +++ b/packages/editor-ui/src/components/NodeSettings.vue @@ -864,6 +864,12 @@ export default defineComponent({ } else { set(nodeParameters as object, parameterPath, newValue); } + // If value is updated, remove parameter values that have invalid options + // so getNodeParameters checks don't fail + this.removeMismatchedOptionValues(nodeType, nodeParameters, { + name: parameterPath, + value: newValue, + }); } // Get the parameters with the now new defaults according to the @@ -919,6 +925,44 @@ export default defineComponent({ this.workflowsStore.setNodeValue(updateInformation); } }, + /** + * Removes node values that are not valid options for the given parameter. + * This can happen when there are multiple node parameters with the same name + * but different options and display conditions + * @param nodeType The node type description + * @param nodeParameterValues Current node parameter values + * @param updatedParameter The parameter that was updated. Will be used to determine which parameters to remove based on their display conditions and option values + */ + removeMismatchedOptionValues( + nodeType: INodeTypeDescription, + nodeParameterValues: INodeParameters | null, + updatedParameter: { name: string; value: NodeParameterValue }, + ) { + nodeType.properties.forEach((prop) => { + const displayOptions = prop.displayOptions; + // Not processing parameters that are not set or don't have options + if (!nodeParameterValues?.hasOwnProperty(prop.name) || !displayOptions || !prop.options) { + return; + } + // Only process the parameters that should be hidden + const showCondition = displayOptions.show?.[updatedParameter.name]; + const hideCondition = displayOptions.hide?.[updatedParameter.name]; + if (showCondition === undefined && hideCondition === undefined) { + return; + } + // Every value should be a possible option + const hasValidOptions = Object.keys(nodeParameterValues).every( + (key) => (prop.options ?? []).find((option) => option.name === key) !== undefined, + ); + if ( + !hasValidOptions || + showCondition !== updatedParameter.value || + hideCondition === updatedParameter.value + ) { + unset(nodeParameterValues as object, prop.name); + } + }); + }, /** * Sets the values of the active node in the internal settings variables */