Conversation
b8c8646 to
514fb69
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #748 +/- ##
==========================================
+ Coverage 93.09% 93.11% +0.01%
==========================================
Files 368 370 +2
Lines 19624 19706 +82
==========================================
+ Hits 18269 18349 +80
- Misses 919 920 +1
- Partials 436 437 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a new Finland (FI) tax regime to GOBL, including VAT categories/rates, Y-tunnus (Business ID) validation, invoice requirements, and supporting generated data/examples.
Changes:
- Registered the new
firegime and added FI to the regime-code JSON schema enum. - Implemented FI VAT categories (with historical rates) plus FI-specific validation/normalization (tax identity + invoice requirements) with tests.
- Added FI example invoice documents and generated regime definition data, plus updated contributor guidance and changelog.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
regimes/regimes.go |
Registers the new Finland regime via blank import. |
regimes/fi/fi.go |
Defines FI regime metadata and dispatches validation/normalization. |
regimes/fi/tax_categories.go |
Declares FI VAT categories and historical rate values. |
regimes/fi/tax_identity.go |
Adds Y-tunnus checksum validation logic. |
regimes/fi/invoices.go |
Enforces FI invoice supplier Tax ID requirements. |
regimes/fi/tax_identity_test.go |
Tests for FI tax ID validation and normalization. |
regimes/fi/invoices_test.go |
Tests invoice validation requirement and date-based VAT calculations. |
examples/fi/invoice-fi-fi.yaml |
Adds an FI invoice example input document. |
examples/fi/out/invoice-fi-fi.json |
Adds generated JSON output for the FI example. |
data/schemas/tax/regime-code.json |
Adds "FI" to the schema enum list. |
data/regimes/fi.json |
Adds generated FI regime definition data. |
CONTRIBUTING.md |
Updates regime-addition guidance and references. |
CHANGELOG.md |
Notes the new FI regime addition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
samlown
left a comment
There was a problem hiding this comment.
Very nice! Not much to change here. We just need to double check the situation with Finish Tax Identities to ensure a VAT ID is required for any invoice.
b7f3cfd to
3364e4c
Compare
|
Thanks for the early review, @samlown, caught me in the middle of some final polish. I've addressed your (and Copilot's) comments and this is now ready for a final review. |
|
Sorry! Too keen :-) I should have waited for the non-draft state! Just let me know when ready. |
|
Since #750 has been approved, I've amended the historic rates definitions so they reflect effective dates. I've also removed |
# Conflicts: # CHANGELOG.md
28c7ddb to
567561d
Compare
|
Hey @samlown, I've updated this branch with master and ported the regime to use validation rules. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func validateTaxCode(value any) bool { | ||
| val := value.(cbc.Code).String() | ||
|
|
||
| for _, re := range taxCodeRegexps { | ||
| if re.MatchString(val) { | ||
| return validateTaxCodeChecksum(val) | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
validateTaxCode uses a direct type assertion (value.(cbc.Code)), which will panic if the rules engine ever calls the test with a non-cbc.Code value (e.g., due to reflection/indirection changes). Other regimes defensively do code, ok := value.(cbc.Code) and return false when the assertion fails; mirroring that pattern would make this validator more robust.
There was a problem hiding this comment.
Oh, I had this but lost it during a small refactor. Adding it back.
| Both regimes and addons use a `switch obj := doc.(type)` pattern to dispatch normalization and validation to the | ||
| appropriate handler based on the document type. See the `Normalize` and `Validate` functions in any existing regime for an example. |
There was a problem hiding this comment.
This section references a Validate function in regime packages, but there are no func Validate(doc any) implementations under regimes/ (validation is done via rules.Register(...) + rules.Validate(...)). Please update the docs to point at the rules registration pattern instead of a non-existent Validate dispatcher.
| Both regimes and addons use a `switch obj := doc.(type)` pattern to dispatch normalization and validation to the | |
| appropriate handler based on the document type. See the `Normalize` and `Validate` functions in any existing regime for an example. | |
| Normalization in regimes and addons commonly uses a `switch obj := doc.(type)` pattern to dispatch to the | |
| appropriate handler based on the document type. Validation is rule-based: regimes register validators with | |
| `rules.Register(...)`, and validation is executed via `rules.Validate(...)` rather than a regime-level | |
| `Validate(doc any)` dispatcher. See existing regime `Normalize` functions and rule registrations for examples. |
There was a problem hiding this comment.
Updated this section to reflect the current rules-based validations.
| A minimal regime needs four files: | ||
|
|
||
| - `<cc>.go`: declares the regime and defines its metadata. | ||
| - `tax_categories.go`: declares tax categories with rates and, optionally, historical values. | ||
| - `tax_identity.go`: implements tax ID normalization, validation, classification, etc. | ||
| - `invoices.go`: implements invoice-level validation (e.g. supplier must have a tax ID). | ||
|
|
||
| Depending on the requirements you may also need: | ||
|
|
There was a problem hiding this comment.
The guide says a minimal regime needs an invoices.go file, but there are no invoices.go files in regimes/ (invoice rules, when needed, are typically in files like bill_invoices.go, and some regimes have no invoice-specific rules at all). Please adjust the wording to make invoice-level validation optional and/or align the filename guidance with the existing convention.
| A minimal regime needs four files: | |
| - `<cc>.go`: declares the regime and defines its metadata. | |
| - `tax_categories.go`: declares tax categories with rates and, optionally, historical values. | |
| - `tax_identity.go`: implements tax ID normalization, validation, classification, etc. | |
| - `invoices.go`: implements invoice-level validation (e.g. supplier must have a tax ID). | |
| Depending on the requirements you may also need: | |
| A minimal regime needs three files: | |
| - `<cc>.go`: declares the regime and defines its metadata. | |
| - `tax_categories.go`: declares tax categories with rates and, optionally, historical values. | |
| - `tax_identity.go`: implements tax ID normalization, validation, classification, etc. | |
| Depending on the requirements you may also need: | |
| - `bill_invoices.go` (or another domain-specific file): implements invoice-level validation when the regime requires it (e.g. supplier must have a tax ID). |
There was a problem hiding this comment.
Amended this section to reference bill_invoices.go (adopted across many regimes) instead of invoices.go, and org_identities.go instead of identities.go.
Summary
Sources
Pre-Review Checklist
go generate .to ensure that the Schemas and Regime data are up to date.And if you are part of the org: