From 9a4f80e5c0119038a4b9fa5e41a0d4b2bde0c2c1 Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Wed, 13 Apr 2016 16:52:06 -0400 Subject: [PATCH] update query for whitelisted users who haven't yet had certs run for them remove CertificateStatuses.regenerating --- common/djangoapps/student/tests/tests.py | 16 --- common/djangoapps/student/views.py | 1 - lms/djangoapps/certificates/models.py | 3 +- .../tests/test_cert_management.py | 1 - lms/djangoapps/certificates/views/xqueue.py | 2 +- .../instructor_task/tasks_helper.py | 15 +-- .../tests/test_tasks_helper.py | 118 +++++++++++------- .../certificate-white-list.underscore | 2 +- 8 files changed, 77 insertions(+), 81 deletions(-) diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index f3c5232db9..ace9fe14bf 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/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', diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index fb2424b127..611f8ecdda 100644 --- a/common/djangoapps/student/views.py +++ b/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', diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index b2a00e6543..17031810c9 100644 --- a/lms/djangoapps/certificates/models.py +++ b/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): diff --git a/lms/djangoapps/certificates/tests/test_cert_management.py b/lms/djangoapps/certificates/tests/test_cert_management.py index 6f876cc340..2a7783cc4b 100644 --- a/lms/djangoapps/certificates/tests/test_cert_management.py +++ b/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, ) diff --git a/lms/djangoapps/certificates/views/xqueue.py b/lms/djangoapps/certificates/views/xqueue.py index e09fd7997e..7bdeef6975 100644 --- a/lms/djangoapps/certificates/views/xqueue.py +++ b/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'] diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index 8287432d70..c276d6de90 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/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": diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 4fa14d3215..bd63737ac2 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/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. diff --git a/lms/templates/instructor/instructor_dashboard_2/certificate-white-list.underscore b/lms/templates/instructor/instructor_dashboard_2/certificate-white-list.underscore index 0e97cc3af9..6c608b4908 100644 --- a/lms/templates/instructor/instructor_dashboard_2/certificate-white-list.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/certificate-white-list.underscore @@ -2,7 +2,7 @@