fix issue where the wrong text appeared for certificate generation history (ECOM-4201)

lms/djangoapps/certificates/models.py

@@ -350,24 +350,31 @@ class CertificateGenerationHistory(TimeStampedModel):
             students.
         """
         task_input = self.instructor_task.task_input
-        try:
-            task_input_json = json.loads(task_input)
-        except ValueError:
+        if not task_input.strip():
             # if task input is empty, it means certificates were generated for all learners
             # Translators: This string represents task was executed for all learners.
             return _("All learners")
 
+        task_input_json = json.loads(task_input)
+
         # get statuses_to_regenerate from task_input convert statuses to human readable strings and return
         statuses = task_input_json.get('statuses_to_regenerate', None)
         if statuses:
-            return ", ".join(
-                [CertificateStatuses.readable_statuses.get(status, "") for status in statuses]
-            )
+            readable_statuses = [
+                CertificateStatuses.readable_statuses.get(status) for status in statuses
+                if CertificateStatuses.readable_statuses.get(status) is not None
+            ]
+            return ", ".join(readable_statuses)
 
-        # If students is present in task_input then, certificate generation task was run to
-        # generate certificates for white listed students otherwise it is for all students.
-        # Translators: This string represents task was executed for students having exceptions.
-        return _("For exceptions") if 'students' in task_input_json else _("All learners")
+        # If "student_set" is present in task_input, then this task only
+        # generates certificates for white listed students. Note that
+        # this key used to be "students", so we include that in this conditional
+        # for backwards compatibility.
+        if 'student_set' in task_input_json or 'students' in task_input_json:
+            # Translators: This string represents task was executed for students having exceptions.
+            return _("For exceptions")
+        else:
+            return _("All learners")
 
     class Meta(object):
         app_label = "certificates"

lms/djangoapps/certificates/tests/test_models.py

@@ -1,10 +1,13 @@
 """Tests for certificate Django models. """
+import ddt
 from django.conf import settings
 from django.core.exceptions import ValidationError
 from django.core.files.uploadedfile import SimpleUploadedFile
 from django.test import TestCase
 from django.test.utils import override_settings
 from nose.plugins.attrib import attr
+import json
+from mock import Mock
 from path import Path as path
 
 from certificates.models import (
@@ -14,10 +17,12 @@ from certificates.models import (
     CertificateTemplateAsset,
     GeneratedCertificate,
     CertificateStatuses,
+    CertificateGenerationHistory,
 )
 from certificates.tests.factories import GeneratedCertificateFactory
+from instructor_task.tests.factories import InstructorTaskFactory
 from opaque_keys.edx.locator import CourseLocator
-from student.tests.factories import UserFactory
+from student.tests.factories import AdminFactory, UserFactory
 from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 
@@ -229,3 +234,69 @@ class EligibleCertificateManagerTest(SharedModuleStoreTestCase):
             list(GeneratedCertificate.objects.filter(user=self.user)),  # pylint: disable=no-member
             [self.eligible_cert, self.ineligible_cert]
         )
+
+
+@attr('shard_1')
+@ddt.ddt
+class TestCertificateGenerationHistory(TestCase):
+    """
+    Test the CertificateGenerationHistory model's methods
+    """
+    @ddt.data(
+        ({"student_set": "whitelisted_not_generated"}, "For exceptions", True),
+        ({"student_set": "whitelisted_not_generated"}, "For exceptions", False),
+        # check "students" key for backwards compatibility
+        ({"students": [1, 2, 3]}, "For exceptions", True),
+        ({"students": [1, 2, 3]}, "For exceptions", False),
+        ({}, "All learners", True),
+        ({}, "All learners", False),
+        # test single status to regenerate returns correctly
+        ({"statuses_to_regenerate": ['downloadable']}, 'already received', True),
+        ({"statuses_to_regenerate": ['downloadable']}, 'already received', False),
+        # test that list of > 1 statuses render correctly
+        ({"statuses_to_regenerate": ['downloadable', 'error']}, 'already received, error states', True),
+        ({"statuses_to_regenerate": ['downloadable', 'error']}, 'already received, error states', False),
+        # test that only "readable" statuses are returned
+        ({"statuses_to_regenerate": ['downloadable', 'not_readable']}, 'already received', True),
+        ({"statuses_to_regenerate": ['downloadable', 'not_readable']}, 'already received', False),
+    )
+    @ddt.unpack
+    def test_get_certificate_generation_candidates(self, task_input, expected, is_regeneration):
+        staff = AdminFactory.create()
+        instructor_task = InstructorTaskFactory.create(
+            task_input=json.dumps(task_input),
+            requester=staff,
+            task_key=Mock(),
+            task_id=Mock(),
+        )
+        certificate_generation_history = CertificateGenerationHistory(
+            course_id=instructor_task.course_id,
+            generated_by=staff,
+            instructor_task=instructor_task,
+            is_regeneration=is_regeneration,
+        )
+        self.assertEqual(
+            certificate_generation_history.get_certificate_generation_candidates(),
+            expected
+        )
+
+    @ddt.data((True, "regenerated"), (False, "generated"))
+    @ddt.unpack
+    def test_get_task_name(self, is_regeneration, expected):
+        staff = AdminFactory.create()
+        instructor_task = InstructorTaskFactory.create(
+            task_input=json.dumps({}),
+            requester=staff,
+            task_key=Mock(),
+            task_id=Mock(),
+        )
+        certificate_generation_history = CertificateGenerationHistory(
+            course_id=instructor_task.course_id,
+            generated_by=staff,
+            instructor_task=instructor_task,
+            is_regeneration=is_regeneration,
+        )
+        self.assertEqual(
+            certificate_generation_history.get_task_name(),
+            expected
+        )