feat: add national_id validation for reference_id

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

eox_nelp/api_clients/certificates.py

@@ -59,7 +59,7 @@ def create_external_certificate_request(certificate_data):
             path = "Certificates"
             user = certificate_data["user"]
             payload = {
-                "reference_id": f"nelc-openedx-lms-cert-{certificate_data['id']}",
+                "reference_id": certificate_data["reference_id"],
                 "date": {
                     "issuance": certificate_data["created_at"],
                     "expiration": None,

eox_nelp/api_clients/tests/tests_certificates.py

@@ -11,6 +11,7 @@
 
 from eox_nelp.api_clients.certificates import ExternalCertificatesApiClient
 from eox_nelp.api_clients.tests import TestBasicAuthApiClientMixin
+from eox_nelp.signals.utils import generate_reference_id
 
 
 class TestExternalCertificatesApiClient(TestBasicAuthApiClientMixin, unittest.TestCase):
@@ -33,12 +34,14 @@ def test_create_certificate(self, post_mock):
         }
         post_mock.return_value = expected_value
         user = {
-            "national_id": "10224587",
+            "national_id": "1222555888",
             "english_name": " Testing",
             "arabic_name": "اختبارات",
         }
+        course_id = "course-v1:FutureX+guide+2023"
         data = {
             "id": "124ABC",
+            "reference_id": generate_reference_id(user["national_id"], course_id),
             "created_at": timezone.now(),
             "expiration_date": timezone.now() + timezone.timedelta(days=365),
             "grade": 10,

eox_nelp/signals/tests/test_utils.py

@@ -5,6 +5,7 @@
 """
 import unittest
 
+from ddt import data, ddt
 from django.conf import settings
 from django.contrib.auth import get_user_model
 from django.test import override_settings
@@ -17,6 +18,9 @@
 
 User = get_user_model()
 
+WRONG_NATIONAL_IDS = [0, "", "324234", "VADER", "3666888999", "166688899", "فيدر"]
+SAML_EXTRA_ASSOCIATIONS_LIST = ["1666888998ASDF", "1222666444a6ca", "12226664443242344334534543", "1222666444#@$%"]
+
 
 class UserHasPassingGradeTestCase(unittest.TestCase):
     """Test class for function `_user_has_passing_grade`"""
@@ -37,13 +41,14 @@ def test_call_user_has_passing_grade(self, course_grade_factory_mock):
         course_grade_factory_mock().read.assert_called_with(user, course_key=CourseKey.from_string(course_id))
 
 
+@ddt
 class GenerateExternalCertificateDataTestCase(unittest.TestCase):
     """Test class for function `_generate_external_certificate_data`"""
 
     def setUp(self):
         """ Set common conditions for test cases."""
         self.user, _ = User.objects.get_or_create(
-            username="10024578",
+            username="1333666888",
         )
         self.certificate_data = CertificateData(
             user=UserData(
@@ -84,6 +89,7 @@ def test_generate_certificate_data(self, generate_certificate_mock, passing_mock
 
         expected_value = {
             "id": certificate.id,
+            'reference_id': '1333666888~course-v1:test+Cx105+2022_T4',
             "created_at": str(time.date()),
             "expiration_date": None,
             "grade": self.certificate_data.grade * 100,
@@ -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 = {
             "time": timezone.now(),
             "certificate_data": self.certificate_data,
         }
 
-        self.assertRaises(KeyError, _generate_external_certificate_data, **data)
+        self.assertRaises(KeyError, _generate_external_certificate_data, **external_certificate_data)
+
+    @override_settings(EXTERNAL_CERTIFICATES_GROUP_CODES={"course-v1:test+Cx105+2022_T4": "ABC123"})
+    @patch("eox_nelp.signals.utils._user_has_passing_grade")
+    @patch("eox_nelp.signals.utils.GeneratedCertificate")
+    @data(*WRONG_NATIONAL_IDS)
+    def test_invalid_mational_id(self, wrong_national_id, generate_certificate_mock, passing_mock):
+        """This tests when the user has an invalid NationalId.
+
+        Expected behavior:
+            - Raise ValueError
+        """
+        certificate = Mock()
+        certificate.id = 85
+        generate_certificate_mock.objects.get.return_value = certificate
+        passing_mock.return_value = True
+        wrong_user, _ = User.objects.get_or_create(
+            username=wrong_national_id,
+        )
+        certificate_data = CertificateData(
+            user=UserData(
+                pii=UserPersonalData(
+                    username=wrong_user.username,
+                    email="[email protected]",
+                    name="Harry Potter",
+                ),
+                id=wrong_user.id,
+                is_active=True,
+            ),
+            course=CourseData(
+                course_key=CourseKey.from_string("course-v1:test+Cx105+2022_T4"),
+            ),
+            mode="audit",
+            grade=0.85,
+            current_status="non-passing",
+            download_url="",
+            name="",
+        )
+        external_certificate_data = {
+            "time": timezone.now(),
+            "certificate_data": certificate_data,
+        }
+
+        self.assertRaisesRegex(
+            ValueError,
+            f"The username or national_id: {wrong_user.username} doesnt match national ID regex",
+            _generate_external_certificate_data,
+            **external_certificate_data,
+        )
+
+    @override_settings(EXTERNAL_CERTIFICATES_GROUP_CODES={"course-v1:test+Cx105+2022_T4": "ABC123"})
+    @patch("eox_nelp.signals.utils._user_has_passing_grade")
+    @patch("eox_nelp.signals.utils.GeneratedCertificate")
+    @data(*SAML_EXTRA_ASSOCIATIONS_LIST)
+    def test_generate_certificate_data_saml_extra_association(
+        self,
+        saml_extra_association,
+        generate_certificate_mock,
+        passing_mock
+    ):
+        """This tests the normal behavior ofa user with saml_extra_association
+        the method `_generate_external_certificate_data`
+
+        Expected behavior:
+            - Result is as the expected value
+            - GeneratedCertificate mock is called with the right parameters.
+            - _user_has_passing_grade is called with the right parameters.
+        """
+        time = timezone.now()
+        certificate = Mock()
+        certificate.id = 99
+        generate_certificate_mock.objects.get.return_value = certificate
+        passing_mock.return_value = True
+        saml_association_user, _ = User.objects.get_or_create(
+            username=saml_extra_association,
+        )
+        certificate_data = CertificateData(
+            user=UserData(
+                pii=UserPersonalData(
+                    username=saml_association_user.username,
+                    email="[email protected]",
+                    name="Harry Potter",
+                ),
+                id=saml_association_user.id,
+                is_active=True,
+            ),
+            course=CourseData(
+                course_key=CourseKey.from_string("course-v1:test+Cx105+2022_T4"),
+            ),
+            mode="audit",
+            grade=0.88,
+            current_status="non-passing",
+            download_url="",
+            name="",
+        )
+        national_id = saml_association_user.username[:10]
+
+        expected_value = {
+            "id": certificate.id,
+            'reference_id': f'{national_id}~course-v1:test+Cx105+2022_T4',
+            "created_at": str(time.date()),
+            "expiration_date": None,
+            "grade": certificate_data.grade * 100,
+            "is_passing": True,
+            "group_code": settings.EXTERNAL_CERTIFICATES_GROUP_CODES[str(self.certificate_data.course.course_key)],
+            "user": {
+                "national_id": national_id,
+                "english_name": certificate_data.user.pii.name,
+                "arabic_name": "",
+            }
+        }
+
+        result = _generate_external_certificate_data(time, certificate_data)
+
+        self.assertEqual(result, expected_value)
+        generate_certificate_mock.objects.get.assert_called_once_with(
+            user=saml_association_user,
+            course_id=certificate_data.course.course_key,
+        )
+        passing_mock.assert_called_once_with(
+            saml_association_user,
+            str(certificate_data.course.course_key)
+        )

eox_nelp/signals/utils.py

@@ -10,6 +10,8 @@
 from eox_core.edxapp_wrapper.grades import get_course_grade_factory
 from opaque_keys.edx.keys import CourseKey
 
+from eox_nelp.utils import is_valid_national_id
+
 CourseGradeFactory = get_course_grade_factory()
 GeneratedCertificate = get_generated_certificate()
 User = get_user_model()
@@ -39,16 +41,18 @@ def _generate_external_certificate_data(time, certificate_data):
     group_codes = getattr(settings, "EXTERNAL_CERTIFICATES_GROUP_CODES", {})
     course_id = str(certificate_data.course.course_key)
     extra_info = getattr(user, "extrainfo", None)
+    national_id = user.username[:10]  # saml association extra filter
 
     return {
         "id": certificate.id,
+        "reference_id": generate_reference_id(national_id, course_id),
         "created_at": str(time.date()),
         "expiration_date": None,
         "grade": float(certificate_data.grade) * 100,
         "is_passing": _user_has_passing_grade(user, course_id),
         "group_code": group_codes[course_id],
         "user": {
-            "national_id": user.username,
+            "national_id": national_id,
             "english_name": certificate_data.user.pii.name,
             "arabic_name": extra_info.arabic_name if extra_info else "",
         }
@@ -67,3 +71,20 @@ def _user_has_passing_grade(user, course_id):
     course_grade = CourseGradeFactory().read(user, course_key=CourseKey.from_string(course_id))
 
     return course_grade.passed
+
+
+def generate_reference_id(national_id, course_id):
+    """Generate string of reference_id shape.
+    Args:
+        national_id<string>: Username of the user(national_id).
+        course_id<str>: Unique course identifier.
+
+    Returns:
+        reference_id<str>: The string of reference_id representation.
+
+    Raise:
+        ValueError: National Id is not valid.
+    """
+    is_valid_national_id(national_id, raise_exception=True)
+
+    return f"{national_id}~{course_id}"

eox_nelp/utils.py

@@ -1,6 +1,9 @@
 """Utils that can be used for the plugin project"""
+import re
 from copy import copy
 
+NATIONAL_ID_REGEX = r"^[1-2]\d{9}$"
+
 
 def map_instance_attributes_to_dict(instance, attributes_mapping):
     """Create a dictionary that represents some fields or attributes of a instance based on
@@ -38,3 +41,40 @@ def map_instance_attributes_to_dict(instance, attributes_mapping):
         instance_dict[extra_field] = extra_value
 
     return instance_dict
+
+
+def check_regex(string, regex):
+    """Checks if the string matches the regex.
+
+    Args:
+        string: The string to check.
+        regex: The regex to match against.
+
+    Returns:
+        True if the string matches the regex, False otherwise.
+    """
+    pattern = re.compile(regex)
+
+    return pattern.match(string) is not None
+
+
+def is_valid_national_id(national_id, raise_exception=False):
+    """Validate if a national_id has the shape of a national_id
+
+    Args:
+        national_id: The string of national_id to check.
+
+    Returns:
+        True if the national_id is ok, False otherwise.
+
+    Raise:
+    ValueError: This will be raised when the username are excluded dont match national Id regex.
+    """
+    check_national_id = check_regex(national_id, NATIONAL_ID_REGEX)
+
+    if raise_exception and not check_national_id:
+        raise ValueError(
+            f"The username or national_id: {national_id} doesnt match national ID regex ({NATIONAL_ID_REGEX})",
+        )
+
+    return check_national_id