Merge pull request #3084 from edx/sarina/improve-bulk-beta-add

Improve the batch beta tester add feature

lms/djangoapps/instructor/views/api.py

@@ -33,7 +33,7 @@ from django_comment_common.models import (
 )
 
 from courseware.models import StudentModule
-from student.models import unique_id_for_user
+from student.models import unique_id_for_user, CourseEnrollment
 import instructor_task.api
 from instructor_task.api_helper import AlreadyRunningError
 from instructor_task.views import get_task_completion_info
@@ -204,7 +204,7 @@ def require_level(level):
 @ensure_csrf_cookie
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
 @require_level('staff')
-@require_query_params(action="enroll or unenroll", emails="stringified list of emails")
+@require_query_params(action="enroll or unenroll", identifiers="stringified list of emails and/or usernames")
 def students_update_enrollment(request, course_id):
     """
     Enroll or unenroll students by email.
@@ -212,7 +212,7 @@ def students_update_enrollment(request, course_id):
 
     Query Parameters:
     - action in ['enroll', 'unenroll']
-    - emails is string containing a list of emails separated by anything split_input_list can handle.
+    - identifiers is string containing a list of emails and/or usernames separated by anything split_input_list can handle.
     - auto_enroll is a boolean (defaults to false)
         If auto_enroll is false, students will be allowed to enroll.
         If auto_enroll is true, students will be enrolled as soon as they register.
@@ -244,8 +244,8 @@ def students_update_enrollment(request, course_id):
     """
 
     action = request.GET.get('action')
-    emails_raw = request.GET.get('emails')
-    emails = _split_input_list(emails_raw)
+    identifiers_raw = request.GET.get('identifiers')
+    identifiers = _split_input_list(identifiers_raw)
     auto_enroll = request.GET.get('auto_enroll') in ['true', 'True', True]
     email_students = request.GET.get('email_students') in ['true', 'True', True]
 
@@ -255,23 +255,23 @@ def students_update_enrollment(request, course_id):
         email_params = get_email_params(course, auto_enroll)
 
     results = []
-    for email in emails:
+    for identifier in identifiers:
+        # First try to get a user object from the identifer
+        user = None
+        email = None
+        try:
+            user = get_student_from_identifier(identifier)
+        except User.DoesNotExist:
+            email = identifier
+        else:
+            email = user.email
+
         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
+            validate_email(email)  # Raises ValidationError if invalid
 
-        try:
             if action == 'enroll':
                 before, after = enroll_email(course_id, email, auto_enroll, email_students, email_params)
             elif action == 'unenroll':
@@ -281,20 +281,29 @@ def students_update_enrollment(request, course_id):
                     "Unrecognized action '{}'".format(action)
                 ))
 
+        except ValidationError:
+            # Flag this email as an error if invalid, but continue checking
+            # the remaining in the list
             results.append({
-                'email': email,
-                'before': before.to_dict(),
-                'after': after.to_dict(),
+                'identifier': identifier,
+                'invalidIdentifier': True,
             })
-        # catch and log any exceptions
-        # so that one error doesn't cause a 500.
+
         except Exception as exc:  # pylint: disable=W0703
+            # catch and log any exceptions
+            # so that one error doesn't cause a 500.
             log.exception("Error while #{}ing student")
             log.exception(exc)
             results.append({
-                'email': email,
+                'identifier': identifier,
                 'error': True,
-                'invalidEmail': False,
+            })
+
+        else:
+            results.append({
+                'identifier': identifier,
+                'before': before.to_dict(),
+                'after': after.to_dict(),
             })
 
     response_payload = {
@@ -310,7 +319,7 @@ def students_update_enrollment(request, course_id):
 @require_level('instructor')
 @common_exceptions_400
 @require_query_params(
-    emails="stringified list of emails",
+    identifiers="stringified list of emails and/or usernames",
     action="add or remove",
 )
 def bulk_beta_modify_access(request, course_id):
@@ -318,13 +327,15 @@ def bulk_beta_modify_access(request, course_id):
     Enroll or unenroll users in beta testing program.
 
     Query parameters:
-    - emails is string containing a list of emails separated by anything split_input_list can handle.
+    - identifiers is string containing a list of emails and/or usernames separated by
+      anything split_input_list can handle.
     - action is one of ['add', 'remove']
     """
     action = request.GET.get('action')
-    emails_raw = request.GET.get('emails')
-    emails = _split_input_list(emails_raw)
+    identifiers_raw = request.GET.get('identifiers')
+    identifiers = _split_input_list(identifiers_raw)
     email_students = request.GET.get('email_students') in ['true', 'True', True]
+    auto_enroll = request.GET.get('auto_enroll') in ['true', 'True', True]
     results = []
     rolename = 'beta'
     course = get_course_by_id(course_id)
@@ -333,11 +344,11 @@ def bulk_beta_modify_access(request, course_id):
     if email_students:
         email_params = get_email_params(course, auto_enroll=False)
 
-    for email in emails:
+    for identifier in identifiers:
         try:
             error = False
             user_does_not_exist = False
-            user = User.objects.get(email=email)
+            user = get_student_from_identifier(identifier)
 
             if action == 'add':
                 allow_access(course, user, rolename)
@@ -360,10 +371,16 @@ def bulk_beta_modify_access(request, course_id):
             # If no exception thrown, see if we should send an email
             if email_students:
                 send_beta_role_email(action, user, email_params)
+            # See if we should autoenroll the student
+            if auto_enroll:
+                # Check if student is already enrolled
+                if not CourseEnrollment.is_enrolled(user, course_id):
+                    CourseEnrollment.enroll(user, course_id)
+
         finally:
             # Tabulate the action result of this email address
             results.append({
-                'email': email,
+                'identifier': identifier,
                 'error': error,
                 'userDoesNotExist': user_does_not_exist
             })
@@ -543,8 +560,9 @@ def get_students_features(request, course_id, csv=False):  # pylint: disable=W06
 
     student_data = analytics.basic.enrolled_students_features(course_id, query_features)
 
-    # Scrape the query features for i18n - can't translate here because it breaks further queries
-    # and how the coffeescript works. The actual translation will be done in data_download.coffee
+    # Provide human-friendly and translatable names for these features. These names
+    # will be displayed in the table generated in data_download.coffee. It is not (yet)
+    # used as the header row in the CSV, but could be in the future.
     query_features_names = {
         'username': _('Username'),
         'name': _('Name'),
@@ -1086,7 +1104,7 @@ def update_forum_role_membership(request, course_id):
     target_is_instructor = has_access(user, course, 'instructor')
     # cannot revoke instructor
     if target_is_instructor and action == 'revoke' and rolename == FORUM_ROLE_ADMINISTRATOR:
-        return HttpResponseBadRequest("Cannot revoke instructor forum admin privelages.")
+        return HttpResponseBadRequest("Cannot revoke instructor forum admin privileges.")
 
     try:
         update_forum_role(course_id, user, rolename, action)

lms/static/sass/course/instructor/_instructor_2.scss

@@ -358,6 +358,11 @@ section.instructor-dashboard-content-2 {
     display: block;
   }
 
+  label[for="auto-enroll-beta"]:hover + .auto-enroll-beta-hint {
+    width: 30%;
+    display: block;
+  }
+
 
   label[for="email-students"]:hover + .email-students-hint {
     display: block;

lms/templates/instructor/instructor_dashboard_2/membership.html

@@ -30,9 +30,10 @@
 <div class="batch-enrollment">
   <h2> ${_("Batch Enrollment")} </h2>
   <p>
-    <label for="student-emails">${_("Enter email addresses separated by new lines or commas.")}
+    <label for="student-ids">
+      ${_("Enter email addresses and/or usernames 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>
+    <textarea rows="6" name="student-ids" placeholder="${_("Email Addresses/Usernames")}" spellcheck="false"></textarea>
   </p>
 
   <div class="enroll-option">
@@ -70,17 +71,30 @@
 
 %if section_data['access']['instructor']:
 <div class="batch-beta-testers">
-  <h2> ${_("Batch Beta Testers")} </h2>
+  <h2> ${_("Batch Beta Tester Addition")} </h2>
   <p>
-    <label for="student-emails-for-beta">
-      ${_("Enter email addresses separated by new lines or commas.")}<br/>
+    <label for="student-ids-for-beta">
+      ${_("Enter email addresses and/or usernames separated by new lines or commas.")}<br/>
       ${_("Note: Users must have an activated {platform_name} account before they can be enrolled as a beta tester.").format(platform_name=settings.PLATFORM_NAME)}
     </label>
 
-  <textarea rows="6" cols="50" name="student-emails-for-beta" placeholder="${_("Email Addresses")}" spellcheck="false"></textarea>
+  <textarea rows="6" cols="50" name="student-ids-for-beta" placeholder="${_("Email Addresses/Usernames")}" spellcheck="false"></textarea>
   </p>
 
   <div class="enroll-option">
+    <input type="checkbox" name="auto-enroll" value="Auto-Enroll" checked="yes">
+    <label for="auto-enroll-beta">${_("Auto Enroll")}</label>
+    <div class="hint auto-enroll-beta-hint">
+      <span class="hint-caret"></span>
+      <p>
+	${_("If this option is <em>checked</em>, users who have not enrolled in your course will be automatically enrolled.")}
+	<br /><br />
+	${_("Checking this box has no effect if 'Remove beta testers' is selected.")}
+      </p>
+    </div>
+  </div>
+
+  <div class="enroll-option">
     <input type="checkbox" name="email-students" value="Notify-students-by-email" checked="yes">
     <label for="email-students-beta">${_("Notify users by email")}</label>
     <div class="hint email-students-beta-hint">