From 994b8a3d49fdb4409137d42c2c0baa4629120d55 Mon Sep 17 00:00:00 2001 From: Deborah Kaplan Date: Wed, 17 Jan 2024 21:41:21 +0000 Subject: [PATCH] feat: adding tests to modify cert template Re-factored to make the code more testable, and added some more tests, also improved dry run logging FIXES: APER-2851 --- .../tests/test_modify_certs_template.py | 3 - lms/djangoapps/certificates/tasks.py | 107 ++++++++++-------- .../certificates/tests/test_tasks.py | 62 +++++----- 3 files changed, 96 insertions(+), 76 deletions(-) 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 index 2ab242d101..c6ac0d2c90 100644 --- a/lms/djangoapps/certificates/management/commands/tests/test_modify_certs_template.py +++ b/lms/djangoapps/certificates/management/commands/tests/test_modify_certs_template.py @@ -13,9 +13,6 @@ from django.test 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( diff --git a/lms/djangoapps/certificates/tasks.py b/lms/djangoapps/certificates/tasks.py index 06078e3461..8229cef394 100644 --- a/lms/djangoapps/certificates/tasks.py +++ b/lms/djangoapps/certificates/tasks.py @@ -4,7 +4,7 @@ Tasks that operate on course certificates for a user from difflib import unified_diff from logging import getLogger -from typing import Any, Dict +from typing import Any, Dict, List from celery import shared_task from celery_utils.persist_on_failure import LoggedPersistOnFailureTask, LoggedTask @@ -61,7 +61,7 @@ def generate_certificate(self, **kwargs): # pylint: disable=unused-argument @shared_task(base=LoggedTask, ignore_result=True) @set_code_owner_attribute -def handle_modify_cert_template(options: Dict[str, Any]): +def handle_modify_cert_template(options: Dict[str, Any]) -> None: """ Celery task to handle the modify_cert_template management command. @@ -81,7 +81,34 @@ def handle_modify_cert_template(options: Dict[str, Any]): num=len(template_ids) ) ) - templates_changed = 0 + + 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: @@ -94,8 +121,8 @@ def handle_modify_cert_template(options: Dict[str, Any]): template_id=template_id, name=template.description ) ) - new_template = get_modified_template_text( - template.template, options["old_text"], options["new_text"] + new_template = template.template.replace( + options["old_text"], options["new_text"], 1 ) if template.template == new_template: log.info( @@ -104,48 +131,36 @@ def handle_modify_cert_template(options: Dict[str, Any]): ) ) else: - log.info( - "[modify_cert_template] Modifying template {template} ({description})".format( - template=template_id, - description=template.description, - ) - ) - templates_changed += 1 - if not options["dry_run"]: - template.template = new_template - template.save() - 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", + 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=templates_changed) + "[modify_cert_template] Modified {num} templates".format( + num=len(templates_changed) + ) ) - -def get_modified_template_text( - template_text: str, - old: str, - new: str, -): - """ - Returns the original template text with the first instance of `old` replaced with `new`. - Case-sensitive. - - Although this is a trivial method, it's factored into its own method to allow us to - write unit tests that can be easily modified if the testing algorithm is made more complex. - """ - return template_text.replace(old, new, 1) + return templates_changed diff --git a/lms/djangoapps/certificates/tests/test_tasks.py b/lms/djangoapps/certificates/tests/test_tasks.py index c02c5f1dbf..8cf8150235 100644 --- a/lms/djangoapps/certificates/tests/test_tasks.py +++ b/lms/djangoapps/certificates/tests/test_tasks.py @@ -16,7 +16,7 @@ from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.certificates.data import CertificateStatuses from lms.djangoapps.certificates.tasks import ( generate_certificate, - handle_modify_cert_template, + get_changed_cert_templates, ) from lms.djangoapps.certificates.tests.factories import CertificateTemplateFactory @@ -108,52 +108,59 @@ class GenerateUserCertificateTest(TestCase): 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 + """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.""" - self.cert1.template = "fiddledee-doo fiddledee-dah" - self.cert2.template = "violadee-doo violadee-dah" - self.cert3.template = "fiddledee-doo fiddledee-dah" + self.template1 = CertificateTemplateFactory.create( + template="fiddledee-doo fiddledee-dah" + ) + self.template2 = CertificateTemplateFactory.create( + template="violadee-doo violadee-dah" + ) + self.template3 = CertificateTemplateFactory.create( + template="fiddledee-doo fiddledee-dah" + ) + self.template1.save() + self.template2.save() + self.template3.save() 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 + 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.""" - self.cert1.template = "fiddledee-doo fiddledee-dah" - self.cert2.template = "violadee-doo violadee-dah" - expected1 = "fiddledee-doo fiddledee-dah" - expected2 = "violadee-doo violadee-dah" + self.template1 = CertificateTemplateFactory.create( + template="fiddledee-doo fiddledee-dah" + ) + self.template2 = CertificateTemplateFactory.create( + template="violadee-doo violadee-dah" + ) + self.template1.save() + self.template2.save() 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 + new_templates = get_changed_cert_templates(options) + assert new_templates == [] def test_multiline_change(self): """Verify template change works with a multiline change string.""" - self.cert1.template = "fiddledee-doo fiddledee-dah" + self.template1 = CertificateTemplateFactory.create( + template="fiddledee-doo fiddledee-dah" + ) + self.template1.save() new_text = """ there's something happening here what it is ain't exactly clear @@ -164,5 +171,6 @@ class ModifyCertTemplateTests(TestCase): "new_text": dedent(new_text), "templates": [1], } - handle_modify_cert_template(options) - assert self.cert1.template == expected + new_templates = get_changed_cert_templates(options) + assert len(new_templates) == 1 + assert new_templates[0].template == expected