Don't sort blocks to retain order.

lms/djangoapps/grades/models.py

@@ -29,13 +29,16 @@ log = logging.getLogger(__name__)
 BlockRecord = namedtuple('BlockRecord', ['locator', 'weight', 'max_score'])
 
 
-class BlockRecordSet(frozenset):
+class BlockRecordList(tuple):
     """
-    An immutable ordered collection of BlockRecord objects.
+    An immutable ordered list of BlockRecord objects.
     """
 
-    def __init__(self, *args, **kwargs):
-        super(BlockRecordSet, self).__init__(*args, **kwargs)
+    def __new__(cls, blocks):
+        return super(BlockRecordList, cls).__new__(cls, tuple(blocks))
+
+    def __init__(self, blocks):
+        super(BlockRecordList, self).__init__(blocks)
         self._json = None
         self._hash = None
 
@@ -56,8 +59,7 @@ class BlockRecordSet(frozenset):
         stable ordering.
         """
         if self._json is None:
-            sorted_blocks = sorted(self, key=attrgetter('locator'))
-            list_of_block_dicts = [block._asdict() for block in sorted_blocks]
+            list_of_block_dicts = [block._asdict() for block in self]
             course_key_string = self._get_course_key_string()  # all blocks are from the same course
 
             for block_dict in list_of_block_dicts:
@@ -77,7 +79,7 @@ class BlockRecordSet(frozenset):
     @classmethod
     def from_json(cls, blockrecord_json):
         """
-        Return a BlockRecordSet from a json list.
+        Return a BlockRecordList from a previously serialized json.
         """
         data = json.loads(blockrecord_json)
         course_key = data['course_key']
@@ -97,6 +99,13 @@ class BlockRecordSet(frozenset):
         )
         return cls(record_generator)
 
+    @classmethod
+    def from_list(cls, blocks):
+        """
+        Return a BlockRecordList from a list.
+        """
+        return cls(tuple(blocks))
+
     def to_hash(self):
         """
         Return a hashed version of the list of block records.
@@ -120,24 +129,18 @@ class VisibleBlocksQuerySet(models.QuerySet):
         """
         Creates a new VisibleBlocks model object.
 
-        Argument 'blocks' should be a BlockRecordSet.
+        Argument 'blocks' should be a BlockRecordList.
         """
-
-        if not isinstance(blocks, BlockRecordSet):
-            blocks = BlockRecordSet(blocks)
-
         model, _ = self.get_or_create(hashed=blocks.to_hash(), defaults={'blocks_json': blocks.to_json()})
         return model
 
     def hash_from_blockrecords(self, blocks):
         """
-        Return the hash for a given BlockRecordSet, serializing the records if
+        Return the hash for a given list of blocks, saving the records if
         possible, but returning the hash even if an IntegrityError occurs.
-        """
-
-        if not isinstance(blocks, BlockRecordSet):
-            blocks = BlockRecordSet(blocks)
 
+        Argument 'blocks' should be a BlockRecordList.
+        """
         try:
             with transaction.atomic():
                 model = self.create_from_blockrecords(blocks)
@@ -176,7 +179,7 @@ class VisibleBlocks(models.Model):
         Returns the blocks_json data stored on this model as a list of
         BlockRecords in the order they were provided.
         """
-        return BlockRecordSet.from_json(self.blocks_json)
+        return BlockRecordList.from_json(self.blocks_json)
 
 
 class PersistentSubsectionGradeQuerySet(models.QuerySet):
@@ -204,7 +207,7 @@ class PersistentSubsectionGradeQuerySet(models.QuerySet):
         if not kwargs.get('course_id', None):
             kwargs['course_id'] = kwargs['usage_key'].course_key
 
-        visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(blocks=visible_blocks)
+        visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(BlockRecordList.from_list(visible_blocks))
         return super(PersistentSubsectionGradeQuerySet, self).create(
             visible_blocks_id=visible_blocks_hash,
             **kwargs
@@ -358,7 +361,7 @@ class PersistentSubsectionGrade(TimeStampedModel):
         Modify an existing PersistentSubsectionGrade object, saving the new
         version.
         """
-        visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(blocks=visible_blocks)
+        visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(BlockRecordList.from_list(visible_blocks))
 
         self.course_version = course_version or ""
         self.subtree_edited_timestamp = subtree_edited_timestamp

lms/djangoapps/grades/tests/test_models.py

@@ -14,27 +14,27 @@ from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator
 
 from lms.djangoapps.grades.models import (
     BlockRecord,
-    BlockRecordSet,
+    BlockRecordList,
     PersistentSubsectionGrade,
     VisibleBlocks
 )
 
 
-class BlockRecordSetTestCase(TestCase):
+class BlockRecordListTestCase(TestCase):
     """
-    Verify the behavior of BlockRecordSets, particularly around edge cases
+    Verify the behavior of BlockRecordList, particularly around edge cases
     """
     empty_json = '{"blocks":[],"course_key":null}'
 
     def test_empty_block_record_set(self):
-        brs = BlockRecordSet()
+        brs = BlockRecordList(())
         self.assertFalse(brs)
         self.assertEqual(
             brs.to_json(),
             self.empty_json
         )
         self.assertEqual(
-            BlockRecordSet.from_json(self.empty_json),
+            BlockRecordList.from_json(self.empty_json),
             brs
         )
 
@@ -108,11 +108,17 @@ class VisibleBlocksTest(GradesModelTestCase):
     """
     Test the VisibleBlocks model.
     """
+    def _create_block_record_list(self, blocks):
+        """
+        Creates and returns a BlockRecordList for the given blocks.
+        """
+        return VisibleBlocks.objects.create_from_blockrecords(BlockRecordList.from_list(blocks))
+
     def test_creation(self):
         """
         Happy path test to ensure basic create functionality works as expected.
         """
-        vblocks = VisibleBlocks.objects.create_from_blockrecords([self.record_a])
+        vblocks = self._create_block_record_list([self.record_a])
         list_of_block_dicts = [self.record_a._asdict()]
         for block_dict in list_of_block_dicts:
             block_dict['locator'] = unicode(block_dict['locator'])  # BlockUsageLocator is not json-serializable
@@ -128,30 +134,34 @@ class VisibleBlocksTest(GradesModelTestCase):
         self.assertEqual(expected_json, vblocks.blocks_json)
         self.assertEqual(expected_hash, vblocks.hashed)
 
-    def test_ordering_does_not_matter(self):
+    def test_ordering_matters(self):
         """
-        When creating new vblocks, a different ordering of blocks produces the
-        same record in the database.
+        When creating new vblocks, different ordering of blocks produces
+        different records in the database.
         """
-        stored_vblocks = VisibleBlocks.objects.create_from_blockrecords([self.record_a, self.record_b])
-        repeat_vblocks = VisibleBlocks.objects.create_from_blockrecords([self.record_b, self.record_a])
-        new_vblocks = VisibleBlocks.objects.create_from_blockrecords([self.record_b])
+        stored_vblocks = self._create_block_record_list([self.record_a, self.record_b])
+        repeat_vblocks = self._create_block_record_list([self.record_b, self.record_a])
+        same_order_vblocks = self._create_block_record_list([self.record_a, self.record_b])
+        new_vblocks = self._create_block_record_list([self.record_b])
+
+        self.assertNotEqual(stored_vblocks.pk, repeat_vblocks.pk)
+        self.assertNotEqual(stored_vblocks.hashed, repeat_vblocks.hashed)
 
-        self.assertEqual(stored_vblocks.pk, repeat_vblocks.pk)
-        self.assertEqual(stored_vblocks.hashed, repeat_vblocks.hashed)
+        self.assertEquals(stored_vblocks.pk, same_order_vblocks.pk)
+        self.assertEquals(stored_vblocks.hashed, same_order_vblocks.hashed)
 
         self.assertNotEqual(stored_vblocks.pk, new_vblocks.pk)
         self.assertNotEqual(stored_vblocks.hashed, new_vblocks.hashed)
 
     def test_blocks_property(self):
         """
-        Ensures that, given an array of BlockRecord, creating visible_blocks and accessing
-        visible_blocks.blocks yields a copy of the initial array. Also, trying to set the blocks property should raise
-        an exception.
+        Ensures that, given an array of BlockRecord, creating visible_blocks
+        and accessing visible_blocks.blocks yields a copy of the initial array.
+        Also, trying to set the blocks property should raise an exception.
         """
-        expected_blocks = [self.record_a, self.record_b]
-        visible_blocks = VisibleBlocks.objects.create_from_blockrecords(expected_blocks)
-        self.assertEqual(BlockRecordSet(expected_blocks), visible_blocks.blocks)
+        expected_blocks = (self.record_a, self.record_b)
+        visible_blocks = self._create_block_record_list(expected_blocks)
+        self.assertEqual(expected_blocks, visible_blocks.blocks)
         with self.assertRaises(AttributeError):
             visible_blocks.blocks = expected_blocks