-
Notifications
You must be signed in to change notification settings - Fork 58
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
TaxID: Add support for Spanish CIF #227
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition! Thanks
localstripe/resources.py
Outdated
if type == 'eu_vat': | ||
country = value[0:2] | ||
else: | ||
country = type[0:2].upper() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
if type == 'eu_vat': | |
country = value[0:2] | |
else: | |
country = type[0:2].upper() | |
if type == 'eu_vat': | |
country = value[0:2] | |
elif type in ('nz_gst', 'au_abn'): | |
country = type[0:2].upper() | |
else: | |
raise |
The goal is to avoid "missing" cases if one day we add another value in the list ('eu_vat', 'nz_gst', 'au_abn')
.
(and nz_gst, au_abn
→ nz_gst, au_abn, es_cif
in the next commit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok interesting ! Do you really suggest adding a naked raise
for "missing" case (I thought this was only for re-raising an exception in an except
block), or do you mean something like raise UserError(500, 'Not implemented')
(that we're already using elsewhere in the codebase) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, raise
is a bit brutal, and there is no exception to reraise...
raise UserError
wouldn't be suited, because it's not a user error, it would be a bug on our side (because we already checked type
is amongst valid values a few lines above). We would need something like raise ABugWasIntroducedInOurCodeAndNeedsToBeFixed()
. Since this doesn't exist (yet) in localstripe, I propose assert False # shouldn't happen because type is checked above
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose assert False # shouldn't happen because type is checked above.
Ok, done in last push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting!
One question on the first commit, and the second one looks 👌
@@ -3276,7 +3276,10 @@ def __init__(self, country=None, customer=None, type=None, value=None, | |||
assert type in ('eu_vat', 'nz_gst', 'au_abn') | |||
assert _type(value) is str and len(value) > 10 | |||
if country is None: | |||
country = value[0:2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably just me, but I don't fully understand this commit. More precisely why not just put .upper()
here?
Or said otherwise, why add an if/else
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sample eu_vat
: ATU12345678
(for Austria, country code: AT
)
sample au_abn
: 12345678912
sample nz_gst
: 123456789
(cf https://docs.stripe.com/billing/customer/tax-ids)
We can use the first two characters of the value is the country code only for eu_vat
. For the two others taxIDs, it's the first two characters of the type (and it seems nice to use this), hence the if/else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, my bad, I didn't read the code carefully enough.
Thanks for the explanation!
'au_abn' and 'nz_gst' are numbers with only digits, they don't contain their country code. We can only guess the country from 'eu_vat' taxIDs, so let's adapt the code to better handle AU and NZ taxIDs number. See [the Stripe documentation] for the exact format of these two taxIDs number. [the Stripe documentation]: https://docs.stripe.com/billing/customer/tax-ids
According to Stripe's documentation[^1], two Spanish Customer Tax IDs (`eu_vat` & `es_cif`) are supported. [^1]: https://docs.stripe.com/billing/customer/tax-ids#supported-tax-id-types Let's add the support for Spain's _código de identificación fiscal_ (CIF) tax ID. Even if Stripe doesn't validate this type of Customer Tax ID (unlike EU VAT)[^2], the related dashboard form input only checks if the given value's length is 9. [^2]: https://docs.stripe.com/billing/customer/tax-ids#validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK for me too 👌
TaxID: Correctly set country for non 'eu_vat' taxIDs
'au_abn' and 'nz_gst' are numbers with only digits, they don't contain their
country code.
We can only guess the country from 'eu_vat' taxIDs, so let's adapt the code to
better handle AU and NZ taxIDs number.
See the Stripe documentation for the exact format of these two taxIDs number.
TaxID: Add support for Spanish CIF
According to Stripe's documentation1, two Spanish Customer Tax IDs (
eu_vat
&
es_cif
) are supported.Let's add the support for Spain's código de identificación fiscal (CIF) tax
ID.
Even if Stripe doesn't validate this type of Customer Tax ID (unlike EU
VAT)2, the related dashboard form input only checks if the given value's
length is 9.
Footnotes
https://docs.stripe.com/billing/customer/tax-ids#supported-tax-id-types ↩
https://docs.stripe.com/billing/customer/tax-ids#validation ↩