Skip to content

Conversation

@jpraet
Copy link
Contributor

@jpraet jpraet commented Oct 8, 2025

No description provided.

Copy link
Collaborator

@jflabatBCSS jflabatBCSS left a comment

Choose a reason for hiding this comment

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

I'm not familiar with employerId, I can't be sure your checksum verification are correct.

@jpraet
Copy link
Contributor Author

jpraet commented Oct 9, 2025

I'm not familiar with employerId, I can't be sure your checksum verification are correct.

I based it on https://www.ict-reuse.be/nl/service/smalsutils-validation.
But indeed, it would be good if someone more familiar with this could also review it.

For example, in that library I also found "NssoCuratorRegistrationNumber" which does a different checksum calculation (it adds 5000000 before calculating the checksum). But I don't know if NssoCuratorRegistrationNumber is considered an EmployerId?

@pvdbosch could you ask a review from someone from Smals/RSZ?

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 9, 2025

// Belgif openapi-employment-identifier EmployerId already enforces 197 -> 5999999999 range:
// we only need to validate the checksum here
int checksum = (int) (getValue() % 100);
if (!(checksum == calculateNssoChecksum() || (isInPlaRange() && checksum == calculatePlaChecksum()))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to smalsutils-validation there is overlap between NssoNumber range and PlaNumber range,
so I chose to also allow NSSO checksum when isInPlaRange() is true.

@pvdbosch
Copy link
Contributor

For example, in that library I also found "NssoCuratorRegistrationNumber" which does a different checksum calculation (it adds 5000000 before calculating the checksum). But I don't know if NssoCuratorRegistrationNumber is considered an EmployerId?

I don't think it is. From FedVoc employerId definition: "Definitive or provisional NSSO number, assigned to each registered employer or local or provincial administration."

So this would correspond to NssoNumber+ProvisionalNssoNumber+PplNumber in smalsutils-validation.

@pvdbosch could you ask a review from someone from Smals/RSZ?

I can ask for an analyst to provide answers to business questions, but not for a code review because the analysts are no developers and we're not using this module. Aside from the question about NssoCuratorRegistrationNumber, I can ask to plan adding an equivalent EmployerId validation to smalsutils-validation. Are there any other questions you'd like to be answered?

@jpraet
Copy link
Contributor Author

jpraet commented Oct 24, 2025

I can ask for an analyst to provide answers to business questions, but not for a code review because the analysts are no developers and we're not using this module. Aside from the question about NssoCuratorRegistrationNumber, I can ask to plan adding an equivalent EmployerId validation to smalsutils-validation. Are there any other questions you'd like to be answered?

Maybe a review from a developer that works on smalsutils-validation? I am mainly looking for some assurance that the checksum verification logic has been implemented correctly.

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.

3 participants