Skip to content
New issue

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

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

Already on GitHub? # to your account

feat(core): Support create, read, update, delete projects in Public API #10269

Merged

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Jul 31, 2024

@ivov ivov marked this pull request as draft July 31, 2024 14:27
@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jul 31, 2024
@ivov ivov marked this pull request as ready for review August 1, 2024 08:19
async (req: Create, res: Response) => {
await Container.get(ProjectController).createProject(req);

return res.status(201).send();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we at least return the ID of the created project? How else do you know which project was created?

Comment on lines +13 to +14
'401':
$ref: '../../../../shared/spec/responses/unauthorized.yml'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we also need 403 here. In case the request identity is missing scopes

await testDb.truncate(['Project', 'User']);
});

describe('GET /projects', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we have test cases for each endpoint that unauthenticated requests are denied?

Comment on lines +54 to +73
it('if not licensed, should reject', async () => {
/**
* Arrange
*/
const owner = await createOwner({ withApiKey: true });

/**
* Act
*/
const response = await testServer.publicApiAgentFor(owner).get('/projects');

/**
* Assert
*/
expect(response.status).toBe(403);
expect(response.body).toHaveProperty(
'message',
new FeatureNotLicensedError('feat:projectRole:admin').message,
);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

The test cases for unlicensed requests are all pretty similar. Maybe they could be combined with test.each? No objections in keeping them like this as well

@ivov
Copy link
Contributor Author

ivov commented Aug 2, 2024

Thanks for reviewing! For future context, we discussed in the team that these PRs are okay to be kept basic as they're meant to unblock customers - the whole Public API needs heavy rework.

@ivov ivov requested a review from tomi August 2, 2024 07:45
Copy link
Member

@netroy netroy left a comment

Choose a reason for hiding this comment

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

For future context, we discussed in the team that these PRs are okay to be kept basic as they're meant to unblock customers

That makes sense. Can we create a single tracking issue for public api refactor, so that we can move these valid review comments in there.

Copy link
Contributor

github-actions bot commented Aug 2, 2024

✅ All Cypress E2E specs passed

Copy link

cypress bot commented Aug 2, 2024



Test summary

389 0 0 0Flakiness 0


Run details

Project n8n
Status Passed
Commit 4377c60
Started Aug 2, 2024 9:49 AM
Ended Aug 2, 2024 9:54 AM
Duration 04:43 💡
OS Linux Debian -
Browser Electron 118

View run in Cypress Cloud ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud

Copy link
Collaborator

@tomi tomi left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Contributor

github-actions bot commented Aug 2, 2024

✅ All Cypress E2E specs passed

@ivov ivov merged commit 489ce10 into master Aug 2, 2024
47 checks passed
@ivov ivov deleted the pay-1800-support-create-read-update-delete-projects-in-public-api branch August 2, 2024 10:02
@tomi
Copy link
Collaborator

tomi commented Aug 2, 2024

@ivov Thank you for addressing the comments 🫶 Wasn't aware of your discussions. Would have been fine as is, if I had known. I created a project in Linear to gather the improvement ideas. FYI @netroy

MiloradFilipovic added a commit that referenced this pull request Aug 2, 2024
* master:
  refactor(core): Clean up event relays (no-changelog) (#10284)
  fix(editor): Fix execution retry button (#10275)
  feat(core): Show sub-node error on the logs pane. Open logs pane on sub-node error (#10248)
  refactor(core): Move instanceRole to InstanceSettings (no-changelog) (#10242)
  feat(core): Allow filtering executions and users by project in Public API  (#10250)
  fix(core): Make execution and its data creation atomic (#10276)
  refactor(core): Mark schema env vars used by cloud hooks (no-changelog) (#10283)
  ci: Fix DB tests (no-changelog) (#10282)
  feat(core): Support create, delete, edit role for users in Public API (#10279)
  refactor(core): Decouple post workflow execute event from internal hooks (no-changelog) (#10280)
  feat(core): Allow transferring credentials in Public API (#10259)
  feat(core): Support create, read, update, delete projects in Public API (#10269)
  ci: Introduce lint rule `no-type-unsafe-event-emitter` (no-changelog) (#10254)
  fix(core): Surface enterprise trial error message (#10267)
  fix(editor): Enable moving resources only if team projects are available by the license (#10271)
  fix(core): Upgrade tournament to address some XSS vulnerabilities (#10277)

# Conflicts:
#	packages/cli/src/Server.ts
@github-actions github-actions bot mentioned this pull request Aug 7, 2024
@janober
Copy link
Member

janober commented Aug 7, 2024

Got released with n8n@1.54.0

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants