Invoice tax

[Valentin Crettaz]

Hello,

We'll need to apply taxes (mainly VAT) to the invoices we're generating. I heard this was on the radar but not implemented yet, so we're going to implement it to fit our needs.

What would be the correct way to handle this with minimal impact (i.e. ideally without having to fork killbill), knowing that we already have developed a plugin for an external payment gateway?

I haven't completely tracked it down yet, but would something like the following scenario work ?

1. our plugin listens for INVOICE_CREATION events

2. it retrieves the invoice entity via the internal API

3. it applies an external charge to that invoice using the OSGIKillbillAPI.getInvoiceUserApi().insertExternalCharges() method.

One thing I'm wondering, though, is whether we can manage to add that tax amount to the invoice before a payment is triggered automatically.

Or do you suggest another approach maybe?

Thanks for your great help and insights.

Val

[stephane brossier]

Val,

I can think of two (main) approaches:

The first one, is basically what you described below; this is basically working around the lack of VAT feature by using external charge items. There are a few caveats:

* By default KB reacts on any generated invoice to make a payment; so, in that scheme, your customer would see two payments, one for the original invoice, and a second one for the tax part. This seems very sub-optimal, but maybe it is good enough. One thing you could do to avoid that, is to make sure that each of your accounts is tagged with AUTO_PAY_OFF, that way the payment system will not attempt to process the payment automatically, but then, it also means you would have to trigger the payment manually.

* Also, in that scheme, your customer will not see a proper TAX item but will see an external charge instead; i suppose if you add a correct description to the item, maybe this is good enough.

A second option -- slightly more ambitious, but your ability to create the Adyen plugin with the little doc we have tells me you are already qualified for it ;-) -- is to make some change in the code: You could add a new invoice item type (TAX) and also a hook in the invoice code, to be able to call a special plugin of yours that would insert the TAX item at the time the invoice gets created.

In the same way the payment system exports a plugin api (https://github.com/killbill/killbill-plugin-api/blob/master/payment/src/main/java/org/killbill/billing/payment/plugin/api/PaymentPluginApi.java) for payment plugin to register, we could have the invoice system to export a new plugin api:

* The new InvoicePluginApi would export a method that is called from within the invoice generation code (prior to https://github.com/killbill/killbill/blob/master/invoice/src/main/java/org/killbill/billing/invoice/InvoiceDispatcher.java line 248)

* The method would allow to return addition items {EXTERNAL_CHARGE, TAX} that would be included in the invoice

* Your plugin would take the responsibility to compute the right VAT items to be added.

There are also other options such as modifying catalog to include VAT info, ... but i think those require more design and understanding of the international tax system.

Let me know if that makes sense and what you decided,

S.

--
You received this message because you are subscribed to the Google Groups "Kill Bill users mailing-list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to killbilling-us...@googlegroups.com.
To post to this group, send email to killbill...@googlegroups.com.
Visit this group at http://groups.google.com/group/killbilling-users.
To view this discussion on the web visit https://groups.google.com/d/msgid/killbilling-users/95d29291-74b1-4d46-bdc7-11a82b4fa15c%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Valentin Crettaz]

Thanks for your answer Stéphane, it all makes sense. 

Even though option 2 sounds appealing and certainly is the right way to do it, I'd like to avoid as much as possible having to fork the code and modify it, unless I find the time to do it of course.

Let's imagine I go with option 1 and I tag accounts with AUTO_PAY_OFF to prevent two invoices from being sent out (we don't want that of course). 

Then instead of having to trigger payments manually, would it work if I had some scheduler on my side calling the "Pay all invoices" API endpoint at regular intervals?

And concerning the description of the external charge and according to commit 28545f50c2900d6e216e17567d4618577c36f058, is it correct that the charge will now be labelled correctly with whatever we specify (e.g. "VAT 8% blablabla")

Thanks for your insights.

[stephane brossier]

[...]

 

Let's imagine I go with option 1 and I tag accounts with AUTO_PAY_OFF to prevent two invoices from being sent out (we don't want that of course). 

Then instead of having to trigger payments manually, would it work if I had some scheduler on my side calling the "Pay all invoices" API endpoint at regular intervals?

That forces more logic on your side, but yes, that is an option. 

 

And concerning the description of the external charge and according to commit 28545f50c2900d6e216e17567d4618577c36f058, is it correct that the charge will now be labelled correctly with whatever we specify (e.g. "VAT 8% blablabla")

I believe so, that was the goal.

Good luck,

S.

[Valentin Crettaz]

Ok, thanks for clearing that up.

Then since OSGIKillbillAPI.getInvoiceUserApi().insertExternalCharges() requires a list of InvoiceItem, the next question would be how to create such an InvoiceItem object from within the plugin knowing that InvoiceItem is an interface and ExternalChargeInvoiceItem is not available at the API level.

I'd appreciate if you can shed some light on this.

Thanks,

Val 

[Valentin Crettaz]

After some discussions, we're actually going to venture into the second option and see how it goes.

We've forked the killbill and killbill-plugin-api repositories. 

We will certainly have some more questions along the way.

Stay tuned...

Val

[stephane brossier]

Good call!

Better invest a bit of time and do something clean. We 'll be available to help you with any questions you have.

S.

--
You received this message because you are subscribed to the Google Groups "Kill Bill users mailing-list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to killbilling-us...@googlegroups.com.
To post to this group, send email to killbill...@googlegroups.com.
Visit this group at http://groups.google.com/group/killbilling-users.

To view this discussion on the web visit https://groups.google.com/d/msgid/killbilling-users/5e5ab843-d531-4997-b48e-8c6a006b5c9e%40googlegroups.com.

[Valentin Crettaz]

Ok, I'm more or less done with the changes (less the test cases of course ;)

So here is basically what I've done:

- added a new TAX element to the InvoiceItemType enum in project killbill-api

- added a new module in killbill-plugin-api-invoice in project killbill-plugin-api. In that module, I've defined a new interface called InvoicePluginApi with a single method getAdditionalInvoiceItems(Invoice)

- modified my plugin to implement that new API and return a tax invoice item if necessary

- modified InvoiceDispatcher in the killbill/invoice module to call external plugins implementing that new API and adding the items (if any) to the invoice being generated

- added a couple new Guice classes in the killbill/invoice module to glue the new stuff (i.e. Provider, Registry, Module, etc... similar to what is done for the PaymentPluginApi)

- I've also modified the pom.xml files wherever it made sense

The whole thing is compiling fine, I can start KB in a local Tomcat server just fine. There's just one tiny little thing that doesn't work yet and it should be straightforward.

I've placed a breakpoint in KillbillActivator.listenForServiceType() to make sure my new InvoicePluginApi implementation would get properly registered. The server sees it when it loads my plugin (which means it's correctly bound via the Guice provider/registry stuff, but the test on line 143 (claz.isAssignableFrom(theServiceObject.getClass())) fails. The reason is simply because of a classloading issue. I've checked what's being run for the PaymentPluginApi interface and how it compares to what I've written for the InvoicePluginApi interface. The result can be seen on the two attached files. The first one shows that the PaymentPluginApi interface is exactly the same (i.e. loaded from the same classloader), while for the InvoicePluginApi interface they are different (i.e. loaded from two different classloaders).

Now I guess the solution to this is simply to have the InvoicePluginApi interface being loaded by the WebappClassLoader instead of Felix's BundleClassLoader. If you have a quick tip on what I did wrong, I appreciate.

Thanks,

Val

[stephane brossier]

You are probably missing the export package defined in https://github.com/killbill/killbill/blob/master/util/src/main/java/org/killbill/billing/util/config/OSGIConfig.java.

There is a property 'org.killbill.osgi.system.bundle.export.packages' which defines the packages that should be exported by Kill Bill.

In the same way we have in that list 'org.killbill.billing.payment.plugin.api' you should also add your new package.

Let us know of that solves the issue.

S.

To view this discussion on the web visit https://groups.google.com/d/msgid/killbilling-users/388e7938-77bc-4413-921e-6a6a02cb992c%40googlegroups.com.

[Valentin Crettaz]

You rock! That was it! 

I've completely overlooked the OSGIConfig class, eeek!

I've got another dependency conflict as Maven will always pull the latest release of the killbill-api (0.9.5) even though I've specified my snapshot version (0.9.6-SNAPSHOT) pretty much everywhere I could think of. This is probably due to Maven's transitive dependency management. In the dependency tree, I see the following lines:

[INFO] +- org.kill-bill.billing.plugin:killbill-plugin-api-invoice:jar:0.7.4-SNAPSHOT:compile

[INFO] |  \- (org.kill-bill.billing:killbill-api:jar:0.9.5:compile - version managed from 0.9.6-SNAPSHOT; omitted for conflict with 0.9.6-20140521.210723-1)

[INFO] +- org.kill-bill.billing.plugin:killbill-plugin-api-payment:jar:0.7.3:compile

[INFO] |  \- (org.kill-bill.billing:killbill-api:jar:0.9.5:compile - version managed from 0.9.6-SNAPSHOT; omitted for conflict with 0.9.6-20140521.210723-1)

When I'm done fixing this, if you guys are interested in a pull request, we'd be happy to contribute, of course.

Thanks much for your help!

Val

[stephane brossier]

On Thu, May 22, 2014 at 2:11 AM, Valentin Crettaz <valentin...@gmail.com> wrote:

You rock! That was it! 

I've completely overlooked the OSGIConfig class, eeek!

I've got another dependency conflict as Maven will always pull the latest release of the killbill-api (0.9.5) even though I've specified my snapshot version (0.9.6-SNAPSHOT) pretty much everywhere I could think of. This is probably due to Maven's transitive dependency management. In the dependency tree, I see the following lines:

I think the easiest route is to update the version in the master pom (https://github.com/killbill/killbill-oss-parent/blob/master/pom.xml) (with your new snapshot versions), recompile the artifact, and include it in you top level killbill pom.

This is usually the way we do it when we modify multiple repos in parallel

[INFO] +- org.kill-bill.billing.plugin:killbill-plugin-api-invoice:jar:0.7.4-SNAPSHOT:compile

[INFO] |  \- (org.kill-bill.billing:killbill-api:jar:0.9.5:compile - version managed from 0.9.6-SNAPSHOT; omitted for conflict with 0.9.6-20140521.210723-1)

[INFO] +- org.kill-bill.billing.plugin:killbill-plugin-api-payment:jar:0.7.3:compile

[INFO] |  \- (org.kill-bill.billing:killbill-api:jar:0.9.5:compile - version managed from 0.9.6-SNAPSHOT; omitted for conflict with 0.9.6-20140521.210723-1)

When I'm done fixing this, if you guys are interested in a pull request, we'd be happy to contribute, of course.

We are definitely interested in a pull request; adding the ability to include extra custom items prior sealing the invoice is something we always wanted -- can be used for tax purpose or other use cases.

Thanks for the contribution!

S.

 

Thanks much for your help!

Val

On Thursday, May 22, 2014 10:03:28 AM UTC+2, stephane wrote:

You are probably missing the export package defined in https://github.com/killbill/killbill/blob/master/util/src/main/java/org/killbill/billing/util/config/OSGIConfig.java.

There is a property 'org.killbill.osgi.system.bundle.export.packages' which defines the packages that should be exported by Kill Bill.

In the same way we have in that list 'org.killbill.billing.payment.plugin.api' you should also add your new package.

Let us know of that solves the issue.

S.

--

You received this message because you are subscribed to the Google Groups "Kill Bill users mailing-list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to killbilling-us...@googlegroups.com.
To post to this group, send email to killbill...@googlegroups.com.
Visit this group at http://groups.google.com/group/killbilling-users.

To view this discussion on the web visit https://groups.google.com/d/msgid/killbilling-users/a08312b2-2315-4275-b57d-588d172303f5%40googlegroups.com.

[Valentin Crettaz]

Thanks, in the meantime I've figured a slightly different way to achieve the same effect by adding a <dependencyManagement> element in the main pom.xml which forces my new SNAPSHOT version for all occurrences of killbill-api and killbill-plugin-api anywhere in the child poms. Once the killbill-api and killbill-plugin-api are released we'll be able to remove this element altogether.

http://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#Dependency_Management

No more issue and I can "almost" see tax items being added to my newly generated invoices. One more NPE to chase and then from there it'll be a downhill trip.

Pretty cool stuff!

Thanks for your help with this I'll keep you posted if I have other issues.

Val

[Valentin Crettaz]

Ok, it's working fine from what I can see. There might probably be some edge cases which I'm not aware of, but this should be enough to get us going for now.

I'll issue pull requests asap.

Val

[Valentin Crettaz]

Ok, three pull requests have been queued (killbill-api, killbill-plugin-api + killbill projects).

Thanks again for your help

Val 

[Valentin Crettaz]

Ok, we've found one edge case when there are credits in the client billing account. Those will be taken into account in the next invoice.

However, when that next invoice is created, the CBA adjustments are performed at invoice creation time (InvoiceDispatcher, line 245), and thus, when our InvoicePluginApi is called to create additional invoice items, i.e. the invoice item containing the tax, the tax cannot be computed correctly since at that time there's no CBA invoice item yet to take the credit into account. That means that the tax is computed on the full charge amount as there is no invoice.creditAmount at that time.

For example, if we have the following item/credit

Recurring: $100

Credit: $10

The correct tax (8%) should be ($100-$10) * 0.08 =  $7.2

However, with the current implementation it is $100 * 0.08 = $8

Do you know of any easy way to circumvent this? Events maybe?

Thanks,

Val

[Valentin Crettaz]

I might have found a workaround to take care of this, i.e. by calling invoiceApi.getAccountCBA() from the plugin to get the current account credit.

That might do the trick until taxes are fully supported by KB.

Val

[stephane brossier]

Hey Val,

The workaround you did seems to be the right solution for the moment. We filed a bug to track the issue -- https://github.com/killbill/killbill/issues/198 -- we actually knew about it.

Cheers,

S.

To view this discussion on the web visit https://groups.google.com/d/msgid/killbilling-users/e1c89b25-a847-498f-877e-02b739fe3659%40googlegroups.com.

[Valentin Crettaz]

Thanks for the confirmation.

The only thing I see that could potentially mess this up is if there are several threads creating several invoices for a given customer at the same time, but I don't think we'll ever get in that case.
That'll do for now, it works well for what we needed.

Thanks

Val

[stephane brossier]

On Wed, May 28, 2014 at 5:00 AM, Valentin Crettaz <valentin...@consulthys.com> wrote:

Thanks for the confirmation.

The only thing I see that could potentially mess this up is if there are several threads creating several invoices for a given customer at the same time, but I don't think we'll ever get in that case.

The code is taking a 'global' per account lock when doing invoice/payment operations so you should be safe.

 

To view this discussion on the web visit https://groups.google.com/d/msgid/killbilling-users/01ef5718-167c-48c3-9d72-4cadb972d97c%40googlegroups.com.

[Valentin Crettaz]

We've found a second edge case occurring when the plan is changed before the invoice is paid (i.e. while it is in pending state and the customer hasn't paid it yet via the 3rd-party payment gateway).

This can arise simply because the customer picked plan A too quickly before noticing the next minute that what he needed in reality was plan B.

In this case, the first invoice is repaired with a REPAIR_ADJ/CBA_ADJ combo and suddenly the TAX invoice item doesn't match the real charged amount anymore.

Val

[Valentin Crettaz]

We're currently trying to fix this from within our plugin by listening to INVOICE_ADJUSTMENT events.

When the plan is changed, a second invoice is created and the first one is "repaired". When that repair occurs, we get such an INVOICE_ADJUSTMENT event.

At this point, we recompute the tax amount and either add the difference as a new external charge (if the new tax is higher) or as a new invoice adjustment item (if the new tax is lower).

The only issue is that adding a new item that way triggers another... INVOICE_ADJUSTMENT event and we're going in circles.

I'm trying to find a way to only react to the first INVOICE_ADJUSTMENT event and ignore the next one.

Val

[Pierre-Alexandre Meyer]

On Tue, Jun 3, 2014 at 8:36 AM, Valentin Crettaz <valentin...@consulthys.com> wrote:

We're currently trying to fix this from within our plugin by listening to INVOICE_ADJUSTMENT events.

That sounds like the right approach - adjust the tax item each time the invoice is created or modified (invoice adjustment, invoice item adjustment, etc.).

I'm trying to find a way to only react to the first INVOICE_ADJUSTMENT event and ignore the next one.

How about making your call idempotent instead? Each time any invoice event is received (invoice creation or update), recompute the new tax amount, and adjust the original item (issuing an item adjustment) if the amount differs (or add the tax item if it didn't exist).

In general, making bus-related calls idempotent is a good practice as it simplifies the logic and works in edge case scenarii (e.g. duplicate events received).

--

Pierre

[Valentin Crettaz]

Thanks for your insights.

The TAX item will always exist for any given invoice since we're returning one from the new InvoicePluginApi.getAdditionalItems() call we've just added.

The most ideal case for us would be to be able to change the amount of an existing TAX item, but that doesn't seem possible from what I saw. Thus, the only way an invoice can be modified right now is by either adding item adjustments (for the TAX item) or new charges (even though TAX != EXTERNAL_CHARGE). Regarding the latter, it seems that even though the method is called "insertExternalCharges" the invoice items that are passed can be of any type (i.e. TAX) and not only EXTERNAL_CHARGE, which is good.

I understand the reason why an idempotent call would be ideal, but I don't quite get how to achieve it in our case.

Here is what I ended up doing when processing INVOICE_ADJUSTMENT events:

1. I retrieve the TAX item. if none found, I stop right there since there's nothing to adjust

2. At this point, a TAX item exists, I then check if there's an ITEM_ADJ linked to the TAX item retrieved in step 1. if I find one, it means the TAX item has already been adjusted, so I stop there

3. Finally, if we land here, we have a non-adjusted TAX item, which we can now adjust. We create the item adjustment by linking it to the TAX item.

4. This will induce a new INVOICE_ADJUSTMENT event, which we're going to ignore given steps 1 and 2

Does this look like a sound approach to you?  

Thanks in advance

Val

[Pierre-Alexandre Meyer]

On Wed, Jun 4, 2014 at 10:48 AM, Valentin Crettaz <valentin...@consulthys.com> wrote:

The most ideal case for us would be to be able to change the amount of an existing TAX item, but that doesn't seem possible from what I saw.

Yes, currently, the only way to change an invoice is to amend it (invoice or invoice item adjustment). This is because we don't support the concept of draft invoices yet (it's on our radar though).

That being said, I'm not sure if it would help your usecase, when the client changes his mind after the payment is already triggered (in PENDING state).

 

I understand the reason why an idempotent call would be ideal, but I don't quite get how to achieve it in our case.

Here is what I ended up doing when processing INVOICE_ADJUSTMENT events:

1. I retrieve the TAX item. if none found, I stop right there since there's nothing to adjust

2. At this point, a TAX item exists, I then check if there's an ITEM_ADJ linked to the TAX item retrieved in step 1. if I find one, it means the TAX item has already been adjusted, so I stop there 

3. Finally, if we land here, we have a non-adjusted TAX item, which we can now adjust. We create the item adjustment by linking it to the TAX item.

4. This will induce a new INVOICE_ADJUSTMENT event, which we're going to ignore given steps 1 and 2

Does this look like a sound approach to you?  

It does although there is a limitation where you can only have a single tax item adjustment. This would break if your client changes its plan again.

How about the following:

1. Retrieve the invoice

2. Compute the total tax amount as it would be done by the invoice plugin (InvoicePluginApi.getAdditionalItems())

3. Compute the sum of all tax items on the invoice

4. If the amount differs, item adjust the original tax item, otherwise do nothing

This should give you idempotency.

Also, how are you dealing with the PENDING payment? On average, this would trigger 3 payments in the gateway (original invoice and two adjustments) unless you play with AUTO_PAY_OFF tags.

--

Pierre

[Valentin Crettaz]

The most ideal case for us would be to be able to change the amount of an existing TAX item, but that doesn't seem possible from what I saw.

Yes, currently, the only way to change an invoice is to amend it (invoice or invoice item adjustment). This is because we don't support the concept of draft invoices yet (it's on our radar though).

That being said, I'm not sure if it would help your usecase, when the client changes his mind after the payment is already triggered (in PENDING state).

Yes, indeed, open invoices would be a great addition. Create an invoice, add stuff to it (items, charges, credits, taxes, etc), and when done, seal it.

That would make the point about adding support for batch subscriptions that I raised the other day (https://github.com/killbill/killbill/issues/199) irrelevant, since we could just create 1+ subscriptions as we do now and when done, we could seal the open invoice.

Good that this is on your radar. We'll do without for now.

[...]  

It does although there is a limitation where you can only have a single tax item adjustment. This would break if your client changes its plan again.

[...]  

Actually when the plan changes, the first invoice is rebalanced, the tax is adjusted and a second invoice gets created.

If the user changes the plan again, I believe the first invoice is untouched, but now the second would be balanced, tax adjusted, etc and a third invoice would be created, right? 

So there will always ever be a single tax item in any invoice.

 

Also, how are you dealing with the PENDING payment? On average, this would trigger 3 payments in the gateway (original invoice and two adjustments) unless you play with AUTO_PAY_OFF tags.

We're "batching" everything we can into a single invoice by using AUTO_INVOICING_OFF as described the other day in https://groups.google.com/d/msg/killbilling-users/w0ZAxa9Z0BA/h3P_nC0QzjIJ

Despite of that, there will still be one PENDING payment per invoice created, which is not ideal in terms of user-friendlyness, but that should do for now.

Val

[Pierre-Alexandre Meyer]

On Thu, Jun 5, 2014 at 4:21 AM, Valentin Crettaz <valentin...@consulthys.com> wrote:

Yes, indeed, open invoices would be a great addition. Create an invoice, add stuff to it (items, charges, credits, taxes, etc), and when done, seal it.

That would make the point about adding support for batch subscriptions that I raised the other day (https://github.com/killbill/killbill/issues/199) irrelevant, since we could just create 1+ subscriptions as we do now and when done, we could seal the open invoice.

I think we want both features. In the "shopping cart" checkout flow scenario, it's nice to be able to create the subscriptions atomically and trigger the invoice / payment in one call.

Actually when the plan changes, the first invoice is rebalanced, the tax is adjusted and a second invoice gets created.

If the user changes the plan again, I believe the first invoice is untouched, but now the second would be balanced, tax adjusted, etc and a third invoice would be created, right? 

So there will always ever be a single tax item in any invoice.

Oh I see, you're right. You can have multiple adjustments though of the tax item (e.g. in the case of a manual invoice adjustment).

We're "batching" everything we can into a single invoice by using AUTO_INVOICING_OFF as described the other day in https://groups.google.com/d/msg/killbilling-users/w0ZAxa9Z0BA/h3P_nC0QzjIJ

Despite of that, there will still be one PENDING payment per invoice created, which is not ideal in terms of user-friendlyness, but that should do for now.

Indeed. A potential workaround would be to delay the payment using the AUTO_PAY_OFF tag (similar behavior than the AUTO_INVOICING_OFF tag, but for the payment subsystem).

--

Pierre

[Valentin Crettaz]

Yes, indeed, open invoices would be a great addition. Create an invoice, add stuff to it (items, charges, credits, taxes, etc), and when done, seal it.

That would make the point about adding support for batch subscriptions that I raised the other day (https://github.com/killbill/killbill/issues/199) irrelevant, since we could just create 1+ subscriptions as we do now and when done, we could seal the open invoice.

I think we want both features. In the "shopping cart" checkout flow scenario, it's nice to be able to create the subscriptions atomically and trigger the invoice / payment in one call.

True 

 

[...]

We're "batching" everything we can into a single invoice by using AUTO_INVOICING_OFF as described the other day in https://groups.google.com/d/msg/killbilling-users/w0ZAxa9Z0BA/h3P_nC0QzjIJ

Despite of that, there will still be one PENDING payment per invoice created, which is not ideal in terms of user-friendlyness, but that should do for now.

Indeed. A potential workaround would be to delay the payment using the AUTO_PAY_OFF tag (similar behavior than the AUTO_INVOICING_OFF tag, but for the payment subsystem).

Indeed, AUTO_PAY_OFF could do the trick... I'm going to investigate this next.

Thanks,

Val