Skip to content
This repository has been archived by the owner on Dec 11, 2020. It is now read-only.

Add a swiss social security number (AVS13) generator #1533

Merged
merged 10 commits into from
Sep 3, 2019
Merged

Add a swiss social security number (AVS13) generator #1533

merged 10 commits into from
Sep 3, 2019

Conversation

nhedger
Copy link
Contributor

@nhedger nhedger commented Jul 26, 2018

Hey,

This is a random generator for swiss social security numbers (AVS13). I've made sure the code is correct using make sniff and ran the tests against the new code. It should all be good. I've also added an entry in the readme.md file to document the new provider.

Any feedback is welcome.

Regards.

Copy link
Contributor

@ppelgrims ppelgrims left a comment

Choose a reason for hiding this comment

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

Almost there! Please get back to us on the requested changes or provide feedback then we can take this further from there. Nice work!

* @return string A string of {@see $length} random digits
* @throws \OutOfBoundsException
*/
protected static function generateRandomDigits($length = 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I reckon this kind of method should go into Faker/Provider/Base but you probably could get the same result using numberBetween

* @param string $digits
* @return int
*/
protected static function computeAvs13Checksum($digits)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would probably fit better in the Faker/Calculator namespace, instead of being buried in a localized provider.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you provide a separate test for this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea. Would it be better to create an Ean13 utility class since it's using the same algorithm ?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a Luhn (etc.) calculator class, that does sound like a good idea to group it like that.

* @see https://www.zas.admin.ch/zas/fr/home/partenaires-et-institutions-/navs13.html
* @return string
*/
public function navs13()
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a static method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will update that.

readme.md Show resolved Hide resolved
public function testAvsNumber()
{
$avs = $this->faker->navs13;
$this->assertRegExp('/^756\.([0-9]{4})\.([0-9]{4})\.([0-9]{2})$/i', $avs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to do an assertion about the checksum here? Would that suffice to check the whole number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, a checksum verification should be enough to assert the number is valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, after a bit of thinking, I believe it's still pertinent to assert the format of the generated number since the format is part of the specification. Sure, the checksum would confirm that the number is a valid EAN-13 number but it could validate any EAN-13 that is NOT an AVS13 number.

@nhedger
Copy link
Contributor Author

nhedger commented Aug 21, 2018

Here a brief of the changes I've made :

  • Created a new randomDigitsString method inside Faker/Provider/Base
  • Created a new Ean class under Faker/Calculator. This class allows the validation of EAN-8 and EAN-13 numbers. It is used internally to generate the checkum for the AVS13 number.
  • Created alias functions inside the de_CH and it_CH providers and left the main code inside the fr_CH provider.
  • Set the methods inside the providers as static, as requested.
  • Wrote the tests for everything

@nhedger
Copy link
Contributor Author

nhedger commented Sep 24, 2018

Any updates regarding this PR ?

@Ph0tonic
Copy link

Ph0tonic commented Jun 12, 2019

Hi, any news about this great PR?

@nhedger
Copy link
Contributor Author

nhedger commented Jun 13, 2019

Hi, @Ph0tonic, thanks for your comment. I was waiting for a validation on this PR but I completely forgot about it. @fzaninotto it would be much appreciated if you could take a look at it.

@Ph0tonic
Copy link

Ph0tonic commented Sep 2, 2019

Hi,
Is there any change to make? @fzaninotto I would be gratefull if you could have a look at this PR.

test/Faker/Provider/de_CH/PersonTest.php Outdated Show resolved Hide resolved
test/Faker/Provider/fr_CH/PersonTest.php Outdated Show resolved Hide resolved
test/Faker/Provider/it_CH/PersonTest.php Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@pimjansen
Copy link
Contributor

Thanks for contributing!

@pimjansen pimjansen merged commit 1cc3ca6 into fzaninotto:master Sep 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants