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

feat: add national_id validation for reference_id #87

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

johanseto
Copy link
Collaborator

Description

feat: add national_id validation for reference_id
Also, the shape of reference_id was updated adding the course_id. {username}~{course_id}"

Testing instructions

Tested in stage debug server.

After

wrong username national id

Peek 2023-08-16 13-24

correct username national id

Peek 2023-08-16 13-26

Additional information

Main jira story parent
https://edunext.atlassian.net/browse/FUTUREX-471

Checklist for Merge

  • Tested in a remote environment
  • Updated documentation
  • Rebased master/main
  • Squashed commits

Copy link
Collaborator

@andrey-canon andrey-canon left a comment

Choose a reason for hiding this comment

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

you are only validating with the regex however the requirement is to validate by using the regex and remove extra characters when the username has an extra suffix added by SAML

@johanseto johanseto force-pushed the jlc/update-certificates-referenceid branch 2 times, most recently from 88ba287 to 985c470 Compare August 16, 2023 19:55
@johanseto
Copy link
Collaborator Author

you are only validating with the regex however the requirement is to validate by using the regex and remove extra characters when the username has an extra suffix added by SAML

985c470

@@ -120,9 +126,131 @@ def test_invalid_group_codes(self, generate_certificate_mock, passing_mock):
certificate.id = 85
generate_certificate_mock.objects.get.return_value = certificate
passing_mock.return_value = True
data = {
external_certificate_data = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

indeed the external certificate data is the output of _generate_external_certificate_data, anyways neither adds nor subtracts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pylint throws me an error when I add the decorator data. That is the reason I had to rename there.
https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/redefined-outer-name.html

@data(*WRONG_NATIONAL_IDS)

But actually I thing I changed more xD

eox_nelp/signals/tests/test_utils.py Outdated Show resolved Hide resolved
eox_nelp/signals/utils.py Show resolved Hide resolved
eox_nelp/utils.py Outdated Show resolved Hide resolved
eox_nelp/utils.py Show resolved Hide resolved
eox_nelp/utils.py Show resolved Hide resolved
eox_nelp/utils.py Show resolved Hide resolved
eox_nelp/utils.py Show resolved Hide resolved

def generate_reference_id(national_id, course_id):
"""Generate string of reference_id shape.
username<string>: Username of the user(national_id).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Args ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

eox_nelp/signals/utils.py Outdated Show resolved Hide resolved
Also the shape of reference_id was updated adding the course_id.
`{national_id}~{course_id}"`

feat: saml extra association filter

With this change we ensure to send certificates if the user was created with a saml extra
association that add more sufix data different than current username. So this ensure the national_id
have to be processed with the 10 first chars.

chore: style suggestions from code review

Co-authored-by: Andrey Cañon <[email protected]>

chore: style recommendations
@johanseto johanseto force-pushed the jlc/update-certificates-referenceid branch from b22763b to ab05dba Compare August 16, 2023 20:44
@johanseto johanseto merged commit a4a4283 into master Aug 16, 2023
6 checks passed
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.

2 participants