Merge pull request #2992 from edx/sarina/fix-beta-batch-enrollment

Simple email checking for batch enrollment on inst dash

lms/djangoapps/instructor/tests/test_api.py

@@ -294,6 +294,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})

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
@@ -251,6 +253,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)
             elif action == 'unenroll':
@@ -273,6 +290,7 @@ def students_update_enrollment(request, course_id):
             results.append({
                 'email': email,
                 'error': True,
+                'invalidEmail': False,
             })
 
     response_payload = {

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.

lms/templates/instructor/instructor_dashboard_2/membership.html

@@ -30,7 +30,8 @@
 <div class="batch-enrollment">
   <h2> ${_("Batch Enrollment")} </h2>
   <p>
-    <label for="student-emails">${_("Enter email addresses separated by new lines or commas.")} </label>
+    <label for="student-emails">${_("Enter email addresses separated by new lines or commas.")}
+      ${_("You will not get notification for emails that bounce, so please double-check spelling.")} </label>
     <textarea rows="6" name="student-emails" placeholder="${_("Email Addresses")}" spellcheck="false"></textarea>
   </p>