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

Added GB using a new Register that calls HMRC's VAT API #62

Merged
merged 4 commits into from
Sep 2, 2024

Conversation

mountinash789
Copy link
Contributor

No description provided.

Copy link

@martindur martindur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Nice error handling on the external API.

@davidjb99
Copy link

It would be good to get this merged now it has been approved!

VIES_REGISTRY = ViesRegistry()
HMRC_REGISTRY = HMRCRegistry()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment "VIES registry instance." should be below line 59 and a new one should be added for HMRC_REGISTRY.

@@ -10,6 +10,7 @@
'ES', # Spain.
'FI', # Finland.
'FR', # France.
'GB', # United Kingdom.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set the comment to United Kingdom because it was that on version 1.3.6, felt it made sense to keep it the same.
The API accepts a UK VAT number so maybe it should be UK instead of GB.

https://developer.service.hmrc.gov.uk/api-documentation/docs/api/service/vat-registered-companies-api/1.0

@@ -243,6 +243,14 @@ def get_vat_rate(self, item_type):
return super(MtVatRules, self).get_vat_rate(item_type)


class GbVatRules(ConstantEuVatRateRules):
"""VAT rules for United Kingdom.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "Great Britain" is we use "GB".

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here: https://www.iso.org/obp/ui/#iso:code:3166:GB

GB is the Alpha 2 Code however it refers to the United Kingdom of Great Britain and Northern Ireland usually referred to as just the United Kingdom or UK. Using Great Britain would exclude Northern Ireland and Northern Ireland business do get GB VAT numbers therefore the United Kingdom is the correct form in this instance.

@martinleblanc
Copy link
Member

@mountinash789 Thank you for contributing, Rowan. I added a few comments regarding comments. It should be good to merge when these are updated.

@MathieuLamiot
Copy link
Collaborator

Hello, do you have an update about this PR? Any chance to have it released soon? Thanks

@MathieuLamiot
Copy link
Collaborator

MathieuLamiot commented Aug 12, 2024

Hello, are there any updates or follow-up actions on this?
If it can help, we are using this branch (through a fork) for our e-commerce apps since Nov. 2023 and it works like a charm.

@martinleblanc
Copy link
Member

@MathieuLamiot Ok, I'll merge this one. Can you help resolve the conflicts and update from Master?

@MathieuLamiot
Copy link
Collaborator

MathieuLamiot commented Sep 2, 2024

I don't have write access to oraclefinance so:

  • I created a branch checking out oraclefinance-pyvat master
  • Merged this repo's master into it and fixed conflicts.
  • Check differences with the code we currently use in production: only linter stuff
  • Fixed a comment based on the reviews above (but keeping UK instead of GB as per the discussions above)
  • Open this PR: Added GB using a new Register that calls HMRC's VAT API - fixed conflicts #74

@martinleblanc martinleblanc merged commit acefd34 into iconfinder:master Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants