From 066d04f59580179c9a0ad2366c4d6fbbd37e718f Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Wed, 19 Mar 2014 09:59:14 -0400 Subject: [PATCH] Simple email checking for batch enrollment on inst dash LMS-2244 --- lms/djangoapps/instructor/tests/test_api.py | 22 ++++++++++ lms/djangoapps/instructor/views/api.py | 18 ++++++++ .../instructor_dashboard/membership.coffee | 41 +++++++++++-------- .../instructor_dashboard_2/membership.html | 3 +- 4 files changed, 67 insertions(+), 17 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 754034e487..98d24ee909 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -293,6 +293,28 @@ class TestInstructorAPIEnrollment(ModuleStoreTestCase, LoginEnrollmentTestCase): response = self.client.get(url, {'emails': self.enrolled_student.email, 'action': action}) self.assertEqual(response.status_code, 400) + def test_enroll_with_username(self): + # Test with an invalid email address (eg, a username). + url = reverse('students_update_enrollment', kwargs={'course_id': self.course.id}) + response = self.client.get(url, {'emails': self.notenrolled_student.username, 'action': 'enroll', 'email_students': False}) + self.assertEqual(response.status_code, 200) + + # test the response data + expected = { + "action": "enroll", + 'auto_enroll': False, + "results": [ + { + "email": self.notenrolled_student.username, + "error": True, + "invalidEmail": True + } + ] + } + + res_json = json.loads(response.content) + self.assertEqual(res_json, expected) + def test_enroll_without_email(self): url = reverse('students_update_enrollment', kwargs={'course_id': self.course.id}) response = self.client.get(url, {'emails': self.notenrolled_student.email, 'action': 'enroll', 'email_students': False}) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index a8bb467a13..ecf5f72bfb 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -13,7 +13,9 @@ import requests from django.conf import settings from django_future.csrf import ensure_csrf_cookie from django.views.decorators.cache import cache_control +from django.core.exceptions import ValidationError from django.core.urlresolvers import reverse +from django.core.validators import validate_email from django.utils.translation import ugettext as _ from django.http import HttpResponse, HttpResponseBadRequest, HttpResponseForbidden from django.utils.html import strip_tags @@ -250,6 +252,21 @@ def students_update_enrollment(request, course_id): results = [] for email in emails: + try: + # Use django.core.validators.validate_email to check email address + # validity (obviously, cannot check if email actually /exists/, + # simply that it is plausibly valid) + validate_email(email) + except ValidationError: + # Flag this email as an error if invalid, but continue checking + # the remaining in the list + results.append({ + 'email': email, + 'error': True, + 'invalidEmail': True, + }) + continue + try: if action == 'enroll': before, after = enroll_email(course_id, email, auto_enroll, email_students, email_params) @@ -273,6 +290,7 @@ def students_update_enrollment(request, course_id): results.append({ 'email': email, 'error': True, + 'invalidEmail': False, }) response_payload = { diff --git a/lms/static/coffee/src/instructor_dashboard/membership.coffee b/lms/static/coffee/src/instructor_dashboard/membership.coffee index cd0379c2cd..d66e4c5e11 100644 --- a/lms/static/coffee/src/instructor_dashboard/membership.coffee +++ b/lms/static/coffee/src/instructor_dashboard/membership.coffee @@ -182,7 +182,7 @@ class BetaTesterBulkAddition url: @$btn_beta_testers.data 'endpoint' data: send_data success: (data) => @display_response data - error: std_ajax_err => @fail_with_error gettext "Error adding/removing user(s) as beta tester(s)." + error: std_ajax_err => @fail_with_error gettext "Error adding/removing users as beta testers." fail_with_error: (msg) -> console.warn msg @@ -219,19 +219,19 @@ class BetaTesterBulkAddition if successes.length and data_from_server.action is 'add' `// Translators: A list of users appears after this sentence` - render_list gettext("These user(s) were successfully added as beta tester(s):"), (sr.email for sr in successes) + render_list gettext("These users were successfully added as beta testers:"), (sr.email for sr in successes) if successes.length and data_from_server.action is 'remove' `// Translators: A list of users appears after this sentence` - render_list gettext("These user(s) were successfully removed as beta tester(s):"), (sr.email for sr in successes) + render_list gettext("These users were successfully removed as beta testers:"), (sr.email for sr in successes) if errors.length and data_from_server.action is 'add' `// Translators: A list of users appears after this sentence` - render_list gettext("These user(s) were not added as beta tester(s):"), (sr.email for sr in errors) + render_list gettext("These users were not added as beta testers:"), (sr.email for sr in errors) if errors.length and data_from_server.action is 'remove' `// Translators: A list of users appears after this sentence` - render_list gettext("These user(s) were not removed as beta tester(s):"), (sr.email for sr in errors) + render_list gettext("These users were not removed as beta testers:"), (sr.email for sr in errors) if no_users.length no_users.push gettext("Users must create and activate their account before they can be promoted to beta tester.") @@ -265,7 +265,7 @@ class BatchEnrollment url: $(event.target).data 'endpoint' data: send_data success: (data) => @display_response data - error: std_ajax_err => @fail_with_error gettext "Error enrolling/unenrolling user(s)." + error: std_ajax_err => @fail_with_error gettext "Error enrolling/unenrolling users." fail_with_error: (msg) -> @@ -281,6 +281,8 @@ class BatchEnrollment # these results arrays contain student_results # only populated arrays will be rendered # + # invalid email addresses + invalid_email = [] # students for which there was an error during the action errors = [] # students who are now enrolled in the course @@ -317,9 +319,13 @@ class BatchEnrollment # student_results is of the form { # 'email': email, # 'error': True, + # 'invalidEmail': True, # if email doesn't match "[^@]+@[^@]+\.[^@]+" # } - if student_results.error + if student_results.invalidEmail + invalid_email.push student_results + + else if student_results.error errors.push student_results else if student_results.after.enrollment @@ -354,6 +360,9 @@ class BatchEnrollment @$task_response.append task_res_section + if invalid_email.length + render_list gettext("The following email addresses are invalid:"), (sr.email for sr in invalid_email) + if errors.length errors_label = do -> if data_from_server.action is 'enroll' @@ -368,49 +377,49 @@ class BatchEnrollment render_list errors_label, (sr.email for sr in errors) if enrolled.length and emailStudents - render_list gettext("Successfully enrolled and sent email to the following user(s):"), (sr.email for sr in enrolled) + render_list gettext("Successfully enrolled and sent email to the following users:"), (sr.email for sr in enrolled) if enrolled.length and not emailStudents `// Translators: A list of users appears after this sentence` - render_list gettext("Successfully enrolled the following user(s):"), (sr.email for sr in enrolled) + render_list gettext("Successfully enrolled the following users:"), (sr.email for sr in enrolled) # Student hasn't registered so we allow them to enroll if allowed.length and emailStudents `// Translators: A list of users appears after this sentence` - render_list gettext("Successfully sent enrollment emails to the following user(s). They will be allowed to enroll once they register:"), + render_list gettext("Successfully sent enrollment emails to the following users. They will be allowed to enroll once they register:"), (sr.email for sr in allowed) # Student hasn't registered so we allow them to enroll if allowed.length and not emailStudents `// Translators: A list of users appears after this sentence` - render_list gettext("These user(s) will be allowed to enroll once they register:"), + render_list gettext("These users will be allowed to enroll once they register:"), (sr.email for sr in allowed) # Student hasn't registered so we allow them to enroll with autoenroll if autoenrolled.length and emailStudents `// Translators: A list of users appears after this sentence` - render_list gettext("Successfully sent enrollment emails to the following user(s). They will be enrolled once they register:"), + render_list gettext("Successfully sent enrollment emails to the following users. They will be enrolled once they register:"), (sr.email for sr in autoenrolled) # Student hasn't registered so we allow them to enroll with autoenroll if autoenrolled.length and not emailStudents `// Translators: A list of users appears after this sentence` - render_list gettext("These user(s) will be enrolled once they register:"), + render_list gettext("These users will be enrolled once they register:"), (sr.email for sr in autoenrolled) if notenrolled.length and emailStudents `// Translators: A list of users appears after this sentence` - render_list gettext("Emails successfully sent. The following user(s) are no longer enrolled in the course:"), + render_list gettext("Emails successfully sent. The following users are no longer enrolled in the course:"), (sr.email for sr in notenrolled) if notenrolled.length and not emailStudents `// Translators: A list of users appears after this sentence` - render_list gettext("The following user(s) are no longer enrolled in the course:"), + render_list gettext("The following users are no longer enrolled in the course:"), (sr.email for sr in notenrolled) if notunenrolled.length `// Translators: A list of users appears after this sentence` - render_list gettext("These user(s) were not affliliated with the course so could not be unenrolled:"), + render_list gettext("These users were not affiliated with the course so could not be unenrolled:"), (sr.email for sr in notunenrolled) # Wrapper for auth list subsection. diff --git a/lms/templates/instructor/instructor_dashboard_2/membership.html b/lms/templates/instructor/instructor_dashboard_2/membership.html index 990b665f7f..3e4002917c 100644 --- a/lms/templates/instructor/instructor_dashboard_2/membership.html +++ b/lms/templates/instructor/instructor_dashboard_2/membership.html @@ -30,7 +30,8 @@

${_("Batch Enrollment")}

- +