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(Invoice Ninja Node): Add more query params to getAll requests #9238

Merged

Conversation

CodeShakingSheep
Copy link
Contributor

@CodeShakingSheep CodeShakingSheep commented Apr 29, 2024

Summary

Describe what the PR does and how to test. Photos and videos are recommended.

This PR adds more query parameters to existing getAll requests:

Additionally, this PR fixes a small typo contactsValues instead of contacstValues.

Related tickets and issues

Include links to Linear ticket or Github issue or Community forum post. Important in order to close automatically and provide context to reviewers.

None available. The background of this PR is that I needed some additional filtering (especially for getting my invoices) to make the requests faster (and not unnecessarily load all entities).

Review / Merge checklist

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

@CodeShakingSheep CodeShakingSheep changed the title feat(InvoiceNinja Node): Add more query params to getAll requests feat(Invoice Ninja Node): Add more query params to getAll requests Apr 29, 2024
@n8n-assistant n8n-assistant bot added community Authored by a community member node/improvement New feature or request labels Apr 29, 2024
@Joffcom
Copy link
Member

Joffcom commented Apr 29, 2024

Hey @CodeShakingSheep,

The name typo you have fixed is annoyingly a breaking change, Are you able to revert that? This is something we will probably fix in a v2 of the node.

@CodeShakingSheep
Copy link
Contributor Author

Hey @CodeShakingSheep,

The name typo you have fixed is annoyingly a breaking change, Are you able to revert that? This is something we will probably fix in a v2 of the node.

Hey @Joffcom ,
Thanks for the hint. I wasn't assuming this would be a breaking change. I just pushed a commit 49f344c to revert the typo fix.

@Joffcom
Copy link
Member

Joffcom commented May 20, 2024

Hey @CodeShakingSheep,

That looks good to me, Thanks 👍🏻

Joffcom
Joffcom previously approved these changes May 20, 2024
@CodeShakingSheep
Copy link
Contributor Author

Hey @CodeShakingSheep,

That looks good to me, Thanks 👍🏻

Hey @Joffcom,
as I was working further with the the InvoiceNinja nodes in n8n, I noticed that this PR wasn't complete as I believe that the query params have to be added to the getAll requests explicitly in InvoiceNinja.node.ts as it's done e.g. for the include query param, see https://github.com/n8n-io/n8n/blob/master/packages/nodes-base/nodes/InvoiceNinja/InvoiceNinja.node.ts#L367. I just pushed commits to fix that.

Also, I fixed the query param name for the invoice getAll request as it apparently changed from invoice_number to number. See https://api-docs.invoicing.co/#get-/api/v1/invoices

@CodeShakingSheep
Copy link
Contributor Author

Hi @Joffcom ,
Thanks for approving this PR 🙏 As it has been approved for almost one month, I'm wondering what is preventing it from getting merged? Thanks for your feedback. Also, I just want to kindly hint at #10196 which fixes another InvoiceNinja v5 bug. Would appreciate a review. Thank you 🙏

@Joffcom
Copy link
Member

Joffcom commented Aug 1, 2024

Hey @CodeShakingSheep,

It looks like on this one I just need to merge master to fix the failed check. I wil get that done and the other PR so they can be in the next release.

@CodeShakingSheep
Copy link
Contributor Author

Thank you @Joffcom . Looks like the checks are all passing now 🙌

@netroy netroy merged commit 50b7238 into n8n-io:master Oct 2, 2024
15 checks passed
This was referenced Oct 2, 2024
@janober
Copy link
Member

janober commented Oct 2, 2024

Got released with [email protected]

@CodeShakingSheep CodeShakingSheep deleted the invoiceninja-add-more-query-params branch October 22, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member node/improvement New feature or request Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants