Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(editor): Fix empty node name handling #9548

Merged
merged 4 commits into from
May 30, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions cypress/e2e/12-canvas.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,17 @@ describe('Canvas Node Manipulation and Navigation', () => {
WorkflowPage.getters.canvasNodeByName(RENAME_NODE_NAME2).should('exist');
});

it('should not allow empty strings for node names', () => {
WorkflowPage.actions.addNodeToCanvas(MANUAL_TRIGGER_NODE_NAME);
WorkflowPage.actions.addNodeToCanvas(CODE_NODE_NAME);
WorkflowPage.getters.canvasNodes().last().click();
cy.get('body').trigger('keydown', { key: 'F2' });
cy.get('.rename-prompt').should('be.visible');
cy.get('body').type('{backspace}');
cy.get('body').type('{enter}');
cy.get('.rename-prompt').should('contain', 'Invalid Name');
});

it('should duplicate nodes (context menu or shortcut)', () => {
WorkflowPage.actions.addNodeToCanvas(MANUAL_TRIGGER_NODE_NAME);
WorkflowPage.getters.canvasNodeByName(MANUAL_TRIGGER_NODE_DISPLAY_NAME).click();
Expand Down
13 changes: 13 additions & 0 deletions cypress/e2e/7-workflow-actions.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,19 @@ describe('Workflow Actions', () => {
});
});

it('should allow importing nodes without names', () => {
cy.fixture('Test_workflow-actions_import_nodes_empty_name.json').then((data) => {
cy.get('body').paste(JSON.stringify(data));
WorkflowPage.actions.zoomToFit();
WorkflowPage.getters.canvasNodes().should('have.length', 3);
WorkflowPage.getters.nodeConnections().should('have.length', 2);
// Check if all nodes have names
WorkflowPage.getters.canvasNodes().each((node) => {
cy.wrap(node).should('have.attr', 'data-name');
});
});
});

it('should update workflow settings', () => {
cy.visit(WorkflowPages.url);
WorkflowPages.getters.workflowCards().then((cards) => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
{
"meta": {
"instanceId": "4a31be0d29cfa6246ba62b359030d712af57b98c5dfe6a7ee8beee0a46c5b5a4"
},
"nodes": [
{
"parameters": {
"operation": "get"
},
"id": "5b084875-bd5e-4731-9591-18d2c8996945",
"name": "",
"type": "n8n-nodes-base.gmail",
"typeVersion": 2.1,
"position": [
900,
460
]
},
{
"parameters": {},
"id": "449ab540-d9d7-480d-b131-05e9989a69cd",
"name": "When clicking \"Test workflow\"",
"type": "n8n-nodes-base.manualTrigger",
"typeVersion": 1,
"position": [
460,
460
]
},
{
"parameters": {
"operation": "get"
},
"id": "3a791321-6f0c-4f92-91e5-20e1be0d4964",
"name": "Gmail",
"type": "n8n-nodes-base.gmail",
"typeVersion": 2.1,
"position": [
680,
460
]
}
],
"connections": {
"When clicking \"Test workflow\"": {
"main": [
[
{
"node": "Gmail",
"type": "main",
"index": 0
}
]
]
},
"Gmail": {
"main": [
[
{
"node": "Gmail1",
"type": "main",
"index": 0
}
]
]
}
},
"pinData": {}
}
16 changes: 15 additions & 1 deletion packages/editor-ui/src/views/NodeView.vue
Original file line number Diff line number Diff line change
Expand Up @@ -2174,7 +2174,15 @@ export default defineComponent({
try {
const nodeIdMap: { [prev: string]: string } = {};
if (workflowData.nodes) {
const nodeNames = workflowData.nodes.map((node) => node.name);
workflowData.nodes.forEach((node: INode) => {
// Provide a new name for nodes that don't have one
if (!node.name) {
const nodeType = this.nodeTypesStore.getNodeType(node.type);
const newName = this.uniqueNodeName(nodeType?.displayName ?? node.type, nodeNames);
node.name = newName;
nodeNames.push(newName);
}
//generate new webhookId if workflow already contains a node with the same webhookId
if (node.webhookId && UPDATE_WEBHOOK_ID_NODE_TYPES.includes(node.type)) {
const isDuplicate = Object.values(
Expand Down Expand Up @@ -4137,6 +4145,12 @@ export default defineComponent({
cancelButtonText: this.$locale.baseText('nodeView.prompt.cancel'),
inputErrorMessage: this.$locale.baseText('nodeView.prompt.invalidName'),
inputValue: currentName,
inputValidator: (value: string) => {
if (!value) {
MiloradFilipovic marked this conversation as resolved.
Show resolved Hide resolved
return this.$locale.baseText('nodeView.prompt.invalidName');
}
return true;
},
},
);

Expand Down Expand Up @@ -4402,7 +4416,7 @@ export default defineComponent({
async addNodesToWorkflow(data: IWorkflowDataUpdate): Promise<IWorkflowDataUpdate> {
// Because nodes with the same name maybe already exist, it could
// be needed that they have to be renamed. Also could it be possible
// that nodes are not allowd to be created because they have a create
// that nodes are not allowed to be created because they have a create
// limit set. So we would then link the new nodes with the already existing ones.
// In this object all that nodes get saved in the format:
// old-name -> new-name
Expand Down
Loading