diff --git a/lms/djangoapps/certificates/admin.py b/lms/djangoapps/certificates/admin.py index 8b98a2c97151..3facf6f6b530 100644 --- a/lms/djangoapps/certificates/admin.py +++ b/lms/djangoapps/certificates/admin.py @@ -20,7 +20,8 @@ CertificateHtmlViewConfiguration, CertificateTemplate, CertificateTemplateAsset, - GeneratedCertificate + GeneratedCertificate, + ModifiedCertificateTemplateCommandConfiguration, ) @@ -92,6 +93,11 @@ class CertificateGenerationCourseSettingAdmin(admin.ModelAdmin): show_full_result_count = False +@admin.register(ModifiedCertificateTemplateCommandConfiguration) +class ModifiedCertificateTemplateCommandConfigurationAdmin(ConfigurationModelAdmin): + pass + + @admin.register(CertificateGenerationCommandConfiguration) class CertificateGenerationCommandConfigurationAdmin(ConfigurationModelAdmin): pass diff --git a/lms/djangoapps/certificates/management/commands/modify_cert_template.py b/lms/djangoapps/certificates/management/commands/modify_cert_template.py new file mode 100644 index 000000000000..f4ae5f64d2b6 --- /dev/null +++ b/lms/djangoapps/certificates/management/commands/modify_cert_template.py @@ -0,0 +1,91 @@ +"""Management command to modify certificate templates.""" +import logging +import shlex +from argparse import RawDescriptionHelpFormatter + +from django.core.management.base import BaseCommand, CommandError + +from lms.djangoapps.certificates.models import ( + ModifiedCertificateTemplateCommandConfiguration, +) +from lms.djangoapps.certificates.tasks import handle_modify_cert_template + +log = logging.getLogger(__name__) + + +class Command(BaseCommand): + """Management command to modify certificate templates. + Example usage: + ./manage.py lms modify_cert_template --old-text "" --new text "

boo!

" --templates 867 3509 + """ + + help = """Modify one or more certificate templates. + This is DANGEROUS. + * This uses string replacement to modify HTML-like templates, because the presence of + Django Templating makes it impossible to do true parsing. + * This isn't parameterizing the replacement text, for the same reason. It has + no way of knowing what is template language and what is HTML. + Do not trust that this will get the conversion right without verification, + and absolutely do not accepted untrusted user input for the replacement text. This is + to be run by trusted users only. + Always run this with dry-run or in a reliable test environment. + """ + + def add_arguments(self, parser): + parser.formatter_class = RawDescriptionHelpFormatter + parser.add_argument( + "--dry-run", + action="store_true", + help="Just show a preview of what would happen.", + ) + parser.add_argument( + "--old-text", + help="Text to replace in the template.", + ) + parser.add_argument( + "--new-text", + help="Replacement text for the template.", + ) + parser.add_argument( + "--templates", + nargs="+", + help="Certificate templates to modify.", + ) + parser.add_argument( + "--args-from-database", + action="store_true", + help="Use arguments from the ModifyCertificateTemplateConfiguration model instead of the command line.", + ) + + def get_args_from_database(self): + """ + Returns an options dictionary from the current ModifiedCertificateTemplateCommandConfiguration instance. + """ + config = ModifiedCertificateTemplateCommandConfiguration.current() + if not config.enabled: + raise CommandError( + "ModifyCertificateTemplateCommandConfiguration is disabled, but --args-from-database was requested" + ) + args = shlex.split(config.arguments) + parser = self.create_parser("manage.py", "modify_cert_template") + return vars(parser.parse_args(args)) + + def handle(self, *args, **options): + # database args will override cmd line args + if options["args_from_database"]: + options = self.get_args_from_database() + # Check required arguments here. We can't rely on marking args "required" because they might come from django + if not (options["old_text"] and options["new_text"] and options["templates"]): + raise CommandError( + "The following arguments are required: --old-text, --new-text, --templates" + ) + log.info( + "modify_cert_template starting, dry-run={dry_run}, templates={templates}, " + "old-text={old}, new-text={new}".format( + dry_run=options["dry_run"], + templates=options["templates"], + old=options["old_text"], + new=options["new_text"], + ) + ) + handle_modify_cert_template.delay(options) diff --git a/lms/djangoapps/certificates/management/commands/tests/test_modify_certs_template.py b/lms/djangoapps/certificates/management/commands/tests/test_modify_certs_template.py new file mode 100644 index 000000000000..03d9b05d5413 --- /dev/null +++ b/lms/djangoapps/certificates/management/commands/tests/test_modify_certs_template.py @@ -0,0 +1,41 @@ +""" +Tests for the modify_cert_template command +""" + +import pytest +from django.core.management import CommandError, call_command +from django.test import TestCase + + +class ModifyCertTemplateTests(TestCase): + """Tests for the modify_cert_template management command""" + + def test_command_with_missing_param_old_text(self): + """Verify command with a missing param --old-text.""" + with pytest.raises( + CommandError, + match="The following arguments are required: --old-text, --new-text, --templates", + ): + call_command( + "modify_cert_template", "--new-text", "blah", "--templates", "1 2 3" + ) + + def test_command_with_missing_param_new_text(self): + """Verify command with a missing param --new-text.""" + with pytest.raises( + CommandError, + match="The following arguments are required: --old-text, --new-text, --templates", + ): + call_command( + "modify_cert_template", "--old-text", "blah", "--templates", "1 2 3" + ) + + def test_command_with_missing_param_templates(self): + """Verify command with a missing param --templates.""" + with pytest.raises( + CommandError, + match="The following arguments are required: --old-text, --new-text, --templates", + ): + call_command( + "modify_cert_template", "--new-text", "blah", "--old-text", "xyzzy" + ) diff --git a/lms/djangoapps/certificates/migrations/0036_modifiedcertificatetemplatecommandconfiguration.py b/lms/djangoapps/certificates/migrations/0036_modifiedcertificatetemplatecommandconfiguration.py new file mode 100644 index 000000000000..ebc6cfd95b9a --- /dev/null +++ b/lms/djangoapps/certificates/migrations/0036_modifiedcertificatetemplatecommandconfiguration.py @@ -0,0 +1,29 @@ +# Generated by Django 3.2.23 on 2024-01-16 18:57 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('certificates', '0035_auto_20230808_0944'), + ] + + operations = [ + migrations.CreateModel( + name='ModifiedCertificateTemplateCommandConfiguration', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('change_date', models.DateTimeField(auto_now_add=True, verbose_name='Change date')), + ('enabled', models.BooleanField(default=False, verbose_name='Enabled')), + ('arguments', models.TextField(blank=True, default='', help_text='Arguments for the \'modify_cert_template\' management command. Specify like \'--old-text "foo" --new-text "bar" --template_ids \'')), + ('changed_by', models.ForeignKey(editable=False, null=True, on_delete=django.db.models.deletion.PROTECT, to=settings.AUTH_USER_MODEL, verbose_name='Changed by')), + ], + options={ + 'verbose_name': 'modify_cert_template argument', + }, + ), + ] diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index fa16112f3add..9c58327b99aa 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -2,11 +2,11 @@ Course certificates are created for a student and an offering of a course (a course run). """ -from datetime import timezone import json import logging import os import uuid +from datetime import timezone from config_models.models import ConfigurationModel from django.apps import apps @@ -16,7 +16,6 @@ from django.db import models, transaction from django.db.models import Count from django.dispatch import receiver - from django.utils.translation import gettext_lazy as _ from model_utils import Choices from model_utils.models import TimeStampedModel @@ -1243,6 +1242,30 @@ class Meta: app_label = "certificates" +class ModifiedCertificateTemplateCommandConfiguration(ConfigurationModel): + """ + Manages configuration for a run of the modify_cert_template management command. + + .. no_pii: + """ + + class Meta: + app_label = "certificates" + verbose_name = "modify_cert_template argument" + + arguments = models.TextField( + blank=True, + help_text=( + "Arguments for the 'modify_cert_template' management command. Specify like '--old-text \"foo\" " + "--new-text \"bar\" --template_ids '" + ), + default="", + ) + + def __str__(self): + return str(self.arguments) + + class CertificateGenerationCommandConfiguration(ConfigurationModel): """ Manages configuration for a run of the cert_generation management command. diff --git a/lms/djangoapps/certificates/tasks.py b/lms/djangoapps/certificates/tasks.py index 6d524352c378..8229cef3948c 100644 --- a/lms/djangoapps/certificates/tasks.py +++ b/lms/djangoapps/certificates/tasks.py @@ -1,17 +1,20 @@ """ -Tasks that generate a course certificate for a user +Tasks that operate on course certificates for a user """ +from difflib import unified_diff from logging import getLogger +from typing import Any, Dict, List from celery import shared_task -from celery_utils.persist_on_failure import LoggedPersistOnFailureTask +from celery_utils.persist_on_failure import LoggedPersistOnFailureTask, LoggedTask from django.contrib.auth import get_user_model from edx_django_utils.monitoring import set_code_owner_attribute from opaque_keys.edx.keys import CourseKey from lms.djangoapps.certificates.data import CertificateStatuses from lms.djangoapps.certificates.generation import generate_course_certificate +from lms.djangoapps.certificates.models import CertificateTemplate log = getLogger(__name__) User = get_user_model() @@ -21,7 +24,9 @@ CERTIFICATE_DELAY_SECONDS = 2 -@shared_task(base=LoggedPersistOnFailureTask, bind=True, default_retry_delay=30, max_retries=2) +@shared_task( + base=LoggedPersistOnFailureTask, bind=True, default_retry_delay=30, max_retries=2 +) @set_code_owner_attribute def generate_certificate(self, **kwargs): # pylint: disable=unused-argument """ @@ -37,12 +42,125 @@ def generate_certificate(self, **kwargs): # pylint: disable=unused-argument - generation_mode: Used when emitting an event. Options are "self" (implying the user generated the cert themself) and "batch" for everything else. Defaults to 'batch'. """ - student = User.objects.get(id=kwargs.pop('student')) - course_key = CourseKey.from_string(kwargs.pop('course_key')) - status = kwargs.pop('status', CertificateStatuses.downloadable) - enrollment_mode = kwargs.pop('enrollment_mode') - course_grade = kwargs.pop('course_grade', '') - generation_mode = kwargs.pop('generation_mode', 'batch') - - generate_course_certificate(user=student, course_key=course_key, status=status, enrollment_mode=enrollment_mode, - course_grade=course_grade, generation_mode=generation_mode) + student = User.objects.get(id=kwargs.pop("student")) + course_key = CourseKey.from_string(kwargs.pop("course_key")) + status = kwargs.pop("status", CertificateStatuses.downloadable) + enrollment_mode = kwargs.pop("enrollment_mode") + course_grade = kwargs.pop("course_grade", "") + generation_mode = kwargs.pop("generation_mode", "batch") + + generate_course_certificate( + user=student, + course_key=course_key, + status=status, + enrollment_mode=enrollment_mode, + course_grade=course_grade, + generation_mode=generation_mode, + ) + + +@shared_task(base=LoggedTask, ignore_result=True) +@set_code_owner_attribute +def handle_modify_cert_template(options: Dict[str, Any]) -> None: + """ + Celery task to handle the modify_cert_template management command. + + Args: + old_text (string): Text in the template of which the first instance should be changed + new_text (string): Replacement text for old_text + template_ids (list[string]): List of template IDs for this run. + dry_run (boolean): Don't do the work, just report the changes that would happen + """ + + template_ids = options["templates"] + if not template_ids: + template_ids = [] + + log.info( + "[modify_cert_template] Attempting to modify {num} templates".format( + num=len(template_ids) + ) + ) + + templates_changed = get_changed_cert_templates(options) + for template in templates_changed: + template.save() + + +def get_changed_cert_templates(options: Dict[str, Any]) -> List[CertificateTemplate]: + """ + Loop through the templates and return instances with changed template text. + + Args: + old_text (string): Text in the template of which the first instance should be changed + new_text (string): Replacement text for old_text + template_ids (list[string]): List of template IDs for this run. + dry_run (boolean): Don't do the work, just report the changes that would happen + """ + template_ids = options["templates"] + if not template_ids: + template_ids = [] + + log.info( + "[modify_cert_template] Attempting to modify {num} templates".format( + num=len(template_ids) + ) + ) + dry_run = options.get("dry_run", None) + templates_changed = [] + + for template_id in template_ids: + template = None + try: + template = CertificateTemplate.objects.get(id=template_id) + except CertificateTemplate.DoesNotExist: + log.warning(f"Template {template_id} could not be found") + if template is not None: + log.info( + "[modify_cert_template] Calling for template {template_id} : {name}".format( + template_id=template_id, name=template.description + ) + ) + new_template = template.template.replace( + options["old_text"], options["new_text"], 1 + ) + if template.template == new_template: + log.info( + "[modify_cert_template] No changes to {template_id}".format( + template_id=template_id + ) + ) + else: + if not dry_run: + log.info( + "[modify_cert_template] Modifying template {template} ({description})".format( + template=template_id, + description=template.description, + ) + ) + template.template = new_template + templates_changed.append(template) + else: + log.info( + "DRY-RUN: Not making the following template change to {id}.".format( + id=template_id + ) + ) + log.info( + "\n".join( + unified_diff( + template.template.splitlines(), + new_template.splitlines(), + lineterm="", + fromfile="old_template", + tofile="new_template", + ) + ), + ) + log.info( + "[modify_cert_template] Modified {num} templates".format( + num=len(templates_changed) + ) + ) + + return templates_changed diff --git a/lms/djangoapps/certificates/tests/factories.py b/lms/djangoapps/certificates/tests/factories.py index 15d5386aa445..ad7727876076 100644 --- a/lms/djangoapps/certificates/tests/factories.py +++ b/lms/djangoapps/certificates/tests/factories.py @@ -6,6 +6,7 @@ import datetime from uuid import uuid4 +from factory import Sequence from factory.django import DjangoModelFactory from common.djangoapps.student.models import LinkedInAddToProfileConfiguration @@ -15,7 +16,8 @@ CertificateHtmlViewConfiguration, CertificateInvalidation, CertificateStatuses, - GeneratedCertificate + CertificateTemplate, + GeneratedCertificate, ) @@ -23,15 +25,16 @@ class GeneratedCertificateFactory(DjangoModelFactory): """ GeneratedCertificate factory """ + class Meta: model = GeneratedCertificate course_id = None status = CertificateStatuses.unavailable mode = GeneratedCertificate.MODES.honor - name = '' + name = "" verify_uuid = uuid4().hex - grade = '' + grade = "" class CertificateAllowlistFactory(DjangoModelFactory): @@ -44,7 +47,7 @@ class Meta: course_id = None allowlist = True - notes = 'Test Notes' + notes = "Test Notes" class CertificateInvalidationFactory(DjangoModelFactory): @@ -55,7 +58,7 @@ class CertificateInvalidationFactory(DjangoModelFactory): class Meta: model = CertificateInvalidation - notes = 'Test Notes' + notes = "Test Notes" active = True @@ -112,8 +115,21 @@ class CertificateDateOverrideFactory(DjangoModelFactory): """ CertificateDateOverride factory """ + class Meta: model = CertificateDateOverride date = datetime.datetime(2021, 5, 11, 0, 0, tzinfo=datetime.timezone.utc) reason = "Learner really wanted this on their birthday" + + +class CertificateTemplateFactory(DjangoModelFactory): + """CertificateTemplate factory""" + + class Meta: + model = CertificateTemplate + + name = Sequence("template{}".format) + description = Sequence("description for template{}".format) + template = "" + is_active = True diff --git a/lms/djangoapps/certificates/tests/test_tasks.py b/lms/djangoapps/certificates/tests/test_tasks.py index c2c73f053cdd..2e6c84464936 100644 --- a/lms/djangoapps/certificates/tests/test_tasks.py +++ b/lms/djangoapps/certificates/tests/test_tasks.py @@ -3,6 +3,7 @@ """ +from textwrap import dedent from unittest import mock from unittest.mock import patch @@ -13,7 +14,11 @@ from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.certificates.data import CertificateStatuses -from lms.djangoapps.certificates.tasks import generate_certificate +from lms.djangoapps.certificates.tasks import ( + generate_certificate, + get_changed_cert_templates, +) +from lms.djangoapps.certificates.tests.factories import CertificateTemplateFactory @ddt.ddt @@ -21,23 +26,24 @@ class GenerateUserCertificateTest(TestCase): """ Tests for course certificate tasks """ + def setUp(self): super().setUp() self.user = UserFactory() - self.course_key = 'course-v1:edX+DemoX+Demo_Course' + self.course_key = "course-v1:edX+DemoX+Demo_Course" - @ddt.data('student', 'course_key', 'enrollment_mode') + @ddt.data("student", "course_key", "enrollment_mode") def test_missing_args(self, missing_arg): kwargs = { - 'student': self.user.id, - 'course_key': self.course_key, - 'other_arg': 'shiny', - 'enrollment_mode': CourseMode.MASTERS + "student": self.user.id, + "course_key": self.course_key, + "other_arg": "shiny", + "enrollment_mode": CourseMode.MASTERS, } del kwargs[missing_arg] - with patch('lms.djangoapps.certificates.tasks.User.objects.get'): + with patch("lms.djangoapps.certificates.tasks.User.objects.get"): with self.assertRaisesRegex(KeyError, missing_arg): generate_certificate.apply_async(kwargs=kwargs).get() @@ -48,13 +54,13 @@ def test_generation(self): enrollment_mode = CourseMode.VERIFIED with mock.patch( - 'lms.djangoapps.certificates.tasks.generate_course_certificate', - return_value=None + "lms.djangoapps.certificates.tasks.generate_course_certificate", + return_value=None, ) as mock_generate_cert: kwargs = { - 'student': self.user.id, - 'course_key': self.course_key, - 'enrollment_mode': enrollment_mode + "student": self.user.id, + "course_key": self.course_key, + "enrollment_mode": enrollment_mode, } generate_certificate.apply_async(kwargs=kwargs) @@ -63,31 +69,31 @@ def test_generation(self): course_key=CourseKey.from_string(self.course_key), status=CertificateStatuses.downloadable, enrollment_mode=enrollment_mode, - course_grade='', - generation_mode='batch' + course_grade="", + generation_mode="batch", ) def test_generation_custom(self): """ Verify the task handles certificate generation custom params """ - gen_mode = 'self' + gen_mode = "self" status = CertificateStatuses.notpassing enrollment_mode = CourseMode.AUDIT - course_grade = '0.89' + course_grade = "0.89" with mock.patch( - 'lms.djangoapps.certificates.tasks.generate_course_certificate', - return_value=None + "lms.djangoapps.certificates.tasks.generate_course_certificate", + return_value=None, ) as mock_generate_cert: kwargs = { - 'status': status, - 'student': self.user.id, - 'course_key': self.course_key, - 'course_grade': course_grade, - 'enrollment_mode': enrollment_mode, - 'generation_mode': gen_mode, - 'what_about': 'dinosaurs' + "status": status, + "student": self.user.id, + "course_key": self.course_key, + "course_grade": course_grade, + "enrollment_mode": enrollment_mode, + "generation_mode": gen_mode, + "what_about": "dinosaurs", } generate_certificate.apply_async(kwargs=kwargs) @@ -97,5 +103,74 @@ def test_generation_custom(self): status=status, enrollment_mode=enrollment_mode, course_grade=course_grade, - generation_mode=gen_mode + generation_mode=gen_mode, ) + + +class ModifyCertTemplateTests(TestCase): + """Tests for get_changed_cert_templates""" + + def test_command_changes_called_templates(self): + """Verify command changes for all and only those templates for which it is called.""" + template1 = CertificateTemplateFactory.create( + template="fiddledee-doo fiddledee-dah" + ) + template2 = CertificateTemplateFactory.create( + template="violadee-doo violadee-dah" + ) + template3 = CertificateTemplateFactory.create( + template="fiddledee-doo fiddledee-dah" + ) + template1.save() + template2.save() + template3.save() + expected1 = "fiddleeep-doo fiddledee-dah" + expected2 = "violaeep-doo violadee-dah" + options = { + "old_text": "dee", + "new_text": "eep", + "templates": [1, 2], + } + new_templates = get_changed_cert_templates(options) + assert len(new_templates) == 2 + assert new_templates[0].template == expected1 + assert new_templates[1].template == expected2 + + def test_dry_run(self): + """Verify command doesn't change anything on dry-run.""" + template1 = CertificateTemplateFactory.create( + template="fiddledee-doo fiddledee-dah" + ) + template2 = CertificateTemplateFactory.create( + template="violadee-doo violadee-dah" + ) + template1.save() + template2.save() + options = { + "old_text": "dee", + "new_text": "eep", + "templates": [1, 2], + "dry_run": True, + } + new_templates = get_changed_cert_templates(options) + assert not new_templates + + def test_multiline_change(self): + """Verify template change works with a multiline change string.""" + template1 = CertificateTemplateFactory.create( + template="fiddledee-doo fiddledee-dah" + ) + template1.save() + new_text = """ + there's something happening here + what it is ain't exactly clear + """ + expected = f"fiddle{dedent(new_text)}-doo fiddledee-dah" + options = { + "old_text": "dee", + "new_text": dedent(new_text), + "templates": [1], + } + new_templates = get_changed_cert_templates(options) + assert len(new_templates) == 1 + assert new_templates[0].template == expected