Merge pull request #10595 from edx/saleem-latif/SOL-1333-feedback-changes

saleem-latif/SOL-1333: Invalidate Generated Certificates before regeneration

lms/djangoapps/instructor_task/tasks_helper.py

@@ -40,7 +40,8 @@ from django.utils.translation import ugettext as _
 from certificates.models import (
     CertificateWhitelist,
     certificate_info_for_user,
-    CertificateStatuses
+    CertificateStatuses,
+    GeneratedCertificate
 )
 from certificates.api import generate_user_certificates
 from courseware.courses import get_course_by_id, get_problems_in_section
@@ -1419,6 +1420,12 @@ def generate_students_certificates(
     statuses_to_regenerate = task_input.get('statuses_to_regenerate', [])
     students_require_certs = students_require_certificate(course_id, enrolled_students, statuses_to_regenerate)
 
+    if statuses_to_regenerate:
+        # Mark existing generated certificates as 'unavailable' before regenerating
+        # We need to call this method after "students_require_certificate" otherwise "students_require_certificate"
+        # would return no results.
+        invalidate_generated_certificates(course_id, enrolled_students, statuses_to_regenerate)
+
     task_progress.skipped = task_progress.total - len(students_require_certs)
 
     current_step = {'step': 'Generating Certificates'}
@@ -1542,10 +1549,12 @@ def students_require_certificate(course_id, enrolled_students, statuses_to_regen
     if statuses_to_regenerate:
         # Return Students that have Generated Certificates and the generated certificate status
         # lies in 'statuses_to_regenerate'
-        return User.objects.filter(
+        students_require_certificates = enrolled_students.filter(
             generatedcertificate__course_id=course_id,
             generatedcertificate__status__in=statuses_to_regenerate
         )
+        # Fetch results otherwise subsequent operations on table cause wrong data fetch
+        return list(students_require_certificates)
     else:
         # compute those students whose certificates are already generated
         students_already_have_certs = User.objects.filter(
@@ -1554,3 +1563,32 @@ def students_require_certificate(course_id, enrolled_students, statuses_to_regen
 
         # Return all the enrolled student skipping the ones whose certificates have already been generated
         return list(set(enrolled_students) - set(students_already_have_certs))
+
+
+def invalidate_generated_certificates(course_id, enrolled_students, certificate_statuses):  # pylint: disable=invalid-name
+    """
+    Invalidate generated certificates for all enrolled students in the given course having status in
+    'certificate_statuses'.
+
+    Generated Certificates are invalidated by marking its status 'unavailable' and updating verify_uuid, download_uuid,
+    download_url and grade with empty string.
+
+    :param course_id: Course Key for the course whose generated certificates need to be removed
+    :param enrolled_students: (queryset or list) students enrolled in the course
+    :param certificate_statuses: certificates statuses for whom to remove generated certificate
+    """
+    certificates = GeneratedCertificate.objects.filter(
+        user__in=enrolled_students,
+        course_id=course_id,
+        status__in=certificate_statuses,
+    )
+
+    # Mark generated certificates as 'unavailable' and update download_url, download_uui, verify_uuid and
+    # grade with empty string for each row
+    certificates.update(
+        status=CertificateStatuses.unavailable,
+        verify_uuid='',
+        download_uuid='',
+        download_url='',
+        grade='',
+    )

lms/djangoapps/instructor_task/tests/test_tasks_helper.py

@@ -16,7 +16,7 @@ from django.core.urlresolvers import reverse
 from django.test.utils import override_settings
 
 from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory
-from certificates.models import CertificateStatuses
+from certificates.models import CertificateStatuses, GeneratedCertificate
 from certificates.tests.factories import GeneratedCertificateFactory, CertificateWhitelistFactory
 from course_modes.models import CourseMode
 from courseware.tests.factories import InstructorFactory
@@ -1708,6 +1708,9 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase):
         Verify that certificates are regenerated for all eligible students enrolled in a course whose generated
         certificate statuses lies in the list 'statuses_to_regenerate' given in task_input.
         """
+        # Default grade for students
+        default_grade = '-1'
+
         # create 10 students
         students = [self.create_student(username='student_{}'.format(i), email='student_{}@example.com'.format(i))
                     for i in xrange(1, 11)]
@@ -1718,7 +1721,8 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase):
                 user=student,
                 course_id=self.course.id,
                 status=CertificateStatuses.downloadable,
-                mode='honor'
+                mode='honor',
+                grade=default_grade
             )
 
         # mark 3 students to have certificates generated with status 'error'
@@ -1727,7 +1731,8 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase):
                 user=student,
                 course_id=self.course.id,
                 status=CertificateStatuses.error,
-                mode='honor'
+                mode='honor',
+                grade=default_grade
             )
 
         # mark 6th students to have certificates generated with status 'deleted'
@@ -1736,7 +1741,8 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase):
                 user=student,
                 course_id=self.course.id,
                 status=CertificateStatuses.deleted,
-                mode='honor'
+                mode='honor',
+                grade=default_grade
             )
 
         # mark rest of the 4 students with having generated certificates with status 'generating'
@@ -1748,7 +1754,8 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase):
                 user=student,
                 course_id=self.course.id,
                 status=CertificateStatuses.generating,
-                mode='honor'
+                mode='honor',
+                grade=default_grade
             )
 
         # white-list 7 students
@@ -1781,3 +1788,138 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase):
             },
             result
         )
+        generated_certificates = GeneratedCertificate.objects.filter(
+            user__in=students,
+            course_id=self.course.id,
+            mode='honor'
+        )
+        certificate_statuses = [generated_certificate.status for generated_certificate in generated_certificates]
+        certificate_grades = [generated_certificate.grade for generated_certificate in generated_certificates]
+
+        # Verify from results from database
+        # Certificates are being generated for 2 white-listed students that had statuses in 'deleted'' and 'generating'
+        self.assertEqual(certificate_statuses.count(CertificateStatuses.generating), 2)
+        # 5 students are skipped that had Certificate Status 'downloadable' and 'error'
+        self.assertEqual(certificate_statuses.count(CertificateStatuses.downloadable), 2)
+        self.assertEqual(certificate_statuses.count(CertificateStatuses.error), 3)
+
+        # grades will be '0.0' as students are either white-listed or ending in error
+        self.assertEqual(certificate_grades.count('0.0'), 5)
+        # grades will be '-1' for students that were skipped
+        self.assertEqual(certificate_grades.count(default_grade), 5)
+
+    def test_certificate_regeneration_with_existing_unavailable_status(self):
+        """
+        Verify that certificates are regenerated for all eligible students enrolled in a course whose generated
+        certificate status lies in the list 'statuses_to_regenerate' given in task_input. but the 'unavailable'
+        status is not touched if it is not in the 'statuses_to_regenerate' list.
+        """
+        # Default grade for students
+        default_grade = '-1'
+
+        # create 10 students
+        students = [self.create_student(username='student_{}'.format(i), email='student_{}@example.com'.format(i))
+                    for i in xrange(1, 11)]
+
+        # mark 2 students to have certificates generated already
+        for student in students[:2]:
+            GeneratedCertificateFactory.create(
+                user=student,
+                course_id=self.course.id,
+                status=CertificateStatuses.downloadable,
+                mode='honor',
+                grade=default_grade
+            )
+
+        # mark 3 students to have certificates generated with status 'error'
+        for student in students[2:5]:
+            GeneratedCertificateFactory.create(
+                user=student,
+                course_id=self.course.id,
+                status=CertificateStatuses.error,
+                mode='honor',
+                grade=default_grade
+            )
+
+        # mark 2 students to have generated certificates with status 'unavailable'
+        for student in students[5:7]:
+            GeneratedCertificateFactory.create(
+                user=student,
+                course_id=self.course.id,
+                status=CertificateStatuses.unavailable,
+                mode='honor',
+                grade=default_grade
+            )
+
+        # mark 3 students to have generated certificates with status 'generating'
+        for student in students[7:]:
+            GeneratedCertificateFactory.create(
+                user=student,
+                course_id=self.course.id,
+                status=CertificateStatuses.generating,
+                mode='honor',
+                grade=default_grade
+            )
+
+        # white-list all students
+        for student in students[:]:
+            CertificateWhitelistFactory.create(user=student, course_id=self.course.id, whitelist=True)
+
+        current_task = Mock()
+        current_task.update_state = Mock()
+
+        # Regenerated certificates for students having generated certificates with status
+        # 'downloadable', 'error' or 'generating'
+        task_input = {
+            'statuses_to_regenerate': [
+                CertificateStatuses.downloadable,
+                CertificateStatuses.error,
+                CertificateStatuses.generating
+            ]
+        }
+
+        with patch('instructor_task.tasks_helper._get_current_task') as mock_current_task:
+            mock_current_task.return_value = current_task
+            with patch('capa.xqueue_interface.XQueueInterface.send_to_queue') as mock_queue:
+                mock_queue.return_value = (0, "Successfully queued")
+                result = generate_students_certificates(
+                    None, None, self.course.id, task_input, 'certificates generated'
+                )
+
+        self.assertDictContainsSubset(
+            {
+                'action_name': 'certificates generated',
+                'total': 10,
+                'attempted': 8,
+                'succeeded': 8,
+                'failed': 0,
+                'skipped': 2
+            },
+            result
+        )
+
+        generated_certificates = GeneratedCertificate.objects.filter(
+            user__in=students,
+            course_id=self.course.id,
+            mode='honor'
+        )
+        certificate_statuses = [generated_certificate.status for generated_certificate in generated_certificates]
+        certificate_grades = [generated_certificate.grade for generated_certificate in generated_certificates]
+
+        # Verify from results from database
+        # Certificates are being generated for 8 students that had statuses in 'downloadable', 'error' and 'generating'
+        self.assertEqual(certificate_statuses.count(CertificateStatuses.generating), 8)
+        # 2 students are skipped that had Certificate Status 'unavailable'
+        self.assertEqual(certificate_statuses.count(CertificateStatuses.unavailable), 2)
+
+        # grades will be '0.0' as students are white-listed and have not completed any tasks
+        self.assertEqual(certificate_grades.count('0.0'), 8)
+        # grades will be '-1' for students that have not been processed
+        self.assertEqual(certificate_grades.count(default_grade), 2)
+
+        # Verify that students with status 'unavailable were skipped
+        unavailable_certificates = \
+            [cert for cert in generated_certificates
+             if cert.status == CertificateStatuses.unavailable and cert.grade == default_grade]
+
+        self.assertEquals(len(unavailable_certificates), 2)