diff --git a/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py b/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py index 21a87ec03e..1b2e3e9a21 100644 --- a/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py +++ b/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py @@ -728,7 +728,7 @@ class CertificatesTest(BaseInstructorDashboardTest): Given that I am on the Certificates tab on the Instructor Dashboard When I fill in student username that already is in the list and click 'Add Exception' button - Then Error Message should say 'username/email already in exception list' + Then Error Message should say 'User (username/email={user}) already in exception list.' """ # Add a student to Certificate exception list self.certificates_section.add_certificate_exception(self.user_name, '') @@ -737,7 +737,7 @@ class CertificatesTest(BaseInstructorDashboardTest): self.certificates_section.add_certificate_exception(self.user_name, '') self.assertIn( - 'username/email already in exception list', + 'User (username/email={user}) already in exception list.'.format(user=self.user_name), self.certificates_section.message.text ) @@ -749,14 +749,17 @@ class CertificatesTest(BaseInstructorDashboardTest): Given that I am on the Certificates tab on the Instructor Dashboard When I click on 'Add Exception' button AND student username/email field is empty - Then Error Message should say 'Student username/email is required.' + Then Error Message should say + 'Student username/email field is required and can not be empty. ' + 'Kindly fill in username/email and then press "Add Exception" button.' """ # Click 'Add Exception' button without filling username/email field self.certificates_section.wait_for_certificate_exceptions_section() self.certificates_section.click_add_exception_button() self.assertIn( - 'Student username/email is required.', + 'Student username/email field is required and can not be empty. ' + 'Kindly fill in username/email and then press "Add Exception" button.', self.certificates_section.message.text ) @@ -768,7 +771,9 @@ class CertificatesTest(BaseInstructorDashboardTest): Given that I am on the Certificates tab on the Instructor Dashboard When I click on 'Add Exception' button AND student username/email does not exists - Then Error Message should say 'Student username/email is required.' + Then Error Message should say + 'Student username/email field is required and can not be empty. ' + 'Kindly fill in username/email and then press "Add Exception" button. """ invalid_user = 'test_user_non_existent' # Click 'Add Exception' button with invalid username/email field @@ -779,7 +784,42 @@ class CertificatesTest(BaseInstructorDashboardTest): self.certificates_section.wait_for_ajax() self.assertIn( - 'Student (username/email={}) does not exist'.format(invalid_user), + "We can't find the user (username/email={user}) you've entered. " + "Make sure the username or email address is correct, then try again.".format(user=invalid_user), + self.certificates_section.message.text + ) + + def test_user_not_enrolled_error(self): + """ + Scenario: On the Certificates tab of the Instructor Dashboard, + Error message appears if user is not enrolled in the course while trying to add a new exception. + + Given that I am on the Certificates tab on the Instructor Dashboard + When I click on 'Add Exception' button + AND student is not enrolled in the course + Then Error Message should say + "The user (username/email={user}) you have entered is not enrolled in this course. + Make sure the username or email address is correct, then try again." + """ + new_user = 'test_user_{uuid}'.format(uuid=self.unique_id[6:12]) + new_email = 'test_user_{uuid}@example.com'.format(uuid=self.unique_id[6:12]) + # Create a new user who is not enrolled in the course + AutoAuthPage(self.browser, username=new_user, email=new_email).visit() + # Login as instructor and visit Certificate Section of Instructor Dashboard + self.user_name, self.user_id = self.log_in_as_instructor() + self.instructor_dashboard_page.visit() + self.certificates_section = self.instructor_dashboard_page.select_certificates() + + # Click 'Add Exception' button with invalid username/email field + self.certificates_section.wait_for_certificate_exceptions_section() + + self.certificates_section.fill_user_name_field(new_user) + self.certificates_section.click_add_exception_button() + self.certificates_section.wait_for_ajax() + + self.assertIn( + "The user (username/email={user}) you have entered is not enrolled in this course. " + "Make sure the username or email address is correct, then try again.".format(user=new_user), self.certificates_section.message.text ) diff --git a/lms/djangoapps/instructor/tests/test_certificates.py b/lms/djangoapps/instructor/tests/test_certificates.py index c3622536bc..4e0cf831fc 100644 --- a/lms/djangoapps/instructor/tests/test_certificates.py +++ b/lms/djangoapps/instructor/tests/test_certificates.py @@ -472,7 +472,8 @@ class CertificateExceptionViewInstructorApiTest(SharedModuleStoreTestCase): # Assert Error Message self.assertEqual( res_json['message'], - u'Student (username/email={user}) does not exist'.format(user=invalid_user) + u"We can't find the user (username/email={user}) you've entered. " + u"Make sure the username or email address is correct, then try again.".format(user=invalid_user) ) def test_certificate_exception_missing_username_and_email_error(self): @@ -497,7 +498,8 @@ class CertificateExceptionViewInstructorApiTest(SharedModuleStoreTestCase): # Assert Error Message self.assertEqual( res_json['message'], - u'Student username/email is required.' + u'Student username/email field is required and can not be empty. ' + u'Kindly fill in username/email and then press "Add Exception" button.' ) def test_certificate_exception_duplicate_user_error(self): @@ -564,6 +566,35 @@ class CertificateExceptionViewInstructorApiTest(SharedModuleStoreTestCase): self.assertEqual(certificate_exception['user_name'], self.user.username) self.assertEqual(certificate_exception['user_id'], self.user.id) # pylint: disable=no-member + def test_certificate_exception_user_not_enrolled_error(self): + """ + Test certificates exception addition api endpoint returns failure when called with + username/email that is not enrolled in the given course. + """ + # Un-enroll student from the course + CourseEnrollment.unenroll(self.user, self.course.id) + response = self.client.post( + self.url, + data=json.dumps(self.certificate_exception), + content_type='application/json' + ) + + # Assert 400 status code in response + self.assertEqual(response.status_code, 400) + res_json = json.loads(response.content) + + # Assert Request not successful + self.assertFalse(res_json['success']) + + # Assert Error Message + self.assertEqual( + res_json['message'], + "The user (username/email={user}) you have entered is not enrolled in this course. " + "Make sure the username or email address is correct, then try again.".format( + user=self.certificate_exception['user_name'] + ) + ) + def test_certificate_exception_removed_successfully(self): """ Test certificates exception removal api endpoint returns success status @@ -613,7 +644,7 @@ class CertificateExceptionViewInstructorApiTest(SharedModuleStoreTestCase): # Assert Error Message self.assertEqual( res_json['message'], - u"Invalid Json data" + u"Invalid Json data, Please refresh the page and then try again." ) def test_remove_certificate_exception_non_existing_error(self): @@ -637,8 +668,8 @@ class CertificateExceptionViewInstructorApiTest(SharedModuleStoreTestCase): # Assert Error Message self.assertEqual( res_json['message'], - u"Certificate exception [user={}] does not exist in " - u"certificate white list.".format(self.certificate_exception['user_name']) + u"Certificate exception (user={user}) does not exist in certificate white list. " + u"Please refresh the page and try again.".format(user=self.certificate_exception['user_name']) ) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 2d42076863..7f2cd8519b 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -2743,7 +2743,7 @@ def certificate_exception_view(request, course_id): course_key = CourseKey.from_string(course_id) # Validate request data and return error response in case of invalid data try: - certificate_exception, student = parse_request_data_and_get_user(request) + certificate_exception, student = parse_request_data_and_get_user(request, course_key) except ValueError as error: return JsonResponse({'success': False, 'message': error.message}, status=400) @@ -2814,8 +2814,8 @@ def remove_certificate_exception(course_key, student): certificate_exception = CertificateWhitelist.objects.get(user=student, course_id=course_key) except ObjectDoesNotExist: raise ValueError( - _('Certificate exception [user={}] does not exist in ' - 'certificate white list.').format(student.username) + _('Certificate exception (user={user}) does not exist in certificate white list. ' + 'Please refresh the page and try again.').format(user=student.username) ) try: @@ -2827,26 +2827,34 @@ def remove_certificate_exception(course_key, student): certificate_exception.delete() -def parse_request_data_and_get_user(request): +def parse_request_data_and_get_user(request, course_key): """ Parse request data into Certificate Exception and User object. Certificate Exception is the dict object containing information about certificate exception. :param request: + :param course_key: Course Identifier of the course for whom to process certificate exception :return: key-value pairs containing certificate exception data and User object """ try: certificate_exception = json.loads(request.body or '{}') except ValueError: - raise ValueError(_('Invalid Json data')) + raise ValueError(_('Invalid Json data, Please refresh the page and then try again.')) user = certificate_exception.get('user_name', '') or certificate_exception.get('user_email', '') if not user: - raise ValueError(_('Student username/email is required.')) + raise ValueError(_('Student username/email field is required and can not be empty. ' + 'Kindly fill in username/email and then press "Add Exception" button.')) try: db_user = get_user_by_username_or_email(user) except ObjectDoesNotExist: - raise ValueError(_('Student (username/email={user}) does not exist').format(user=user)) + raise ValueError(_("We can't find the user (username/email={user}) you've entered. " + "Make sure the username or email address is correct, then try again.").format(user=user)) + + # Make Sure the given student is enrolled in the course + if not CourseEnrollment.is_enrolled(db_user, course_key): + raise ValueError(_("The user (username/email={user}) you have entered is not enrolled in this course. " + "Make sure the username or email address is correct, then try again.").format(user=user)) return certificate_exception, db_user @@ -2873,7 +2881,7 @@ def generate_certificate_exceptions(request, course_id, generate_for=None): except ValueError: return JsonResponse({ 'success': False, - 'message': _('Invalid Json data') + 'message': _('Invalid Json data, Please refresh the page and then try again.') }, status=400) users = [exception.get('user_id', False) for exception in certificate_white_list] diff --git a/lms/static/js/certificates/models/certificate_exception.js b/lms/static/js/certificates/models/certificate_exception.js index 617832193e..ad54f9776b 100644 --- a/lms/static/js/certificates/models/certificate_exception.js +++ b/lms/static/js/certificates/models/certificate_exception.js @@ -30,7 +30,8 @@ validate: function(attrs){ if (!_.str.trim(attrs.user_name) && !_.str.trim(attrs.user_email)) { - return gettext('Student username/email is required.'); + return gettext('Student username/email field is required and can not be empty. ' + + 'Kindly fill in username/email and then press "Add Exception" button.'); } } diff --git a/lms/static/js/certificates/views/certificate_whitelist.js b/lms/static/js/certificates/views/certificate_whitelist.js index ec6d6a7fb7..bc93a0e776 100644 --- a/lms/static/js/certificates/views/certificate_whitelist.js +++ b/lms/static/js/certificates/views/certificate_whitelist.js @@ -77,7 +77,9 @@ caller_object.showMessage(response.message, 'msg-error'); } catch(exception){ - caller_object.showMessage("Server Error, Please try again later.", 'msg-error'); + caller_object.showMessage( + "Server Error, Please refresh the page and try again.", 'msg-error' + ); } }; } diff --git a/lms/static/js/certificates/views/certificate_whitelist_editor.js b/lms/static/js/certificates/views/certificate_whitelist_editor.js index 074a5861a0..f979a54aa8 100644 --- a/lms/static/js/certificates/views/certificate_whitelist_editor.js +++ b/lms/static/js/certificates/views/certificate_whitelist_editor.js @@ -58,7 +58,10 @@ }); if(this.collection.findWhere(model)){ - this.showMessage("username/email already in exception list", 'msg-error'); + this.showMessage( + "User (username/email=" + (user_name || user_email) + ") already in exception list.", + 'msg-error' + ); } else if(certificate_exception.isValid()){ certificate_exception.save( @@ -67,7 +70,7 @@ success: this.showSuccess( this, true, - 'Students added to Certificate white list successfully' + 'Student added to Certificate white list successfully.' ), error: this.showError(this) } @@ -98,7 +101,11 @@ this.showMessage('Exception is being removed from server.', 'msg-success'); } else{ - this.showMessage('Could not find Certificate Exception in white list.', 'msg-error'); + this.showMessage( + 'Could not find Certificate Exception in white list. ' + + 'Please refresh the page and try again', + 'msg-error' + ); } }, @@ -131,7 +138,9 @@ caller.showMessage(response_data.message, 'msg-error'); } catch(exception){ - caller.showMessage("Server Error, Please try again later.", 'msg-error'); + caller.showMessage("" + + "Server Error, Please refresh the page and try again.", 'msg-error' + ); } }; } diff --git a/lms/static/js/spec/instructor_dashboard/certificates_exception_spec.js b/lms/static/js/spec/instructor_dashboard/certificates_exception_spec.js index eb60f56c6c..da76c17ec9 100644 --- a/lms/static/js/spec/instructor_dashboard/certificates_exception_spec.js +++ b/lms/static/js/spec/instructor_dashboard/certificates_exception_spec.js @@ -24,7 +24,8 @@ define([ }; var EXPECTED_ERRORS = { - user_name_or_email_required: "Student username/email is required." + user_name_or_email_required: 'Student username/email field is required and can not be empty. ' + + 'Kindly fill in username/email and then press "Add Exception" button.' }; beforeEach(function() { @@ -331,12 +332,14 @@ define([ var message_selector='.message', error_class = 'msg-error', success_class = 'msg-success', - success_message = 'Students added to Certificate white list successfully', - requests = AjaxHelpers.requests(this); + success_message = 'Student added to Certificate white list successfully.', + requests = AjaxHelpers.requests(this), + duplicate_user='test_user'; var error_messages = { - empty_user_name_email: 'Student username/email is required.', - duplicate_user: 'username/email already in exception list' + empty_user_name_email: 'Student username/email field is required and can not be empty. ' + + 'Kindly fill in username/email and then press "Add Exception" button.', + duplicate_user: "User (username/email=" + (duplicate_user) + ") already in exception list." }; // click 'Add Exception' button with empty username/email field @@ -348,7 +351,7 @@ define([ expect(view.$el.find(message_selector).html()).toMatch(error_messages.empty_user_name_email); // Add a new Exception to list - view.$el.find('#certificate-exception').val("test_user"); + view.$el.find('#certificate-exception').val(duplicate_user); view.$el.find('#notes').val("test user notes"); view.$el.find('#add-exception').click(); @@ -357,7 +360,7 @@ define([ { id: 3, user_id : 3, - user_name: "test_user", + user_name: duplicate_user, user_email : "test2@test.com", course_id: "edX/test/course", created: "Thursday, October 29, 2015", @@ -370,13 +373,13 @@ define([ expect(view.$el.find(message_selector).html()).toMatch(success_message); // Add a duplicate Certificate Exception - view.$el.find('#certificate-exception').val("test_user"); + view.$el.find('#certificate-exception').val(duplicate_user); view.$el.find('#notes').val("test user notes"); view.$el.find('#add-exception').click(); // Verify success message expect(view.$el.find(message_selector)).toHaveClass(error_class); - expect(view.$el.find(message_selector).html()).toMatch(error_messages.duplicate_user); + expect(view.$el.find(message_selector).html()).toEqual(error_messages.duplicate_user); }); it('verifies certificate exception can be deleted by clicking "delete" ', function(){