From cec9a5a121a2da0914f7595087b86b47d11dd4f5 Mon Sep 17 00:00:00 2001 From: Nathan Sprenkle Date: Mon, 15 Mar 2021 11:24:52 -0400 Subject: [PATCH] Grade override history column filtering (#71) * Only return modified columns in override report * Update to super-csv 2.0.1 to allow for column filtering * Update docs and .gitignore * Bump to version 0.8.7 --- .gitignore | 4 +- CHANGELOG.rst | 5 +++ bulk_grades/__init__.py | 2 +- bulk_grades/api.py | 30 +++++++++++++ bulk_grades/views.py | 1 + requirements/base.txt | 2 +- requirements/dev.txt | 2 +- requirements/doc.txt | 2 +- requirements/pii_check.txt | 2 +- requirements/quality.txt | 2 +- requirements/test.txt | 2 +- tests/test_api.py | 90 +++++++++++++++++++++++++++++++++++++- tests/test_views.py | 11 +++++ 13 files changed, 145 insertions(+), 10 deletions(-) diff --git a/.gitignore b/.gitignore index f51d96b..aecd24d 100644 --- a/.gitignore +++ b/.gitignore @@ -45,6 +45,7 @@ pii_report .project .pycharm_helpers/ .pydevproject +.vscode # The Silver Searcher .agignore @@ -77,6 +78,7 @@ tests/__init__.py # Development task artifacts default.db +csv/ # virtualenvs -bulk-grades-env/ \ No newline at end of file +bulk-grades-env/ diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 2f8809b..dbb16df 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,11 @@ Change Log Unreleased ~~~~~~~~~~ +[0.8.7] - 2021-03-15 +~~~~~~~~~~~~~~~~~~~~~ +* Upgrade super-csv to 2.0.1 +* Only show modified subsections in the Bulk Grade Override Report. + [0.8.6] - 2021-01-22 ~~~~~~~~~~~~~~~~~~~~~ * Added a management command ``install-local`` that will install your local code into devstack LMS diff --git a/bulk_grades/__init__.py b/bulk_grades/__init__.py index b526472..4c5c684 100644 --- a/bulk_grades/__init__.py +++ b/bulk_grades/__init__.py @@ -2,6 +2,6 @@ Support for bulk scoring and grading. """ -__version__ = '0.8.6' +__version__ = '0.8.7' default_app_config = 'bulk_grades.apps.BulkGradesConfig' # pylint: disable=invalid-name diff --git a/bulk_grades/api.py b/bulk_grades/api.py index 4f21841..253536c 100644 --- a/bulk_grades/api.py +++ b/bulk_grades/api.py @@ -437,6 +437,36 @@ def get_rows_to_export(self): row[f'grade-{block_id}'] = effective_grade yield row + def filtered_column_headers(self): + """ + To trim down grade exports, only show subsections which were modified in a bulk update. + Returns: a filtered list of columns to export, preserving modified and non-subsection columns + """ + unmodified_subsections = set(self._subsections.keys()) + + for row in self.result_data: + # Ignore rows which didn't introduce changes + if row['status'] == 'No Action': + continue + + subsections_modified_by_row = set() + + # Get changes this row introduced for an as-yet-unmodified subsection + for subsection in unmodified_subsections: + if row[f'new_override-{subsection}'].strip() != '': + subsections_modified_by_row.add(subsection) + + # Remove modified subsections from the unmodified list + if subsections_modified_by_row: + unmodified_subsections.difference_update(subsections_modified_by_row) + + # Find and remove all column names referring to unmodified subsections, preserving others + columns = self.columns.copy() + for unmodified_column in self._subsection_column_names(unmodified_subsections, self.subsection_prefixes): + columns.remove(unmodified_column) + + return columns + class InterventionCSVProcessor(GradedSubsectionMixin, CSVProcessor): """ diff --git a/bulk_grades/views.py b/bulk_grades/views.py index 459e7f1..9b2f0d7 100644 --- a/bulk_grades/views.py +++ b/bulk_grades/views.py @@ -114,6 +114,7 @@ def initialize_processor(self, request, course_id): operation_id = request.GET.get('error_id', '') if operation_id: self.processor = api.GradeCSVProcessor.load(operation_id) + self.processor.columns = self.processor.filtered_column_headers() if self.processor.course_id != course_id: return HttpResponseForbidden() self.extra_filename = 'graded-results' diff --git a/requirements/base.txt b/requirements/base.txt index 19d5944..0bad64e 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -100,7 +100,7 @@ stevedore==3.3.0 # via # edx-django-utils # edx-opaque-keys -super-csv==2.0.0 +super-csv==2.0.1 # via -r requirements/base.in urllib3==1.26.3 # via requests diff --git a/requirements/dev.txt b/requirements/dev.txt index 7362c12..3277ec9 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -328,7 +328,7 @@ stevedore==3.3.0 # code-annotations # edx-django-utils # edx-opaque-keys -super-csv==2.0.0 +super-csv==2.0.1 # via -r requirements/quality.txt text-unidecode==1.3 # via diff --git a/requirements/doc.txt b/requirements/doc.txt index 5d2d70c..bf1c180 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -294,7 +294,7 @@ stevedore==3.3.0 # doc8 # edx-django-utils # edx-opaque-keys -super-csv==2.0.0 +super-csv==2.0.1 # via -r requirements/test.txt text-unidecode==1.3 # via diff --git a/requirements/pii_check.txt b/requirements/pii_check.txt index 4f91386..1f9b675 100644 --- a/requirements/pii_check.txt +++ b/requirements/pii_check.txt @@ -162,7 +162,7 @@ stevedore==3.3.0 # code-annotations # edx-django-utils # edx-opaque-keys -super-csv==2.0.0 +super-csv==2.0.1 # via -r requirements/base.txt text-unidecode==1.3 # via python-slugify diff --git a/requirements/quality.txt b/requirements/quality.txt index dee5c37..96f0da3 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -254,7 +254,7 @@ stevedore==3.3.0 # code-annotations # edx-django-utils # edx-opaque-keys -super-csv==2.0.0 +super-csv==2.0.1 # via -r requirements/test.txt text-unidecode==1.3 # via diff --git a/requirements/test.txt b/requirements/test.txt index 5b78160..3df9455 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -183,7 +183,7 @@ stevedore==3.3.0 # code-annotations # edx-django-utils # edx-opaque-keys -super-csv==2.0.0 +super-csv==2.0.1 # via -r requirements/base.txt text-unidecode==1.3 # via python-slugify diff --git a/tests/test_api.py b/tests/test_api.py index b67b3d0..8b3aed9 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -6,7 +6,7 @@ import datetime from copy import deepcopy -from itertools import chain, cycle, repeat +from itertools import chain, cycle, product, repeat from unittest.mock import MagicMock, Mock, patch import ddt @@ -63,6 +63,32 @@ def _mock_graded_subsections(self): return_value.append(subsection) return return_value + def _mock_result_data(self, override=True): + """ + Return some row data that mocks what would be loaded from an override history CSV + """ + result_data = [] + prefixes = ['name', 'original_grade', 'previous_override', 'new_override'] + subsections = ['homework', 'lab_ques'] + + for learner in self.learners: + row = { + 'user_id': learner.id, + 'username': learner.username, + 'student_key': '', + 'course_id': self.course_id, + 'track': 'masters' if learner.username == 'masters@example.com' else 'audit', + 'cohort': '', + 'error': '', + 'status': 'No Action' + } + # Add subsection data + for short_id, prefix in product(subsections, prefixes): + row[f'{prefix}-{short_id}'] = '' + result_data.append(row) + + return result_data + class TestApi(BaseTests): """ @@ -400,7 +426,6 @@ def test_no_subsection_grade(self, mock_graded_subsections): row = rows[i].split(',') assert row[grade_column_index] == '1' - @patch('lms.djangoapps.grades.api.CourseGradeFactory.read') def test_course_grade_filters(self, course_grade_factory_mock): course_grade_factory_mock.side_effect = cycle((Mock(percent=0.50), Mock(percent=0.70), Mock(percent=0.90))) @@ -530,6 +555,67 @@ def test_assignment_grade(self, mocked_graded_subsections, mocked_get_subsection assert learner_data_row['original_grade-lab_ques'] == '3' assert learner_data_row['previous_override-lab_ques'] == '5' + @patch('lms.djangoapps.grades.api.graded_subsections_for_course_id') + def test_filter_override_history_columns(self, mocked_graded_subsections): + # Given 2 graded subsections ... + mocked_graded_subsections.return_value = self._mock_graded_subsections() + processor = api.GradeCSVProcessor(course_id=self.course_id) + processor.result_data = self._mock_result_data() + + # One of which, "homework", was overridden for 2 students + processor.result_data[0].update({'new_override-homework': '1', 'status': 'Success'}) + processor.result_data[2].update({'new_override-homework': '2', 'status': 'Success'}) + + # When columns are filtered and I request a copy of the report + processor.columns = processor.filtered_column_headers() + rows = list(processor.get_iterator(error_data='1')) + + # Then my headers include the modified subsection headers, and exclude the unmodified section + headers = rows[0].strip().split(',') + expected_headers = [ + 'user_id', + 'username', + 'student_key', + 'course_id', + 'track', + 'cohort', + 'name-homework', + 'grade-homework', + 'original_grade-homework', + 'previous_override-homework', + 'new_override-homework', + 'status', + 'error'] + + assert headers == expected_headers + assert len(rows) == self.NUM_USERS + 1 + + @patch('lms.djangoapps.grades.api.graded_subsections_for_course_id') + def test_filter_override_history_noop(self, mocked_graded_subsections): + # Given no overrides for a given report + mocked_graded_subsections.return_value = self._mock_graded_subsections() + processor = api.GradeCSVProcessor(course_id=self.course_id) + processor.result_data = self._mock_result_data() + + # When columns are filtered and I request a copy of the report + processor.columns = processor.filtered_column_headers() + rows = list(processor.get_iterator(error_data='1')) + + # Then my headers don't include any subsections + headers = rows[0].strip().split(',') + expected_headers = [ + 'user_id', + 'username', + 'student_key', + 'course_id', + 'track', + 'cohort', + 'status', + 'error'] + + assert headers == expected_headers + assert len(rows) == self.NUM_USERS + 1 + @ddt.ddt class TestInterventionProcessor(BaseTests): diff --git a/tests/test_views.py b/tests/test_views.py index bab692d..32daec3 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -58,6 +58,17 @@ def test_get(self, mock_grade_factory): for row in data: self.assertNotIn(inactive_learner.username, str(row)) + @patch.object(GradeCSVProcessor, 'load') + @patch.object(GradeCSVProcessor, 'filtered_column_headers') + def test_get_history_filters_columns(self, csv_load, filter_columns): + # When I request the bulk grade history (error_id set in query) + self.client.login(username=self.staff.username, password=self.password) + response = self.client.get(f'{reverse("bulk_grades", args=[self.course_id])}?error_id=1') + + # Assert that column filtering is called for the history report... + self.assertEqual(response.status_code, 200) + filter_columns.assert_called() + def test_post(self): csv_content = 'user_id,username,course_id,track,cohort' csv_content += ',new_override-' + self.subsection_short_ids[0]