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

Fix Italian fiscal code generation #497

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CostantiniMatteo
Copy link

There were some bugs in the generation of the fiscal code that failed validity checks in our codebase:

  • The rule for the three characters in name and surname is: first the consonants, then the vowels, then pad right with Xs if the name is too short (e.g. "Xi" -> "XIX"). The generator did not follow the rule and mixed vowels and consonants. To keep the code simpler the new implementation just uses consonants, therefore only a subset of valid fiscal codes are generated. If you want, I can change it to also add vowels and account for 1 and 2 letter names.
  • Birth dates could've been invalid (e.g. 2020-02-31)
  • Birth day didn't account properly for female sex (male rule is to just use the day number; female rule is to add 40 to that. So allowed ranges are 1-31 and 41-71)
  • The numeric part of town codes for foreign countries is in the range 100-999 and not 000-999

I didn't add any test as I don't know how to properly test those fixes besides some kind of property-based testing and fuzzing, but since it's not used in the codebase I avoided adding it. Let me know if you'd like some kind of test.

  • USAGE.md docs if applicable
  • CHANGELOG.md

@CostantiniMatteo
Copy link
Author

Hi guys, is there anything I can do to help you merge this?

@tobyhinloopen
Copy link
Contributor

I don't know italian standards so I don't know if the results are correct. However, the code LGTM.

The CI is failing for reasons not known to me, I might have to check that out.

@CostantiniMatteo
Copy link
Author

I've never seen this "Expected — Waiting for status to be reported" but apparently a maintainer approval is needed to run the CI, so I guess after the approval the CI will run? 🤷‍♂️

Let me know if I can help!

Copy link
Contributor

@tobyhinloopen tobyhinloopen left a comment

Choose a reason for hiding this comment

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

CI trigger (edit: my review doesn't count / i'm not a maintainer?)

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