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): Fix populating of node custom api call options #5303

Merged
merged 5 commits into from
Feb 2, 2023

Conversation

OlegIvaniv
Copy link
Contributor

Github issue / Community forum post (link here to close automatically): https://community.n8n.io/t/shopify-generic-api-call/22375

@OlegIvaniv OlegIvaniv requested a review from ivov January 31, 2023 10:00
@OlegIvaniv OlegIvaniv self-assigned this Jan 31, 2023
@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Jan 31, 2023
Copy link
Contributor

@ivov ivov left a comment

Choose a reason for hiding this comment

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

It'd be nice to add an e2e test to verify that the injected option is there, if not too time-consuming.

packages/core/src/DirectoryLoader.ts Outdated Show resolved Hide resolved
packages/core/src/DirectoryLoader.ts Outdated Show resolved Hide resolved
packages/core/src/DirectoryLoader.ts Outdated Show resolved Hide resolved
packages/core/src/DirectoryLoader.ts Outdated Show resolved Hide resolved
packages/core/src/DirectoryLoader.ts Outdated Show resolved Hide resolved
packages/core/src/DirectoryLoader.ts Outdated Show resolved Hide resolved
@OlegIvaniv
Copy link
Contributor Author

@ivov Thanks for reviewing and for all the helpful suggestions. I've addressed them and added the e2e test to check for the Custom API option and hint. Can you have another look, please?

@OlegIvaniv OlegIvaniv requested a review from ivov February 1, 2023 13:50
Copy link
Contributor

@ivov ivov left a comment

Choose a reason for hiding this comment

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

Thanks for addressing everything!

packages/core/src/DirectoryLoader.ts Outdated Show resolved Hide resolved
packages/core/src/DirectoryLoader.ts Show resolved Hide resolved
cypress/e2e/5-ndv.cy.ts Show resolved Hide resolved
@OlegIvaniv OlegIvaniv requested a review from ivov February 2, 2023 09:55
Copy link
Contributor

@ivov ivov left a comment

Choose a reason for hiding this comment

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

🚀

@OlegIvaniv OlegIvaniv merged commit e58bc41 into master Feb 2, 2023
@OlegIvaniv OlegIvaniv deleted the ado-242-bug-custom-api-hint-does-not-show branch February 2, 2023 13:53
@n8n-assistant n8n-assistant bot added the Upcoming Release Will be part of the upcoming release label Feb 2, 2023
if (!description.credentials) return false;

return description.credentials.some(({ name }) => {
const credType = this.credentialTypes[name].type;
Copy link
Member

@netroy netroy Feb 2, 2023

Choose a reason for hiding this comment

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

This check here assumes that all credentials are loaded before the nodes. This is breaking custom nodes. Here is the fix.
There is also the issue of nodes only being aware of credentials that the loader they are a part of, which means that custom nodes can't use one of the officially shipped credentials.

OlegIvaniv added a commit that referenced this pull request Feb 2, 2023
OlegIvaniv added a commit that referenced this pull request Feb 2, 2023
Revert "feat(core): Fix populating of node custom api call options (#5303)"

This reverts commit e58bc41.
@janober
Copy link
Member

janober commented Feb 3, 2023

Got released with [email protected]

@janober janober removed the Upcoming Release Will be part of the upcoming release label Feb 3, 2023
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 ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants