From cdeda7a3133eb457ba3dee09866b7b5ef58fc4b8 Mon Sep 17 00:00:00 2001 From: Christie Rice <8483753+crice100@users.noreply.github.com> Date: Tue, 3 Aug 2021 11:24:11 -0400 Subject: [PATCH] fix: Update mode in course certificate when marking cert as invalidated, not verified or not passing (#28330) MICROBA-1410 --- lms/djangoapps/certificates/generation.py | 2 +- .../certificates/generation_handler.py | 6 +- lms/djangoapps/certificates/models.py | 47 +++++-- lms/djangoapps/certificates/signals.py | 9 +- lms/djangoapps/certificates/tests/test_api.py | 36 +++++ .../certificates/tests/test_generation.py | 2 +- .../certificates/tests/test_models.py | 131 ++++++++++++++++-- 7 files changed, 202 insertions(+), 31 deletions(-) diff --git a/lms/djangoapps/certificates/generation.py b/lms/djangoapps/certificates/generation.py index 4e5ed4d6b0..9a0b944f8a 100644 --- a/lms/djangoapps/certificates/generation.py +++ b/lms/djangoapps/certificates/generation.py @@ -50,7 +50,7 @@ def generate_course_certificate(user, course_key, status, enrollment_mode, cours emit_certificate_event(event_name='created', user=user, course_id=course_key, event_data=event_data) elif CertificateStatuses.unverified == cert.status: - cert.mark_unverified(source='certificate_generation') + cert.mark_unverified(mode=enrollment_mode, source='certificate_generation') return cert diff --git a/lms/djangoapps/certificates/generation_handler.py b/lms/djangoapps/certificates/generation_handler.py index c9ab850529..39a01eff6b 100644 --- a/lms/djangoapps/certificates/generation_handler.py +++ b/lms/djangoapps/certificates/generation_handler.py @@ -220,7 +220,7 @@ def _set_regular_cert_status(user, course_key, enrollment_mode, course_grade): if IDVerificationService.user_is_verified(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(course_grade_val, source='certificate_generation') + cert.mark_notpassing(mode=enrollment_mode, grade=course_grade_val, source='certificate_generation') return CertificateStatuses.notpassing return None @@ -235,7 +235,7 @@ def _get_cert_status_common(user, course_key, enrollment_mode, course_grade, cer """ if CertificateInvalidation.has_certificate_invalidation(user, course_key) and cert is not None: if cert.status != CertificateStatuses.unavailable: - cert.invalidate(source='certificate_generation') + 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, @@ -245,7 +245,7 @@ def _get_cert_status_common(user, course_key, enrollment_mode, course_grade, cer course_grade=course_grade, status=CertificateStatuses.unverified, generation_mode='batch') elif cert.status != CertificateStatuses.unverified: - cert.mark_unverified(source='certificate_generation') + cert.mark_unverified(mode=enrollment_mode, source='certificate_generation') return CertificateStatuses.unverified return None diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index 225b166164..005f4c66e3 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -23,6 +23,8 @@ from model_utils.models import TimeStampedModel from opaque_keys.edx.django.models import CourseKeyField from simple_history.models import HistoricalRecords +from common.djangoapps.student import models_api as student_api +from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.util.milestones_helpers import fulfill_course_milestone, is_prerequisite_courses_enabled from lms.djangoapps.badges.events.course_complete import course_badge_check from lms.djangoapps.badges.events.course_meta import completion_check, course_group_check @@ -296,41 +298,51 @@ class GeneratedCertificate(models.Model): user=self.user ) - def invalidate(self, source=None): + def invalidate(self, mode=None, source=None): """ Invalidate Generated Certificate by marking it 'unavailable'. For additional information see the `_revoke_certificate()` function. Args: + mode (String) - learner's current enrollment mode. May be none as the caller likely does not need to + evaluate the mode before deciding to invalidate the cert. source (String) - source requesting invalidation of the certificate for tracking purposes """ - log.info(f'Marking certificate as unavailable for {self.user.id} : {self.course_id}') - self._revoke_certificate(CertificateStatuses.unavailable, source=source) + if not mode: + mode, __ = CourseEnrollment.enrollment_mode_for_user(self.user, self.course_id) - def mark_notpassing(self, grade, source=None): + log.info(f'Marking certificate as unavailable for {self.user.id} : {self.course_id} with mode {mode} from ' + f'source {source}') + self._revoke_certificate(status=CertificateStatuses.unavailable, mode=mode, source=source) + + def mark_notpassing(self, mode, grade, source=None): """ Invalidates a Generated Certificate by marking it as 'notpassing'. For additional information see the `_revoke_certificate()` function. Args: + mode (String) - learner's current enrollment mode grade (float) - snapshot of the learner's current grade as a decimal source (String) - source requesting invalidation of the certificate for tracking purposes """ - log.info(f'Marking certificate as notpassing for {self.user.id} : {self.course_id}') - self._revoke_certificate(CertificateStatuses.notpassing, grade=grade, source=source) + log.info(f'Marking certificate as notpassing for {self.user.id} : {self.course_id} with mode {mode} from ' + f'source {source}') + self._revoke_certificate(status=CertificateStatuses.notpassing, mode=mode, grade=grade, source=source) - def mark_unverified(self, source=None): + def mark_unverified(self, mode, source=None): """ Invalidates a Generated Certificate by marking it as 'unverified'. For additional information see the `_revoke_certificate()` function. Args: + mode (String) - learner's current enrollment mode source (String) - source requesting invalidation of the certificate for tracking purposes """ - log.info(f'Marking certificate as unverified for {self.user.id} : {self.course_id}') - self._revoke_certificate(CertificateStatuses.unverified, source=source) + log.info(f'Marking certificate as unverified for {self.user.id} : {self.course_id} with mode {mode} from ' + f'source {source}') + self._revoke_certificate(status=CertificateStatuses.unverified, mode=mode, source=source) - def _revoke_certificate(self, status, grade=None, source=None): + def _revoke_certificate(self, status, mode=None, grade=None, source=None): """ Revokes a course certificate from a learner, updating the certificate's status as specified by the value of the `status` argument. This will prevent the learner from being able to access their certificate in the associated @@ -346,16 +358,29 @@ class GeneratedCertificate(models.Model): Args: status (CertificateStatus) - certificate status to set for the `GeneratedCertificate` record + mode (String) - learner's current enrollment mode grade (float) - snapshot of the learner's current grade as a decimal source (String) - source requesting invalidation of the certificate for tracking purposes """ previous_certificate_status = self.status + if not grade: + grade = '' + + if not mode: + mode = self.mode + + profile_name = student_api.get_name(self.user.id) + if not profile_name: + profile_name = '' + self.error_reason = '' self.download_uuid = '' self.download_url = '' - self.grade = grade or '' + self.grade = grade self.status = status + self.mode = mode + self.name = profile_name self.save() COURSE_CERT_REVOKED.send_robust( diff --git a/lms/djangoapps/certificates/signals.py b/lms/djangoapps/certificates/signals.py index 2039c4e470..e54d690a53 100644 --- a/lms/djangoapps/certificates/signals.py +++ b/lms/djangoapps/certificates/signals.py @@ -97,12 +97,9 @@ def _listen_for_failing_grade(sender, user, course_id, grade, **kwargs): # pyli cert = GeneratedCertificate.certificate_for_student(user, course_id) if cert is not None: if CertificateStatuses.is_passing_status(cert.status): - cert.mark_notpassing(grade.percent, source='notpassing_signal') - log.info('Certificate marked not passing for {user} : {course} via failing grade: {grade}'.format( - user=user.id, - course=course_id, - grade=grade - )) + enrollment_mode, __ = CourseEnrollment.enrollment_mode_for_user(user, course_id) + cert.mark_notpassing(mode=enrollment_mode, grade=grade.percent, source='notpassing_signal') + log.info(f'Certificate marked not passing for {user.id} : {course_id} via failing grade') @receiver(LEARNER_NOW_VERIFIED, dispatch_uid="learner_track_changed") diff --git a/lms/djangoapps/certificates/tests/test_api.py b/lms/djangoapps/certificates/tests/test_api.py index 45720bc650..2cee0d135d 100644 --- a/lms/djangoapps/certificates/tests/test_api.py +++ b/lms/djangoapps/certificates/tests/test_api.py @@ -596,6 +596,42 @@ class GenerateUserCertificatesTest(ModuleStoreTestCase): assert cert.status == CertificateStatuses.downloadable assert cert.mode == CourseMode.VERIFIED + @patch.dict(settings.FEATURES, {'CERTIFICATES_HTML_VIEW': True}) + def test_generation_unverified(self): + """ + Test that a cert is successfully generated with a status of unverified + """ + cert = get_certificate_for_user_id(self.user.id, self.course_run_key) + assert not cert + + with mock.patch(PASSING_GRADE_METHOD, return_value=True): + with mock.patch(ID_VERIFIED_METHOD, return_value=False): + generate_certificate_task(self.user, self.course_run_key) + + cert = get_certificate_for_user_id(self.user.id, self.course_run_key) + assert cert.status == CertificateStatuses.unverified + assert cert.mode == CourseMode.VERIFIED + + @patch.dict(settings.FEATURES, {'CERTIFICATES_HTML_VIEW': True}) + def test_generation_notpassing(self): + """ + Test that a cert is successfully generated with a status of notpassing + """ + GeneratedCertificateFactory( + user=self.user, + course_id=self.course_run_key, + status=CertificateStatuses.unavailable, + mode=CourseMode.AUDIT + ) + + with mock.patch(PASSING_GRADE_METHOD, return_value=False): + with mock.patch(ID_VERIFIED_METHOD, return_value=True): + generate_certificate_task(self.user, self.course_run_key) + + cert = get_certificate_for_user_id(self.user.id, self.course_run_key) + assert cert.status == CertificateStatuses.notpassing + assert cert.mode == CourseMode.VERIFIED + @ddt.ddt class CertificateGenerationEnabledTest(EventTestMixin, TestCase): diff --git a/lms/djangoapps/certificates/tests/test_generation.py b/lms/djangoapps/certificates/tests/test_generation.py index 8687632e23..e7e752fd50 100644 --- a/lms/djangoapps/certificates/tests/test_generation.py +++ b/lms/djangoapps/certificates/tests/test_generation.py @@ -141,7 +141,7 @@ class CertificateTests(EventTestMixin, ModuleStoreTestCase): assert generated_cert.status, CertificateStatuses.downloadable assert generated_cert.verify_uuid, verify_uuid - generated_cert.mark_notpassing(50.00) + generated_cert.mark_notpassing(mode=generated_cert.mode, grade=50.00) assert generated_cert.status, CertificateStatuses.notpassing assert generated_cert.verify_uuid, verify_uuid diff --git a/lms/djangoapps/certificates/tests/test_models.py b/lms/djangoapps/certificates/tests/test_models.py index 6409a9969d..456c471cb5 100644 --- a/lms/djangoapps/certificates/tests/test_models.py +++ b/lms/djangoapps/certificates/tests/test_models.py @@ -3,6 +3,7 @@ import json from unittest.mock import patch +from unittest import mock import ddt import pytest @@ -14,6 +15,8 @@ from django.test.utils import override_settings from opaque_keys.edx.locator import CourseKey, CourseLocator from path import Path as path +from common.djangoapps.course_modes.models import CourseMode +from common.djangoapps.student.models import UserProfile from common.djangoapps.student.tests.factories import AdminFactory, UserFactory from lms.djangoapps.certificates.models import ( CertificateAllowlist, @@ -36,6 +39,9 @@ from openedx.core.djangoapps.content.course_overviews.tests.factories import Cou from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory +ENROLLMENT_METHOD = 'common.djangoapps.student.models.CourseEnrollment.enrollment_mode_for_user' +PROFILE_METHOD = 'common.djangoapps.student.models_api.get_name' + FEATURES_INVALID_FILE_PATH = settings.FEATURES.copy() FEATURES_INVALID_FILE_PATH['CERTS_HTML_VIEW_CONFIG_PATH'] = 'invalid/path/to/config.json' @@ -384,24 +390,125 @@ class GeneratedCertificateTest(SharedModuleStoreTestCase): cert = GeneratedCertificateFactory.create( status=CertificateStatuses.downloadable, user=self.user, - course_id=self.course_key + course_id=self.course_key, + mode=CourseMode.AUDIT, + name='Fuzzy Hippo' ) + mode = CourseMode.VERIFIED source = 'invalidated_test' - cert.invalidate(source=source) + cert.invalidate(mode=mode, source=source) cert = GeneratedCertificate.objects.get(user=self.user, course_id=self.course_key) + profile = UserProfile.objects.get(user=self.user) assert cert.status == CertificateStatuses.unavailable + assert cert.mode == mode + assert cert.name == profile.name expected_event_data = { 'user_id': self.user.id, 'course_id': str(self.course_key), 'certificate_id': cert.verify_uuid, - 'enrollment_mode': cert.mode, + 'enrollment_mode': mode, 'source': source, } self._assert_event_data(mock_emit_certificate_event, expected_event_data) + @patch('lms.djangoapps.certificates.utils.emit_certificate_event') + def test_invalidate_find_mode(self, mock_emit_certificate_event): + """ + Test the invalidate method when mode is retrieved from the enrollment + """ + cert = GeneratedCertificateFactory.create( + status=CertificateStatuses.downloadable, + user=self.user, + course_id=self.course_key, + mode=CourseMode.AUDIT + ) + + mode = CourseMode.MASTERS + source = 'invalidated_test' + with mock.patch(ENROLLMENT_METHOD, return_value=(mode, None)): + cert.invalidate(source=source) + + cert = GeneratedCertificate.objects.get(user=self.user, course_id=self.course_key) + assert cert.status == CertificateStatuses.unavailable + assert cert.mode == mode + + expected_event_data = { + 'user_id': self.user.id, + 'course_id': str(self.course_key), + 'certificate_id': cert.verify_uuid, + 'enrollment_mode': mode, + 'source': source, + } + + self._assert_event_data(mock_emit_certificate_event, expected_event_data) + + @patch('lms.djangoapps.certificates.utils.emit_certificate_event') + def test_invalidate_no_mode(self, mock_emit_certificate_event): + """ + Test the invalidate method when there is no enrollment mode + """ + initial_mode = CourseMode.AUDIT + cert = GeneratedCertificateFactory.create( + status=CertificateStatuses.downloadable, + user=self.user, + course_id=self.course_key, + mode=initial_mode + ) + + source = 'invalidated_test' + with mock.patch(ENROLLMENT_METHOD, return_value=(None, None)): + cert.invalidate(source=source) + + cert = GeneratedCertificate.objects.get(user=self.user, course_id=self.course_key) + assert cert.status == CertificateStatuses.unavailable + assert cert.mode == initial_mode + + expected_event_data = { + 'user_id': self.user.id, + 'course_id': str(self.course_key), + 'certificate_id': cert.verify_uuid, + 'enrollment_mode': initial_mode, + 'source': source, + } + + self._assert_event_data(mock_emit_certificate_event, expected_event_data) + + @patch('lms.djangoapps.certificates.utils.emit_certificate_event') + def test_invalidate_no_profile(self, mock_emit_certificate_event): + """ + Test the invalidate method when there is no user profile + """ + cert = GeneratedCertificateFactory.create( + status=CertificateStatuses.downloadable, + user=self.user, + course_id=self.course_key, + mode=CourseMode.AUDIT, + name='Squeaky Frog' + ) + + mode = CourseMode.VERIFIED + source = 'invalidated_test' + with mock.patch(PROFILE_METHOD, return_value=None): + cert.invalidate(mode=mode, source=source) + + cert = GeneratedCertificate.objects.get(user=self.user, course_id=self.course_key) + assert cert.status == CertificateStatuses.unavailable + assert cert.mode == mode + assert cert.name == '' + + expected_event_data = { + 'user_id': self.user.id, + 'course_id': str(self.course_key), + 'certificate_id': cert.verify_uuid, + 'enrollment_mode': cert.mode, + 'source': source, + } + + self._assert_event_data(mock_emit_certificate_event, expected_event_data) + @patch('lms.djangoapps.certificates.utils.emit_certificate_event') def test_notpassing(self, mock_emit_certificate_event): """ @@ -410,21 +517,24 @@ class GeneratedCertificateTest(SharedModuleStoreTestCase): cert = GeneratedCertificateFactory.create( status=CertificateStatuses.downloadable, user=self.user, - course_id=self.course_key + course_id=self.course_key, + mode=CourseMode.AUDIT ) + mode = CourseMode.VERIFIED grade = '.3' source = "notpassing_test" - cert.mark_notpassing(grade, source=source) + cert.mark_notpassing(mode=mode, grade=grade, source=source) cert = GeneratedCertificate.objects.get(user=self.user, course_id=self.course_key) assert cert.status == CertificateStatuses.notpassing + assert cert.mode == mode assert cert.grade == grade expected_event_data = { 'user_id': self.user.id, 'course_id': str(self.course_key), 'certificate_id': cert.verify_uuid, - 'enrollment_mode': cert.mode, + 'enrollment_mode': mode, 'source': source, } @@ -438,19 +548,22 @@ class GeneratedCertificateTest(SharedModuleStoreTestCase): cert = GeneratedCertificateFactory.create( status=CertificateStatuses.downloadable, user=self.user, - course_id=self.course_key + course_id=self.course_key, + mode=CourseMode.AUDIT ) + mode = CourseMode.VERIFIED source = "unverified_test" - cert.mark_unverified(source=source) + cert.mark_unverified(mode=mode, source=source) cert = GeneratedCertificate.objects.get(user=self.user, course_id=self.course_key) assert cert.status == CertificateStatuses.unverified + assert cert.mode == mode expected_event_data = { 'user_id': self.user.id, 'course_id': str(self.course_key), 'certificate_id': cert.verify_uuid, - 'enrollment_mode': cert.mode, + 'enrollment_mode': mode, 'source': source, }