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 actions for bank transactions #10389

Merged

Conversation

CodeShakingSheep
Copy link
Contributor

Summary

Since InvoiceNinja v5 the API is capable to work with bank transactions which would be really handy for some of my workflows. So, I added the following actions for bank transactions:

  • Create
  • Delete
  • Get
  • Get Many
  • Match Payment

Related Linear tickets, Github issues, and Community forum posts

None

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@n8n-assistant n8n-assistant bot added community Authored by a community member node/improvement New feature or request in linear Issue or PR has been created in Linear for internal review labels Aug 14, 2024
@Joffcom
Copy link
Member

Joffcom commented Aug 14, 2024

Hey @CodeShakingSheep

Thanks for the PR, I have added it to my review session for next week.

At some point this is going to reach the stage where it will be tricky to maintain and we will need to think about dropping v4 support and maybe doing a v2 of the node that only has v5 features.

For now could we make this the last feature set for this node while I check with the rest of the team, I will let you know in a few days what the plans are for the node and we can work from there.

@CodeShakingSheep
Copy link
Contributor Author

Hey @Joffcom ,
Thanks for adding it to your review session for next week.

Yes, it makes sense to drop v4 support at one point. Yes, please let me know what's the plan for this node. In this context I also want to hint to @paulwer 's PR #4807 in which he already created a v4 and v5 of this node and did lots of work. Thanks a lot for your work @paulwer! 🙏

I didn't check everything but from what I saw briefly, it looks pretty solid. So, I think it would be awesome to use his PR as a base, update it with the latest Invoice Ninja node changes and then merge it.

Regarding this PR, I have a question. The linter complains about

/home/runner/work/n8n/n8n/packages/nodes-base/nodes/InvoiceNinja/BankTransactionDescription.ts
Error:    74:5  error  End with '{Entity} Name or ID' [autofixable]  n8n-nodes-base/node-param-display-name-wrong-for-dynamic-options
Error:   197:3  error  End with '{Entity} Name or ID' [autofixable]  n8n-nodes-base/node-param-display-name-wrong-for-dynamic-options

I don't really get those warnings. They refer to

{
	displayName: 'Bank Integration ID',
	name: 'bankIntegrationId',
        [...]
}

and

{
	displayName: 'Payment ID',
	name: 'paymentId',
        [...]
}

So, the display names do end with ID. Also, when I change the display name from Payment ID to Payment the lint error is still there. Can you give me a hint how the display names are supposed to be?

@Joffcom
Copy link
Member

Joffcom commented Aug 14, 2024

Hey @CodeShakingSheep,

The PR you mentioned sadly required a lot of work to bring it inline and the previous advice on how to handle it was dismissed which made things tricky. When we get around updating the node we will likely use that PR to see which features we should implement which is why it is still open.

With the lint error the wording is exactly what it is asking for so Bank Integration Name or ID and Payment Name or ID, If the label doesn't make sense you can add an ignore for that specific rule on the line before it.

@CodeShakingSheep
Copy link
Contributor Author

Hi @Joffcom ,
I could fix the linting 2 weeks ago. So this is ready for review. Also just wanted to ask if you already got any updates for the future processing of the Invoice Ninja node. I won't do any more PRs for this node until an update but I would appreciate if my 2 open PRs would still get merged. Thank you 🙏

@Joffcom Joffcom merged commit 5a2c7e0 into n8n-io:master Sep 17, 2024
15 checks passed
MiloradFilipovic added a commit that referenced this pull request Sep 17, 2024
* master:
  ci: Setup biome and pre-commit hooks for formatting (no-changelog) (#10795)
  feat(editor): Add truncate directive (#10842)
  fix(editor): Allow custom git repo urls in source control settings (#10849)
  fix(Invoice Ninja Node): Fix lint error (no-changelog) (#10848)
  fix(editor): Minimap Show nodes outside viewport (#10843)
  fix(core): Prevent shutdown error in regular mode (#10844)
  feat(Invoice Ninja Node): Add actions for bank transactions (#10389)
  fix(editor): Address edge toolbar rendering glitches (#10839)
@github-actions github-actions bot mentioned this pull request Sep 18, 2024
@janober
Copy link
Member

janober commented Sep 18, 2024

Got released with [email protected]

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 in linear Issue or PR has been created in Linear for internal review node/improvement New feature or request Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants