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
openedx · Mar 15, 2021 · cec9a5a · cec9a5a
1 parent 4f1bbdf
commit cec9a5a
Show file tree

Hide file tree

Showing 13 changed files with 145 additions and 10 deletions.
diff --git 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/
+bulk-grades-env/
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 == '[email protected]' 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
@@ -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]