From 6d3a4014bbffcd76a2f6f28d0a5b777c2eab18e4 Mon Sep 17 00:00:00 2001 From: Christie Rice <8483753+crice100@users.noreply.github.com> Date: Thu, 15 Jul 2021 09:51:24 -0400 Subject: [PATCH] fix: Save parameter values in the certificate, if available (#28178) MICROBA-1373 --- lms/djangoapps/certificates/data.py | 2 +- lms/djangoapps/certificates/generation.py | 49 +++++++--- .../certificates/tests/test_generation.py | 90 +++++++++++++++---- 3 files changed, 113 insertions(+), 28 deletions(-) diff --git a/lms/djangoapps/certificates/data.py b/lms/djangoapps/certificates/data.py index f8c670fffb..e69fc7dcb9 100644 --- a/lms/djangoapps/certificates/data.py +++ b/lms/djangoapps/certificates/data.py @@ -27,7 +27,7 @@ class CertificateStatuses: unavailable - Certificate has been invalidated. unverified - The user does not have an approved, unexpired identity verification. - The following statuses are set by V2 of course certificates: + The following statuses are set by the current course certificates code: downloadable - See generation.py notpassing - See GeneratedCertificate.mark_notpassing() unavailable - See GeneratedCertificate.invalidate() diff --git a/lms/djangoapps/certificates/generation.py b/lms/djangoapps/certificates/generation.py index d3ce2f2b25..eb764369f6 100644 --- a/lms/djangoapps/certificates/generation.py +++ b/lms/djangoapps/certificates/generation.py @@ -4,8 +4,6 @@ Course certificate generation These methods generate course certificates (they create a new course certificate if it does not yet exist, or update the existing cert if it does already exist). -For now, these methods deal primarily with allowlist certificates, and are part of the V2 certificates revamp. - These methods should be called from tasks. """ @@ -21,7 +19,8 @@ from lms.djangoapps.grades.api import CourseGradeFactory log = logging.getLogger(__name__) -def generate_course_certificate(user, course_key, status, generation_mode): +def generate_course_certificate(user, course_key, status, enrollment_mode=None, course_grade=None, + generation_mode=None): """ Generate a course certificate for this user, in this course run. If the certificate has a passing status, also emit a certificate event. @@ -33,10 +32,20 @@ def generate_course_certificate(user, course_key, status, generation_mode): user: user for whom to generate a certificate course_key: course run key for which to generate a certificate status: certificate status (value from the CertificateStatuses model) - generation_mode: Used when emitting an events. Options are "self" (implying the user generated the cert + enrollment_mode: user's enrollment mode (ex. verified) + course_grade: user's course grade + generation_mode: used when emitting an event. Options are "self" (implying the user generated the cert themself) and "batch" for everything else. """ - cert = _generate_certificate(user, course_key, status) + if not enrollment_mode: + enrollment_mode = _get_enrollment_mode(user, course_key) + if not course_grade: + course_grade = _get_course_grade(user, course_key) + if not generation_mode: + generation_mode = 'batch' + + cert = _generate_certificate(user=user, course_key=course_key, status=status, enrollment_mode=enrollment_mode, + course_grade=course_grade) if CertificateStatuses.is_passing_status(cert.status): # Emit a certificate event @@ -55,9 +64,13 @@ def generate_course_certificate(user, course_key, status, generation_mode): return cert -def _generate_certificate(user, course_key, status): +def _generate_certificate(user, course_key, status, enrollment_mode, course_grade): """ Generate a certificate for this user, in this course run. + + This method takes things like grade and enrollment mode as parameters because these are used to determine if the + user is eligible for a certificate, and they're also saved in the cert itself. We want the cert to reflect the + values that were used when determining if it was eligible for generation. """ # Retrieve the existing certificate for the learner if it exists existing_certificate = GeneratedCertificate.certificate_for_student(user, course_key) @@ -65,9 +78,6 @@ def _generate_certificate(user, course_key, status): profile = UserProfile.objects.get(user=user) profile_name = profile.name - course_grade = CourseGradeFactory().read(user, course_key=course_key) - enrollment_mode, __ = CourseEnrollment.enrollment_mode_for_user(user, course_key) - # Retain the `verify_uuid` from an existing certificate if possible, this will make it possible for the learner to # keep the existing URL to their certificate if existing_certificate and existing_certificate.verify_uuid: @@ -84,7 +94,7 @@ def _generate_certificate(user, course_key, status): 'mode': enrollment_mode, 'name': profile_name, 'status': status, - 'grade': course_grade.percent, + 'grade': course_grade, 'download_url': '', 'key': '', 'verify_uuid': uuid, @@ -96,5 +106,22 @@ def _generate_certificate(user, course_key, status): created_msg = 'Certificate was created.' else: created_msg = 'Certificate already existed and was updated.' - log.info(f'Generated certificate with status {cert.status} for {user.id} : {course_key}. {created_msg}') + log.info(f'Generated certificate with status {cert.status}, mode {cert.mode} and grade {cert.grade} for {user.id} ' + f': {course_key}. {created_msg}') return cert + + +def _get_course_grade(user, course_key): + """ + Get the user's course grade in this course run + """ + course_grade = CourseGradeFactory().read(user, course_key=course_key) + return course_grade.percent + + +def _get_enrollment_mode(user, course_key): + """ + Get the user's enrollment mode in this course run + """ + enrollment_mode, __ = CourseEnrollment.enrollment_mode_for_user(user, course_key) + return enrollment_mode diff --git a/lms/djangoapps/certificates/tests/test_generation.py b/lms/djangoapps/certificates/tests/test_generation.py index a2f55d91a3..3e1a548aec 100644 --- a/lms/djangoapps/certificates/tests/test_generation.py +++ b/lms/djangoapps/certificates/tests/test_generation.py @@ -2,7 +2,9 @@ Tests for certificate generation """ import logging +from unittest import mock +from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory from common.djangoapps.util.testing import EventTestMixin from lms.djangoapps.certificates.data import CertificateStatuses @@ -14,6 +16,9 @@ from xmodule.modulestore.tests.factories import CourseFactory log = logging.getLogger(__name__) +ENROLLMENT_METHOD = 'lms.djangoapps.certificates.generation._get_enrollment_mode' +GRADE_METHOD = 'lms.djangoapps.certificates.generation._get_course_grade' + class CertificateTests(EventTestMixin, ModuleStoreTestCase): """ @@ -31,9 +36,11 @@ class CertificateTests(EventTestMixin, ModuleStoreTestCase): user=self.u, course_id=self.key, is_active=True, - mode='verified', + mode=CourseMode.VERIFIED, ) self.gen_mode = 'batch' + self.grade = '.85' + self.enrollment_mode = CourseMode.VERIFIED def test_generation(self): """ @@ -42,11 +49,8 @@ class CertificateTests(EventTestMixin, ModuleStoreTestCase): certs = GeneratedCertificate.objects.filter(user=self.u, course_id=self.key) assert len(certs) == 0 - generated_cert = generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable, self.gen_mode) - assert generated_cert.status, CertificateStatuses.downloadable - - certs = GeneratedCertificate.objects.filter(user=self.u, course_id=self.key) - assert len(certs) == 1 + generated_cert = generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable, + self.enrollment_mode, self.grade, self.gen_mode) self.assert_event_emitted( 'edx.certificate.created', @@ -58,33 +62,69 @@ class CertificateTests(EventTestMixin, ModuleStoreTestCase): generation_mode=self.gen_mode ) - def test_generation_existing(self): + certs = GeneratedCertificate.objects.filter(user=self.u, course_id=self.key) + assert len(certs) == 1 + + cert = GeneratedCertificate.objects.get(user=self.u, course_id=self.key) + assert cert.status == CertificateStatuses.downloadable + assert cert.mode == self.enrollment_mode + assert cert.grade == self.grade + + def test_generation_existing_unverified(self): """ - Test certificate generation when a certificate already exists + Test certificate generation when a certificate already exists and we want to mark it as unverified """ error_reason = 'Some PDF error' GeneratedCertificateFactory( user=self.u, course_id=self.key, - mode='verified', + mode=CourseMode.AUDIT, status=CertificateStatuses.error, error_reason=error_reason ) cert = GeneratedCertificate.objects.get(user=self.u, course_id=self.key) assert cert.error_reason == error_reason + assert cert.mode == CourseMode.AUDIT + assert cert.status == CertificateStatuses.error - generated_cert = generate_course_certificate(self.u, self.key, CertificateStatuses.unverified, self.gen_mode) - assert generated_cert.status, CertificateStatuses.unverified + generate_course_certificate(self.u, self.key, CertificateStatuses.unverified, self.enrollment_mode, self.grade, + self.gen_mode) cert = GeneratedCertificate.objects.get(user=self.u, course_id=self.key) assert cert.error_reason == '' + assert cert.status == CertificateStatuses.unverified + assert cert.mode == self.enrollment_mode + assert cert.grade == '' + + def test_generation_existing_downloadable(self): + """ + Test certificate generation when a certificate already exists and we want to mark it as downloadable + """ + error_reason = 'Some PDF error' + GeneratedCertificateFactory( + user=self.u, + course_id=self.key, + mode=CourseMode.AUDIT, + status=CertificateStatuses.error, + error_reason=error_reason + ) + + generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable, self.enrollment_mode, + self.grade, self.gen_mode) + + cert = GeneratedCertificate.objects.get(user=self.u, course_id=self.key) + assert cert.error_reason == '' + assert cert.status == CertificateStatuses.downloadable + assert cert.mode == self.enrollment_mode + assert cert.grade == self.grade def test_generation_uuid_persists_through_revocation(self): """ Test that the `verify_uuid` value of a certificate does not change when it is revoked and re-awarded. """ - generated_cert = generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable, self.gen_mode) + generated_cert = generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable, + self.enrollment_mode, self.grade, self.gen_mode) assert generated_cert.status, CertificateStatuses.downloadable verify_uuid = generated_cert.verify_uuid @@ -93,7 +133,8 @@ class CertificateTests(EventTestMixin, ModuleStoreTestCase): assert generated_cert.status, CertificateStatuses.unavailable assert generated_cert.verify_uuid, verify_uuid - generated_cert = generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable, self.gen_mode) + generated_cert = generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable, + self.enrollment_mode, self.grade, self.gen_mode) assert generated_cert.status, CertificateStatuses.downloadable assert generated_cert.verify_uuid, verify_uuid @@ -101,7 +142,8 @@ class CertificateTests(EventTestMixin, ModuleStoreTestCase): assert generated_cert.status, CertificateStatuses.notpassing assert generated_cert.verify_uuid, verify_uuid - generated_cert = generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable, self.gen_mode) + generated_cert = generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable, + self.enrollment_mode, self.grade, self.gen_mode) assert generated_cert.status, CertificateStatuses.downloadable assert generated_cert.verify_uuid, verify_uuid @@ -112,11 +154,27 @@ class CertificateTests(EventTestMixin, ModuleStoreTestCase): GeneratedCertificateFactory( user=self.u, course_id=self.key, - mode='verified', + mode=CourseMode.VERIFIED, status=CertificateStatuses.unverified, verify_uuid='' ) - generated_cert = generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable, self.gen_mode) + generated_cert = generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable, + self.enrollment_mode, self.grade, self.gen_mode) assert generated_cert.status, CertificateStatuses.downloadable assert generated_cert.verify_uuid != '' + + def test_generation_few_params(self): + """ + Test that ensures we retrieve values as needed + """ + grade = '.33' + enrollment_mode = CourseMode.AUDIT + + with mock.patch(ENROLLMENT_METHOD, return_value=enrollment_mode): + with mock.patch(GRADE_METHOD, return_value=grade): + generated_cert = generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable) + + assert generated_cert.status, CertificateStatuses.downloadable + assert generated_cert.mode, enrollment_mode + assert generated_cert.grade, grade