EDUCATOR-4618 | Prevent csv processor objects from having duplicate c…

…olumns.

CHANGELOG.rst

@@ -16,6 +16,10 @@ Unreleased
 
 *
 
+[0.6.0] - 2019-09-10
+~~~~~~~~~~~~~~~~~~~~~
+* Prevent Grade and Intervention CSV processors from producing duplicate columns.
+
 [0.5.10] - 2019-09-06
 ~~~~~~~~~~~~~~~~~~~~~
 * Prevent user from setting negative grades

bulk_grades/__init__.py

@@ -4,6 +4,6 @@
 
 from __future__ import absolute_import, unicode_literals
 
-__version__ = '0.5.11'
+__version__ = '0.6.0'
 
 default_app_config = 'bulk_grades.apps.BulkGradesConfig'  # pylint: disable=invalid-name

bulk_grades/api.py

@@ -4,6 +4,8 @@
 from __future__ import absolute_import, division, unicode_literals
 
 import logging
+from collections import OrderedDict
+from itertools import product
 
 from django.apps import apps
 from django.core.exceptions import ObjectDoesNotExist
@@ -194,30 +196,89 @@ def commit(self, running_task=None):
             grades_api.task_compute_all_grades_for_course.apply_async(kwargs={'course_key': text_type(course_key)})
 
 
-class GradeCSVProcessor(DeferrableMixin, CSVProcessor):
+class GradedSubsectionMixin(object):
+    """
+    Mixin to help generated lists of graded subsections
+    and appropriate column names for each.
+    """
+    def append_columns(self, new_column_names):
+        """
+        Appends items from ``new_column_names`` to ``self.columns``
+        if the item is not already contained therein.
+        """
+        current_columns = set(self.columns)
+        for new_column_name in new_column_names:
+            if new_column_name not in current_columns:
+                self.columns.append(new_column_name)
+
+    @staticmethod
+    def _get_graded_subsections(course_id, filter_subsection=None, filter_assignment_type=None):
+        """
+        Return list of graded subsections.
+
+        If filter_subsection (block usage id) is set, return only that subsection.
+        If filter_assignment_type (string) is set, return only subsections of the appropriate type.
+        """
+        subsections = OrderedDict()
+        for subsection in grades_api.graded_subsections_for_course_id(course_id):
+            block_id = text_type(subsection.location.block_id)
+            if ((filter_subsection and block_id != filter_subsection.block_id)
+                    or filter_assignment_type and filter_assignment_type != text_type(subsection.format)):
+                continue
+            short_block_id = block_id[:8]
+            if short_block_id not in subsections:
+                subsections[short_block_id] = (subsection, subsection.display_name)
+        return subsections
+
+    @staticmethod
+    def _subsection_column_names(short_subsection_ids, prefixes):
+        """
+        Given an iterable of ``short_subsection_ids`` (usually from ``_get_graded_subsections`` above),
+        and ``prefixes`` to append to each, returns a list of names
+        formed from the product of the subsection ids and prefixes.
+        """
+        return ['{}-{}'.format(short_id, prefix) for short_id, prefix in product(prefixes, short_subsection_ids)]
+
+
+class GradeCSVProcessor(DeferrableMixin, GradedSubsectionMixin, CSVProcessor):
     """
     CSV Processor for subsection grades.
     """
 
     required_columns = ['user_id', 'course_id']
+    subsection_prefixes = ('name', 'original_grade', 'previous_override', 'new_override',)
 
     def __init__(self, **kwargs):
         """
         Create GradeCSVProcessor.
         """
+        # First, set some default values.
         self.columns = ['user_id', 'username', 'course_id', 'track', 'cohort']
         self.course_id = None
-        self.subsection_grade_max = self.subsection_grade_min = None
-        self.course_grade_min = self.course_grade_max = None
-        self.subsection = self.track = self.cohort = self._user = None
+        self.subsection_grade_max = None
+        self.subsection_grade_min = None
+        self.course_grade_min = None
+        self.course_grade_max = None
+        self.subsection = None
+        self.track = None
+        self.cohort = None
+        self._user = None
+
+        # The CSVProcessor.__init__ method will set attributes on self
+        # from items in kwargs, so this super().__init__() call will
+        # override any attribute values assigned above.
         super(GradeCSVProcessor, self).__init__(**kwargs)
+
         self._course_key = CourseKey.from_string(self.course_id)
         self._subsection = UsageKey.from_string(self.subsection) if self.subsection else None
         self._subsections = self._get_graded_subsections(
             self._course_key,
             filter_subsection=self._subsection,
             filter_assignment_type=kwargs.get('assignment_type', None),
         )
+        self.append_columns(
+            self._subsection_column_names(self._subsections.keys(), self.subsection_prefixes)
+        )
         self._users_seen = set()
 
     def get_unique_path(self):
@@ -226,26 +287,6 @@ def get_unique_path(self):
         """
         return self.course_id
 
-    def _get_graded_subsections(self, course_id, filter_subsection=None, filter_assignment_type=None):
-        """
-        Return list of graded subsections.
-
-        If filter_subsection (block usage id) is set, return only that subsection.
-        If filter_assignment_type (string) is set, return only subsections of the appropriate type.
-        """
-        subsections = {}
-        for subsection in grades_api.graded_subsections_for_course_id(course_id):
-            block_id = text_type(subsection.location.block_id)
-            if ((filter_subsection and block_id != filter_subsection.block_id)
-                    or filter_assignment_type and filter_assignment_type != text_type(subsection.format)):
-                continue
-            short_block_id = block_id[:8]
-            if short_block_id not in subsections:
-                for key in ('name', 'original_grade', 'previous_override', 'new_override'):
-                    self.columns.append('{}-{}'.format(key, short_block_id))
-                subsections[short_block_id] = (subsection, subsection.display_name)
-        return subsections
-
     def validate_row(self, row):
         """
         Validate row.
@@ -353,56 +394,53 @@ def get_rows_to_export(self):
             yield row
 
 
-class InterventionCSVProcessor(CSVProcessor):
+class InterventionCSVProcessor(GradedSubsectionMixin, CSVProcessor):
     """
     CSV Processor for intervention report grades for masters track only.
     """
 
     MASTERS_TRACK = 'masters'
+    subsection_prefixes = ('name', 'grade',)
 
     def __init__(self, **kwargs):
         """
         Create InterventionCSVProcessor.
         """
-        self.columns = ['user_id', 'username', 'email', 'student_key', 'full_name', 'course_id', 'track', 'cohort',
-                        'number of videos overall', 'number of videos last week', 'number of problems overall',
-                        'number of problems last week',
-                        'number of correct problems overall', 'number of correct problems last week',
-                        'number of problem attempts overall', 'number of problem attempts last week',
-                        'number of forum posts overall', 'number of forum posts last week',
-                        'date last active']
+        # Set some default values for the attributes below
+        self.columns = [
+            'user_id', 'username', 'email', 'student_key', 'full_name', 'course_id', 'track', 'cohort',
+            'number of videos overall', 'number of videos last week', 'number of problems overall',
+            'number of problems last week',
+            'number of correct problems overall', 'number of correct problems last week',
+            'number of problem attempts overall', 'number of problem attempts last week',
+            'number of forum posts overall', 'number of forum posts last week',
+            'date last active',
+        ]
         self.course_id = None
-        self.cohort = self.subsection = \
-            self.assignment_type = self.subsection_grade_min = \
-            self.subsection_grade_max = self.course_grade_min = \
-            self.course_grade_max = None
+        self.cohort = None
+        self.subsection = None
+        self.assignment_type = None
+        self.subsection_grade_min = None
+        self.subsection_grade_max = None
+        self.course_grade_min = None
+        self.course_grade_max = None
+
+        # The CSVProcessor.__init__ method will set attributes on self
+        # from items in kwargs, so this super().__init__() call will
+        # potentially override any attribute values assigned above.
         super(InterventionCSVProcessor, self).__init__(**kwargs)
+
         self._course_key = CourseKey.from_string(self.course_id)
         self._subsection = UsageKey.from_string(self.subsection) if self.subsection else None
         self._subsections = self._get_graded_subsections(
             self._course_key,
             filter_subsection=self._subsection,
-            filter_assignment_type=self.assignment_type
+            filter_assignment_type=kwargs.get('assignment_type', None),
         )
-        self.columns.append('course grade letter')
-        self.columns.append('course grade numeric')
-
-    def _get_graded_subsections(self, course_id, filter_subsection=None, filter_assignment_type=None):
-        """
-        Return list of graded subsections.
-        """
-        subsections = {}
-        for subsection in grades_api.graded_subsections_for_course_id(course_id):
-            block_id = text_type(subsection.location.block_id)
-            if ((filter_subsection and block_id != filter_subsection.block_id)
-                    or filter_assignment_type and filter_assignment_type != text_type(subsection.format)):
-                continue
-            short_block_id = block_id[:8]
-            if short_block_id not in subsections:
-                for key in ('name', 'grade'):
-                    self.columns.append('{}-{}'.format(key, short_block_id))
-                subsections[short_block_id] = (subsection, subsection.display_name)
-        return subsections
+        self.append_columns(
+            self._subsection_column_names(self._subsections.keys(), self.subsection_prefixes)
+        )
+        self.append_columns(('course grade letter', 'course grade numeric'))
 
     def get_rows_to_export(self):
         """

tests/test_api.py

@@ -3,9 +3,10 @@
 """
 Tests for the `edx-bulk-grades` api module.
 """
-
 from __future__ import absolute_import, unicode_literals
 
+from copy import deepcopy
+
 from django.contrib.auth.models import User
 from django.test import TestCase
 from mock import MagicMock, Mock, patch
@@ -19,19 +20,22 @@ class BaseTests(TestCase):
     """
     Common setup functionality for all test cases
     """
-    def setUp(self):
-        super(BaseTests, self).setUp()
-        self.learner = User.objects.create(username='[email protected]')
-        self.staff = User.objects.create(username='[email protected]')
-        self.block_id_in_module_id = '85bb02dbd2c14ba5bc31a0264b140dda'
-        self.block_id = 'block-v1:testX+sg101+2019+type@test+block@%s' % self.block_id_in_module_id
-        self.course_id = 'course-v1:testX+sg101+2019'
-
-    def _make_enrollments(self):
+    @classmethod
+    def setUpTestData(cls):
+        super(BaseTests, cls).setUpTestData()
+        cls.learner = User.objects.create(username='[email protected]')
+        cls.staff = User.objects.create(username='[email protected]')
+        cls.block_id_in_module_id = '85bb02dbd2c14ba5bc31a0264b140dda'
+        cls.block_id = 'block-v1:testX+sg101+2019+type@test+block@%s' % cls.block_id_in_module_id
+        cls.course_id = 'course-v1:testX+sg101+2019'
+        cls._make_enrollments()
+
+    @classmethod
+    def _make_enrollments(cls):
         for name in ['audit', 'verified', 'masters']:
             user = User.objects.create(username='%[email protected]' % name)
             Profile.objects.create(user=user, name=name)
-            enroll = CourseEnrollment.objects.create(course_id=self.course_id, user=user, mode=name)
+            enroll = CourseEnrollment.objects.create(course_id=cls.course_id, user=user, mode=name)
             if name == 'masters':
                 ProgramCourseEnrollment.objects.create(course_enrollment=enroll)
 
@@ -73,7 +77,6 @@ def _get_row(self, **kwargs):
         return row
 
     def test_export(self):
-        self._make_enrollments()
         processor = api.ScoreCSVProcessor(block_id=self.block_id)
         rows = list(processor.get_iterator())
         assert len(rows) == 4
@@ -128,17 +131,27 @@ class TestGradeProcessor(BaseTests):
     """
     NUM_USERS = 3
 
-    @patch('lms.djangoapps.grades.api.CourseGradeFactory.read')
-    def test_export(self, course_grade_factory_mock):
-        self._make_enrollments()
-        course_grade_factory_mock.return_value = Mock(percent=0.50)
+    @patch('lms.djangoapps.grades.api.CourseGradeFactory.read', return_value=Mock(percent=0.50))
+    def test_export(self, course_grade_factory_mock):  # pylint: disable=unused-argument
         processor = api.GradeCSVProcessor(course_id=self.course_id)
         rows = list(processor.get_iterator())
         assert len(rows) == self.NUM_USERS + 1
 
+    def test_columns_not_duplicated_during_init(self):
+        """
+        Tests that GradeCSVProcessor.__init__() does not cause
+        column names to be duplicated.
+        """
+        processor_1 = api.GradeCSVProcessor(course_id=self.course_id)
+
+        # pretend that we serialize the processor data to some "state"
+        state = deepcopy(processor_1.__dict__)
+        processor_2 = api.GradeCSVProcessor(**state)
+
+        assert processor_1.columns == processor_2.columns
+
     @patch('lms.djangoapps.grades.api.graded_subsections_for_course_id')
     def test_subsection_max_min(self, mock_graded_subsections):
-        self._make_enrollments()
         subsection = MagicMock()
         subsection.location = MagicMock()
         subsection.display_name = 'asdf'
@@ -155,8 +168,6 @@ def test_subsection_max_min(self, mock_graded_subsections):
 
     @patch('lms.djangoapps.grades.api.CourseGradeFactory.read')
     def test_course_grade_filters(self, course_grade_factory_mock):
-        self._make_enrollments()
-
         course_grade_factory_mock.return_value = Mock(percent=0.50)
 
         processor = api.GradeCSVProcessor(course_id=self.course_id, max_points=100, course_grade_min=60)
@@ -169,8 +180,6 @@ def test_course_grade_filters(self, course_grade_factory_mock):
 
     @patch('lms.djangoapps.grades.api.CourseGradeFactory.read')
     def test_less_than_zero(self, course_grade_factory_mock):
-        self._make_enrollments()
-
         course_grade_factory_mock.return_value = Mock(percent=0.50)
         processor = api.GradeCSVProcessor(course_id=self.course_id)
 
@@ -193,7 +202,6 @@ class TestInterventionProcessor(BaseTests):
     @patch('lms.djangoapps.grades.api.CourseGradeFactory.read')
     @patch('bulk_grades.api.LearnerAPIClient')
     def test_export(self, mocked_api, mocked_course_grade_factory):
-        self._make_enrollments()
         data = {
                 '[email protected]': {'videos_overall': 2, 'videos_last_week': 0, 'problems_overall': 10,
                                       'problems_last_week': 5,
@@ -216,3 +224,16 @@ def test_export(self, mocked_api, mocked_course_grade_factory):
         processor = api.InterventionCSVProcessor(course_id=self.course_id)
         rows = list(processor.get_iterator())
         assert len(rows) == 2
+
+    def test_columns_not_duplicated_during_init(self):
+        """
+        Tests that InterventionCSVProcessor.__init__() does not cause
+        column names to be duplicated.
+        """
+        processor_1 = api.InterventionCSVProcessor(course_id=self.course_id)
+
+        # pretend that we serialize the processor data to some "state"
+        state = deepcopy(processor_1.__dict__)
+        processor_2 = api.InterventionCSVProcessor(**state)
+
+        assert processor_1.columns == processor_2.columns