From f43f1635dfd3dbef77724b1a97c2e7c2e3c2e52e Mon Sep 17 00:00:00 2001 From: Samuel Walladge Date: Tue, 2 Jul 2019 12:23:51 +0930 Subject: [PATCH] Add new functionality to generate ora summary report --- lms/djangoapps/instructor/tests/test_api.py | 22 ++++++++++ lms/djangoapps/instructor/views/api.py | 18 ++++++++ lms/djangoapps/instructor/views/api_urls.py | 1 + .../instructor/views/instructor_dashboard.py | 1 + lms/djangoapps/instructor_task/api.py | 13 ++++++ lms/djangoapps/instructor_task/tasks.py | 12 ++++++ .../instructor_task/tasks_helper/misc.py | 36 ++++++++++++++-- .../instructor_task/tests/test_tasks.py | 33 +++++++++++++- .../tests/test_tasks_helper.py | 43 ++++++++++++++++--- .../instructor_dashboard_2/data_download.html | 1 + 10 files changed, 167 insertions(+), 13 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 7123b9758e..05832f8ebd 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -154,6 +154,7 @@ INSTRUCTOR_POST_ENDPOINTS = { 'change_due_date', 'export_ora2_data', 'export_ora2_submission_files', + 'export_ora2_summary', 'get_grading_config', 'get_problem_responses', 'get_proctored_exam_results', @@ -426,6 +427,7 @@ class TestInstructorAPIDenyLevels(SharedModuleStoreTestCase, LoginEnrollmentTest ('get_problem_responses', {}), ('export_ora2_data', {}), ('export_ora2_submission_files', {}), + ('export_ora2_summary', {}), ('rescore_problem', {'problem_to_reset': self.problem_urlname, 'unique_student_identifier': self.user.email}), ('override_problem_score', @@ -2853,6 +2855,26 @@ class TestInstructorAPILevelsDataDump(SharedModuleStoreTestCase, LoginEnrollment self.assertContains(response, already_running_status, status_code=400) + def test_get_ora2_summary_responses_success(self): + url = reverse('export_ora2_summary', kwargs={'course_id': str(self.course.id)}) + + with patch('lms.djangoapps.instructor_task.api.submit_export_ora2_summary') as mock_submit_ora2_task: + mock_submit_ora2_task.return_value = True + response = self.client.post(url, {}) + success_status = "The ORA summary report is being created." + self.assertContains(response, success_status) + + def test_get_ora2_summary_responses_already_running(self): + url = reverse('export_ora2_summary', kwargs={'course_id': str(self.course.id)}) + task_type = 'export_ora2_summary' + already_running_status = generate_already_running_error_message(task_type) + + with patch('lms.djangoapps.instructor_task.api.submit_export_ora2_summary') as mock_submit_ora2_task: + mock_submit_ora2_task.side_effect = AlreadyRunningError(already_running_status) + response = self.client.post(url, {}) + + self.assertContains(response, already_running_status, status_code=400) + def test_get_student_progress_url(self): """ Test that progress_url is in the successful response. """ url = reverse('get_student_progress_url', kwargs={'course_id': str(self.course.id)}) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 0213c06cfc..2ee85149ad 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -2028,6 +2028,24 @@ def export_ora2_data(request, course_id): return JsonResponse({"status": success_status}) +@transaction.non_atomic_requests +@require_POST +@ensure_csrf_cookie +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_course_permission(permissions.CAN_RESEARCH) +@common_exceptions_400 +def export_ora2_summary(request, course_id): + """ + Pushes a Celery task which will aggregate a summary students' progress in ora2 tasks for a course into a .csv + """ + course_key = CourseKey.from_string(course_id) + report_type = _('ORA summary') + task_api.submit_export_ora2_summary(request, course_key) + success_status = SUCCESS_MESSAGE_TEMPLATE.format(report_type=report_type) + + return JsonResponse({"status": success_status}) + + @transaction.non_atomic_requests @require_POST @ensure_csrf_cookie diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index b7cf4dfa10..8fc74f80df 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -53,6 +53,7 @@ urlpatterns = [ # Reports.. url(r'^get_course_survey_results$', api.get_course_survey_results, name='get_course_survey_results'), url(r'^export_ora2_data', api.export_ora2_data, name='export_ora2_data'), + url(r'^export_ora2_summary', api.export_ora2_summary, name='export_ora2_summary'), url(r'^export_ora2_submission_files', api.export_ora2_submission_files, name='export_ora2_submission_files'), diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 9ee10ab442..3401724aa3 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -633,6 +633,7 @@ def _section_data_download(course, access): 'export_ora2_submission_files_url': reverse( 'export_ora2_submission_files', kwargs={'course_id': str(course_key)} ), + 'export_ora2_summary_url': reverse('export_ora2_summary', kwargs={'course_id': str(course_key)}), } if not access.get('data_researcher'): section_data['is_hidden'] = True diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index 8870e25a34..2f2a963590 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -36,6 +36,7 @@ from lms.djangoapps.instructor_task.tasks import ( delete_problem_state, export_ora2_data, export_ora2_submission_files, + export_ora2_summary, generate_certificates, override_problem_score, proctored_exam_results_csv, @@ -463,6 +464,18 @@ def submit_export_ora2_submission_files(request, course_key): return submit_task(request, task_type, task_class, course_key, task_input, task_key) +def submit_export_ora2_summary(request, course_key): + """ + AlreadyRunningError is raised if an ora2 report is already being generated. + """ + task_type = 'export_ora2_summary' + task_class = export_ora2_summary + task_input = {} + task_key = '' + + return submit_task(request, task_type, task_class, course_key, task_input, task_key) + + def generate_certificates_for_students(request, course_key, student_set=None, specific_student_id=None): """ Submits a task to generate certificates for given students enrolled in the course. diff --git a/lms/djangoapps/instructor_task/tasks.py b/lms/djangoapps/instructor_task/tasks.py index 085684511d..93b2ee5f9c 100644 --- a/lms/djangoapps/instructor_task/tasks.py +++ b/lms/djangoapps/instructor_task/tasks.py @@ -38,6 +38,7 @@ from lms.djangoapps.instructor_task.tasks_helper.misc import ( upload_course_survey_report, upload_ora2_data, upload_ora2_submission_files, + upload_ora2_summary, upload_proctored_exam_results_report ) from lms.djangoapps.instructor_task.tasks_helper.module_state import ( @@ -316,3 +317,14 @@ def export_ora2_submission_files(entry_id, xmodule_instance_args): action_name = ugettext_noop('compressed') task_fn = partial(upload_ora2_submission_files, xmodule_instance_args) return run_main_task(entry_id, task_fn, action_name) + + +@shared_task(base=BaseInstructorTask) +@set_code_owner_attribute +def export_ora2_summary(entry_id, xmodule_instance_args): + """ + Generate a CSV of ora2/student summaries and push it to S3. + """ + action_name = ugettext_noop('generated') + task_fn = partial(upload_ora2_summary, xmodule_instance_args) + return run_main_task(entry_id, task_fn, action_name) diff --git a/lms/djangoapps/instructor_task/tasks_helper/misc.py b/lms/djangoapps/instructor_task/tasks_helper/misc.py index ad124b10d3..f9b0619239 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/misc.py +++ b/lms/djangoapps/instructor_task/tasks_helper/misc.py @@ -275,6 +275,32 @@ def upload_ora2_data( Collect ora2 responses and upload them to S3 as a CSV """ + return _upload_ora2_data_common( + _xmodule_instance_args, _entry_id, course_id, _task_input, action_name, + 'data', OraAggregateData.collect_ora2_data + ) + + +def upload_ora2_summary( + _xmodule_instance_args, _entry_id, course_id, _task_input, action_name +): + """ + Collect ora2/student summaries and upload them to file storage as a CSV + """ + + return _upload_ora2_data_common( + _xmodule_instance_args, _entry_id, course_id, _task_input, action_name, + 'summary', OraAggregateData.collect_ora2_summary + ) + + +def _upload_ora2_data_common( + _xmodule_instance_args, _entry_id, course_id, _task_input, action_name, + report_name, csv_gen_func +): + """ + Common code for uploading data or summary csv report. + """ start_date = datetime.now(UTC) start_time = time() @@ -304,8 +330,10 @@ def upload_ora2_data( task_progress.update_task_state(extra_meta=curr_step) try: - header, datarows = OraAggregateData.collect_ora2_data(course_id) - rows = [header] + [row for row in datarows] # lint-amnesty, pylint: disable=unnecessary-comprehension + header, datarows = csv_gen_func(course_id) + rows = [header] + for row in datarows: + rows.append(row) # Update progress to failed regardless of error type except Exception: # pylint: disable=broad-except TASK_LOG.exception('Failed to get ORA data.') @@ -326,9 +354,9 @@ def upload_ora2_data( ) task_progress.update_task_state(extra_meta=curr_step) - upload_csv_to_report_store(rows, 'ORA_data', course_id, start_date) + upload_csv_to_report_store(rows, 'ORA_{}'.format(report_name), course_id, start_date) - curr_step = {'step': 'Finalizing ORA data report'} + curr_step = {'step': 'Finalizing ORA {} report'.format(report_name)} task_progress.update_task_state(extra_meta=curr_step) TASK_LOG.info('%s, Task type: %s, Upload complete.', task_info_string, action_name) diff --git a/lms/djangoapps/instructor_task/tests/test_tasks.py b/lms/djangoapps/instructor_task/tests/test_tasks.py index 0d8c4d536c..dec7e0c84b 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks.py @@ -25,13 +25,12 @@ from lms.djangoapps.instructor_task.tasks import ( delete_problem_state, export_ora2_data, export_ora2_submission_files, + export_ora2_summary, generate_certificates, override_problem_score, rescore_problem, reset_problem_attempts ) -from lms.djangoapps.instructor_task.tasks_helper.misc import \ - upload_ora2_data # lint-amnesty, pylint: disable=unused-import from lms.djangoapps.instructor_task.tests.factories import InstructorTaskFactory from lms.djangoapps.instructor_task.tests.test_base import InstructorTaskModuleTestCase from xmodule.modulestore.exceptions import ItemNotFoundError @@ -715,3 +714,33 @@ class TestOra2ExportSubmissionFilesInstructorTask(TestInstructorTasks): assert args[0] == task_entry.id assert callable(args[1]) assert args[2] == action_name + + +class TestOra2SummaryInstructorTask(TestInstructorTasks): + """Tests instructor task that fetches ora2 response summary.""" + + def test_ora2_missing_current_task(self): + self._test_missing_current_task(export_ora2_summary) + + def test_ora2_with_failure(self): + self._test_run_with_failure(export_ora2_summary, 'We expected this to fail') + + def test_ora2_with_long_error_msg(self): + self._test_run_with_long_error_msg(export_ora2_summary) + + def test_ora2_with_short_error_msg(self): + self._test_run_with_short_error_msg(export_ora2_summary) + + def test_ora2_runs_task(self): + task_entry = self._create_input_entry() + task_xmodule_args = self._get_xmodule_instance_args() + + with patch('lms.djangoapps.instructor_task.tasks.run_main_task') as mock_main_task: + export_ora2_summary(task_entry.id, task_xmodule_args) + action_name = ugettext_noop('generated') + + assert mock_main_task.call_count == 1 + args = mock_main_task.call_args[0] + assert args[0] == task_entry.id + assert callable(args[1]) + assert args[2] == action_name diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 2a6c26fa57..dcec0f3fb1 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -50,7 +50,8 @@ from lms.djangoapps.instructor_task.tasks_helper.misc import ( cohort_students_and_upload, upload_course_survey_report, upload_ora2_data, - upload_ora2_submission_files + upload_ora2_submission_files, + upload_ora2_summary ) from lms.djangoapps.instructor_task.tests.test_base import ( InstructorTaskCourseTestCase, @@ -2537,6 +2538,7 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase): ] +@ddt.ddt class TestInstructorOra2Report(SharedModuleStoreTestCase): """ Tests that ORA2 response report generation works. @@ -2557,17 +2559,20 @@ class TestInstructorOra2Report(SharedModuleStoreTestCase): if os.path.exists(settings.GRADES_DOWNLOAD['ROOT_PATH']): shutil.rmtree(settings.GRADES_DOWNLOAD['ROOT_PATH']) - def test_report_fails_if_error(self): - with patch( - 'lms.djangoapps.instructor_task.tasks_helper.misc.OraAggregateData.collect_ora2_data' - ) as mock_collect_data: + @ddt.data( + ('lms.djangoapps.instructor_task.tasks_helper.misc.OraAggregateData.collect_ora2_data', upload_ora2_data), + ('lms.djangoapps.instructor_task.tasks_helper.misc.OraAggregateData.collect_ora2_summary', upload_ora2_summary), + ) + @ddt.unpack + def test_report_fails_if_error(self, data_collector_module, upload_func): + with patch(data_collector_module) as mock_collect_data: mock_collect_data.side_effect = KeyError with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task') as mock_current_task: mock_current_task.return_value = self.current_task - response = upload_ora2_data(None, None, self.course.id, None, 'generated') - assert response == UPDATE_STATUS_FAILED + response = upload_func(None, None, self.course.id, None, 'generated') + self.assertEqual(response, UPDATE_STATUS_FAILED) def test_report_stores_results(self): with ExitStack() as stack: @@ -2628,6 +2633,30 @@ class TestInstructorOra2AttachmentsExport(SharedModuleStoreTestCase): response = upload_ora2_submission_files(None, None, self.course.id, None, 'compressed') assert response == UPDATE_STATUS_FAILED + def test_summary_report_stores_results(self): + with freeze_time('2001-01-01 00:00:00'): + test_header = ['field1', 'field2'] + test_rows = [['row1_field1', 'row1_field2'], ['row2_field1', 'row2_field2']] + + with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task') as mock_current_task: + mock_current_task.return_value = self.current_task + + with patch( + 'lms.djangoapps.instructor_task.tasks_helper.misc.OraAggregateData.collect_ora2_summary' + ) as mock_collect_summary: + mock_collect_summary.return_value = (test_header, test_rows) + with patch( + 'lms.djangoapps.instructor_task.models.DjangoStorageReportStore.store_rows' + ) as mock_store_rows: + return_val = upload_ora2_summary(None, None, self.course.id, None, 'generated') + + timestamp_str = datetime.now(UTC).strftime('%Y-%m-%d-%H%M') + course_id_string = quote(str(self.course.id).replace('/', '_')) + filename = '{}_ORA_summary_{}.csv'.format(course_id_string, timestamp_str) + + self.assertEqual(return_val, UPDATE_STATUS_SUCCEEDED) + mock_store_rows.assert_called_once_with(self.course.id, filename, [test_header] + test_rows) + def test_export_fails_if_error_on_create_zip_step(self): with ExitStack() as stack: mock_current_task = stack.enter_context( diff --git a/lms/templates/instructor/instructor_dashboard_2/data_download.html b/lms/templates/instructor/instructor_dashboard_2/data_download.html index 4170763439..fefcfef668 100644 --- a/lms/templates/instructor/instructor_dashboard_2/data_download.html +++ b/lms/templates/instructor/instructor_dashboard_2/data_download.html @@ -95,6 +95,7 @@ from openedx.core.djangolib.markup import HTML, Text +

${_("Click to generate a ZIP file that contains all submission texts and attachments.")}