From 7e3ef93a63db8c0fb95c67a7dbac4e92fadcf18f Mon Sep 17 00:00:00 2001 From: Andy Shultz Date: Thu, 21 Oct 2021 15:45:20 -0400 Subject: [PATCH] feat: make IDV optional for cert generation IDV is on its way to retirement, so it's not going to be necessary for cert generation forever. Introduces a function to combine the honor code flag with IDV to tell cert generation if it should care about a missing verification. Various tests expanded to cover the retired case. The additional calls in test_task_helper.py are caused by one call to fetch course overrides which finds none, and that forces one check of the background flag per student, 71 + 1 + 5 = 77. MST-854 --- .../certificates/generation_handler.py | 16 ++++- .../tests/test_generation_handler.py | 72 +++++++++++++------ .../tests/test_tasks_helper.py | 2 +- 3 files changed, 63 insertions(+), 27 deletions(-) diff --git a/lms/djangoapps/certificates/generation_handler.py b/lms/djangoapps/certificates/generation_handler.py index d475986099..a8b59b7cb9 100644 --- a/lms/djangoapps/certificates/generation_handler.py +++ b/lms/djangoapps/certificates/generation_handler.py @@ -21,6 +21,7 @@ from lms.djangoapps.certificates.utils import has_html_certificates_enabled from lms.djangoapps.grades.api import CourseGradeFactory from lms.djangoapps.instructor.access import is_beta_tester from lms.djangoapps.verify_student.services import IDVerificationService +from openedx.core.djangoapps.agreements.toggles import is_integrity_signature_enabled from openedx.core.djangoapps.content.course_overviews.api import get_course_overview_or_none log = logging.getLogger(__name__) @@ -168,7 +169,7 @@ def _can_generate_certificate_common(user, course_key, enrollment_mode): f'certificate. Certificate cannot be generated.') return False - if not IDVerificationService.user_is_verified(user): + if _required_verification_missing(course_key, user): log.info(f'{user.id} does not have a verified id. Certificate cannot be generated for {course_key}.') return False @@ -217,7 +218,9 @@ def _set_regular_cert_status(user, course_key, enrollment_mode, course_grade): if status is not None: return status - if IDVerificationService.user_is_verified(user) and not _is_passing_grade(course_grade) and cert is not None: + if not _required_verification_missing(course_key, user) \ + and not _is_passing_grade(course_grade) \ + and cert is not None: if cert.status != CertificateStatuses.notpassing: course_grade_val = _get_grade_value(course_grade) cert.mark_notpassing(mode=enrollment_mode, grade=course_grade_val, source='certificate_generation') @@ -238,7 +241,7 @@ def _get_cert_status_common(user, course_key, enrollment_mode, course_grade, cer cert.invalidate(mode=enrollment_mode, source='certificate_generation') return CertificateStatuses.unavailable - if not IDVerificationService.user_is_verified(user) and _has_passing_grade_or_is_allowlisted(user, course_key, + if _required_verification_missing(course_key, user) and _has_passing_grade_or_is_allowlisted(user, course_key, course_grade): if cert is None: _generate_certificate_task(user=user, course_key=course_key, enrollment_mode=enrollment_mode, @@ -397,3 +400,10 @@ def _is_mode_now_eligible(enrollment_mode, cert): if modes_api.is_eligible_for_certificate(enrollment_mode) and not modes_api.is_eligible_for_certificate(cert.mode): return True return False + + +def _required_verification_missing(course_key, user): + """ + Return true if IDV is required for this course and the user does not have it + """ + return not is_integrity_signature_enabled(course_key) and not IDVerificationService.user_is_verified(user) diff --git a/lms/djangoapps/certificates/tests/test_generation_handler.py b/lms/djangoapps/certificates/tests/test_generation_handler.py index 6b32042b6b..c590c192cf 100644 --- a/lms/djangoapps/certificates/tests/test_generation_handler.py +++ b/lms/djangoapps/certificates/tests/test_generation_handler.py @@ -36,11 +36,13 @@ BETA_TESTER_METHOD = 'lms.djangoapps.certificates.generation_handler.is_beta_tes COURSE_OVERVIEW_METHOD = 'lms.djangoapps.certificates.generation_handler.get_course_overview_or_none' CCX_COURSE_METHOD = 'lms.djangoapps.certificates.generation_handler._is_ccx_course' GET_GRADE_METHOD = 'lms.djangoapps.certificates.generation_handler._get_course_grade' +INTEGRITY_ENABLED_METHOD = 'lms.djangoapps.certificates.generation_handler.is_integrity_signature_enabled' ID_VERIFIED_METHOD = 'lms.djangoapps.verify_student.services.IDVerificationService.user_is_verified' PASSING_GRADE_METHOD = 'lms.djangoapps.certificates.generation_handler._is_passing_grade' WEB_CERTS_METHOD = 'lms.djangoapps.certificates.generation_handler.has_html_certificates_enabled' +@mock.patch(INTEGRITY_ENABLED_METHOD, mock.Mock(return_value=False)) @mock.patch(ID_VERIFIED_METHOD, mock.Mock(return_value=True)) @mock.patch(WEB_CERTS_METHOD, mock.Mock(return_value=True)) @ddt.ddt @@ -199,14 +201,19 @@ class AllowlistTests(ModuleStoreTestCase): """ assert generate_certificate_task(self.user, self.course_run_key) - def test_can_generate_not_verified(self): + @ddt.data(False, True) + def test_can_generate_not_verified(self, idv_retired): """ Test handling when the user's id is not verified """ - with mock.patch(ID_VERIFIED_METHOD, return_value=False): - assert not _can_generate_allowlist_certificate(self.user, self.course_run_key, self.enrollment_mode) - assert _set_allowlist_cert_status(self.user, self.course_run_key, self.enrollment_mode, self.grade) == \ - CertificateStatuses.unverified + with mock.patch(ID_VERIFIED_METHOD, return_value=False), \ + mock.patch(INTEGRITY_ENABLED_METHOD, return_value=idv_retired): + self.assertEqual(idv_retired, + _can_generate_allowlist_certificate(self.user, self.course_run_key, self.enrollment_mode)) + self.assertIsNot(idv_retired, + _set_allowlist_cert_status( + self.user, self.course_run_key, + self.enrollment_mode, self.grade) == CertificateStatuses.unverified) def test_can_generate_not_enrolled(self): """ @@ -324,6 +331,7 @@ class AllowlistTests(ModuleStoreTestCase): assert _set_allowlist_cert_status(u, key, self.enrollment_mode, self.grade) is None +@mock.patch(INTEGRITY_ENABLED_METHOD, mock.Mock(return_value=False)) @mock.patch(ID_VERIFIED_METHOD, mock.Mock(return_value=True)) @mock.patch(CCX_COURSE_METHOD, mock.Mock(return_value=False)) @mock.patch(PASSING_GRADE_METHOD, mock.Mock(return_value=True)) @@ -480,7 +488,8 @@ class CertificateTests(ModuleStoreTestCase): """ assert _can_generate_certificate_for_status(None, None, None) - def test_can_generate_not_verified_cert(self): + @ddt.data(False, True) + def test_can_generate_not_verified_cert(self, idv_retired): """ Test handling when the user's id is not verified and they have a cert """ @@ -498,12 +507,15 @@ class CertificateTests(ModuleStoreTestCase): status=CertificateStatuses.generating ) - with mock.patch(ID_VERIFIED_METHOD, return_value=False): - assert not _can_generate_regular_certificate(u, self.course_run_key, self.enrollment_mode, self.grade) - assert _set_regular_cert_status(u, self.course_run_key, self.enrollment_mode, - self.grade) == CertificateStatuses.unverified + with mock.patch(ID_VERIFIED_METHOD, return_value=False), \ + mock.patch(INTEGRITY_ENABLED_METHOD, return_value=idv_retired): + self.assertEqual(idv_retired, _can_generate_regular_certificate(u, self.course_run_key, + self.enrollment_mode, self.grade)) + self.assertIsNot(idv_retired, _set_regular_cert_status(u, self.course_run_key, self.enrollment_mode, + self.grade) == CertificateStatuses.unverified) - def test_can_generate_not_verified_no_cert(self): + @ddt.data(False, True) + def test_can_generate_not_verified_no_cert(self, idv_retired): """ Test handling when the user's id is not verified and they don't have a cert """ @@ -515,12 +527,15 @@ class CertificateTests(ModuleStoreTestCase): mode=CourseMode.VERIFIED, ) - with mock.patch(ID_VERIFIED_METHOD, return_value=False): - assert not _can_generate_regular_certificate(u, self.course_run_key, self.enrollment_mode, self.grade) - assert _set_regular_cert_status(u, self.course_run_key, self.enrollment_mode, - self.grade) == CertificateStatuses.unverified + with mock.patch(ID_VERIFIED_METHOD, return_value=False), \ + mock.patch(INTEGRITY_ENABLED_METHOD, return_value=idv_retired): + self.assertEqual(idv_retired, _can_generate_regular_certificate(u, self.course_run_key, + self.enrollment_mode, self.grade)) + self.assertIsNot(idv_retired, _set_regular_cert_status(u, self.course_run_key, self.enrollment_mode, + self.grade) == CertificateStatuses.unverified) - def test_can_generate_not_verified_not_passing(self): + @ddt.data(False, True) + def test_can_generate_not_verified_not_passing(self, idv_retired): """ Test handling when the user's id is not verified and the user is not passing """ @@ -538,12 +553,18 @@ class CertificateTests(ModuleStoreTestCase): status=CertificateStatuses.generating ) - with mock.patch(ID_VERIFIED_METHOD, return_value=False): - with mock.patch(PASSING_GRADE_METHOD, return_value=False): - assert not _can_generate_regular_certificate(u, self.course_run_key, self.enrollment_mode, self.grade) + with mock.patch(ID_VERIFIED_METHOD, return_value=False), \ + mock.patch(INTEGRITY_ENABLED_METHOD, return_value=idv_retired), \ + mock.patch(PASSING_GRADE_METHOD, return_value=False): + assert not _can_generate_regular_certificate(u, self.course_run_key, self.enrollment_mode, self.grade) + if idv_retired: + assert _set_regular_cert_status(u, self.course_run_key, self.enrollment_mode, self.grade) \ + == CertificateStatuses.notpassing + else: assert _set_regular_cert_status(u, self.course_run_key, self.enrollment_mode, self.grade) is None - def test_can_generate_not_verified_not_passing_allowlist(self): + @ddt.data(False, True) + def test_can_generate_not_verified_not_passing_allowlist(self, idv_retired): """ Test handling when the user's id is not verified and the user is not passing but is on the allowlist """ @@ -562,9 +583,14 @@ class CertificateTests(ModuleStoreTestCase): ) CertificateAllowlistFactory(course_id=self.course_run_key, user=u) - with mock.patch(ID_VERIFIED_METHOD, return_value=False): - with mock.patch(PASSING_GRADE_METHOD, return_value=False): - assert not _can_generate_regular_certificate(u, self.course_run_key, self.enrollment_mode, self.grade) + with mock.patch(ID_VERIFIED_METHOD, return_value=False), \ + mock.patch(INTEGRITY_ENABLED_METHOD, return_value=idv_retired), \ + mock.patch(PASSING_GRADE_METHOD, return_value=False): + assert not _can_generate_regular_certificate(u, self.course_run_key, self.enrollment_mode, self.grade) + if idv_retired: + assert _set_regular_cert_status(u, self.course_run_key, self.enrollment_mode, + self.grade) == CertificateStatuses.notpassing + else: assert _set_regular_cert_status(u, self.course_run_key, self.enrollment_mode, self.grade) == CertificateStatuses.unverified diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 7420dfa9d5..8efb7a13f1 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -2082,7 +2082,7 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase): 'failed': 0, 'skipped': 2 } - with self.assertNumQueries(71): + with self.assertNumQueries(77): self.assertCertificatesGenerated(task_input, expected_results) @ddt.data(