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

Adding TIN algorithms #400

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Adding TIN algorithms #400

wants to merge 7 commits into from

Conversation

Menendez6
Copy link
Contributor

@Menendez6 Menendez6 commented Oct 22, 2024

Describe your changes

I added TINs algorithms in identities for Austria, France, Great Britain and Poland and I have created a first version of the regime in Ireland.

Checklist before requesting a review

  • I have performed a self-review of my code.
  • I have added thorough tests with at least 90% code coverage.
  • I've modified or created example GOBL documents the show my changes in use, if appropriate.
  • When adding or modifying a tax regime or addon, I've added links to the source of the changes either structured or in the comments.
  • I've run go generate . to ensure that Schemas and Regime data are up to date.
  • All linter warnings have been reviewed and fixed.
  • I've been obsessive with pointer nil checks to avoid panics.
  • Updated CHANGELOG with list of changes.
  • Requested a review from @samlown.

@Menendez6 Menendez6 requested a review from samlown October 22, 2024 15:51
Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 79.21569% with 53 lines in your changes missing coverage. Please review.

Project coverage is 78.89%. Comparing base (74c80f7) to head (05c13b6).

Files with missing lines Patch % Lines
regimes/gb/identities.go 59.52% 23 Missing and 11 partials ⚠️
regimes/at/identitites.go 83.33% 4 Missing and 2 partials ⚠️
regimes/fr/identities.go 78.26% 4 Missing and 1 partial ⚠️
regimes/ie/ie.go 86.48% 5 Missing ⚠️
regimes/ie/tax_identity.go 80.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main     #400    +/-   ##
========================================
  Coverage   78.88%   78.89%            
========================================
  Files         217      223     +6     
  Lines       10722    10967   +245     
========================================
+ Hits         8458     8652   +194     
- Misses       1909     1945    +36     
- Partials      355      370    +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@samlown samlown left a comment

Choose a reason for hiding this comment

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

Lot's going on here... we need consistency on naming, and I'd also try to standardise on using type codes instead of keys for local identities.

Also, patch test coverage IS NOT 90%!

@@ -50,7 +52,10 @@ func Validate(doc any) error {
return validateInvoice(obj)
case *tax.Identity:
return validateTaxIdentity(obj)
case *org.Identity:
return validateTaxNumber(obj)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be:

Suggested change
return validateTaxNumber(obj)
return validateIdentity(obj)

// people that can be included on invoices inside Austria. For international
// sales, the registered VAT number (Umsatzsteueridentifikationsnummer) should
// be used instead.
IdentityKeyTaxNumber cbc.Key = "at-tax-number"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking that these should be Identity types also, in this case ChatGTP suggests STNR. The same could also be applied to Germany (but we don't want to do that yet!)

},
}

func normalizeTaxNumber(id *org.Identity) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There needs to be a generic normalize method. Naming also needs to be consistent, so this should be normalizeIdentity.

}

// validateNino validates the normalized National Insurance Number (NINO).
func validateNino(value interface{}) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd stick to the validateIdentity convention and check the type.

return nil
}

if id.Type == IdentityTypeNINO {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Switches are always clearer in these circumstances.

@@ -72,6 +74,8 @@ func Validate(doc interface{}) error {
return validateTaxIdentity(obj)
case *bill.Invoice:
return validateInvoice(obj)
case *org.Identity:
return validateTaxNumber(obj)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return validateTaxNumber(obj)
return validateIdentity(obj)

{
Value: IdentityTypePESEL.String(),
Name: i18n.String{
i18n.EN: "Tax Number",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this name needs to be a bit more descriptive.

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