From 8a2d499dd2af75dd4de165f549ff72f3847c20d5 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 11 Sep 2019 14:15:04 -0700 Subject: [PATCH] Track grades for Blockstore content, emit tracking logs --- common/djangoapps/track/contexts.py | 53 ++++++++++-- .../commands/recalculate_subsection_grades.py | 6 ++ .../test_recalculate_subsection_grades.py | 7 +- lms/djangoapps/grades/signals/handlers.py | 4 + lms/djangoapps/lti_provider/signals.py | 8 ++ .../content_libraries/tests/test_runtime.py | 77 ++++++++++++++++- .../core/djangoapps/xblock/runtime/runtime.py | 85 ++++++++++++++++++- .../core/djangoapps/xblock/runtime/shims.py | 13 +++ 8 files changed, 237 insertions(+), 16 deletions(-) diff --git a/common/djangoapps/track/contexts.py b/common/djangoapps/track/contexts.py index 0954a8b548..2f110de3e2 100644 --- a/common/djangoapps/track/contexts.py +++ b/common/djangoapps/track/contexts.py @@ -4,7 +4,7 @@ from __future__ import absolute_import import logging from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.keys import CourseKey, LearningContextKey from six import text_type from openedx.core.lib.request_utils import COURSE_REGEX @@ -40,20 +40,55 @@ def course_context_from_course_id(course_id): """ Creates a course context from a `course_id`. + For newer parts of the system (i.e. Blockstore-based libraries/courses/etc.) + use context_dict_for_learning_context instead of this method. + Example Returned Context:: { 'course_id': 'org/course/run', 'org_id': 'org' } + """ + context_dict = context_dict_for_learning_context(course_id) + # Remove the newer 'context_id' field for now in this method so we're not + # adding a new field to the course tracking logs + del context_dict['context_id'] + return context_dict + + +def context_dict_for_learning_context(context_key): + """ + Creates a tracking log context dictionary for the given learning context + key, which may be None, a CourseKey, a content library key, or any other + type of LearningContextKey. + + Example Returned Context Dict:: + + { + 'context_id': 'course-v1:org+course+run', + 'course_id': 'course-v1:org+course+run', + 'org_id': 'org' + } + + Example 2:: + + { + 'context_id': 'lib:edX:a-content-library', + 'course_id': '', + 'org_id': 'edX' + } """ - if course_id is None: - return {'course_id': '', 'org_id': ''} - - # TODO: Make this accept any CourseKey, and serialize it using .to_string - assert isinstance(course_id, CourseKey) - return { - 'course_id': text_type(course_id), - 'org_id': course_id.org, + context_dict = { + 'context_id': text_type(context_key) if context_key else '', + 'course_id': '', + 'org_id': '', } + if context_key is not None: + assert isinstance(context_key, LearningContextKey) + if context_key.is_course: + context_dict['course_id'] = text_type(context_key) + if hasattr(context_key, 'org'): + context_dict['org_id'] = context_key.org + return context_dict diff --git a/lms/djangoapps/grades/management/commands/recalculate_subsection_grades.py b/lms/djangoapps/grades/management/commands/recalculate_subsection_grades.py index 993e80b742..e4f7895297 100644 --- a/lms/djangoapps/grades/management/commands/recalculate_subsection_grades.py +++ b/lms/djangoapps/grades/management/commands/recalculate_subsection_grades.py @@ -63,6 +63,9 @@ class Command(BaseCommand): set_event_transaction_type(PROBLEM_SUBMITTED_EVENT_TYPE) kwargs = {'modified__range': (modified_start, modified_end), 'module_type': 'problem'} for record in StudentModule.objects.filter(**kwargs): + if not record.course_id.is_course: + # This is not a course, so we don't store subsection grades for it. + continue task_args = { "user_id": record.student_id, "course_id": six.text_type(record.course_id), @@ -78,6 +81,9 @@ class Command(BaseCommand): kwargs = {'created_at__range': (modified_start, modified_end)} for record in Submission.objects.filter(**kwargs): + if not record.student_item.course_id.is_course: + # This is not a course, so ignore it + continue task_args = { "user_id": user_by_anonymous_id(record.student_item.student_id).id, "anonymous_user_id": record.student_item.student_id, diff --git a/lms/djangoapps/grades/management/commands/tests/test_recalculate_subsection_grades.py b/lms/djangoapps/grades/management/commands/tests/test_recalculate_subsection_grades.py index b06134aee5..41a5ca40ee 100644 --- a/lms/djangoapps/grades/management/commands/tests/test_recalculate_subsection_grades.py +++ b/lms/djangoapps/grades/management/commands/tests/test_recalculate_subsection_grades.py @@ -10,6 +10,7 @@ import ddt import six from django.conf import settings from mock import MagicMock, patch +from opaque_keys.edx.keys import CourseKey from pytz import utc from lms.djangoapps.grades.constants import ScoreDatabaseTableEnum @@ -40,7 +41,7 @@ class TestRecalculateSubsectionGrades(HasCourseWithProblemsMixin, ModuleStoreTes submission = MagicMock() submission.student_item = MagicMock( student_id="anonymousID", - course_id='x/y/z', + course_id=CourseKey.from_string('course-v1:x+y+z'), item_id='abc', ) submission.created_at = utc.localize(datetime.strptime('2016-08-23 16:43', DATE_FORMAT)) @@ -55,7 +56,7 @@ class TestRecalculateSubsectionGrades(HasCourseWithProblemsMixin, ModuleStoreTes def test_csm(self, task_mock, id_mock, csm_mock): csm_record = MagicMock() csm_record.student_id = "ID" - csm_record.course_id = "x/y/z" + csm_record.course_id = CourseKey.from_string('course-v1:x+y+z') csm_record.module_state_key = "abc" csm_record.modified = utc.localize(datetime.strptime('2016-08-23 16:43', DATE_FORMAT)) csm_mock.objects.filter.return_value = [csm_record] @@ -67,7 +68,7 @@ class TestRecalculateSubsectionGrades(HasCourseWithProblemsMixin, ModuleStoreTes self.command.handle(modified_start='2016-08-25 16:42', modified_end='2018-08-25 16:44') kwargs = { "user_id": "ID", - "course_id": u'x/y/z', + "course_id": u'course-v1:x+y+z', "usage_id": u'abc', "only_if_higher": False, "expected_modified_time": to_timestamp(utc.localize(datetime.strptime('2016-08-23 16:43', DATE_FORMAT))), diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index c6be5b58ec..bb5bf19b98 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/lms/djangoapps/grades/signals/handlers.py @@ -8,6 +8,7 @@ from logging import getLogger import six from django.dispatch import receiver +from opaque_keys.edx.keys import LearningContextKey from submissions.models import score_reset, score_set from xblock.scorable import ScorableXBlockMixin, Score @@ -220,6 +221,9 @@ def enqueue_subsection_update(sender, **kwargs): # pylint: disable=unused-argum enqueueing a subsection update operation to occur asynchronously. """ events.grade_updated(**kwargs) + context_key = LearningContextKey.from_string(kwargs['course_id']) + if not context_key.is_course: + return # If it's not a course, it has no subsections, so skip the subsection grading update recalculate_subsection_grade_v3.apply_async( kwargs=dict( user_id=kwargs['user_id'], diff --git a/lms/djangoapps/lti_provider/signals.py b/lms/djangoapps/lti_provider/signals.py index 9956bf8785..5d2318d66d 100644 --- a/lms/djangoapps/lti_provider/signals.py +++ b/lms/djangoapps/lti_provider/signals.py @@ -6,6 +6,7 @@ import logging from django.conf import settings from django.dispatch import receiver +from opaque_keys.edx.keys import LearningContextKey import lti_provider.outcomes as outcomes from lms.djangoapps.grades.api import signals as grades_signals @@ -47,6 +48,13 @@ def score_changed_handler(sender, **kwargs): # pylint: disable=unused-argument course_id = kwargs.get('course_id', None) usage_id = kwargs.get('usage_id', None) + # Make sure this came from a course because this code only works with courses + if not course_id: + return + context_key = LearningContextKey.from_string(course_id) + if not context_key.is_course: + return # This is a content library or something else... + if None not in (points_earned, points_possible, user_id, course_id): course_key, usage_key = parse_course_and_usage_keys(course_id, usage_id) assignments = increment_assignment_versions(course_key, usage_key, user_id) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_runtime.py b/openedx/core/djangoapps/content_libraries/tests/test_runtime.py index 7a3f1aea6d..365e209caf 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_runtime.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_runtime.py @@ -3,15 +3,22 @@ Test the Blockstore-based XBlock runtime and content libraries together. """ from __future__ import absolute_import, division, print_function, unicode_literals +import json import unittest from django.conf import settings from django.test import TestCase from organizations.models import Organization +from rest_framework.test import APIClient from xblock.core import XBlock, Scope from xblock import fields +from courseware.model_data import get_score from openedx.core.djangoapps.content_libraries import api as library_api +from openedx.core.djangoapps.content_libraries.tests.test_content_libraries import ( + URL_BLOCK_RENDER_VIEW, + URL_BLOCK_GET_HANDLER_URL, +) from openedx.core.djangoapps.xblock import api as xblock_api from openedx.core.lib import blockstore_api from student.tests.factories import UserFactory @@ -40,8 +47,8 @@ class ContentLibraryContentTestMixin(object): def setUpClass(cls): super(ContentLibraryContentTestMixin, cls).setUpClass() # Create a couple students that the tests can use - cls.student_a = UserFactory.create(username="Alice", email="alice@example.com") - cls.student_b = UserFactory.create(username="Bob", email="bob@example.com") + cls.student_a = UserFactory.create(username="Alice", email="alice@example.com", password="edx") + cls.student_b = UserFactory.create(username="Bob", email="bob@example.com", password="edx") # Create a collection using Blockstore API directly only because there # is not yet any Studio REST API for doing so: cls.collection = blockstore_api.create_collection("Content Library Test Collection") @@ -187,3 +194,69 @@ class ContentLibraryXBlockUserStateTest(ContentLibraryContentTestMixin, TestCase block_instance2 = block_instance1.runtime.get_block(block_usage_key) # Now they should be equal, because we've saved and re-loaded instance2: self.assertEqual(block_instance1.user_str, block_instance2.user_str) + + @unittest.skipUnless(settings.ROOT_URLCONF == "lms.urls", "Scores are only used in the LMS") + def test_scores_persisted(self): + """ + Test that a block's emitted scores are cached in StudentModule + + In the future if there is a REST API to retrieve individual block's + scores, that should be used instead of checking StudentModule directly. + """ + block_id = library_api.create_library_block(self.library.key, "problem", "scored_problem").usage_key + new_olx = """ + + +

This is a normal capa problem. It has "maximum attempts" set to **5**.

+ + + XBlock metadata only + XBlock data/metadata and associated static asset files + Static asset files for XBlocks and courseware + XModule metadata only + + + + """.strip() + library_api.set_library_block_olx(block_id, new_olx) + library_api.publish_changes(self.library.key) + + # Now view the problem as Alice: + client = APIClient() + client.login(username=self.student_a.username, password='edx') + student_view_result = client.get(URL_BLOCK_RENDER_VIEW.format(block_key=block_id, view_name='student_view')) + problem_key = "input_{}_2_1".format(block_id) + self.assertIn(problem_key, student_view_result.data["content"]) + # And submit a wrong answer: + result = client.get(URL_BLOCK_GET_HANDLER_URL.format(block_key=block_id, handler_name='xmodule_handler')) + problem_check_url = result.data["handler_url"] + 'problem_check' + + submit_result = client.post(problem_check_url, data={problem_key: "choice_3"}) + self.assertEqual(submit_result.status_code, 200) + submit_data = json.loads(submit_result.content) + self.assertDictContainsSubset({ + "current_score": 0, + "total_possible": 1, + "attempts_used": 1, + }, submit_data) + + # Now test that the score is also persisted in StudentModule: + # If we add a REST API to get an individual block's score, that should be checked instead of StudentModule. + sm = get_score(self.student_a, block_id) + self.assertEqual(sm.grade, 0) + self.assertEqual(sm.max_grade, 1) + + # And submit a correct answer: + submit_result = client.post(problem_check_url, data={problem_key: "choice_1"}) + self.assertEqual(submit_result.status_code, 200) + submit_data = json.loads(submit_result.content) + self.assertDictContainsSubset({ + "current_score": 1, + "total_possible": 1, + "attempts_used": 2, + }, submit_data) + # Now test that the score is also updated in StudentModule: + # If we add a REST API to get an individual block's score, that should be checked instead of StudentModule. + sm = get_score(self.student_a, block_id) + self.assertEqual(sm.grade, 1) + self.assertEqual(sm.max_grade, 1) diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index 3bb4da1d50..be68a18784 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -4,9 +4,14 @@ Common base classes for all new XBlock runtimes. from __future__ import absolute_import, division, print_function, unicode_literals import logging +from completion import waffle as completion_waffle +import crum from django.contrib.auth import get_user_model from django.utils.lru_cache import lru_cache +from eventtracking import tracker from six.moves.urllib.parse import urljoin # pylint: disable=import-error +import track.contexts +import track.views from xblock.exceptions import NoSuchServiceError from xblock.field_data import SplitFieldData from xblock.fields import Scope @@ -14,6 +19,7 @@ from xblock.runtime import DictKeyValueStore, KvsFieldData, NullI18nService, Mem from web_fragments.fragment import Fragment from courseware.model_data import DjangoKeyValueStore, FieldDataCache +from lms.djangoapps.grades.api import signals as grades_signals from openedx.core.djangoapps.xblock.apps import get_xblock_app_config from openedx.core.djangoapps.xblock.runtime.blockstore_field_data import BlockstoreFieldData from openedx.core.djangoapps.xblock.runtime.mixin import LmsBlockMixin @@ -27,6 +33,17 @@ log = logging.getLogger(__name__) User = get_user_model() +def make_track_function(): + """ + Make a tracking function that logs what happened, for XBlock events. + """ + current_request = crum.get_current_request() + + def function(event_type, event): + return track.views.server_track(current_request, event_type, event, page='x_module') + return function + + class XBlockRuntime(RuntimeShim, Runtime): """ This class manages one or more instantiated XBlocks for a particular user, @@ -94,8 +111,72 @@ class XBlockRuntime(RuntimeShim, Runtime): return absolute_url def publish(self, block, event_type, event_data): - # TODO: publish events properly - log.info("XBlock %s has published a '%s' event.", block.scope_ids.usage_id, event_type) + """ Handle XBlock events like grades and completion """ + special_handler = self.get_event_handler(event_type) + if special_handler: + special_handler(block, event_data) + else: + self.log_event_to_tracking_log(block, event_type, event_data) + + def get_event_handler(self, event_type): + """ + Return an appropriate function to handle the event. + + Returns None if no special processing is required. + """ + if self.user_id is None: + # We don't/cannot currently record grades or completion for anonymous users. + return None + # In the future when/if we support masquerading, need to be careful here not to affect the user's grades + if event_type == 'grade': + return self.handle_grade_event + elif event_type == 'completion': + if completion_waffle.waffle().is_enabled(completion_waffle.ENABLE_COMPLETION_TRACKING): + return self.handle_completion_event + return None + + def log_event_to_tracking_log(self, block, event_type, event_data): + """ + Log this XBlock event to the tracking log + """ + log_context = track.contexts.context_dict_for_learning_context(block.scope_ids.usage_id.context_key) + if self.user_id: + log_context['user_id'] = self.user_id + log_context['asides'] = {} + track_function = make_track_function() + with tracker.get_tracker().context(event_type, log_context): + track_function(event_type, event_data) + + def handle_grade_event(self, block, event): + """ + Submit a grade for the block. + """ + if not self.user.is_anonymous(): + grades_signals.SCORE_PUBLISHED.send( + sender=None, + block=block, + user=self.user, + raw_earned=event['value'], + raw_possible=event['max_value'], + only_if_higher=event.get('only_if_higher'), + score_deleted=event.get('score_deleted'), + grader_response=event.get('grader_response') + ) + + def handle_completion_event(self, block, event): + """ + Submit a completion object for the block. + """ + block_key = block.scope_ids.usage_id + # edx-completion needs to be updated to support learning contexts, which is coming soon in a separate PR. + # For now just log a debug statement to confirm this plumbing is ready to send those events through. + log.debug("Completion event for block {}: new completion = {}".format(block_key, event['completion'])) + # BlockCompletion.objects.submit_completion( + # user=self.user, + # course_key=block_key.context_key, + # block_key=block_key, + # completion=event['completion'], + # ) def applicable_aside_types(self, block): """ Disable XBlock asides in this runtime """ diff --git a/openedx/core/djangoapps/xblock/runtime/shims.py b/openedx/core/djangoapps/xblock/runtime/shims.py index 062118a9be..6e9189ebd5 100644 --- a/openedx/core/djangoapps/xblock/runtime/shims.py +++ b/openedx/core/djangoapps/xblock/runtime/shims.py @@ -317,6 +317,19 @@ class RuntimeShim(object): result['default_value'] = field.to_json(field.default) return result + def track_function(self, title, event_info): + """ + Publish an event to the tracking log. + + This is deprecated in favor of runtime.publish + See https://git.io/JeGLf and https://git.io/JeGLY for context. + """ + warnings.warn( + "runtime.track_function is deprecated. Use runtime.publish() instead.", + DeprecationWarning, stacklevel=2, + ) + self.publish(self._active_block, title, event_info) + @property def user_location(self): """