diff --git a/lms/djangoapps/instructor/settings/common.py b/lms/djangoapps/instructor/settings/common.py index 0271822dea..d9a8ad60d1 100644 --- a/lms/djangoapps/instructor/settings/common.py +++ b/lms/djangoapps/instructor/settings/common.py @@ -71,6 +71,18 @@ def plugin_settings(settings): # .. toggle_tickets: https://github.com/edx/edx-platform/pull/5670 'ALLOW_AUTOMATED_SIGNUPS': False, + # .. toggle_name: FEATURES['ENABLE_AUTOMATED_SIGNUPS_EXTRA_FIELDS'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. toggle_description: When True, the CSV file that contains a list of + # new accounts to create and register for a course in the membership + # tab of the instructor dashboard will accept the cohort name to + # assign the new user and the enrollment course mode. + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2021-10-26 + # .. toggle_tickets: https://github.com/edx/edx-platform/pull/21260 + 'ENABLE_AUTOMATED_SIGNUPS_EXTRA_FIELDS': False, + # .. toggle_name: FEATURES['CERTIFICATES_INSTRUCTOR_GENERATION'] # lint-amnesty, pylint: disable=annotation-missing-token # .. toggle_implementation: DjangoSetting # .. toggle_default: False diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 7c7923ac9a..d8e97d9c93 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -639,7 +639,7 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas """ csv_content = b"test_student@example.com,test_student_1,tester1,USA" uploaded_file = SimpleUploadedFile("temp.csv", csv_content) - response = self.client.post(self.url, {'students_list': uploaded_file}) + response = self.client.post(self.url, {'students_list': uploaded_file, 'email-students': True}) assert response.status_code == 200 data = json.loads(response.content.decode('utf-8')) assert len(data['row_errors']) == 0 @@ -660,7 +660,7 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas """ csv_content = b"\ntest_student@example.com,test_student_1,tester1,USA\n\n" uploaded_file = SimpleUploadedFile("temp.csv", csv_content) - response = self.client.post(self.url, {'students_list': uploaded_file}) + response = self.client.post(self.url, {'students_list': uploaded_file, 'email-students': True}) assert response.status_code == 200 data = json.loads(response.content.decode('utf-8')) assert len(data['row_errors']) == 0 @@ -742,10 +742,10 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas assert len(data['row_errors']) == 0 assert len(data['warnings']) == 0 assert len(data['general_errors']) == 1 - assert data['general_errors'][0]['response'] ==\ - 'Data in row #1 must have exactly four columns: email, username, full name, and country' - # lint-amnesty, pylint: disable=line-too-long - + assert data['general_errors'][0]['response'] == ( + 'Data in row #1 must have exactly four columns: email, username, ' + 'full name, and country.' + ) manual_enrollments = ManualEnrollmentAudit.objects.all() assert manual_enrollments.count() == 0 diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 6ace29e39c..b5d4e6d30b 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -33,6 +33,7 @@ from edx_rest_framework_extensions.auth.session.authentication import SessionAut from edx_when.api import get_date_for_block from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey +from openedx.core.djangoapps.course_groups.cohorts import get_cohort_by_name from rest_framework import serializers, status from rest_framework.permissions import IsAdminUser, IsAuthenticated from rest_framework.response import Response @@ -101,7 +102,8 @@ from lms.djangoapps.instructor_task import api as task_api from lms.djangoapps.instructor_task.api_helper import AlreadyRunningError, QueueConnectionError from lms.djangoapps.instructor_task.models import ReportStore from openedx.core.djangoapps.content.course_overviews.models import CourseOverview -from openedx.core.djangoapps.course_groups.cohorts import is_course_cohorted +from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort, is_course_cohorted +from openedx.core.djangoapps.course_groups.models import CourseUserGroup from openedx.core.djangoapps.django_comment_common.models import ( CourseDiscussionSettings, FORUM_ROLE_ADMINISTRATOR, @@ -274,12 +276,6 @@ def require_finance_admin(func): return wrapped -EMAIL_INDEX = 0 -USERNAME_INDEX = 1 -NAME_INDEX = 2 -COUNTRY_INDEX = 3 - - @require_POST @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @@ -289,6 +285,7 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man Create new account and Enroll students in this course. Passing a csv file that contains a list of students. Order in csv should be the following email = 0; username = 1; name = 2; country = 3. + If there are more than 4 columns in the csv: cohort = 4, course mode = 5. Requires staff access. -If the email address and username already exists and the user is enrolled in the course, @@ -315,14 +312,24 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man row_errors = [] general_errors = [] + # email-students is a checkbox input type; will be present in POST if checked, absent otherwise + notify_by_email = 'email-students' in request.POST + # for white labels we use 'shopping cart' which uses CourseMode.HONOR as # course mode for creating course enrollments. if CourseMode.is_white_label(course_id): - course_mode = CourseMode.HONOR + default_course_mode = CourseMode.HONOR else: - course_mode = None + default_course_mode = None - if 'students_list' in request.FILES: + # Allow bulk enrollments in all non-expired course modes including "credit" (which is non-selectable) + valid_course_modes = set(map(lambda x: x.slug, CourseMode.modes_for_course( + course_id=course_id, + only_selectable=False, + include_expired=False, + ))) + + if 'students_list' in request.FILES: # lint-amnesty, pylint: disable=too-many-nested-blocks students = [] try: @@ -345,27 +352,107 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man upload_file.close() generated_passwords = [] - row_num = 0 - for student in students: - row_num = row_num + 1 + # To skip fetching cohorts from the DB while iterating on students, + # {: CourseUserGroup} + cohorts_cache = {} + already_warned_not_cohorted = False + extra_fields_is_enabled = configuration_helpers.get_value( + 'ENABLE_AUTOMATED_SIGNUPS_EXTRA_FIELDS', + settings.FEATURES.get('ENABLE_AUTOMATED_SIGNUPS_EXTRA_FIELDS', False), + ) - # verify that we have exactly four columns in every row but allow for blank lines - if len(student) != 4: - if student: - error = _('Data in row #{row_num} must have exactly four columns: ' - 'email, username, full name, and country').format(row_num=row_num) - general_errors.append({ - 'username': '', - 'email': '', - 'response': error - }) + # Iterate each student in the uploaded csv file. + for row_num, student in enumerate(students, 1): + + # Verify that we have the expected number of columns in every row + # but allow for blank lines. + if not student: continue - # Iterate each student in the uploaded csv file. - email = student[EMAIL_INDEX] - username = student[USERNAME_INDEX] - name = student[NAME_INDEX] - country = student[COUNTRY_INDEX][:2] + if extra_fields_is_enabled: + is_valid_csv = 4 <= len(student) <= 6 + error = _('Data in row #{row_num} must have between four and six columns: ' + 'email, username, full name, country, cohort, and course mode. ' + 'The last two columns are optional.').format(row_num=row_num) + else: + is_valid_csv = len(student) == 4 + error = _('Data in row #{row_num} must have exactly four columns: ' + 'email, username, full name, and country.').format(row_num=row_num) + + if not is_valid_csv: + general_errors.append({ + 'username': '', + 'email': '', + 'response': error + }) + continue + + # Extract each column, handle optional columns if they exist. + email, username, name, country, *optional_cols = student + if optional_cols: + optional_cols.append(default_course_mode) + cohort_name, course_mode, *_tail = optional_cols + else: + cohort_name = None + course_mode = None + + # Validate cohort name, and get the cohort object. Skip if course + # is not cohorted. + cohort = None + + if cohort_name and not already_warned_not_cohorted: + if not is_course_cohorted(course_id): + row_errors.append({ + 'username': username, + 'email': email, + 'response': _('Course is not cohorted but cohort provided. ' + 'Ignoring cohort assignment for all users.') + }) + already_warned_not_cohorted = True + elif cohort_name in cohorts_cache: + cohort = cohorts_cache[cohort_name] + else: + # Don't attempt to create cohort or assign student if cohort + # does not exist. + try: + cohort = get_cohort_by_name(course_id, cohort_name) + except CourseUserGroup.DoesNotExist: + row_errors.append({ + 'username': username, + 'email': email, + 'response': _('Cohort name not found: {cohort}. ' + 'Ignoring cohort assignment for ' + 'all users.').format(cohort=cohort_name) + }) + cohorts_cache[cohort_name] = cohort + + # Validate course mode. + if not course_mode: + course_mode = default_course_mode + + if (course_mode is not None + and course_mode not in valid_course_modes): + # If `default is None` and the user is already enrolled, + # `CourseEnrollment.change_mode()` will not update the mode, + # hence two error messages. + if default_course_mode is None: + err_msg = _( + 'Invalid course mode: {mode}. Falling back to the ' + 'default mode, or keeping the current mode in case the ' + 'user is already enrolled.' + ).format(mode=course_mode) + else: + err_msg = _( + 'Invalid course mode: {mode}. Failling back to ' + '{default_mode}, or resetting to {default_mode} in case ' + 'the user is already enrolled.' + ).format(mode=course_mode, default_mode=default_course_mode) + row_errors.append({ + 'username': username, + 'email': email, + 'response': err_msg, + }) + course_mode = default_course_mode email_params = get_email_params(course, True, secure=request.is_secure()) try: @@ -415,8 +502,19 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man enroll_email(course_id=course_id, student_email=email, auto_enroll=True, - email_students=True, + email_students=notify_by_email, email_params=email_params) + else: + # update the course mode if already enrolled + existing_enrollment = CourseEnrollment.get_enrollment(user, course_id) + if existing_enrollment.mode != course_mode: + existing_enrollment.change_mode(mode=course_mode) + if cohort: + try: + add_user_to_cohort(cohort, user) + except ValueError: + # user already in this cohort; ignore + pass elif is_email_retired(email): # We are either attempting to enroll a retired user or create a new user with an email which is # already associated with a retired account. Simply block these attempts. @@ -433,9 +531,32 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man # will raise an IntegrityError exception. password = generate_unique_password(generated_passwords) errors = create_and_enroll_user( - email, username, name, country, password, course_id, course_mode, request.user, email_params + email, + username, + name, + country, + password, + course_id, + course_mode, + request.user, + email_params, + email_user=notify_by_email, ) row_errors.extend(errors) + if cohort: + try: + add_user_to_cohort(cohort, email) + except ValueError: + # user already in this cohort; ignore + # NOTE: Checking this here may be unnecessary if we can prove that a new user will never be + # automatically assigned to a cohort from the above. + pass + except ValidationError: + row_errors.append({ + 'username': username, + 'email': email, + 'response': _('Invalid email {email_address}.').format(email_address=email), + }) else: general_errors.append({ @@ -521,7 +642,18 @@ def create_manual_course_enrollment(user, course_id, mode, enrolled_by, reason, return enrollment_obj -def create_and_enroll_user(email, username, name, country, password, course_id, course_mode, enrolled_by, email_params): +def create_and_enroll_user( + email, + username, + name, + country, + password, + course_id, + course_mode, + enrolled_by, + email_params, + email_user=True, +): """ Create a new user and enroll him/her to the given course, return list of errors in the following format Error format: @@ -539,7 +671,8 @@ def create_and_enroll_user(email, username, name, country, password, course_id, :param course_mode: mode for user enrollment, e.g. 'honor', 'audit' etc. :param enrolled_by: User who made the manual enrollment entry (usually instructor or support) :param email_params: information to send to the user via email - + :param email_user: If True and it's a new user, an email will be sent to + them upon account creation. :return: list of errors """ errors = [] @@ -569,29 +702,32 @@ def create_and_enroll_user(email, username, name, country, password, course_id, 'username': username, 'email': email, 'response': type(ex).__name__, }) else: - try: - # It's a new user, an email will be sent to each newly created user. - email_params.update({ - 'message_type': 'account_creation_and_enrollment', - 'email_address': email, - 'password': password, - 'platform_name': configuration_helpers.get_value('platform_name', settings.PLATFORM_NAME), - }) - send_mail_to_student(email, email_params) - except Exception as ex: # pylint: disable=broad-except - log.exception( - f"Exception '{type(ex).__name__}' raised while sending email to new user." - ) - errors.append({ - 'username': username, - 'email': email, - 'response': - _("Error '{error}' while sending email to new user (user email={email}). " - "Without the email student would not be able to login. " - "Please contact support for further information.").format(error=type(ex).__name__, email=email), - }) - else: - log.info('email sent to new created user at %s', email) + if email_user: + try: + # It's a new user, an email will be sent to each newly created user. + email_params.update({ + 'message_type': 'account_creation_and_enrollment', + 'email_address': email, + 'password': password, + 'platform_name': configuration_helpers.get_value('platform_name', settings.PLATFORM_NAME), + }) + send_mail_to_student(email, email_params) + except Exception as ex: # pylint: disable=broad-except + log.exception( + f"Exception '{type(ex).__name__}' raised while sending email to new user." + ) + errors.append({ + 'username': username, + 'email': email, + 'response': + _("Error '{error}' while sending email to new user (user email={email}). " + "Without the email student would not be able to login. " + "Please contact support for further information.").format( + error=type(ex).__name__, email=email + ), + }) + else: + log.info('email sent to new created user at %s', email) return errors diff --git a/lms/templates/instructor/instructor_dashboard_2/membership.html b/lms/templates/instructor/instructor_dashboard_2/membership.html index baa11767a9..6d64f47c8e 100644 --- a/lms/templates/instructor/instructor_dashboard_2/membership.html +++ b/lms/templates/instructor/instructor_dashboard_2/membership.html @@ -62,7 +62,7 @@ from openedx.core.djangolib.markup import HTML, Text

${_("Register/Enroll Students")}

-

${_("To register and enroll a list of users in this course, choose a CSV file that contains the following columns in this exact order: email, username, name, and country. Please include one student per row and do not include any headers, footers, or blank lines.")}

+

${_("To register and enroll a list of users in this course, choose a CSV file that contains the following columns in this exact order: email, username, name, and country. Please include one student per row and do not include any headers, footers, or blank lines. Optionally, extra columns can be provided: cohort and course mode, in that order. These extra columns may be blank.")}

@@ -73,6 +73,19 @@ from openedx.core.djangolib.markup import HTML, Text
+
+ +