From b9dade5512e235428f035de17244b31f99a8b523 Mon Sep 17 00:00:00 2001 From: Sanford Student Date: Tue, 16 Aug 2016 15:29:16 -0400 Subject: [PATCH] handler for SCORE_CHANGED signal --- lms/djangoapps/courseware/module_render.py | 2 +- .../courseware/tests/test_module_render.py | 2 +- lms/djangoapps/gating/signals.py | 2 +- lms/djangoapps/gating/tests/test_signals.py | 9 +- lms/djangoapps/grades/new/subsection_grade.py | 18 +++ lms/djangoapps/grades/signals.py | 40 ++++++- lms/djangoapps/grades/tests/test_signals.py | 110 +++++++++++++++++- lms/djangoapps/lti_provider/tasks.py | 12 +- 8 files changed, 176 insertions(+), 19 deletions(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 6b1cf3c3f4..6c22a86587 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -539,7 +539,7 @@ def get_module_system_for_user(user, student_data, # TODO # pylint: disable=to sender=None, points_possible=event['max_value'], points_earned=event['value'], - user_id=user_id, + user=user, course_id=unicode(course_id), usage_id=unicode(descriptor.location) ) diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 3ec81a2844..590186963e 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -1843,7 +1843,7 @@ class TestXmoduleRuntimeEvent(TestSubmittingProblems): 'sender': None, 'points_possible': self.grade_dict['max_value'], 'points_earned': self.grade_dict['value'], - 'user_id': self.student_user.id, + 'user': self.student_user, 'course_id': unicode(self.course.id), 'usage_id': unicode(self.problem.location) } diff --git a/lms/djangoapps/gating/signals.py b/lms/djangoapps/gating/signals.py index 8f14f8c87b..561d8df0ac 100644 --- a/lms/djangoapps/gating/signals.py +++ b/lms/djangoapps/gating/signals.py @@ -26,5 +26,5 @@ def handle_score_changed(**kwargs): gating_api.evaluate_prerequisite( course, UsageKey.from_string(kwargs.get('usage_id')), - kwargs.get('user_id'), + kwargs.get('user').id, ) diff --git a/lms/djangoapps/gating/tests/test_signals.py b/lms/djangoapps/gating/tests/test_signals.py index 10f5e84540..16116da608 100644 --- a/lms/djangoapps/gating/tests/test_signals.py +++ b/lms/djangoapps/gating/tests/test_signals.py @@ -4,6 +4,7 @@ Unit tests for gating.signals module from mock import patch from opaque_keys.edx.keys import UsageKey +from student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.django import modulestore @@ -18,7 +19,7 @@ class TestHandleScoreChanged(ModuleStoreTestCase): def setUp(self): super(TestHandleScoreChanged, self).setUp() self.course = CourseFactory.create(org='TestX', number='TS01', run='2016_Q1') - self.test_user_id = 0 + self.user = UserFactory() self.test_usage_key = UsageKey.from_string('i4x://the/content/key/12345678') @patch('gating.signals.gating_api.evaluate_prerequisite') @@ -30,11 +31,11 @@ class TestHandleScoreChanged(ModuleStoreTestCase): sender=None, points_possible=1, points_earned=1, - user_id=self.test_user_id, + user=self.user, course_id=unicode(self.course.id), usage_id=unicode(self.test_usage_key) ) - mock_evaluate.assert_called_with(self.course, self.test_usage_key, self.test_user_id) + mock_evaluate.assert_called_with(self.course, self.test_usage_key, self.user.id) # pylint: disable=no-member @patch('gating.signals.gating_api.evaluate_prerequisite') def test_gating_disabled(self, mock_evaluate): @@ -43,7 +44,7 @@ class TestHandleScoreChanged(ModuleStoreTestCase): sender=None, points_possible=1, points_earned=1, - user_id=self.test_user_id, + user=self.user, course_id=unicode(self.course.id), usage_id=unicode(self.test_usage_key) ) diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index 7f027a32b2..781567de23 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -6,6 +6,7 @@ from lazy import lazy from django.conf import settings +from course_blocks.api import get_course_blocks from courseware.model_data import ScoresClient from lms.djangoapps.grades.scores import get_score, possibly_scored from lms.djangoapps.grades.models import BlockRecord, PersistentSubsectionGrade @@ -59,6 +60,7 @@ class SubsectionGrade(object): 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, @@ -166,6 +168,22 @@ class SubsectionGradeFactory(object): self._compute_and_save_grade(subsection, course_structure, course) ) + def update(self, usage_key, course_key): + """ + Updates the SubsectionGrade object for the student and subsection + identified by the given usage key. + """ + from courseware.courses import get_course_by_id # avoids circular import with courseware.py + course = get_course_by_id(course_key, depth=0) + # save ourselves the extra queries if the course does not use subsection grades + if not course.enable_subsection_grades_saved: + return + + course_structure = get_course_blocks(self.student, usage_key) + subsection = course_structure[usage_key] + self._prefetch_scores(course_structure, course) + 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. diff --git a/lms/djangoapps/grades/signals.py b/lms/djangoapps/grades/signals.py index b3cc91667c..ba034a8c52 100644 --- a/lms/djangoapps/grades/signals.py +++ b/lms/djangoapps/grades/signals.py @@ -1,12 +1,14 @@ """ Grades related signals. """ +from django.conf import settings from django.dispatch import receiver, Signal from logging import getLogger +from opaque_keys.edx.locator import CourseLocator +from opaque_keys.edx.keys import UsageKey from student.models import user_by_anonymous_id from submissions.models import score_set, score_reset - log = getLogger(__name__) @@ -58,7 +60,7 @@ def submissions_score_set_handler(sender, **kwargs): # pylint: disable=unused-a sender=None, points_possible=points_possible, points_earned=points_earned, - user_id=user.id, + user=user, course_id=course_id, usage_id=usage_id ) @@ -97,7 +99,7 @@ def submissions_score_reset_handler(sender, **kwargs): # pylint: disable=unused sender=None, points_possible=0, points_earned=0, - user_id=user.id, + user=user, course_id=course_id, usage_id=usage_id ) @@ -106,3 +108,35 @@ def submissions_score_reset_handler(sender, **kwargs): # pylint: disable=unused u"Failed to process score_reset signal from Submissions API. " "user: %s, course_id: %s, usage_id: %s", user, course_id, usage_id ) + + +@receiver(SCORE_CHANGED) +def recalculate_subsection_grade_handler(sender, **kwargs): # pylint: disable=unused-argument + """ + Consume the SCORE_CHANGED signal and trigger an update. + This method expects that the kwargs dictionary will contain the following + entries (See the definition of SCORE_CHANGED): + - points_possible: Maximum score available for the exercise + - points_earned: Score obtained by the user + - user: User object + - course_id: Unicode string representing the course + - usage_id: Unicode string indicating the courseware instance + """ + if not settings.FEATURES.get('ENABLE_SUBSECTION_GRADES_SAVED', False): + return + try: + course_id = kwargs.get('course_id', None) + usage_id = kwargs.get('usage_id', None) + student = kwargs.get('user', None) + + course_key = CourseLocator.from_string(course_id) + usage_key = UsageKey.from_string(usage_id).replace(course_key=course_key) + + from lms.djangoapps.grades.new.subsection_grade import SubsectionGradeFactory + SubsectionGradeFactory(student).update(usage_key, course_key) + except Exception as ex: # pylint: disable=broad-except + log.exception( + u"Failed to process SCORE_CHANGED signal. " + "user: %s, course_id: %s, " + "usage_id: %s. Exception: %s", unicode(student), course_id, usage_id, ex.message + ) diff --git a/lms/djangoapps/grades/tests/test_signals.py b/lms/djangoapps/grades/tests/test_signals.py index 1c14d2bded..517fbec914 100644 --- a/lms/djangoapps/grades/tests/test_signals.py +++ b/lms/djangoapps/grades/tests/test_signals.py @@ -2,10 +2,22 @@ Tests for the score change signals defined in the courseware models module. """ +import ddt +from unittest import skip from django.test import TestCase from mock import patch, MagicMock +from student.models import anonymous_id_for_user +from student.tests.factories import UserFactory +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls -from ..signals import submissions_score_set_handler, submissions_score_reset_handler +from ..signals import ( + submissions_score_set_handler, + submissions_score_reset_handler, + recalculate_subsection_grade_handler, + SCORE_CHANGED +) SUBMISSION_SET_KWARGS = { @@ -62,7 +74,7 @@ class SubmissionSignalRelayTest(TestCase): 'sender': None, 'points_possible': 10, 'points_earned': 5, - 'user_id': 42, + 'user': self.user_mock, 'course_id': 'CourseID', 'usage_id': 'i4x://org/course/usage/123456' } @@ -111,7 +123,7 @@ class SubmissionSignalRelayTest(TestCase): 'sender': None, 'points_possible': 0, 'points_earned': 0, - 'user_id': 42, + 'user': self.user_mock, 'course_id': 'CourseID', 'usage_id': 'i4x://org/course/usage/123456' } @@ -148,3 +160,95 @@ class SubmissionSignalRelayTest(TestCase): self.get_user_mock = self.setup_patch('lms.djangoapps.grades.signals.user_by_anonymous_id', None) submissions_score_reset_handler(None, **SUBMISSION_RESET_KWARGS) self.signal_mock.assert_not_called() + + +@ddt.ddt +class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): + """ + Ensures that upon SCORE_CHANGED signals, the handler + initiates an update to the affected subsection grade. + """ + def setUp(self): + super(ScoreChangedUpdatesSubsectionGradeTest, self).setUp() + self.user = UserFactory() + + def set_up_course(self, enable_subsection_grades=True): + """ + Configures the course for this test. + """ + # pylint: disable=attribute-defined-outside-init,no-member + self.course = CourseFactory.create( + org='edx', + name='course', + run='run', + metadata={'enable_subsection_grades_saved': enable_subsection_grades}) + + self.chapter = ItemFactory.create(parent=self.course, category="chapter", display_name="Chapter") + self.sequential = ItemFactory.create(parent=self.chapter, category='sequential', display_name="Open Sequential") + self.problem = ItemFactory.create(parent=self.sequential, category='problem', display_name='problem') + + self.score_changed_kwargs = { + 'points_possible': 10, + 'points_earned': 5, + 'user': self.user, + 'course_id': unicode(self.course.id), + 'usage_id': unicode(self.problem.location), + } + + # this call caches the anonymous id on the user object, saving 4 queries in all happy path tests + _ = anonymous_id_for_user(self.user, self.course.id) + # pylint: enable=attribute-defined-outside-init,no-member + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_subsection_grade_updated_on_signal(self, default_store): + with self.store.default_store(default_store): + self.set_up_course() + with check_mongo_calls(2) and self.assertNumQueries(13): + recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_query_count_does_not_change_with_more_problems(self, default_store): + with self.store.default_store(default_store): + self.set_up_course() + ItemFactory.create(parent=self.sequential, category='problem', display_name='problem2') + ItemFactory.create(parent=self.sequential, category='problem', display_name='problem3') + with check_mongo_calls(2) and self.assertNumQueries(13): + recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_subsection_grades_not_enabled_on_course(self, default_store): + with self.store.default_store(default_store): + self.set_up_course(enable_subsection_grades=False) + with check_mongo_calls(2) and self.assertNumQueries(0): + recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) + + @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_score_changed_sent_with_feature_flag(self, default_store, feature_flag): + with patch.dict('django.conf.settings.FEATURES', {'ENABLE_SUBSECTION_GRADES_SAVED': feature_flag}): + with self.store.default_store(default_store): + self.set_up_course() + with check_mongo_calls(0) and self.assertNumQueries(19 if feature_flag else 1): + SCORE_CHANGED.send(sender=None, **self.score_changed_kwargs) + + @ddt.data( + ('points_possible', 2, 13), + ('points_earned', 2, 13), + ('user', 0, 0), + ('course_id', 0, 0), + ('usage_id', 0, 0), + ) + @ddt.unpack + def test_missing_kwargs(self, kwarg, expected_mongo_calls, expected_sql_calls): + self.set_up_course() + del self.score_changed_kwargs[kwarg] + with patch('lms.djangoapps.grades.signals.log') as log_mock: + with check_mongo_calls(expected_mongo_calls) and self.assertNumQueries(expected_sql_calls): + recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) + self.assertEqual(log_mock.exception.called, kwarg not in ['points_possible', 'points_earned']) diff --git a/lms/djangoapps/lti_provider/tasks.py b/lms/djangoapps/lti_provider/tasks.py index dd31a00617..fa0958f04a 100644 --- a/lms/djangoapps/lti_provider/tasks.py +++ b/lms/djangoapps/lti_provider/tasks.py @@ -27,13 +27,13 @@ def score_changed_handler(sender, **kwargs): # pylint: disable=unused-argument """ points_possible = kwargs.get('points_possible', None) points_earned = kwargs.get('points_earned', None) - user_id = kwargs.get('user_id', None) + user = kwargs.get('user', None) course_id = kwargs.get('course_id', None) usage_id = kwargs.get('usage_id', None) - if None not in (points_earned, points_possible, user_id, course_id, user_id): + if None not in (points_earned, points_possible, user.id, course_id, user.id): course_key, usage_key = parse_course_and_usage_keys(course_id, usage_id) - assignments = increment_assignment_versions(course_key, usage_key, user_id) + assignments = increment_assignment_versions(course_key, usage_key, user.id) for assignment in assignments: if assignment.usage_key == usage_key: send_leaf_outcome.delay( @@ -41,15 +41,15 @@ def score_changed_handler(sender, **kwargs): # pylint: disable=unused-argument ) else: send_composite_outcome.apply_async( - (user_id, course_id, assignment.id, assignment.version_number), + (user.id, course_id, assignment.id, assignment.version_number), countdown=settings.LTI_AGGREGATE_SCORE_PASSBACK_DELAY ) else: log.error( "Outcome Service: Required signal parameter is None. " - "points_possible: %s, points_earned: %s, user_id: %s, " + "points_possible: %s, points_earned: %s, user: %s, " "course_id: %s, usage_id: %s", - points_possible, points_earned, user_id, course_id, usage_id + points_possible, points_earned, unicode(user), course_id, usage_id )