n8n
n8n copied to clipboard
n8n-io
feat(InvoiceNinja Node): extended support for most of the available resources, operations and events in v5
Changes to the node implementing current invoiceninja methods and events. Some things, like methods for CRUD projects or vendors are still missing.
Event-IDs are read by invoiceninja database administration.
Documentation should be after merging.
There are no breaking changes involved. Do we need an updated version-number of the node here? (outdated, see info later)
there are some mayor changes between v4 and v5. I've tried to update the node ready for v5 endpoints, but encountered many fields, which does not exists on v4 and are present within the old code.
Did someone know, if they come from v4 and therefore should not be deleted to downward-compatibility?
new webhooks are tested on a live version enpoints are tested each and partialy for the fields
i will split the codebase for v4 and v5 more strictly to ensure backward compatibility.
@Joffcom Is there a way, to split the Descriptions based on a node-parameter, like here called "apiVersion"?
If not, I would like to split the current node into 2 nodes. (Invoice Ninja V4 & Invoice Ninja V5), because the differ to much from one another and are hardly to manage both at the same time. I've tried to split them based on the Description parameters, but it seems not to integrate well into the n8n system. (like attached)
- operations get displayed multiple times
- in eighter way, all oprations are always visible, no matter, which apiVersion is selected
Please present me the optimal next steps, to make this separation happen. Does the exsisting node have to be deprecated and 2 separate new Nodes to be created? Or do you prefer anything else? InvoiceNinja.zip
Hey @paulwer,
There shouldn't be any need to make 2 nodes as we support v4 and v5 in the node through the node versioning, If you have a setting that only exists for v5 you can set the display the display to only show for that version and the same for any data structures that are different you can use the apiVersion to work out what endpoint to use.
If you look at the current node you can see where we are already doing this.
@Joffcom i've implemented the node versioning pattern from HTTPRequest
Please check and may give some feedback
There are also more changes required, so development will take some time.
Hey @paulwer,
That is a massive amount of work there but maybe a little but over the top, We already had the light versioning enabled in the node which was used to allow support for v4 and v5 which I would have expected to work. It also allows users to change the version they are using from the n8n UI.
Hey @Joffcom, the current node does only supports some of the api-features. I'am currently developing this only for InvoiceNinja V5. The user still has the option to choose between v4 and v5 in the node. The code is just more split, than before, because some of the fields are called differently or neighter exists.
Because, there are some mayor breaking changes (other parameter names in n8n), I had implemented versioning of the whole node, like in the HTTPRequest node. (dont get confused, between versions of N8N and versions of InvoiceNinja)
There are 4 more ressources to come, so development is still in progress.
@Joffcom
When a user enables apiVersion v4 they can do the same operations, like before, for the givven resources. When they switch/migrate to v5, they can use even more resources/operations, with focus on the v5 of InvoiceNinja.
If you have the time, checkout my fork and run the node to view the actual aproach.
6 Resources within v4: (as already implemented)
12 (later 16) Resources within v5: (partialy new and not available for v4-users)
- new operations, like downloading an invoice/quote etc
Hey @paulwer,
The HTTP Request node versioning is more for what we call Full Versioning which would apply for a complete node overhaul which would be an internal task for when we completely change the functionality.
Different field names or new ones should have been achievable using what was there already but once you are finished we can create a ticket for getting this reviewed.
@Joffcom imagine, if we are using the node versioning strategy, how does users are promted to update their nodes to the newest version?
@Joffcom the main work is done, I will now just get an review by the invoiceninja team for the fields and will come back to your, after we had finished the testing and QS.
Please tell me in the meantime, how we want to process after this phase.
Hey @paulwer,
I spotted the thread on the Invoice Ninja issues, I think on our side we would need to review the code and the structure to check the quality and that it can be easily maintained, passes the lint rules, doesn't have a lot of code-reuse and that all the node versions work.
As it adds more features we will need to look at updates to the docs and also see where Invoice Ninja is on our node overhaul plan as we are changing the design and standardising some of the inputs / fields that nodes have.
Because there is such a drastic change to this node though that may not have been needed rather than an hour or 2 to review this will likely take a lot longer so will need to go through a prioritisation process to see where it can fit between all of the other things we are currently doing.
I will of course let you know when we start the review.
Hey @Joffcom, thanks for your reply. docs are already prepared: https://github.com/n8n-io/n8n-docs/pull/1033 Please reach out to me with more informations, when you have them. Do you have a time-frame, in which I can exspect the next action?
Hey @paulwer,
No timeframe at the moment, I have a few PRs in my queue ahead of this one but we will need to prioritise this one.
Hey @Joffcom, thanks, so we are talkiing about weeks and not multiple months? Would sounds great for me. I will take a note here, if the pr is reviewable.
Thank you kindly
Hey @paulwer,
It all comes down to that priority and if anything urgent comes up before then so it could be weeks or it could slip to months. Ideally I would love to aim for weeks but I can't promise anything as things could change quickly.
Generally as a rule I spent time on Thursdays doing PR reviews but I may split that over a couple of days to get the count down.
@o-psi @Joffcom I've runned the command lint:fix and the resources were ordered by alphanumeric.
is it wanted / possible to order the ressources like the order within the docs for use and topics?
@Joffcom A reworking of this node is needed anyway as the node does not work currently, correct?
We use N8N primarily with Invoice ninja, so its fairly important for us to have this working.
@paulwer I am going to install your fork and begin testing.
@o-psi there was a bug within the latest version, of creating new webhooks, that should be fixed now. https://github.com/invoiceninja/invoiceninja/issues/8260
Edit1: the dockerimage is still not updated, but should be within the day i guess. my version has a workaround implemented, so if you want to fix the node shortly, just add rest_method: 'POST', within the body of a creation hook in v5.
Edit2: the dockerimage was updated and after a fresh pull the error is resolved (at my end)
@Joffcom @o-psi i am done with my tests.
the "problem" is, that we dont have any DTOs defined by InvoiceNinja, so I try to test every parameter if it have an effect for the result and removed these in the last updates while testing. I hoped, something has not slipped through.
@o-psi if you need anything, just reach out :)
@Joffcom @alexgrozav I included the latest change from https://github.com/n8n-io/n8n/pull/5538 into the pr.
@Joffcom i updated the pr to the latest changes of the master branch.
please check the name-convention of the trigger, because i had to set an eslint disable here to ignore the accuring error.
is the review already in progress?
Hey @paulwer
The review is not in progress yet as I still have a fairly large list of other requests to look at which can be seen by looking at the PRs with a community label.
Once it starts I will let you know but looking at how much this has changed there is a lot of testing to do internally and it could be that we drop the versioning in favour of the previously mentioned method with the light versioning so that users are able to swap between versions which is important if someone is likely to be self hosting.