From d244715e87c313bd61276243d22ec7dcc542613f Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Sat, 10 Sep 2016 11:27:47 -0400 Subject: [PATCH] Don't sort blocks to retain order. --- lms/djangoapps/grades/models.py | 43 ++++++++++--------- lms/djangoapps/grades/tests/test_models.py | 50 +++++++++++++--------- 2 files changed, 53 insertions(+), 40 deletions(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index 837445a91c..e82568ff11 100644 --- a/lms/djangoapps/grades/models.py +++ b/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. + + Argument 'blocks' should be a BlockRecordList. """ - - if not isinstance(blocks, BlockRecordSet): - blocks = BlockRecordSet(blocks) - 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 diff --git a/lms/djangoapps/grades/tests/test_models.py b/lms/djangoapps/grades/tests/test_models.py index d1d9b5e683..0428dd4bf8 100644 --- a/lms/djangoapps/grades/tests/test_models.py +++ b/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.assertEqual(stored_vblocks.pk, repeat_vblocks.pk) - self.assertEqual(stored_vblocks.hashed, repeat_vblocks.hashed) + self.assertNotEqual(stored_vblocks.pk, repeat_vblocks.pk) + self.assertNotEqual(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