From cccb960f209722063beeff1acdfa9cca773ddd1d Mon Sep 17 00:00:00 2001 From: Deborah Kaplan Date: Tue, 16 Jan 2024 23:07:51 +0000 Subject: [PATCH] feat:tests for certificate template modifier still needs mocks for all tests to work FIXES: APER-2851 --- lms/djangoapps/certificates/admin.py | 8 +- .../commands/modify_cert_template.py | 13 +- .../tests/test_modify_certs_template.py | 47 +++++++ ...certificatetemplatecommandconfiguration.py | 4 +- lms/djangoapps/certificates/models.py | 3 +- lms/djangoapps/certificates/tasks.py | 6 +- .../certificates/tests/factories.py | 26 +++- .../certificates/tests/test_tasks.py | 121 ++++++++++++++---- 8 files changed, 183 insertions(+), 45 deletions(-) create mode 100644 lms/djangoapps/certificates/management/commands/tests/test_modify_certs_template.py diff --git a/lms/djangoapps/certificates/admin.py b/lms/djangoapps/certificates/admin.py index 8b98a2c971..3facf6f6b5 100644 --- a/lms/djangoapps/certificates/admin.py +++ b/lms/djangoapps/certificates/admin.py @@ -20,7 +20,8 @@ from lms.djangoapps.certificates.models import ( 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 index f300514617..d720c628e6 100644 --- a/lms/djangoapps/certificates/management/commands/modify_cert_template.py +++ b/lms/djangoapps/certificates/management/commands/modify_cert_template.py @@ -1,5 +1,4 @@ -"""Management command to modify certificate templates. -""" +"""Management command to modify certificate templates.""" import logging import shlex from argparse import RawDescriptionHelpFormatter @@ -41,18 +40,15 @@ class Command(BaseCommand): ) parser.add_argument( "--old-text", - required=True, help="Text to replace in the template.", ) parser.add_argument( "--new-text", - required=True, help="Replacement text for the template.", ) parser.add_argument( "--templates", nargs="+", - required=True, help="Certificate templates to modify.", ) parser.add_argument( @@ -63,7 +59,7 @@ class Command(BaseCommand): def get_args_from_database(self): """ - Returns an options dictionary from the current CertificateGenerationCommandConfiguration model. + Returns an options dictionary from the current ModifiedCertificateTemplateCommandConfiguration instance. """ config = ModifiedCertificateTemplateCommandConfiguration.current() if not config.enabled: @@ -78,6 +74,11 @@ class Command(BaseCommand): # 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( 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 0000000000..2ab242d101 --- /dev/null +++ b/lms/djangoapps/certificates/management/commands/tests/test_modify_certs_template.py @@ -0,0 +1,47 @@ +""" +Tests for the modify_cert_template command +""" + +import pytest +from django.core.management import CommandError, call_command +from django.test import ( + TestCase, + override_settings, +) # lint-amnesty, pylint: disable=unused-import + + +class ModifyCertTemplateTests(TestCase): + """Tests for the modify_cert_template management command""" + + def setUp(self): + super().setUp() + + 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_old_text(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 index d318a74636..ebc6cfd95b 100644 --- a/lms/djangoapps/certificates/migrations/0036_modifiedcertificatetemplatecommandconfiguration.py +++ b/lms/djangoapps/certificates/migrations/0036_modifiedcertificatetemplatecommandconfiguration.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.23 on 2024-01-11 22:12 +# Generated by Django 3.2.23 on 2024-01-16 18:57 from django.conf import settings from django.db import migrations, models @@ -19,7 +19,7 @@ class Migration(migrations.Migration): ('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 '-template_ids '")), + ('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={ diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index 1798881c81..9c58327b99 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -1256,7 +1256,8 @@ class ModifiedCertificateTemplateCommandConfiguration(ConfigurationModel): arguments = models.TextField( blank=True, help_text=( - "Arguments for the 'modify_cert_template' management command. Specify like '-template_ids '" + "Arguments for the 'modify_cert_template' management command. Specify like '--old-text \"foo\" " + "--new-text \"bar\" --template_ids '" ), default="", ) diff --git a/lms/djangoapps/certificates/tasks.py b/lms/djangoapps/certificates/tasks.py index 2b710626d2..06078e3461 100644 --- a/lms/djangoapps/certificates/tasks.py +++ b/lms/djangoapps/certificates/tasks.py @@ -66,15 +66,16 @@ def handle_modify_cert_template(options: Dict[str, Any]): Celery task to handle the modify_cert_template management command. Args: - FIXME + 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 = [] - # FIXME Check to see if there was that particular logging configuration log.info( "[modify_cert_template] Attempting to modify {num} templates".format( num=len(template_ids) @@ -130,7 +131,6 @@ def handle_modify_cert_template(options: Dict[str, Any]): ) ), ) - # FIXME commit templates_to_change log.info( "[modify_cert_template] Modified {num} templates".format(num=templates_changed) ) diff --git a/lms/djangoapps/certificates/tests/factories.py b/lms/djangoapps/certificates/tests/factories.py index 15d5386aa4..ad77278760 100644 --- a/lms/djangoapps/certificates/tests/factories.py +++ b/lms/djangoapps/certificates/tests/factories.py @@ -6,6 +6,7 @@ Certificates factories 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 @@ from lms.djangoapps.certificates.models import ( 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 CertificateAllowlistFactory(DjangoModelFactory): 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 c2c73f053c..c02c5f1dbf 100644 --- a/lms/djangoapps/certificates/tests/test_tasks.py +++ b/lms/djangoapps/certificates/tests/test_tasks.py @@ -3,6 +3,7 @@ Tests for course certificate tasks. """ +from textwrap import dedent from unittest import mock from unittest.mock import patch @@ -13,7 +14,11 @@ from opaque_keys.edx.keys import CourseKey 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, + handle_modify_cert_template, +) +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 @@ class GenerateUserCertificateTest(TestCase): 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 @@ class GenerateUserCertificateTest(TestCase): 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,66 @@ class GenerateUserCertificateTest(TestCase): status=status, enrollment_mode=enrollment_mode, course_grade=course_grade, - generation_mode=gen_mode + generation_mode=gen_mode, ) + + +class ModifyCertTemplateTests(TestCase): + """Tests for handle_modify_cert_template""" + + # FIXME: put in mocks for .get and .save + def setUp(self): + super().setUp() + self.cert1 = CertificateTemplateFactory + self.cert2 = CertificateTemplateFactory + self.cert3 = CertificateTemplateFactory + + def test_command_changes_called_templates(self): + """Verify command changes for all and only those templates for which it is called.""" + self.cert1.template = "fiddledee-doo fiddledee-dah" + self.cert2.template = "violadee-doo violadee-dah" + self.cert3.template = "fiddledee-doo fiddledee-dah" + expected1 = "fiddleeep-doo fiddledee-dah" + expected2 = "violaeep-doo violadee-dah" + expected3 = "fiddledee-doo fiddledee-dah" + options = { + "old_text": "dee", + "new_text": "eep", + "templates": [1, 2], + } + handle_modify_cert_template(options) + assert self.cert1.template == expected1 + assert self.cert2.template == expected2 + assert self.cert3.template == expected3 + + def test_dry_run(self): + """Verify command doesn't change anything on dry-run.""" + self.cert1.template = "fiddledee-doo fiddledee-dah" + self.cert2.template = "violadee-doo violadee-dah" + expected1 = "fiddledee-doo fiddledee-dah" + expected2 = "violadee-doo violadee-dah" + options = { + "old_text": "dee", + "new_text": "eep", + "templates": [1, 2], + "dry_run": True, + } + handle_modify_cert_template(options) + assert self.cert1.template == expected1 + assert self.cert2.template == expected2 + + def test_multiline_change(self): + """Verify template change works with a multiline change string.""" + self.cert1.template = "fiddledee-doo fiddledee-dah" + 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], + } + handle_modify_cert_template(options) + assert self.cert1.template == expected