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): Filter executions by project ID in internal API #10976

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Sep 26, 2024

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Sep 26, 2024
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.

LGTM, but I have two concerns for the long run

  1. The queries in this repository are starting to become a bit too complicated and hard to maintain. We should look into refactoring these.
  2. We already support filtering by projectId in the public api because that endpoint has it's own implementation that does not use queries from this repository. We should try to consolidate these codes soon, before they diverge so far that we have to start worrying about maintaining two implementations to provide backward compatibility on the public API.

@ivov
Copy link
Contributor Author

ivov commented Sep 26, 2024

LGTM, but I have two concerns for the long run

1. The queries in this repository are starting to become a bit too complicated and hard to maintain. We should look into refactoring these.

2. We already support filtering by projectId in the public api because that endpoint has it's own implementation that does not use queries from this repository. We should try to consolidate these codes soon, before they diverge so far that we have to start worrying about maintaining two implementations to provide backward compatibility on the public API.

100% - I've brought up these two concerns on executions repository and Public API before, e.g. here. We only need the bandwidth.

Copy link

cypress bot commented Sep 26, 2024

n8n    Run #7057

Run Properties:  status check passed Passed #7057  •  git commit b9b1954322: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Project n8n
Branch Review pay-1999-update-executions-api-filtering
Run status status check passed Passed #7057
Run duration 04m 20s
Commit git commit b9b1954322: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Committer Iván Ovejero
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 2
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 432
View all changes introduced in this branch ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

@ivov ivov merged commit 06d749f into master Sep 26, 2024
40 checks passed
@ivov ivov deleted the pay-1999-update-executions-api-filtering branch September 26, 2024 11:31
This was referenced Oct 2, 2024
@janober
Copy link
Member

janober commented Oct 2, 2024

Got released with [email protected]

ivov added a commit that referenced this pull request Nov 6, 2024
Follow-up to #10976

This was not caught there as the tests were for logic after middleware.
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 Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants