-
Notifications
You must be signed in to change notification settings - Fork 21
Add Sweden regime #518
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
Add Sweden regime #518
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.
Great first attempt and especially documentation! Also good to see validations for some of the more local identities.
regimes/se/invoices.go
Outdated
| validation.Field(&party.Addresses, | ||
| validation.Required, | ||
| ), |
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.
We generally try to avoid sweeping assumptions about the presence of data. I'd expect the address to be required in a specific format and circumstance, rather than in all invoices and contexts for Swedish invoices.
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.
If I understood the specs correctly, they do not give any requirements for the format of the address itself, but it must be present for all invoices and parties involved.
Additionally, neither party need be located in Sweden, only (at least one of them) registered for tax. I may be mistaken, however, so I'll double check and come back to you on that
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.
The question is which specs you're looking at. Tax regimes should reflect what is defined in the law as opposed to any specific format, its very unlikely that the law requires customer address details, but if it does, a comment that links to that requirement would be very useful.
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.
For regular invoices, yes, both customer and supplier addresses are necessary. They explicitly ruled it, it seems (both in Swedish).
For simplified invoices, however, customer is omitted and supplier can be identified exclusively by VAT ID (in Swedish)
Update: I've been working on adding support for simplified invoices and currently the bill.Invoice itself has a check for Supplier.Name that impedes Tax ID-only simplified invoices. For now, I'll add a Name to the example I'm using, but we should allow overriding that rule for cases like this.
|
@samlown How can I get the |
1fcdecc to
15aa758
Compare
Out directories are generated with the |
I may be missing something. I also tried in rm ./examples/*/out
go generate . |
|
|
15aa758 to
e81cde1
Compare
Even when the Running |
e81cde1 to
dbc57a0
Compare
|
@samlown What's left for this to be merged? |
dbc57a0 to
08ddc6e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #518 +/- ##
==========================================
+ Coverage 92.17% 92.24% +0.07%
==========================================
Files 297 302 +5
Lines 14555 14742 +187
==========================================
+ Hits 13416 13599 +183
- Misses 819 821 +2
- Partials 320 322 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f700043 to
9a1fd3c
Compare
|
@samlown This should be ready to merge now. Lmk if you have any feedback! |
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.
Thanks for updating this one @BoscoDomingo! It'd be great to get this into GOBL. I've added a set of comments on details that I think should be fixed before merging.
internal/luhn.go
Outdated
| // [Source] | ||
| // | ||
| // [Source]: https://github.com/luhnmod10/go | ||
| func ValidateLuhn(number string) bool { |
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 see your point, I think the main issue however is polluting the internal package with utility functions. Until we have more use-cases of this, I'd suggest sticking with the current common package associated with Regimes.
An alternative approach would be to create a package in pkg called checksum or similar that provides these utility methods.
Another approach would be to incorporate a validation Rule inside the cbc package alongside the Code type to specifically check the luhn check digit.
|
|
||
| return validation.ValidateStruct(id, | ||
| validation.Field(&id.Code, | ||
| validation.By(func(value any) error { |
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'd split this validation into its own function for clarity and an appropriate name.
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.
The main problem is that 2 fields in this struct (Code and Type) are intertwined. Without an inlined anonymous function, I'm not sure I can access both at once (at least not from what I've seen in the validation package)
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 results in something like this:
return validation.ValidateStruct(id,
validation.Field(&id.Code,
validation.By(func(value any) error {
return validateOrgIdentityCode(value, id)
}),
validation.Skip,
),
)694fa46 to
27ab49e
Compare
27ab49e to
eab9921
Compare
eab9921 to
dcd4090
Compare
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.
Finally ready to merge this one! Many thanks @BoscoDomingo!
Love to see it!! Glad I could be of help <3 |
ℹ️ This PR comes with a sister PR in this repo, #519, which has multiple QoL changes unrelated to this task I made whilst developing it.
I'm still missing documentation in gobl.docs and the
outdir in examples. However, functionality is ready to be checked.First time contribution, feedback is highly welcome!
CountryMap)Pre-Review Checklist
go generate .to ensure that the Schemas and Regime data are up to date.