From ba3f7f9409cb3c27c0a23fc5cbcd53bbca4c363e Mon Sep 17 00:00:00 2001 From: Daniel Friedman Date: Fri, 26 Sep 2014 17:01:04 -0400 Subject: [PATCH] Make student profile CSV download a background task, add cohort column --- lms/djangoapps/instructor/features/common.py | 7 +- .../instructor/features/data_download.feature | 9 +++ .../instructor/features/data_download.py | 19 +++-- lms/djangoapps/instructor/tests/test_api.py | 74 +++++++++++++------ lms/djangoapps/instructor/views/api.py | 23 ++++-- lms/djangoapps/instructor_analytics/basic.py | 19 ++++- .../instructor_analytics/tests/test_basic.py | 41 +++++++++- lms/djangoapps/instructor_task/api.py | 17 ++++- lms/djangoapps/instructor_task/tasks.py | 14 ++++ .../instructor_task/tasks_helper.py | 63 ++++++++++++---- .../instructor_task/tests/test_api.py | 35 +++++++-- .../tests/test_tasks_helper.py | 39 +++++++--- .../instructor_dashboard/data_download.coffee | 37 ++++++---- lms/static/js/views/cohorts.js | 10 ++- .../sass/course/instructor/_instructor_2.scss | 5 +- .../instructor_dashboard_2/cohorts.underscore | 11 +++ .../instructor_dashboard_2/data_download.html | 42 +++++------ 17 files changed, 351 insertions(+), 114 deletions(-) diff --git a/lms/djangoapps/instructor/features/common.py b/lms/djangoapps/instructor/features/common.py index c6bf6f45c7..bca2c37904 100644 --- a/lms/djangoapps/instructor/features/common.py +++ b/lms/djangoapps/instructor/features/common.py @@ -92,9 +92,9 @@ def click_a_button(step, button): # pylint: disable=unused-argument # Expect to see a message that grade report is being generated expected_msg = "Your grade report is being generated! You can view the status of the generation task in the 'Pending Instructor Tasks' section." - world.wait_for_visible('#grade-request-response') + world.wait_for_visible('#report-request-response') assert_in( - expected_msg, world.css_text('#grade-request-response'), + expected_msg, world.css_text('#report-request-response'), msg="Could not find grade report generation success message." ) @@ -113,7 +113,8 @@ def click_a_button(step, button): # pylint: disable=unused-argument elif button == "Download profile information as a CSV": # Go to the data download section of the instructor dash go_to_section("data_download") - # Don't do anything else, next step will handle clicking & downloading + + world.css_click('input[name="list-profiles-csv"]') else: raise ValueError("Unrecognized button option " + button) diff --git a/lms/djangoapps/instructor/features/data_download.feature b/lms/djangoapps/instructor/features/data_download.feature index 6efe9c6397..f83f785226 100644 --- a/lms/djangoapps/instructor/features/data_download.feature +++ b/lms/djangoapps/instructor/features/data_download.feature @@ -44,3 +44,12 @@ Feature: LMS.Instructor Dash Data Download | Role | | instructor | | staff | + + Scenario: Generate & download a student profile report + Given I am "" for a course + When I click "Download profile information as a CSV" + Then I see a student profile csv file in the reports table + Examples: + | Role | + | instructor | + | staff | diff --git a/lms/djangoapps/instructor/features/data_download.py b/lms/djangoapps/instructor/features/data_download.py index 6d08551781..dd60438d4e 100644 --- a/lms/djangoapps/instructor/features/data_download.py +++ b/lms/djangoapps/instructor/features/data_download.py @@ -68,14 +68,23 @@ length=0""" assert_in(expected_config, world.css_text('#data-grade-config-text')) -@step(u"I see a grade report csv file in the reports table") -def find_grade_report_csv_link(step): # pylint: disable=unused-argument - # Need to reload the page to see the grades download table +def verify_report_is_generated(report_name_substring): + # Need to reload the page to see the reports table updated reload_the_page(step) world.wait_for_visible('#report-downloads-table') # Find table and assert a .csv file is present - expected_file_regexp = 'edx_999_Test_Course_grade_report_\d{4}-\d{2}-\d{2}-\d{4}\.csv' + expected_file_regexp = 'edx_999_Test_Course_{0}_'.format(report_name_substring) + '\d{4}-\d{2}-\d{2}-\d{4}\.csv' assert_regexp_matches( world.css_html('#report-downloads-table'), expected_file_regexp, - msg="Expected grade report filename was not found." + msg="Expected report filename was not found." ) + + +@step(u"I see a grade report csv file in the reports table") +def find_grade_report_csv_link(step): # pylint: disable=unused-argument + verify_report_is_generated('grade_report') + + +@step(u"I see a student profile csv file in the reports table") +def find_student_profile_report_csv_link(step): # pylint: disable=unused-argument + verify_report_is_generated('student_profile_info') diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 757adfd58d..480622672b 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -55,6 +55,22 @@ from ..views.tools import get_extended_due EXPECTED_CSV_HEADER = '"code","course_id","company_name","created_by","redeemed_by","invoice_id","purchaser","customer_reference_number","internal_reference"' EXPECTED_COUPON_CSV_HEADER = '"course_id","percentage_discount","code_redeemed_count","description"' +# ddt data for test cases involving reports +REPORTS_DATA = ( + { + 'report_type': 'grade', + 'instructor_api_endpoint': 'calculate_grades_csv', + 'task_api_endpoint': 'instructor_task.api.submit_calculate_grades_csv', + 'extra_instructor_api_kwargs': {} + }, + { + 'report_type': 'enrolled student profile', + 'instructor_api_endpoint': 'get_students_features', + 'task_api_endpoint': 'instructor_task.api.submit_calculate_students_features_csv', + 'extra_instructor_api_kwargs': {'csv': '/csv'} + } +) + @common_exceptions_400 def view_success(request): # pylint: disable=W0613 @@ -156,6 +172,7 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase): ('list_background_email_tasks', {}), ('list_report_downloads', {}), ('calculate_grades_csv', {}), + ('get_students_features', {}), ] # Endpoints that only Instructors can access self.instructor_level_endpoints = [ @@ -1343,6 +1360,7 @@ class TestInstructorAPILevelsAccess(ModuleStoreTestCase, LoginEnrollmentTestCase self.assertNotIn(rolename, user_roles) +@ddt.ddt @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCase): """ @@ -1350,6 +1368,7 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa """ def setUp(self): + super(TestInstructorAPILevelsDataDump, self).setUp() self.course = CourseFactory.create() self.course_mode = CourseMode(course_id=self.course.id, mode_slug="honor", @@ -1613,6 +1632,21 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa self.assertEqual(student_json['username'], student.username) self.assertEqual(student_json['email'], student.email) + @ddt.data(True, False) + def test_get_students_features_cohorted(self, is_cohorted): + """ + Test that get_students_features includes cohort info when the course is + cohorted, and does not when the course is not cohorted. + """ + url = reverse('get_students_features', kwargs={'course_id': unicode(self.course.id)}) + self.course.cohort_config = {'cohorted': is_cohorted} + self.store.update_item(self.course, self.instructor.id) + + response = self.client.get(url, {}) + res_json = json.loads(response.content) + + self.assertEqual('cohort' in res_json['feature_names'], is_cohorted) + @patch.object(instructor.views.api, 'anonymous_id_for_user', Mock(return_value='42')) @patch.object(instructor.views.api, 'unique_id_for_user', Mock(return_value='41')) def test_get_anon_ids(self): @@ -1625,9 +1659,9 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa body = response.content.replace('\r', '') self.assertTrue(body.startswith( '"User ID","Anonymized User ID","Course Specific Anonymized User ID"' - '\n"2","41","42"\n' + '\n"3","41","42"\n' )) - self.assertTrue(body.endswith('"7","41","42"\n')) + self.assertTrue(body.endswith('"8","41","42"\n')) def test_list_report_downloads(self): url = reverse('list_report_downloads', kwargs={'course_id': self.course.id.to_deprecated_string()}) @@ -1655,33 +1689,31 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa res_json = json.loads(response.content) self.assertEqual(res_json, expected_response) - def test_calculate_grades_csv_success(self): - url = reverse('calculate_grades_csv', kwargs={'course_id': self.course.id.to_deprecated_string()}) + @ddt.data(*REPORTS_DATA) + @ddt.unpack + def test_calculate_report_csv_success(self, report_type, instructor_api_endpoint, task_api_endpoint, extra_instructor_api_kwargs): + kwargs = {'course_id': unicode(self.course.id)} + kwargs.update(extra_instructor_api_kwargs) + url = reverse(instructor_api_endpoint, kwargs=kwargs) - with patch('instructor_task.api.submit_calculate_grades_csv') as mock_cal_grades: - mock_cal_grades.return_value = True + with patch(task_api_endpoint): response = self.client.get(url, {}) - success_status = "Your grade report is being generated! You can view the status of the generation task in the 'Pending Instructor Tasks' section." + success_status = "Your {report_type} report is being generated! You can view the status of the generation task in the 'Pending Instructor Tasks' section.".format(report_type=report_type) self.assertIn(success_status, response.content) - def test_calculate_grades_csv_already_running(self): - url = reverse('calculate_grades_csv', kwargs={'course_id': self.course.id.to_deprecated_string()}) + @ddt.data(*REPORTS_DATA) + @ddt.unpack + def test_calculate_report_csv_already_running(self, report_type, instructor_api_endpoint, task_api_endpoint, extra_instructor_api_kwargs): + kwargs = {'course_id': unicode(self.course.id)} + kwargs.update(extra_instructor_api_kwargs) + url = reverse(instructor_api_endpoint, kwargs=kwargs) - with patch('instructor_task.api.submit_calculate_grades_csv') as mock_cal_grades: - mock_cal_grades.side_effect = AlreadyRunningError() + with patch(task_api_endpoint) as mock: + mock.side_effect = AlreadyRunningError() response = self.client.get(url, {}) - already_running_status = "A grade report generation task is already in progress. Check the 'Pending Instructor Tasks' table for the status of the task. When completed, the report will be available for download in the table below." + already_running_status = "{report_type} report generation task is already in progress. Check the 'Pending Instructor Tasks' table for the status of the task. When completed, the report will be available for download in the table below.".format(report_type=report_type) self.assertIn(already_running_status, response.content) - def test_get_students_features_csv(self): - """ - Test that some minimum of information is formatted - correctly in the response to get_students_features. - """ - url = reverse('get_students_features', kwargs={'course_id': self.course.id.to_deprecated_string()}) - response = self.client.get(url + '/csv', {}) - self.assertEqual(response['Content-Type'], 'text/csv') - def test_get_distribution_no_feature(self): """ Test that get_distribution lists available features diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 931a3da703..f62119aa99 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -80,6 +80,7 @@ from .tools import ( bulk_email_is_enabled_for_course, add_block_ids, ) +from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locations import SlashSeparatedCourseKey from opaque_keys import InvalidKeyError @@ -690,7 +691,8 @@ def get_students_features(request, course_id, csv=False): # pylint: disable=W06 TO DO accept requests for different attribute sets. """ - course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id) + course_key = CourseKey.from_string(course_id) + course = get_course_by_id(course_key) available_features = instructor_analytics.basic.AVAILABLE_FEATURES @@ -704,8 +706,6 @@ def get_students_features(request, course_id, csv=False): # pylint: disable=W06 'goals' ] - student_data = instructor_analytics.basic.enrolled_students_features(course_id, query_features) - # Provide human-friendly and translatable names for these features. These names # will be displayed in the table generated in data_download.coffee. It is not (yet) # used as the header row in the CSV, but could be in the future. @@ -723,9 +723,15 @@ def get_students_features(request, course_id, csv=False): # pylint: disable=W06 'goals': _('Goals'), } + if course.is_cohorted: + # Translators: 'Cohort' refers to a group of students within a course. + query_features.append('cohort') + query_features_names['cohort'] = _('Cohort') + if not csv: + student_data = instructor_analytics.basic.enrolled_students_features(course_key, query_features) response_payload = { - 'course_id': course_id.to_deprecated_string(), + 'course_id': unicode(course_key), 'students': student_data, 'students_count': len(student_data), 'queried_features': query_features, @@ -734,8 +740,13 @@ def get_students_features(request, course_id, csv=False): # pylint: disable=W06 } return JsonResponse(response_payload) else: - header, datarows = instructor_analytics.csvs.format_dictlist(student_data, query_features) - return instructor_analytics.csvs.create_csv_response("enrolled_profiles.csv", header, datarows) + try: + instructor_task.api.submit_calculate_students_features_csv(request, course_key, query_features) + success_status = _("Your enrolled student profile report is being generated! You can view the status of the generation task in the 'Pending Instructor Tasks' section.") + return JsonResponse({"status": success_status}) + except AlreadyRunningError: + already_running_status = _("An enrolled student profile report generation task is already in progress. Check the 'Pending Instructor Tasks' table for the status of the task. When completed, the report will be available for download in the table below.") + return JsonResponse({"status": already_running_status}) @ensure_csrf_cookie diff --git a/lms/djangoapps/instructor_analytics/basic.py b/lms/djangoapps/instructor_analytics/basic.py index 3b394a6001..eecede860a 100644 --- a/lms/djangoapps/instructor_analytics/basic.py +++ b/lms/djangoapps/instructor_analytics/basic.py @@ -122,22 +122,27 @@ def purchase_transactions(course_id, features): return [purchase_transactions_info(purchased_course, features) for purchased_course in purchased_courses] -def enrolled_students_features(course_id, features): +def enrolled_students_features(course_key, features): """ Return list of student features as dictionaries. - enrolled_students_features(course_id, ['username, first_name']) + enrolled_students_features(course_key, ['username', 'first_name']) would return [ {'username': 'username1', 'first_name': 'firstname1'} {'username': 'username2', 'first_name': 'firstname2'} {'username': 'username3', 'first_name': 'firstname3'} ] """ + include_cohort_column = 'cohort' in features + students = User.objects.filter( - courseenrollment__course_id=course_id, + courseenrollment__course_id=course_key, courseenrollment__is_active=1, ).order_by('username').select_related('profile') + if include_cohort_column: + students = students.prefetch_related('course_groups') + def extract_student(student, features): """ convert student to dictionary """ student_features = [x for x in STUDENT_FEATURES if x in features] @@ -151,6 +156,14 @@ def enrolled_students_features(course_id, features): for feature in profile_features) student_dict.update(profile_dict) + if include_cohort_column: + # Note that we use student.course_groups.all() here instead of + # student.course_groups.filter(). The latter creates a fresh query, + # therefore negating the performance gain from prefetch_related(). + student_dict['cohort'] = next( + (cohort.name for cohort in student.course_groups.all() if cohort.course_id == course_key), + "[unassigned]" + ) return student_dict return [extract_student(student, features) for student in students] diff --git a/lms/djangoapps/instructor_analytics/tests/test_basic.py b/lms/djangoapps/instructor_analytics/tests/test_basic.py index 77c6e1dfdc..788c55fb12 100644 --- a/lms/djangoapps/instructor_analytics/tests/test_basic.py +++ b/lms/djangoapps/instructor_analytics/tests/test_basic.py @@ -13,15 +13,19 @@ from instructor_analytics.basic import ( sale_record_features, enrolled_students_features, course_registration_features, coupon_codes_features, AVAILABLE_FEATURES, STUDENT_FEATURES, PROFILE_FEATURES ) +from course_groups.tests.helpers import CohortFactory +from course_groups.models import CourseUserGroup from courseware.tests.factories import InstructorFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -class TestAnalyticsBasic(TestCase): +class TestAnalyticsBasic(ModuleStoreTestCase): """ Test basic analytics functions. """ def setUp(self): + super(TestAnalyticsBasic, self).setUp() self.course_key = SlashSeparatedCourseKey('robot', 'course', 'id') self.users = tuple(UserFactory() for _ in xrange(30)) self.ces = tuple(CourseEnrollment.enroll(user, self.course_key) @@ -40,7 +44,8 @@ class TestAnalyticsBasic(TestCase): query_features = ('username', 'name', 'email') for feature in query_features: self.assertIn(feature, AVAILABLE_FEATURES) - userreports = enrolled_students_features(self.course_key, query_features) + with self.assertNumQueries(1): + userreports = enrolled_students_features(self.course_key, query_features) self.assertEqual(len(userreports), len(self.users)) for userreport in userreports: self.assertEqual(set(userreport.keys()), set(query_features)) @@ -48,6 +53,37 @@ class TestAnalyticsBasic(TestCase): self.assertIn(userreport['email'], [user.email for user in self.users]) self.assertIn(userreport['name'], [user.profile.name for user in self.users]) + def test_enrolled_students_features_keys_cohorted(self): + course = CourseFactory.create(course_key=self.course_key) + course.cohort_config = {'cohorted': True, 'auto_cohort': True, 'auto_cohort_groups': ['cohort']} + self.store.update_item(course, self.instructor.id) + cohort = CohortFactory.create(name='cohort', course_id=course.id) + cohorted_students = [UserFactory.create() for _ in xrange(10)] + cohorted_usernames = [student.username for student in cohorted_students] + non_cohorted_student = UserFactory.create() + for student in cohorted_students: + cohort.users.add(student) + CourseEnrollment.enroll(student, course.id) + CourseEnrollment.enroll(non_cohorted_student, course.id) + instructor = InstructorFactory(course_key=course.id) + self.client.login(username=instructor.username, password='test') + + query_features = ('username', 'cohort') + # There should be a constant of 2 SQL queries when calling + # enrolled_students_features. The first query comes from the call to + # User.objects.filter(...), and the second comes from + # prefetch_related('course_groups'). + with self.assertNumQueries(2): + userreports = enrolled_students_features(course.id, query_features) + self.assertEqual(len([r for r in userreports if r['username'] in cohorted_usernames]), len(cohorted_students)) + self.assertEqual(len([r for r in userreports if r['username'] == non_cohorted_student.username]), 1) + for report in userreports: + self.assertEqual(set(report.keys()), set(query_features)) + if report['username'] in cohorted_usernames: + self.assertEqual(report['cohort'], cohort.name) + else: + self.assertEqual(report['cohort'], '[unassigned]') + def test_available_features(self): self.assertEqual(len(AVAILABLE_FEATURES), len(STUDENT_FEATURES + PROFILE_FEATURES)) self.assertEqual(set(AVAILABLE_FEATURES), set(STUDENT_FEATURES + PROFILE_FEATURES)) @@ -59,6 +95,7 @@ class TestCourseSaleRecordsAnalyticsBasic(ModuleStoreTestCase): """ Fixtures. """ + super(TestCourseSaleRecordsAnalyticsBasic, self).setUp() self.course = CourseFactory.create() self.instructor = InstructorFactory(course_key=self.course.id) self.client.login(username=self.instructor.username, password='test') diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index e91e58fa0f..219df82899 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -17,7 +17,8 @@ from instructor_task.tasks import (rescore_problem, reset_problem_attempts, delete_problem_state, send_bulk_course_email, - calculate_grades_csv) + calculate_grades_csv, + calculate_students_features_csv) from instructor_task.api_helper import (check_arguments_for_rescoring, encode_problem_and_student_input, @@ -218,3 +219,17 @@ def submit_calculate_grades_csv(request, course_key): task_key = "" return submit_task(request, task_type, task_class, course_key, task_input, task_key) + + +def submit_calculate_students_features_csv(request, course_key, features): + """ + Submits a task to generate a CSV containing student profile info. + + Raises AlreadyRunningError if said CSV is already being updated. + """ + task_type = 'profile_info_csv' + task_class = calculate_students_features_csv + task_input = {'features': features} + task_key = "" + + return submit_task(request, task_type, task_class, course_key, task_input, task_key) diff --git a/lms/djangoapps/instructor_task/tasks.py b/lms/djangoapps/instructor_task/tasks.py index 69e8ee1865..297c306637 100644 --- a/lms/djangoapps/instructor_task/tasks.py +++ b/lms/djangoapps/instructor_task/tasks.py @@ -31,6 +31,7 @@ from instructor_task.tasks_helper import ( reset_attempts_module_state, delete_problem_module_state, push_grades_to_s3, + push_students_csv_to_s3 ) from bulk_email.tasks import perform_delegate_email_batches @@ -136,6 +137,19 @@ def calculate_grades_csv(entry_id, xmodule_instance_args): """ Grade a course and push the results to an S3 bucket for download. """ + # Translators: This is a past-tense verb that is inserted into task progress messages as {action}. action_name = ugettext_noop('graded') task_fn = partial(push_grades_to_s3, xmodule_instance_args) return run_main_task(entry_id, task_fn, action_name) + + +@task(base=BaseInstructorTask, routing_key=settings.GRADES_DOWNLOAD_ROUTING_KEY) # pylint: disable=E1102 +def calculate_students_features_csv(entry_id, xmodule_instance_args): + """ + Compute student profile information for a course and upload the + CSV to an S3 bucket for download. + """ + # Translators: This is a past-tense verb that is inserted into task progress messages as {action}. + action_name = ugettext_noop('generated') + task_fn = partial(push_students_csv_to_s3, 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 c13d22271f..44beec3bb3 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -23,6 +23,8 @@ from courseware.grades import iterate_grades_for from courseware.models import StudentModule from courseware.model_data import FieldDataCache from courseware.module_render import get_module_for_descriptor_internal +from instructor_analytics.basic import enrolled_students_features +from instructor_analytics.csvs import format_dictlist from instructor_task.models import ReportStore, InstructorTask, PROGRESS from student.models import CourseEnrollment @@ -477,6 +479,32 @@ def delete_problem_module_state(xmodule_instance_args, _module_descriptor, stude return UPDATE_STATUS_SUCCEEDED +def upload_csv_to_report_store(rows, csv_name, course_id, timestamp): + """ + Upload data as a CSV using ReportStore. + + Arguments: + rows: CSV data in the following format (first column may be a + header): + [ + [row1_colum1, row1_colum2, ...], + ... + ] + csv_name: Name of the resulting CSV + course_id: ID of the course + """ + report_store = ReportStore.from_config() + report_store.store_rows( + course_id, + u"{course_prefix}_{csv_name}_{timestamp_str}.csv".format( + course_prefix=urllib.quote(unicode(course_id).replace("/", "_")), + csv_name=csv_name, + timestamp_str=timestamp.strftime("%Y-%m-%d-%H%M") + ), + rows + ) + + def push_grades_to_s3(_xmodule_instance_args, _entry_id, course_id, _task_input, action_name): """ For a given `course_id`, generate a grades CSV file for all students that @@ -557,25 +585,30 @@ def push_grades_to_s3(_xmodule_instance_args, _entry_id, course_id, _task_input, curr_step = "Uploading CSVs" update_task_progress() - # Generate parts of the file name - timestamp_str = start_time.strftime("%Y-%m-%d-%H%M") - course_id_prefix = urllib.quote(course_id.to_deprecated_string().replace("/", "_")) - # Perform the actual upload - report_store = ReportStore.from_config() - report_store.store_rows( - course_id, - u"{}_grade_report_{}.csv".format(course_id_prefix, timestamp_str), - rows - ) + upload_csv_to_report_store(rows, 'grade_report', course_id, start_time) # If there are any error rows (don't count the header), write them out as well if len(err_rows) > 1: - report_store.store_rows( - course_id, - u"{}_grade_report_{}_err.csv".format(course_id_prefix, timestamp_str), - err_rows - ) + upload_csv_to_report_store(err_rows, 'grade_report_err', course_id, start_time) # One last update before we close out... return update_task_progress() + + +def push_students_csv_to_s3(_xmodule_instance_args, _entry_id, course_id, task_input, _action_name): + """ + For a given `course_id`, generate a CSV file containing profile + information for all students that are enrolled, and store using a + `ReportStore`. + """ + # compute the student features table and format it + query_features = task_input.get('features') + student_data = enrolled_students_features(course_id, query_features) + header, rows = format_dictlist(student_data, query_features) + rows.insert(0, header) + + # Perform the upload + upload_csv_to_report_store(rows, 'student_profile_info', course_id, datetime.now(UTC)) + + return UPDATE_STATUS_SUCCEEDED diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index dac66fdac1..ce2960307b 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -15,6 +15,7 @@ from instructor_task.api import ( submit_reset_problem_attempts_for_all_students, submit_delete_problem_state_for_all_students, submit_bulk_course_email, + submit_calculate_students_features_csv, ) from instructor_task.api_helper import AlreadyRunningError @@ -170,14 +171,34 @@ class InstructorTaskCourseSubmitTest(InstructorTaskCourseTestCase): course_email = CourseEmail.create(self.course.id, self.instructor, SEND_TO_ALL, "Test Subject", "

This is a test message

") return course_email.id # pylint: disable=E1101 - def test_submit_bulk_email_all(self): - email_id = self._define_course_email() - instructor_task = submit_bulk_course_email(self.create_task_request(self.instructor), self.course.id, email_id) - - # test resubmitting, by updating the existing record: + def _test_resubmission(self, api_call): + """ + Tests the resubmission of an instructor task through the API. + The call to the API is a lambda expression passed via + `api_call`. Expects that the API call returns the resulting + InstructorTask object, and that its resubmission raises + `AlreadyRunningError`. + """ + instructor_task = api_call() instructor_task = InstructorTask.objects.get(id=instructor_task.id) # pylint: disable=E1101 instructor_task.task_state = PROGRESS instructor_task.save() - with self.assertRaises(AlreadyRunningError): - instructor_task = submit_bulk_course_email(self.create_task_request(self.instructor), self.course.id, email_id) + api_call() + + def test_submit_bulk_email_all(self): + email_id = self._define_course_email() + api_call = lambda: submit_bulk_course_email( + self.create_task_request(self.instructor), + self.course.id, + email_id + ) + self._test_resubmission(api_call) + + def test_submit_calculate_students_features(self): + api_call = lambda: submit_calculate_students_features_csv( + self.create_task_request(self.instructor), + self.course.id, + features=[] + ) + self._test_resubmission(api_call) diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 99bc997a18..976022e93c 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -13,32 +13,32 @@ from mock import Mock, patch from django.conf import settings 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 instructor_task.tasks_helper import push_grades_to_s3 +from instructor_task.models import ReportStore +from instructor_task.tasks_helper import push_grades_to_s3, push_students_csv_to_s3, UPDATE_STATUS_SUCCEEDED -TEST_COURSE_ORG = 'edx' -TEST_COURSE_NAME = 'test_course' -TEST_COURSE_NUMBER = '1.23x' - - -@ddt.ddt -class TestInstructorGradeReport(TestCase): +class TestReport(ModuleStoreTestCase): """ - Tests that CSV grade report generation works. + Base class for testing CSV download tasks. """ def setUp(self): - self.course = CourseFactory.create(org=TEST_COURSE_ORG, - number=TEST_COURSE_NUMBER, - display_name=TEST_COURSE_NAME) + self.course = CourseFactory.create() def tearDown(self): if os.path.exists(settings.GRADES_DOWNLOAD['ROOT_PATH']): shutil.rmtree(settings.GRADES_DOWNLOAD['ROOT_PATH']) + +@ddt.ddt +class TestInstructorGradeReport(TestReport): + """ + Tests that CSV grade report generation works. + """ def create_student(self, username, email): student = UserFactory.create(username=username, email=email) CourseEnrollmentFactory.create(user=student, course_id=self.course.id) @@ -58,3 +58,18 @@ class TestInstructorGradeReport(TestCase): result = push_grades_to_s3(None, None, self.course.id, None, 'graded') #This assertion simply confirms that the generation completed with no errors self.assertEquals(result['succeeded'], result['attempted']) + + +class TestStudentReport(TestReport): + """ + Tests that CSV student profile report generation works. + """ + def test_success(self): + task_input = {'features': []} + with patch('instructor_task.tasks_helper._get_current_task'): + result = push_students_csv_to_s3(None, None, self.course.id, task_input, 'calculated') + report_store = ReportStore.from_config() + links = report_store.links_for(self.course.id) + + self.assertEquals(len(links), 1) + self.assertEquals(result, UPDATE_STATUS_SUCCEEDED) diff --git a/lms/static/coffee/src/instructor_dashboard/data_download.coffee b/lms/static/coffee/src/instructor_dashboard/data_download.coffee index 3ff456c871..4931e35c83 100644 --- a/lms/static/coffee/src/instructor_dashboard/data_download.coffee +++ b/lms/static/coffee/src/instructor_dashboard/data_download.coffee @@ -26,11 +26,11 @@ class DataDownload # response areas @$download = @$section.find '.data-download-container' @$download_display_text = @$download.find '.data-display-text' - @$download_display_table = @$download.find '.data-display-table' @$download_request_response_error = @$download.find '.request-response-error' - @$grades = @$section.find '.grades-download-container' - @$grades_request_response = @$grades.find '.request-response' - @$grades_request_response_error = @$grades.find '.request-response-error' + @$reports = @$section.find '.reports-download-container' + @$download_display_table = @$reports.find '.data-display-table' + @$reports_request_response = @$reports.find '.request-response' + @$reports_request_response_error = @$reports.find '.request-response-error' @report_downloads = new ReportDownloads(@$section) @instructor_tasks = new (PendingInstructorTasks()) @$section @@ -45,11 +45,22 @@ class DataDownload # this handler binds to both the download # and the csv button @$list_studs_csv_btn.click (e) => + @clear_display() + url = @$list_studs_csv_btn.data 'endpoint' # handle csv special case # redirect the document to the csv file. url += '/csv' - location.href = url + + $.ajax + dataType: 'json' + url: url + error: (std_ajax_err) => + @$reports_request_response_error.text gettext("Error generating student profile information. Please try again.") + $(".msg-error").css({"display":"block"}) + success: (data) => + @$reports_request_response.text data['status'] + $(".msg-confirm").css({"display":"block"}) @$list_studs_btn.click (e) => url = @$list_studs_btn.data 'endpoint' @@ -62,7 +73,7 @@ class DataDownload $.ajax dataType: 'json' url: url - error: std_ajax_err => + error: (std_ajax_err) => @clear_display() @$download_request_response_error.text gettext("Error getting student list.") success: (data) => @@ -89,7 +100,7 @@ class DataDownload $.ajax dataType: 'json' url: url - error: std_ajax_err => + error: (std_ajax_err) => @clear_display() @$download_request_response_error.text gettext("Error retrieving grading configuration.") success: (data) => @@ -105,11 +116,11 @@ class DataDownload $.ajax dataType: 'json' url: url - error: std_ajax_err => - @$grades_request_response_error.text gettext("Error generating grades. Please try again.") + error: (std_ajax_err) => + @$reports_request_response_error.text gettext("Error generating grades. Please try again.") $(".msg-error").css({"display":"block"}) success: (data) => - @$grades_request_response.text data['status'] + @$reports_request_response.text data['status'] $(".msg-confirm").css({"display":"block"}) # handler for when the section title is clicked. @@ -129,8 +140,8 @@ class DataDownload @$download_display_text.empty() @$download_display_table.empty() @$download_request_response_error.empty() - @$grades_request_response.empty() - @$grades_request_response_error.empty() + @$reports_request_response.empty() + @$reports_request_response_error.empty() # Clear any CSS styling from the request-response areas $(".msg-confirm").css({"display":"none"}) $(".msg-error").css({"display":"none"}) @@ -157,7 +168,7 @@ class ReportDownloads @create_report_downloads_table data.downloads else console.log "No reports ready for download" - error: std_ajax_err => console.error "Error finding report downloads" + error: (std_ajax_err) => console.error "Error finding report downloads" create_report_downloads_table: (report_downloads_data) -> @$report_downloads_table.empty() diff --git a/lms/static/js/views/cohorts.js b/lms/static/js/views/cohorts.js index c7cf01aeba..9f9131f316 100644 --- a/lms/static/js/views/cohorts.js +++ b/lms/static/js/views/cohorts.js @@ -7,7 +7,8 @@ 'change .cohort-select': 'onCohortSelected', 'click .action-create': 'showAddCohortForm', 'click .action-cancel': 'cancelAddCohortForm', - 'click .action-save': 'saveAddCohortForm' + 'click .action-save': 'saveAddCohortForm', + 'click .link-cross-reference': 'showSection' }, initialize: function(options) { @@ -170,6 +171,13 @@ event.preventDefault(); this.removeNotification(); this.onSync(); + }, + + showSection: function(event) { + event.preventDefault(); + var section = $(event.currentTarget).data("section"); + $(".instructor-nav .nav-item a[data-section='" + section + "']").click(); + $(window).scrollTop(0); } }); }).call(this, $, _, Backbone, gettext, interpolate_text, CohortEditorView, NotificationModel, NotificationView); diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss index d530877f3f..7ae6a4fc6e 100644 --- a/lms/static/sass/course/instructor/_instructor_2.scss +++ b/lms/static/sass/course/instructor/_instructor_2.scss @@ -894,15 +894,12 @@ section.instructor-dashboard-content-2 { line-height: 1.3em; } - .data-download-container { + .reports-download-container { .data-display-table { .slickgrid { height: 400px; } } - } - - .grades-download-container { .report-downloads-table { .slickgrid { height: 300px; diff --git a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore index 2cd0f98bc0..5c3e1710c5 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore @@ -25,3 +25,14 @@
+ +
+

+ + <%= 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 + ) %> +

+
diff --git a/lms/templates/instructor/instructor_dashboard_2/data_download.html b/lms/templates/instructor/instructor_dashboard_2/data_download.html index b931e93855..c43411f5d4 100644 --- a/lms/templates/instructor/instructor_dashboard_2/data_download.html +++ b/lms/templates/instructor/instructor_dashboard_2/data_download.html @@ -6,16 +6,6 @@

${_("Data Download")}

-

${_("Click to generate a CSV file of all students enrolled in this course, along with profile information such as email address and username:")}

- -

- - % if not disable_buttons: -

${_("For smaller courses, click to list profile information for enrolled students directly on this page:")}

-

- %endif -
-

${_("Click to display the grading configuration for the course. The grading configuration is the breakdown of graded subsections of the course (such as exams and problem sets), and can be changed on the 'Grading' page (under 'Settings') in Studio.")}

@@ -29,27 +19,37 @@ %if settings.FEATURES.get('ENABLE_S3_GRADE_DOWNLOADS'): -
+

${_("Reports")}

+

${_("For large courses, generating some reports can take several hours. When report generation is complete, a link that includes the date and time of generation appears in the table below. These reports are generated in the background, meaning it is OK to navigate away from this page while your report is generating.")}

+ +

${_("Please be patient and do not click these buttons multiple times. Clicking these buttons multiple times will significantly slow the generation process.")}

+ +

${_("Click to generate a CSV file of all students enrolled in this course, along with profile information such as email address and username:")}

+ +

+ + % if not disable_buttons: +

${_("For smaller courses, click to list profile information for enrolled students directly on this page:")}

+

+ %endif +
+ %if settings.FEATURES.get('ALLOW_COURSE_STAFF_GRADE_DOWNLOADS') or section_data['access']['admin']: -

${_("Click to generate a CSV grade report for all currently enrolled students. Links to generated reports appear in a table below when report generation is complete.")}

- -

${_("For large courses, generating this report may take several hours. Please be patient and do not click the button multiple times. Clicking the button multiple times will significantly slow the grade generation process.")}

- -

${_("The report is generated in the background, meaning it is OK to navigate away from this page while your report is generating.")}

- -
-
-
+

${_("Click to generate a CSV grade report for all currently enrolled students.")}

%endif +
+
+
+

${_("Reports Available for Download")}

- ${_("The grade reports listed below are generated each time the Generate Grade Report button is clicked. A link to each grade report remains available on this page, identified by the UTC date and time of generation. Grade reports are not deleted, so you will always be able to access previously generated reports from this page.")} + ${_("The reports listed below are available for download. A link to every report remains available on this page, identified by the UTC date and time of generation. Reports are not deleted, so you will always be able to access previously generated reports from this page.")}

%if settings.FEATURES.get('ENABLE_ASYNC_ANSWER_DISTRIBUTION'):