From 4dbbe513be7db5b6785954d44b085dc9b905dbfe Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Wed, 14 Sep 2016 10:31:48 -0400 Subject: [PATCH] Reduce sql queries with persisted grades TNL-5458 TNL-5493 --- lms/djangoapps/courseware/views/views.py | 2 +- lms/djangoapps/grades/models.py | 365 ++++++++--------- lms/djangoapps/grades/new/course_grade.py | 60 +-- lms/djangoapps/grades/new/subsection_grade.py | 367 +++++++++++------- lms/djangoapps/grades/scores.py | 34 +- lms/djangoapps/grades/signals/handlers.py | 5 +- lms/djangoapps/grades/tests/test_grades.py | 6 +- lms/djangoapps/grades/tests/test_models.py | 99 ++--- lms/djangoapps/grades/tests/test_new.py | 50 +-- 9 files changed, 526 insertions(+), 462 deletions(-) diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 92ba91eef9..611a8833d8 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -719,7 +719,7 @@ def _progress(request, course_key, student_id): # additional DB lookup (this kills the Progress page in particular). student = User.objects.prefetch_related("groups").get(id=student.id) - course_grade = CourseGradeFactory(student).create(course) + course_grade = CourseGradeFactory(student).create(course, read_only=False) if not course_grade.has_access_to_course: # This means the student didn't have access to the course (which the instructor requested) raise Http404 diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index e88e7fccc6..dfb415e412 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -9,11 +9,10 @@ from base64 import b64encode from collections import namedtuple from hashlib import sha1 import json +from lazy import lazy import logging -from operator import attrgetter -from django.db import models, transaction -from django.db.utils import IntegrityError +from django.db import models from model_utils.models import TimeStampedModel from coursewarehistoryextended.fields import UnsignedBigIntAutoField @@ -34,60 +33,62 @@ class BlockRecordList(tuple): An immutable ordered list of BlockRecord objects. """ - def __new__(cls, blocks): - return super(BlockRecordList, cls).__new__(cls, tuple(blocks)) + def __new__(cls, blocks, course_key): # pylint: disable=unused-argument + return super(BlockRecordList, cls).__new__(cls, blocks) - def __init__(self, blocks): + def __init__(self, blocks, course_key): super(BlockRecordList, self).__init__(blocks) - self._json = None - self._hash = None + self.course_key = course_key - def _get_course_key_string(self): - """ - Get the course key as a string. All blocks are from the same course, - so just grab one arbitrarily. If no blocks are present, return None. - """ - if self: - a_block = next(iter(self)) - return unicode(a_block.locator.course_key) - else: - return None + def __eq__(self, other): + assert isinstance(other, BlockRecordList) + return hash(self) == hash(other) - def to_json(self): + def __hash__(self): + """ + Returns an integer Type value of the hash of this + list of block records, as required by python. + """ + return hash(self.hash_value) + + @lazy + def hash_value(self): + """ + Returns a hash value of the list of block records. + + This currently hashes using sha1, and returns a base64 encoded version + of the binary digest. In the future, different algorithms could be + supported by adding a label indicated which algorithm was used, e.g., + "sha256$j0NDRmSPa5bfid2pAcUXaxCm2Dlh3TwayItZstwyeqQ=". + """ + return b64encode(sha1(self.json_value).digest()) + + @lazy + def json_value(self): """ Return a JSON-serialized version of the list of block records, using a stable ordering. """ - if self._json is None: - 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: - block_dict['locator'] = unicode(block_dict['locator']) # BlockUsageLocator is not json-serializable - data = { - 'course_key': course_key_string, - 'blocks': list_of_block_dicts, - } - - self._json = json.dumps( - data, - separators=(',', ':'), # Remove spaces from separators for more compact representation - sort_keys=True, - ) - return self._json + list_of_block_dicts = [block._asdict() for block in self] + for block_dict in list_of_block_dicts: + block_dict['locator'] = unicode(block_dict['locator']) # BlockUsageLocator is not json-serializable + data = { + 'course_key': unicode(self.course_key), + 'blocks': list_of_block_dicts, + } + return json.dumps( + data, + separators=(',', ':'), # Remove spaces from separators for more compact representation + sort_keys=True, + ) @classmethod def from_json(cls, blockrecord_json): """ - Return a BlockRecordList from a previously serialized json. + Return a BlockRecordList from previously serialized json. """ data = json.loads(blockrecord_json) - course_key = data['course_key'] - if course_key is not None: - course_key = CourseKey.from_string(course_key) - else: - # If there was no course key, there are no blocks. - assert len(data['blocks']) == 0 + course_key = CourseKey.from_string(data['course_key']) block_dicts = data['blocks'] record_generator = ( BlockRecord( @@ -97,27 +98,14 @@ class BlockRecordList(tuple): ) for block in block_dicts ) - return cls(record_generator) + return cls(record_generator, course_key) @classmethod - def from_list(cls, blocks): + def from_list(cls, blocks, course_key): """ - Return a BlockRecordList from a list. + Return a BlockRecordList from the given list and course_key. """ - return cls(tuple(blocks)) - - def to_hash(self): - """ - Return a hashed version of the list of block records. - - This currently hashes using sha1, and returns a base64 encoded version - of the binary digest. In the future, different algorithms could be - supported by adding a label indicated which algorithm was used, e.g., - "sha256$j0NDRmSPa5bfid2pAcUXaxCm2Dlh3TwayItZstwyeqQ=". - """ - if self._hash is None: - self._hash = b64encode(sha1(self.to_json()).digest()) - return self._hash + return cls(blocks, course_key) class VisibleBlocksQuerySet(models.QuerySet): @@ -131,27 +119,12 @@ class VisibleBlocksQuerySet(models.QuerySet): Argument 'blocks' should be a BlockRecordList. """ - model, _ = self.get_or_create(hashed=blocks.to_hash(), defaults={'blocks_json': blocks.to_json()}) + model, _ = self.get_or_create( + hashed=blocks.hash_value, + defaults={'blocks_json': blocks.json_value, 'course_id': blocks.course_key}, + ) return model - def hash_from_blockrecords(self, blocks): - """ - 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. - """ - try: - with transaction.atomic(): - model = self.create_from_blockrecords(blocks) - except IntegrityError: - # If an integrity error occurs, the VisibleBlocks model we want to - # create already exists. The hash is still the correct value. - return blocks.to_hash() - else: - # No error occurred - return model.hashed - class VisibleBlocks(models.Model): """ @@ -164,6 +137,7 @@ class VisibleBlocks(models.Model): """ blocks_json = models.TextField() hashed = models.CharField(max_length=100, unique=True) + course_id = CourseKeyField(blank=False, max_length=255, db_index=True) objects = VisibleBlocksQuerySet.as_manager() @@ -181,37 +155,41 @@ class VisibleBlocks(models.Model): """ return BlockRecordList.from_json(self.blocks_json) - -class PersistentSubsectionGradeQuerySet(models.QuerySet): - """ - A custom QuerySet, that handles creating a VisibleBlocks model on creation, and - extracts the course id from the provided usage_key. - """ - def create(self, **kwargs): + @classmethod + def bulk_read(cls, course_key): """ - Instantiates a new model instance after creating a VisibleBlocks instance. + Reads all visible block records for the given course. Arguments: - user_id (int) - usage_key (serialized UsageKey) - course_version (str) - subtree_edited_timestamp (datetime) - earned_all (float) - possible_all (float) - earned_graded (float) - possible_graded (float) - visible_blocks (iterable of BlockRecord) + course_key: The course identifier for the desired records """ - visible_blocks = kwargs.pop('visible_blocks') - kwargs['course_version'] = kwargs.get('course_version', None) or "" - if not kwargs.get('course_id', None): - kwargs['course_id'] = kwargs['usage_key'].course_key + return cls.objects.filter(course_id=course_key) - 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 - ) + @classmethod + def bulk_create(cls, block_record_lists): + """ + Bulk creates VisibleBlocks for the given iterator of + BlockRecordList objects. + """ + return cls.objects.bulk_create([ + VisibleBlocks( + blocks_json=brl.json_value, + hashed=brl.hash_value, + course_id=brl.course_key, + ) + for brl in block_record_lists + ]) + + @classmethod + def bulk_get_or_create(cls, block_record_lists, course_key): + """ + Bulk creates VisibleBlocks for the given iterator of + BlockRecordList objects for the given course_key, but + only for those that aren't already created. + """ + existent_records = {record.hashed: record for record in cls.bulk_read(course_key)} + non_existent_brls = {brl for brl in block_record_lists if brl.hash_value not in existent_records} + cls.bulk_create(non_existent_brls) class PersistentSubsectionGrade(TimeStampedModel): @@ -233,6 +211,10 @@ class PersistentSubsectionGrade(TimeStampedModel): # uniquely identify this particular grade object user_id = models.IntegerField(blank=False) course_id = CourseKeyField(blank=False, max_length=255) + + # note: the usage_key may not have the run filled in for + # old mongo courses. Use the full_usage_key property + # instead when you want to use/compare the usage_key. usage_key = UsageKeyField(blank=False, max_length=255) # Information relating to the state of content when grade was calculated @@ -249,8 +231,15 @@ class PersistentSubsectionGrade(TimeStampedModel): # track which blocks were visible at the time of grade calculation visible_blocks = models.ForeignKey(VisibleBlocks, db_column='visible_blocks_hash', to_field='hashed') - # use custom manager - objects = PersistentSubsectionGradeQuerySet.as_manager() + @property + def full_usage_key(self): + """ + Returns the "correct" usage key value with the run filled in. + """ + if self.usage_key.run is None: # pylint: disable=no-member + return self.usage_key.replace(course_key=self.course_id) + else: + return self.usage_key def __unicode__(self): """ @@ -268,38 +257,6 @@ class PersistentSubsectionGrade(TimeStampedModel): self.possible_all, ) - @classmethod - def save_grade(cls, **kwargs): - """ - Wrapper for create_grade or update_grade, depending on which applies. - Takes the same arguments as both of those methods. - """ - user_id = kwargs.pop('user_id') - usage_key = kwargs.pop('usage_key') - - try: - with transaction.atomic(): - grade, is_created = cls.objects.get_or_create( - user_id=user_id, - course_id=usage_key.course_key, - usage_key=usage_key, - defaults=kwargs, - ) - log.info(u"Persistent Grades: Grade model saved: {0}".format(grade)) - except IntegrityError: - cls.update_grade(user_id=user_id, usage_key=usage_key, **kwargs) - log.warning( - u"Persistent Grades: Integrity error trying to save grade for user: {0}, usage key: {1}, defaults: {2}" - .format( - user_id, - usage_key, - **kwargs - ) - ) - else: - if not is_created: - grade.update(**kwargs) - @classmethod def read_grade(cls, user_id, usage_key): """ @@ -311,74 +268,94 @@ class PersistentSubsectionGrade(TimeStampedModel): Raises PersistentSubsectionGrade.DoesNotExist if applicable """ - return cls.objects.get( + return cls.objects.select_related('visible_blocks').get( user_id=user_id, course_id=usage_key.course_key, # course_id is included to take advantage of db indexes usage_key=usage_key, ) @classmethod - def update_grade( - cls, - user_id, - usage_key, - course_version, - subtree_edited_timestamp, - earned_all, - possible_all, - earned_graded, - possible_graded, - visible_blocks, - ): + def bulk_read_grades(cls, user_id, course_key): """ - Updates a previously existing grade. + Reads all grades for the given user and course. - This is distinct from update() in that `grade.update()` operates on an - existing grade object, while this is a classmethod that pulls the grade - from the database, and then updates it. If you already have a grade - object, use the update() method on that object to avoid an extra - round-trip to the database. Use this classmethod if all you have are a - user and the usage key of an existing grade. - - Requires all the arguments listed in docstring for create_grade + Arguments: + user_id: The user associated with the desired grades + course_key: The course identifier for the desired grades """ - grade = cls.read_grade( + return cls.objects.select_related('visible_blocks').filter( user_id=user_id, + course_id=course_key, + ) + + @classmethod + def update_or_create_grade(cls, **kwargs): + """ + Wrapper for objects.update_or_create. + """ + cls._prepare_params_and_visible_blocks(kwargs) + + user_id = kwargs.pop('user_id') + usage_key = kwargs.pop('usage_key') + + grade, _ = cls.objects.update_or_create( + user_id=user_id, + course_id=usage_key.course_key, usage_key=usage_key, + defaults=kwargs, ) + return grade - grade.update( - course_version=course_version, - subtree_edited_timestamp=subtree_edited_timestamp, - earned_all=earned_all, - possible_all=possible_all, - earned_graded=earned_graded, - possible_graded=possible_graded, - visible_blocks=visible_blocks, - ) - - def update( - self, - course_version, - subtree_edited_timestamp, - earned_all, - possible_all, - earned_graded, - possible_graded, - visible_blocks, - ): + @classmethod + def create_grade(cls, **kwargs): """ - Modify an existing PersistentSubsectionGrade object, saving the new - version. + Wrapper for objects.create. """ - visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(BlockRecordList.from_list(visible_blocks)) + cls._prepare_params_and_visible_blocks(kwargs) + return cls.objects.create(**kwargs) - self.course_version = course_version or "" - self.subtree_edited_timestamp = subtree_edited_timestamp - self.earned_all = earned_all - self.possible_all = possible_all - self.earned_graded = earned_graded - self.possible_graded = possible_graded - self.visible_blocks_id = visible_blocks_hash # pylint: disable=attribute-defined-outside-init - self.save() - log.info(u"Persistent Grades: Grade model updated: {0}".format(self)) + @classmethod + def bulk_create_grades(cls, grade_params_iter, course_key): + """ + Bulk creation of grades. + """ + if not grade_params_iter: + return + + map(cls._prepare_params, grade_params_iter) + VisibleBlocks.bulk_get_or_create([params['visible_blocks'] for params in grade_params_iter], course_key) + map(cls._prepare_params_visible_blocks_id, grade_params_iter) + + return cls.objects.bulk_create([PersistentSubsectionGrade(**params) for params in grade_params_iter]) + + @classmethod + def _prepare_params_and_visible_blocks(cls, params): + """ + Prepares the fields for the grade record, while + creating the related VisibleBlocks, if needed. + """ + cls._prepare_params(params) + params['visible_blocks'] = VisibleBlocks.objects.create_from_blockrecords(params['visible_blocks']) + + @classmethod + def _prepare_params(cls, params): + """ + Prepares the fields for the grade record. + """ + if not params.get('course_id', None): + params['course_id'] = params['usage_key'].course_key + params['course_version'] = params.get('course_version', None) or "" + params['visible_blocks'] = BlockRecordList.from_list(params['visible_blocks'], params['course_id']) + + @classmethod + def _prepare_params_visible_blocks_id(cls, params): + """ + Prepares the visible_blocks_id field for the grade record, + using the hash of the visible_blocks field. Specifying + the hashed field eliminates extra queries to get the + VisibleBlocks record. Use this variation of preparing + the params when you are sure of the existence of the + VisibleBlock. + """ + params['visible_blocks_id'] = params['visible_blocks'].hash_value + del params['visible_blocks'] diff --git a/lms/djangoapps/grades/new/course_grade.py b/lms/djangoapps/grades/new/course_grade.py index 9f85ff8b7f..d9f2837330 100644 --- a/lms/djangoapps/grades/new/course_grade.py +++ b/lms/djangoapps/grades/new/course_grade.py @@ -40,10 +40,7 @@ class CourseGrade(object): graded_total = subsection_grade.graded_total if graded_total.possible > 0: subsections_by_format[subsection_grade.format].append(graded_total) - log.info(u"Persistent Grades: Calculated subsections_by_format. course id: {0}, user: {1}".format( - self.course.location, - self.student.id - )) + self._log_event(log.info, u"subsections_by_format") return subsections_by_format @lazy @@ -55,10 +52,7 @@ class CourseGrade(object): for chapter in self.chapter_grades: for subsection_grade in chapter['sections']: locations_to_weighted_scores.update(subsection_grade.locations_to_weighted_scores) - log.info(u"Persistent Grades: Calculated locations_to_weighted_scores. course id: {0}, user: {1}".format( - self.course.id, - self.student.id - )) + self._log_event(log.info, u"locations_to_weighted_scores") return locations_to_weighted_scores @lazy @@ -72,10 +66,7 @@ class CourseGrade(object): self.subsection_grade_totals_by_format, generate_random_scores=settings.GENERATE_PROFILE_SCORES ) - log.info(u"Persistent Grades: Calculated grade_value. course id: {0}, user: {1}".format( - self.course.location, - self.student.id - )) + self._log_event(log.info, u"grade_value") return grade_value @property @@ -123,31 +114,34 @@ class CourseGrade(object): grade_summary['totaled_scores'] = self.subsection_grade_totals_by_format grade_summary['raw_scores'] = list(self.locations_to_weighted_scores.itervalues()) + self._log_event(log.warning, u"grade_summary, percent: {0}, grade: {1}".format(self.percent, self.letter_grade)) return grade_summary - def compute(self): + def compute_and_update(self, read_only=False): """ Computes the grade for the given student and course. + + If read_only is True, doesn't save any updates to the grades. """ - subsection_grade_factory = SubsectionGradeFactory(self.student) + self._log_event(log.warning, u"compute_and_update, read_only: {}".format(read_only)) + subsection_grade_factory = SubsectionGradeFactory(self.student, self.course, self.course_structure) for chapter_key in self.course_structure.get_children(self.course.location): chapter = self.course_structure[chapter_key] - subsection_grades = [] + chapter_subsection_grades = [] for subsection_key in self.course_structure.get_children(chapter_key): - subsection_grades.append( - subsection_grade_factory.create( - self.course_structure[subsection_key], - self.course_structure, - self.course, - ) + chapter_subsection_grades.append( + subsection_grade_factory.create(self.course_structure[subsection_key], read_only=True) ) self.chapter_grades.append({ 'display_name': block_metadata_utils.display_name_with_default_escaped(chapter), 'url_name': block_metadata_utils.url_name_for_block(chapter), - 'sections': subsection_grades + 'sections': chapter_subsection_grades }) + if not read_only: + subsection_grade_factory.bulk_create_unsaved() + self._signal_listeners_when_grade_computed() def score_for_module(self, location): @@ -212,6 +206,16 @@ class CourseGrade(object): receiver, response ) + def _log_event(self, log_func, log_statement): + """ + Logs the given statement, for this instance. + """ + log_func(u"Persistent Grades: CourseGrade.{0}, course: {1}, user: {2}".format( + log_statement, + self.course.id, + self.student.id + )) + class CourseGradeFactory(object): """ @@ -220,22 +224,26 @@ class CourseGradeFactory(object): def __init__(self, student): self.student = student - def create(self, course): + def create(self, course, read_only=False): """ Returns the CourseGrade object for the given student and course. + + If read_only is True, doesn't save any updates to the grades. """ course_structure = get_course_blocks(self.student, course.location) return ( self._get_saved_grade(course, course_structure) or - self._compute_and_update_grade(course, course_structure) + self._compute_and_update_grade(course, course_structure, read_only) ) - def _compute_and_update_grade(self, course, course_structure): + def _compute_and_update_grade(self, course, course_structure, read_only): """ Freshly computes and updates the grade for the student and course. + + If read_only is True, doesn't save any updates to the grades. """ course_grade = CourseGrade(self.student, course, course_structure) - course_grade.compute() + course_grade.compute_and_update(read_only) return course_grade def _get_saved_grade(self, course, course_structure): # pylint: disable=unused-argument diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index 05fde13d21..5b8ea8761a 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -21,7 +21,7 @@ class SubsectionGrade(object): """ Class for Subsection Grades. """ - def __init__(self, subsection): + def __init__(self, subsection, course): self.location = subsection.location self.display_name = block_metadata_utils.display_name_with_default_escaped(subsection) self.url_name = block_metadata_utils.url_name_for_block(subsection) @@ -30,59 +30,48 @@ class SubsectionGrade(object): self.due = getattr(subsection, 'due', None) self.graded = getattr(subsection, 'graded', False) + self.course_version = getattr(course, 'course_version', None) + self.subtree_edited_timestamp = subsection.subtree_edited_on + self.graded_total = None # aggregated grade for all graded problems self.all_total = None # aggregated grade for all problems, regardless of whether they are graded self.locations_to_weighted_scores = OrderedDict() # dict of problem locations to (Score, weight) tuples - @lazy + self._scores = None + + @property def scores(self): """ List of all problem scores in the subsection. """ - log.info(u"Persistent Grades: calculated scores property for subsection {0}".format(self.location)) - return [score for score, _ in self.locations_to_weighted_scores.itervalues()] + if self._scores is None: + self._scores = [score for score, _ in self.locations_to_weighted_scores.itervalues()] + return self._scores - def compute(self, student, course_structure, scores_client, submissions_scores): + def init_from_structure(self, student, course_structure, scores_client, submissions_scores): """ Compute the grade of this subsection for the given student and course. """ - lazy.invalidate(self, 'scores') + assert self._scores is None for descendant_key in course_structure.post_order_traversal( filter_func=possibly_scored, start_node=self.location, ): - self._compute_block_score(student, descendant_key, course_structure, scores_client, submissions_scores) + self._compute_block_score( + student, descendant_key, course_structure, scores_client, submissions_scores, persisted_values={}, + ) self.all_total, self.graded_total = graders.aggregate_scores(self.scores, self.display_name, self.location) + self._log_event(log.warning, u"init_from_structure", student) - def save(self, student, subsection, course): - """ - Persist the SubsectionGrade. - """ - visible_blocks = [ - BlockRecord(location, weight, score.possible) - for location, (score, weight) in self.locations_to_weighted_scores.iteritems() - ] - - PersistentSubsectionGrade.save_grade( - user_id=student.id, - usage_key=self.location, - course_version=getattr(course, 'course_version', None), - subtree_edited_timestamp=subsection.subtree_edited_on, - earned_all=self.all_total.earned, - possible_all=self.all_total.possible, - earned_graded=self.graded_total.earned, - possible_graded=self.graded_total.possible, - visible_blocks=visible_blocks, - ) - - def load_from_data(self, model, course_structure, scores_client, submissions_scores): + def init_from_model(self, student, model, course_structure, scores_client, submissions_scores): """ Load the subsection grade from the persisted model. """ + assert self._scores is None for block in model.visible_blocks.blocks: persisted_values = {'weight': block.weight, 'possible': block.max_score} self._compute_block_score( - User.objects.get(id=model.user_id), + student, block.locator, course_structure, scores_client, @@ -104,21 +93,32 @@ class SubsectionGrade(object): section=self.display_name, module_id=self.location, ) + self._log_event(log.warning, u"init_from_model", student) - def __unicode__(self): + @classmethod + def bulk_create_models(cls, student, subsection_grades, course_key): """ - Provides a unicode representation of the scoring - data for this subsection. Used for logging. + Saves the subsection grade in a persisted model. """ - return u"SubsectionGrade|total: {0}/{1}|graded: {2}/{3}|location: {4}|display name: {5}".format( - self.all_total.earned, - self.all_total.possible, - self.graded_total.earned, - self.graded_total.possible, - self.location, - self.display_name + return PersistentSubsectionGrade.bulk_create_grades( + [subsection_grade._persisted_model_params(student) for subsection_grade in subsection_grades], # pylint: disable=protected-access + course_key, ) + def create_model(self, student): + """ + Saves the subsection grade in a persisted model. + """ + self._log_event(log.info, u"create_model", student) + return PersistentSubsectionGrade.create_grade(**self._persisted_model_params(student)) + + def update_or_create_model(self, student): + """ + Saves or updates the subsection grade in a persisted model. + """ + self._log_event(log.info, u"update_or_create_model", student) + return PersistentSubsectionGrade.update_or_create_grade(**self._persisted_model_params(student)) + def _compute_block_score( self, student, @@ -126,30 +126,35 @@ class SubsectionGrade(object): course_structure, scores_client, submissions_scores, - persisted_values=None, + persisted_values, ): """ - Compute score for the given block. If persisted_values is provided, it will be used for possible and weight. + Compute score for the given block. If persisted_values + is provided, it is used for possible and weight. """ block = course_structure[block_key] if getattr(block, 'has_score', False): + + possible = persisted_values.get('possible', None) + weight = persisted_values.get('weight', getattr(block, 'weight', None)) + (earned, possible) = get_score( student, block, scores_client, submissions_scores, + weight, + possible, ) - # There's a chance that the value of weight is not the same value used when the problem was scored, - # since we can get the value from either block_structure or CSM/submissions. - weight = getattr(block, 'weight', None) - if persisted_values: - possible = persisted_values.get('possible', possible) - weight = persisted_values.get('weight', weight) - if earned is not None or possible is not None: - # cannot grade a problem with a denominator of 0 + # There's a chance that the value of graded is not the same + # value when the problem was scored. Since we get the value + # from the block_structure. + # + # Cannot grade a problem with a denominator of 0. + # TODO: None > 0 is not python 3 compatible. block_graded = block.graded if possible > 0 else False self.locations_to_weighted_scores[block.location] = ( @@ -163,110 +168,202 @@ class SubsectionGrade(object): weight, ) + def _persisted_model_params(self, student): + """ + Returns the parameters for creating/updating the + persisted model for this subsection grade. + """ + return dict( + user_id=student.id, + usage_key=self.location, + course_version=self.course_version, + subtree_edited_timestamp=self.subtree_edited_timestamp, + earned_all=self.all_total.earned, + possible_all=self.all_total.possible, + earned_graded=self.graded_total.earned, + possible_graded=self.graded_total.possible, + visible_blocks=self._get_visible_blocks, + ) + + @property + def _get_visible_blocks(self): + """ + Returns the list of visible blocks. + """ + return [ + BlockRecord(location, weight, score.possible) + for location, (score, weight) in + self.locations_to_weighted_scores.iteritems() + ] + + def _log_event(self, log_func, log_statement, student): + """ + Logs the given statement, for this instance. + """ + log_func( + u"Persistent Grades: SG.{}, subsection: {}, course: {}, " + u"version: {}, edit: {}, user: {}," + u"total: {}/{}, graded: {}/{}".format( + log_statement, + self.location, + self.location.course_key, + self.course_version, + self.subtree_edited_timestamp, + student.id, + self.all_total.earned, + self.all_total.possible, + self.graded_total.earned, + self.graded_total.possible, + ) + ) + class SubsectionGradeFactory(object): """ Factory for Subsection Grades. """ - def __init__(self, student): + def __init__(self, student, course, course_structure): self.student = student + self.course = course + self.course_structure = course_structure - self._scores_client = None - self._submissions_scores = None + self._cached_subsection_grades = None + self._unsaved_subsection_grades = [] - def create(self, subsection, course_structure, course): + def create(self, subsection, block_structure=None, read_only=False): """ Returns the SubsectionGrade object for the student and subsection. + + If block_structure is provided, uses it for finding and computing + the grade instead of the course_structure passed in earlier. + + If read_only is True, doesn't save any updates to the grades. """ - self._prefetch_scores(course_structure, course) - return ( - self._get_saved_grade(subsection, course_structure, course) or - self._compute_and_save_grade(subsection, course_structure, course) + self._log_event( + log.warning, u"create, read_only: {0}, subsection: {1}".format(read_only, subsection.location) ) - def update(self, usage_key, course_structure, course): - """ - Updates the SubsectionGrade object for the student and subsection - identified by the given usage key. - """ - # save ourselves the extra queries if the course does not use subsection grades - if not PersistentGradesEnabledFlag.feature_enabled(course.id): - return - - self._prefetch_scores(course_structure, course) - subsection = course_structure[usage_key] - return self._compute_and_save_grade(subsection, course_structure, course) - - def _compute_and_save_grade(self, subsection, course_structure, course): - """ - Freshly computes and updates the grade for the student and subsection. - """ - subsection_grade = SubsectionGrade(subsection) - subsection_grade.compute(self.student, course_structure, self._scores_client, self._submissions_scores) - self._save_grade(subsection_grade, subsection, course) + block_structure = self._get_block_structure(block_structure) + subsection_grade = self._get_saved_grade(subsection, block_structure) + if not subsection_grade: + subsection_grade = SubsectionGrade(subsection, self.course) + subsection_grade.init_from_structure( + self.student, block_structure, self._scores_client, self._submissions_scores + ) + if PersistentGradesEnabledFlag.feature_enabled(self.course.id): + if read_only: + self._unsaved_subsection_grades.append(subsection_grade) + else: + grade_model = subsection_grade.create_model(self.student) + self._update_saved_subsection_grade(subsection.location, grade_model) return subsection_grade - def _get_saved_grade(self, subsection, course_structure, course): # pylint: disable=unused-argument + def bulk_create_unsaved(self): + """ + Bulk creates all the unsaved subsection_grades to this point. + """ + self._log_event(log.warning, u"bulk_create_unsaved") + + SubsectionGrade.bulk_create_models(self.student, self._unsaved_subsection_grades, self.course.id) + self._unsaved_subsection_grades = [] + + def update(self, subsection, block_structure=None): + """ + Updates the SubsectionGrade object for the student and subsection. + """ + # Save ourselves the extra queries if the course does not persist + # subsection grades. + if not PersistentGradesEnabledFlag.feature_enabled(self.course.id): + return + + self._log_event(log.warning, u"update, subsection: {}".format(subsection.location)) + + block_structure = self._get_block_structure(block_structure) + subsection_grade = SubsectionGrade(subsection, self.course) + subsection_grade.init_from_structure( + self.student, block_structure, self._scores_client, self._submissions_scores + ) + + grade_model = subsection_grade.update_or_create_model(self.student) + self._update_saved_subsection_grade(subsection.location, grade_model) + return subsection_grade + + @lazy + def _scores_client(self): + """ + Lazily queries and returns all the scores stored in the user + state (in CSM) for the course, while caching the result. + """ + scorable_locations = [block_key for block_key in self.course_structure if possibly_scored(block_key)] + return ScoresClient.create_for_locations(self.course.id, self.student.id, scorable_locations) + + @lazy + def _submissions_scores(self): + """ + Lazily queries and returns the scores stored by the + Submissions API for the course, while caching the result. + """ + anonymous_user_id = anonymous_id_for_user(self.student, self.course.id) + return submissions_api.get_scores(unicode(self.course.id), anonymous_user_id) + + def _get_saved_grade(self, subsection, block_structure): # pylint: disable=unused-argument """ Returns the saved grade for the student and subsection. """ - if PersistentGradesEnabledFlag.feature_enabled(course.id): - try: - model = PersistentSubsectionGrade.read_grade( - user_id=self.student.id, - usage_key=subsection.location, - ) - subsection_grade = SubsectionGrade(subsection) - subsection_grade.load_from_data(model, course_structure, self._scores_client, self._submissions_scores) - log.warning( - u"Persistent Grades: Loaded grade for course id: {0}, version: {1}, subtree edited on: {2}," - u" grade: {3}, user: {4}".format( - course.id, - getattr(course, 'course_version', None), - course.subtree_edited_on, - subsection_grade, - self.student.id - ) - ) - return subsection_grade - except PersistentSubsectionGrade.DoesNotExist: - log.warning( - u"Persistent Grades: Could not find grade for course id: {0}, version: {1}, subtree edited" - u" on: {2}, subsection: {3}, user: {4}".format( - course.id, - getattr(course, 'course_version', None), - course.subtree_edited_on, - subsection.location, - self.student.id - ) - ) - return None + if not PersistentGradesEnabledFlag.feature_enabled(self.course.id): + return - def _save_grade(self, subsection_grade, subsection, course): # pylint: disable=unused-argument - """ - Updates the saved grade for the student and subsection. - """ - if PersistentGradesEnabledFlag.feature_enabled(course.id): - subsection_grade.save(self.student, subsection, course) - log.warning(u"Persistent Grades: Saved grade for course id: {0}, version: {1}, subtree_edited_on: {2}, grade: " - u"{3}, user: {4}" - .format( - course.id, - getattr(course, 'course_version', None), - course.subtree_edited_on, - subsection_grade, - self.student.id - )) + saved_subsection_grade = self._get_saved_subsection_grade(subsection.location) + if saved_subsection_grade: + subsection_grade = SubsectionGrade(subsection, self.course) + subsection_grade.init_from_model( + self.student, saved_subsection_grade, block_structure, self._scores_client, self._submissions_scores + ) + return subsection_grade - def _prefetch_scores(self, course_structure, course): + def _get_saved_subsection_grade(self, subsection_usage_key): """ - Returns the prefetched scores for the given student and course. + Returns the saved value of the subsection grade for + the given subsection usage key, caching the value. + Returns None if not found. """ - if not self._scores_client: - scorable_locations = [block_key for block_key in course_structure if possibly_scored(block_key)] - self._scores_client = ScoresClient.create_for_locations( - course.id, self.student.id, scorable_locations - ) - self._submissions_scores = submissions_api.get_scores( - unicode(course.id), anonymous_id_for_user(self.student, course.id) - ) + if self._cached_subsection_grades is None: + self._cached_subsection_grades = { + record.full_usage_key: record + for record in PersistentSubsectionGrade.bulk_read_grades(self.student.id, self.course.id) + } + return self._cached_subsection_grades.get(subsection_usage_key) + + def _update_saved_subsection_grade(self, subsection_usage_key, subsection_model): + """ + Updates (or adds) the subsection grade for the given + subsection usage key in the local cache, iff the cache + is populated. + """ + if self._cached_subsection_grades is not None: + self._cached_subsection_grades[subsection_usage_key] = subsection_model + + def _get_block_structure(self, block_structure): + """ + If block_structure is None, returns self.course_structure. + Otherwise, returns block_structure after verifying that the + given block_structure is a sub-structure of self.course_structure. + """ + if block_structure: + if block_structure.root_block_usage_key not in self.course_structure: + raise ValueError + return block_structure + else: + return self.course_structure + + def _log_event(self, log_func, log_statement): + """ + Logs the given statement, for this instance. + """ + log_func(u"Persistent Grades: SGF.{}, course: {}, version: {}, edit: {}, user: {}".format( + log_statement, + self.course.id, + getattr(self.course, 'course_version', None), + self.course.subtree_edited_on, + self.student.id, + )) diff --git a/lms/djangoapps/grades/scores.py b/lms/djangoapps/grades/scores.py index 98f79b50c9..e653ae2521 100644 --- a/lms/djangoapps/grades/scores.py +++ b/lms/djangoapps/grades/scores.py @@ -1,11 +1,16 @@ """ Functionality for problem scores. """ +from logging import getLogger + from openedx.core.lib.cache_utils import memoized from xblock.core import XBlock from .transformer import GradesTransformer +log = getLogger(__name__) + + @memoized def block_types_possibly_scored(): """ @@ -30,15 +35,17 @@ def possibly_scored(usage_key): return usage_key.block_type in block_types_possibly_scored() -def weighted_score(raw_earned, raw_possible, weight): - """Return a tuple that represents the weighted (correct, total) score.""" - # If there is no weighting, or weighting can't be applied, return input. +def weighted_score(raw_earned, raw_possible, weight=None): + """ + Return a tuple that represents the weighted (earned, possible) score. + If weight is None or raw_possible is 0, returns the original values. + """ if weight is None or raw_possible == 0: return (raw_earned, raw_possible) return float(raw_earned) * weight / raw_possible, float(weight) -def get_score(user, block, scores_client, submissions_scores_cache): +def get_score(user, block, scores_client, submissions_scores_cache, weight, possible=None): """ Return the score for a user on a problem, as a tuple (earned, possible). e.g. (5,7) if you got 5 out of 7 points. @@ -49,8 +56,13 @@ def get_score(user, block, scores_client, submissions_scores_cache): user: a Student object block: a BlockStructure's BlockData object scores_client: an initialized ScoresClient - submissions_scores_cache: A dict of location names to (earned, possible) point tuples. - If an entry is found in this cache, it takes precedence. + submissions_scores_cache: A dict of location names to (earned, possible) + point tuples. If an entry is found in this cache, it takes precedence. + weight: The weight of the problem to use in the calculation. A value of + None signifies that the weight should not be applied. + possible (optional): The possible maximum score of the problem to use in the + calculation. If None, uses the value found either in scores_client or + from the block. """ submissions_scores_cache = submissions_scores_cache or {} @@ -74,16 +86,20 @@ def get_score(user, block, scores_client, submissions_scores_cache): if score and score.total is not None: # We have a valid score, just use it. earned = score.correct if score.correct is not None else 0.0 - possible = score.total + if possible is None: + possible = score.total + elif possible != score.total: + log.error(u"Persisted Grades: possible value {} != score.total value {}".format(possible, score.total)) else: # This means we don't have a valid score entry and we don't have a # cached_max_score on hand. We know they've earned 0.0 points on this. earned = 0.0 - possible = block.transformer_data[GradesTransformer].max_score + if possible is None: + possible = block.transformer_data[GradesTransformer].max_score # Problem may be an error module (if something in the problem builder failed) # In which case possible might be None if possible is None: return (None, None) - return weighted_score(earned, possible, block.weight) + return weighted_score(earned, possible, weight) diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index 905d62810b..b13fefd7c9 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/lms/djangoapps/grades/signals/handlers.py @@ -108,10 +108,13 @@ def recalculate_subsection_grade_handler(sender, **kwargs): # pylint: disable=u 'subsections', set() ) + subsection_grade_factory = SubsectionGradeFactory(student, course, collected_block_structure) for subsection_usage_key in subsections_to_update: transformed_subsection_structure = get_course_blocks( student, subsection_usage_key, collected_block_structure=collected_block_structure, ) - SubsectionGradeFactory(student).update(subsection_usage_key, transformed_subsection_structure, course) + subsection_grade_factory.update( + transformed_subsection_structure[subsection_usage_key], transformed_subsection_structure + ) diff --git a/lms/djangoapps/grades/tests/test_grades.py b/lms/djangoapps/grades/tests/test_grades.py index 1e70acfc9a..ce6bf22899 100644 --- a/lms/djangoapps/grades/tests/test_grades.py +++ b/lms/djangoapps/grades/tests/test_grades.py @@ -395,7 +395,7 @@ class TestGetModuleScore(LoginEnrollmentTestCase, SharedModuleStoreTestCase): # then stored in the request). with self.assertNumQueries(1): score = get_module_score(self.request.user, self.course, self.seq1) - new_score = SubsectionGradeFactory(self.request.user).create(self.seq1, self.course_structure, self.course) + new_score = SubsectionGradeFactory(self.request.user, self.course, self.course_structure).create(self.seq1) self.assertEqual(score, 0) self.assertEqual(new_score.all_total.earned, 0) @@ -404,7 +404,7 @@ class TestGetModuleScore(LoginEnrollmentTestCase, SharedModuleStoreTestCase): with self.assertNumQueries(1): score = get_module_score(self.request.user, self.course, self.seq1) - new_score = SubsectionGradeFactory(self.request.user).create(self.seq1, self.course_structure, self.course) + new_score = SubsectionGradeFactory(self.request.user, self.course, self.course_structure).create(self.seq1) self.assertEqual(score, 1.0) self.assertEqual(new_score.all_total.earned, 2.0) # These differ because get_module_score normalizes the subsection score @@ -416,7 +416,7 @@ class TestGetModuleScore(LoginEnrollmentTestCase, SharedModuleStoreTestCase): with self.assertNumQueries(1): score = get_module_score(self.request.user, self.course, self.seq1) - new_score = SubsectionGradeFactory(self.request.user).create(self.seq1, self.course_structure, self.course) + new_score = SubsectionGradeFactory(self.request.user, self.course, self.course_structure).create(self.seq1) self.assertEqual(score, .5) self.assertEqual(new_score.all_total.earned, 1.0) diff --git a/lms/djangoapps/grades/tests/test_models.py b/lms/djangoapps/grades/tests/test_models.py index eb453a8bbb..b4dc2dc77e 100644 --- a/lms/djangoapps/grades/tests/test_models.py +++ b/lms/djangoapps/grades/tests/test_models.py @@ -6,7 +6,6 @@ from collections import OrderedDict import ddt from hashlib import sha1 import json -from mock import patch from django.db.utils import IntegrityError from django.test import TestCase @@ -24,17 +23,25 @@ class BlockRecordListTestCase(TestCase): """ Verify the behavior of BlockRecordList, particularly around edge cases """ - empty_json = '{"blocks":[],"course_key":null}' + def setUp(self): + super(BlockRecordListTestCase, self).setUp() + self.course_key = CourseLocator( + org='some_org', + course='some_course', + run='some_run' + ) def test_empty_block_record_set(self): - brs = BlockRecordList(()) + empty_json = '{0}"blocks":[],"course_key":"{1}"{2}'.format('{', unicode(self.course_key), '}') + + brs = BlockRecordList((), self.course_key) self.assertFalse(brs) self.assertEqual( - brs.to_json(), - self.empty_json + brs.json_value, + empty_json ) self.assertEqual( - BlockRecordList.from_json(self.empty_json), + BlockRecordList.from_json(empty_json), brs ) @@ -112,7 +119,7 @@ class VisibleBlocksTest(GradesModelTestCase): """ Creates and returns a BlockRecordList for the given blocks. """ - return VisibleBlocks.objects.create_from_blockrecords(BlockRecordList.from_list(blocks)) + return VisibleBlocks.objects.create_from_blockrecords(BlockRecordList.from_list(blocks, self.course_key)) def test_creation(self): """ @@ -159,7 +166,7 @@ class VisibleBlocksTest(GradesModelTestCase): 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) + expected_blocks = BlockRecordList.from_list([self.record_a, self.record_b], self.course_key) visible_blocks = self._create_block_record_list(expected_blocks) self.assertEqual(expected_blocks, visible_blocks.blocks) with self.assertRaises(AttributeError): @@ -178,6 +185,7 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): block_type='subsection', block_id='subsection_12345', ) + self.block_records = BlockRecordList([self.record_a, self.record_b], self.course_key) self.params = { "user_id": 12345, "usage_key": self.usage_key, @@ -187,21 +195,23 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): "possible_all": 12.0, "earned_graded": 6.0, "possible_graded": 8.0, - "visible_blocks": [self.record_a, self.record_b], + "visible_blocks": self.block_records, } def test_create(self): """ Tests model creation, and confirms error when trying to recreate model. """ - created_grade = PersistentSubsectionGrade.objects.create(**self.params) - read_grade = PersistentSubsectionGrade.read_grade( - user_id=self.params["user_id"], - usage_key=self.params["usage_key"], - ) - self.assertEqual(created_grade, read_grade) + created_grade = PersistentSubsectionGrade.create_grade(**self.params) + with self.assertNumQueries(1): + read_grade = PersistentSubsectionGrade.read_grade( + user_id=self.params["user_id"], + usage_key=self.params["usage_key"], + ) + self.assertEqual(created_grade, read_grade) + self.assertEquals(read_grade.visible_blocks.blocks, self.block_records) with self.assertRaises(IntegrityError): - created_grade = PersistentSubsectionGrade.objects.create(**self.params) + PersistentSubsectionGrade.create_grade(**self.params) def test_create_bad_params(self): """ @@ -209,56 +219,19 @@ class PersistentSubsectionGradeTest(GradesModelTestCase): """ del self.params["earned_graded"] with self.assertRaises(IntegrityError): - PersistentSubsectionGrade.objects.create(**self.params) + PersistentSubsectionGrade.create_grade(**self.params) def test_course_version_is_optional(self): del self.params["course_version"] - PersistentSubsectionGrade.objects.create(**self.params) - - def test_update_grade(self): - """ - Tests model update, and confirms error when updating a nonexistent model. - """ - with self.assertRaises(PersistentSubsectionGrade.DoesNotExist): - PersistentSubsectionGrade.update_grade(**self.params) - PersistentSubsectionGrade.objects.create(**self.params) - self.params['earned_all'] = 12.0 - self.params['earned_graded'] = 8.0 - - with patch('lms.djangoapps.grades.models.log') as log_mock: - PersistentSubsectionGrade.update_grade(**self.params) - read_grade = PersistentSubsectionGrade.read_grade( - user_id=self.params["user_id"], - usage_key=self.params["usage_key"], - ) - log_mock.info.assert_called_with( - u"Persistent Grades: Grade model updated: {0}".format(read_grade) - ) - - self.assertEqual(read_grade.earned_all, 12.0) - self.assertEqual(read_grade.earned_graded, 8.0) + PersistentSubsectionGrade.create_grade(**self.params) @ddt.data(True, False) - def test_save(self, already_created): - if already_created: - PersistentSubsectionGrade.objects.create(**self.params) - module_prefix = "lms.djangoapps.grades.models.PersistentSubsectionGrade." - with patch( - module_prefix + "objects.get_or_create", - wraps=PersistentSubsectionGrade.objects.get_or_create - ) as mock_get_or_create: - with patch(module_prefix + "update") as mock_update: - PersistentSubsectionGrade.save_grade(**self.params) - self.assertTrue(mock_get_or_create.called) - self.assertEqual(mock_update.called, already_created) + def test_update_or_create_grade(self, already_created): + created_grade = PersistentSubsectionGrade.create_grade(**self.params) if already_created else None - def test_logging_for_save(self): - with patch('lms.djangoapps.grades.models.log') as log_mock: - PersistentSubsectionGrade.save_grade(**self.params) - read_grade = PersistentSubsectionGrade.read_grade( - user_id=self.params["user_id"], - usage_key=self.params["usage_key"], - ) - log_mock.info.assert_called_with( - u"Persistent Grades: Grade model saved: {0}".format(read_grade) - ) + self.params["earned_all"] = 7 + updated_grade = PersistentSubsectionGrade.update_or_create_grade(**self.params) + self.assertEquals(updated_grade.earned_all, 7) + if already_created: + self.assertEquals(created_grade.id, updated_grade.id) + self.assertEquals(created_grade.earned_all, 6) diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index 746f14959a..777f4f37dc 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -61,8 +61,8 @@ class GradeTestBase(SharedModuleStoreTestCase): super(GradeTestBase, self).setUp() self.request = get_request_for_user(UserFactory()) self.client.login(username=self.request.user.username, password="test") - self.subsection_grade_factory = SubsectionGradeFactory(self.request.user) self.course_structure = get_course_blocks(self.request.user, self.course.location) + self.subsection_grade_factory = SubsectionGradeFactory(self.request.user, self.course, self.course_structure) CourseEnrollment.enroll(self.request.user, self.course.id) @@ -110,33 +110,25 @@ class SubsectionGradeFactoryTest(GradeTestBase): Tests to ensure that a persistent subsection grade is created, saved, then fetched on re-request. """ with patch( - 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._save_grade', - wraps=self.subsection_grade_factory._save_grade # pylint: disable=protected-access - ) as mock_save_grades: + 'lms.djangoapps.grades.new.subsection_grade.PersistentSubsectionGrade.create_grade', + wraps=PersistentSubsectionGrade.create_grade + ) as mock_create_grade: with patch( 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_saved_grade', wraps=self.subsection_grade_factory._get_saved_grade # pylint: disable=protected-access ) as mock_get_saved_grade: - with self.assertNumQueries(19): - grade_a = self.subsection_grade_factory.create( - self.sequence, - self.course_structure, - self.course - ) + with self.assertNumQueries(12): + grade_a = self.subsection_grade_factory.create(self.sequence) self.assertTrue(mock_get_saved_grade.called) - self.assertTrue(mock_save_grades.called) + self.assertTrue(mock_create_grade.called) mock_get_saved_grade.reset_mock() - mock_save_grades.reset_mock() + mock_create_grade.reset_mock() - with self.assertNumQueries(3): - grade_b = self.subsection_grade_factory.create( - self.sequence, - self.course_structure, - self.course - ) + with self.assertNumQueries(0): + grade_b = self.subsection_grade_factory.create(self.sequence) self.assertTrue(mock_get_saved_grade.called) - self.assertFalse(mock_save_grades.called) + self.assertFalse(mock_create_grade.called) self.assertEqual(grade_a.url_name, grade_b.url_name) self.assertEqual(grade_a.all_total, grade_b.all_total) @@ -153,7 +145,7 @@ class SubsectionGradeFactoryTest(GradeTestBase): # Grades are only saved if the feature flag and the advanced setting are # both set to True. with patch( - 'lms.djangoapps.grades.models.PersistentSubsectionGrade.read_grade' + 'lms.djangoapps.grades.models.PersistentSubsectionGrade.bulk_read_grades' ) as mock_read_saved_grade: with persistent_grades_feature_flags( global_flag=feature_flag, @@ -161,7 +153,7 @@ class SubsectionGradeFactoryTest(GradeTestBase): course_id=self.course.id, enabled_for_course=course_setting ): - self.subsection_grade_factory.create(self.sequence, self.course_structure, self.course) + self.subsection_grade_factory.create(self.sequence) self.assertEqual(mock_read_saved_grade.called, feature_flag and course_setting) @@ -174,10 +166,8 @@ class SubsectionGradeTest(GradeTestBase): """ Assuming the underlying score reporting methods work, test that the score is calculated properly. """ - grade = self.subsection_grade_factory.create(self.sequence, self.course_structure, self.course) with mock_get_score(1, 2): - # The final 2 parameters are only passed through to our mocked-out get_score method - grade.compute(self.request.user, self.course_structure, None, None) + grade = self.subsection_grade_factory.create(self.sequence) self.assertEqual(grade.all_total.earned, 1) self.assertEqual(grade.all_total.possible, 2) @@ -186,9 +176,8 @@ class SubsectionGradeTest(GradeTestBase): Test that grades are persisted to the database properly, and that loading saved grades returns the same data. """ # Create a grade that *isn't* saved to the database - self.subsection_grade_factory._prefetch_scores(self.course_structure, self.course) # pylint: disable=protected-access - input_grade = SubsectionGrade(self.sequence) - input_grade.compute( + input_grade = SubsectionGrade(self.sequence, self.course) + input_grade.init_from_structure( self.request.user, self.course_structure, self.subsection_grade_factory._scores_client, # pylint: disable=protected-access @@ -197,16 +186,17 @@ class SubsectionGradeTest(GradeTestBase): self.assertEqual(PersistentSubsectionGrade.objects.count(), 0) # save to db, and verify object is in database - input_grade.save(self.request.user, self.sequence, self.course) + input_grade.create_model(self.request.user) self.assertEqual(PersistentSubsectionGrade.objects.count(), 1) # load from db, and ensure output matches input - loaded_grade = SubsectionGrade(self.sequence) + loaded_grade = SubsectionGrade(self.sequence, self.course) saved_model = PersistentSubsectionGrade.read_grade( user_id=self.request.user.id, usage_key=self.sequence.location, ) - loaded_grade.load_from_data( + loaded_grade.init_from_model( + self.request.user, saved_model, self.course_structure, self.subsection_grade_factory._scores_client, # pylint: disable=protected-access