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

Update the logic to adapt for the new UK VAT endpoint #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

CrochetFeve0251
Copy link

@CrochetFeve0251 CrochetFeve0251 commented Mar 4, 2025

Description

Fixes #149

Fix the bug by using the new API from gov.uk where it returns a 404.

For that we had to upgrade the library to use the version 2 of the API with Oauth authentification.

Type of change

  • New feature (non-breaking change which adds functionality).
  • Bug fix (non-breaking change which fixes an issue).
  • Enhancement (non-breaking change which improves an existing functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as before).
  • Sub-task of #(issue number)
  • Chore
  • Release

Detailed scenario

What was tested

Describe the scenarios that you tested, and specify if it is automated or manual. For manual scenarios, provide a screenshot of the results.

The new code was tested manually on the testing env using this script:

from pyvat.registries import HMRCRegistry

r = HMRCRegistry()

res =  r.check_vat_number('553557881', 'GB', True)

print(res.is_valid)

Then I used the keys to test both a valid VAT number and an invalid one in production and testing.

How to test

Describe how the PR can be tested so that the validator can be autonomous: environment, dependencies, specific setup, steps to perform, API requests, etc.

Use the script and set the env variables:
PYVAT_UK_CLIENT_ID=
PYVAT_UK_CLIENT_SECRET=

You need to find a client in WP Rocket with a valid VAT and use a fake VAT as such as 123456789 to make sure both cases are covered.

Technical description

Documentation

Explain how this code works. Diagrams & drawings are welcome.

New dependencies

List any new dependencies that are required for this change.

Risks

List possible performance & security issues or risks, and explain how they have been mitigated.

Mandatory Checklist

Code validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Code style

  • I wrote a self-explanatory code about what it does.
  • I protected entry points against unexpected inputs.
  • I did not introduce unnecessary complexity.
  • Output messages (errors, notices, logs) are explicit enough for users to understand the issue and are actionnable.

Unticked items justification

This is a library with few tests and a fork.
If some mandatory items are not relevant, explain why in this section.

Additional Checks

  • In the case of complex code, I wrote comments to explain it.
  • When possible, I prepared ways to observe the implemented system (logs, data, etc.).
  • I added error handling logic when using functions that could throw errors (HTTP/API request, filesystem, etc.)

@CrochetFeve0251 CrochetFeve0251 self-assigned this Mar 4, 2025
@CrochetFeve0251 CrochetFeve0251 requested a review from a team March 4, 2025 11:22
@nicomollet nicomollet requested a review from Copilot March 5, 2025 15:39
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR fixes the bug related to the new HMRC VAT validation endpoint and updates naming conventions from “United Kingdom” to “Great Britain”.

  • Updated HMRCRegistry to use separate base URLs and added explicit authentication logic with OAuth.
  • Renamed “GB” comments and docstrings in both the country definitions and VAT rules to reflect the updated naming.
  • Reordered instantiation of HMRCRegistry in init.py for improved clarity.

Reviewed Changes

File Description
pyvat/registries.py Updated HMRCRegistry logic for new API endpoints and OAuth flow.
pyvat/countries.py Updated comment for 'GB' to “Great Britain”.
pyvat/vat_rules.py Updated VAT rules docstring to use “Great Britain” instead of “United Kingdom”.
pyvat/init.py Reordered instantiation of HMRCRegistry.

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment on lines +216 to 219
url = "{0}/organisations/vat/check-vat-number/lookup/".format(base_url)
headers = self._authentication_headers()
response = requests.get(
url + vat_number,
Copy link
Preview

Copilot AI Mar 5, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider using a URL joining function (such as urllib.parse.urljoin) for more robust URL construction and to avoid potential issues with missing or extra slashes.

Suggested change
url = "{0}/organisations/vat/check-vat-number/lookup/".format(base_url)
headers = self._authentication_headers()
response = requests.get(
url + vat_number,
url = urljoin(base_url, "/organisations/vat/check-vat-number/lookup/")
headers = self._authentication_headers()
response = requests.get(
urljoin(url, vat_number),

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Copy link

@nicomollet nicomollet left a comment

Choose a reason for hiding this comment

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

Approved (FYI this branch is on your personal GH account)

Can this be deployed somehow to Monies staging?
Can we change Monies requirements to use this branch https://github.com/wp-media/monies/blob/develop/requirements.txt

@CrochetFeve0251
Copy link
Author

CrochetFeve0251 commented Mar 6, 2025

@nicomollet we need to add the env secrets into the k8 namespaces first let me handle that

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.

2 participants