fix: UK VAT service 404 (not found) misreported as service outage#15
Draft
akinsteph wants to merge 1 commit into
Draft
fix: UK VAT service 404 (not found) misreported as service outage#15akinsteph wants to merge 1 commit into
akinsteph wants to merge 1 commit into
Conversation
HMRCRegistry.check_vat_number returned the generic "nondeterministic" outage signal for a 404 response, even though a 404 is a deterministic "VAT number not found" result from the HMRC API. Add a dedicated 404 branch before the existing status/content-type check so 404s are reported as invalid without the outage signal, while timeouts, 500s, and malformed responses still trigger the outage path unchanged. Fixes #14 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@akinsteph Looks good we would be more confortable with a test on Monies staging. Create an issue for Monies (update Pyvat) and PR pointing to this Pyvat commit and test with a UK/GB VAT number: Monies staging to test Current behavior on Monies production: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #14.
HMRCRegistry.check_vat_numbertreated an HTTP 404 from the HMRC VAT-check API exactly like a generic outage (timeout, 500, malformed response, wrong content type) — logging a"Response is nondeterministic..."line and returningis_valid=Falsewith no way for a caller to distinguish "VAT number not found" from "the service is down."A 404 from this API is a deterministic answer, not an outage: the VAT number does not exist in HMRC's registry. This PR adds a dedicated branch that catches
status_code == 404before the existing generic status/content-type check, logs a plain "VAT number not found (404 NOT_FOUND)" line instead, and returnsis_valid=Falsewith no outage/nondeterministic signal.All other error paths (timeouts, 500s, non-JSON content type, other non-200 statuses) are untouched, so downstream outage detection (e.g. wp-media/monies, which greps
log_linesfor"Response is nondeterministic") keeps working exactly as before.Changes
pyvat/registries.py: added aresponse.status_code == 404branch inHMRCRegistry.check_vat_number, placed immediately before the existingif response.status_code != 200 or ...outage check. No signature or public API changes.tests/test_hmrc_registry.py(new): regression tests for the HMRC registry usingunittest.mock.patchonrequests.get(no live HMRC calls), covering:is_valid=False, nonondeterministic/exception/ServerErrorsignal inlog_linesis_valid=False,nondeterministicpresent inlog_lines(outage path still works)targetpayload →is_valid=True,business_name/business_addresspopulatednondeterministicTimeout→ still flagged as a timed-out requestAcceptance criteria
is_valid=Falsewithout any exception,ServerError, ornondeterministic/service-unavailable signal inresult.log_lines.nondeterministiclog line.Test plan
python -m pytest tests/ -v— 21 passed, 2 skipped (pre-existing, network-dependent live-VAT-check tests unrelated to this change).pyvat.check_vat_number()entry point with a mocked 404 response, confirmingis_valid=Falseand no outage signal inlog_lines.flake8run on the changed file — no new violations introduced (pre-existing violations on unrelated lines untouched).Notes
unittest.mock.patchonrequests.getwas used for the new test file, seedingaccess_tokenon the registry instance insetUpso the_authenticate()retry path (which itself makes an HTTP call) doesn't need to be mocked for these tests.Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com