Merge pull request #12140 from edx/fix-certificate-query

Fix certificate query

common/djangoapps/student/tests/tests.py

@@ -117,22 +117,6 @@ class CourseEndingTest(TestCase):
             }
         )
 
-        cert_status = {'status': 'regenerating', 'grade': '67', 'mode': 'verified'}
-        self.assertEqual(
-            _cert_info(user, course, cert_status, course_mode),
-            {
-                'status': 'generating',
-                'show_disabled_download_button': True,
-                'show_download_url': False,
-                'show_survey_button': True,
-                'survey_url': survey_url,
-                'grade': '67',
-                'mode': 'verified',
-                'linked_in_url': None,
-                'can_unenroll': False,
-            }
-        )
-
         download_url = 'http://s3.edx/cert'
         cert_status = {
             'status': 'downloadable', 'grade': '67',

common/djangoapps/student/views.py

@@ -312,7 +312,6 @@ def _cert_info(user, course_overview, cert_status, course_mode):  # pylint: disa
     # simplify the status for the template using this lookup table
     template_state = {
         CertificateStatuses.generating: 'generating',
-        CertificateStatuses.regenerating: 'generating',
         CertificateStatuses.downloadable: 'ready',
         CertificateStatuses.notpassing: 'notpassing',
         CertificateStatuses.restricted: 'restricted',

lms/djangoapps/certificates/models.py

@@ -83,7 +83,6 @@ class CertificateStatuses(object):
     error = 'error'
     generating = 'generating'
     notpassing = 'notpassing'
-    regenerating = 'regenerating'
     restricted = 'restricted'
     unavailable = 'unavailable'
     auditing = 'auditing'
@@ -96,7 +95,7 @@ class CertificateStatuses(object):
         error: "error states"
     }
 
-    PASSED_STATUSES = (downloadable, generating, regenerating)
+    PASSED_STATUSES = (downloadable, generating)
 
     @classmethod
     def is_passing_status(cls, status):

lms/djangoapps/certificates/tests/test_cert_management.py

@@ -103,7 +103,6 @@ class ResubmitErrorCertificatesTest(CertificateManagementTest):
         CertificateStatuses.downloadable,
         CertificateStatuses.generating,
         CertificateStatuses.notpassing,
-        CertificateStatuses.regenerating,
         CertificateStatuses.restricted,
         CertificateStatuses.unavailable,
     )

lms/djangoapps/certificates/views/xqueue.py

@@ -110,7 +110,7 @@ def update_certificate(request):
 
                 cert.error_reason = xqueue_body['error_reason']
         else:
-            if cert.status in [status.generating, status.regenerating]:
+            if cert.status == status.generating:
                 cert.download_uuid = xqueue_body['download_uuid']
                 cert.verify_uuid = xqueue_body['verify_uuid']
                 cert.download_url = xqueue_body['url']

lms/djangoapps/instructor_task/tasks_helper.py

@@ -1420,20 +1420,13 @@ def generate_students_certificates(
         )
 
     elif student_set == 'whitelisted_not_generated':
-        # All Whitelisted students
+        # Whitelist students who did not get certificates already.
         students_to_generate_certs_for = students_to_generate_certs_for.filter(
             certificatewhitelist__course_id=course_id,
             certificatewhitelist__whitelist=True
-        )
-
-        # Whitelisted students which got certificates already.
-        certificate_generated_students = GeneratedCertificate.objects.filter(  # pylint: disable=no-member
-            course_id=course_id,
-        )
-        certificate_generated_students_ids = set(certificate_generated_students.values_list('user_id', flat=True))
-
-        students_to_generate_certs_for = students_to_generate_certs_for.exclude(
-            id__in=certificate_generated_students_ids
+        ).exclude(
+            generatedcertificate__course_id=course_id,
+            generatedcertificate__status__in=CertificateStatuses.PASSED_STATUSES
         )
 
     elif student_set == "specific_student":

lms/djangoapps/instructor_task/tests/test_tasks_helper.py

@@ -1674,6 +1674,7 @@ class TestGradeReportEnrollmentAndCertificateInfo(TestReportMixin, InstructorTas
         self._verify_csv_data(user.username, expected_output)
 
 
+@ddt.ddt
 @override_settings(CERT_QUEUE='test-queue')
 class TestCertificateGeneration(InstructorTaskModuleTestCase):
     """
@@ -1716,65 +1717,70 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase):
         with self.assertNumQueries(214):
             self.assertCertificatesGenerated(task_input, expected_results)
 
-    def test_certificate_generation_all_whitelisted(self):
+    @ddt.data(
+        CertificateStatuses.downloadable,
+        CertificateStatuses.generating,
+        CertificateStatuses.notpassing,
+        CertificateStatuses.audit_passing,
+    )
+    def test_certificate_generation_all_whitelisted(self, status):
         """
-        Verify that certificates generated for all white-listed students when using semantic task_input as
-        `all_whitelisted`.
+        Verify that certificates are generated for all white-listed students,
+        whether or not they already had certs generated for them.
         """
-        # create 5 students
         students = self._create_students(5)
 
-        # white-list 5 students
-        for student in students:
-            CertificateWhitelistFactory.create(user=student, course_id=self.course.id, whitelist=True)
+        # whitelist 3
+        for student in students[:3]:
+            CertificateWhitelistFactory.create(
+                user=student, course_id=self.course.id, whitelist=True
+            )
 
-        task_input = {'student_set': 'all_whitelisted'}
-        expected_results = {
-            'action_name': 'certificates generated',
-            'total': 5,
-            'attempted': 5,
-            'succeeded': 5,
-            'failed': 0,
-            'skipped': 0
-        }
-        self.assertCertificatesGenerated(task_input, expected_results)
-
-    def test_certificate_generation_whitelist_already_generated(self):
-        """
-        Verify that certificates generated for all white-listed students having certifcates already when using
-        semantic task_input as `all_whitelisted`.
-        """
-        # create 5 students
-        students = self._create_students(5)
-
-        # white-list 5 students
-        for student in students:
-            CertificateWhitelistFactory.create(user=student, course_id=self.course.id, whitelist=True)
-
-        # mark 5 students to have certificates generated already
-        for student in students:
+        # generate certs for 2
+        for student in students[:2]:
             GeneratedCertificateFactory.create(
                 user=student,
                 course_id=self.course.id,
-                status=CertificateStatuses.downloadable,
-                mode='honor'
+                status=status,
             )
 
         task_input = {'student_set': 'all_whitelisted'}
+        # only certificates for the 3 whitelisted students should have been run
         expected_results = {
             'action_name': 'certificates generated',
-            'total': 5,
-            'attempted': 5,
-            'succeeded': 5,
+            'total': 3,
+            'attempted': 3,
+            'succeeded': 3,
             'failed': 0,
             'skipped': 0
         }
         self.assertCertificatesGenerated(task_input, expected_results)
 
-    def test_certificate_generation_whitelisted_not_generated(self):
+        # the first 3 students (who were whitelisted) have passing
+        # certificate statuses
+        for student in students[:3]:
+            self.assertIn(
+                GeneratedCertificate.certificate_for_student(student, self.course.id).status,
+                CertificateStatuses.PASSED_STATUSES
+            )
+
+        # The last 2 students still don't have certs
+        for student in students[3:]:
+            self.assertIsNone(
+                GeneratedCertificate.certificate_for_student(student, self.course.id)
+            )
+
+    @ddt.data(
+        (CertificateStatuses.downloadable, 2),
+        (CertificateStatuses.generating, 2),
+        (CertificateStatuses.notpassing, 4),
+        (CertificateStatuses.audit_passing, 4),
+    )
+    @ddt.unpack
+    def test_certificate_generation_whitelisted_not_generated(self, status, expected_certs):
         """
-        Verify that certificates only generated for those students which does not have certificates yet when
-        using semantic task_input as `whitelisted_not_generated`.
+        Verify that certificates are generated only for those students
+        who do not have `downloadable` or `generating` certificates.
         """
         # create 5 students
         students = self._create_students(5)
@@ -1784,21 +1790,24 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase):
             GeneratedCertificateFactory.create(
                 user=student,
                 course_id=self.course.id,
-                status=CertificateStatuses.downloadable,
-                mode='honor'
+                status=status,
             )
 
-        # white-list 5 students
-        for student in students:
-            CertificateWhitelistFactory.create(user=student, course_id=self.course.id, whitelist=True)
+        # white-list 4 students
+        for student in students[:4]:
+            CertificateWhitelistFactory.create(
+                user=student, course_id=self.course.id, whitelist=True
+            )
 
         task_input = {'student_set': 'whitelisted_not_generated'}
 
+        # certificates should only be generated for the whitelisted students
+        # who do not yet have passing certificates.
         expected_results = {
             'action_name': 'certificates generated',
-            'total': 3,
-            'attempted': 3,
-            'succeeded': 3,
+            'total': expected_certs,
+            'attempted': expected_certs,
+            'succeeded': expected_certs,
             'failed': 0,
             'skipped': 0
         }
@@ -1807,6 +1816,19 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase):
             expected_results
         )
 
+        # the first 4 students have passing certificate statuses since
+        # they either were whitelisted or had one before
+        for student in students[:4]:
+            self.assertIn(
+                GeneratedCertificate.certificate_for_student(student, self.course.id).status,
+                CertificateStatuses.PASSED_STATUSES
+            )
+
+        # The last student still doesn't have a cert
+        self.assertIsNone(
+            GeneratedCertificate.certificate_for_student(students[4], self.course.id)
+        )
+
     def test_certificate_generation_specific_student(self):
         """
         Tests generating a certificate for a specific student.

lms/templates/instructor/instructor_dashboard_2/certificate-white-list.underscore

@@ -2,7 +2,7 @@
 <p class="under-heading">
 <label>
     <input type='radio' name='generate-exception-certificates-radio' checked="checked" value='new' aria-describedby='generate-exception-certificates-radio-new-tip'>
-    <span id='generate-exception-certificates-radio-new-tip'><%- gettext('Generate certificates for all users on the Exception list for whom certificates have not yet been run') %></span>
+    <span id='generate-exception-certificates-radio-new-tip'><%- gettext('Generate certificates for all users on the Exception list who do not yet have a certificate') %></span>
 
 </label>
 <br/>