From 4122f695d89fc3f4be5bcbf77036865f62859053 Mon Sep 17 00:00:00 2001 From: Saleem Latif Date: Tue, 12 Jan 2016 17:28:49 +0500 Subject: [PATCH 1/3] Bulk uploads (CSV) of manual enrollments on white labels should be performed as 'honor' modes --- lms/djangoapps/instructor/tests/test_api.py | 111 +++++++++++++++++++- lms/djangoapps/instructor/views/api.py | 17 ++- 2 files changed, 123 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 016af27a45..ce021ca2b8 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -394,13 +394,39 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas def setUpClass(cls): super(TestInstructorAPIBulkAccountCreationAndEnrollment, cls).setUpClass() cls.course = CourseFactory.create() - cls.url = reverse('register_and_enroll_students', kwargs={'course_id': cls.course.id.to_deprecated_string()}) + + # Create a course with mode 'audit' + cls.audit_course = CourseFactory.create() + CourseModeFactory(course_id=cls.audit_course.id, mode_slug=CourseMode.AUDIT) + + cls.url = reverse( + 'register_and_enroll_students', kwargs={'course_id': cls.course.id.to_deprecated_string()} + ) + cls.audit_course_url = reverse( + 'register_and_enroll_students', kwargs={'course_id': cls.audit_course.id.to_deprecated_string()} + ) def setUp(self): super(TestInstructorAPIBulkAccountCreationAndEnrollment, self).setUp() + # Create a course with mode 'honor' and with price + self.white_label_course = CourseFactory.create() + self.white_label_course_mode = CourseModeFactory( + course_id=self.white_label_course.id, + mode_slug=CourseMode.HONOR, + min_price=10, + suggested_prices='10', + ) + + self.white_label_course_url = reverse( + 'register_and_enroll_students', kwargs={'course_id': self.white_label_course.id.to_deprecated_string()} + ) + self.request = RequestFactory().request() self.instructor = InstructorFactory(course_key=self.course.id) + self.audit_course_instructor = InstructorFactory(course_key=self.audit_course.id) + self.white_label_course_instructor = InstructorFactory(course_key=self.white_label_course.id) + self.client.login(username=self.instructor.username, password='test') self.not_enrolled_student = UserFactory( @@ -687,6 +713,89 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas manual_enrollments = ManualEnrollmentAudit.objects.all() self.assertEqual(manual_enrollments.count(), 0) + @patch.dict(settings.FEATURES, {'ALLOW_AUTOMATED_SIGNUPS': True}) + def test_audit_enrollment_mode(self): + """ + Test that enrollment mode for audit courses (paid courses) is 'audit'. + """ + # Login Audit Course instructor + self.client.login(username=self.audit_course_instructor.username, password='test') + + csv_content = "test_student_wl@example.com,test_student_wl,Test Student,USA" + uploaded_file = SimpleUploadedFile("temp.csv", csv_content) + response = self.client.post(self.audit_course_url, {'students_list': uploaded_file}) + + self.assertEqual(response.status_code, 200) + data = json.loads(response.content) + self.assertEquals(len(data['row_errors']), 0) + self.assertEquals(len(data['warnings']), 0) + self.assertEquals(len(data['general_errors']), 0) + + manual_enrollments = ManualEnrollmentAudit.objects.all() + self.assertEqual(manual_enrollments.count(), 1) + self.assertEqual(manual_enrollments[0].state_transition, UNENROLLED_TO_ENROLLED) + + # Verify enrollment modes to be 'audit' + for enrollment in manual_enrollments: + self.assertEqual(enrollment.enrollment.mode, CourseMode.AUDIT) + + @patch.dict(settings.FEATURES, {'ALLOW_AUTOMATED_SIGNUPS': True}) + def test_honor_enrollment_mode(self): + """ + Test that enrollment mode for unpaid honor courses is 'honor'. + """ + # Remove white label course price + self.white_label_course_mode.min_price = 0 + self.white_label_course_mode.suggested_prices = '' + self.white_label_course_mode.save() # pylint: disable=no-member + + # Login Audit Course instructor + self.client.login(username=self.white_label_course_instructor.username, password='test') + + csv_content = "test_student_wl@example.com,test_student_wl,Test Student,USA" + uploaded_file = SimpleUploadedFile("temp.csv", csv_content) + response = self.client.post(self.white_label_course_url, {'students_list': uploaded_file}) + + self.assertEqual(response.status_code, 200) + data = json.loads(response.content) + self.assertEquals(len(data['row_errors']), 0) + self.assertEquals(len(data['warnings']), 0) + self.assertEquals(len(data['general_errors']), 0) + + manual_enrollments = ManualEnrollmentAudit.objects.all() + self.assertEqual(manual_enrollments.count(), 1) + self.assertEqual(manual_enrollments[0].state_transition, UNENROLLED_TO_ENROLLED) + + # Verify enrollment modes to be 'honor' + for enrollment in manual_enrollments: + self.assertEqual(enrollment.enrollment.mode, CourseMode.HONOR) + + @patch.dict(settings.FEATURES, {'ALLOW_AUTOMATED_SIGNUPS': True}) + def test_default_shopping_cart_enrollment_mode_for_white_label(self): + """ + Test that enrollment mode for white label courses (paid courses) is DEFAULT_SHOPPINGCART_MODE_SLUG. + """ + # Login white label course instructor + self.client.login(username=self.white_label_course_instructor.username, password='test') + + csv_content = "test_student_wl@example.com,test_student_wl,Test Student,USA" + uploaded_file = SimpleUploadedFile("temp.csv", csv_content) + response = self.client.post(self.white_label_course_url, {'students_list': uploaded_file}) + + self.assertEqual(response.status_code, 200) + data = json.loads(response.content) + self.assertEquals(len(data['row_errors']), 0) + self.assertEquals(len(data['warnings']), 0) + self.assertEquals(len(data['general_errors']), 0) + + manual_enrollments = ManualEnrollmentAudit.objects.all() + self.assertEqual(manual_enrollments.count(), 1) + self.assertEqual(manual_enrollments[0].state_transition, UNENROLLED_TO_ENROLLED) + + # Verify enrollment modes to be CourseMode.DEFAULT_SHOPPINGCART_MODE_SLUG + for enrollment in manual_enrollments: + self.assertEqual(enrollment.enrollment.mode, CourseMode.DEFAULT_SHOPPINGCART_MODE_SLUG) + @attr('shard_1') @ddt.ddt diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index d52efa6c10..0027034329 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -343,6 +343,13 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man row_errors = [] general_errors = [] + # for white labels we use 'shopping cart' which uses CourseMode.DEFAULT_SHOPPINGCART_MODE_SLUG as + # course mode for creating course enrollments. + if CourseMode.is_white_label(course_id): + course_mode = CourseMode.DEFAULT_SHOPPINGCART_MODE_SLUG + else: + course_mode = None + if 'students_list' in request.FILES: students = [] @@ -418,7 +425,7 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man # make sure user is enrolled in course if not CourseEnrollment.is_enrolled(user, course_id): - enrollment_obj = CourseEnrollment.enroll(user, course_id) + enrollment_obj = CourseEnrollment.enroll(user, course_id, mode=course_mode) reason = 'Enrolling via csv upload' ManualEnrollmentAudit.create_manual_enrollment_audit( request.user, email, UNENROLLED_TO_ENROLLED, reason, enrollment_obj @@ -437,7 +444,9 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man try: with transaction.atomic(): - enrollment_obj = create_and_enroll_user(email, username, name, country, password, course_id) + enrollment_obj = create_and_enroll_user( + email, username, name, country, password, course_id, course_mode + ) reason = 'Enrolling via csv upload' ManualEnrollmentAudit.create_manual_enrollment_audit( request.user, email, UNENROLLED_TO_ENROLLED, reason, enrollment_obj @@ -497,7 +506,7 @@ def generate_unique_password(generated_passwords, password_length=12): return password -def create_and_enroll_user(email, username, name, country, password, course_id): +def create_and_enroll_user(email, username, name, country, password, course_id, course_mode=None): """ Creates a user and enroll him/her in the course""" user = User.objects.create_user(username, email, password) @@ -510,7 +519,7 @@ def create_and_enroll_user(email, username, name, country, password, course_id): profile.save() # try to enroll the user in this course - return CourseEnrollment.enroll(user, course_id) + return CourseEnrollment.enroll(user, course_id, mode=course_mode) @ensure_csrf_cookie From 041ec4c26788761c8a88f15090fbcf02bd5a7c70 Mon Sep 17 00:00:00 2001 From: Saleem Latif Date: Thu, 14 Jan 2016 13:36:27 +0500 Subject: [PATCH 2/3] Remove unused imports from /tests/test_api.py --- lms/djangoapps/instructor/tests/test_api.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index ce021ca2b8..0af1d9b132 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -9,7 +9,6 @@ import random import pytz import io import json -import requests import shutil import tempfile from urllib import quote @@ -51,8 +50,6 @@ from student.models import ( ) from student.tests.factories import UserFactory, CourseModeFactory, AdminFactory from student.roles import CourseBetaTesterRole, CourseSalesAdminRole, CourseFinanceAdminRole, CourseInstructorRole -from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.fields import Date From a856fec574f5ad25e5c50600274e68c16edf610e Mon Sep 17 00:00:00 2001 From: Saleem Latif Date: Thu, 14 Jan 2016 14:33:01 +0500 Subject: [PATCH 3/3] refactor code, remove duplicate code --- lms/djangoapps/instructor/tests/test_api.py | 8 +- lms/djangoapps/instructor/views/api.py | 159 +++++++++++++++----- 2 files changed, 124 insertions(+), 43 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 0af1d9b132..63fd554d0b 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -397,10 +397,10 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas CourseModeFactory(course_id=cls.audit_course.id, mode_slug=CourseMode.AUDIT) cls.url = reverse( - 'register_and_enroll_students', kwargs={'course_id': cls.course.id.to_deprecated_string()} + 'register_and_enroll_students', kwargs={'course_id': unicode(cls.course.id)} ) cls.audit_course_url = reverse( - 'register_and_enroll_students', kwargs={'course_id': cls.audit_course.id.to_deprecated_string()} + 'register_and_enroll_students', kwargs={'course_id': unicode(cls.audit_course.id)} ) def setUp(self): @@ -416,7 +416,7 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas ) self.white_label_course_url = reverse( - 'register_and_enroll_students', kwargs={'course_id': self.white_label_course.id.to_deprecated_string()} + 'register_and_enroll_students', kwargs={'course_id': unicode(self.white_label_course.id)} ) self.request = RequestFactory().request() @@ -650,7 +650,7 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas "test_student2@example.com,test_student_1,tester2,US" uploaded_file = SimpleUploadedFile("temp.csv", csv_content) - with patch('instructor.views.api.create_and_enroll_user') as mock: + with patch('instructor.views.api.create_manual_course_enrollment') as mock: mock.side_effect = NonExistentCourseError() response = self.client.post(self.url, {'students_list': uploaded_file}) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 0027034329..2d15676fdd 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -423,17 +423,16 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man email ) - # make sure user is enrolled in course + # enroll a user if it is not already enrolled. if not CourseEnrollment.is_enrolled(user, course_id): - enrollment_obj = CourseEnrollment.enroll(user, course_id, mode=course_mode) - reason = 'Enrolling via csv upload' - ManualEnrollmentAudit.create_manual_enrollment_audit( - request.user, email, UNENROLLED_TO_ENROLLED, reason, enrollment_obj - ) - log.info( - u'user %s enrolled in the course %s', - username, - course.id, + # Enroll user to the course and add manual enrollment audit trail + create_manual_course_enrollment( + user=user, + course_id=course_id, + mode=course_mode, + enrolled_by=request.user, + reason='Enrolling via csv upload', + state_transition=UNENROLLED_TO_ENROLLED, ) enroll_email(course_id=course_id, student_email=email, auto_enroll=True, email_students=True, email_params=email_params) else: @@ -441,31 +440,10 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man # If username already exists in the database, then create_and_enroll_user # will raise an IntegrityError exception. password = generate_unique_password(generated_passwords) - - try: - with transaction.atomic(): - enrollment_obj = create_and_enroll_user( - email, username, name, country, password, course_id, course_mode - ) - reason = 'Enrolling via csv upload' - ManualEnrollmentAudit.create_manual_enrollment_audit( - request.user, email, UNENROLLED_TO_ENROLLED, reason, enrollment_obj - ) - except IntegrityError: - row_errors.append({ - 'username': username, 'email': email, 'response': _('Username {user} already exists.').format(user=username)}) - except Exception as ex: - log.exception(type(ex).__name__) - row_errors.append({ - 'username': username, 'email': email, 'response': type(ex).__name__}) - else: - # It's a new user, an email will be sent to each newly created user. - email_params['message'] = 'account_creation_and_enrollment' - email_params['email_address'] = email - email_params['password'] = password - email_params['platform_name'] = microsite.get_value('platform_name', settings.PLATFORM_NAME) - send_mail_to_student(email, email_params) - log.info(u'email sent to new created user at %s', email) + errors = create_and_enroll_user( + email, username, name, country, password, course_id, course_mode, request.user, email_params + ) + row_errors.extend(errors) else: general_errors.append({ @@ -506,9 +484,18 @@ def generate_unique_password(generated_passwords, password_length=12): return password -def create_and_enroll_user(email, username, name, country, password, course_id, course_mode=None): - """ Creates a user and enroll him/her in the course""" +def create_user_and_user_profile(email, username, name, country, password): + """ + Create a new user, add a new Registration instance for letting user verify its identity and create a user profile. + :param email: user's email address + :param username: user's username + :param name: user's name + :param country: user's country + :param password: user's password + + :return: User instance of the new user. + """ user = User.objects.create_user(username, email, password) reg = Registration() reg.register(user) @@ -518,8 +505,102 @@ def create_and_enroll_user(email, username, name, country, password, course_id, profile.country = country profile.save() - # try to enroll the user in this course - return CourseEnrollment.enroll(user, course_id, mode=course_mode) + return user + + +def create_manual_course_enrollment(user, course_id, mode, enrolled_by, reason, state_transition): + """ + Create course enrollment for the given student and create manual enrollment audit trail. + + :param user: User who is to enroll in course + :param course_id: course identifier of the course in which to enroll the user. + :param 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 reason: Reason behind manual enrollment + :param state_transition: state transition denoting whether student enrolled from un-enrolled, + un-enrolled from enrolled etc. + :return CourseEnrollment instance. + """ + enrollment_obj = CourseEnrollment.enroll(user, course_id, mode=mode) + ManualEnrollmentAudit.create_manual_enrollment_audit( + enrolled_by, user.email, state_transition, reason, enrollment_obj + ) + + log.info(u'user %s enrolled in the course %s', user.username, course_id) + return enrollment_obj + + +def create_and_enroll_user(email, username, name, country, password, course_id, course_mode, enrolled_by, email_params): + """ + Create a new user and enroll him/her to the given course, return list of errors in the following format + Error format: + each error is key-value pait dict with following key-value pairs. + 1. username: username of the user to enroll + 1. email: email of the user to enroll + 1. response: readable error message + + :param email: user's email address + :param username: user's username + :param name: user's name + :param country: user's country + :param password: user's password + :param course_id: course identifier of the course in which to enroll the user. + :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 + + :return: list of errors + """ + errors = list() + try: + with transaction.atomic(): + # Create a new user + user = create_user_and_user_profile(email, username, name, country, password) + + # Enroll user to the course and add manual enrollment audit trail + create_manual_course_enrollment( + user=user, + course_id=course_id, + mode=course_mode, + enrolled_by=enrolled_by, + reason='Enrolling via csv upload', + state_transition=UNENROLLED_TO_ENROLLED, + ) + except IntegrityError: + errors.append({ + 'username': username, 'email': email, 'response': _('Username {user} already exists.').format(user=username) + }) + except Exception as ex: # pylint: disable=broad-except + log.exception(type(ex).__name__) + errors.append({ + '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': 'account_creation_and_enrollment', + 'email_address': email, + 'password': password, + 'platform_name': microsite.get_value('platform_name', settings.PLATFORM_NAME), + }) + send_mail_to_student(email, email_params) + except Exception as ex: # pylint: disable=broad-except + log.exception( + "Exception '{exception}' raised while sending email to new user.".format(exception=type(ex).__name__) + ) + 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(u'email sent to new created user at %s', email) + + return errors @ensure_csrf_cookie