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

feat(core): Create names for personal projects in one place and resuse it #8995

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

despairblue
Copy link
Contributor

@despairblue despairblue commented Mar 28, 2024

Summary

This adds a helper function to the user entity that generates the name for the user's personal project.
Next step would be to mark the field as non-nullable in the entity and the migration.

Related tickets and issues

none

Review / Merge checklist

  • PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
  • 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.

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Mar 28, 2024
@despairblue despairblue marked this pull request as ready for review March 28, 2024 10:38
if (this.firstName && this.lastName && this.email) {
return `${this.firstName} ${this.lastName} <${this.email}>`;
} else if (this.email) {
return `<${this.email}>`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krynble This is what it generates for shell accounts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I'm wondering is the fact that we tested a lot of places that use this function but we haven't tested this function itself. We have 3 possible outcomes and we have no tests for it.

Would you mind adding a few tests for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -543,7 +544,7 @@ describe('DELETE /users/:id', () => {
});

test('should fail to delete a user that does not exist', async () => {
await ownerAgent.delete('/users/foobar').query({ transferId: '' }).expect(404);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated fix for the postgres tests.

@despairblue despairblue force-pushed the create-default-name-for-project branch from fa93117 to 5ef982b Compare April 8, 2024 15:20
@despairblue despairblue requested a review from krynble April 8, 2024 15:37
Copy link
Contributor

@krynble krynble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

cypress bot commented Apr 8, 2024

29 failed and 1 flaky tests on run #4620 ↗︎

29 245 5 0 Flakiness 1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 despairblue 🗃️ e2e/*
Project: n8n Commit: 5ef982bff5
Status: Failed Duration: 05:56 💡
Started: Apr 8, 2024 3:44 PM Ended: Apr 8, 2024 3:50 PM
Failed  5-ndv.cy.ts • 1 failed test

View Output Video

Test Artifacts
NDV > Stop listening for trigger event from NDV Test Replay Screenshots Video
Failed  2-credentials.cy.ts • 2 failed tests

View Output Video

Test Artifacts
Credentials > should create credentials from NDV for node with multiple auth options Test Replay Screenshots Video
Credentials > should show multiple credential types in the same dropdown Test Replay Screenshots Video
Failed  30-editor-after-route-changes.cy.ts • 2 failed tests

View Output Video

Test Artifacts
Editor actions should work > after switching between Editor and Debug Test Replay Screenshots Video
Editor zoom should work after route changes > after switching between Editor and Workflow history and Workflow list Test Replay Screenshots Video
Failed  19-execution.cy.ts • 4 failed tests

View Output Video

Test Artifacts
Execution > should send proper payload for node rerun Test Replay Screenshots Video
Execution > should send proper payload for manual node run Test Replay Screenshots Video
Execution > should successfully execute partial executions with nodes attached to the second output Test Replay Screenshots Video
Execution > should execute workflow partially up to the node that has issues Test Replay Screenshots Video
Failed  30-langchain.cy.ts • 3 failed tests

View Output Video

Test Artifacts
Langchain Integration > should be able to open and execute Basic LLM Chain node Test Replay Screenshots Video
Langchain Integration > should be able to open and execute Agent node Test Replay Screenshots Video
Langchain Integration > should add and use Manual Chat Trigger node together with Agent node Test Replay Screenshots Video

The first 5 failed specs are shown, see all 36 specs in Cypress Cloud.

Flakiness  cypress/e2e/5-ndv.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > should not retrieve remote options when required params throw errors Test Replay Screenshots Video

Review all test suite changes for PR #8995 ↗︎

Copy link
Contributor

github-actions bot commented Apr 8, 2024

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@despairblue despairblue merged commit 170b07f into feature/rbac Apr 8, 2024
23 of 35 checks passed
@despairblue despairblue deleted the create-default-name-for-project branch April 8, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants