diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 5db9d5ccfc..dc4790e281 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -56,7 +56,11 @@ from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, U from lms.djangoapps.bulk_email.models import BulkEmailFlag, CourseEmail, CourseEmailTemplate from lms.djangoapps.certificates.api import generate_user_certificates from lms.djangoapps.certificates.models import CertificateStatuses -from lms.djangoapps.certificates.tests.factories import GeneratedCertificateFactory +from lms.djangoapps.certificates.tests.factories import ( + CertificateInvalidationFactory, + CertificateWhitelistFactory, + GeneratedCertificateFactory +) from lms.djangoapps.courseware.models import StudentModule from lms.djangoapps.courseware.tests.factories import ( BetaTesterFactory, @@ -68,6 +72,10 @@ from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase from lms.djangoapps.experiments.testutils import override_experiment_waffle_flag from lms.djangoapps.instructor.tests.utils import FakeContentTask, FakeEmail, FakeEmailInfo from lms.djangoapps.instructor.views.api import ( + _check_if_learner_on_allowlist, + _check_if_learner_on_blocklist, + _get_certificate_for_user, + _get_student_from_request_data, _split_input_list, common_exceptions_400, generate_unique_password, @@ -4362,3 +4370,176 @@ class TestBulkCohorting(SharedModuleStoreTestCase): self.verify_success_on_file_content( 'username,email,cohort\r\nfoo_username,bar_email,baz_cohort', mock_store_upload, mock_cohort_task ) + + +class TestInstructorCertificateExceptions(SharedModuleStoreTestCase): + """ + Tests for utility functions utilized in the Instructor Dashboard Certificates app. + """ + + def setUp(self): + super().setUp() + self.global_staff = GlobalStaffFactory() + self.course = CourseFactory.create() + self.user = UserFactory() + CourseEnrollment.enroll(self.user, self.course.id) + + def test_get_student_from_request_data(self): + """ + Test ability to retrieve a learner record using their username and course id + """ + student = _get_student_from_request_data({"user": self.user.username}, self.course.id) + + assert student.username == self.user.username + + def test_get_student_from_request_data_empty_username(self): + """ + Test that we receive an expected error when no learner's username or email is entered + """ + with pytest.raises(ValueError) as error: + _get_student_from_request_data({"user": ""}, self.course.id) + + assert str(error.value) == ( + 'Student username/email field is required and can not be empty. Kindly fill in username/email and then ' + 'press "Invalidate Certificate" button.' + ) + + def test_get_student_from_request_data_user_dne(self): + """ + Test to verify an expected error message is returned when attempting to retrieve a learner that does not exist + in the LMS. + """ + with pytest.raises(ValueError) as error: + _get_student_from_request_data({"user": "Neo"}, self.course.id) + + assert str(error.value) == "Neo does not exist in the LMS. Please check your spelling and retry." + + def test_get_student_from_request_data_user_not_enrolled(self): + """ + Test to verify an expected error message is returned when attempting to retrieve a learner that is not enrolled + in a course-run. + """ + new_course = CourseFactory.create() + + with pytest.raises(ValueError) as error: + _get_student_from_request_data({"user": self.user.username}, new_course.id) + + assert str(error.value) == ( + f"{self.user.username} is not enrolled in this course. Please check your spelling and retry." + ) + + def test_get_certificate_for_user(self): + """ + Test that attempts to retrieve a Certificate for a learner in a course-run. + """ + generated_certificate = GeneratedCertificateFactory.create( + user=self.user, + course_id=self.course.id, + mode='verified', + status=CertificateStatuses.downloadable, + ) + + retrieved_certificate = _get_certificate_for_user(self.course.id, self.user) + + assert retrieved_certificate.id == generated_certificate.id + assert retrieved_certificate.user == self.user + assert retrieved_certificate.course_id == self.course.id + + def test_get_certificate_for_user_no_certificate(self): + """ + Test to verify an expected error message is returned when attempting to retrieve a certificate for a learner + that does not exist yet. + """ + with pytest.raises(ValueError) as error: + _get_certificate_for_user(self.course.id, self.user) + + assert str(error.value) == ( + f"The student {self.user} does not have certificate for the course {self.course.id.course}. Kindly " + "verify student username/email and the selected course are correct and try again." + ) + + def test_check_if_learner_on_allowlist(self): + """ + Test to verify that no learner is returned if the learner does not have an active entry on the allowlist. + """ + result = _check_if_learner_on_allowlist(self.course.id, self.user) + + assert not result + + def test_check_if_learner_on_allowlist_allowlist_entry_exists(self): + """ + Test that verifies the correct result is returned if a learner has an active entry on the allowlist. + """ + CertificateWhitelistFactory.create( + course_id=self.course.id, + user=self.user + ) + + result = _check_if_learner_on_allowlist(self.course.id, self.user) + + assert result + + def test_check_if_learner_on_blocklist_no_cert(self): + """ + Test to verify that the correct result is returned if a learner does not currently have a certificate in a + course-run. + """ + result = _check_if_learner_on_blocklist(self.course.id, self.user) + + assert not result + + def test_check_if_learner_on_blocklist_with_cert(self): + """ + Test to verify that the correct result is returned if a learner has a certificate but does not have an active + entry on the blocklist. + """ + GeneratedCertificateFactory.create( + user=self.user, + course_id=self.course.id, + mode='verified', + status=CertificateStatuses.downloadable, + ) + + result = _check_if_learner_on_blocklist(self.course.id, self.user) + assert not result + + def test_check_if_learner_on_blocklist_blocklist_entry_exists(self): + """ + Test to verify that the correct result is returned if a learner has a Certificate and an active entry on + the blocklist. + """ + generated_certificate = GeneratedCertificateFactory.create( + user=self.user, + course_id=self.course.id, + mode='verified', + status=CertificateStatuses.downloadable, + ) + + CertificateInvalidationFactory.create( + generated_certificate=generated_certificate, + invalidated_by=self.global_staff, + active=True + ) + + result = _check_if_learner_on_blocklist(self.course.id, self.user) + assert result + + def test_check_if_learner_on_blocklist_inactive_blocklist_entry_exists(self): + """ + Test to verify that the correct result is returned if a learner has an inactive entry on the blocklist. + """ + generated_certificate = GeneratedCertificateFactory.create( + user=self.user, + course_id=self.course.id, + mode='verified', + status=CertificateStatuses.downloadable, + ) + + CertificateInvalidationFactory.create( + generated_certificate=generated_certificate, + invalidated_by=self.global_staff, + active=False + ) + + result = _check_if_learner_on_blocklist(self.course.id, self.user) + assert not result diff --git a/lms/djangoapps/instructor/tests/test_certificates.py b/lms/djangoapps/instructor/tests/test_certificates.py index c60c81f7c6..78ee8d1360 100644 --- a/lms/djangoapps/instructor/tests/test_certificates.py +++ b/lms/djangoapps/instructor/tests/test_certificates.py @@ -743,6 +743,40 @@ class CertificateExceptionViewInstructorApiTest(SharedModuleStoreTestCase): 'Certificate exception (user={user}) does not exist in certificate white list.' \ ' Please refresh the page and try again.'.format(user=self.certificate_exception['user_name']) + def test_certificate_invalidation_already_exists(self): + """ + Test to confirm an error message is raised when generating a certificate exception for a learner that already + has an active certificate invalidation. + """ + # generate a certificate for the test learner in our course + generated_certificate = GeneratedCertificateFactory.create( + user=self.user, + course_id=self.course.id, + status=CertificateStatuses.downloadable, + mode='honor', + ) + + # create a certificate invalidation tied to the generated certificate + CertificateInvalidationFactory.create( + generated_certificate=generated_certificate, + invalidated_by=self.global_staff, + ) + + # attempt to add learner to the allowlist, expect an error + response = self.client.post( + self.url, + data=json.dumps(self.certificate_exception), + content_type='application/json', + REQUEST_METHOD='POST' + ) + + res_json = json.loads(response.content.decode('utf-8')) + assert response.status_code == 400 + assert res_json['message'] == ( + f"Student {self.user.username} is already on the certificate invalidation list and cannot be added to " + "the certificate exception list." + ) + @override_settings(CERT_QUEUE='certificates') @ddt.ddt @@ -997,6 +1031,30 @@ class TestCertificatesInstructorApiBulkWhiteListExceptions(SharedModuleStoreTest assert len(data['general_errors']) == 1 assert len(data['success']) == 0 + def test_certificate_invalidation_already_exists(self): + """ + Test to confirm an error message is raised when generating a certificate exception for a learner appears in the + CSV file who has an active certificate invalidation. + """ + # generate a certificate for the test learner in our course + generated_certificate = GeneratedCertificateFactory.create( + user=self.enrolled_user_1, + course_id=self.course.id, + status=CertificateStatuses.downloadable, + mode='honor', + ) + + CertificateInvalidationFactory.create( + generated_certificate=generated_certificate, + invalidated_by=self.global_staff, + ) + + # attempt to add learner to the allowlist, expect an error + csv_content = b"test_student1@example.com,notes" + data = self.upload_file(csv_content=csv_content) + assert len(data['row_errors']['user_on_certificate_invalidation_list']) == 1 + assert data['row_errors']['user_on_certificate_invalidation_list'][0] == 'user "TestStudent1" in row# 1' + def upload_file(self, csv_content): """ Upload a csv file. @@ -1272,3 +1330,26 @@ class CertificateInvalidationViewTests(SharedModuleStoreTestCase): # Assert Error Message assert res_json['message'] == 'Certificate Invalidation does not exist, Please refresh the page and try again.' + + def test_learner_already_on_certificate_exception_list(self): + """ + Test to make sure we don't allow a single to learner to appear on both the certificate exception and + invalidation lists. + """ + # add test learner to the allowlist + CertificateWhitelistFactory.create(user=self.enrolled_user_1, course_id=self.course.id) + + # now try and add them to the invalidation list, expect an error + response = self.client.post( + self.url, + data=json.dumps(self.certificate_invalidation_data), + content_type='application/json', + ) + + res_json = json.loads(response.content.decode('utf-8')) + assert response.status_code == 400 + assert res_json['message'] == ( + f"The student {self.enrolled_user_1.username} appears on the Certificate Exception list in course " + f"{self.course.id}. Please remove them from the Certificate Exception list before attempting to " + "invalidate their certificate." + ) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 90d005fa7e..0008b01312 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -2647,13 +2647,20 @@ def certificate_exception_view(request, course_id): def add_certificate_exception(course_key, student, certificate_exception): """ Add a certificate exception to CertificateWhitelist table. - Raises ValueError in case Student is already white listed. + + Raises ValueError in case Student is already white listed or if they appear on the block list. :param course_key: identifier of the course whose certificate exception will be added. :param student: User object whose certificate exception will be added. :param certificate_exception: A dict object containing certificate exception info. :return: CertificateWhitelist item in dict format containing certificate exception info. """ + if _check_if_learner_on_blocklist(course_key, student): + raise ValueError( + _("Student {user} is already on the certificate invalidation list and cannot be added to the certificate " + "exception list.").format(user=student.username) + ) + if CertificateWhitelist.get_certificate_white_list(course_key, student): raise ValueError( _("Student (username/email={user}) already in certificate exception list.").format(user=student.username) @@ -2667,7 +2674,8 @@ def add_certificate_exception(course_key, student, certificate_exception): 'notes': certificate_exception.get('notes', '') } ) - log.info('%s has been added to the whitelist in course %s', student.username, course_key) + + log.info(f"Student {student.id} has been added to the whitelist in course {course_key}") generated_certificate = GeneratedCertificate.eligible_certificates.filter( user=student, @@ -2835,17 +2843,24 @@ def generate_bulk_certificate_exceptions(request, course_id): { general_errors: [errors related to csv file e.g. csv uploading, csv attachment, content reading etc. ], row_errors: { - data_format_error: [users/data in csv file that are not well formatted], - user_not_exist: [csv with none exiting users in LMS system], - user_already_white_listed: [users that are already white listed], - user_not_enrolled: [rows with not enrolled users in the given course] + data_format_error: [users/data in csv file that are not well formatted], + user_not_exist: [csv with none exiting users in LMS system], + user_already_white_listed: [users that are already white listed], + user_not_enrolled: [rows with not enrolled users in the given course], + user_on_certificate_invalidation_list: [users that are currently have an active blocklist entry] }, success: [list of successfully added users to the certificate white list model] } """ user_index = 0 notes_index = 1 - row_errors_key = ['data_format_error', 'user_not_exist', 'user_already_white_listed', 'user_not_enrolled'] + row_errors_key = [ + 'data_format_error', + 'user_not_exist', + 'user_already_white_listed', + 'user_not_enrolled', + 'user_on_certificate_invalidation_list' + ] course_key = CourseKey.from_string(course_id) students, general_errors, success = [], [], [] row_errors = {key: [] for key in row_errors_key} @@ -2864,7 +2879,6 @@ def generate_bulk_certificate_exceptions(request, course_id): else: general_errors.append(_('Make sure that the file you upload is in CSV format with no ' 'extraneous characters or rows.')) - except Exception: # pylint: disable=broad-except general_errors.append(_('Could not read uploaded file.')) finally: @@ -2878,7 +2892,7 @@ def generate_bulk_certificate_exceptions(request, course_id): if len(student) != 2: if student: build_row_errors('data_format_error', student[user_index], row_num) - log.info('invalid data/format in csv row# %s', row_num) + log.info(f'Invalid data/format in csv row# {row_num}') continue user = student[user_index] @@ -2886,17 +2900,21 @@ def generate_bulk_certificate_exceptions(request, course_id): user = get_user_by_username_or_email(user) except ObjectDoesNotExist: build_row_errors('user_not_exist', user, row_num) - log.info('student %s does not exist', user) + log.info(f'Student {user} does not exist') else: - if CertificateWhitelist.get_certificate_white_list(course_key, user): + # make sure learner isn't on the blocklist + if _check_if_learner_on_blocklist(course_key, user): + build_row_errors('user_on_certificate_invalidation_list', user, row_num) + log.warning(f'Student {user.id} is blocked from receiving a Certificate in Course ' + f'{course_key}') + # make sure user isn't already on the exception list + elif CertificateWhitelist.get_certificate_white_list(course_key, user): build_row_errors('user_already_white_listed', user, row_num) - log.warning('student %s already exist.', user.username) - + log.warning(f'Student {user.id} already on exception list in Course {course_key}.') # make sure user is enrolled in course elif not CourseEnrollment.is_enrolled(user, course_key): build_row_errors('user_not_enrolled', user, row_num) - log.warning('student %s is not enrolled in course.', user.username) - + log.warning(f'Student {user.id} is not enrolled in Course {course_key}') else: CertificateWhitelist.objects.create( user=user, @@ -2905,7 +2923,6 @@ def generate_bulk_certificate_exceptions(request, course_id): notes=student[notes_index] ) success.append(_('user "{username}" in row# {row}').format(username=user.username, row=row_num)) - else: general_errors.append(_('File is not attached.')) @@ -2914,7 +2931,6 @@ def generate_bulk_certificate_exceptions(request, course_id): 'row_errors': row_errors, 'success': success } - return JsonResponse(results) @@ -2935,13 +2951,23 @@ def certificate_invalidation_view(request, course_id): # Validate request data and return error response in case of invalid data try: certificate_invalidation_data = parse_request_data(request) - certificate = validate_request_data_and_get_certificate(certificate_invalidation_data, course_key) + student = _get_student_from_request_data(certificate_invalidation_data, course_key) + certificate = _get_certificate_for_user(course_key, student) except ValueError as error: return JsonResponse({'message': str(error)}, status=400) # Invalidate certificate of the given student for the course course if request.method == 'POST': try: + if _check_if_learner_on_allowlist(course_key, student): + log.warning(f"Invalidating certificate for student {student.id} in course {course_key} failed. " + "Student is currently on the allow list.") + raise ValueError( + _("The student {student} appears on the Certificate Exception list in course {course}. Please " + "remove them from the Certificate Exception list before attempting to invalidate their " + "certificate.").format(student=student, course=course_key) + ) + certificate_invalidation = invalidate_certificate(request, certificate, certificate_invalidation_data) except ValueError as error: return JsonResponse({'message': str(error)}, status=400) @@ -3031,36 +3057,6 @@ def re_validate_certificate(request, course_key, generated_certificate): ) -def validate_request_data_and_get_certificate(certificate_invalidation, course_key): - """ - Fetch and return GeneratedCertificate of the student passed in request data for the given course. - - Raises ValueError in case of missing student username/email or - if student does not have certificate for the given course. - - :param certificate_invalidation: dict containing certificate invalidation data - :param course_key: CourseKey object identifying the current course. - :return: GeneratedCertificate object of the student for the given course - """ - user = certificate_invalidation.get("user") - - if not user: - raise ValueError( - _('Student username/email field is required and can not be empty. ' - 'Kindly fill in username/email and then press "Invalidate Certificate" button.') - ) - - student = get_student(user, course_key) - - certificate = GeneratedCertificate.certificate_for_student(student, course_key) - if not certificate: - raise ValueError(_( - "The student {student} does not have certificate for the course {course}. Kindly verify student " - "username/email and the selected course are correct and try again." - ).format(student=student.username, course=course_key.course)) - return certificate - - def _get_boolean_param(request, param_name): """ Returns the value of the boolean parameter with the given @@ -3076,3 +3072,62 @@ def _create_error_response(request, msg): in JSON form. """ return JsonResponse({"error": msg}, 400) + + +def _get_student_from_request_data(request_data, course_key): + """ + Attempts to retrieve the student information from the incoming request data. + + :param request: HttpRequest object + :param course_key: CourseKey object identifying the current course. + """ + user = request_data.get("user") + if not user: + raise ValueError( + _('Student username/email field is required and can not be empty. ' + 'Kindly fill in username/email and then press "Invalidate Certificate" button.') + ) + + return get_student(user, course_key) + + +def _get_certificate_for_user(course_key, student): + """ + Attempts to retrieve a Certificate for a learner in a given course run key. + """ + log.info(f"Retrieving certificate for student {student.id} in course {course_key}") + certificate = GeneratedCertificate.certificate_for_student(student, course_key) + if not certificate: + raise ValueError(_( + "The student {student} does not have certificate for the course {course}. Kindly verify student " + "username/email and the selected course are correct and try again.").format( + student=student.username, course=course_key.course) + ) + + return certificate + + +def _check_if_learner_on_allowlist(course_key, student): + """ + Utility method that will try to determine if the learner is currently on the allowlist. This is a check that + occurs as part of adding a learner to the CertificateInvalidation list. + """ + log.info(f"Checking if student {student.id} is currently on the allowlist of course {course_key}") + return CertificateWhitelist.objects.filter(user=student, course_id=course_key, whitelist=True).exists() + + +def _check_if_learner_on_blocklist(course_key, student): + """ + Utility method that will try to determine if the learner is currently on the block list. This is a check that + occurs as part of adding a learner to the Allow list. + + The CertificateInvalidation model does not store a username or user id, just a reference to the id of the + invalidated Certificate. We check if the learner has a Certificate in the Course-Run and then use that to check if + the learner has an active entry on the block list. + """ + log.info(f"Checking if student {student.id} is currently on the blocklist of course {course_key}") + cert = GeneratedCertificate.certificate_for_student(student, course_key) + if cert: + return CertificateInvalidation.objects.filter(generated_certificate_id=cert.id, active=True).exists() + + return False diff --git a/lms/static/js/certificates/views/certificate_bulk_whitelist.js b/lms/static/js/certificates/views/certificate_bulk_whitelist.js index 6e46cea5e9..be94068979 100644 --- a/lms/static/js/certificates/views/certificate_bulk_whitelist.js +++ b/lms/static/js/certificates/views/certificate_bulk_whitelist.js @@ -25,7 +25,8 @@ data_format_error: 'data-format-error', user_not_exist: 'user-not-exist', user_already_white_listed: 'user-already-white-listed', - user_not_enrolled: 'user-not-enrolled' + user_not_enrolled: 'user-not-enrolled', + user_on_certificate_invalidation_list: 'user-on-certificate-invalidation-list' }; return Backbone.View.extend({ @@ -43,7 +44,7 @@ render: function() { var template = this.loadTemplate('certificate-bulk-white-list'); - this.$el.html(template()); + this.$el.html(template()); // xss-lint: disable=javascript-jquery-html }, loadTemplate: function(name) { @@ -73,12 +74,91 @@ }, display_response: function(data_from_server) { + var UserOnCertificateInvalidationList; + $('.bulk-exception-results').removeClass('hidden').empty(); + function generateDiv(group, heading, displayData) { + // inner function generate div and display response messages. + $('
', { + class: 'message ' + group + }).appendTo('.bulk-exception-results').prepend( // eslint-disable-line max-len, xss-lint: disable=javascript-jquery-insert-into-target,javascript-jquery-prepend + "" + heading) // eslint-disable-line max-len, xss-lint: disable=javascript-concat-html + .append($('