diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a0f53af981..06fa45818e 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -11,6 +11,8 @@ LMS: Add support for user partitioning based on cohort. TNL-710 Platform: Add base support for cohorted group configurations. TNL-649 +LMS: Support assigning students to cohorts via a CSV file upload. TNL-735 + Common: Add configurable reset button to units Studio: Add support xblock validation messages on Studio unit/container page. TNL-683 diff --git a/common/djangoapps/student/management/commands/create_random_users.py b/common/djangoapps/student/management/commands/create_random_users.py index 5ad55db7be..088d4484ab 100644 --- a/common/djangoapps/student/management/commands/create_random_users.py +++ b/common/djangoapps/student/management/commands/create_random_users.py @@ -1,12 +1,27 @@ """ A script to create some dummy users """ +import uuid + from django.core.management.base import BaseCommand from student.models import CourseEnrollment from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locations import SlashSeparatedCourseKey -from student.views import _do_create_account, get_random_post_override +from student.views import _do_create_account + + +def get_random_post_override(): + """ + Generate unique user data for dummy users. + """ + identification = uuid.uuid4().hex[:8] + return { + 'username': 'user_{id}'.format(id=identification), + 'email': 'email_{id}@example.com'.format(id=identification), + 'password': '12345', + 'name': 'User {id}'.format(id=identification), + } def create(num, course_key): diff --git a/common/djangoapps/util/file.py b/common/djangoapps/util/file.py new file mode 100644 index 0000000000..25d2bc9b63 --- /dev/null +++ b/common/djangoapps/util/file.py @@ -0,0 +1,168 @@ +""" +Utility methods related to file handling. +""" + +from datetime import datetime +import os +from pytz import UTC + +from django.core.exceptions import PermissionDenied +from django.core.files.storage import DefaultStorage, get_valid_filename +from django.utils.translation import ugettext as _ +from django.utils.translation import ungettext + + +class FileValidationException(Exception): + """ + An exception thrown during file validation. + """ + pass + + +def store_uploaded_file( + request, file_key, allowed_file_types, base_storage_filename, max_file_size, validator=None, +): + """ + Stores an uploaded file to django file storage. + + Args: + request (HttpRequest): A request object from which a file will be retrieved. + file_key (str): The key for retrieving the file from `request.FILES`. If no entry exists with this + key, a `ValueError` will be thrown. + allowed_file_types (list): a list of allowable file type extensions. These should start with a period + and be specified in lower-case. For example, ['.txt', '.csv']. If the uploaded file does not end + with one of these extensions, a `PermissionDenied` exception will be thrown. Note that the uploaded file + extension does not need to be lower-case. + base_storage_filename (str): the filename to be used for the stored file, not including the extension. + The same extension as the uploaded file will be appended to this value. + max_file_size (int): the maximum file size in bytes that the uploaded file can be. If the uploaded file + is larger than this size, a `PermissionDenied` exception will be thrown. + validator (function): an optional validation method that, if defined, will be passed the stored file (which + is copied from the uploaded file). This method can do validation on the contents of the file and throw + a `FileValidationException` if the file is not properly formatted. If any exception is thrown, the stored + file will be deleted before the exception is re-raised. Note that the implementor of the validator function + should take care to close the stored file if they open it for reading. + + Returns: + Storage: the file storage object where the file can be retrieved from + str: stored_file_name: the name of the stored file (including extension) + + """ + + if file_key not in request.FILES: + raise ValueError("No file uploaded with key '" + file_key + "'.") + + uploaded_file = request.FILES[file_key] + try: + file_extension = os.path.splitext(uploaded_file.name)[1].lower() + if not file_extension in allowed_file_types: + file_types = "', '".join(allowed_file_types) + msg = ungettext( + "The file must end with the extension '{file_types}'.", + "The file must end with one of the following extensions: '{file_types}'.", + len(allowed_file_types)).format(file_types=file_types) + raise PermissionDenied(msg) + + if uploaded_file.size > max_file_size: + msg = _("Maximum upload file size is {file_size} bytes.").format(file_size=max_file_size) + raise PermissionDenied(msg) + + stored_file_name = base_storage_filename + file_extension + + file_storage = DefaultStorage() + file_storage.save(stored_file_name, uploaded_file) + + if validator: + try: + validator(file_storage, stored_file_name) + except: + file_storage.delete(stored_file_name) + raise + + finally: + uploaded_file.close() + + return file_storage, stored_file_name + + +# pylint: disable=invalid-name +def course_filename_prefix_generator(course_id, separator='_'): + """ + Generates a course-identifying unicode string for use in a file + name. + + Args: + course_id (object): A course identification object. + Returns: + str: A unicode string which can safely be inserted into a + filename. + """ + return get_valid_filename(unicode(separator).join([course_id.org, course_id.course, course_id.run])) + + +# pylint: disable=invalid-name +def course_and_time_based_filename_generator(course_id, base_name): + """ + Generates a filename (without extension) based on the current time and the supplied filename. + + Args: + course_id (object): A course identification object (must have org, course, and run). + base_name (str): A name describing what type of file this is. Any characters that are not safe for + filenames will be converted per django.core.files.storage.get_valid_filename (Specifically, + leading and trailing spaces are removed; other spaces are converted to underscores; and anything + that is not a unicode alphanumeric, dash, underscore, or dot, is removed). + + Returns: + str: a concatenation of the org, course and run from the input course_id, the input base_name, + and the current time. Note that there will be no extension. + + """ + return u"{course_prefix}_{base_name}_{timestamp_str}".format( + course_prefix=course_filename_prefix_generator(course_id), + base_name=get_valid_filename(base_name), + timestamp_str=datetime.now(UTC).strftime("%Y-%m-%d-%H%M%S") # pylint: disable=maybe-no-member + ) + + +class UniversalNewlineIterator(object): + """ + This iterable class can be used as a wrapper around a file-like + object which does not inherently support being read in + universal-newline mode. It returns a line at a time. + """ + def __init__(self, original_file, buffer_size=4096): + self.original_file = original_file + self.buffer_size = buffer_size + + def __iter__(self): + return self.generate_lines() + + @staticmethod + def sanitize(string): + """ + Replace CR and CRLF with LF within `string`. + """ + return string.replace('\r\n', '\n').replace('\r', '\n') + + def generate_lines(self): + """ + Return data from `self.original_file` a line at a time, + replacing CR and CRLF with LF. + """ + buf = self.original_file.read(self.buffer_size) + line = '' + while buf: + for char in buf: + if line.endswith('\r') and char == '\n': + last_line = line + line = '' + yield self.sanitize(last_line) + elif line.endswith('\r') or line.endswith('\n'): + last_line = line + line = char + yield self.sanitize(last_line) + else: + line += char + buf = self.original_file.read(self.buffer_size) + if not buf and line: + yield self.sanitize(line) diff --git a/common/djangoapps/util/tests/test_file.py b/common/djangoapps/util/tests/test_file.py new file mode 100644 index 0000000000..ed50202151 --- /dev/null +++ b/common/djangoapps/util/tests/test_file.py @@ -0,0 +1,257 @@ +# -*- coding: utf-8 -*- +""" +Tests for file.py +""" +import ddt +from io import StringIO + +from django.test import TestCase +from datetime import datetime +from django.utils.timezone import UTC +from mock import patch, Mock +from django.http import HttpRequest +from django.core.files.uploadedfile import SimpleUploadedFile +import util.file +from util.file import ( + course_and_time_based_filename_generator, + course_filename_prefix_generator, + store_uploaded_file, + FileValidationException, + UniversalNewlineIterator +) +from opaque_keys.edx.locations import CourseLocator, SlashSeparatedCourseKey +from django.core import exceptions +import os + + +@ddt.ddt +class FilenamePrefixGeneratorTestCase(TestCase): + """ + Tests for course_filename_prefix_generator + """ + @ddt.data(CourseLocator, SlashSeparatedCourseKey) + def test_locators(self, course_key_class): + self.assertEqual( + course_filename_prefix_generator(course_key_class(org='foo', course='bar', run='baz')), + u'foo_bar_baz' + ) + + @ddt.data(CourseLocator, SlashSeparatedCourseKey) + def test_custom_separator(self, course_key_class): + self.assertEqual( + course_filename_prefix_generator(course_key_class(org='foo', course='bar', run='baz'), separator='-'), + u'foo-bar-baz' + ) + + +@ddt.ddt +class FilenameGeneratorTestCase(TestCase): + """ + Tests for course_and_time_based_filename_generator + """ + NOW = datetime.strptime('1974-06-22T01:02:03', '%Y-%m-%dT%H:%M:%S').replace(tzinfo=UTC()) + + def setUp(self): + datetime_patcher = patch.object( + util.file, 'datetime', + Mock(wraps=datetime) + ) + mocked_datetime = datetime_patcher.start() + mocked_datetime.now.return_value = self.NOW + self.addCleanup(datetime_patcher.stop) + + @ddt.data(CourseLocator, SlashSeparatedCourseKey) + def test_filename_generator(self, course_key_class): + """ + Tests that the generator creates names based on course_id, base name, and date. + """ + self.assertEqual( + u'foo_bar_baz_file_1974-06-22-010203', + course_and_time_based_filename_generator(course_key_class(org='foo', course='bar', run='baz'), 'file') + ) + + self.assertEqual( + u'foo_bar_baz_base_name_ø_1974-06-22-010203', + course_and_time_based_filename_generator( + course_key_class(org='foo', course='bar', run='baz'), ' base` name ø ' + ) + ) + + +class StoreUploadedFileTestCase(TestCase): + """ + Tests for store_uploaded_file. + """ + + def setUp(self): + self.request = Mock(spec=HttpRequest) + self.file_content = "test file content" + self.request.FILES = {"uploaded_file": SimpleUploadedFile("tempfile.csv", self.file_content)} + self.stored_file_name = None + self.file_storage = None + self.default_max_size = 2000000 + + def tearDown(self): + if self.file_storage and self.stored_file_name: + self.file_storage.delete(self.stored_file_name) + + def verify_exception(self, expected_message, error): + """ + Helper method to verify exception text. + """ + self.assertEqual(expected_message, error.exception.message) + + def test_error_conditions(self): + """ + Verifies that exceptions are thrown in the expected cases. + """ + with self.assertRaises(ValueError) as error: + store_uploaded_file(self.request, "wrong_key", [".txt", ".csv"], "stored_file", self.default_max_size) + self.verify_exception("No file uploaded with key 'wrong_key'.", error) + + with self.assertRaises(exceptions.PermissionDenied) as error: + store_uploaded_file(self.request, "uploaded_file", [], "stored_file", self.default_max_size) + self.verify_exception("The file must end with one of the following extensions: ''.", error) + + with self.assertRaises(exceptions.PermissionDenied) as error: + store_uploaded_file(self.request, "uploaded_file", [".bar"], "stored_file", self.default_max_size) + self.verify_exception("The file must end with the extension '.bar'.", error) + + with self.assertRaises(exceptions.PermissionDenied) as error: + store_uploaded_file(self.request, "uploaded_file", [".xxx", ".bar"], "stored_file", self.default_max_size) + self.verify_exception("The file must end with one of the following extensions: '.xxx', '.bar'.", error) + + with self.assertRaises(exceptions.PermissionDenied) as error: + store_uploaded_file(self.request, "uploaded_file", [".csv"], "stored_file", 2) + self.verify_exception("Maximum upload file size is 2 bytes.", error) + + def test_validator(self): + """ + Verify that a validator function can throw an exception. + """ + validator_data = {} + + def verify_file_presence(should_exist): + """ Verify whether or not the stored file, passed to the validator, exists. """ + self.assertEqual(should_exist, validator_data["storage"].exists(validator_data["filename"])) + + def store_file_data(storage, filename): + """ Stores file validator data for testing after validation is complete. """ + validator_data["storage"] = storage + validator_data["filename"] = filename + verify_file_presence(True) + + def exception_validator(storage, filename): + """ Validation test function that throws an exception """ + self.assertEqual("error_file.csv", os.path.basename(filename)) + with storage.open(filename, 'rU') as f: + self.assertEqual(self.file_content, f.read()) + store_file_data(storage, filename) + raise FileValidationException("validation failed") + + def success_validator(storage, filename): + """ Validation test function that is a no-op """ + self.assertEqual("success_file.csv", os.path.basename(filename)) + store_file_data(storage, filename) + + with self.assertRaises(FileValidationException) as error: + store_uploaded_file( + self.request, "uploaded_file", [".csv"], "error_file", + self.default_max_size, validator=exception_validator + ) + self.verify_exception("validation failed", error) + # Verify the file was deleted. + verify_file_presence(False) + + store_uploaded_file( + self.request, "uploaded_file", [".csv"], "success_file", self.default_max_size, validator=success_validator + ) + # Verify the file still exists + verify_file_presence(True) + + def test_file_upload_lower_case_extension(self): + """ + Tests uploading a file with lower case extension. Verifies that the stored file contents are correct. + """ + self.file_storage, self.stored_file_name = store_uploaded_file( + self.request, "uploaded_file", [".csv"], "stored_file", self.default_max_size + ) + self._verify_successful_upload() + + def test_file_upload_upper_case_extension(self): + """ + Tests uploading a file with upper case extension. Verifies that the stored file contents are correct. + """ + self.request.FILES = {"uploaded_file": SimpleUploadedFile("tempfile.CSV", self.file_content)} + self.file_storage, self.stored_file_name = store_uploaded_file( + self.request, "uploaded_file", [".gif", ".csv"], "second_stored_file", self.default_max_size + ) + self._verify_successful_upload() + + def _verify_successful_upload(self): + """ Helper method that checks that the stored version of the uploaded file has the correct content """ + self.assertTrue(self.file_storage.exists(self.stored_file_name)) + with self.file_storage.open(self.stored_file_name, 'r') as f: + self.assertEqual(self.file_content, f.read()) + + +@ddt.ddt +class TestUniversalNewlineIterator(TestCase): + """ + Tests for the UniversalNewlineIterator class. + """ + @ddt.data(1, 2, 999) + def test_line_feeds(self, buffer_size): + self.assertEqual( + [thing for thing in UniversalNewlineIterator(StringIO(u'foo\nbar\n'), buffer_size=buffer_size)], + ['foo\n', 'bar\n'] + ) + + @ddt.data(1, 2, 999) + def test_carriage_returns(self, buffer_size): + self.assertEqual( + [thing for thing in UniversalNewlineIterator(StringIO(u'foo\rbar\r'), buffer_size=buffer_size)], + ['foo\n', 'bar\n'] + ) + + @ddt.data(1, 2, 999) + def test_carriage_returns_and_line_feeds(self, buffer_size): + self.assertEqual( + [thing for thing in UniversalNewlineIterator(StringIO(u'foo\r\nbar\r\n'), buffer_size=buffer_size)], + ['foo\n', 'bar\n'] + ) + + @ddt.data(1, 2, 999) + def test_no_trailing_newline(self, buffer_size): + self.assertEqual( + [thing for thing in UniversalNewlineIterator(StringIO(u'foo\nbar'), buffer_size=buffer_size)], + ['foo\n', 'bar'] + ) + + @ddt.data(1, 2, 999) + def test_only_one_line(self, buffer_size): + self.assertEqual( + [thing for thing in UniversalNewlineIterator(StringIO(u'foo\n'), buffer_size=buffer_size)], + ['foo\n'] + ) + + @ddt.data(1, 2, 999) + def test_only_one_line_no_trailing_newline(self, buffer_size): + self.assertEqual( + [thing for thing in UniversalNewlineIterator(StringIO(u'foo'), buffer_size=buffer_size)], + ['foo'] + ) + + @ddt.data(1, 2, 999) + def test_empty_file(self, buffer_size): + self.assertEqual( + [thing for thing in UniversalNewlineIterator(StringIO(u''), buffer_size=buffer_size)], + [] + ) + + @ddt.data(1, 2, 999) + def test_unicode_data(self, buffer_size): + self.assertEqual( + [thing for thing in UniversalNewlineIterator(StringIO(u'héllø wo®ld'), buffer_size=buffer_size)], + [u'héllø wo®ld'] + ) diff --git a/common/static/js/spec_helpers/ajax_helpers.js b/common/static/js/spec_helpers/ajax_helpers.js index 0b2769be57..3e51f91fd2 100644 --- a/common/static/js/spec_helpers/ajax_helpers.js +++ b/common/static/js/spec_helpers/ajax_helpers.js @@ -77,13 +77,20 @@ define(['sinon', 'underscore'], function(sinon, _) { JSON.stringify(jsonResponse)); }; - respondWithError = function(requests, requestIndex) { + respondWithError = function(requests, statusCode, jsonResponse, requestIndex) { if (_.isUndefined(requestIndex)) { requestIndex = requests.length - 1; } - requests[requestIndex].respond(500, + if (_.isUndefined(statusCode)) { + statusCode = 500; + } + if (_.isUndefined(jsonResponse)) { + jsonResponse = {}; + } + requests[requestIndex].respond(statusCode, { 'Content-Type': 'application/json' }, - JSON.stringify({ })); + JSON.stringify(jsonResponse) + ); }; respondToDelete = function(requests, requestIndex) { diff --git a/common/test/acceptance/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py index f6dd4b45f1..5b251c7140 100644 --- a/common/test/acceptance/pages/lms/instructor_dashboard.py +++ b/common/test/acceptance/pages/lms/instructor_dashboard.py @@ -26,6 +26,37 @@ class InstructorDashboardPage(CoursePage): membership_section.wait_for_page() return membership_section + def select_data_download(self): + """ + Selects the data download tab and returns a DataDownloadPage. + """ + self.q(css='a[data-section=data_download]').first.click() + data_download_section = DataDownloadPage(self.browser) + data_download_section.wait_for_page() + return data_download_section + + @staticmethod + def get_asset_path(file_name): + """ + Returns the full path of the file to upload. + These files have been placed in edx-platform/common/test/data/uploads/ + """ + + # Separate the list of folders in the path reaching to the current file, + # e.g. '... common/test/acceptance/pages/lms/instructor_dashboard.py' will result in + # [..., 'common', 'test', 'acceptance', 'pages', 'lms', 'instructor_dashboard.py'] + folders_list_in_path = __file__.split(os.sep) + + # Get rid of the last 4 elements: 'acceptance', 'pages', 'lms', and 'instructor_dashboard.py' + # to point to the 'test' folder, a shared point in the path's tree. + folders_list_in_path = folders_list_in_path[:-4] + + # Append the folders in the asset's path + folders_list_in_path.extend(['data', 'uploads', file_name]) + + # Return the joined path of the required asset. + return os.sep.join(folders_list_in_path) + class MembershipPage(PageObject): """ @@ -38,15 +69,39 @@ class MembershipPage(PageObject): def select_auto_enroll_section(self): """ - returns the MembershipPageAutoEnrollSection + Returns the MembershipPageAutoEnrollSection page object. """ return MembershipPageAutoEnrollSection(self.browser) + def select_cohort_management_section(self): + """ + Returns the MembershipPageCohortManagementSection page object. + """ + return MembershipPageCohortManagementSection(self.browser) + + +class MembershipPageCohortManagementSection(PageObject): + """ + The cohort management subsection of the Membership section of the Instructor dashboard. + """ + url = None + csv_browse_button_selector = '.csv-upload #file-upload-form-file' + csv_upload_button_selector = '.csv-upload #file-upload-form-submit' + + def is_browser_on_page(self): + return self.q(css='.cohort-management.membership-section').present + + def _bounded_selector(self, selector): + """ + Return `selector`, but limited to the cohort management context. + """ + return '.cohort-management.membership-section {}'.format(selector) + def _get_cohort_options(self): """ Returns the available options in the cohort dropdown, including the initial "Select a cohort group". """ - return self.q(css=".cohort-management #cohort-select option") + return self.q(css=self._bounded_selector("#cohort-select option")) def _cohort_name(self, label): """ @@ -89,7 +144,7 @@ class MembershipPage(PageObject): """ Selects the given cohort in the drop-down. """ - self.q(css=".cohort-management #cohort-select option").filter( + self.q(css=self._bounded_selector("#cohort-select option")).filter( lambda el: self._cohort_name(el.text) == cohort_name ).first.click() @@ -97,45 +152,64 @@ class MembershipPage(PageObject): """ Adds a new manual cohort with the specified name. """ - self.q(css="div.cohort-management-nav .action-create").first.click() - textinput = self.q(css="#cohort-create-name").results[0] + self.q(css=self._bounded_selector("div.cohort-management-nav .action-create")).first.click() + textinput = self.q(css=self._bounded_selector("#cohort-create-name")).results[0] textinput.send_keys(cohort_name) - self.q(css="div.form-actions .action-save").first.click() + self.q(css=self._bounded_selector("div.form-actions .action-save")).first.click() def get_cohort_group_setup(self): """ Returns the description of the current cohort """ - return self.q(css='.cohort-management-group-setup .setup-value').first.text[0] + return self.q(css=self._bounded_selector('.cohort-management-group-setup .setup-value')).first.text[0] def select_edit_settings(self): - self.q(css=".action-edit").first.click() + self.q(css=self._bounded_selector(".action-edit")).first.click() def add_students_to_selected_cohort(self, users): """ Adds a list of users (either usernames or email addresses) to the currently selected cohort. """ - textinput = self.q(css="#cohort-management-group-add-students").results[0] + textinput = self.q(css=self._bounded_selector("#cohort-management-group-add-students")).results[0] for user in users: textinput.send_keys(user) textinput.send_keys(",") - self.q(css="div.cohort-management-group-add .action-primary").first.click() + self.q(css=self._bounded_selector("div.cohort-management-group-add .action-primary")).first.click() def get_cohort_student_input_field_value(self): """ Returns the contents of the input field where students can be added to a cohort. """ - return self.q(css="#cohort-management-group-add-students").results[0].get_attribute("value") + return self.q( + css=self._bounded_selector("#cohort-management-group-add-students") + ).results[0].get_attribute("value") def _get_cohort_messages(self, type): """ - Returns array of messages for given type. + Returns array of messages related to manipulating cohorts directly through the UI for the given type. """ - message_title = self.q(css="div.cohort-management-group-add .cohort-" + type + " .message-title") + title_css = "div.cohort-management-group-add .cohort-" + type + " .message-title" + detail_css = "div.cohort-management-group-add .cohort-" + type + " .summary-item" + + return self._get_messages(title_css, detail_css) + + def get_csv_messages(self): + """ + Returns array of messages related to a CSV upload of cohort assignments. + """ + title_css = ".csv-upload .message-title" + detail_css = ".csv-upload .summary-item" + return self._get_messages(title_css, detail_css) + + def _get_messages(self, title_css, details_css): + """ + Helper method to get messages given title and details CSS. + """ + message_title = self.q(css=self._bounded_selector(title_css)) if len(message_title.results) == 0: return [] messages = [message_title.first.text[0]] - details = self.q(css="div.cohort-management-group-add .cohort-" + type + " .summary-item").results + details = self.q(css=self._bounded_selector(details_css)).results for detail in details: messages.append(detail.text) return messages @@ -158,7 +232,20 @@ class MembershipPage(PageObject): """ Click on the link to the Data Download Page. """ - self.q(css="a.link-cross-reference[data-section=data_download]").first.click() + self.q(css=self._bounded_selector("a.link-cross-reference[data-section=data_download]")).first.click() + + def upload_cohort_file(self, filename): + """ + Uploads a file with cohort assignment information. + """ + # If the CSV upload section has not yet been toggled on, click on the toggle link. + cvs_upload_toggle = self.q(css=self._bounded_selector(".toggle-cohort-management-secondary")).first + if cvs_upload_toggle: + cvs_upload_toggle.click() + path = InstructorDashboardPage.get_asset_path(filename) + file_input = self.q(css=self._bounded_selector(self.csv_browse_button_selector)).results[0] + file_input.send_keys(path) + self.q(css=self._bounded_selector(self.csv_upload_button_selector)).first.click() class MembershipPageAutoEnrollSection(PageObject): @@ -215,49 +302,30 @@ class MembershipPageAutoEnrollSection(PageObject): self.wait_for_element_presence(error_message_selector, "%s message" % section_type.title()) return self.q(css=error_message_selector).text[0] - def get_asset_path(self, file_name): - """ - Returns the full path of the file to upload. - These files have been placed in edx-platform/common/test/data/uploads/ - """ - - # Separate the list of folders in the path reaching to the current file, - # e.g. '... common/test/acceptance/pages/lms/instructor_dashboard.py' will result in - # [..., 'common', 'test', 'acceptance', 'pages', 'lms', 'instructor_dashboard.py'] - folders_list_in_path = __file__.split(os.sep) - - # Get rid of the last 4 elements: 'acceptance', 'pages', 'lms', and 'instructor_dashboard.py' - # to point to the 'test' folder, a shared point in the path's tree. - folders_list_in_path = folders_list_in_path[:-4] - - # Append the folders in the asset's path - folders_list_in_path.extend(['data', 'uploads', file_name]) - - # Return the joined path of the required asset. - return os.sep.join(folders_list_in_path) - def upload_correct_csv_file(self): """ Selects the correct file and clicks the upload button. """ - correct_files_path = self.get_asset_path('auto_reg_enrollment.csv') - self.q(css=self.auto_enroll_browse_button_selector).results[0].send_keys(correct_files_path) - self.click_upload_file_button() + self._upload_file('auto_reg_enrollment.csv') def upload_csv_file_with_errors_warnings(self): """ Selects the file which will generate errors and warnings and clicks the upload button. """ - errors_warnings_files_path = self.get_asset_path('auto_reg_enrollment_errors_warnings.csv') - self.q(css=self.auto_enroll_browse_button_selector).results[0].send_keys(errors_warnings_files_path) - self.click_upload_file_button() + self._upload_file('auto_reg_enrollment_errors_warnings.csv') def upload_non_csv_file(self): """ Selects an image file and clicks the upload button. """ - errors_warnings_files_path = self.get_asset_path('image.jpg') - self.q(css=self.auto_enroll_browse_button_selector).results[0].send_keys(errors_warnings_files_path) + self._upload_file('image.jpg') + + def _upload_file(self, filename): + """ + Helper method to upload a file with registration and enrollment information. + """ + file_path = InstructorDashboardPage.get_asset_path(filename) + self.q(css=self.auto_enroll_browse_button_selector).results[0].send_keys(file_path) self.click_upload_file_button() @@ -269,3 +337,10 @@ class DataDownloadPage(PageObject): def is_browser_on_page(self): return self.q(css='a[data-section=data_download].active-section').present + + def get_available_reports_for_download(self): + """ + Returns a list of all the available reports for download. + """ + reports = self.q(css="#report-downloads-table .file-download-link>a").map(lambda el: el.text) + return reports.results diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py index 146c9ee586..cf963e7f72 100644 --- a/common/test/acceptance/tests/discussion/test_cohort_management.py +++ b/common/test/acceptance/tests/discussion/test_cohort_management.py @@ -7,6 +7,7 @@ from datetime import datetime from pymongo import MongoClient +from pytz import UTC, utc from bok_choy.promise import EmptyPromise from .helpers import CohortTestMixin from ..helpers import UniqueCourseTest @@ -41,28 +42,37 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): # create a non-instructor who will be registered for the course and in the manual cohort. self.student_name = "student_user" self.student_id = AutoAuthPage( - self.browser, username=self.student_name, course_id=self.course_id, staff=False + self.browser, username=self.student_name, email="student_user@example.com", + course_id=self.course_id, staff=False ).visit().get_user_id() self.add_user_to_cohort(self.course_fixture, self.student_name, self.manual_cohort_id) + # create a user with unicode characters in their username + self.unicode_student_id = AutoAuthPage( + self.browser, username="Ωπ", email="unicode_student_user@example.com", + course_id=self.course_id, staff=False + ).visit().get_user_id() + # login as an instructor self.instructor_name = "instructor_user" self.instructor_id = AutoAuthPage( - self.browser, username=self.instructor_name, course_id=self.course_id, staff=True + self.browser, username=self.instructor_name, email="instructor_user@example.com", + course_id=self.course_id, staff=True ).visit().get_user_id() # go to the membership page on the instructor dashboard - instructor_dashboard_page = InstructorDashboardPage(self.browser, self.course_id) - instructor_dashboard_page.visit() - self.membership_page = instructor_dashboard_page.select_membership() + self.instructor_dashboard_page = InstructorDashboardPage(self.browser, self.course_id) + self.instructor_dashboard_page.visit() + membership_page = self.instructor_dashboard_page.select_membership() + self.cohort_management_page = membership_page.select_cohort_management_section() def verify_cohort_description(self, cohort_name, expected_description): """ Selects the cohort with the given name and verifies the expected description is presented. """ - self.membership_page.select_cohort(cohort_name) - self.assertEquals(self.membership_page.get_selected_cohort(), cohort_name) - self.assertIn(expected_description, self.membership_page.get_cohort_group_setup()) + self.cohort_management_page.select_cohort(cohort_name) + self.assertEquals(self.cohort_management_page.get_selected_cohort(), cohort_name) + self.assertIn(expected_description, self.cohort_management_page.get_cohort_group_setup()) def test_cohort_description(self): """ @@ -93,8 +103,8 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): When I view the cohort in the LMS instructor dashboard There is a link to take me to the Studio Advanced Settings for the course """ - self.membership_page.select_cohort(self.manual_cohort_name) - self.membership_page.select_edit_settings() + self.cohort_management_page.select_cohort(self.manual_cohort_name) + self.cohort_management_page.select_edit_settings() advanced_settings_page = AdvancedSettingsPage( self.browser, self.course_info['org'], self.course_info['number'], self.course_info['run'] ) @@ -114,19 +124,19 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): And the user input field is empty And appropriate events have been emitted """ - start_time = datetime.now() - self.membership_page.select_cohort(self.auto_cohort_name) - self.assertEqual(0, self.membership_page.get_selected_cohort_count()) - self.membership_page.add_students_to_selected_cohort([self.student_name, self.instructor_name]) + start_time = datetime.now(UTC) + self.cohort_management_page.select_cohort(self.auto_cohort_name) + self.assertEqual(0, self.cohort_management_page.get_selected_cohort_count()) + self.cohort_management_page.add_students_to_selected_cohort([self.student_name, self.instructor_name]) # Wait for the number of users in the cohort to change, indicating that the add operation is complete. EmptyPromise( - lambda: 2 == self.membership_page.get_selected_cohort_count(), 'Waiting for added students' + lambda: 2 == self.cohort_management_page.get_selected_cohort_count(), 'Waiting for added students' ).fulfill() - confirmation_messages = self.membership_page.get_cohort_confirmation_messages() + confirmation_messages = self.cohort_management_page.get_cohort_confirmation_messages() self.assertEqual(2, len(confirmation_messages)) self.assertEqual("2 students have been added to this cohort group", confirmation_messages[0]) self.assertEqual("1 student was removed from " + self.manual_cohort_name, confirmation_messages[1]) - self.assertEqual("", self.membership_page.get_cohort_student_input_field_value()) + self.assertEqual("", self.cohort_management_page.get_cohort_student_input_field_value()) self.assertEqual( self.event_collection.find({ "name": "edx.cohort.user_added", @@ -178,27 +188,27 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): And I get a notification that one user is unknown And the user input field still contains the incorrect email addresses """ - self.membership_page.select_cohort(self.manual_cohort_name) - self.assertEqual(1, self.membership_page.get_selected_cohort_count()) - self.membership_page.add_students_to_selected_cohort([self.student_name, "unknown_user"]) + self.cohort_management_page.select_cohort(self.manual_cohort_name) + self.assertEqual(1, self.cohort_management_page.get_selected_cohort_count()) + self.cohort_management_page.add_students_to_selected_cohort([self.student_name, "unknown_user"]) # Wait for notification messages to appear, indicating that the add operation is complete. EmptyPromise( - lambda: 2 == len(self.membership_page.get_cohort_confirmation_messages()), 'Waiting for notification' + lambda: 2 == len(self.cohort_management_page.get_cohort_confirmation_messages()), 'Waiting for notification' ).fulfill() - self.assertEqual(1, self.membership_page.get_selected_cohort_count()) + self.assertEqual(1, self.cohort_management_page.get_selected_cohort_count()) - confirmation_messages = self.membership_page.get_cohort_confirmation_messages() + confirmation_messages = self.cohort_management_page.get_cohort_confirmation_messages() self.assertEqual(2, len(confirmation_messages)) self.assertEqual("0 students have been added to this cohort group", confirmation_messages[0]) self.assertEqual("1 student was already in the cohort group", confirmation_messages[1]) - error_messages = self.membership_page.get_cohort_error_messages() + error_messages = self.cohort_management_page.get_cohort_error_messages() self.assertEqual(2, len(error_messages)) self.assertEqual("There was an error when trying to add students:", error_messages[0]) self.assertEqual("Unknown user: unknown_user", error_messages[1]) self.assertEqual( self.student_name + ",unknown_user,", - self.membership_page.get_cohort_student_input_field_value() + self.cohort_management_page.get_cohort_student_input_field_value() ) def test_add_new_cohort(self): @@ -212,19 +222,19 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): Then the cohort has 1 user And appropriate events have been emitted """ - start_time = datetime.now() + start_time = datetime.now(UTC) new_cohort = str(uuid.uuid4().get_hex()[0:20]) - self.assertFalse(new_cohort in self.membership_page.get_cohorts()) - self.membership_page.add_cohort(new_cohort) + self.assertFalse(new_cohort in self.cohort_management_page.get_cohorts()) + self.cohort_management_page.add_cohort(new_cohort) # After adding the cohort, it should automatically be selected EmptyPromise( - lambda: new_cohort == self.membership_page.get_selected_cohort(), "Waiting for new cohort to appear" + lambda: new_cohort == self.cohort_management_page.get_selected_cohort(), "Waiting for new cohort to appear" ).fulfill() - self.assertEqual(0, self.membership_page.get_selected_cohort_count()) - self.membership_page.add_students_to_selected_cohort([self.instructor_name]) + self.assertEqual(0, self.cohort_management_page.get_selected_cohort_count()) + self.cohort_management_page.add_students_to_selected_cohort([self.instructor_name]) # Wait for the number of users in the cohort to change, indicating that the add operation is complete. EmptyPromise( - lambda: 1 == self.membership_page.get_selected_cohort_count(), 'Waiting for student to be added' + lambda: 1 == self.cohort_management_page.get_selected_cohort_count(), 'Waiting for student to be added' ).fulfill() self.assertEqual( self.event_collection.find({ @@ -252,6 +262,162 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): When I view the cohort in the LMS instructor dashboard There is a link to take me to the Data Download section of the Instructor Dashboard. """ - self.membership_page.select_data_download() + self.cohort_management_page.select_data_download() data_download_page = DataDownloadPage(self.browser) data_download_page.wait_for_page() + + def test_cohort_by_csv_both_columns(self): + """ + Scenario: the instructor can upload a file with user and cohort assignments, using both emails and usernames. + + Given I have a course with two cohorts defined + When I go to the cohort management section of the instructor dashboard + I can upload a CSV file with assignments of users to cohorts via both usernames and emails + Then I can download a file with results + And appropriate events have been emitted + """ + # cohort_users_both_columns.csv adds instructor_user to ManualCohort1 via username and + # student_user to AutoCohort1 via email + self._verify_csv_upload_acceptable_file("cohort_users_both_columns.csv") + + def test_cohort_by_csv_only_email(self): + """ + Scenario: the instructor can upload a file with user and cohort assignments, using only emails. + + Given I have a course with two cohorts defined + When I go to the cohort management section of the instructor dashboard + I can upload a CSV file with assignments of users to cohorts via only emails + Then I can download a file with results + And appropriate events have been emitted + """ + # cohort_users_only_email.csv adds instructor_user to ManualCohort1 and student_user to AutoCohort1 via email + self._verify_csv_upload_acceptable_file("cohort_users_only_email.csv") + + def test_cohort_by_csv_only_username(self): + """ + Scenario: the instructor can upload a file with user and cohort assignments, using only usernames. + + Given I have a course with two cohorts defined + When I go to the cohort management section of the instructor dashboard + I can upload a CSV file with assignments of users to cohorts via only usernames + Then I can download a file with results + And appropriate events have been emitted + """ + # cohort_users_only_username.csv adds instructor_user to ManualCohort1 and + # student_user to AutoCohort1 via username + self._verify_csv_upload_acceptable_file("cohort_users_only_username.csv") + + def _verify_csv_upload_acceptable_file(self, filename): + """ + Helper method to verify cohort assignments after a successful CSV upload. + """ + start_time = datetime.now(UTC) + self.cohort_management_page.upload_cohort_file(filename) + self._verify_cohort_by_csv_notification( + "Your file '{}' has been uploaded. Please allow a few minutes for processing.".format(filename) + ) + + # student_user is moved from manual cohort group to auto cohort group + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.user_added", + "time": {"$gt": start_time}, + "event.user_id": {"$in": [int(self.student_id)]}, + "event.cohort_name": self.auto_cohort_name, + }).count(), + 1 + ) + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.user_removed", + "time": {"$gt": start_time}, + "event.user_id": int(self.student_id), + "event.cohort_name": self.manual_cohort_name, + }).count(), + 1 + ) + # instructor_user (previously unassigned) is added to manual cohort group + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.user_added", + "time": {"$gt": start_time}, + "event.user_id": {"$in": [int(self.instructor_id)]}, + "event.cohort_name": self.manual_cohort_name, + }).count(), + 1 + ) + # unicode_student_user (previously unassigned) is added to manual cohort group + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.user_added", + "time": {"$gt": start_time}, + "event.user_id": {"$in": [int(self.unicode_student_id)]}, + "event.cohort_name": self.manual_cohort_name, + }).count(), + 1 + ) + + # Verify the results can be downloaded. + data_download = self.instructor_dashboard_page.select_data_download() + EmptyPromise( + lambda: 1 == len(data_download.get_available_reports_for_download()), 'Waiting for downloadable report' + ).fulfill() + report = data_download.get_available_reports_for_download()[0] + base_file_name = "cohort_results_" + self.assertIn("{}_{}".format( + '_'.join([self.course_info['org'], self.course_info['number'], self.course_info['run']]), base_file_name + ), report) + report_datetime = datetime.strptime( + report[report.index(base_file_name) + len(base_file_name):-len(".csv")], + "%Y-%m-%d-%H%M" + ) + self.assertLessEqual(start_time.replace(second=0, microsecond=0), utc.localize(report_datetime)) + + def test_cohort_by_csv_wrong_file_type(self): + """ + Scenario: if the instructor uploads a non-csv file, an error message is presented. + + Given I have a course with cohorting enabled + When I go to the cohort management section of the instructor dashboard + And I upload a file without the CSV extension + Then I get an error message stating that the file must have a CSV extension + """ + self.cohort_management_page.upload_cohort_file("image.jpg") + self._verify_cohort_by_csv_notification("The file must end with the extension '.csv'.") + + def test_cohort_by_csv_missing_cohort(self): + """ + Scenario: if the instructor uploads a csv file with no cohort column, an error message is presented. + + Given I have a course with cohorting enabled + When I go to the cohort management section of the instructor dashboard + And I upload a CSV file that is missing the cohort column + Then I get an error message stating that the file must have a cohort column + """ + self.cohort_management_page.upload_cohort_file("cohort_users_missing_cohort_column.csv") + self._verify_cohort_by_csv_notification("The file must contain a 'cohort' column containing cohort names.") + + def test_cohort_by_csv_missing_user(self): + """ + Scenario: if the instructor uploads a csv file with no username or email column, an error message is presented. + + Given I have a course with cohorting enabled + When I go to the cohort management section of the instructor dashboard + And I upload a CSV file that is missing both the username and email columns + Then I get an error message stating that the file must have either a username or email column + """ + self.cohort_management_page.upload_cohort_file("cohort_users_missing_user_columns.csv") + self._verify_cohort_by_csv_notification( + "The file must contain a 'username' column, an 'email' column, or both." + ) + + def _verify_cohort_by_csv_notification(self, expected_message): + """ + Helper method to check the CSV file upload notification message. + """ + # Wait for notification message to appear, indicating file has been uploaded. + EmptyPromise( + lambda: 1 == len(self.cohort_management_page.get_csv_messages()), 'Waiting for notification' + ).fulfill() + messages = self.cohort_management_page.get_csv_messages() + self.assertEquals(expected_message, messages[0]) diff --git a/common/test/acceptance/tests/lms/test_lms_staff_view.py b/common/test/acceptance/tests/lms/test_lms_staff_view.py index 3d6bf77c09..12d0608eb0 100644 --- a/common/test/acceptance/tests/lms/test_lms_staff_view.py +++ b/common/test/acceptance/tests/lms/test_lms_staff_view.py @@ -107,10 +107,7 @@ class StaffDebugTest(UniqueCourseTest): staff_debug_page = staff_page.open_staff_debug_info() staff_debug_page.rescore() msg = staff_debug_page.idash_msg[0] - # Since we aren't running celery stuff, this will fail badly - # for now, but is worth excercising that bad of a response - self.assertEqual(u'Failed to rescore problem. ' - 'Unknown Error Occurred.', msg) + self.assertEqual(u'Successfully rescored problem for user STAFF_TESTER', msg) def test_student_state_delete(self): """ @@ -176,10 +173,7 @@ class StaffDebugTest(UniqueCourseTest): staff_debug_page = staff_page.open_staff_debug_info() staff_debug_page.rescore() msg = staff_debug_page.idash_msg[0] - # Since we aren't running celery stuff, this will fail badly - # for now, but is worth excercising that bad of a response - self.assertEqual(u'Failed to rescore problem. ' - 'Unknown Error Occurred.', msg) + self.assertEqual(u'Successfully rescored problem for user STAFF_TESTER', msg) def test_student_state_delete_for_problem_loaded_via_ajax(self): """ diff --git a/common/test/data/uploads/cohort_users_both_columns.csv b/common/test/data/uploads/cohort_users_both_columns.csv new file mode 100644 index 0000000000..eced3db09d --- /dev/null +++ b/common/test/data/uploads/cohort_users_both_columns.csv @@ -0,0 +1,4 @@ +username,email,ignored_column,cohort +instructor_user,,June,ManualCohort1 +,student_user@example.com,Spring,AutoCohort1 +Ωπ,,Fall,ManualCohort1 diff --git a/common/test/data/uploads/cohort_users_missing_cohort_column.csv b/common/test/data/uploads/cohort_users_missing_cohort_column.csv new file mode 100644 index 0000000000..01a8bb2237 --- /dev/null +++ b/common/test/data/uploads/cohort_users_missing_cohort_column.csv @@ -0,0 +1,3 @@ +username,email +instructor_user, +,student_user@example.com diff --git a/common/test/data/uploads/cohort_users_missing_user_columns.csv b/common/test/data/uploads/cohort_users_missing_user_columns.csv new file mode 100644 index 0000000000..e41a1b26aa --- /dev/null +++ b/common/test/data/uploads/cohort_users_missing_user_columns.csv @@ -0,0 +1,3 @@ +cohort +ManualCohort1 +AutoCohort1 diff --git a/common/test/data/uploads/cohort_users_only_email.csv b/common/test/data/uploads/cohort_users_only_email.csv new file mode 100644 index 0000000000..7835d455fb --- /dev/null +++ b/common/test/data/uploads/cohort_users_only_email.csv @@ -0,0 +1,5 @@ +email,cohort +instructor_user@example.com,ManualCohort1 +student_user@example.com,AutoCohort1 +unicode_student_user@example.com,ManualCohort1 + diff --git a/common/test/data/uploads/cohort_users_only_username.csv b/common/test/data/uploads/cohort_users_only_username.csv new file mode 100644 index 0000000000..0c7588030a --- /dev/null +++ b/common/test/data/uploads/cohort_users_only_username.csv @@ -0,0 +1,4 @@ +username,cohort +instructor_user,ManualCohort1 +student_user,AutoCohort1 +Ωπ,ManualCohort1 \ No newline at end of file diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index 222754b9d7..c3db473e63 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -8,7 +8,6 @@ import urlparse from django.contrib.auth.decorators import login_required from django.contrib.auth.models import User from django.core import exceptions -from django.core.files.storage import get_storage_class from django.http import Http404, HttpResponseBadRequest from django.utils.translation import ugettext as _ from django.views.decorators import csrf @@ -17,6 +16,7 @@ from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locations import SlashSeparatedCourseKey from courseware.access import has_access +from util.file import store_uploaded_file from courseware.courses import get_course_with_access, get_course_by_id import django_comment_client.settings as cc_settings from django_comment_client.utils import ( @@ -558,7 +558,6 @@ def upload(request, course_id): # ajax upload file to a question or answer """ # check upload permission - result = '' error = '' new_file_name = '' try: @@ -570,29 +569,11 @@ def upload(request, course_id): # ajax upload file to a question or answer #request.user.assert_can_upload_file() - # check file type - f = request.FILES['file-upload'] - file_extension = os.path.splitext(f.name)[1].lower() - if not file_extension in cc_settings.ALLOWED_UPLOAD_FILE_TYPES: - file_types = "', '".join(cc_settings.ALLOWED_UPLOAD_FILE_TYPES) - msg = _("allowed file types are '%(file_types)s'") % \ - {'file_types': file_types} - raise exceptions.PermissionDenied(msg) - - # generate new file name - new_file_name = str(time.time()).replace('.', str(random.randint(0, 100000))) + file_extension - - file_storage = get_storage_class()() - # use default storage to store file - file_storage.save(new_file_name, f) - # check file size - # byte - size = file_storage.size(new_file_name) - if size > cc_settings.MAX_UPLOAD_FILE_SIZE: - file_storage.delete(new_file_name) - msg = _("Maximum upload file size is %(file_size)s bytes.") % \ - {'file_size': cc_settings.MAX_UPLOAD_FILE_SIZE} - raise exceptions.PermissionDenied(msg) + base_file_name = str(time.time()).replace('.', str(random.randint(0, 100000))) + file_storage, new_file_name = store_uploaded_file( + request, 'file-upload', cc_settings.ALLOWED_UPLOAD_FILE_TYPES, base_file_name, + max_file_size=cc_settings.MAX_UPLOAD_FILE_SIZE + ) except exceptions.PermissionDenied, err: error = unicode(err) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index aaf0f57f85..36ce43e068 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -6,8 +6,11 @@ import datetime import ddt import io import json +import os import random import requests +import shutil +import tempfile from unittest import TestCase from urllib import quote @@ -3289,3 +3292,166 @@ class TestCourseRegistrationCodes(ModuleStoreTestCase): self.assertEqual(response['Content-Type'], 'text/csv') body = response.content.replace('\r', '') self.assertTrue(body.startswith(EXPECTED_COUPON_CSV_HEADER)) + + +@override_settings(MODULESTORE=TEST_DATA_MOCK_MODULESTORE) +class TestBulkCohorting(ModuleStoreTestCase): + """ + Test adding users to cohorts in bulk via CSV upload. + """ + def setUp(self): + super(TestBulkCohorting, self).setUp() + self.course = CourseFactory.create() + self.staff_user = StaffFactory(course_key=self.course.id) + self.non_staff_user = UserFactory.create() + self.tempdir = tempfile.mkdtemp() + + def tearDown(self): + if os.path.exists(self.tempdir): + shutil.rmtree(self.tempdir) + + def call_add_users_to_cohorts(self, csv_data, suffix='.csv', method='POST'): + """ + Call `add_users_to_cohorts` with a file generated from `csv_data`. + """ + # this temporary file will be removed in `self.tearDown()` + __, file_name = tempfile.mkstemp(suffix=suffix, dir=self.tempdir) + with open(file_name, 'w') as file_pointer: + file_pointer.write(csv_data.encode('utf-8')) + with open(file_name, 'r') as file_pointer: + url = reverse('add_users_to_cohorts', kwargs={'course_id': unicode(self.course.id)}) + if method == 'POST': + return self.client.post(url, {'uploaded-file': file_pointer}) + elif method == 'GET': + return self.client.get(url, {'uploaded-file': file_pointer}) + + def expect_error_on_file_content(self, file_content, error, file_suffix='.csv'): + """ + Verify that we get the error we expect for a given file input. + """ + self.client.login(username=self.staff_user.username, password='test') + response = self.call_add_users_to_cohorts(file_content, suffix=file_suffix) + self.assertEqual(response.status_code, 400) + result = json.loads(response.content) + self.assertEqual(result['error'], error) + + def verify_success_on_file_content(self, file_content, mock_store_upload, mock_cohort_task): + """ + Verify that `addd_users_to_cohorts` successfully validates the + file content, uploads the input file, and triggers the + background task. + """ + mock_store_upload.return_value = (None, 'fake_file_name.csv') + self.client.login(username=self.staff_user.username, password='test') + response = self.call_add_users_to_cohorts(file_content) + self.assertEqual(response.status_code, 204) + self.assertTrue(mock_store_upload.called) + self.assertTrue(mock_cohort_task.called) + + def test_no_cohort_field(self): + """ + Verify that we get a descriptive verification error when we haven't + included a cohort field in the uploaded CSV. + """ + self.expect_error_on_file_content( + 'username,email\n', "The file must contain a 'cohort' column containing cohort names." + ) + + def test_no_username_or_email_field(self): + """ + Verify that we get a descriptive verification error when we haven't + included a username or email field in the uploaded CSV. + """ + self.expect_error_on_file_content( + 'cohort\n', "The file must contain a 'username' column, an 'email' column, or both." + ) + + def test_empty_csv(self): + """ + Verify that we get a descriptive verification error when we haven't + included any data in the uploaded CSV. + """ + self.expect_error_on_file_content( + '', "The file must contain a 'cohort' column containing cohort names." + ) + + def test_wrong_extension(self): + """ + Verify that we get a descriptive verification error when we haven't + uploaded a file with a '.csv' extension. + """ + self.expect_error_on_file_content( + '', "The file must end with the extension '.csv'.", file_suffix='.notcsv' + ) + + def test_non_staff_no_access(self): + """ + Verify that we can't access the view when we aren't a staff user. + """ + self.client.login(username=self.non_staff_user.username, password='test') + response = self.call_add_users_to_cohorts('') + self.assertEqual(response.status_code, 403) + + def test_post_only(self): + """ + Verify that we can't call the view when we aren't using POST. + """ + self.client.login(username=self.staff_user.username, password='test') + response = self.call_add_users_to_cohorts('', method='GET') + self.assertEqual(response.status_code, 405) + + @patch('instructor.views.api.instructor_task.api.submit_cohort_students') + @patch('instructor.views.api.store_uploaded_file') + def test_success_username(self, mock_store_upload, mock_cohort_task): + """ + Verify that we store the input CSV and call a background task when + the CSV has username and cohort columns. + """ + self.verify_success_on_file_content( + 'username,cohort\nfoo_username,bar_cohort', mock_store_upload, mock_cohort_task + ) + + @patch('instructor.views.api.instructor_task.api.submit_cohort_students') + @patch('instructor.views.api.store_uploaded_file') + def test_success_email(self, mock_store_upload, mock_cohort_task): + """ + Verify that we store the input CSV and call the cohorting background + task when the CSV has email and cohort columns. + """ + self.verify_success_on_file_content( + 'email,cohort\nfoo_email,bar_cohort', mock_store_upload, mock_cohort_task + ) + + @patch('instructor.views.api.instructor_task.api.submit_cohort_students') + @patch('instructor.views.api.store_uploaded_file') + def test_success_username_and_email(self, mock_store_upload, mock_cohort_task): + """ + Verify that we store the input CSV and call the cohorting background + task when the CSV has username, email and cohort columns. + """ + self.verify_success_on_file_content( + 'username,email,cohort\nfoo_username,bar_email,baz_cohort', mock_store_upload, mock_cohort_task + ) + + @patch('instructor.views.api.instructor_task.api.submit_cohort_students') + @patch('instructor.views.api.store_uploaded_file') + def test_success_carriage_return(self, mock_store_upload, mock_cohort_task): + """ + Verify that we store the input CSV and call the cohorting background + task when lines in the CSV are delimited by carriage returns. + """ + self.verify_success_on_file_content( + 'username,email,cohort\rfoo_username,bar_email,baz_cohort', mock_store_upload, mock_cohort_task + ) + + @patch('instructor.views.api.instructor_task.api.submit_cohort_students') + @patch('instructor.views.api.store_uploaded_file') + def test_success_carriage_return_line_feed(self, mock_store_upload, mock_cohort_task): + """ + Verify that we store the input CSV and call the cohorting background + task when lines in the CSV are delimited by carriage returns and line + feeds. + """ + self.verify_success_on_file_content( + 'username,email,cohort\r\nfoo_username,bar_email,baz_cohort', mock_store_upload, mock_cohort_task + ) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index a5efa2b738..45fce6b7f7 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -15,7 +15,7 @@ from django.conf import settings from django_future.csrf import ensure_csrf_cookie from django.views.decorators.http import require_POST from django.views.decorators.cache import cache_control -from django.core.exceptions import ValidationError +from django.core.exceptions import ValidationError, PermissionDenied from django.core.mail.message import EmailMessage from django.db import IntegrityError from django.core.urlresolvers import reverse @@ -25,7 +25,9 @@ from django.http import HttpResponse, HttpResponseBadRequest, HttpResponseForbid from django.utils.html import strip_tags import string # pylint: disable=deprecated-module import random +import unicodecsv import urllib +from util.file import store_uploaded_file, course_and_time_based_filename_generator, FileValidationException, UniversalNewlineIterator from util.json_request import JsonResponse from instructor.views.instructor_task_helpers import extract_email_features, extract_task_features @@ -956,6 +958,51 @@ def get_students_features(request, course_id, csv=False): # pylint: disable=red return JsonResponse({"status": already_running_status}) +@ensure_csrf_cookie +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_POST +@require_level('staff') +def add_users_to_cohorts(request, course_id): + """ + View method that accepts an uploaded file (using key "uploaded-file") + containing cohort assignments for users. This method spawns a celery task + to do the assignments, and a CSV file with results is provided via data downloads. + """ + course_key = SlashSeparatedCourseKey.from_string(course_id) + + try: + def validator(file_storage, file_to_validate): + """ + Verifies that the expected columns are present. + """ + with file_storage.open(file_to_validate) as f: + reader = unicodecsv.reader(UniversalNewlineIterator(f), encoding='utf-8') + try: + fieldnames = next(reader) + except StopIteration: + fieldnames = [] + msg = None + if "cohort" not in fieldnames: + msg = _("The file must contain a 'cohort' column containing cohort names.") + elif "email" not in fieldnames and "username" not in fieldnames: + msg = _("The file must contain a 'username' column, an 'email' column, or both.") + if msg: + raise FileValidationException(msg) + + __, filename = store_uploaded_file( + request, 'uploaded-file', ['.csv'], + course_and_time_based_filename_generator(course_key, "cohorts"), + max_file_size=2000000, # limit to 2 MB + validator=validator + ) + # The task will assume the default file storage. + instructor_task.api.submit_cohort_students(request, course_key, filename) + except (FileValidationException, PermissionDenied) as err: + return JsonResponse({"error": unicode(err)}, status=400) + + return JsonResponse() + + @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @require_level('staff') diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index e1d929fbcd..328190cdae 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -83,4 +83,8 @@ urlpatterns = patterns('', # nopep8 # spoc gradebook url(r'^gradebook$', 'instructor.views.api.spoc_gradebook', name='spoc_gradebook'), + + # Cohort management + url(r'add_users_to_cohorts$', + 'instructor.views.api.add_users_to_cohorts', name="add_users_to_cohorts"), ) diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index d12474fbf5..d6ccb40805 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -104,7 +104,7 @@ def instructor_dashboard_2(request, course_id): context = { 'course': course, - 'old_dashboard_url': reverse('instructor_dashboard_legacy', kwargs={'course_id': course_key.to_deprecated_string()}), + 'old_dashboard_url': reverse('instructor_dashboard_legacy', kwargs={'course_id': unicode(course_key)}), 'studio_url': get_studio_url(course, 'course'), 'sections': sections, 'disable_buttons': disable_buttons, @@ -141,23 +141,23 @@ def _section_e_commerce(course, access): 'section_key': 'e-commerce', 'section_display_name': _('E-Commerce'), 'access': access, - 'course_id': course_key.to_deprecated_string(), + 'course_id': unicode(course_key), 'currency_symbol': settings.PAID_COURSE_REGISTRATION_CURRENCY[1], - 'ajax_remove_coupon_url': reverse('remove_coupon', kwargs={'course_id': course_key.to_deprecated_string()}), - 'ajax_get_coupon_info': reverse('get_coupon_info', kwargs={'course_id': course_key.to_deprecated_string()}), - 'get_user_invoice_preference_url': reverse('get_user_invoice_preference', kwargs={'course_id': course_key.to_deprecated_string()}), - 'sale_validation_url': reverse('sale_validation', kwargs={'course_id': course_key.to_deprecated_string()}), - 'ajax_update_coupon': reverse('update_coupon', kwargs={'course_id': course_key.to_deprecated_string()}), - 'ajax_add_coupon': reverse('add_coupon', kwargs={'course_id': course_key.to_deprecated_string()}), - 'get_sale_records_url': reverse('get_sale_records', kwargs={'course_id': course_key.to_deprecated_string()}), - 'get_sale_order_records_url': reverse('get_sale_order_records', kwargs={'course_id': course_key.to_deprecated_string()}), - 'instructor_url': reverse('instructor_dashboard', kwargs={'course_id': course_key.to_deprecated_string()}), - 'get_registration_code_csv_url': reverse('get_registration_codes', kwargs={'course_id': course_key.to_deprecated_string()}), - 'generate_registration_code_csv_url': reverse('generate_registration_codes', kwargs={'course_id': course_key.to_deprecated_string()}), - 'active_registration_code_csv_url': reverse('active_registration_codes', kwargs={'course_id': course_key.to_deprecated_string()}), - 'spent_registration_code_csv_url': reverse('spent_registration_codes', kwargs={'course_id': course_key.to_deprecated_string()}), - 'set_course_mode_url': reverse('set_course_mode_price', kwargs={'course_id': course_key.to_deprecated_string()}), - 'download_coupon_codes_url': reverse('get_coupon_codes', kwargs={'course_id': course_key.to_deprecated_string()}), + 'ajax_remove_coupon_url': reverse('remove_coupon', kwargs={'course_id': unicode(course_key)}), + 'ajax_get_coupon_info': reverse('get_coupon_info', kwargs={'course_id': unicode(course_key)}), + 'get_user_invoice_preference_url': reverse('get_user_invoice_preference', kwargs={'course_id': unicode(course_key)}), + 'sale_validation_url': reverse('sale_validation', kwargs={'course_id': unicode(course_key)}), + 'ajax_update_coupon': reverse('update_coupon', kwargs={'course_id': unicode(course_key)}), + 'ajax_add_coupon': reverse('add_coupon', kwargs={'course_id': unicode(course_key)}), + 'get_sale_records_url': reverse('get_sale_records', kwargs={'course_id': unicode(course_key)}), + 'get_sale_order_records_url': reverse('get_sale_order_records', kwargs={'course_id': unicode(course_key)}), + 'instructor_url': reverse('instructor_dashboard', kwargs={'course_id': unicode(course_key)}), + 'get_registration_code_csv_url': reverse('get_registration_codes', kwargs={'course_id': unicode(course_key)}), + 'generate_registration_code_csv_url': reverse('generate_registration_codes', kwargs={'course_id': unicode(course_key)}), + 'active_registration_code_csv_url': reverse('active_registration_codes', kwargs={'course_id': unicode(course_key)}), + 'spent_registration_code_csv_url': reverse('spent_registration_codes', kwargs={'course_id': unicode(course_key)}), + 'set_course_mode_url': reverse('set_course_mode_price', kwargs={'course_id': unicode(course_key)}), + 'download_coupon_codes_url': reverse('get_coupon_codes', kwargs={'course_id': unicode(course_key)}), 'coupons': coupons, 'course_price': course_price, 'total_amount': total_amount @@ -213,7 +213,7 @@ def _section_course_info(course, access): 'course_display_name': course.display_name, 'has_started': course.has_started(), 'has_ended': course.has_ended(), - 'list_instructor_tasks_url': reverse('list_instructor_tasks', kwargs={'course_id': course_key.to_deprecated_string()}), + 'list_instructor_tasks_url': reverse('list_instructor_tasks', kwargs={'course_id': unicode(course_key)}), } if settings.FEATURES.get('DISPLAY_ANALYTICS_ENROLLMENTS'): @@ -246,16 +246,17 @@ def _section_membership(course, access): 'section_key': 'membership', 'section_display_name': _('Membership'), '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()}), - 'list_forum_members_url': reverse('list_forum_members', kwargs={'course_id': course_key.to_deprecated_string()}), - 'update_forum_role_membership_url': reverse('update_forum_role_membership', kwargs={'course_id': course_key.to_deprecated_string()}), - 'cohorts_ajax_url': reverse('cohorts', kwargs={'course_key_string': course_key.to_deprecated_string()}), + 'enroll_button_url': reverse('students_update_enrollment', kwargs={'course_id': unicode(course_key)}), + 'unenroll_button_url': reverse('students_update_enrollment', kwargs={'course_id': unicode(course_key)}), + 'upload_student_csv_button_url': reverse('register_and_enroll_students', kwargs={'course_id': unicode(course_key)}), + 'modify_beta_testers_button_url': reverse('bulk_beta_modify_access', kwargs={'course_id': unicode(course_key)}), + 'list_course_role_members_url': reverse('list_course_role_members', kwargs={'course_id': unicode(course_key)}), + 'modify_access_url': reverse('modify_access', kwargs={'course_id': unicode(course_key)}), + 'list_forum_members_url': reverse('list_forum_members', kwargs={'course_id': unicode(course_key)}), + 'update_forum_role_membership_url': reverse('update_forum_role_membership', kwargs={'course_id': unicode(course_key)}), + 'cohorts_ajax_url': reverse('cohorts', kwargs={'course_key_string': unicode(course_key)}), 'advanced_settings_url': get_studio_url(course, 'settings/advanced'), + 'upload_cohorts_csv_url': reverse('add_users_to_cohorts', kwargs={'course_id': unicode(course_key)}), } return section_data @@ -280,12 +281,12 @@ def _section_student_admin(course, access): 'section_display_name': _('Student Admin'), 'access': access, 'is_small_course': is_small_course, - 'get_student_progress_url_url': reverse('get_student_progress_url', kwargs={'course_id': course_key.to_deprecated_string()}), - 'enrollment_url': reverse('students_update_enrollment', kwargs={'course_id': course_key.to_deprecated_string()}), - 'reset_student_attempts_url': reverse('reset_student_attempts', kwargs={'course_id': course_key.to_deprecated_string()}), - 'rescore_problem_url': reverse('rescore_problem', kwargs={'course_id': course_key.to_deprecated_string()}), - 'list_instructor_tasks_url': reverse('list_instructor_tasks', kwargs={'course_id': course_key.to_deprecated_string()}), - 'spoc_gradebook_url': reverse('spoc_gradebook', kwargs={'course_id': course_key.to_deprecated_string()}), + 'get_student_progress_url_url': reverse('get_student_progress_url', kwargs={'course_id': unicode(course_key)}), + 'enrollment_url': reverse('students_update_enrollment', kwargs={'course_id': unicode(course_key)}), + 'reset_student_attempts_url': reverse('reset_student_attempts', kwargs={'course_id': unicode(course_key)}), + 'rescore_problem_url': reverse('rescore_problem', kwargs={'course_id': unicode(course_key)}), + 'list_instructor_tasks_url': reverse('list_instructor_tasks', kwargs={'course_id': unicode(course_key)}), + 'spoc_gradebook_url': reverse('spoc_gradebook', kwargs={'course_id': unicode(course_key)}), } return section_data @@ -295,12 +296,12 @@ def _section_extensions(course): section_data = { 'section_key': 'extensions', 'section_display_name': _('Extensions'), - 'units_with_due_dates': [(title_or_url(unit), unit.location.to_deprecated_string()) + 'units_with_due_dates': [(title_or_url(unit), unicode(unit.location)) for unit in get_units_with_due_date(course)], - 'change_due_date_url': reverse('change_due_date', kwargs={'course_id': course.id.to_deprecated_string()}), - 'reset_due_date_url': reverse('reset_due_date', kwargs={'course_id': course.id.to_deprecated_string()}), - 'show_unit_extensions_url': reverse('show_unit_extensions', kwargs={'course_id': course.id.to_deprecated_string()}), - 'show_student_extensions_url': reverse('show_student_extensions', kwargs={'course_id': course.id.to_deprecated_string()}), + 'change_due_date_url': reverse('change_due_date', kwargs={'course_id': unicode(course.id)}), + 'reset_due_date_url': reverse('reset_due_date', kwargs={'course_id': unicode(course.id)}), + 'show_unit_extensions_url': reverse('show_unit_extensions', kwargs={'course_id': unicode(course.id)}), + 'show_student_extensions_url': reverse('show_student_extensions', kwargs={'course_id': unicode(course.id)}), } return section_data @@ -312,12 +313,12 @@ def _section_data_download(course, access): 'section_key': 'data_download', 'section_display_name': _('Data Download'), 'access': access, - 'get_grading_config_url': reverse('get_grading_config', kwargs={'course_id': course_key.to_deprecated_string()}), - 'get_students_features_url': reverse('get_students_features', kwargs={'course_id': course_key.to_deprecated_string()}), - 'get_anon_ids_url': reverse('get_anon_ids', kwargs={'course_id': course_key.to_deprecated_string()}), - 'list_instructor_tasks_url': reverse('list_instructor_tasks', kwargs={'course_id': course_key.to_deprecated_string()}), - 'list_report_downloads_url': reverse('list_report_downloads', kwargs={'course_id': course_key.to_deprecated_string()}), - 'calculate_grades_csv_url': reverse('calculate_grades_csv', kwargs={'course_id': course_key.to_deprecated_string()}), + 'get_grading_config_url': reverse('get_grading_config', kwargs={'course_id': unicode(course_key)}), + 'get_students_features_url': reverse('get_students_features', kwargs={'course_id': unicode(course_key)}), + 'get_anon_ids_url': reverse('get_anon_ids', kwargs={'course_id': unicode(course_key)}), + 'list_instructor_tasks_url': reverse('list_instructor_tasks', kwargs={'course_id': unicode(course_key)}), + 'list_report_downloads_url': reverse('list_report_downloads', kwargs={'course_id': unicode(course_key)}), + 'calculate_grades_csv_url': reverse('calculate_grades_csv', kwargs={'course_id': unicode(course_key)}), } return section_data @@ -335,8 +336,8 @@ def _section_send_email(course, access): fragment = course.system.render(html_module, 'studio_view') fragment = wrap_xblock( 'LmsRuntime', html_module, 'studio_view', fragment, None, - extra_data={"course-id": course_key.to_deprecated_string()}, - usage_id_serializer=lambda usage_id: quote_slashes(usage_id.to_deprecated_string()), + extra_data={"course-id": unicode(course_key)}, + usage_id_serializer=lambda usage_id: quote_slashes(unicode(usage_id)), # Generate a new request_token here at random, because this module isn't connected to any other # xblock rendering. request_token=uuid.uuid1().get_hex() @@ -346,16 +347,16 @@ def _section_send_email(course, access): 'section_key': 'send_email', 'section_display_name': _('Email'), 'access': access, - 'send_email': reverse('send_email', kwargs={'course_id': course_key.to_deprecated_string()}), + 'send_email': reverse('send_email', kwargs={'course_id': unicode(course_key)}), 'editor': email_editor, 'list_instructor_tasks_url': reverse( - 'list_instructor_tasks', kwargs={'course_id': course_key.to_deprecated_string()} + 'list_instructor_tasks', kwargs={'course_id': unicode(course_key)} ), 'email_background_tasks_url': reverse( - 'list_background_email_tasks', kwargs={'course_id': course_key.to_deprecated_string()} + 'list_background_email_tasks', kwargs={'course_id': unicode(course_key)} ), 'email_content_history_url': reverse( - 'list_email_content', kwargs={'course_id': course_key.to_deprecated_string()} + 'list_email_content', kwargs={'course_id': unicode(course_key)} ), } return section_data @@ -376,8 +377,8 @@ def _section_analytics(course, access): 'section_key': 'instructor_analytics', 'section_display_name': _('Analytics'), 'access': access, - 'get_distribution_url': reverse('get_distribution', kwargs={'course_id': course_key.to_deprecated_string()}), - 'proxy_legacy_analytics_url': reverse('proxy_legacy_analytics', kwargs={'course_id': course_key.to_deprecated_string()}), + 'get_distribution_url': reverse('get_distribution', kwargs={'course_id': unicode(course_key)}), + 'proxy_legacy_analytics_url': reverse('proxy_legacy_analytics', kwargs={'course_id': unicode(course_key)}), } if settings.ANALYTICS_DASHBOARD_URL: @@ -395,7 +396,7 @@ def _section_metrics(course, access): 'section_key': 'metrics', 'section_display_name': _('Metrics'), 'access': access, - 'course_id': course_key.to_deprecated_string(), + 'course_id': unicode(course_key), 'sub_section_display_name': get_section_display_name(course_key), 'section_has_problem': get_array_section_has_problem(course_key), 'get_students_opened_subsection_url': reverse('get_students_opened_subsection'), diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index 219df82899..5f18c86342 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -13,12 +13,15 @@ from celery.states import READY_STATES from xmodule.modulestore.django import modulestore from instructor_task.models import InstructorTask -from instructor_task.tasks import (rescore_problem, - reset_problem_attempts, - delete_problem_state, - send_bulk_course_email, - calculate_grades_csv, - calculate_students_features_csv) +from instructor_task.tasks import ( + rescore_problem, + reset_problem_attempts, + delete_problem_state, + send_bulk_course_email, + calculate_grades_csv, + calculate_students_features_csv, + cohort_students, +) from instructor_task.api_helper import (check_arguments_for_rescoring, encode_problem_and_student_input, @@ -233,3 +236,17 @@ def submit_calculate_students_features_csv(request, course_key, features): task_key = "" return submit_task(request, task_type, task_class, course_key, task_input, task_key) + + +def submit_cohort_students(request, course_key, file_name): + """ + Request to have students cohorted in bulk. + + Raises AlreadyRunningError if students are currently being cohorted. + """ + task_type = 'cohort_students' + task_class = cohort_students + task_input = {'file_name': file_name} + task_key = "" + + return submit_task(request, task_type, task_class, course_key, task_input, task_key) diff --git a/lms/djangoapps/instructor_task/models.py b/lms/djangoapps/instructor_task/models.py index e23f2ad1da..32415185fb 100644 --- a/lms/djangoapps/instructor_task/models.py +++ b/lms/djangoapps/instructor_task/models.py @@ -238,6 +238,7 @@ class S3ReportStore(ReportStore): settings.AWS_ACCESS_KEY_ID, settings.AWS_SECRET_ACCESS_KEY ) + self.bucket = conn.get_bucket(bucket_name) @classmethod @@ -327,13 +328,10 @@ class S3ReportStore(ReportStore): can be plugged straight into an href """ course_dir = self.key_for(course_id, '') - return sorted( - [ - (key.key.split("/")[-1], key.generate_url(expires_in=300)) - for key in self.bucket.list(prefix=course_dir.key) - ], - reverse=True - ) + return [ + (key.key.split("/")[-1], key.generate_url(expires_in=300)) + for key in sorted(self.bucket.list(prefix=course_dir.key), reverse=True, key=lambda k: k.last_modified) + ] class LocalFSReportStore(ReportStore): @@ -410,10 +408,10 @@ class LocalFSReportStore(ReportStore): course_dir = self.path_to(course_id, '') if not os.path.exists(course_dir): return [] - return sorted( - [ - (filename, ("file://" + urllib.quote(os.path.join(course_dir, filename)))) - for filename in os.listdir(course_dir) - ], - reverse=True - ) + files = [(filename, os.path.join(course_dir, filename)) for filename in os.listdir(course_dir)] + files.sort(key=lambda (filename, full_path): os.path.getmtime(full_path), reverse=True) + + return [ + (filename, ("file://" + urllib.quote(full_path))) + for filename, full_path in files + ] diff --git a/lms/djangoapps/instructor_task/tasks.py b/lms/djangoapps/instructor_task/tasks.py index 518d71da0a..76c8998090 100644 --- a/lms/djangoapps/instructor_task/tasks.py +++ b/lms/djangoapps/instructor_task/tasks.py @@ -31,7 +31,8 @@ from instructor_task.tasks_helper import ( reset_attempts_module_state, delete_problem_module_state, upload_grades_csv, - upload_students_csv + upload_students_csv, + cohort_students_and_upload ) from bulk_email.tasks import perform_delegate_email_batches @@ -153,3 +154,15 @@ def calculate_students_features_csv(entry_id, xmodule_instance_args): action_name = ugettext_noop('generated') task_fn = partial(upload_students_csv, xmodule_instance_args) return run_main_task(entry_id, task_fn, action_name) + + +@task(base=BaseInstructorTask) # pylint: disable=E1102 +def cohort_students(entry_id, xmodule_instance_args): + """ + Cohort students in bulk, and upload the results. + """ + # Translators: This is a past-tense verb that is inserted into task progress messages as {action}. + # An example of such a message is: "Progress: {action} {succeeded} of {attempted} so far" + action_name = ugettext_noop('cohorted') + task_fn = partial(cohort_students_and_upload, xmodule_instance_args) + return run_main_task(entry_id, task_fn, action_name) diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index d712e824b2..70a5e2d42d 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -7,18 +7,23 @@ import json import urllib from datetime import datetime from time import time +import unicodecsv from celery import Task, current_task from celery.utils.log import get_task_logger from celery.states import SUCCESS, FAILURE from django.contrib.auth.models import User +from django.core.files.storage import DefaultStorage from django.db import transaction, reset_queries import dogstats_wrapper as dog_stats_api from pytz import UTC -from xmodule.modulestore.django import modulestore from track.views import task_track +from util.file import course_filename_prefix_generator, UniversalNewlineIterator +from xmodule.modulestore.django import modulestore +from openedx.core.djangoapps.course_groups.models import CourseUserGroup +from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort from courseware.grades import iterate_grades_for from courseware.models import StudentModule from courseware.model_data import FieldDataCache @@ -515,7 +520,7 @@ def upload_csv_to_report_store(rows, csv_name, course_id, timestamp): report_store.store_rows( course_id, u"{course_prefix}_{csv_name}_{timestamp_str}.csv".format( - course_prefix=urllib.quote(unicode(course_id).replace("/", "_")), + course_prefix=course_filename_prefix_generator(course_id), csv_name=csv_name, timestamp_str=timestamp.strftime("%Y-%m-%d-%H%M") ), @@ -624,3 +629,88 @@ def upload_students_csv(_xmodule_instance_args, _entry_id, course_id, task_input upload_csv_to_report_store(rows, 'student_profile_info', course_id, start_date) return task_progress.update_task_state(extra_meta=current_step) + + +def cohort_students_and_upload(_xmodule_instance_args, _entry_id, course_id, task_input, action_name): + """ + Within a given course, cohort students in bulk, then upload the results + using a `ReportStore`. + """ + start_time = time() + start_date = datetime.now(UTC) + + # Iterate through rows to get total assignments for task progress + with DefaultStorage().open(task_input['file_name']) as f: + total_assignments = 0 + for _line in unicodecsv.DictReader(UniversalNewlineIterator(f)): + total_assignments += 1 + + task_progress = TaskProgress(action_name, total_assignments, start_time) + current_step = {'step': 'Cohorting Students'} + task_progress.update_task_state(extra_meta=current_step) + + # cohorts_status is a mapping from cohort_name to metadata about + # that cohort. The metadata will include information about users + # successfully added to the cohort, users not found, and a cached + # reference to the corresponding cohort object to prevent + # redundant cohort queries. + cohorts_status = {} + + with DefaultStorage().open(task_input['file_name']) as f: + for row in unicodecsv.DictReader(UniversalNewlineIterator(f), encoding='utf-8'): + # Try to use the 'email' field to identify the user. If it's not present, use 'username'. + username_or_email = row.get('email') or row.get('username') + cohort_name = row.get('cohort') or '' + task_progress.attempted += 1 + + if not cohorts_status.get(cohort_name): + cohorts_status[cohort_name] = { + 'Cohort Name': cohort_name, + 'Students Added': 0, + 'Students Not Found': set() + } + try: + cohorts_status[cohort_name]['cohort'] = CourseUserGroup.objects.get( + course_id=course_id, + group_type=CourseUserGroup.COHORT, + name=cohort_name + ) + cohorts_status[cohort_name]["Exists"] = True + except CourseUserGroup.DoesNotExist: + cohorts_status[cohort_name]["Exists"] = False + + if not cohorts_status[cohort_name]['Exists']: + task_progress.failed += 1 + continue + + try: + with transaction.commit_on_success(): + add_user_to_cohort(cohorts_status[cohort_name]['cohort'], username_or_email) + cohorts_status[cohort_name]['Students Added'] += 1 + task_progress.succeeded += 1 + except User.DoesNotExist: + cohorts_status[cohort_name]['Students Not Found'].add(username_or_email) + task_progress.failed += 1 + except ValueError: + # Raised when the user is already in the given cohort + task_progress.skipped += 1 + + task_progress.update_task_state(extra_meta=current_step) + + current_step['step'] = 'Uploading CSV' + task_progress.update_task_state(extra_meta=current_step) + + # Filter the output of `add_users_to_cohorts` in order to upload the result. + output_header = ['Cohort Name', 'Exists', 'Students Added', 'Students Not Found'] + output_rows = [ + [ + ','.join(status_dict.get(column_name, '')) if column_name == 'Students Not Found' + else status_dict[column_name] + for column_name in output_header + ] + for _cohort_name, status_dict in cohorts_status.iteritems() + ] + output_rows.insert(0, output_header) + upload_csv_to_report_store(output_rows, 'cohort_results', course_id, start_date) + + return task_progress.update_task_state(extra_meta=current_step) diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index 961c3b3dc0..81a63527d4 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -14,6 +14,7 @@ from instructor_task.api import ( submit_delete_problem_state_for_all_students, submit_bulk_course_email, submit_calculate_students_features_csv, + submit_cohort_students, ) from instructor_task.api_helper import AlreadyRunningError @@ -201,3 +202,11 @@ class InstructorTaskCourseSubmitTest(TestReportMixin, InstructorTaskCourseTestCa features=[] ) self._test_resubmission(api_call) + + def test_submit_cohort_students(self): + api_call = lambda: submit_cohort_students( + self.create_task_request(self.instructor), + self.course.id, + file_name=u'filename.csv' + ) + self._test_resubmission(api_call) diff --git a/lms/djangoapps/instructor_task/tests/test_base.py b/lms/djangoapps/instructor_task/tests/test_base.py index 1ce6eff9e8..bdc89cb0b8 100644 --- a/lms/djangoapps/instructor_task/tests/test_base.py +++ b/lms/djangoapps/instructor_task/tests/test_base.py @@ -6,6 +6,7 @@ import os import json from mock import Mock import shutil +import unicodecsv from uuid import uuid4 from celery.states import SUCCESS, FAILURE @@ -26,7 +27,7 @@ from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from instructor_task.api_helper import encode_problem_and_student_input -from instructor_task.models import PROGRESS, QUEUING +from instructor_task.models import PROGRESS, QUEUING, ReportStore from instructor_task.tests.factories import InstructorTaskFactory from instructor_task.views import instructor_task_status @@ -246,3 +247,27 @@ class TestReportMixin(object): reports_download_path = settings.GRADES_DOWNLOAD['ROOT_PATH'] if os.path.exists(reports_download_path): shutil.rmtree(reports_download_path) + + def verify_rows_in_csv(self, expected_rows, verify_order=True): + """ + Verify that the last ReportStore CSV contains the expected content. + + Arguments: + expected_rows (iterable): An iterable of dictionaries, + where each dict represents a row of data in the last + ReportStore CSV. Each dict maps keys from the CSV + header to values in that row's corresponding cell. + verify_order (boolean): When True, we verify that both the + content and order of `expected_rows` matches the + actual csv rows. When False (default), we only verify + that the content matches. + """ + report_store = ReportStore.from_config() + report_csv_filename = report_store.links_for(self.course.id)[0][0] + with open(report_store.path_to(self.course.id, report_csv_filename)) as csv_file: + # Expand the dict reader generator so we don't lose it's content + csv_rows = [row for row in unicodecsv.DictReader(csv_file)] + if verify_order: + self.assertEqual(csv_rows, expected_rows) + else: + self.assertItemsEqual(csv_rows, expected_rows) diff --git a/lms/djangoapps/instructor_task/tests/test_integration.py b/lms/djangoapps/instructor_task/tests/test_integration.py index eee8f124d3..68c1a44cbc 100644 --- a/lms/djangoapps/instructor_task/tests/test_integration.py +++ b/lms/djangoapps/instructor_task/tests/test_integration.py @@ -5,7 +5,6 @@ Runs tasks on answers to course problems to validate that code paths actually work. """ -import csv import json import logging from mock import patch @@ -28,7 +27,7 @@ from instructor_task.api import (submit_rescore_problem_for_all_students, submit_rescore_problem_for_student, submit_reset_problem_attempts_for_all_students, submit_delete_problem_state_for_all_students) -from instructor_task.models import InstructorTask, ReportStore +from instructor_task.models import InstructorTask from instructor_task.tasks_helper import upload_grades_csv from instructor_task.tests.test_base import (InstructorTaskModuleTestCase, TestReportMixin, TEST_COURSE_ORG, TEST_COURSE_NUMBER, OPTION_1, OPTION_2) @@ -602,23 +601,6 @@ class TestGradeReportConditionalContent(TestReportMixin, TestIntegrationTask): """ self.assertDictContainsSubset({'attempted': 2, 'succeeded': 2, 'failed': 0}, task_result) - def verify_rows_in_csv(self, expected_rows): - """ - Verify that the grades CSV contains the expected content. - - Arguments: - expected_rows (iterable): An iterable of dictionaries, where - each dict represents a row of data in the grades - report CSV. Each dict maps keys from the CSV header - to values in that row's corresponding cell. - """ - report_store = ReportStore.from_config() - report_csv_filename = report_store.links_for(self.course.id)[0][0] - with open(report_store.path_to(self.course.id, report_csv_filename)) as csv_file: - # Expand the dict reader generator so we don't lose it's content - csv_rows = [row for row in csv.DictReader(csv_file)] - self.assertEqual(csv_rows, expected_rows) - def verify_grades_in_csv(self, students_grades): """ Verify that the grades CSV contains the expected grades data. diff --git a/lms/djangoapps/instructor_task/tests/test_models.py b/lms/djangoapps/instructor_task/tests/test_models.py new file mode 100644 index 0000000000..f681b71f85 --- /dev/null +++ b/lms/djangoapps/instructor_task/tests/test_models.py @@ -0,0 +1,107 @@ +""" +Tests for instructor_task/models.py. +""" + +from cStringIO import StringIO +import mock +import time +from datetime import datetime +from unittest import TestCase + +from instructor_task.models import LocalFSReportStore, S3ReportStore +from instructor_task.tests.test_base import TestReportMixin +from opaque_keys.edx.locator import CourseLocator + + +class MockKey(object): + """ + Mocking a boto S3 Key object. + """ + def __init__(self, bucket): + self.last_modified = datetime.now() + self.bucket = bucket + + def set_contents_from_string(self, contents, headers): # pylint: disable=unused-argument + """ Expected method on a Key object. """ + self.bucket.store_key(self) + + def generate_url(self, expires_in): # pylint: disable=unused-argument + """ Expected method on a Key object. """ + return "http://fake-edx-s3.edx.org/" + + +class MockBucket(object): + """ Mocking a boto S3 Bucket object. """ + def __init__(self, _name): + self.keys = [] + + def store_key(self, key): + """ Not a Bucket method, created just to store the keys in the Bucket for testing purposes. """ + self.keys.append(key) + + def list(self, prefix): # pylint: disable=unused-argument + """ Expected method on a Bucket object. """ + return self.keys + + +class MockS3Connection(object): + """ Mocking a boto S3 Connection """ + def __init__(self, access_key, secret_key): + pass + + def get_bucket(self, bucket_name): + """ Expected method on an S3Connection object. """ + return MockBucket(bucket_name) + + +class ReportStoreTestMixin(object): + """ + Mixin for report store tests. + """ + def setUp(self): + self.course_id = CourseLocator(org="testx", course="coursex", run="runx") + + def create_report_store(self): + """ + Subclasses should override this and return their report store. + """ + pass + + def test_links_for_order(self): + """ + Test that ReportStore.links_for() returns file download links + in reverse chronological order. + """ + report_store = self.create_report_store() + report_store.store(self.course_id, 'old_file', StringIO()) + time.sleep(1) # Ensure we have a unique timestamp. + report_store.store(self.course_id, 'middle_file', StringIO()) + time.sleep(1) # Ensure we have a unique timestamp. + report_store.store(self.course_id, 'new_file', StringIO()) + + self.assertEqual( + [link[0] for link in report_store.links_for(self.course_id)], + ['new_file', 'middle_file', 'old_file'] + ) + + +class LocalFSReportStoreTestCase(ReportStoreTestMixin, TestReportMixin, TestCase): + """ + Test the LocalFSReportStore model. + """ + def create_report_store(self): + """ Create and return a LocalFSReportStore. """ + return LocalFSReportStore.from_config() + + +@mock.patch('instructor_task.models.S3Connection', new=MockS3Connection) +@mock.patch('instructor_task.models.Key', new=MockKey) +@mock.patch('instructor_task.models.settings.AWS_SECRET_ACCESS_KEY', create=True, new="access_key") +@mock.patch('instructor_task.models.settings.AWS_ACCESS_KEY_ID', create=True, new="access_id") +class S3ReportStoreTestCase(ReportStoreTestMixin, TestReportMixin, TestCase): + """ + Test the S3ReportStore model. + """ + def create_report_store(self): + """ Create and return a S3ReportStore. """ + return S3ReportStore.from_config() diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 027e132cc9..7c324be9c8 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -6,16 +6,13 @@ Tests that CSV grade report generation works with unicode emails. """ import ddt from mock import Mock, patch +import tempfile -from django.test.testcases import TestCase - -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from student.tests.factories import CourseEnrollmentFactory, UserFactory - +from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory from instructor_task.models import ReportStore -from instructor_task.tasks_helper import upload_grades_csv, upload_students_csv +from instructor_task.tasks_helper import cohort_students_and_upload, upload_grades_csv, upload_students_csv from instructor_task.tests.test_base import InstructorTaskCourseTestCase, TestReportMixin @@ -104,3 +101,237 @@ class TestStudentReport(TestReportMixin, InstructorTaskCourseTestCase): #This assertion simply confirms that the generation completed with no errors num_students = len(students) self.assertDictContainsSubset({'attempted': num_students, 'succeeded': num_students, 'failed': 0}, result) + + +class MockDefaultStorage(object): + """Mock django's DefaultStorage""" + def __init__(self): + pass + + def open(self, file_name): + """Mock out DefaultStorage.open with standard python open""" + return open(file_name) + + +@patch('instructor_task.tasks_helper.DefaultStorage', new=MockDefaultStorage) +class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase): + """ + Tests that bulk student cohorting works. + """ + def setUp(self): + self.course = CourseFactory.create() + self.cohort_1 = CohortFactory(course_id=self.course.id, name='Cohort 1') + self.cohort_2 = CohortFactory(course_id=self.course.id, name='Cohort 2') + self.student_1 = self.create_student(username=u'student_1\xec', email='student_1@example.com') + self.student_2 = self.create_student(username='student_2', email='student_2@example.com') + self.csv_header_row = ['Cohort Name', 'Exists', 'Students Added', 'Students Not Found'] + + def _cohort_students_and_upload(self, csv_data): + """ + Call `cohort_students_and_upload` with a file generated from `csv_data`. + """ + with tempfile.NamedTemporaryFile() as temp_file: + temp_file.write(csv_data.encode('utf-8')) + temp_file.flush() + with patch('instructor_task.tasks_helper._get_current_task'): + return cohort_students_and_upload(None, None, self.course.id, {'file_name': temp_file.name}, 'cohorted') + + def test_username(self): + result = self._cohort_students_and_upload( + u'username,email,cohort\n' + u'student_1\xec,,Cohort 1\n' + u'student_2,,Cohort 2' + ) + self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result) + self.verify_rows_in_csv( + [ + dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])), + dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])), + ], + verify_order=False + ) + + def test_email(self): + result = self._cohort_students_and_upload( + 'username,email,cohort\n' + ',student_1@example.com,Cohort 1\n' + ',student_2@example.com,Cohort 2' + ) + self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result) + self.verify_rows_in_csv( + [ + dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])), + dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])), + ], + verify_order=False + ) + + def test_username_and_email(self): + result = self._cohort_students_and_upload( + u'username,email,cohort\n' + u'student_1\xec,student_1@example.com,Cohort 1\n' + u'student_2,student_2@example.com,Cohort 2' + ) + self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result) + self.verify_rows_in_csv( + [ + dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])), + dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])), + ], + verify_order=False + ) + + def test_prefer_email(self): + """ + Test that `cohort_students_and_upload` greedily prefers 'email' over + 'username' when identifying the user. This means that if a correct + email is present, an incorrect or non-matching username will simply be + ignored. + """ + result = self._cohort_students_and_upload( + u'username,email,cohort\n' + u'student_1\xec,student_1@example.com,Cohort 1\n' # valid username and email + u'Invalid,student_2@example.com,Cohort 2' # invalid username, valid email + ) + self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result) + self.verify_rows_in_csv( + [ + dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])), + dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])), + ], + verify_order=False + ) + + def test_non_existent_user(self): + result = self._cohort_students_and_upload( + 'username,email,cohort\n' + 'Invalid,,Cohort 1\n' + 'student_2,also_fake@bad.com,Cohort 2' + ) + self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 0, 'failed': 2}, result) + self.verify_rows_in_csv( + [ + dict(zip(self.csv_header_row, ['Cohort 1', 'True', '0', 'Invalid'])), + dict(zip(self.csv_header_row, ['Cohort 2', 'True', '0', 'also_fake@bad.com'])), + ], + verify_order=False + ) + + def test_non_existent_cohort(self): + result = self._cohort_students_and_upload( + 'username,email,cohort\n' + ',student_1@example.com,Does Not Exist\n' + 'student_2,,Cohort 2' + ) + self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 1, 'failed': 1}, result) + self.verify_rows_in_csv( + [ + dict(zip(self.csv_header_row, ['Does Not Exist', 'False', '0', ''])), + dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])), + ], + verify_order=False + ) + + def test_too_few_commas(self): + """ + A CSV file may be malformed and lack traling commas at the end of a row. + In this case, those cells take on the value None by the CSV parser. + Make sure we handle None values appropriately. + + i.e.: + header_1,header_2,header_3 + val_1,val_2,val_3 <- good row + val_1,, <- good row + val_1 <- bad row; no trailing commas to indicate empty rows + """ + result = self._cohort_students_and_upload( + u'username,email,cohort\n' + u'student_1\xec,\n' + u'student_2' + ) + self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 0, 'failed': 2}, result) + self.verify_rows_in_csv( + [ + dict(zip(self.csv_header_row, ['', 'False', '0', ''])), + ], + verify_order=False + ) + + def test_only_header_row(self): + result = self._cohort_students_and_upload( + u'username,email,cohort' + ) + self.assertDictContainsSubset({'total': 0, 'attempted': 0, 'succeeded': 0, 'failed': 0}, result) + self.verify_rows_in_csv([]) + + def test_carriage_return(self): + """ + Test that we can handle carriage returns in our file. + """ + result = self._cohort_students_and_upload( + u'username,email,cohort\r' + u'student_1\xec,,Cohort 1\r' + u'student_2,,Cohort 2' + ) + self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result) + self.verify_rows_in_csv( + [ + dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])), + dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])), + ], + verify_order=False + ) + + def test_carriage_return_line_feed(self): + """ + Test that we can handle carriage returns and line feeds in our file. + """ + result = self._cohort_students_and_upload( + u'username,email,cohort\r\n' + u'student_1\xec,,Cohort 1\r\n' + u'student_2,,Cohort 2' + ) + self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result) + self.verify_rows_in_csv( + [ + dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])), + dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])), + ], + verify_order=False + ) + + def test_move_users_to_new_cohort(self): + self.cohort_1.users.add(self.student_1) + self.cohort_2.users.add(self.student_2) + + result = self._cohort_students_and_upload( + u'username,email,cohort\n' + u'student_1\xec,,Cohort 2\n' + u'student_2,,Cohort 1' + ) + self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result) + self.verify_rows_in_csv( + [ + dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])), + dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])), + ], + verify_order=False + ) + + def test_move_users_to_same_cohort(self): + self.cohort_1.users.add(self.student_1) + self.cohort_2.users.add(self.student_2) + + result = self._cohort_students_and_upload( + u'username,email,cohort\n' + u'student_1\xec,,Cohort 1\n' + u'student_2,,Cohort 2' + ) + self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'skipped': 2, 'failed': 0}, result) + self.verify_rows_in_csv( + [ + dict(zip(self.csv_header_row, ['Cohort 1', 'True', '0', ''])), + dict(zip(self.csv_header_row, ['Cohort 2', 'True', '0', ''])), + ], + verify_order=False + ) diff --git a/lms/envs/bok_choy.py b/lms/envs/bok_choy.py index fbb2d6ef70..af3b992808 100644 --- a/lms/envs/bok_choy.py +++ b/lms/envs/bok_choy.py @@ -42,6 +42,17 @@ update_module_store_settings( default_store=os.environ.get('DEFAULT_STORE', 'draft'), ) +############################ STATIC FILES ############################# +DEFAULT_FILE_STORAGE = 'django.core.files.storage.FileSystemStorage' +MEDIA_ROOT = TEST_ROOT / "uploads" +MEDIA_URL = "/static/uploads/" + +################################# CELERY ###################################### + +CELERY_ALWAYS_EAGER = True +CELERY_RESULT_BACKEND = 'cache' +BROKER_TRANSPORT = 'memory' + ###################### Grade Downloads ###################### GRADES_DOWNLOAD = { 'STORAGE_TYPE': 'localfs', diff --git a/lms/static/js/spec/main.js b/lms/static/js/spec/main.js index d177124b9c..3242093e3d 100644 --- a/lms/static/js/spec/main.js +++ b/lms/static/js/spec/main.js @@ -60,6 +60,7 @@ 'js/staff_debug_actions': 'js/staff_debug_actions', // Backbone classes loaded explicitly until they are converted to use RequireJS + 'js/views/file_uploader': 'js/views/file_uploader', 'js/models/cohort': 'js/models/cohort', 'js/collections/cohort': 'js/collections/cohort', 'js/views/cohort_editor': 'js/views/cohort_editor', @@ -82,7 +83,8 @@ exports: 'gettext' }, 'string_utils': { - deps: ['underscore'] + deps: ['underscore'], + exports: 'interpolate_text' }, 'date': { exports: 'Date' @@ -283,7 +285,9 @@ }, 'js/views/cohorts': { exports: 'CohortsView', - deps: ['backbone', 'js/views/cohort_editor'] + deps: ['jquery', 'underscore', 'backbone', 'gettext', 'string_utils', 'js/views/cohort_editor', + 'js/views/notification', 'js/models/notification', 'js/views/file_uploader' + ] }, 'js/models/notification': { exports: 'NotificationModel', @@ -293,6 +297,12 @@ exports: 'NotificationView', deps: ['backbone', 'jquery', 'underscore'] }, + 'js/views/file_uploader': { + exports: 'FileUploaderView', + deps: ['backbone', 'jquery', 'underscore', 'gettext', 'string_utils', 'js/views/notification', + 'js/models/notification', 'jquery.fileupload' + ] + }, 'js/student_account/enrollment': { exports: 'edx.student.account.EnrollmentInterface', deps: ['jquery', 'jquery.cookie'] @@ -385,6 +395,7 @@ 'lms/include/js/spec/photocapture_spec.js', 'lms/include/js/spec/staff_debug_actions_spec.js', 'lms/include/js/spec/views/notification_spec.js', + 'lms/include/js/spec/views/file_uploader_spec.js', 'lms/include/js/spec/dashboard/donation.js', 'lms/include/js/spec/shoppingcart/shoppingcart_spec.js', 'lms/include/js/spec/student_account/account_spec.js', diff --git a/lms/static/js/spec/views/cohorts_spec.js b/lms/static/js/spec/views/cohorts_spec.js index 0bf1a1a5d6..d3ff4cbfbb 100644 --- a/lms/static/js/spec/views/cohorts_spec.js +++ b/lms/static/js/spec/views/cohorts_spec.js @@ -28,7 +28,8 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe cohorts.url = '/mock_service'; requests = AjaxHelpers.requests(test); cohortsView = new CohortsView({ - model: cohorts + model: cohorts, + upload_cohorts_csv_url: "http://upload-csv-file-url/" }); cohortsView.render(); if (initialCohortID) { @@ -91,12 +92,13 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe }; beforeEach(function () { - setFixtures("
"); + setFixtures('
'); TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/cohorts'); TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/add-cohort-form'); TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/cohort-selector'); TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/cohort-editor'); TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/notification'); + TemplateHelpers.installTemplate('templates/file-upload'); }); it("Show an error if no cohorts are defined", function() { @@ -106,6 +108,18 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe 'warning', 'Add Cohort Group' ); + + // If no cohorts have been created, can't upload a CSV file. + expect(cohortsView.$('.wrapper-cohort-supplemental')).toHaveClass('is-hidden'); + }); + + it("Syncs data when membership tab is clicked", function() { + createCohortsView(this, 1); + verifyHeader(1, 'Cat Lovers', catLoversInitialCount); + $(cohortsView.getSectionCss("membership")).click(); + AjaxHelpers.expectRequest(requests, 'GET', '/mock_service'); + respondToRefresh(1001, 2); + verifyHeader(1, 'Cat Lovers', 1001); }); describe("Cohort Selector", function () { @@ -115,6 +129,34 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe expect(cohortsView.$('.cohort-management-group-header .title-value').text()).toBe(''); }); + it('can upload a CSV of cohort assignments if a cohort exists', function () { + var uploadCsvToggle, fileUploadForm, fileUploadFormCss='#file-upload-form'; + + createCohortsView(this); + + // Should see the control to toggle CSV file upload. + expect(cohortsView.$('.wrapper-cohort-supplemental')).not.toHaveClass('is-hidden'); + // But upload form should not be visible until toggle is clicked. + expect(cohortsView.$(fileUploadFormCss).length).toBe(0); + uploadCsvToggle = cohortsView.$('.toggle-cohort-management-secondary'); + expect(uploadCsvToggle.text()). + toContain('Assign students to cohort groups by uploading a CSV file'); + uploadCsvToggle.click(); + // After toggle is clicked, it should be hidden. + expect(uploadCsvToggle).toHaveClass('is-hidden'); + + fileUploadForm = cohortsView.$(fileUploadFormCss); + expect(fileUploadForm.length).toBe(1); + cohortsView.$(fileUploadForm).fileupload('add', {files: [{name: 'upload_file.txt'}]}); + cohortsView.$('.submit-file-button').click(); + + // No file will actually be uploaded because "uploaded_file.txt" doesn't actually exist. + AjaxHelpers.expectRequest(requests, 'POST', "http://upload-csv-file-url/", new FormData()); + AjaxHelpers.respondWithJson(requests, {}); + expect(cohortsView.$('.file-upload-form-result .message-confirmation .message-title').text().trim()) + .toBe("Your file 'upload_file.txt' has been uploaded. Please allow a few minutes for processing."); + }); + it('can select a cohort', function () { createCohortsView(this, 1); verifyHeader(1, 'Cat Lovers', catLoversInitialCount); diff --git a/lms/static/js/spec/views/file_uploader_spec.js b/lms/static/js/spec/views/file_uploader_spec.js new file mode 100644 index 0000000000..d70c552ac4 --- /dev/null +++ b/lms/static/js/spec/views/file_uploader_spec.js @@ -0,0 +1,169 @@ +define(['backbone', 'jquery', 'js/views/file_uploader', 'js/common_helpers/template_helpers', + 'js/common_helpers/ajax_helpers', 'js/models/notification', 'string_utils'], + function (Backbone, $, FileUploaderView, TemplateHelpers, AjaxHelpers, NotificationModel) { + describe("FileUploaderView", function () { + var verifyTitle, verifyInputLabel, verifyInputTip, verifySubmitButton, verifyExtensions, verifyText, + verifyFileUploadOption, verifyNotificationMessage, verifySubmitButtonEnabled, mimicUpload, + respondWithSuccess, respondWithError, fileUploaderView, url="http://test_url/"; + + verifyText = function (css, expectedText) { + expect(fileUploaderView.$(css).text().trim()).toBe(expectedText); + }; + + verifyTitle = function (expectedTitle) { verifyText('.form-title', expectedTitle); }; + + verifyInputLabel = function (expectedLabel) { verifyText('.field-label', expectedLabel); }; + + verifyInputTip = function (expectedTip) { verifyText('.tip', expectedTip); }; + + verifySubmitButton = function (expectedButton) { verifyText('.submit-file-button', expectedButton); }; + + verifyExtensions = function (expectedExtensions) { + var acceptAttribute = fileUploaderView.$('input.input-file').attr('accept'); + if (expectedExtensions) { + expect(acceptAttribute).toBe(expectedExtensions); + } + else { + expect(acceptAttribute).toBe(undefined); + } + }; + + verifySubmitButtonEnabled = function (expectedEnabled) { + var submitButton = fileUploaderView.$('.submit-file-button'); + if (expectedEnabled) { + expect(submitButton).not.toHaveClass("is-disabled"); + } + else { + expect(submitButton).toHaveClass("is-disabled"); + } + }; + + verifyFileUploadOption = function (option, expectedValue) { + expect(fileUploaderView.$('#file-upload-form').fileupload('option', option)).toBe(expectedValue); + }; + + verifyNotificationMessage = function (expectedMessage, type) { + verifyText('.file-upload-form-result .message-' + type + ' .message-title', expectedMessage); + }; + + mimicUpload = function (test) { + var requests = AjaxHelpers.requests(test); + + var param = {files: [{name: 'upload_file.txt'}]}; + fileUploaderView.$('#file-upload-form').fileupload('add', param); + verifySubmitButtonEnabled(true); + fileUploaderView.$('.submit-file-button').click(); + + // No file will actually be uploaded because "uploaded_file.txt" doesn't actually exist. + AjaxHelpers.expectRequest(requests, 'POST', url, new FormData()); + return requests; + }; + + respondWithSuccess = function (requests) { + AjaxHelpers.respondWithJson(requests, {}); + }; + + respondWithError = function (requests, errorMessage) { + if (errorMessage) { + AjaxHelpers.respondWithError(requests, 500, {error: errorMessage}); + } + else { + AjaxHelpers.respondWithError(requests); + } + }; + + beforeEach(function () { + setFixtures("
"); + TemplateHelpers.installTemplate('templates/file-upload'); + TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/notification'); + fileUploaderView = new FileUploaderView({url: url}).render(); + }); + + it('has default values', function () { + verifyTitle(""); + verifyInputLabel(""); + verifyInputTip(""); + verifySubmitButton("Upload File"); + verifyExtensions(null); + verifySubmitButtonEnabled(false); + }); + + it ('can set text values and extensions', function () { + fileUploaderView = new FileUploaderView({ + title: "file upload title", + inputLabel: "test label", + inputTip: "test tip", + submitButtonText: "upload button text", + extensions: ".csv,.txt" + }).render(); + + verifyTitle("file upload title"); + verifyInputLabel("test label"); + verifyInputTip("test tip"); + verifySubmitButton("upload button text"); + verifyExtensions(".csv,.txt"); + }); + + it ('can store upload URL', function () { + expect(fileUploaderView.$('#file-upload-form').attr('action')).toBe(url); + }); + + it ('sets autoUpload to false', function () { + verifyFileUploadOption('autoUpload', false); + }); + + it ('sets replaceFileInput to false', function () { + verifyFileUploadOption('replaceFileInput', false); + }); + + it ('handles errors with default message', function () { + var requests = mimicUpload(this); + respondWithError(requests); + verifyNotificationMessage("Your upload of 'upload_file.txt' failed.", "error"); + }); + + it ('handles errors with custom message', function () { + fileUploaderView = new FileUploaderView({ + url: url, + errorNotification: function (file, event, data) { + var message = interpolate_text("Custom error for '{file}'", {file: file}); + return new NotificationModel({ + type: "customized", + title: message + }); + } + }).render(); + var requests = mimicUpload(this); + respondWithError(requests, "server error"); + verifyNotificationMessage("Custom error for 'upload_file.txt'", "customized"); + }); + + it ('handles server error message', function () { + var requests = mimicUpload(this); + respondWithError(requests, "server error"); + verifyNotificationMessage("server error", "error"); + }); + + it ('handles success with default message', function () { + var requests = mimicUpload(this); + respondWithSuccess(requests); + verifyNotificationMessage("Your upload of 'upload_file.txt' succeeded.", "confirmation"); + }); + + it ('handles success with custom message', function () { + fileUploaderView = new FileUploaderView({ + url: url, + successNotification: function (file, event, data) { + var message = interpolate_text("Custom success message for '{file}'", {file: file}); + return new NotificationModel({ + type: "customized", + title: message + }); + } + }).render(); + var requests = mimicUpload(this); + respondWithSuccess(requests); + verifyNotificationMessage("Custom success message for 'upload_file.txt'", "customized"); + }); + }); + }); diff --git a/lms/static/js/views/cohorts.js b/lms/static/js/views/cohorts.js index 9f9131f316..7d1d9627a6 100644 --- a/lms/static/js/views/cohorts.js +++ b/lms/static/js/views/cohorts.js @@ -1,4 +1,4 @@ -(function($, _, Backbone, gettext, interpolate_text, CohortEditorView, NotificationModel, NotificationView) { +(function($, _, Backbone, gettext, interpolate_text, CohortEditorView, NotificationModel, NotificationView, FileUploaderView) { var hiddenClass = 'is-hidden', disabledClass = 'is-disabled'; @@ -8,15 +8,26 @@ 'click .action-create': 'showAddCohortForm', 'click .action-cancel': 'cancelAddCohortForm', 'click .action-save': 'saveAddCohortForm', - 'click .link-cross-reference': 'showSection' + 'click .link-cross-reference': 'showSection', + 'click .toggle-cohort-management-secondary': 'showCsvUpload' }, initialize: function(options) { + var model = this.model; + this.template = _.template($('#cohorts-tpl').text()); this.selectorTemplate = _.template($('#cohort-selector-tpl').text()); this.addCohortFormTemplate = _.template($('#add-cohort-form-tpl').text()); this.advanced_settings_url = options.advanced_settings_url; - this.model.on('sync', this.onSync, this); + this.upload_cohorts_csv_url = options.upload_cohorts_csv_url; + model.on('sync', this.onSync, this); + + // Update cohort counts when the user clicks back on the membership tab + // (for example, after uploading a csv file of cohort assignments and then + // checking results on data download tab). + $(this.getSectionCss('membership')).click(function () { + model.fetch(); + }); }, render: function() { @@ -36,16 +47,20 @@ onSync: function() { var selectedCohort = this.lastSelectedCohortId && this.model.get(this.lastSelectedCohortId), - hasCohorts = this.model.length > 0; + hasCohorts = this.model.length > 0, + cohortNavElement = this.$('.cohort-management-nav'), + additionalCohortControlElement = this.$('.wrapper-cohort-supplemental'); this.hideAddCohortForm(); if (hasCohorts) { - this.$('.cohort-management-nav').removeClass(hiddenClass); + cohortNavElement.removeClass(hiddenClass); + additionalCohortControlElement.removeClass(hiddenClass); this.renderSelector(selectedCohort); if (selectedCohort) { this.showCohortEditor(selectedCohort); } } else { - this.$('.cohort-management-nav').addClass(hiddenClass); + cohortNavElement.addClass(hiddenClass); + additionalCohortControlElement.addClass(hiddenClass); this.showNotification({ type: 'warning', title: gettext('You currently have no cohort groups configured'), @@ -176,8 +191,41 @@ showSection: function(event) { event.preventDefault(); var section = $(event.currentTarget).data("section"); - $(".instructor-nav .nav-item a[data-section='" + section + "']").click(); + $(this.getSectionCss(section)).click(); $(window).scrollTop(0); + }, + + showCsvUpload: function(event) { + event.preventDefault(); + + $(event.currentTarget).addClass(hiddenClass); + var uploadElement = this.$('.csv-upload').removeClass(hiddenClass); + + if (!this.fileUploaderView) { + this.fileUploaderView = new FileUploaderView({ + el: uploadElement, + title: gettext("Assign students to cohort groups by uploading a CSV file."), + inputLabel: gettext("Choose a .csv file"), + inputTip: gettext("Only properly formatted .csv files will be accepted."), + submitButtonText: gettext("Upload File and Assign Students"), + extensions: ".csv", + url: this.upload_cohorts_csv_url, + successNotification: function (file, event, data) { + var message = interpolate_text(gettext( + "Your file '{file}' has been uploaded. Please allow a few minutes for processing." + ), {file: file}); + return new NotificationModel({ + type: "confirmation", + title: message + }); + } + }).render(); + } + }, + + getSectionCss: function (section) { + return ".instructor-nav .nav-item a[data-section='" + section + "']"; } + }); -}).call(this, $, _, Backbone, gettext, interpolate_text, CohortEditorView, NotificationModel, NotificationView); +}).call(this, $, _, Backbone, gettext, interpolate_text, CohortEditorView, NotificationModel, NotificationView, FileUploaderView); diff --git a/lms/static/js/views/file_uploader.js b/lms/static/js/views/file_uploader.js new file mode 100644 index 0000000000..528c905176 --- /dev/null +++ b/lms/static/js/views/file_uploader.js @@ -0,0 +1,126 @@ +/** + * A view for uploading a file. + * + * Currently only single-file upload is supported (to support multiple-file uploads, the HTML + * input must be changed to specify "multiple" and the notification messaging needs to be changed + * to support the display of multiple status messages). + * + * There is no associated model, but the view supports the following options: + * + * @param title, the title to display. + * @param inputLabel, a label that will be added for the file input field. Note that this label is only shown to + * screen readers. + * @param inputTip, a tooltip linked to the file input field. Can be used to state what sort of file can be uploaded. + * @param extensions, the allowed file extensions of the uploaded file, as a comma-separated string (ex, ".csv,.txt"). + * Some browsers will enforce that only files with these extensions can be uploaded, but others + * (for instance, Firefox), will not. By default, no extensions are specified and any file can be uploaded. + * @param submitButtonText, text to display on the submit button to upload the file. The default value for this is + * "Upload File". + * @param url, the url for posting the uploaded file. + * @param successNotification, optional callback that can return a success NotificationModel for display + * after a file was successfully uploaded. This method will be passed the uploaded file, event, and data. + * @param errorNotification, optional callback that can return a success NotificationModel for display + * after a file failed to upload. This method will be passed the attempted file, event, and data. + */ +(function (Backbone, $, _, gettext, interpolate_text, NotificationModel, NotificationView) { + // Requires JQuery-File-Upload. + var FileUploaderView = Backbone.View.extend({ + + initialize: function (options) { + this.template = _.template($('#file-upload-tpl').text()); + this.options = options; + }, + + render: function () { + var options = this.options, + get_option_with_default = function(option, default_value) { + var optionVal = options[option]; + return optionVal ? optionVal : default_value; + }, + submitButton, resultNotification; + + this.$el.html(this.template({ + title: get_option_with_default("title", ""), + inputLabel: get_option_with_default("inputLabel", ""), + inputTip: get_option_with_default("inputTip", ""), + extensions: get_option_with_default("extensions", ""), + submitButtonText: get_option_with_default("submitButtonText", gettext("Upload File")), + url: get_option_with_default("url", "") + })); + + submitButton = this.$el.find('.submit-file-button'); + resultNotification = this.$el.find('.result'), + + this.$el.find('#file-upload-form').fileupload({ + dataType: 'json', + type: 'POST', + done: this.successHandler.bind(this), + fail: this.errorHandler.bind(this), + autoUpload: false, + replaceFileInput: false, + add: function (e, data) { + var file = data.files[0]; + submitButton.removeClass("is-disabled"); + submitButton.unbind('click'); + submitButton.click(function (event) { + event.preventDefault(); + data.submit(); + }); + resultNotification.html(""); + } + }); + + return this; + }, + + successHandler: function (event, data) { + var file = data.files[0].name; + var notificationModel; + if (this.options.successNotification) { + notificationModel = this.options.successNotification(file, event, data); + } + else { + notificationModel = new NotificationModel({ + type: "confirmation", + title: interpolate_text(gettext("Your upload of '{file}' succeeded."), {file: file}) + }); + } + var notification = new NotificationView({ + el: this.$('.result'), + model: notificationModel + }); + notification.render(); + }, + + errorHandler: function (event, data) { + var file = data.files[0].name, message = null, jqXHR = data.response().jqXHR; + var notificationModel; + if (this.options.errorNotification) { + notificationModel = this.options.errorNotification(file, event, data); + } + else { + if (jqXHR.responseText) { + try { + message = JSON.parse(jqXHR.responseText).error; + } + catch (err) { + } + } + if (!message) { + message = interpolate_text(gettext("Your upload of '{file}' failed."), {file: file}); + } + notificationModel = new NotificationModel({ + type: "error", + title: message + }); + } + var notification = new NotificationView({ + el: this.$('.result'), + model: notificationModel + }); + notification.render(); + } + }); + + this.FileUploaderView = FileUploaderView; +}).call(this, Backbone, $, _, gettext, interpolate_text, NotificationModel, NotificationView); diff --git a/lms/static/js_test.yml b/lms/static/js_test.yml index 10f140a9e3..410c92032c 100644 --- a/lms/static/js_test.yml +++ b/lms/static/js_test.yml @@ -41,6 +41,8 @@ lib_paths: - xmodule_js/common_static/js/vendor/flot/jquery.flot.js - xmodule_js/common_static/js/vendor/CodeMirror/codemirror.js - xmodule_js/common_static/js/vendor/URI.min.js + - xmodule_js/common_static/js/vendor/jQuery-File-Upload/js/jquery.fileupload.js + - xmodule_js/common_static/js/vendor/jQuery-File-Upload/js/jquery.iframe-transport.js - xmodule_js/common_static/js/vendor/url.min.js - xmodule_js/common_static/coffee/src/jquery.immediateDescendents.js - xmodule_js/common_static/coffee/src/xblock @@ -76,6 +78,7 @@ fixture_paths: - templates/dashboard - templates/student_account - templates/student_profile + - templates/file-upload.underscore requirejs: paths: diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss index dfff5cb518..67c7652f6b 100644 --- a/lms/static/sass/course/instructor/_instructor_2.scss +++ b/lms/static/sass/course/instructor/_instructor_2.scss @@ -106,6 +106,19 @@ } } } + + // UI: visual dividers + .divider-lv0 { + border-top: ($baseline/5) solid $gray-l4; + } + + .divider-lv1 { + border-top: ($baseline/10) solid $gray-l4; + } + + .divider-lv2 { + border-top: ($baseline/20) solid $gray-l4; + } } // instructor dashboard 2 @@ -311,6 +324,12 @@ section.instructor-dashboard-content-2 { color: $gray; } } + + .subsection-title { + @extend %hd-lv5; + @extend %t-weight4; + margin-bottom: ($baseline/2); + } } @@ -434,7 +453,7 @@ section.instructor-dashboard-content-2 { .tip { @extend %t-copy-sub1; margin-top: ($baseline/4); - color: $gray-l3; + color: $gray-l2; } .field-text { @@ -449,6 +468,10 @@ section.instructor-dashboard-content-2 { padding: ($baseline/2) ($baseline*0.75); } } + + .input-file { + margin-bottom: ($baseline/2); + } } .form-submit, .form-cancel { @@ -472,7 +495,6 @@ section.instructor-dashboard-content-2 { .cohort-management-nav { @include clearfix(); - margin-bottom: $baseline; .cohort-management-nav-form { width: 60%; @@ -486,10 +508,10 @@ section.instructor-dashboard-content-2 { .action-create { @include idashbutton($blue); + @extend %t-weight4; float: right; text-align: right; text-shadow: none; - font-weight: 600; } // STATE: is disabled @@ -538,10 +560,6 @@ section.instructor-dashboard-content-2 { } // cohort group - .cohort-management-group { - border: 1px solid $gray-l5; - } - .cohort-management-group-header { border-bottom: ($baseline/10) solid $gray-l4; background: $gray-l5; @@ -610,6 +628,7 @@ section.instructor-dashboard-content-2 { .cohort-management-group-add { @extend %cohort-management-form; + border: 1px solid $gray-l5; padding: $baseline $baseline 0 $baseline; .message-title { @@ -646,10 +665,51 @@ section.instructor-dashboard-content-2 { } } + // CSV-based file upload for auto cohort assigning + .toggle-cohort-management-secondary { + @extend %t-copy-sub1; + } + + .cohort-management-file-upload { + + .message-title { + @extend %t-title7; + } + + .form-introduction { + @extend %t-copy-sub1; + margin-bottom: $baseline; + + p { + color: $gray-l1; + } + } + } + + .file-upload-form { + @extend %cohort-management-form; + + .form-fields { + margin-bottom: $baseline; + } + + .action-submit { + @include idashbutton($blue); + // needed to override very poor specificity and base rules for blue button + @include font-size(14); + margin-bottom: 0; + font-weight: 700; + text-shadow: none; + } + } .cohort-management-supplemental { @extend %t-copy-sub1; - margin-top: ($baseline/2); + margin-top: $baseline; + padding: ($baseline/2) $baseline; + background: $gray-l5; + border-radius: ($baseline/10); + .icon { margin-right: ($baseline/4); diff --git a/lms/templates/file-upload.underscore b/lms/templates/file-upload.underscore new file mode 100644 index 0000000000..5c9a99edfd --- /dev/null +++ b/lms/templates/file-upload.underscore @@ -0,0 +1,24 @@ +
+

<%- title %>

+ +
+ +
+ +
+
+ + + accept="<%- extensions %>" + <% } %> + /> + <%- inputTip %> +
+
+ +
+ +
+
+
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore index 5c3e1710c5..db6fae6b5c 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore @@ -3,11 +3,12 @@ -
+

<%- gettext('Assign students to cohort groups manually') %>

+
- +
@@ -26,13 +27,22 @@
-
-

- - <%= interpolate( - gettext('To review all student cohort group assignments, download course profile information on %(link_start)s the Data Download page. %(link_end)s'), - {link_start: '', link_end: ''}, - true - ) %> -

+
+ +
+ + + <%- gettext('Assign students to cohort groups by uploading a CSV file') %> + + +
+

+ + <%= interpolate( + gettext('To review student cohort group assignments or see the results of uploading a CSV file, download course profile information or cohort results on %(link_start)s the Data Download page. %(link_end)s'), + {link_start: '', link_end: ''}, + true + ) %> +

+
diff --git a/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html b/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html index 931f424b80..efee29766d 100644 --- a/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html +++ b/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html @@ -47,15 +47,17 @@ + <%static:js group='module-descriptor-js'/> <%static:js group='instructor_dash'/> <%static:js group='application'/> ## Backbone classes declared explicitly until RequireJS is supported + + - @@ -67,6 +69,10 @@ <%static:include path="instructor/instructor_dashboard_2/${template_name}.underscore" /> % endfor + + ## NOTE that instructor is set as the active page so that the instructor button lights up, even though this is the instructor_2 page. diff --git a/lms/templates/instructor/instructor_dashboard_2/membership.html b/lms/templates/instructor/instructor_dashboard_2/membership.html index 905ad0fe87..2d18373c37 100644 --- a/lms/templates/instructor/instructor_dashboard_2/membership.html +++ b/lms/templates/instructor/instructor_dashboard_2/membership.html @@ -249,6 +249,7 @@
@@ -262,7 +263,8 @@ var cohortsView = new CohortsView({ el: cohortManagementElement, model: cohorts, - advanced_settings_url: cohortManagementElement.data('advanced-settings-url') + advanced_settings_url: cohortManagementElement.data('advanced-settings-url'), + upload_cohorts_csv_url: cohortManagementElement.data('upload_cohorts_csv_url') }); cohorts.fetch().done(function() { cohortsView.render(); diff --git a/openedx/core/djangoapps/course_groups/views.py b/openedx/core/djangoapps/course_groups/views.py index fa92262486..37872e60bf 100644 --- a/openedx/core/djangoapps/course_groups/views.py +++ b/openedx/core/djangoapps/course_groups/views.py @@ -12,6 +12,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey from courseware.courses import get_course_with_access from edxmako.shortcuts import render_to_response +from util.json_request import JsonResponse from . import cohorts from .models import CourseUserGroup @@ -23,7 +24,7 @@ def json_http_response(data): Return an HttpResponse with the data json-serialized and the right content type header. """ - return HttpResponse(json.dumps(data), content_type="application/json") + return JsonResponse(data) def split_by_comma_and_whitespace(cstr): diff --git a/test_root/uploads/.git-keep b/test_root/uploads/.git-keep deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/test_root/uploads/.gitignore b/test_root/uploads/.gitignore new file mode 100644 index 0000000000..afed0735dc --- /dev/null +++ b/test_root/uploads/.gitignore @@ -0,0 +1 @@ +*.csv