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: Accept French Postal Services identifiers in forms #505

Merged

Conversation

Chatewgne
Copy link
Contributor

@Chatewgne Chatewgne commented Mar 27, 2024

This PR is intended to fix the following issue : #504

The form validation in FRSIRENENumberMixin is missing a special case from the SIRENE register.

The identifiers for Postal Services need a special validation process and are not currently validated.

Source (in french) : https://www.sirene.fr/static-resources/doc/lettres/lettre-16-novembre-2013.pdf

All Changes

  • Add an entry to the docs/changelog.rst describing the change.

  • Add an entry for your name in the docs/authors.rst file if it's not
    already there.

@Chatewgne Chatewgne force-pushed the accept_french_postal_services_identifiers branch 2 times, most recently from 451a775 to 4632104 Compare March 29, 2024 09:29
@claudep
Copy link
Member

claudep commented Apr 11, 2024

Sorry for the delay for reviewing, could you please add/complement a test for this?

@Chatewgne Chatewgne force-pushed the accept_french_postal_services_identifiers branch from 4632104 to 1baf78b Compare April 22, 2024 08:29
@Chatewgne
Copy link
Contributor Author

Thanks for getting back to me @claudep, I have added the test cases :)

@benkonrath
Copy link
Member

The exception only applies to SIRET, not to SIREN, so this update isn't correct. I think we should remove the mixin and use stdnum to validate both of the fields directly in the form class. This would add few lines of common code to both classes but it makes things clearer.

@Chatewgne Chatewgne force-pushed the accept_french_postal_services_identifiers branch from 1baf78b to f673d11 Compare August 13, 2024 15:40
@Chatewgne Chatewgne marked this pull request as draft August 14, 2024 12:03
@claudep
Copy link
Member

claudep commented Aug 16, 2024

Any reason to turn this PR as a draft?

@Chatewgne Chatewgne force-pushed the accept_french_postal_services_identifiers branch from f673d11 to 8da80b4 Compare August 23, 2024 14:32
@Chatewgne Chatewgne marked this pull request as ready for review August 23, 2024 14:33
@Chatewgne
Copy link
Contributor Author

The exception only applies to SIRET, not to SIREN, so this update isn't correct. I think we should remove the mixin and use stdnum to validate both of the fields directly in the form class. This would add few lines of common code to both classes but it makes things clearer.

I have removed the mixin as suggested and revised validation for both SIRET and SIREN.

Reminder : the SIREN is included in the SIRET (9 first characters)

Validation :

  • SIREN has 9 chars AND validates luhn algorithm
  • SIRET has 14 chars AND validates luhn algorithm OR starts with "356000000" and the sum of the 14 characters is a multiple of 5

Sources :

@benkonrath benkonrath merged commit 085fb2b into django:master Aug 23, 2024
7 checks passed
@benkonrath
Copy link
Member

Thanks!

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.

3 participants