From 1be408046b48a72e23e556cbbba3f50c64f5a29f Mon Sep 17 00:00:00 2001 From: Andy Shultz Date: Tue, 26 Oct 2021 13:35:01 -0400 Subject: [PATCH 1/2] feat: explain certificate delay seconds and make it overrideable this appears to be a throttling method on first glance but is instead an async task handling hack, explain it and then provide a way to override it for tight control of timing while regenerating multiple certs --- .../certificates/generation_handler.py | 24 ++++++++++++------- lms/djangoapps/certificates/tasks.py | 3 +++ 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/lms/djangoapps/certificates/generation_handler.py b/lms/djangoapps/certificates/generation_handler.py index a8b59b7cb9..d671a4ade0 100644 --- a/lms/djangoapps/certificates/generation_handler.py +++ b/lms/djangoapps/certificates/generation_handler.py @@ -27,7 +27,7 @@ from openedx.core.djangoapps.content.course_overviews.api import get_course_over log = logging.getLogger(__name__) -def generate_certificate_task(user, course_key, generation_mode=None): +def generate_certificate_task(user, course_key, generation_mode=None, delay_seconds=CERTIFICATE_DELAY_SECONDS): """ Create a task to generate a certificate for this user in this course run, if the user is eligible and a certificate can be generated. @@ -38,13 +38,16 @@ def generate_certificate_task(user, course_key, generation_mode=None): if is_on_certificate_allowlist(user, course_key): log.info(f'User {user.id} is on the allowlist for {course_key}. Attempt will be made to generate an allowlist ' f'certificate.') - return generate_allowlist_certificate_task(user, course_key, generation_mode) + return generate_allowlist_certificate_task(user, course_key, generation_mode=generation_mode, + delay_seconds=delay_seconds) log.info(f'Attempt will be made to generate course certificate for user {user.id} : {course_key}') - return _generate_regular_certificate_task(user, course_key, generation_mode) + return _generate_regular_certificate_task(user, course_key, generation_mode=generation_mode, + delay_seconds=delay_seconds) -def generate_allowlist_certificate_task(user, course_key, generation_mode=None): +def generate_allowlist_certificate_task(user, course_key, generation_mode=None, + delay_seconds=CERTIFICATE_DELAY_SECONDS): """ Create a task to generate an allowlist certificate for this user in this course run. """ @@ -52,7 +55,8 @@ def generate_allowlist_certificate_task(user, course_key, generation_mode=None): course_grade = _get_course_grade(user, course_key) if _can_generate_allowlist_certificate(user, course_key, enrollment_mode): return _generate_certificate_task(user=user, course_key=course_key, enrollment_mode=enrollment_mode, - course_grade=course_grade, generation_mode=generation_mode) + course_grade=course_grade, generation_mode=generation_mode, + delay_seconds=delay_seconds) status = _set_allowlist_cert_status(user, course_key, enrollment_mode, course_grade) if status is not None: @@ -61,7 +65,7 @@ def generate_allowlist_certificate_task(user, course_key, generation_mode=None): return False -def _generate_regular_certificate_task(user, course_key, generation_mode=None): +def _generate_regular_certificate_task(user, course_key, generation_mode=None, delay_seconds=CERTIFICATE_DELAY_SECONDS): """ Create a task to generate a regular (non-allowlist) certificate for this user in this course run, if the user is eligible and a certificate can be generated. @@ -70,7 +74,8 @@ def _generate_regular_certificate_task(user, course_key, generation_mode=None): course_grade = _get_course_grade(user, course_key) if _can_generate_regular_certificate(user, course_key, enrollment_mode, course_grade): return _generate_certificate_task(user=user, course_key=course_key, enrollment_mode=enrollment_mode, - course_grade=course_grade, generation_mode=generation_mode) + course_grade=course_grade, generation_mode=generation_mode, + delay_seconds=delay_seconds) status = _set_regular_cert_status(user, course_key, enrollment_mode, course_grade) if status is not None: @@ -79,7 +84,8 @@ def _generate_regular_certificate_task(user, course_key, generation_mode=None): return False -def _generate_certificate_task(user, course_key, enrollment_mode, course_grade, status=None, generation_mode=None): +def _generate_certificate_task(user, course_key, enrollment_mode, course_grade, status=None, generation_mode=None, + delay_seconds=CERTIFICATE_DELAY_SECONDS): """ Create a task to generate a certificate """ @@ -98,7 +104,7 @@ def _generate_certificate_task(user, course_key, enrollment_mode, course_grade, if generation_mode is not None: kwargs['generation_mode'] = generation_mode - generate_certificate.apply_async(countdown=CERTIFICATE_DELAY_SECONDS, kwargs=kwargs) + generate_certificate.apply_async(countdown=delay_seconds, kwargs=kwargs) return True diff --git a/lms/djangoapps/certificates/tasks.py b/lms/djangoapps/certificates/tasks.py index 184a353652..6d524352c3 100644 --- a/lms/djangoapps/certificates/tasks.py +++ b/lms/djangoapps/certificates/tasks.py @@ -15,6 +15,9 @@ from lms.djangoapps.certificates.generation import generate_course_certificate log = getLogger(__name__) User = get_user_model() + +# Certificate generation is delayed in case the caller is still completing their changes +# (for example a certificate regeneration reacting to a post save rather than post commit signal) CERTIFICATE_DELAY_SECONDS = 2 From 5ec9fca8d4c2390c67f03695c9ccf48f54ad95fd Mon Sep 17 00:00:00 2001 From: Andy Shultz Date: Thu, 21 Oct 2021 14:19:03 -0400 Subject: [PATCH 2/2] feat: provide command to regenerate certs for honor code courses Only courses which actually have honor code on and only unverified certs will be regenerated. All other certs wouldn't change so don't cause trouble. MST-855 --- .../commands/regenerate_noidv_cert.py | 97 ++++++++ .../tests/test_regenerate_noidv_cert.py | 229 ++++++++++++++++++ 2 files changed, 326 insertions(+) create mode 100644 lms/djangoapps/certificates/management/commands/regenerate_noidv_cert.py create mode 100644 lms/djangoapps/certificates/management/commands/tests/test_regenerate_noidv_cert.py diff --git a/lms/djangoapps/certificates/management/commands/regenerate_noidv_cert.py b/lms/djangoapps/certificates/management/commands/regenerate_noidv_cert.py new file mode 100644 index 0000000000..2bd4e57ae6 --- /dev/null +++ b/lms/djangoapps/certificates/management/commands/regenerate_noidv_cert.py @@ -0,0 +1,97 @@ +""" +Management command to regenerate unverified certificates when a course +transitions to honor code. +""" +import logging +import time + +from django.contrib.auth import get_user_model +from django.core.management.base import BaseCommand, CommandError +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey + +from lms.djangoapps.certificates.data import CertificateStatuses +from lms.djangoapps.certificates.generation_handler import generate_certificate_task +from lms.djangoapps.certificates.models import GeneratedCertificate +from openedx.core.djangoapps.agreements.toggles import is_integrity_signature_enabled + +log = logging.getLogger(__name__) +User = get_user_model() + + +class Command(BaseCommand): + """ + Management command to regenerate unverified certificates when a course + transitions to honor code. + """ + def add_arguments(self, parser): + parser.add_argument( + '-c', '--course-keys', + nargs='+', + dest='course_keys', + help='course run key or space separated list of course run keys' + ) + parser.add_argument( + '--batch_size', + action='store', + dest='batch_size', + type=int, + default=200, + help='Number of certs per batch' + ) + parser.add_argument( + '--sleep_seconds', + action='store', + dest='sleep_seconds', + type=int, + default=20, + help='Seconds to sleep between batches' + ) + + def handle(self, *args, **options): + courses_str = options['course_keys'] + if not courses_str: + raise CommandError('You must specify a course-key or keys') + + batch_size = options['batch_size'] + sleep_seconds = options['sleep_seconds'] + course_keys = [] + for course_id in courses_str: + # Parse the serialized course key into a CourseKey + try: + course_key = CourseKey.from_string(course_id) + except InvalidKeyError as e: + raise CommandError(f'{course_id} is not a valid course-key') from e + course_keys.append(course_key) + + count = 0 + for key in course_keys: + #rolling count to maintain batch_size across courses + count = _handle_course(key, batch_size, sleep_seconds, count) + return f'{count}' + + +def _handle_course(course_key, batch_size, sleep_seconds, count): + """ + Regenerates unverified status certificates for the designated course with delay seconds between certs. + Returns how many certs were originally unverified and regenerated. + """ + + if not is_integrity_signature_enabled(course_key): + log.warning(f'Skipping {course_key} which does not have honor code enabled') + return 0 + + certs = GeneratedCertificate.objects.filter( + course_id=course_key, + status=CertificateStatuses.unverified, + ) + + log.info(f'Regenerating {len(certs)} unverified certificates for {course_key}') + + for cert in certs: + user = User.objects.get(id=cert.user_id) + generate_certificate_task(user, course_key, generation_mode='batch', delay_seconds=0) + count += 1 + if count % batch_size == 0: + time.sleep(sleep_seconds) + return count diff --git a/lms/djangoapps/certificates/management/commands/tests/test_regenerate_noidv_cert.py b/lms/djangoapps/certificates/management/commands/tests/test_regenerate_noidv_cert.py new file mode 100644 index 0000000000..6273b8814c --- /dev/null +++ b/lms/djangoapps/certificates/management/commands/tests/test_regenerate_noidv_cert.py @@ -0,0 +1,229 @@ +""" +Tests for the cert_generation command +""" + +from unittest import mock + +import pytest +from django.core.management import CommandError, call_command + +from common.djangoapps.course_modes.models import CourseMode +from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory +from lms.djangoapps.certificates.data import CertificateStatuses +from lms.djangoapps.certificates.models import GeneratedCertificate +from lms.djangoapps.certificates.tests.factories import GeneratedCertificateFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory + +COMMAND_INTEGRITY_ENABLED = \ + 'lms.djangoapps.certificates.management.commands.regenerate_noidv_cert.is_integrity_signature_enabled' +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' + + +# base setup is unverified users, honor code (integrity) turned on wherever imports it +# and normal passing grade certificates for convenience +@mock.patch(ID_VERIFIED_METHOD, mock.Mock(return_value=False)) +@mock.patch(INTEGRITY_ENABLED_METHOD, mock.Mock(return_value=True)) +@mock.patch(COMMAND_INTEGRITY_ENABLED, mock.Mock(return_value=True)) +@mock.patch(PASSING_GRADE_METHOD, mock.Mock(return_value=True)) +@mock.patch(WEB_CERTS_METHOD, mock.Mock(return_value=True)) +class RegenerateNoIDVCertTests(ModuleStoreTestCase): + """ + Tests for the regenerate_noidv_cert management command + """ + + def test_command_with_missing_param_course_key(self): + """ + Verify command with a missing param -- course key. + """ + with pytest.raises(CommandError, match="You must specify a course-key or keys"): + call_command("regenerate_noidv_cert") + + def test_command_with_invalid_key(self): + """ + Verify command with an invalid course run key + """ + with pytest.raises(CommandError, match=" is not a valid course-key"): + call_command("regenerate_noidv_cert", "-c", "blah") + + def test_regeneration(self): + """ + Single regeneration base case + """ + course_run = CourseFactory() + course_run_key = course_run.id + + user = UserFactory() + CourseEnrollmentFactory( + user=user, + course_id=course_run_key, + is_active=True, + mode=CourseMode.VERIFIED, + ) + GeneratedCertificateFactory( + user=user, + course_id=course_run_key, + mode=GeneratedCertificate.MODES.verified, + status=CertificateStatuses.unverified + ) + + regenerated = call_command("regenerate_noidv_cert", "-c", course_run_key) + self.assertEqual('1', regenerated) + + def test_regeneration_verified(self): + """ + Only unverified certificates should get regenerated + """ + course_run = CourseFactory() + course_run_key = course_run.id + + user = UserFactory() + CourseEnrollmentFactory( + user=user, + course_id=course_run_key, + is_active=True, + mode=CourseMode.VERIFIED, + ) + GeneratedCertificateFactory( + user=user, + course_id=course_run_key, + mode=GeneratedCertificate.MODES.verified, + status=CertificateStatuses.downloadable + ) + + regenerated = call_command("regenerate_noidv_cert", "-c", course_run_key) + self.assertEqual('0', regenerated) + + def test_regeneration_honor_off(self): + """ + If a course does not have the honor code enabled, no point regenerating + """ + course_run = CourseFactory() + course_run_key = course_run.id + + user = UserFactory() + CourseEnrollmentFactory( + user=user, + course_id=course_run_key, + is_active=True, + mode=CourseMode.VERIFIED, + ) + GeneratedCertificateFactory( + user=user, + course_id=course_run_key, + mode=GeneratedCertificate.MODES.verified, + status=CertificateStatuses.unverified + ) + + with mock.patch(COMMAND_INTEGRITY_ENABLED, mock.Mock(return_value=False)): + regenerated = call_command("regenerate_noidv_cert", "-c", course_run_key) + self.assertEqual('0', regenerated) + + def _multisetup(self): + """ + setup certs across two course runs + """ + course_run = CourseFactory() + course_run_key = course_run.id + course_run2 = CourseFactory() + course_run_key2 = course_run2.id + + user = UserFactory() + CourseEnrollmentFactory( + user=user, + course_id=course_run_key, + is_active=True, + mode=CourseMode.VERIFIED, + ) + GeneratedCertificateFactory( + user=user, + course_id=course_run_key, + mode=GeneratedCertificate.MODES.verified, + status=CertificateStatuses.unverified + ) + + user1 = UserFactory() + CourseEnrollmentFactory( + user=user1, + course_id=course_run_key, + is_active=True, + mode=CourseMode.VERIFIED, + ) + GeneratedCertificateFactory( + user=user1, + course_id=course_run_key, + mode=GeneratedCertificate.MODES.verified, + status=CertificateStatuses.unverified + ) + + user2 = UserFactory() + CourseEnrollmentFactory( + user=user2, + course_id=course_run_key2, + is_active=True, + mode=CourseMode.VERIFIED, + ) + GeneratedCertificateFactory( + user=user2, + course_id=course_run_key2, + mode=GeneratedCertificate.MODES.verified, + status=CertificateStatuses.unverified + ) + + user_verified = UserFactory() + CourseEnrollmentFactory( + user=user_verified, + course_id=course_run_key, + is_active=True, + mode=CourseMode.VERIFIED, + ) + GeneratedCertificateFactory( + user=user_verified, + course_id=course_run_key, + mode=GeneratedCertificate.MODES.verified, + status=CertificateStatuses.downloadable + ) + + user_failing = UserFactory() + CourseEnrollmentFactory( + user=user_failing, + course_id=course_run_key2, + is_active=True, + mode=CourseMode.VERIFIED, + ) + GeneratedCertificateFactory( + user=user_failing, + course_id=course_run_key2, + mode=GeneratedCertificate.MODES.verified, + status=CertificateStatuses.notpassing + ) + return (course_run_key, course_run_key2) + + def test_multiple_regeneration(self): + """ + Verify regeneration across multiple courses and users + """ + course_run_key, course_run_key2 = self._multisetup() + + #3/5 in unverified status + regenerated = call_command("regenerate_noidv_cert", "-c", course_run_key, course_run_key2) + self.assertEqual('3', regenerated) + + #nothing left unverified for another run + regenerated = call_command("regenerate_noidv_cert", "-c", course_run_key, course_run_key2) + self.assertEqual('0', regenerated) + + def test_course_at_a_time(self): + """ + Verify course regeneration separated + """ + course_run_key, course_run_key2 = self._multisetup() + + regenerated = call_command("regenerate_noidv_cert", "-c", course_run_key) + self.assertEqual('2', regenerated) + + regenerated = call_command("regenerate_noidv_cert", "-c", course_run_key2) + self.assertEqual('1', regenerated)