diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index d6cf8a5420..c66fa1e4e8 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -1848,7 +1848,8 @@ class TestXmoduleRuntimeEvent(TestSubmittingProblems): 'course_id': unicode(self.course.id), 'usage_id': unicode(self.problem.location), 'only_if_higher': None, - 'modified': datetime.now().replace(tzinfo=pytz.UTC) + 'modified': datetime.now().replace(tzinfo=pytz.UTC), + 'score_db_table': 'csm', } send_mock.assert_called_with(**expected_signal_kwargs) diff --git a/lms/djangoapps/grades/constants.py b/lms/djangoapps/grades/constants.py new file mode 100644 index 0000000000..2ac18f3ddb --- /dev/null +++ b/lms/djangoapps/grades/constants.py @@ -0,0 +1,11 @@ +""" +Constants and Enums used by Grading. +""" + + +class ScoreDatabaseTableEnum(object): + """ + The various database tables that store scores. + """ + courseware_student_module = 'csm' + submissions = 'submissions' diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index 74bd721acc..4c66bdb848 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/lms/djangoapps/grades/signals/handlers.py @@ -24,9 +24,10 @@ from .signals import ( SUBSECTION_SCORE_CHANGED, SCORE_PUBLISHED, ) +from ..constants import ScoreDatabaseTableEnum from ..new.course_grade import CourseGradeFactory from ..scores import weighted_score -from ..tasks import recalculate_subsection_grade_v2 +from ..tasks import recalculate_subsection_grade_v3 log = getLogger(__name__) @@ -62,9 +63,11 @@ def submissions_score_set_handler(sender, **kwargs): # pylint: disable=unused-a weighted_earned=points_earned, weighted_possible=points_possible, user_id=user.id, + anonymous_user_id=kwargs['anonymous_user_id'], course_id=course_id, usage_id=usage_id, modified=kwargs['created_at'], + score_db_table=ScoreDatabaseTableEnum.submissions, ) @@ -93,9 +96,11 @@ def submissions_score_reset_handler(sender, **kwargs): # pylint: disable=unused weighted_earned=0, weighted_possible=0, user_id=user.id, + anonymous_user_id=kwargs['anonymous_user_id'], course_id=course_id, usage_id=usage_id, modified=kwargs['created_at'], + score_db_table=ScoreDatabaseTableEnum.submissions, ) @@ -133,6 +138,7 @@ def score_published_handler(sender, block, user, raw_earned, raw_possible, only_ usage_id=unicode(block.location), only_if_higher=only_if_higher, modified=score_modified_time, + score_db_table=ScoreDatabaseTableEnum.courseware_student_module, ) return update_score @@ -162,6 +168,7 @@ def problem_raw_score_changed_handler(sender, **kwargs): # pylint: disable=unus only_if_higher=kwargs['only_if_higher'], score_deleted=kwargs.get('score_deleted', False), modified=kwargs['modified'], + score_db_table=kwargs['score_db_table'], ) @@ -172,9 +179,10 @@ def enqueue_subsection_update(sender, **kwargs): # pylint: disable=unused-argum enqueueing a subsection update operation to occur asynchronously. """ _emit_problem_submitted_event(kwargs) - result = recalculate_subsection_grade_v2.apply_async( + result = recalculate_subsection_grade_v3.apply_async( kwargs=dict( user_id=kwargs['user_id'], + anonymous_user_id=kwargs.get('anonymous_user_id'), course_id=kwargs['course_id'], usage_id=kwargs['usage_id'], only_if_higher=kwargs.get('only_if_higher'), @@ -182,6 +190,7 @@ def enqueue_subsection_update(sender, **kwargs): # pylint: disable=unused-argum score_deleted=kwargs.get('score_deleted', False), event_transaction_id=unicode(get_event_transaction_id()), event_transaction_type=unicode(get_event_transaction_type()), + score_db_table=kwargs['score_db_table'], ) ) log.info( diff --git a/lms/djangoapps/grades/signals/signals.py b/lms/djangoapps/grades/signals/signals.py index a2c3c21a00..4dc274f6ae 100644 --- a/lms/djangoapps/grades/signals/signals.py +++ b/lms/djangoapps/grades/signals/signals.py @@ -22,6 +22,7 @@ PROBLEM_RAW_SCORE_CHANGED = Signal( # made only if the new score is higher than previous. 'modified', # A datetime indicating when the database representation of # this the problem score was saved. + 'score_db_table', # The database table that houses the score that changed. ] ) @@ -35,6 +36,7 @@ PROBLEM_RAW_SCORE_CHANGED = Signal( PROBLEM_WEIGHTED_SCORE_CHANGED = Signal( providing_args=[ 'user_id', # Integer User ID + 'anonymous_user_id', # Anonymous User ID 'course_id', # Unicode string representing the course 'usage_id', # Unicode string indicating the courseware instance 'weighted_earned', # Score obtained by the user @@ -43,6 +45,7 @@ PROBLEM_WEIGHTED_SCORE_CHANGED = Signal( # made only if the new score is higher than previous. 'modified', # A datetime indicating when the database representation of # this the problem score was saved. + 'score_db_table', # The database table that houses the score that changed. ] ) @@ -59,6 +62,7 @@ SCORE_PUBLISHED = Signal( 'raw_possible', # Maximum score available for the exercise 'only_if_higher', # Boolean indicating whether updates should be # made only if the new score is higher than previous. + 'score_db_table', # The database table that houses the score that changed. ] ) diff --git a/lms/djangoapps/grades/tasks.py b/lms/djangoapps/grades/tasks.py index 6a5a235839..45f5fa385d 100644 --- a/lms/djangoapps/grades/tasks.py +++ b/lms/djangoapps/grades/tasks.py @@ -26,6 +26,7 @@ from util.date_utils import from_timestamp from xmodule.modulestore.django import modulestore from .config.models import PersistentGradesEnabledFlag +from .constants import ScoreDatabaseTableEnum from .new.subsection_grade import SubsectionGradeFactory from .signals.signals import SUBSECTION_SCORE_CHANGED from .transformer import GradesTransformer @@ -35,33 +36,31 @@ log = getLogger(__name__) KNOWN_RETRY_ERRORS = (DatabaseError, ValidationError) # Errors we expect occasionally, should be resolved on retry -@task(default_retry_delay=30, routing_key=settings.RECALCULATE_GRADES_ROUTING_KEY) -def recalculate_subsection_grade( - # pylint: disable=unused-argument - user_id, course_id, usage_id, only_if_higher, weighted_earned, weighted_possible, **kwargs -): - """ - Shim to allow us to modify this task's signature without blowing up production on deployment. - """ - recalculate_subsection_grade_v2.apply( - kwargs=dict( - user_id=user_id, - course_id=course_id, - usage_id=usage_id, - only_if_higher=only_if_higher, - expected_modified_time=kwargs.get('expected_modified_time', 0), # Use the unix epoch as a default - score_deleted=kwargs['score_deleted'], - ) - ) - - +# TODO (TNL-6373) DELETE ME once v3 is successfully deployed to Prod. @task(base=PersistOnFailureTask, default_retry_delay=30, routing_key=settings.RECALCULATE_GRADES_ROUTING_KEY) def recalculate_subsection_grade_v2(**kwargs): + """ + Shim to support tasks enqueued by older workers during initial deployment. + """ + _recalculate_subsection_grade(recalculate_subsection_grade_v2, **kwargs) + + +@task(base=PersistOnFailureTask, default_retry_delay=30, routing_key=settings.RECALCULATE_GRADES_ROUTING_KEY) +def recalculate_subsection_grade_v3(**kwargs): + """ + Latest version of the recalculate_subsection_grade task. See docstring + for _recalculate_subsection_grade for further description. + """ + _recalculate_subsection_grade(recalculate_subsection_grade_v3, **kwargs) + + +def _recalculate_subsection_grade(task_func, **kwargs): """ Updates a saved subsection grade. - Arguments: + Keyword Arguments: user_id (int): id of applicable User object + anonymous_user_id (int, OPTIONAL): Anonymous ID of the User course_id (string): identifying the course usage_id (string): identifying the course block only_if_higher (boolean): indicating whether grades should @@ -71,36 +70,44 @@ def recalculate_subsection_grade_v2(**kwargs): was queued so that we can verify the underlying data update. score_deleted (boolean): indicating whether the grade change is a result of the problem's score being deleted. - event_transaction_id(string): uuid identifying the current + event_transaction_id (string): uuid identifying the current event transaction. - event_transaction_type(string): human-readable type of the + event_transaction_type (string): human-readable type of the event at the root of the current event transaction. + score_db_table (ScoreDatabaseTableEnum): database table that houses + the changed score. Used in conjunction with expected_modified_time. """ try: course_key = CourseLocator.from_string(kwargs['course_id']) if not PersistentGradesEnabledFlag.feature_enabled(course_key): return - score_deleted = kwargs['score_deleted'] scored_block_usage_key = UsageKey.from_string(kwargs['usage_id']).replace(course_key=course_key) - expected_modified_time = from_timestamp(kwargs['expected_modified_time']) # The request cache is not maintained on celery workers, # where this code runs. So we take the values from the # main request cache and store them in the local request # cache. This correlates model-level grading events with # higher-level ones. - set_event_transaction_id(kwargs.pop('event_transaction_id', None)) - set_event_transaction_type(kwargs.pop('event_transaction_type', None)) + set_event_transaction_id(kwargs.get('event_transaction_id')) + set_event_transaction_type(kwargs.get('event_transaction_type')) # Verify the database has been updated with the scores when the task was # created. This race condition occurs if the transaction in the task # creator's process hasn't committed before the task initiates in the worker # process. - if not _has_database_updated_with_new_score( - kwargs['user_id'], scored_block_usage_key, expected_modified_time, score_deleted, - ): - raise _retry_recalculate_subsection_grade(**kwargs) + if task_func == recalculate_subsection_grade_v2: + has_database_updated = _has_db_updated_with_new_score_bwc_v2( + kwargs['user_id'], + scored_block_usage_key, + from_timestamp(kwargs['expected_modified_time']), + kwargs['score_deleted'], + ) + else: + has_database_updated = _has_db_updated_with_new_score(scored_block_usage_key, **kwargs) + + if not has_database_updated: + raise _retry_recalculate_subsection_grade(task_func, **kwargs) _update_subsection_grades( course_key, @@ -115,13 +122,45 @@ def recalculate_subsection_grade_v2(**kwargs): repr(exc), kwargs )) - raise _retry_recalculate_subsection_grade(exc=exc, **kwargs) + raise _retry_recalculate_subsection_grade(task_func, exc=exc, **kwargs) -def _has_database_updated_with_new_score( +def _has_db_updated_with_new_score(scored_block_usage_key, **kwargs): + """ + Returns whether the database has been updated with the + expected new score values for the given problem and user. + """ + if kwargs['score_db_table'] == ScoreDatabaseTableEnum.courseware_student_module: + score = get_score(kwargs['user_id'], scored_block_usage_key) + found_modified_time = score.modified if score is not None else None + + else: + assert kwargs['score_db_table'] == ScoreDatabaseTableEnum.submissions + score = sub_api.get_score( + { + "student_id": kwargs['anonymous_user_id'], + "course_id": unicode(scored_block_usage_key.course_key), + "item_id": unicode(scored_block_usage_key), + "item_type": scored_block_usage_key.block_type, + } + ) + found_modified_time = score['created_at'] if score is not None else None + + if score is None: + # score should be None only if it was deleted. + # Otherwise, it hasn't yet been saved. + return kwargs['score_deleted'] + + return found_modified_time >= from_timestamp(kwargs['expected_modified_time']) + + +# TODO (TNL-6373) DELETE ME once v3 is successfully deployed to Prod. +def _has_db_updated_with_new_score_bwc_v2( user_id, scored_block_usage_key, expected_modified_time, score_deleted, ): """ + DEPRECATED version for backward compatibility with v2 tasks. + Returns whether the database has been updated with the expected new score values for the given problem and user. """ @@ -186,7 +225,7 @@ def _update_subsection_grades( only_if_higher, ) SUBSECTION_SCORE_CHANGED.send( - sender=recalculate_subsection_grade, + sender=None, course=course, course_structure=course_structure, user=student, @@ -194,29 +233,9 @@ def _update_subsection_grades( ) -def _retry_recalculate_subsection_grade( - user_id, - course_id, - usage_id, - only_if_higher, - expected_modified_time, - score_deleted, - exc=None, -): +def _retry_recalculate_subsection_grade(task_func, exc=None, **kwargs): """ Calls retry for the recalculate_subsection_grade task with the given inputs. """ - recalculate_subsection_grade_v2.retry( - kwargs=dict( - user_id=user_id, - course_id=course_id, - usage_id=usage_id, - only_if_higher=only_if_higher, - expected_modified_time=expected_modified_time, - score_deleted=score_deleted, - event_transaction_id=unicode(get_event_transaction_id()), - event_transaction_type=unicode(get_event_transaction_type()), - ), - exc=exc, - ) + task_func.retry(kwargs=kwargs, exc=exc) diff --git a/lms/djangoapps/grades/tests/test_signals.py b/lms/djangoapps/grades/tests/test_signals.py index 26f61e1180..0c2b65000b 100644 --- a/lms/djangoapps/grades/tests/test_signals.py +++ b/lms/djangoapps/grades/tests/test_signals.py @@ -11,6 +11,7 @@ from mock import patch, MagicMock import pytz from util.date_utils import to_timestamp +from ..constants import ScoreDatabaseTableEnum from ..signals.handlers import ( enqueue_subsection_update, submissions_score_set_handler, @@ -32,7 +33,6 @@ SUBMISSION_SET_KWARGS = { 'created_at': FROZEN_NOW_TIMESTAMP, } - SUBMISSION_RESET_KWARGS = { 'anonymous_user_id': 'anonymous_id', 'course_id': 'CourseID', @@ -50,6 +50,20 @@ PROBLEM_RAW_SCORE_CHANGED_KWARGS = { 'only_if_higher': False, 'score_deleted': True, 'modified': FROZEN_NOW_TIMESTAMP, + 'score_db_table': ScoreDatabaseTableEnum.courseware_student_module, +} + +PROBLEM_WEIGHTED_SCORE_CHANGED_KWARGS = { + 'sender': None, + 'weighted_earned': 2.0, + 'weighted_possible': 4.0, + 'user_id': 'UserID', + 'course_id': 'CourseID', + 'usage_id': 'i4x://org/course/usage/123456', + 'only_if_higher': False, + 'score_deleted': True, + 'modified': FROZEN_NOW_TIMESTAMP, + 'score_db_table': ScoreDatabaseTableEnum.courseware_student_module, } @@ -110,9 +124,11 @@ class ScoreChangedSignalRelayTest(TestCase): 'weighted_possible': possible, 'weighted_earned': earned, 'user_id': self.user_mock.id, + 'anonymous_user_id': 'anonymous_id', 'course_id': 'CourseID', 'usage_id': 'i4x://org/course/usage/123456', 'modified': FROZEN_NOW_TIMESTAMP, + 'score_db_table': 'submissions', } self.signal_mock.assert_called_once_with(**expected_set_kwargs) self.get_user_mock.assert_called_once_with(kwargs['anonymous_user_id']) @@ -153,44 +169,26 @@ class ScoreChangedSignalRelayTest(TestCase): def test_raw_score_changed_signal_handler(self): problem_raw_score_changed_handler(None, **PROBLEM_RAW_SCORE_CHANGED_KWARGS) - expected_set_kwargs = { - 'sender': None, - 'weighted_earned': 2.0, - 'weighted_possible': 4.0, - 'user_id': 'UserID', - 'course_id': 'CourseID', - 'usage_id': 'i4x://org/course/usage/123456', - 'only_if_higher': False, - 'score_deleted': True, - 'modified': FROZEN_NOW_TIMESTAMP - } + expected_set_kwargs = PROBLEM_WEIGHTED_SCORE_CHANGED_KWARGS.copy() self.signal_mock.assert_called_with(**expected_set_kwargs) def test_raw_score_changed_score_deleted_optional(self): local_kwargs = PROBLEM_RAW_SCORE_CHANGED_KWARGS.copy() del local_kwargs['score_deleted'] problem_raw_score_changed_handler(None, **local_kwargs) - expected_set_kwargs = { - 'sender': None, - 'weighted_earned': 2.0, - 'weighted_possible': 4.0, - 'user_id': 'UserID', - 'course_id': 'CourseID', - 'usage_id': 'i4x://org/course/usage/123456', - 'only_if_higher': False, - 'score_deleted': False, - 'modified': FROZEN_NOW_TIMESTAMP - } + expected_set_kwargs = PROBLEM_WEIGHTED_SCORE_CHANGED_KWARGS.copy() + expected_set_kwargs['score_deleted'] = False self.signal_mock.assert_called_with(**expected_set_kwargs) @patch('lms.djangoapps.grades.signals.handlers.log.info') - def test_problem_score_changed_logging(self, mocklog): + def test_subsection_update_logging(self, mocklog): enqueue_subsection_update( sender='test', user_id=1, course_id=u'course-v1:edX+Demo_Course+DemoX', usage_id=u'block-v1:block-key', modified=FROZEN_NOW_DATETIME, + score_db_table=ScoreDatabaseTableEnum.courseware_student_module, ) log_statement = mocklog.call_args[0][0] log_statement = UUID_REGEX.sub(u'*UUID*', log_statement) @@ -199,6 +197,7 @@ class ScoreChangedSignalRelayTest(TestCase): ( u'Grades: Request async calculation of subsection grades with args: ' u'course_id:course-v1:edX+Demo_Course+DemoX, modified:{time}, ' + u'score_db_table:csm, ' u'usage_id:block-v1:block-key, user_id:1. Task [*UUID*]' ).format(time=FROZEN_NOW_DATETIME) ) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 2cfde45b3c..c1ee158efc 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -8,10 +8,10 @@ from datetime import datetime, timedelta import ddt from django.conf import settings from django.db.utils import IntegrityError +import itertools from mock import patch, MagicMock import pytz from util.date_utils import to_timestamp -from unittest import skip from student.models import anonymous_id_for_user from student.tests.factories import UserFactory @@ -25,8 +25,9 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag +from lms.djangoapps.grades.constants import ScoreDatabaseTableEnum from lms.djangoapps.grades.signals.signals import PROBLEM_WEIGHTED_SCORE_CHANGED -from lms.djangoapps.grades.tasks import recalculate_subsection_grade_v2 +from lms.djangoapps.grades.tasks import recalculate_subsection_grade_v3 @patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False}) @@ -40,7 +41,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): self.user = UserFactory() PersistentGradesEnabledFlag.objects.create(enabled_for_all_courses=True, enabled=True) - def set_up_course(self, enable_subsection_grades=True): + def set_up_course(self, enable_persistent_grades=True): """ Configures the course for this test. """ @@ -50,7 +51,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): name='course', run='run', ) - if not enable_subsection_grades: + if not enable_persistent_grades: PersistentGradesEnabledFlag.objects.create(enabled=False) self.chapter = ItemFactory.create(parent=self.course, category="chapter", display_name="Chapter") @@ -64,10 +65,12 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): ('weighted_earned', 1.0), ('weighted_possible', 2.0), ('user_id', self.user.id), + ('anonymous_user_id', 5), ('course_id', unicode(self.course.id)), ('usage_id', unicode(self.problem.location)), ('only_if_higher', None), ('modified', self.frozen_now_datetime), + ('score_db_table', ScoreDatabaseTableEnum.courseware_student_module), ]) create_new_event_transaction_id() @@ -76,11 +79,13 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): ('user_id', self.user.id), ('course_id', unicode(self.course.id)), ('usage_id', unicode(self.problem.location)), + ('anonymous_user_id', 5), ('only_if_higher', None), ('expected_modified_time', self.frozen_now_timestamp), ('score_deleted', False), ('event_transaction_id', unicode(get_event_transaction_id())), ('event_transaction_type', u'edx.grades.problem.submitted'), + ('score_db_table', ScoreDatabaseTableEnum.courseware_student_module), ]) # this call caches the anonymous id on the user object, saving 4 queries in all happy path tests @@ -96,7 +101,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): with patch("lms.djangoapps.grades.tasks.get_score", return_value=score): yield - def test_problem_weighted_score_changed_queues_task(self): + def test_triggered_by_problem_weighted_score_change(self): """ Ensures that the PROBLEM_WEIGHTED_SCORE_CHANGED signal enqueues the correct task. """ @@ -105,27 +110,37 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): local_task_args = self.recalculate_subsection_grade_kwargs.copy() local_task_args['event_transaction_type'] = u'edx.grades.problem.submitted' with self.mock_get_score() and patch( - 'lms.djangoapps.grades.tasks.recalculate_subsection_grade_v2.apply_async', + 'lms.djangoapps.grades.tasks.recalculate_subsection_grade_v3.apply_async', return_value=None ) as mock_task_apply: PROBLEM_WEIGHTED_SCORE_CHANGED.send(sender=None, **send_args) mock_task_apply.assert_called_once_with(kwargs=local_task_args) @patch('lms.djangoapps.grades.signals.signals.SUBSECTION_SCORE_CHANGED.send') - def test_subsection_update_triggers_signal(self, mock_subsection_signal): + def test_triggers_subsection_score_signal(self, mock_subsection_signal): """ - Ensures that the subsection update operation triggers a signal. + Ensures that a subsection grade recalculation triggers a signal. """ self.set_up_course() self._apply_recalculate_subsection_grade() self.assertTrue(mock_subsection_signal.called) + def test_block_structure_created_only_once(self): + self.set_up_course() + self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) + with patch( + 'openedx.core.lib.block_structure.factory.BlockStructureFactory.create_from_cache', + return_value=None, + ) as mock_block_structure_create: + self._apply_recalculate_subsection_grade() + self.assertEquals(mock_block_structure_create.call_count, 1) + @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 23), (ModuleStoreEnum.Type.split, 3, 22), ) @ddt.unpack - def test_subsection_grade_updated(self, default_store, num_mongo_calls, num_sql_calls): + def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls): with self.store.default_store(default_store): self.set_up_course() self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) @@ -133,6 +148,30 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): with self.assertNumQueries(num_sql_calls): self._apply_recalculate_subsection_grade() + # TODO (TNL-6225) Fix the number of SQL queries so they + # don't grow linearly with the number of sequentials. + @ddt.data( + (ModuleStoreEnum.Type.mongo, 1, 46), + (ModuleStoreEnum.Type.split, 3, 45), + ) + @ddt.unpack + def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): + with self.store.default_store(default_store): + self.set_up_course() + self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) + + num_problems = 10 + for _ in range(num_problems): + ItemFactory.create(parent=self.sequential, category='problem') + + num_sequentials = 10 + for _ in range(num_sequentials): + ItemFactory.create(parent=self.chapter, category='sequential') + + with check_mongo_calls(num_mongo_calls): + with self.assertNumQueries(num_sql_calls): + self._apply_recalculate_subsection_grade() + @patch('lms.djangoapps.grades.signals.signals.SUBSECTION_SCORE_CHANGED.send') def test_other_inaccessible_subsection(self, mock_subsection_signal): self.set_up_course() @@ -157,112 +196,15 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): {self.sequential.location, accessible_seq.location}, ) - def test_single_call_to_create_block_structure(self): - self.set_up_course() - self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) - with patch( - 'openedx.core.lib.block_structure.factory.BlockStructureFactory.create_from_cache', - return_value=None, - ) as mock_block_structure_create: - self._apply_recalculate_subsection_grade() - self.assertEquals(mock_block_structure_create.call_count, 1) - - # TODO (TNL-6225) Fix the number of SQL queries so they - # don't grow linearly with the number of sequentials. - @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 46), - (ModuleStoreEnum.Type.split, 3, 45), - ) - @ddt.unpack - def test_query_count_does_not_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): - with self.store.default_store(default_store): - self.set_up_course() - self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) - - num_problems = 10 - for _ in range(num_problems): - ItemFactory.create(parent=self.sequential, category='problem') - - num_sequentials = 10 - for _ in range(num_sequentials): - ItemFactory.create(parent=self.chapter, category='sequential') - - with check_mongo_calls(num_mongo_calls): - with self.assertNumQueries(num_sql_calls): - self._apply_recalculate_subsection_grade() - @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) - def test_subsection_grades_not_enabled_on_course(self, default_store): + def test_persistent_grades_not_enabled_on_course(self, default_store): with self.store.default_store(default_store): - self.set_up_course(enable_subsection_grades=False) + self.set_up_course(enable_persistent_grades=False) self.assertFalse(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) with check_mongo_calls(0): with self.assertNumQueries(0): self._apply_recalculate_subsection_grade() - @skip("Pending completion of TNL-5089") - @ddt.data( - (ModuleStoreEnum.Type.mongo, True), - (ModuleStoreEnum.Type.split, True), - (ModuleStoreEnum.Type.mongo, False), - (ModuleStoreEnum.Type.split, False), - ) - @ddt.unpack - def test_query_counts_with_feature_flag(self, default_store, feature_flag): - PersistentGradesEnabledFlag.objects.create(enabled=feature_flag) - with self.store.default_store(default_store): - self.set_up_course() - with check_mongo_calls(0): - with self.assertNumQueries(3 if feature_flag else 2): - self._apply_recalculate_subsection_grade() - - @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v2.retry') - @patch('lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory.update') - def test_retry_subsection_update_on_integrity_error(self, mock_update, mock_retry): - """ - Ensures that tasks will be retried if IntegrityErrors are encountered. - """ - self.set_up_course() - mock_update.side_effect = IntegrityError("WHAMMY") - self._apply_recalculate_subsection_grade() - self._assert_retry_called(mock_retry) - - @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v2.retry') - def test_retry_subsection_grade_on_update_not_complete(self, mock_retry): - self.set_up_course() - self._apply_recalculate_subsection_grade( - mock_score=MagicMock(modified=datetime.utcnow().replace(tzinfo=pytz.UTC) - timedelta(days=1)) - ) - self._assert_retry_called(mock_retry) - - @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v2.retry') - def test_retry_subsection_grade_on_update_not_complete_sub(self, mock_retry): - self.set_up_course() - with patch('lms.djangoapps.grades.tasks.sub_api.get_score') as mock_sub_score: - mock_sub_score.return_value = { - 'created_at': datetime.utcnow().replace(tzinfo=pytz.UTC) - timedelta(days=1) - } - self._apply_recalculate_subsection_grade( - mock_score=MagicMock(module_type='openassessment') - ) - self._assert_retry_called(mock_retry) - - @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v2.retry') - def test_retry_subsection_grade_on_no_score(self, mock_retry): - self.set_up_course() - self._apply_recalculate_subsection_grade(mock_score=None) - self._assert_retry_called(mock_retry) - - @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v2.retry') - def test_retry_subsection_grade_on_no_sub_score(self, mock_retry): - self.set_up_course() - with patch('lms.djangoapps.grades.tasks.sub_api.get_score') as mock_sub_score: - mock_sub_score.return_value = None - self._apply_recalculate_subsection_grade( - mock_score=MagicMock(module_type='openassessment') - ) - self._assert_retry_called(mock_retry) - @patch('lms.djangoapps.grades.signals.signals.SUBSECTION_SCORE_CHANGED.send') @patch('lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory.update') def test_retry_first_time_only(self, mock_update, mock_course_signal): @@ -274,8 +216,68 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): self._apply_recalculate_subsection_grade() self.assertEquals(mock_course_signal.call_count, 1) + @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v3.retry') + @patch('lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory.update') + def test_retry_on_integrity_error(self, mock_update, mock_retry): + """ + Ensures that tasks will be retried if IntegrityErrors are encountered. + """ + self.set_up_course() + mock_update.side_effect = IntegrityError("WHAMMY") + self._apply_recalculate_subsection_grade() + self._assert_retry_called(mock_retry) + + @ddt.data(ScoreDatabaseTableEnum.courseware_student_module, ScoreDatabaseTableEnum.submissions) + @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v3.retry') + def test_retry_when_db_not_updated(self, score_db_table, mock_retry): + self.set_up_course() + self.recalculate_subsection_grade_kwargs['score_db_table'] = score_db_table + + modified_datetime = datetime.utcnow().replace(tzinfo=pytz.UTC) - timedelta(days=1) + if score_db_table == ScoreDatabaseTableEnum.submissions: + with patch('lms.djangoapps.grades.tasks.sub_api.get_score') as mock_sub_score: + mock_sub_score.return_value = { + 'created_at': modified_datetime + } + self._apply_recalculate_subsection_grade( + mock_score=MagicMock(module_type='any_block_type') + ) + else: + self._apply_recalculate_subsection_grade( + mock_score=MagicMock(modified=modified_datetime) + ) + + self._assert_retry_called(mock_retry) + + @ddt.data( + *itertools.product( + (True, False), + (ScoreDatabaseTableEnum.courseware_student_module, ScoreDatabaseTableEnum.submissions), + ) + ) + @ddt.unpack + @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v3.retry') + def test_when_no_score_found(self, score_deleted, score_db_table, mock_retry): + self.set_up_course() + self.recalculate_subsection_grade_kwargs['score_deleted'] = score_deleted + self.recalculate_subsection_grade_kwargs['score_db_table'] = score_db_table + + if score_db_table == ScoreDatabaseTableEnum.submissions: + with patch('lms.djangoapps.grades.tasks.sub_api.get_score') as mock_sub_score: + mock_sub_score.return_value = None + self._apply_recalculate_subsection_grade( + mock_score=MagicMock(module_type='any_block_type') + ) + else: + self._apply_recalculate_subsection_grade(mock_score=None) + + if score_deleted: + self._assert_retry_not_called(mock_retry) + else: + self._assert_retry_called(mock_retry) + @patch('lms.djangoapps.grades.tasks.log') - @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v2.retry') + @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v3.retry') @patch('lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory.update') def test_log_unknown_error(self, mock_update, mock_retry, mock_log): """ @@ -288,7 +290,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): self._assert_retry_called(mock_retry) @patch('lms.djangoapps.grades.tasks.log') - @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v2.retry') + @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade_v3.retry') @patch('lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory.update') def test_no_log_known_error(self, mock_update, mock_retry, mock_log): """ @@ -309,7 +311,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): mocking in place. """ with self.mock_get_score(mock_score): - recalculate_subsection_grade_v2.apply(kwargs=self.recalculate_subsection_grade_kwargs) + recalculate_subsection_grade_v3.apply(kwargs=self.recalculate_subsection_grade_kwargs) def _assert_retry_called(self, mock_retry): """ @@ -318,3 +320,9 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): """ self.assertTrue(mock_retry.called) self.assertEquals(len(mock_retry.call_args[1]['kwargs']), len(self.recalculate_subsection_grade_kwargs)) + + def _assert_retry_not_called(self, mock_retry): + """ + Verifies the task was not retried. + """ + self.assertFalse(mock_retry.called) diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index 168754899b..c9273dfde1 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -16,10 +16,8 @@ from django.utils.translation import override as override_language from eventtracking import tracker import pytz -from course_modes.models import CourseMode -from courseware.models import StudentModule -from edxmako.shortcuts import render_to_string from lms.djangoapps.grades.signals.signals import PROBLEM_RAW_SCORE_CHANGED +from lms.djangoapps.grades.constants import ScoreDatabaseTableEnum from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_api.models import UserPreference @@ -329,19 +327,22 @@ def _fire_score_changed_for_block( The earned points are always zero. We must retrieve the possible points from the XModule, as noted below. The effective time is now(). """ - if block and block.has_score and block.max_score() is not None: - PROBLEM_RAW_SCORE_CHANGED.send( - sender=None, - raw_earned=0, - raw_possible=block.max_score(), - weight=getattr(block, 'weight', None), - user_id=student.id, - course_id=unicode(course_id), - usage_id=unicode(module_state_key), - score_deleted=True, - only_if_higher=False, - modified=datetime.now().replace(tzinfo=pytz.UTC), - ) + if block and block.has_score: + max_score = block.max_score() + if max_score is not None: + PROBLEM_RAW_SCORE_CHANGED.send( + sender=None, + raw_earned=0, + raw_possible=max_score, + weight=getattr(block, 'weight', None), + user_id=student.id, + course_id=unicode(course_id), + usage_id=unicode(module_state_key), + score_deleted=True, + only_if_higher=False, + modified=datetime.now().replace(tzinfo=pytz.UTC), + score_db_table=ScoreDatabaseTableEnum.courseware_student_module, + ) def get_email_params(course, auto_enroll, secure=True, course_key=None, display_name=None):