Details

Reviewers

Maniphest Tasks

T86492: Implement VAT logic for subscriptions

Summary

Goal

The goal of this patch is to have support for taxable subscription services, such as Cloud subscriptions, without breaking donations already used by Development Fund.

See the specs for the VAT calculations here: https://developer.blender.org/T86492 for details.

Implementation details

pyvat is used to calculate VAT charge types and rates for subscriptions.

Because VAT calculations actually affect the price displayed to the customer, something has to store the "original" price.
For the lack of better alternative, Subscription.price is now considered the "original" price. This means that a subscription can generate an order with a price that differs from the subscription's price, because tax is subtracted from the Subscription.price: this will happen when a reverse-charge applies to subscription's customer.
All the newly added or renamed tax fields remain shared and have the same meaning across the models.

Examples of how this looks in the WIP Cloud's checkout:

Diff Detail

Repository: rL Looper

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Anna Sirota (railla) edited the summary of this revision. (Show Details)May 11 2021, 4:32 PM

Unless I'm mistaken, the Store currently checks VAT numbers with an online service called VIES (which always makes me giggle because it means "dirty" in Dutch). Is this also done by pyvat?

Because VAT calculations actually affect the price displayed to the customer, something has to store the "original" price.

What is "original" in this case? Is it the price excluding VAT? In the Store the prices are always shown including VAT, so unless you're a company in the EU that can do VAT tricks, people always pay the same price. The fact that different countries have different VAT amounts is reflected in our revenue, and not in the price paid by the subscriber. Is this still the intended behaviour?

All the newly added or renamed tax fields remain shared

What does "shared" mean?

For me the "Per month" is confusing. This is probably the yearly price divided by 12, to show some per-month price. This is not explicit, though, and it could be interpreted as if we'll charge per month. Which then conflicts with the "due today" amount.

The video is a bit vague, as I don't know exactly what's going on. It also looks like there is some background processing happening, as some fields seem to update non-instantly. This isn't clear from the UI, though.

In D11118#286906, @Sybren A. Stüvel (sybren) wrote:

Unless I'm mistaken, the Store currently checks VAT numbers with an online service called VIES (which always makes me giggle because it means "dirty" in Dutch). Is this also done by pyvat?

Yes, it supports checking VATINs with VIES. However, this functionality is not yet used, only a basic format check is performed.
Enabling a check with an API (VIES or a paid one) can be easily plugged into Studio checkout, there's a separate ticket for this.
If the whole process of validating VATINs becomes part of looper, it will happen in a separate patch, so I'd consider this out of scope of this one, if it makes sense.

Because VAT calculations actually affect the price displayed to the customer, something has to store the "original" price.

What is "original" in this case? Is it the price excluding VAT? In the Store the prices are always shown including VAT, so unless you're a company in the EU that can do VAT tricks, people always pay the same price. The fact that different countries have different VAT amounts is reflected in our revenue, and not in the price paid by the subscriber. Is this still the intended behaviour?

As you stated above and as is stated in the description and the ticket with specs, it is reflected in the price paid by the subscriber, in case customer has a valid VATIN:

This means that a subscription can generate an order with a price that differs from the subscription's price, because tax is subtracted from the Subscription.price: this will happen when a reverse-charge applies to subscription's customer.

In all other scenarios the price is the same, including tax or excluding tax: below the address changes from US to Netherlands (no change in price, only tax is displayed) and then Germany with a VATIN (price changes, VAT is excluded)

All the newly added or renamed tax fields remain shared

What does "shared" mean?

Apologies, that wording was confusing. There were already a few unused tax* fields in the SharedFields model, they were changed, e.g. tax_region got renamed to tax_country, tax_rate was added etc. (see the migration), but remained on the SharedFields model.

For me the "Per month" is confusing. This is probably the yearly price divided by 12, to show some per-month price. This is not explicit, though, and it could be interpreted as if we'll charge per month. Which then conflicts with the "due today" amount.

Fair enough, it's meant to show approximate price-per-month, depending on the plan you choose:

The exact wording there will be changed, the checkout is still work in progress.

The video is a bit vague, as I don't know exactly what's going on. It also looks like there is some background processing happening, as some fields seem to update non-instantly. This isn't clear from the UI, though.

That's also fair, it could use some UX changes, like indications of loading and blocking of the form etc. The video is to illustrate how the price is changing when a valid VATIN is added.

Order must have store VATIN that was supplied at the moment of charge: add Order.vat_number column rLcfcc426ac9e7: Add VATIN field to the Order: Order must have a snapshot of this value

@Sybren A. Stüvel (sybren) After going over reporting in Store's admin today with Francesco, I had to add another change to this patch: Order has to snapshot not only billing address and email, but also VATIN, if it's set on the Customer when the order is generated.
This is because all orders affected by VATINs have to be included into a special report, regardless of whether customer changes or removes their VATIN later on.

The change adds a migration that adds a column vat_number to orders, and also copies vat_number from the Customer in generate_order().

In D11118#286920, @Anna Sirota (railla) wrote:

Yes, it supports checking VATINs with VIES. However, this functionality is not yet used, only a basic format check is performed. [...] it will happen in a separate patch, so I'd consider this out of scope of this one, if it makes sense.

Yup, makes perfect sense.

What is "original" in this case? Is it the price excluding VAT? In the Store the prices are always shown including VAT, so unless you're a company in the EU that can do VAT tricks, people always pay the same price. The fact that different countries have different VAT amounts is reflected in our revenue, and not in the price paid by the subscriber. Is this still the intended behaviour?

As you stated above and as is stated in the description and the ticket with specs, it is reflected in the price paid by the subscriber, in case customer has a valid VATIN:

I still don't understand what the definition is of "original"-between-quotes-price.

In all other scenarios the price is the same, including tax or excluding tax

The same as what?

Apologies, that wording was confusing. There were already a few unused tax* fields in the SharedFields model, they were changed, e.g. tax_region got renamed to tax_country, tax_rate was added etc. (see the migration), but remained on the SharedFields model.

Ah ok, that's all fine then.

For me the "Per month" is confusing. This is probably the yearly price divided by 12, to show some per-month price. This is not explicit, though, and it could be interpreted as if we'll charge per month. Which then conflicts with the "due today" amount.

Fair enough, it's meant to show approximate price-per-month, depending on the plan you choose:

Ok.

The exact wording there will be changed, the checkout is still work in progress.

That's fine, but I can't review a patch when I don't know what things are supposed to mean ;-)

In D11118#287351, @Anna Sirota (railla) wrote:

I had to add another change to this patch: Order has to snapshot not only billing address and email, but also VATIN, if it's set on the Customer when the order is generated.

👍

There seem to be limitations as to what MyPy can test, as I've found quite a few cases where the code doesn't match the type declarations. Maybe worth checking out how these managed to slip through.

looper/models.py
994–999	Constructing a `Taxable` from `old_state` could probably be its own function as well.
1000	Construction of a `Taxable` given `self` is something that happens more than once. It'll be nice to just have a function for this.
1001	This can be flipped to `if old_taxable == new_taxable: return`, so that the actual tax-handling code is not in a conditional block
1002–1005	This looks like it should be a function on `Taxable`, so that you can do `self.tax = new_taxable.charged_tax()` or something along those lines. With all the above notes, the function would become something like this: old_taxable = self.taxable_from_old_state(old_state) new_taxable = self.taxable() if old_taxable == new_taxable: return self.tax = new_taxable.charged_tax_amount() if update_fields is not None: update_fields.add('tax')
1086	This could use the `taxable = self.taxable()` function mentioned above.
1221	There are so many possible uses of the word "project" that it could use some specification here. I'm guessing it's referring to the Django project?
looper/taxes.py
23	This could probably be an enum?
29–32	This field doesn't seem used in this class, and since it's a private field (my assumption, given the initial underscore) it can be removed.
38–43	This will raise a `KeyError` on dontations, instead of returning `None` as the function declaration would suggest.
46	This can be an enum. That way values of this type can be passed as actual `TaxType` instances, making type declarations easier to understand (the very generic `str` would become the specific `TaxType`).
51	The description of the supported scenario doesn't mention anything about charging taxes or not. This makes the 2nd sentence unclear. The second sentence also seems to indicate that "other scenarios" are actually supported in this code, which conflicts with the fact that "other scenarios" actually refers to "other than what we support".
54	There is no definition of what is the default type. I could assume it's the first-mentioned one, or the empty one (which happen to be the same option in this particular case), but such things should be written down explicitly.
59–63	This field doesn't seem used in this class, and since it's a private field (my assumption, given the initial underscore) it can be removed.
71	I would expect a classmethod `TaxType.get(...)` to return an instance of `TaxType`. Why not turn this tuple-of-optionals into some class instance?
71	What is the semantic difference between returning `None` tax percentage or `0.0` tax percentage? Wouldn't both convey "no tax paid"? If that is the case, you can remove the `Optional[]` and always return a valid `Decimal`. If there is a semantic difference, this needs to be documented, otherwise the `Optional` should be removed.
94	There should be protection here against unexpected values. Right now this just returns `None`, which is invalid given the declared return type.
96	This function is only used in `TaxType`, and thus should be a private member of that class to express that relationship.
103–107	Flip the condition: `if not item_type: return None`. That way the core functionality of the function can remain outside of conditionals.
110	Same question as above: what is the semantic difference between `None` tax and `0.0` tax? This MUST be documented, or the `Optional` should be removed.
110	This function is only used in `TaxType`, and thus should be a private member of that class to express that relationship.
113–117	Reduce cognitive complexity by flipping conditions & returning early.
114–115	What does it mean when `vat_rules` is false-y? Does that mean the VAT rules are unknown? Or that the country doesn't exist? Or something else?
124	This MUST include information about whether this is the price including or excluding tax.
126	Units MUST be documented. Is 21% tax stored as `21` or as `0.21`?
128	What is this field?
129–130	What's the semantic difference between `None` money and `Money(0)`? This must be documented, or the `Optional` should be removed.
134–145	This is another indication that the tax type should be a class instance, and not just a string. The initialisation of `self.tax = Money(currency, 0)` and these three `if`-statements can then all be removed and replaced by a few calls on `self.tax_type`. The specific `TaxType` instance can then handle the type-specific stuff.
looper/templatetags/looper.py
28–29	This conflicts with the declared return type.
36–42	I'm not a fan of "set these variables, and then immediately set them to something else" code. To me this is clearer: if not user or user.is_anonymous: return looper.taxes.Taxable(price, tax_type=None, tax_rate=None) tax_type, tax_rate = user.customer.get_tax(product_type) return looper.taxes.Taxable(price, tax_type=tax_type, tax_rate=tax_rate) But I can also see the advantage of having one `looper.taxes.Taxable()` call. Maybe it shows that "get the tax type/rate for the current user" should be a different function. That function can then handle the differentiation between anonymous and authenticated users. If nothing else, this exercise shows that the call to `looper.taxes.Taxable(..., None, None)` is invalid, because neither `Taxable.tax_type` nor `Taxable.tax_rate` are allowed to be `None`.

This revision now requires changes to proceed.May 17 2021, 12:53 PM

What is "original" in this case? Is it the price excluding VAT? In the Store the prices are always shown including VAT, so unless you're a company in the EU that can do VAT tricks, people always pay the same price. The fact that different countries have different VAT amounts is reflected in our revenue, and not in the price paid by the subscriber. Is this still the intended behaviour?

As you stated above and as is stated in the description and the ticket with specs, it is reflected in the price paid by the subscriber, in case customer has a valid VATIN:

I still don't understand what the definition is of "original"-between-quotes-price.

"Original" meaning one that was copied from the plan variation when Subscription was created vs what is actually charged in a transaction.

In all other scenarios the price is the same, including tax or excluding tax

The same as what?

The same as the price set on the chosen price variation, which was copied to the Subscription.price.

This patch introduces a scenario in which Order.price != Subscription.price, which didn't exist before.
A Subscription doesn't link to a plan variation, so its price is the only field we can take the charged amount from in case, for example, a business's VATIN becomes invalid and the amount they are charged changes from Order.price = Subscription.price - tax to just Order.price = Subscription.price.

The exact wording there will be changed, the checkout is still work in progress.

That's fine, but I can't review a patch when I don't know what things are supposed to mean ;-)

Fair point, in the meantime the wording was updated, here's the latest version of the "plan selector" in action which just shows what will be charged immediately and what will be charged next time, same as Store does:

There seem to be limitations as to what MyPy can test, as I've found quite a few cases where the code doesn't match the type declarations. Maybe worth checking out how these managed to slip through.

I have to admit that I don't run it often, and should probably make MyPy check part of looper test run, same as DevFund does.
The reason I don't normally run it is because there are over 200 existing typing errors in looper even without this patch. I'll try to go over the ones that are introduced in the patch, but it's a bit time consuming.

looper/taxes.py
23	It could, but if it were, it would complicate the code without adding actual benefits and taking away some readability. The disadvantages in this case include: Utility attributes (`_choices`, `_product_to_item_type`) would have to become methods or be moved elsewhere; Direct mapping or referencing of values will no longer be possible: `ProductType.DONATION` is no longer a string value that can be used, meaning that all the code that currently conviniently uses `ProductType.DONATION` will have to use `ProductType.DONATION.value`; @classmethod def as_choices(cls) -> Tuple['ProductType', str]: return ( (ProductType.ELECTRONIC_SERVICE.value, 'Generic electronic service'), (ProductType.DONATION.value, 'Donation'), ) def get_taxable_item_type(self) -> Optional[pyvat.ItemType]: """Returned pyvat's ItemType matching our ProductType. Out of supported types -- donation and electronic service, only the latter is taxable. """ if self == ProductType.ELECTRONIC_SERVICE: return pyvat.ItemType.generic_electronic_service return None There will be an artificial difference between the value of the column `Product.type` and the values of enum, which would require keeping in mind where it has to be a `.value` and where just passed as it is, which seems completely unnecessary: type = models.CharField( choices=ProductType.as_choices(), default=ProductType.DONATION.value, max_length=20, help_text='The nature and type of goods or services supplied (for tax administration)', ) ... # in update_tax ... product_type = self.plan.product.type self.tax_type, self.tax_rate = customer.get_tax(product_type=ProductType(product_type)) ... Enums are useful when used for enumeration, mapping attributes to strings not so much, and this class is added only to add syntactic sugar for `CharField` choices.
29–32	It's used in `looper.models.Product` to define `Product.type` choices, it doesn't make sense to make it private though (renamed `_choices` to `choices`).
38–43	`dict.get(key, value=None)` returns `None` when `key` is not present (unlike `getattr` which doesn't have a default value for `value`). $ python Python 3.9.2 (default, Feb 20 2021, 18:40:11) [GCC 10.2.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> {}.get('foobar') is None True >>> $ python3.6 Python 3.6.12 (default, Jan 15 2021, 09:51:15) [GCC 10.2.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> {}.get('foobar') is None True >>>
46	Same as the above: I don't think trading readable code for better type declarations makes sense here.
51	Oh, the second sentence has somehow lost a `not` so it makes no sense at all. Should at least be the following: In other scenarios taxes are not charged I've rephrased the comment to only refer to supported scenarios: In other scenarios, such as donations and sales to non-EU countries, taxes are not charged.
54	I've rephrased the comment so that it clearly refers to the `SharedFields.tax_type` and explains why `NO_CHARGE` is a blank string.
59–63	same as the other `_choices`: used in the `SharedFields.tax_type.choices`, removed the underscore here as well.
71	I've been hesitating between keeping the semantic difference with `None`/`NULL` meaning that no tax applies, but after thinking about it some more I can't really argue for it: Will it make querying for tax reports easier? not really, even though indexing a decimal won't make much sense, the queries will most likely use tax type and tax country, not tax rate. Does it make sense from the legal standpoint, e.g. to a donation 0% tax applies, but tax does not apply when selling to US? Not sure, but then again you are probably right and we won't be using this semantics. Will get rid of the `Optional[]` and always return a `Decimal` here.
96	I guess you meant `ProductType`, right? (moved it into the class)
113–117	Doesn't work well here, in my opinion, because of the nested condition: adds at least one identical `return Decimal(0)` statement
114–115	At this point we assume that `buyer_country_code` is a valid country code (it was validated elsewhere, e.g. in the form), and `vat_rules` being false-y means that there's no applicable VAT-rule.
129–130	If `Optional` is removed this error is raised: `ValueError: No mandatory attributes allowed after an attribute with a default value or factory. Attribute in question: Attribute(name='tax',...` `attr` doesn't allow to initialise the `Taxable` without some value of `tax`, not sure there's a way to have `tax: Money` given that `Money` has to be passed the a currency value (`Money(0)` is not a possible value).

Address the review comment

Anna Sirota (railla) updated this revision to Diff 37200.May 18 2021, 12:54 PM

Anna Sirota (railla) marked an inline comment as done.

Anna Sirota (railla) planned changes to this revision.May 18 2021, 2:26 PM

Make tax_rate NOT NULL, add default value of 0, for consistency with typing;
Add Dropin UI style overrides (TODO(anna): cherry-pick into master before requesting another review, out of scope);
Regenerate migrations to have one single clean migration before ready to merge.

rLe61b33bc7c87: get_plan_variation should fail or return a PlanVariation
rLdefd75288fe9: Refactor TaxType and ProductType as Enums

rLf7520ad00bb3: Show VATid in the admin

Anna Sirota (railla) planned changes to this revision.May 19 2021, 2:46 PM

Anna Sirota (railla) added inline comments.

looper/templatetags/looper.py
36–42	This code has been updated: at least there's no invalid call with `None`s. I'd rather not change it further because it will most certainly change again later in a separate patch, polishing GeoIP features, as `TODO` suggests.

rL69c1fd967d13: Move TaxType on top of the module, to have a logical order of tax classes
rL96c45b1eaea5: Move docstring about tax calculations into the class that actually does them
rL0b9f8666a76c: Minor change
rL724ae6d00e3b: Move display_name to TaxType; fewer confusing attributes in Taxable
rLf7520ad00bb3: Show VATid in the admin

Anna Sirota (railla) marked 2 inline comments as done.May 19 2021, 3:45 PM

Anna Sirota (railla) added inline comments.

looper/taxes.py
23	Refactored both `ProductType` and `TaxType` into enums, seems to be indeed better all in all.
51	This comment was moved to `ProductType`'s docstring, because there is where the described tax logic actually resides.

Anna Sirota (railla) updated this revision to Diff 37299.May 20 2021, 9:51 AM

Sybren A. Stüvel (sybren) requested changes to this revision.May 21 2021, 3:50 PM

Sybren A. Stüvel (sybren) added inline comments.

looper/models.py
744	Add `-> Taxable` so that MyPy knows it's a typed function.
1501	`del` is unnecessary here, as `order_tmp` is a local name and will go out of scope one line later.
looper/taxes.py
97–104	Cognitive complexity can be reduced by flipping the conditions and not nesting the conditionals.
135	"including the tax" is not correct. When passed to the initialiser, it's correct, but in the case of a reverse-charge the meaning of this variable changes to "excluding the tax". Such a change of semantics is a nice source of confusion & bugs, so I would suggest refactoring this code to remove this. Maybe a property 'final_price' could be added, which would get the post-init value of 'price'; that way there is clear separation between the two.
137	It should be documented whether 100% = 100 or 100% = 1.
looper/templatetags/looper.py
36–42	👍

This revision now requires changes to proceed.May 21 2021, 3:50 PM

rLc57e2a86e6bc: Specify what tax_rate value is
rLba5843157cdb: Resolve confusion with Taxable.price with Taxable.price_with_tax
rLaff125f4ff0e: Flip some conditions
rL8fd85d526150: Useless del removed

Anna Sirota (railla) marked 12 inline comments as done.May 21 2021, 6:11 PM

Anna Sirota (railla) added inline comments.

looper/taxes.py
135	Added `Taxable.price_with_tax` which stores the unchaged price, this way there's no need to change any code in `blender-studio` (and obvious naming seems to help too). Also, after trying to figure out how to get rid of `Option[Money]` for `price` and `tax`, which are only `Optional` because these attributes are calculated after the `__init__` and `Money` cannot have a default value without currency, turned both into `@property`s. This seems to make more sense overall: makes typing more correct and makes it obvious that these values are calculated.

LGTM. I had two unit tests fail on my local machine (see P2135), but those didn't seem to be related to this patch.

looper/taxes.py
135	Nice solution!

This revision is now accepted and ready to land.May 25 2021, 12:20 PM

Landed in rLb2787d9860cd: VAT calculations and other tax-related functionality, closes T86492

VAT calculations for subscriptions
ClosedPublic
Actions

Details

Goal

Implementation details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 37260

looper/admin.py

looper/migrations/0058_adjust_tax_fields.py

looper/models.py

looper/money.py

looper/taxes.py

looper/templatetags/looper.py

looper/tests/test_customer.py

looper/tests/test_moneyfield.py

looper/tests/test_orders.py

looper/tests/test_subscriptions.py

looper/tests/test_taxes.py

poetry.lock

pyproject.toml

VAT calculations for subscriptionsClosedPublicActions

Details

Goal

Implementation details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 37260

looper/admin.py

looper/migrations/0058_adjust_tax_fields.py

looper/models.py

looper/money.py

looper/taxes.py

looper/templatetags/looper.py

looper/tests/test_customer.py

looper/tests/test_moneyfield.py

looper/tests/test_orders.py

looper/tests/test_subscriptions.py

looper/tests/test_taxes.py

poetry.lock

pyproject.toml

VAT calculations for subscriptions
ClosedPublic
Actions

Revision Contents
Changeset List