Skip to content

Commit

Permalink
fix(editor): Fix operation change failing in certain conditions (#8114)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
MiloradFilipovic authored and ivov committed Dec 27, 2023
1 parent fc2f528 commit 228d95a
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 1 deletion.
15 changes: 15 additions & 0 deletions cypress/e2e/5-ndv.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
6 changes: 5 additions & 1 deletion cypress/pages/ndv.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
},
};
}

Expand Down
44 changes: 44 additions & 0 deletions packages/editor-ui/src/components/NodeSettings.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
*/
Expand Down

0 comments on commit 228d95a

Please sign in to comment.