From b6f0c8dd30a7527b7ce7a371243c24aa4e7e76f0 Mon Sep 17 00:00:00 2001 From: stephensanchez Date: Fri, 10 Oct 2014 13:42:37 +0000 Subject: [PATCH 01/12] Update the student transfer command to transfer to multiple courses. Quick cleanup Making kwarg to suppress unenroll events. pylint and pep8 errors cleaned up. add commit on success. Changing transactional behavior of the management command. --- .../management/commands/transfer_students.py | 152 ++++++++++++------ .../student/management/tests/__init__.py | 1 + .../tests/test_transfer_students.py | 60 +++++++ common/djangoapps/student/models.py | 11 +- 4 files changed, 171 insertions(+), 53 deletions(-) create mode 100644 common/djangoapps/student/management/tests/__init__.py create mode 100644 common/djangoapps/student/management/tests/test_transfer_students.py diff --git a/common/djangoapps/student/management/commands/transfer_students.py b/common/djangoapps/student/management/commands/transfer_students.py index c1b46cdeff..041402e19e 100644 --- a/common/djangoapps/student/management/commands/transfer_students.py +++ b/common/djangoapps/student/management/commands/transfer_students.py @@ -1,81 +1,135 @@ +""" +Transfer Student Management Command +""" +from django.db import transaction +from opaque_keys.edx.keys import CourseKey from optparse import make_option -from django.core.management.base import BaseCommand from django.contrib.auth.models import User from student.models import CourseEnrollment from shoppingcart.models import CertificateItem -from opaque_keys.edx.locations import SlashSeparatedCourseKey +from track.management.tracked_command import TrackedCommand -class Command(BaseCommand): +class TransferStudentError(Exception): + """Generic Error when handling student transfers.""" + pass + + +class Command(TrackedCommand): + """Management Command for transferring students from one course to new courses.""" help = """ This command takes two course ids as input and transfers all students enrolled in one course into the other. This will - remove them from the first class and enroll them in the second - class in the same mode as the first one. eg. honor, verified, + remove them from the first class and enroll them in the specified + class(es) in the same mode as the first one. eg. honor, verified, audit. example: # Transfer students from the old demoX class to a new one. manage.py ... transfer_students -f edX/Open_DemoX/edx_demo_course -t edX/Open_DemoX/new_demoX + + # Transfer students from old course to new, with original certificate items. + manage.py ... transfer_students -f edX/Open_DemoX/edx_demo_course -t edX/Open_DemoX/new_demoX -c true + + # Transfer students from the old demoX class into two new classes. + manage.py ... transfer_students -f edX/Open_DemoX/edx_demo_course + -t edX/Open_DemoX/new_demoX,edX/Open_DemoX/edX_Insider + """ - option_list = BaseCommand.option_list + ( + option_list = TrackedCommand.option_list + ( make_option('-f', '--from', metavar='SOURCE_COURSE', dest='source_course', help='The course to transfer students from.'), make_option('-t', '--to', - metavar='DEST_COURSE', - dest='dest_course', - help='The new course to enroll the student into.'), + metavar='DEST_COURSE_LIST', + dest='dest_course_list', + help='The new course(es) to enroll the student into.'), + make_option('-c', '--transfer-certificates', + metavar='TRANSFER_CERTIFICATES', + dest='transfer_certificates', + help="If True, try to transfer certificate items to the new course.") ) - def handle(self, *args, **options): - source_key = SlashSeparatedCourseKey.from_deprecated_string(options['source_course']) - dest_key = SlashSeparatedCourseKey.from_deprecated_string(options['dest_course']) + @transaction.commit_manually + def handle(self, *args, **options): # pylint: disable=unused-argument + source_key = CourseKey.from_string(options.get('source_course', '')) + dest_keys = [] + for course_key in options.get('dest_course_list', '').split(','): + dest_keys.append(CourseKey.from_string(course_key)) + + if not source_key or not dest_keys: + raise TransferStudentError(u"Must have a source course and destination course specified.") + + tc_option = options.get('transfer_certificates', '') + transfer_certificates = ('true' == tc_option.lower()) if tc_option else False + if transfer_certificates and len(dest_keys) != 1: + raise TransferStudentError(u"Cannot transfer certificate items from one course to many.") source_students = User.objects.filter( courseenrollment__course_id=source_key ) for user in source_students: - if CourseEnrollment.is_enrolled(user, dest_key): - # Un Enroll from source course but don't mess - # with the enrollment in the destination course. - CourseEnrollment.unenroll(user, source_key) - print("Unenrolled {} from {}".format(user.username, source_key.to_deprecated_string())) - msg = "Skipping {}, already enrolled in destination course {}" - print(msg.format(user.username, dest_key.to_deprecated_string())) - continue + with transaction.commit_on_success(): + print("Moving {}.".format(user.username)) + # Find the old enrollment. + enrollment = CourseEnrollment.objects.get( + user=user, + course_id=source_key + ) - print("Moving {}.".format(user.username)) - # Find the old enrollment. - enrollment = CourseEnrollment.objects.get( - user=user, - course_id=source_key + # Move the Student between the classes. + mode = enrollment.mode + old_is_active = enrollment.is_active + CourseEnrollment.unenroll(user, source_key, emit_unenrollment_event=False) + print(u"Unenrolled {} from {}".format(user.username, unicode(source_key))) + + for dest_key in dest_keys: + if CourseEnrollment.is_enrolled(user, dest_key): + # Un Enroll from source course but don't mess + # with the enrollment in the destination course. + msg = u"Skipping {}, already enrolled in destination course {}" + print(msg.format(user.username, unicode(dest_key))) + else: + new_enrollment = CourseEnrollment.enroll(user, dest_key, mode=mode) + + # Un-enroll from the new course if the user had un-enrolled + # form the old course. + if not old_is_active: + new_enrollment.update_enrollment(is_active=False, emit_unenrollment_event=False) + + if transfer_certificates: + self._transfer_certificate_item(source_key, enrollment, user, dest_keys, new_enrollment) + + @staticmethod + def _transfer_certificate_item(source_key, enrollment, user, dest_keys, new_enrollment): + """ Transfer the certificate item from one course to another. + + Do not use this generally, since certificate items are directly associated with a particular purchase. + This should only be used when a single course to a new location. This cannot be used when transferring + from one course to many. + + Args: + source_key (str): The course key string representation for the original course. + enrollment (CourseEnrollment): The original enrollment to move the certificate item from. + user (User): The user to transfer the item for. + dest_keys (list): A list of course key strings to transfer the item to. + new_enrollment (CourseEnrollment): The new enrollment to associate the certificate item with. + + Returns: + None + + """ + try: + certificate_item = CertificateItem.objects.get( + course_id=source_key, + course_enrollment=enrollment ) + except CertificateItem.DoesNotExist: + print(u"No certificate for {}".format(user)) + return - # Move the Student between the classes. - mode = enrollment.mode - old_is_active = enrollment.is_active - CourseEnrollment.unenroll(user, source_key) - new_enrollment = CourseEnrollment.enroll(user, dest_key, mode=mode) - - # Unenroll from the new coures if the user had unenrolled - # form the old course. - if not old_is_active: - new_enrollment.update_enrollment(is_active=False) - - if mode == 'verified': - try: - certificate_item = CertificateItem.objects.get( - course_id=source_key, - course_enrollment=enrollment - ) - except CertificateItem.DoesNotExist: - print("No certificate for {}".format(user)) - continue - - certificate_item.course_id = dest_key - certificate_item.course_enrollment = new_enrollment - certificate_item.save() + certificate_item.course_id = dest_keys[0] + certificate_item.course_enrollment = new_enrollment diff --git a/common/djangoapps/student/management/tests/__init__.py b/common/djangoapps/student/management/tests/__init__.py new file mode 100644 index 0000000000..b7d6ea0722 --- /dev/null +++ b/common/djangoapps/student/management/tests/__init__.py @@ -0,0 +1 @@ +"""Tests for Student Management Commands.""" diff --git a/common/djangoapps/student/management/tests/test_transfer_students.py b/common/djangoapps/student/management/tests/test_transfer_students.py new file mode 100644 index 0000000000..caebeeace2 --- /dev/null +++ b/common/djangoapps/student/management/tests/test_transfer_students.py @@ -0,0 +1,60 @@ +""" +Tests the transfer student management command +""" +from django.conf import settings +from opaque_keys.edx import locator +import unittest +import ddt +from student.management.commands import transfer_students +from student.models import CourseEnrollment +from student.tests.factories import UserFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory + + +@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') +@ddt.ddt +class TestTransferStudents(ModuleStoreTestCase): + """Tests for transferring students between courses.""" + + PASSWORD = 'test' + + def test_transfer_students(self): + student = UserFactory() + student.set_password(self.PASSWORD) # pylint: disable=E1101 + student.save() # pylint: disable=E1101 + + # Original Course + original_course_location = locator.CourseLocator('Org0', 'Course0', 'Run0') + course = self._create_course(original_course_location) + # Enroll the student in 'verified' + CourseEnrollment.enroll(student, course.id, mode="verified") + + # New Course 1 + course_location_one = locator.CourseLocator('Org1', 'Course1', 'Run1') + new_course_one = self._create_course(course_location_one) + + # New Course 2 + course_location_two = locator.CourseLocator('Org2', 'Course2', 'Run2') + new_course_two = self._create_course(course_location_two) + original_key = unicode(course.id) + new_key_one = unicode(new_course_one.id) + new_key_two = unicode(new_course_two.id) + + # Run the actual management command + transfer_students.Command().handle( + source_course=original_key, dest_course_list=new_key_one + "," + new_key_two + ) + + # Confirm the enrollment mode is verified on the new courses, and enrollment is enabled as appropriate. + self.assertEquals(('verified', False), CourseEnrollment.enrollment_mode_for_user(student, course.id)) + self.assertEquals(('verified', True), CourseEnrollment.enrollment_mode_for_user(student, new_course_one.id)) + self.assertEquals(('verified', True), CourseEnrollment.enrollment_mode_for_user(student, new_course_two.id)) + + def _create_course(self, course_location): + """ Creates a course """ + return CourseFactory.create( + org=course_location.org, + number=course_location.course, + run=course_location.run + ) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 13910c2fe2..783d0f3072 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -776,7 +776,7 @@ class CourseEnrollment(models.Model): is_course_full = cls.num_enrolled_in(course.id) >= course.max_student_enrollments_allowed return is_course_full - def update_enrollment(self, mode=None, is_active=None): + def update_enrollment(self, mode=None, is_active=None, emit_unenrollment_event=True): """ Updates an enrollment for a user in a class. This includes options like changing the mode, toggling is_active True/False, etc. @@ -784,6 +784,7 @@ class CourseEnrollment(models.Model): Also emits relevant events for analytics purposes. This saves immediately. + """ activation_changed = False # if is_active is None, then the call to update_enrollment didn't specify @@ -813,7 +814,7 @@ class CourseEnrollment(models.Model): u"mode:{}".format(self.mode)] ) - else: + elif emit_unenrollment_event: UNENROLL_DONE.send(sender=None, course_enrollment=self) self.emit_event(EVENT_NAME_ENROLLMENT_DEACTIVATED) @@ -987,7 +988,7 @@ class CourseEnrollment(models.Model): raise @classmethod - def unenroll(cls, user, course_id): + def unenroll(cls, user, course_id, emit_unenrollment_event=True): """ Remove the user from a given course. If the relevant `CourseEnrollment` object doesn't exist, we log an error but don't throw an exception. @@ -997,10 +998,12 @@ class CourseEnrollment(models.Model): adding an enrollment for it. `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall) + + `emit_unenrollment_events` can be set to False to suppress events firing. """ try: record = CourseEnrollment.objects.get(user=user, course_id=course_id) - record.update_enrollment(is_active=False) + record.update_enrollment(is_active=False, emit_unenrollment_event=emit_unenrollment_event) except cls.DoesNotExist: err_msg = u"Tried to unenroll student {} from {} but they were not enrolled" From d238fa1477da949fda525cc9ecab6ff219eb9c4b Mon Sep 17 00:00:00 2001 From: Brian Talbot Date: Mon, 20 Oct 2014 16:55:14 -0400 Subject: [PATCH 02/12] Studio: resolving a display issue for longer usernames in Studio's navigation --- cms/static/sass/elements/_header.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/static/sass/elements/_header.scss b/cms/static/sass/elements/_header.scss index 3bfafc7d28..afc4eabaaa 100644 --- a/cms/static/sass/elements/_header.scss +++ b/cms/static/sass/elements/_header.scss @@ -201,7 +201,7 @@ > .label { display: inline-block; - max-width: 85%; + max-width: 84%; overflow: hidden; white-space: nowrap; text-overflow: ellipsis; From c62bb614f3e589cc6f93cc98e96e3e70cf6bc5e8 Mon Sep 17 00:00:00 2001 From: Brian Talbot Date: Mon, 20 Oct 2014 16:57:01 -0400 Subject: [PATCH 03/12] LMS: resolving display issue with edx-footer's width --- lms/static/sass/shared/_footer.scss | 1 - 1 file changed, 1 deletion(-) diff --git a/lms/static/sass/shared/_footer.scss b/lms/static/sass/shared/_footer.scss index 1420370be9..9a6e4c37b2 100644 --- a/lms/static/sass/shared/_footer.scss +++ b/lms/static/sass/shared/_footer.scss @@ -285,7 +285,6 @@ $edx-footer-bg-color: rgb(252,252,252); } .edx-footer-new { - width: 100%; background: $edx-footer-bg-color; // NOTE: resetting older footer styles - can be removed once not needed From 19ab2b93b4e57fd7d9176b4f19f00c85861f4f85 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 21 Oct 2014 12:56:57 -0400 Subject: [PATCH 04/12] Fix unicode problem and also make display links a bit shorter wip remove unused kwarg --- lms/djangoapps/shoppingcart/models.py | 2 +- lms/templates/shoppingcart/receipt.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index 2b27dcc252..4e821e0858 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/lms/djangoapps/shoppingcart/models.py @@ -254,7 +254,7 @@ class Order(models.Model): for registration_code in registration_codes: redemption_url = reverse('register_code_redemption', args=[registration_code.code]) url = '{base_url}{redemption_url}'.format(base_url=site_name, redemption_url=redemption_url) - csv_writer.writerow([course.display_name, registration_code.code, url]) + csv_writer.writerow([unicode(course.display_name).encode("utf-8"), registration_code.code, url]) return csv_file, course_info diff --git a/lms/templates/shoppingcart/receipt.html b/lms/templates/shoppingcart/receipt.html index fac1ef230d..630a6fb76a 100644 --- a/lms/templates/shoppingcart/receipt.html +++ b/lms/templates/shoppingcart/receipt.html @@ -70,7 +70,7 @@ from courseware.courses import course_image_url, get_course_about_section, get_c ${registration_code.code} <% redemption_url = reverse('register_code_redemption', args = [registration_code.code] ) %> - <% enrollment_url = '{base_url}{redemption_url}'.format(base_url=site_name, redemption_url=redemption_url) %> + <% enrollment_url = '{redemption_url}'.format(redemption_url=redemption_url) %> ${enrollment_url} % endfor From f95ecea9a9106ecf11b13e3e127a45ccb5059256 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 21 Oct 2014 13:55:29 -0400 Subject: [PATCH 05/12] Temporary fix for runtime.resources_fs error --- lms/lib/xblock/runtime.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lms/lib/xblock/runtime.py b/lms/lib/xblock/runtime.py index f02657c511..6195048190 100644 --- a/lms/lib/xblock/runtime.py +++ b/lms/lib/xblock/runtime.py @@ -196,3 +196,11 @@ class LmsModuleSystem(LmsHandlerUrls, ModuleSystem): # pylint: disable=abstract ) services['fs'] = xblock.reference.plugins.FSService() super(LmsModuleSystem, self).__init__(**kwargs) + + # backward compatibility fix for callers not knowing this is a ModuleSystem v DescriptorSystem + @property + def resources_fs(self): + """ + Return what would be the resources_fs on a DescriptorSystem + """ + return getattr(self, 'filestore', None) From 1035d67a2558883cb0c6e494117f9ab8640dfb1c Mon Sep 17 00:00:00 2001 From: asadiqbal08 Date: Fri, 17 Oct 2014 19:42:20 +0500 Subject: [PATCH 06/12] WL-98 fix typo and add more security on API fix some bugs and typos address PR feedback be sure to send emails when accounts already exist PR feedback fix multiple uploads pep8 fixes pep8 fix pylint fixes fix url mapping WL-98 - Complete code coverage - Update code for error and warning messages. - improve code as per some suggestions updated the UI of the auto_enroll feature fixed the errors PR feedback add test add back file filtering add some more error handling of input remove unneeded coffeescript code pylint fixes add pep8 space WL-98 - Updated and added test cases. - Updated membership coffee file for errors display handling. - fixed minor text issues. allow for blank lines and add a test add blank line (pep8) --- lms/djangoapps/instructor/enrollment.py | 4 + lms/djangoapps/instructor/tests/test_api.py | 240 ++++++++++++++++++ lms/djangoapps/instructor/views/api.py | 190 +++++++++++++- lms/djangoapps/instructor/views/api_urls.py | 2 + .../instructor/views/instructor_dashboard.py | 1 + lms/envs/common.py | 5 + .../instructor_dashboard/membership.coffee | 113 ++++++++- lms/static/sass/base/_variables.scss | 1 + .../sass/course/instructor/_instructor_2.scss | 46 ++++ ...count_creation_and_enroll_emailMessage.txt | 14 + .../instructor_dashboard_2/membership.html | 26 ++ 11 files changed, 632 insertions(+), 10 deletions(-) create mode 100644 lms/templates/emails/account_creation_and_enroll_emailMessage.txt diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index 7e6ac0f2f1..f0198e92a5 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -331,6 +331,10 @@ def send_mail_to_student(student, param_dict): 'emails/remove_beta_tester_email_subject.txt', 'emails/remove_beta_tester_email_message.txt' ), + 'account_creation_and_enrollment': ( + 'emails/enroll_email_enrolledsubject.txt', + 'emails/account_creation_and_enroll_emailMessage.txt' + ), } subject_template, message_template = email_template_dict.get(message_type, (None, None)) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 94c980fa5a..3c529ec077 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -9,6 +9,7 @@ import requests import datetime import ddt import random +import io from urllib import quote from django.test import TestCase from nose.tools import raises @@ -40,6 +41,7 @@ from courseware.models import StudentModule # modules which are mocked in test cases. import instructor_task.api import instructor.views.api +from instructor.views.api import generate_unique_password from instructor.views.api import _split_input_list, common_exceptions_400 from instructor_task.api_helper import AlreadyRunningError from opaque_keys.edx.locations import SlashSeparatedCourseKey @@ -48,6 +50,8 @@ from shoppingcart.models import ( PaidCourseRegistration, Coupon, Invoice, CourseRegistrationCode ) from course_modes.models import CourseMode +from django.core.files.uploadedfile import SimpleUploadedFile +from student.models import NonExistentCourseError from .test_tools import msk_from_problem_urlname from ..views.tools import get_extended_due @@ -285,6 +289,242 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase): ) +@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +@patch.dict(settings.FEATURES, {'ALLOW_AUTOMATED_SIGNUPS': True}) +class TestInstructorAPIBulkAccountCreationAndEnrollment(ModuleStoreTestCase, LoginEnrollmentTestCase): + """ + Test Bulk account creation and enrollment from csv file + """ + def setUp(self): + self.request = RequestFactory().request() + self.course = CourseFactory.create() + self.instructor = InstructorFactory(course_key=self.course.id) + self.client.login(username=self.instructor.username, password='test') + self.url = reverse('register_and_enroll_students', kwargs={'course_id': self.course.id.to_deprecated_string()}) + + self.not_enrolled_student = UserFactory( + username='NotEnrolledStudent', + email='nonenrolled@test.com', + first_name='NotEnrolled', + last_name='Student' + ) + + @patch('instructor.views.api.log.info') + def test_account_creation_and_enrollment_with_csv(self, info_log): + """ + Happy path test to create a single new user + """ + csv_content = "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}) + 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) + + # test the log for email that's send to new created user. + info_log.assert_called_with('email sent to new created user at test_student@example.com') + + @patch('instructor.views.api.log.info') + def test_account_creation_and_enrollment_with_csv_with_blank_lines(self, info_log): + """ + Happy path test to create a single new user + """ + csv_content = "\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}) + 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) + + # test the log for email that's send to new created user. + info_log.assert_called_with('email sent to new created user at test_student@example.com') + + @patch('instructor.views.api.log.info') + def test_email_and_username_already_exist(self, info_log): + """ + If the email address and username already exists + and the user is enrolled in the course, do nothing (including no email gets sent out) + """ + csv_content = "test_student@example.com,test_student_1,tester1,USA\n" \ + "test_student@example.com,test_student_1,tester2,US" + uploaded_file = SimpleUploadedFile("temp.csv", csv_content) + response = self.client.post(self.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) + + # test the log for email that's send to new created user. + info_log.assert_called_with("user already exists with username '{username}' and email '{email}'".format(username='test_student_1', email='test_student@example.com')) + + def test_bad_file_upload_type(self): + """ + Try uploading some non-CSV file and verify that it is rejected + """ + uploaded_file = SimpleUploadedFile("temp.jpg", io.BytesIO(b"some initial binary data: \x00\x01").read()) + response = self.client.post(self.url, {'students_list': uploaded_file}) + self.assertEqual(response.status_code, 200) + data = json.loads(response.content) + self.assertNotEquals(len(data['general_errors']), 0) + self.assertEquals(data['general_errors'][0]['response'], 'Could not read uploaded file.') + + def test_insufficient_data(self): + """ + Try uploading a CSV file which does not have the exact four columns of data + """ + csv_content = "test_student@example.com,test_student_1\n" + uploaded_file = SimpleUploadedFile("temp.csv", csv_content) + response = self.client.post(self.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']), 1) + self.assertEquals(data['general_errors'][0]['response'], 'Data in row #1 must have exactly four columns: email, username, full name, and country') + + def test_invalid_email_in_csv(self): + """ + Test failure case of a poorly formatted email field + """ + csv_content = "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}) + data = json.loads(response.content) + self.assertEqual(response.status_code, 200) + self.assertNotEquals(len(data['row_errors']), 0) + self.assertEquals(len(data['warnings']), 0) + self.assertEquals(len(data['general_errors']), 0) + self.assertEquals(data['row_errors'][0]['response'], 'Invalid email {0}.'.format('test_student.example.com')) + + @patch('instructor.views.api.log.info') + def test_csv_user_exist_and_not_enrolled(self, info_log): + """ + If the email address and username already exists + and the user is not enrolled in the course, enrolled him/her and iterate to next one. + """ + csv_content = "nonenrolled@test.com,NotEnrolledStudent,tester1,USA" + + uploaded_file = SimpleUploadedFile("temp.csv", csv_content) + response = self.client.post(self.url, {'students_list': uploaded_file}) + self.assertEqual(response.status_code, 200) + info_log.assert_called_with('user {username} enrolled in the course {course}'.format(username='NotEnrolledStudent', course=self.course.id)) + + def test_user_with_already_existing_email_in_csv(self): + """ + If the email address already exists, but the username is different, + assume it is the correct user and just register the user in the course. + """ + csv_content = "test_student@example.com,test_student_1,tester1,USA\n" \ + "test_student@example.com,test_student_2,tester2,US" + + uploaded_file = SimpleUploadedFile("temp.csv", csv_content) + response = self.client.post(self.url, {'students_list': uploaded_file}) + self.assertEqual(response.status_code, 200) + data = json.loads(response.content) + warning_message = 'An account with email {email} exists but the provided username {username} ' \ + 'is different. Enrolling anyway with {email}.'.format(email='test_student@example.com', username='test_student_2') + self.assertNotEquals(len(data['warnings']), 0) + self.assertEquals(data['warnings'][0]['response'], warning_message) + user = User.objects.get(email='test_student@example.com') + self.assertTrue(CourseEnrollment.is_enrolled(user, self.course.id)) + + def test_user_with_already_existing_username_in_csv(self): + """ + If the username already exists (but not the email), + assume it is a different user and fail to create the new account. + """ + csv_content = "test_student1@example.com,test_student_1,tester1,USA\n" \ + "test_student2@example.com,test_student_1,tester2,US" + + uploaded_file = SimpleUploadedFile("temp.csv", csv_content) + + response = self.client.post(self.url, {'students_list': uploaded_file}) + self.assertEqual(response.status_code, 200) + data = json.loads(response.content) + self.assertNotEquals(len(data['row_errors']), 0) + self.assertEquals(data['row_errors'][0]['response'], 'Username {user} already exists.'.format(user='test_student_1')) + + def test_csv_file_not_attached(self): + """ + Test when the user does not attach a file + """ + csv_content = "test_student1@example.com,test_student_1,tester1,USA\n" \ + "test_student2@example.com,test_student_1,tester2,US" + + uploaded_file = SimpleUploadedFile("temp.csv", csv_content) + + response = self.client.post(self.url, {'file_not_found': uploaded_file}) + self.assertEqual(response.status_code, 200) + data = json.loads(response.content) + self.assertNotEquals(len(data['general_errors']), 0) + self.assertEquals(data['general_errors'][0]['response'], 'File is not attached.') + + def test_raising_exception_in_auto_registration_and_enrollment_case(self): + """ + Test that exceptions are handled well + """ + csv_content = "test_student1@example.com,test_student_1,tester1,USA\n" \ + "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: + mock.side_effect = NonExistentCourseError() + response = self.client.post(self.url, {'students_list': uploaded_file}) + + self.assertEqual(response.status_code, 200) + data = json.loads(response.content) + self.assertNotEquals(len(data['row_errors']), 0) + self.assertEquals(data['row_errors'][0]['response'], 'NonExistentCourseError') + + def test_generate_unique_password(self): + """ + generate_unique_password should generate a unique password string that excludes certain characters. + """ + password = generate_unique_password([], 12) + self.assertEquals(len(password), 12) + for letter in password: + self.assertNotIn(letter, 'aAeEiIoOuU1l') + + def test_users_created_and_enrolled_successfully_if_others_fail(self): + + csv_content = "test_student1@example.com,test_student_1,tester1,USA\n" \ + "test_student3@example.com,test_student_1,tester3,CA\n" \ + "test_student2@example.com,test_student_2,tester2,USA" + + uploaded_file = SimpleUploadedFile("temp.csv", csv_content) + response = self.client.post(self.url, {'students_list': uploaded_file}) + self.assertEqual(response.status_code, 200) + data = json.loads(response.content) + self.assertNotEquals(len(data['row_errors']), 0) + self.assertEquals(data['row_errors'][0]['response'], 'Username {user} already exists.'.format(user='test_student_1')) + self.assertTrue(User.objects.filter(username='test_student_1', email='test_student1@example.com').exists()) + self.assertTrue(User.objects.filter(username='test_student_2', email='test_student2@example.com').exists()) + self.assertFalse(User.objects.filter(email='test_student3@example.com').exists()) + + @patch.object(instructor.views.api, 'generate_random_string', + Mock(side_effect=['first', 'first', 'second'])) + def test_generate_unique_password_no_reuse(self): + """ + generate_unique_password should generate a unique password string that hasn't been generated before. + """ + generated_password = ['first'] + password = generate_unique_password(generated_password, 12) + self.assertNotEquals(password, 'first') + + @patch.dict(settings.FEATURES, {'ALLOW_AUTOMATED_SIGNUPS': False}) + def test_allow_automated_signups_flag_not_set(self): + csv_content = "test_student1@example.com,test_student_1,tester1,USA" + uploaded_file = SimpleUploadedFile("temp.csv", csv_content) + response = self.client.post(self.url, {'students_list': uploaded_file}) + self.assertEquals(response.status_code, 403) + + @ddt.ddt @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) class TestInstructorAPIEnrollment(ModuleStoreTestCase, LoginEnrollmentTestCase): diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 7b92a13516..3d5bb7a566 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -50,6 +50,7 @@ from instructor_task.models import ReportStore import instructor.enrollment as enrollment from instructor.enrollment import ( enroll_email, + send_mail_to_student, get_email_params, send_beta_role_email, unenroll_email @@ -83,6 +84,7 @@ from .tools import ( from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locations import SlashSeparatedCourseKey from opaque_keys import InvalidKeyError +from student.models import UserProfile, Registration log = logging.getLogger(__name__) @@ -216,6 +218,187 @@ def require_level(level): return decorator +EMAIL_INDEX = 0 +USERNAME_INDEX = 1 +NAME_INDEX = 2 +COUNTRY_INDEX = 3 + + +@ensure_csrf_cookie +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_level('staff') +def register_and_enroll_students(request, course_id): # pylint: disable=R0915 + """ + 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. + Requires staff access. + + -If the email address and username already exists and the user is enrolled in the course, + do nothing (including no email gets sent out) + + -If the email address already exists, but the username is different, + match on the email address only and continue to enroll the user in the course using the email address + as the matching criteria. Note the change of username as a warning message (but not a failure). Send a standard enrollment email + which is the same as the existing manual enrollment + + -If the username already exists (but not the email), assume it is a different user and fail to create the new account. + The failure will be messaged in a response in the browser. + """ + + if not microsite.get_value('ALLOW_AUTOMATED_SIGNUPS', settings.FEATURES.get('ALLOW_AUTOMATED_SIGNUPS', False)): + return HttpResponseForbidden() + + course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id) + warnings = [] + row_errors = [] + general_errors = [] + + if 'students_list' in request.FILES: + students = [] + + try: + upload_file = request.FILES.get('students_list') + students = [row for row in csv.reader(upload_file.read().splitlines())] + except Exception: # pylint: disable=W0703 + general_errors.append({ + 'username': '', 'email': '', 'response': _('Could not read uploaded file.') + }) + finally: + upload_file.close() + + generated_passwords = [] + course = get_course_by_id(course_id) + row_num = 0 + for student in students: + row_num = row_num + 1 + + # verify that we have exactly four columns in every row but allow for blank lines + if len(student) != 4: + if len(student) > 0: + general_errors.append({ + 'username': '', + 'email': '', + 'response': _('Data in row #{row_num} must have exactly four columns: email, username, full name, and country').format(row_num=row_num) + }) + 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] + + email_params = get_email_params(course, True, secure=request.is_secure()) + try: + validate_email(email) # Raises ValidationError if invalid + except ValidationError: + row_errors.append({ + 'username': username, 'email': email, 'response': _('Invalid email {email_address}.').format(email_address=email)}) + else: + if User.objects.filter(email=email).exists(): + # Email address already exists. assume it is the correct user + # and just register the user in the course and send an enrollment email. + user = User.objects.get(email=email) + + # see if it is an exact match with email and username + # if it's not an exact match then just display a warning message, but continue onwards + if not User.objects.filter(email=email, username=username).exists(): + warning_message = _( + 'An account with email {email} exists but the provided username {username} ' + 'is different. Enrolling anyway with {email}.' + ).format(email=email, username=username) + + warnings.append({ + 'username': username, 'email': email, 'response': warning_message}) + log.warning('email {email} already exist'.format(email=email)) + else: + log.info("user already exists with username '{username}' and email '{email}'".format(email=email, username=username)) + + # make sure user is enrolled in course + if not CourseEnrollment.is_enrolled(user, course_id): + CourseEnrollment.enroll(user, course_id) + log.info('user {username} enrolled in the course {course}'.format(username=username, course=course.id)) + enroll_email(course_id=course_id, student_email=email, auto_enroll=True, email_students=True, email_params=email_params) + else: + # This email does not yet exist, so we need to create a new account + # If username already exists in the database, then create_and_enroll_user + # will raise an IntegrityError exception. + password = generate_unique_password(generated_passwords) + + try: + create_and_enroll_user(email, username, name, country, password, course_id) + 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('email sent to new created user at {email}'.format(email=email)) + + else: + general_errors.append({ + 'username': '', 'email': '', 'response': _('File is not attached.') + }) + + results = { + 'row_errors': row_errors, + 'general_errors': general_errors, + 'warnings': warnings + } + return JsonResponse(results) + + +def generate_random_string(length): + """ + Create a string of random characters of specified length + """ + chars = [ + char for char in string.ascii_uppercase + string.digits + string.ascii_lowercase + if char not in 'aAeEiIoOuU1l' + ] + + return string.join((random.choice(chars) for __ in range(length)), '') + + +def generate_unique_password(generated_passwords, password_length=12): + """ + generate a unique password for each student. + """ + + password = generate_random_string(password_length) + while password in generated_passwords: + password = generate_random_string(password_length) + + generated_passwords.append(password) + + return password + + +def create_and_enroll_user(email, username, name, country, password, course_id): + """ Creates a user and enroll him/her in the course""" + + user = User.objects.create_user(username, email, password) + reg = Registration() + reg.register(user) + + profile = UserProfile(user=user) + profile.name = name + profile.country = country + profile.save() + + # try to enroll the user in this course + CourseEnrollment.enroll(user, course_id) + + @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @require_level('staff') @@ -852,13 +1035,8 @@ def random_code_generator(): generate a random alphanumeric code of length defined in REGISTRATION_CODE_LENGTH settings """ - chars = '' - for char in string.ascii_uppercase + string.digits + string.ascii_lowercase: - # removing vowel words and specific characters - chars += char.strip('aAeEiIoOuU1l') - code_length = getattr(settings, 'REGISTRATION_CODE_LENGTH', 8) - return string.join((random.choice(chars) for _ in range(code_length)), '') + return generate_random_string(code_length) @ensure_csrf_cookie diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index 4968f1b104..d7abd9269d 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -7,6 +7,8 @@ from django.conf.urls import patterns, url urlpatterns = patterns('', # nopep8 url(r'^students_update_enrollment$', 'instructor.views.api.students_update_enrollment', name="students_update_enrollment"), + url(r'^register_and_enroll_students$', + 'instructor.views.api.register_and_enroll_students', name="register_and_enroll_students"), url(r'^list_course_role_members$', 'instructor.views.api.list_course_role_members', name="list_course_role_members"), url(r'^modify_access$', diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 12e01a32c3..cdcf2b4e31 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -250,6 +250,7 @@ def _section_membership(course, access): 'access': access, 'enroll_button_url': reverse('students_update_enrollment', kwargs={'course_id': course_key.to_deprecated_string()}), 'unenroll_button_url': reverse('students_update_enrollment', kwargs={'course_id': course_key.to_deprecated_string()}), + 'upload_student_csv_button_url': reverse('register_and_enroll_students', kwargs={'course_id': course_key.to_deprecated_string()}), 'modify_beta_testers_button_url': reverse('bulk_beta_modify_access', kwargs={'course_id': course_key.to_deprecated_string()}), 'list_course_role_members_url': reverse('list_course_role_members', kwargs={'course_id': course_key.to_deprecated_string()}), 'modify_access_url': reverse('modify_access', kwargs={'course_id': course_key.to_deprecated_string()}), diff --git a/lms/envs/common.py b/lms/envs/common.py index c57ff34565..5569e6223f 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -287,6 +287,11 @@ FEATURES = { # Enable the new dashboard, account, and profile pages 'ENABLE_NEW_DASHBOARD': False, + + # Show a section in the membership tab of the instructor dashboard + # to allow an upload of a CSV file that contains a list of new accounts to create + # and register for course. + 'ALLOW_AUTOMATED_SIGNUPS': False, } # Ignore static asset files on import which match this pattern diff --git a/lms/static/coffee/src/instructor_dashboard/membership.coffee b/lms/static/coffee/src/instructor_dashboard/membership.coffee index 744c2b225e..132ae7e1d0 100644 --- a/lms/static/coffee/src/instructor_dashboard/membership.coffee +++ b/lms/static/coffee/src/instructor_dashboard/membership.coffee @@ -141,7 +141,7 @@ class AuthListWidget extends MemberListWidget url: @list_endpoint data: rolename: @rolename success: (data) => cb? null, data[@rolename] - error: std_ajax_err => + error: std_ajax_err => `// Translators: A rolename appears this sentence. A rolename is something like "staff" or "beta tester".` cb? gettext("Error fetching list for role") + " '#{@rolename}'" @@ -174,6 +174,108 @@ class AuthListWidget extends MemberListWidget else @reload_list() +class AutoEnrollmentViaCsv + constructor: (@$container) -> + # Wrapper for the AutoEnrollmentViaCsv subsection. + # This object handles buttons, success and failure reporting, + # and server communication. + @$student_enrollment_form = @$container.find("form#student-auto-enroll-form") + @$enrollment_signup_button = @$container.find("[name='enrollment_signup_button']") + @$students_list_file = @$container.find("input[name='students_list']") + @$csrf_token = @$container.find("input[name='csrfmiddlewaretoken']") + @$results = @$container.find("div.results") + @$browse_button = @$container.find("#browseBtn") + @$browse_file = @$container.find("#browseFile") + + @processing = false + + @$browse_button.on "change", (event) => + if event.currentTarget.files.length == 1 + @$browse_file.val(event.currentTarget.value.substring(event.currentTarget.value.lastIndexOf("\\") + 1)) + + # attach click handler for @$enrollment_signup_button + @$enrollment_signup_button.click => + @$student_enrollment_form.submit (event) => + if @processing + return false + + @processing = true + + event.preventDefault() + data = new FormData(event.currentTarget) + $.ajax + dataType: 'json' + type: 'POST' + url: event.currentTarget.action + data: data + processData: false + contentType: false + success: (data) => + @processing = false + @display_response data + + return false + + display_response: (data_from_server) -> + @$results.empty() + errors = [] + warnings = [] + + result_from_server_is_success = true + + if data_from_server.general_errors.length + result_from_server_is_success = false + for general_error in data_from_server.general_errors + general_error['is_general_error'] = true + errors.push general_error + + if data_from_server.row_errors.length + result_from_server_is_success = false + for error in data_from_server.row_errors + error['is_general_error'] = false + errors.push error + + if data_from_server.warnings.length + result_from_server_is_success = false + for warning in data_from_server.warnings + warning['is_general_error'] = false + warnings.push warning + + render_response = (label, type, student_results) => + if type is 'success' + task_res_section = $ '
', class: 'message message-confirmation' + message_title = $ '

', class: 'message-title', text: label + task_res_section.append message_title + @$results.append task_res_section + return + + if type is 'error' + task_res_section = $ '
', class: 'message message-error' + if type is 'warning' + task_res_section = $ '
', class: 'message message-warning' + + message_title = $ '

', class: 'message-title', text: label + task_res_section. append message_title + messages_copy = $ '
', class: 'message-copy' + task_res_section. append messages_copy + messages_summary = $ '
    ', class: 'list-summary summary-items' + messages_copy.append messages_summary + + for student_result in student_results + if student_result.is_general_error + response_message = student_result.response + else + response_message = student_result.username + ' ('+ student_result.email + '): ' + ' (' + student_result.response + ')' + messages_summary.append $ '
  • ', class: 'summary-item', text: response_message + + @$results.append task_res_section + + if errors.length + render_response gettext("The following errors were generated:"), 'error', errors + if warnings.length + render_response gettext("The following warnings were generated:"), 'warning', warnings + if result_from_server_is_success + render_response gettext("All accounts were created successfully."), 'success', [] class BetaTesterBulkAddition constructor: (@$container) -> @@ -189,7 +291,7 @@ class BetaTesterBulkAddition @$btn_beta_testers.click (event) => emailStudents = @$checkbox_emailstudents.is(':checked') autoEnroll = @$checkbox_autoenroll.is(':checked') - send_data = + send_data = action: $(event.target).data('action') # 'add' or 'remove' identifiers: @$identifier_input.val() email_students: emailStudents @@ -580,7 +682,10 @@ class Membership # isolate # initialize BatchEnrollment subsection plantTimeout 0, => new BatchEnrollment @$section.find '.batch-enrollment' - + + # isolate # initialize AutoEnrollmentViaCsv subsection + plantTimeout 0, => new AutoEnrollmentViaCsv @$section.find '.auto_enroll_csv' + # initialize BetaTesterBulkAddition subsection plantTimeout 0, => new BetaTesterBulkAddition @$section.find '.batch-beta-testers' @@ -626,4 +731,4 @@ class Membership _.defaults window, InstructorDashboard: {} _.defaults window.InstructorDashboard, sections: {} _.defaults window.InstructorDashboard.sections, - Membership: Membership + Membership: Membership \ No newline at end of file diff --git a/lms/static/sass/base/_variables.scss b/lms/static/sass/base/_variables.scss index 91cb01dfdf..4a149a134b 100644 --- a/lms/static/sass/base/_variables.scss +++ b/lms/static/sass/base/_variables.scss @@ -143,6 +143,7 @@ $light-gray: #ddd; // used by descriptor css $lightGrey: #edf1f5; $darkGrey: #8891a1; +$lightGrey1: #ccc; $blue-d1: shade($blue,20%); $blue-d2: shade($blue,40%); $blue-d4: shade($blue,80%); diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss index 8aebd745be..92965a18aa 100644 --- a/lms/static/sass/course/instructor/_instructor_2.scss +++ b/lms/static/sass/course/instructor/_instructor_2.scss @@ -684,6 +684,52 @@ section.instructor-dashboard-content-2 { } } } + // Auto Enroll Csv Section + .auto_enroll_csv { + .results { + + } + .enrollment_signup_button { + margin-right: ($baseline/4); + } + // Custom File upload + .customBrowseBtn { + margin: ($baseline/2) 0; + display: inline-block; + .file-browse { + position:relative; + overflow:hidden; + display: inline; + margin-left: -5px; + span.browse{ + @include button(simple, $blue); + padding: 6px ($baseline/2); + font-size: 12px; + border-radius: 0 3px 3px 0; + margin-right: ($baseline); + } + input.file_field { + position:absolute; + top:0; + right:0; + margin:0; + padding:0; + cursor:pointer; + opacity:0; + filter:alpha(opacity=0); + } + } + & > span, & input[disabled]{ + vertical-align: middle; + } + input[disabled] { + cursor: not-allowed; + border: 1px solid $lightGrey1; + border-radius: 4px 0 0 4px; + padding: 6px 6px 5px; + } + } + } .enroll-option { margin: ($baseline/2) 0; diff --git a/lms/templates/emails/account_creation_and_enroll_emailMessage.txt b/lms/templates/emails/account_creation_and_enroll_emailMessage.txt new file mode 100644 index 0000000000..c9930d4db6 --- /dev/null +++ b/lms/templates/emails/account_creation_and_enroll_emailMessage.txt @@ -0,0 +1,14 @@ +<%! from django.utils.translation import ugettext as _ %> + +${_("Welcome to {course_name}").format(course_name=course.display_name_with_default)} + +${_("To get started, please visit https://{site_name}. The login information for your account follows.").format(site_name=site_name)} + +${_("email: {email}").format(email=email_address)} +${_("password: {password}").format(password=password)} + +${_("It is recommended that you change your password.")} + +${_("Sincerely yours,")} + +${_("The {course_name} Team").format(course_name=course.display_name_with_default)} diff --git a/lms/templates/instructor/instructor_dashboard_2/membership.html b/lms/templates/instructor/instructor_dashboard_2/membership.html index 6a91820f3e..905ad0fe87 100644 --- a/lms/templates/instructor/instructor_dashboard_2/membership.html +++ b/lms/templates/instructor/instructor_dashboard_2/membership.html @@ -1,5 +1,6 @@ <%! from django.utils.translation import ugettext as _ %> <%page args="section_data"/> +<%! from microsite_configuration import microsite %> + % endif From cb6f96603b50491ac2a67c853a49dfd55c1aa749 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 22 Oct 2014 00:37:46 -0400 Subject: [PATCH 07/12] only multi-seat emails is html --- lms/djangoapps/shoppingcart/models.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index 4e821e0858..4864d930f9 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/lms/djangoapps/shoppingcart/models.py @@ -312,7 +312,12 @@ class Order(models.Model): from_email=from_address, to=[recipient[1]] ) - email.content_subtype = "html" + + # only the business order is HTML formatted + # the single seat is simple text + if is_order_type_business: + email.content_subtype = "html" + if csv_file: email.attach(u'RegistrationCodesRedemptionUrls.csv', csv_file.getvalue(), 'text/csv') email.send() From b0802aafc877775c716650d4ca8c3bf15ad3e12e Mon Sep 17 00:00:00 2001 From: Muhammad Shoaib Date: Wed, 22 Oct 2014 16:18:50 +0500 Subject: [PATCH 08/12] =?UTF-8?q?css=20fix=20of=20remove=20icon=20and=20Fi?= =?UTF-8?q?xed=20a=20bug=20where=20the=20discount=20coupon=20applied=20?= =?UTF-8?q?=E2=80=98disappeared=E2=80=99=20=20when=20changing=20the=20quan?= =?UTF-8?q?tity=20between=20enrollment=20vs=20reg=20code=20cart=20types.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lms/djangoapps/shoppingcart/models.py | 13 ++++++++++--- lms/static/sass/views/_shoppingcart.scss | 3 +++ lms/templates/shoppingcart/shopping_cart.html | 2 +- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index 4864d930f9..84c6a096fc 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/lms/djangoapps/shoppingcart/models.py @@ -223,14 +223,21 @@ class Order(models.Model): if is_order_type_business: for cart_item in cart_items: if hasattr(cart_item, 'paidcourseregistration'): - CourseRegCodeItem.add_to_order(self, cart_item.paidcourseregistration.course_id, cart_item.qty) + course_reg_code_item = CourseRegCodeItem.add_to_order(self, cart_item.paidcourseregistration.course_id, cart_item.qty) + # update the discounted prices if coupon redemption applied + course_reg_code_item.list_price = cart_item.list_price + course_reg_code_item.unit_cost = cart_item.unit_cost + course_reg_code_item.save() items_to_delete.append(cart_item) else: for cart_item in cart_items: if hasattr(cart_item, 'courseregcodeitem'): - PaidCourseRegistration.add_to_order(self, cart_item.courseregcodeitem.course_id) + paid_course_registration = PaidCourseRegistration.add_to_order(self, cart_item.courseregcodeitem.course_id) + # update the discounted prices if coupon redemption applied + paid_course_registration.list_price = cart_item.list_price + paid_course_registration.unit_cost = cart_item.unit_cost + paid_course_registration.save() items_to_delete.append(cart_item) - # CourseRegCodeItem.add_to_order for item in items_to_delete: item.delete() diff --git a/lms/static/sass/views/_shoppingcart.scss b/lms/static/sass/views/_shoppingcart.scss index 65dcd8c00d..b4d8eaea4f 100644 --- a/lms/static/sass/views/_shoppingcart.scss +++ b/lms/static/sass/views/_shoppingcart.scss @@ -479,6 +479,9 @@ pointer-events: none; } } + .no-width { + width: 0px !important; + } .col-3{ width: 100px; float: right; diff --git a/lms/templates/shoppingcart/shopping_cart.html b/lms/templates/shoppingcart/shopping_cart.html index 0bd92f3882..449a320731 100644 --- a/lms/templates/shoppingcart/shopping_cart.html +++ b/lms/templates/shoppingcart/shopping_cart.html @@ -58,7 +58,7 @@ from django.utils.translation import ugettext as _

-
+
From d5966b1e21621c047445e26523e44c17d9ae1b88 Mon Sep 17 00:00:00 2001 From: stephensanchez Date: Wed, 22 Oct 2014 18:20:32 +0000 Subject: [PATCH 09/12] Adding an additional check to avoid donations showing up for microsites. --- common/djangoapps/student/views.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 94a81590e4..682b0bf005 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -697,7 +697,10 @@ def _allow_donation(course_modes, course_id): True if the course is allowing donations. """ - return DonationConfiguration.current().enabled and not CourseMode.has_verified_mode(course_modes[course_id]) + donations_enabled = DonationConfiguration.current().enabled + is_verified_mode = CourseMode.has_verified_mode(course_modes[course_id]) + is_microsite = microsite.is_request_in_microsite() + return donations_enabled and not is_verified_mode and not is_microsite def try_change_enrollment(request): From f463aca922e742a44cc5507fe6f6067fbefda646 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 22 Oct 2014 15:09:10 -0400 Subject: [PATCH 10/12] Dont use site_name, use platform_name in the shopping cart flow template --- lms/templates/shoppingcart/shopping_cart_flow.html | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lms/templates/shoppingcart/shopping_cart_flow.html b/lms/templates/shoppingcart/shopping_cart_flow.html index bc7fa47317..298a8e6f11 100644 --- a/lms/templates/shoppingcart/shopping_cart_flow.html +++ b/lms/templates/shoppingcart/shopping_cart_flow.html @@ -5,13 +5,14 @@ from django.utils.translation import ugettext as _ <%namespace name='static' file='/static_content.html'/> <%block name="pagetitle">${_("Shopping cart")} - +<%! from django.conf import settings %> +<%! from microsite_configuration import microsite %> <%block name="bodyextra">
-

${_("{site_name} - Shopping Cart").format(site_name=site_name)}

+

${_("{platform_name} - Shopping Cart").format(platform_name=microsite.get_value('platform_name', settings.PLATFORM_NAME))}

% if shoppingcart_items:
  • >${_('Review')}
    • From de4a2191c8e5dea26a5dea94c7d4e0914de31f8d Mon Sep 17 00:00:00 2001 From: stephensanchez Date: Wed, 22 Oct 2014 19:53:59 +0000 Subject: [PATCH 11/12] pep8 error. --- common/djangoapps/student/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 682b0bf005..4c941818fa 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -700,7 +700,7 @@ def _allow_donation(course_modes, course_id): donations_enabled = DonationConfiguration.current().enabled is_verified_mode = CourseMode.has_verified_mode(course_modes[course_id]) is_microsite = microsite.is_request_in_microsite() - return donations_enabled and not is_verified_mode and not is_microsite + return donations_enabled and not is_verified_mode and not is_microsite def try_change_enrollment(request): From 128d4836c042aa8131b9cdda2a56f3b003f0596a Mon Sep 17 00:00:00 2001 From: stephensanchez Date: Wed, 22 Oct 2014 20:26:03 +0000 Subject: [PATCH 12/12] Change design to check for min price or suggested prices. --- common/djangoapps/course_modes/models.py | 19 +++++++++++++++++++ .../course_modes/tests/test_models.py | 19 +++++++++++++++++++ common/djangoapps/student/views.py | 4 ++-- 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/course_modes/models.py b/common/djangoapps/course_modes/models.py index 072f8e79a0..94466bf20d 100644 --- a/common/djangoapps/course_modes/models.py +++ b/common/djangoapps/course_modes/models.py @@ -163,6 +163,25 @@ class CourseMode(models.Model): return mode.min_price return 0 + @classmethod + def has_payment_options(cls, course_id): + """Determines if there is any mode that has payment options + + Check the dict of course modes and see if any of them have a minimum price or + suggested prices. Returns True if any course mode has a payment option. + + Args: + course_mode_dict (dict): Dictionary mapping course mode slugs to Modes + + Returns: + True if any course mode has a payment option. + + """ + for mode in cls.modes_for_course(course_id): + if mode.min_price > 0 or mode.suggested_prices != '': + return True + return False + @classmethod def min_course_price_for_currency(cls, course_id, currency): """ diff --git a/common/djangoapps/course_modes/tests/test_models.py b/common/djangoapps/course_modes/tests/test_models.py index c369aaaf0f..88571d449d 100644 --- a/common/djangoapps/course_modes/tests/test_models.py +++ b/common/djangoapps/course_modes/tests/test_models.py @@ -127,3 +127,22 @@ class CourseModeModelTest(TestCase): mode = CourseMode.verified_mode_for_course(self.course_key) self.assertEqual(mode.slug, 'professional') + + def test_course_has_payment_options(self): + # Has no payment options. + honor, _ = self.create_mode('honor', 'Honor') + self.assertFalse(CourseMode.has_payment_options(self.course_key)) + + # Now we do have a payment option. + verified, _ = self.create_mode('verified', 'Verified', min_price=5) + self.assertTrue(CourseMode.has_payment_options(self.course_key)) + + # Unset verified's minimum price. + verified.min_price = 0 + verified.save() + self.assertFalse(CourseMode.has_payment_options(self.course_key)) + + # Finally, give the honor mode payment options + honor.suggested_prices = '5, 10, 15' + honor.save() + self.assertTrue(CourseMode.has_payment_options(self.course_key)) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 4c941818fa..eb23c1f876 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -699,8 +699,8 @@ def _allow_donation(course_modes, course_id): """ donations_enabled = DonationConfiguration.current().enabled is_verified_mode = CourseMode.has_verified_mode(course_modes[course_id]) - is_microsite = microsite.is_request_in_microsite() - return donations_enabled and not is_verified_mode and not is_microsite + has_payment_option = CourseMode.has_payment_options(course_id) + return donations_enabled and not is_verified_mode and not has_payment_option def try_change_enrollment(request):