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

Estonian local flavor: added business registry code field #135

Merged
merged 5 commits into from
Nov 10, 2015

Conversation

kendas
Copy link
Contributor

@kendas kendas commented Dec 10, 2014

I've added the field to validate the Estonian business registry code (äriregistri registrikood).

Although I couldn't find a standard or a legitimate source for the business registry code structure, it seems to have the same checksum method as the personal identification code. Even though its 8 characters long, the basic math would make sense (just imagine the last 3 non-checksum numbers being 0's). After testing a fair bit of known correct codes, the checksum works.

The EEBusinessRegistryCode field itself behaves in a very similar manner to the existing EEPersonalIdentificationCode field.

@jezdez jezdez added this to the 1.2 milestone Dec 10, 2014

@staticmethod
def ee_checksum(value):
return EEPersonalIdentificationCode.ee_checksum(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

First, note that I'm just a contributor and not a committer, so this is just advice at best.

  1. Personally I don't see any point in creating this shim function, any reason why you want to avoid calling it directly from clean?
  2. I think the docstring in EEPersonalIdentificationCode.ee_checksum needs updating, it should not say "10 digits" any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created the ee_checksum method there just because EEPersonalIdentificationCode.ee_checksum seemed too long, but you are probably right - it would be clearer if it was explicitly called.

As for the docstring - I totally missed that.

@intgr
Copy link
Contributor

intgr commented Dec 10, 2014

Verified against our database of ~2000 companies, works correctly.

Other than the minor quibbles, LGTM. Thanks!

if not match:
raise ValidationError(self.error_messages['invalid_format'])

check = int(match.group(2))
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking again at this, I think this servers no purpose in business code validation, you might as well use value[7] here and remove the parentheses/groups in the regex.

@intgr
Copy link
Contributor

intgr commented Jan 6, 2015

+1 for merge.

@intgr
Copy link
Contributor

intgr commented Jun 8, 2015

@jezdez Can this be merged, please? It's been over 6 months now.

@nagisa
Copy link
Contributor

nagisa commented Jun 8, 2015

The only blocker I see is lack of .. versionadded:: 1.2 in the docstring. As for correctness I’m inclined to trust @intgr.

@intgr
Copy link
Contributor

intgr commented Nov 10, 2015

Please merge this.

@nagisa
Copy link
Contributor

nagisa commented Nov 10, 2015

Sorry for taking so long to merge this. I’ve totally forgotten all about this and github doesn’t send notifications when new commits are added to PRs.

Thanks for submitting this, I will merge.

nagisa added a commit that referenced this pull request Nov 10, 2015
Estonian local flavor: added business registry code field
@nagisa nagisa merged commit 32b391c into django:master Nov 10, 2015
@kendas
Copy link
Contributor Author

kendas commented Nov 11, 2015

Thank you for pointing out the no notification thing @nagisa. In the future I will make a comment on such occasions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants