From b8202e40de604a9a426934d3dabd1b350dcb5251 Mon Sep 17 00:00:00 2001 From: "J. Cliff Dyer" Date: Tue, 10 Oct 2017 12:14:28 -0400 Subject: [PATCH] Handle default complete-by-viewing completion method. * Vertical marks blocks completed when viewed. * Functionality is hidden behind a waffle switch * Submissions from front-end are limited to known-incomplete blocks * Upgrades xblock to version 1.1.1 * Related future requirements listed in TODO tagged with EDUCATOR-1778 and relevant opencraft OC-* ticket IDs. OC-3088 --- .../public/js/vertical_student_view.js | 39 +++++++ .../xmodule/xmodule/library_content_module.py | 1 + common/lib/xmodule/xmodule/seq_module.py | 2 + .../xmodule/xmodule/tests/test_vertical.py | 110 ++++++++++++++++-- common/lib/xmodule/xmodule/vertical_block.py | 76 +++++++++++- lms/djangoapps/completion/services.py | 61 ++++++++++ lms/djangoapps/completion/test_utils.py | 22 ++++ .../completion/tests/test_handlers.py | 23 +--- .../completion/tests/test_services.py | 74 ++++++++++++ lms/djangoapps/completion/waffle.py | 6 + lms/djangoapps/lms_xblock/runtime.py | 7 +- lms/templates/vert_module.html | 6 +- requirements/edx/base.txt | 2 +- 13 files changed, 393 insertions(+), 36 deletions(-) create mode 100644 lms/djangoapps/completion/services.py create mode 100644 lms/djangoapps/completion/test_utils.py create mode 100644 lms/djangoapps/completion/tests/test_services.py diff --git a/common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js b/common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js index 065480b8b6..b045fee0de 100644 --- a/common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js +++ b/common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js @@ -1,4 +1,15 @@ /* JavaScript for Vertical Student View. */ + +/* global Set:false */ // false means do not assign to Set + +// The vertical marks blocks complete if they are completable by viewing. The +// global variable SEEN_COMPLETABLES tracks blocks between separate loads of +// the same vertical (when a learner goes from one tab to the next, and then +// navigates back within a given sequential) to protect against duplicate calls +// to the server. + +var SEEN_COMPLETABLES = new Set(); + window.VerticalStudentView = function(runtime, element) { 'use strict'; RequireJS.require(['course_bookmarks/js/views/bookmark_button'], function(BookmarkButton) { @@ -13,4 +24,32 @@ window.VerticalStudentView = function(runtime, element) { apiUrl: $bookmarkButtonElement.data('bookmarksApiUrl') }); }); + $(element).find('.vert').each( + function(idx, block) { + var blockKey = block.dataset.id; + + if (block.dataset.completableByViewing === undefined) { + return; + } + // TODO: EDUCATOR-1778 + // * Check if blocks are in the browser's view window or in focus + // before marking complete. This will include a configurable + // delay so that blocks must be seen for a few seconds before + // being marked complete, to prevent completion via rapid + // scrolling. (OC-3358) + // * Limit network traffic by batching and throttling calls. + // (OC-3090) + if (blockKey && !SEEN_COMPLETABLES.has(blockKey)) { + $.ajax({ + type: 'POST', + url: runtime.handlerUrl(element, 'publish_completion'), + data: JSON.stringify({ + block_key: blockKey, + completion: 1.0 + }) + }); + SEEN_COMPLETABLES.add(blockKey); + } + } + ); }; diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index 77d5dde436..8358b2c6af 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -313,6 +313,7 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): 'items': contents, 'xblock_context': context, 'show_bookmark_button': False, + 'watched_completable_blocks': set(), })) return fragment diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 567a6bedb0..e9c03279a0 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -11,6 +11,7 @@ from datetime import datetime from lxml import etree from pkg_resources import resource_string from pytz import UTC +from xblock.completable import XBlockCompletionMode from xblock.core import XBlock from xblock.fields import Boolean, Integer, List, Scope, String from xblock.fragment import Fragment @@ -40,6 +41,7 @@ _ = lambda text: text class SequenceFields(object): has_children = True + completion_mode = XBlockCompletionMode.AGGREGATOR # NOTE: Position is 1-indexed. This is silly, but there are now student # positions saved on prod, so it's not easy to fix. diff --git a/common/lib/xmodule/xmodule/tests/test_vertical.py b/common/lib/xmodule/xmodule/tests/test_vertical.py index 54d4d40ddb..8995849162 100644 --- a/common/lib/xmodule/xmodule/tests/test_vertical.py +++ b/common/lib/xmodule/xmodule/tests/test_vertical.py @@ -1,14 +1,60 @@ """ Tests for vertical module. """ + +# pylint: disable=protected-access +from __future__ import absolute_import, division, print_function, unicode_literals + +from collections import namedtuple +import json + import ddt -from mock import Mock from fs.memoryfs import MemoryFS -from xmodule.tests import get_test_system -from xmodule.tests.helpers import StubUserService -from xmodule.tests.xml import XModuleXmlImportTest -from xmodule.tests.xml import factories as xml -from xmodule.x_module import STUDENT_VIEW, AUTHOR_VIEW +from mock import Mock, patch +import six + +from . import get_test_system +from .helpers import StubUserService +from .xml import XModuleXmlImportTest +from .xml import factories as xml +from ..x_module import STUDENT_VIEW, AUTHOR_VIEW + + +JsonRequest = namedtuple('JsonRequest', ['method', 'body']) + + +def get_json_request(data): + """ + Given a data dictionary, return an appropriate JSON request. + """ + return JsonRequest( + method='POST', + body=json.dumps(data), + ) + + +class StubCompletionService(object): + """ + A stub implementation of the CompletionService for testing without access to django + """ + + def __init__(self, enabled, completion_value): + self._enabled = enabled + self._completion_value = completion_value + + def completion_tracking_enabled(self): + """ + Turn on or off completion tracking for clients of the + StubCompletionService. + """ + return self._enabled + + def get_completions(self, candidates): + """ + Return the (dummy) completion values for each specified candidate + block. + """ + return {candidate: self._completion_value for candidate in candidates} class BaseVerticalBlockTest(XModuleXmlImportTest): @@ -33,12 +79,15 @@ class BaseVerticalBlockTest(XModuleXmlImportTest): course_seq = self.course.get_children()[0] self.module_system = get_test_system() - self.module_system.descriptor_runtime = self.course._runtime # pylint: disable=protected-access + self.module_system.descriptor_runtime = self.course._runtime self.course.runtime.export_fs = MemoryFS() self.vertical = course_seq.get_children()[0] self.vertical.xmodule_runtime = self.module_system + self.html1block = self.vertical.get_children()[0] + self.html2block = self.vertical.get_children()[1] + self.username = "bilbo" self.default_context = {"bookmarked": False, "username": self.username} @@ -66,8 +115,8 @@ class VerticalBlockTestCase(BaseVerticalBlockTest): """ Test the rendering of the student view. """ - self.module_system._services['bookmarks'] = Mock() # pylint: disable=protected-access - self.module_system._services['user'] = StubUserService() # pylint: disable=protected-access + self.module_system._services['bookmarks'] = Mock() + self.module_system._services['user'] = StubUserService() html = self.module_system.render( self.vertical, STUDENT_VIEW, self.default_context if context is None else context @@ -76,6 +125,38 @@ class VerticalBlockTestCase(BaseVerticalBlockTest): self.assertIn(self.test_html_2, html) self.assert_bookmark_info_in(html) + @staticmethod + def _render_completable_blocks(template, context): # pylint: disable=unused-argument + """ + A custom template rendering function that displays the + watched_completable_blocks of the template. + + This is used because the default test renderer is haphazardly + formatted, and is difficult to make assertions about. + """ + return u'|'.join(context['watched_completable_blocks']) + + @ddt.unpack + @ddt.data( + (True, 0.9, 'assertIn'), + (False, 0.9, 'assertNotIn'), + (True, 1.0, 'assertNotIn'), + ) + def test_completion_data_attrs(self, completion_enabled, completion_value, assertion_method): + """ + Test that data-completable-by-viewing attributes are included only when + the completion service is enabled, and only for blocks with a + completion value less than 1.0. + """ + with patch.object(self.module_system, 'render_template', new=self._render_completable_blocks): + self.module_system._services['completion'] = StubCompletionService( + enabled=completion_enabled, + completion_value=completion_value, + ) + response = self.module_system.render(self.vertical, STUDENT_VIEW, self.default_context) + getattr(self, assertion_method)(six.text_type(self.html1block.location), response.content) + getattr(self, assertion_method)(six.text_type(self.html2block.location), response.content) + def test_render_studio_view(self): """ Test the rendering of the Studio author view @@ -97,3 +178,14 @@ class VerticalBlockTestCase(BaseVerticalBlockTest): html = self.module_system.render(self.vertical, AUTHOR_VIEW, context).content self.assertIn(self.test_html_1, html) self.assertIn(self.test_html_2, html) + + def test_publish_completion(self): + request = get_json_request({"block_key": six.text_type(self.html1block.location), "completion": 1.0}) + with patch.object(self.vertical.runtime, 'publish') as mock_publisher: + response = self.vertical.publish_completion(request) + self.assertEqual( + response.status_code, + 200, + "Expected 200, got {}: {}".format(response.status_code, response.body), + ) + mock_publisher.assert_called_with(self.html1block, "completion", {"completion": 1.0}) diff --git a/common/lib/xmodule/xmodule/vertical_block.py b/common/lib/xmodule/xmodule/vertical_block.py index d253a1f67e..86ef5485ab 100644 --- a/common/lib/xmodule/xmodule/vertical_block.py +++ b/common/lib/xmodule/xmodule/vertical_block.py @@ -1,13 +1,21 @@ """ VerticalBlock - an XBlock which renders its children in a column. """ -import logging + +from __future__ import absolute_import, division, print_function, unicode_literals + from copy import copy +import logging from lxml import etree +from opaque_keys.edx.keys import UsageKey +import six +from xblock.completable import XBlockCompletionMode from xblock.core import XBlock +from xblock.exceptions import JsonHandlerError from xblock.fragment import Fragment + from xmodule.mako_module import MakoTemplateBlockBase from xmodule.progress import Progress from xmodule.seq_module import SequenceFields @@ -15,6 +23,7 @@ from xmodule.studio_editable import StudioEditableBlock from xmodule.x_module import STUDENT_VIEW, XModuleFields from xmodule.xml_module import XmlParserMixin + log = logging.getLogger(__name__) # HACK: This shouldn't be hard-coded to two types @@ -22,7 +31,21 @@ log = logging.getLogger(__name__) CLASS_PRIORITY = ['video', 'problem'] +def is_completable_by_viewing(block): + """ + Returns True if the block can by completed by viewing it. + + This is true of any non-customized, non-scorable, completable block. + """ + return ( + getattr(block, 'completion_mode', XBlockCompletionMode.COMPLETABLE) == XBlockCompletionMode.COMPLETABLE + and not getattr(block, 'has_custom_completion', False) + and not block.has_score + ) + + @XBlock.needs('user', 'bookmarks') +@XBlock.wants('completion') class VerticalBlock(SequenceFields, XModuleFields, StudioEditableBlock, XmlParserMixin, MakoTemplateBlockBase, XBlock): """ Layout XBlock for rendering subblocks vertically. @@ -37,6 +60,26 @@ class VerticalBlock(SequenceFields, XModuleFields, StudioEditableBlock, XmlParse show_in_read_only_mode = True + def get_completable_by_viewing(self): + """ + Return a set of descendent blocks that this vertical still needs to + mark complete upon viewing. + + Completed blocks are excluded to reduce network traffic from clients. + """ + completion_service = self.runtime.service(self, 'completion') + if completion_service is None: + return set() + if not completion_service.completion_tracking_enabled(): + return set() + # pylint: disable=no-member + blocks = {block.location for block in self.get_display_items() if is_completable_by_viewing(block)} + # pylint: enable=no-member + + # Exclude completed blocks to reduce traffic from client. + completions = completion_service.get_completions(blocks) + return {six.text_type(block_key) for block_key in blocks if completions[block_key] < 1.0} + def student_view(self, context): """ Renders the student view of the block in the LMS. @@ -66,7 +109,7 @@ class VerticalBlock(SequenceFields, XModuleFields, StudioEditableBlock, XmlParse fragment.add_frag_resources(rendered_child) contents.append({ - 'id': child.location.to_deprecated_string(), + 'id': six.text_type(child.location), 'content': rendered_child.content }) @@ -76,7 +119,8 @@ class VerticalBlock(SequenceFields, XModuleFields, StudioEditableBlock, XmlParse 'unit_title': self.display_name_with_default if not is_child_of_vertical else None, 'show_bookmark_button': child_context.get('show_bookmark_button', not is_child_of_vertical), 'bookmarked': child_context['bookmarked'], - 'bookmark_id': u"{},{}".format(child_context['username'], unicode(self.location)) + 'bookmark_id': u"{},{}".format(child_context['username'], unicode(self.location)), # pylint: disable=no-member + 'watched_completable_blocks': self.get_completable_by_viewing(), })) fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/vertical_student_view.js')) @@ -177,3 +221,29 @@ class VerticalBlock(SequenceFields, XModuleFields, StudioEditableBlock, XmlParse xblock_body["content_type"] = "Sequence" return xblock_body + + def find_descendent(self, block_key): + """ + Return the descendent block with the given block key if it exists. + + Otherwise return None. + """ + for block in self.get_display_items(): # pylint: disable=no-member + if block.location == block_key: + return block + + @XBlock.json_handler + def publish_completion(self, data, suffix=''): # pylint: disable=unused-argument + """ + Publish data from the front end. + """ + block_key = UsageKey.from_string(data.pop('block_key')).map_into_course(self.course_id) + block = self.find_descendent(block_key) + if block is None: + message = "Invalid block: {} not found in {}" + raise JsonHandlerError(400, message.format(block_key, self.location)) # pylint: disable=no-member + elif not is_completable_by_viewing(block): + message = "Invalid block type: {} in block {} not configured for completion by viewing" + raise JsonHandlerError(400, message.format(type(block), block_key)) + self.runtime.publish(block, "completion", data) + return {'result': 'ok'} diff --git a/lms/djangoapps/completion/services.py b/lms/djangoapps/completion/services.py new file mode 100644 index 0000000000..862a2af338 --- /dev/null +++ b/lms/djangoapps/completion/services.py @@ -0,0 +1,61 @@ +""" +Runtime service for communicating completion information to the xblock system. +""" + +from .models import BlockCompletion +from . import waffle + + +class CompletionService(object): + """ + Service for handling completions for a user within a course. + + Exposes + + * self.completion_tracking_enabled() -> bool + * self.get_completions(candidates) + + Constructor takes a user object and course_key as arguments. + """ + def __init__(self, user, course_key): + self._user = user + self._course_key = course_key + + def completion_tracking_enabled(self): + """ + Exposes ENABLE_COMPLETION_TRACKING waffle switch to XModule runtime + + Return value: + + bool -> True if completion tracking is enabled. + """ + return waffle.waffle().is_enabled(waffle.ENABLE_COMPLETION_TRACKING) + + def get_completions(self, candidates): + """ + Given an iterable collection of block_keys in the course, returns a + mapping of the block_keys to the present completion values of their + associated blocks. + + If a completion is not found for a given block in the current course, + 0.0 is returned. The service does not attempt to verify that the block + exists within the course. + + Parameters: + + candidates: collection of BlockKeys within the current course. + + Return value: + + dict[BlockKey] -> float: Mapping blocks to their completion value. + """ + completion_queryset = BlockCompletion.objects.filter( + user=self._user, + course_key=self._course_key, + block_key__in=candidates, + ) + completions = {block.block_key: block.completion for block in completion_queryset} + for candidate in candidates: + if candidate not in completions: + completions[candidate] = 0.0 + return completions diff --git a/lms/djangoapps/completion/test_utils.py b/lms/djangoapps/completion/test_utils.py new file mode 100644 index 0000000000..f0d8cb0039 --- /dev/null +++ b/lms/djangoapps/completion/test_utils.py @@ -0,0 +1,22 @@ +""" +Common functionality to support writing tests around completion. +""" + +from . import waffle + + +class CompletionWaffleTestMixin(object): + """ + Common functionality for completion waffle tests. + """ + def override_waffle_switch(self, override): + """ + Override the setting of the ENABLE_COMPLETION_TRACKING waffle switch + for the course of the test. + + Parameters: + override (bool): True if tracking should be enabled. + """ + _waffle_overrider = waffle.waffle().override(waffle.ENABLE_COMPLETION_TRACKING, override) + _waffle_overrider.__enter__() + self.addCleanup(_waffle_overrider.__exit__, None, None, None) diff --git a/lms/djangoapps/completion/tests/test_handlers.py b/lms/djangoapps/completion/tests/test_handlers.py index b9f393e99c..813a068392 100644 --- a/lms/djangoapps/completion/tests/test_handlers.py +++ b/lms/djangoapps/completion/tests/test_handlers.py @@ -16,28 +16,11 @@ from student.tests.factories import UserFactory from .. import handlers from ..models import BlockCompletion -from .. import waffle - - -class CompletionHandlerMixin(object): - """ - Common functionality for completion handler tests. - """ - def override_waffle_switch(self, override): - """ - Override the setting of the ENABLE_COMPLETION_TRACKING waffle switch - for the course of the test. - - Parameters: - override (bool): True if tracking should be enabled. - """ - _waffle_overrider = waffle.waffle().override(waffle.ENABLE_COMPLETION_TRACKING, override) - _waffle_overrider.__enter__() - self.addCleanup(_waffle_overrider.__exit__, None, None, None) +from ..test_utils import CompletionWaffleTestMixin @ddt.ddt -class ScorableCompletionHandlerTestCase(CompletionHandlerMixin, TestCase): +class ScorableCompletionHandlerTestCase(CompletionWaffleTestMixin, TestCase): """ Test the signal handler """ @@ -89,7 +72,7 @@ class ScorableCompletionHandlerTestCase(CompletionHandlerMixin, TestCase): mock_handler.assert_called() -class DisabledCompletionHandlerTestCase(CompletionHandlerMixin, TestCase): +class DisabledCompletionHandlerTestCase(CompletionWaffleTestMixin, TestCase): """ Test that disabling the ENABLE_COMPLETION_TRACKING waffle switch prevents the signal handler from submitting a completion. diff --git a/lms/djangoapps/completion/tests/test_services.py b/lms/djangoapps/completion/tests/test_services.py new file mode 100644 index 0000000000..61d84df05b --- /dev/null +++ b/lms/djangoapps/completion/tests/test_services.py @@ -0,0 +1,74 @@ +""" +Tests of completion xblock runtime services +""" +import ddt +from django.test import TestCase +from opaque_keys.edx.keys import CourseKey, UsageKey +from student.tests.factories import UserFactory + +from ..models import BlockCompletion +from ..services import CompletionService +from ..test_utils import CompletionWaffleTestMixin + + +@ddt.ddt +class CompletionServiceTestCase(CompletionWaffleTestMixin, TestCase): + """ + Test the data returned by the CompletionService. + """ + + def setUp(self): + super(CompletionServiceTestCase, self).setUp() + self.override_waffle_switch(True) + self.user = UserFactory.create() + self.other_user = UserFactory.create() + self.course_key = CourseKey.from_string("edX/MOOC101/2049_T2") + self.other_course_key = CourseKey.from_string("course-v1:ReedX+Hum110+1904") + self.block_keys = [UsageKey.from_string("i4x://edX/MOOC101/video/{}".format(number)) for number in xrange(5)] + + self.completion_service = CompletionService(self.user, self.course_key) + + # Proper completions for the given runtime + for idx, block_key in enumerate(self.block_keys[0:3]): + BlockCompletion.objects.submit_completion( + user=self.user, + course_key=self.course_key, + block_key=block_key, + completion=1.0 - (0.2 * idx), + ) + + # Wrong user + for idx, block_key in enumerate(self.block_keys[2:]): + BlockCompletion.objects.submit_completion( + user=self.other_user, + course_key=self.course_key, + block_key=block_key, + completion=0.9 - (0.2 * idx), + ) + + # Wrong course + BlockCompletion.objects.submit_completion( + user=self.user, + course_key=self.other_course_key, + block_key=self.block_keys[4], + completion=0.75, + ) + + def test_completion_service(self): + # Only the completions for the user and course specified for the CompletionService + # are returned. Values are returned for all keys provided. + self.assertEqual( + self.completion_service.get_completions(self.block_keys), + { + self.block_keys[0]: 1.0, + self.block_keys[1]: 0.8, + self.block_keys[2]: 0.6, + self.block_keys[3]: 0.0, + self.block_keys[4]: 0.0, + }, + ) + + @ddt.data(True, False) + def test_enabled_honors_waffle_switch(self, enabled): + self.override_waffle_switch(enabled) + self.assertEqual(self.completion_service.completion_tracking_enabled(), enabled) diff --git a/lms/djangoapps/completion/waffle.py b/lms/djangoapps/completion/waffle.py index 7e0fe70fff..d0195042cc 100644 --- a/lms/djangoapps/completion/waffle.py +++ b/lms/djangoapps/completion/waffle.py @@ -10,6 +10,12 @@ from openedx.core.djangoapps.waffle_utils import WaffleSwitchNamespace WAFFLE_NAMESPACE = 'completion' # Switches + +# Full name: completion.enable_completion_tracking +# Indicates whether or not to track completion of individual blocks. Keeping +# this disabled will prevent creation of BlockCompletion objects in the +# database, as well as preventing completion-related network access by certain +# xblocks. ENABLE_COMPLETION_TRACKING = 'enable_completion_tracking' diff --git a/lms/djangoapps/lms_xblock/runtime.py b/lms/djangoapps/lms_xblock/runtime.py index 20aa3a2601..f4be8eebf3 100644 --- a/lms/djangoapps/lms_xblock/runtime.py +++ b/lms/djangoapps/lms_xblock/runtime.py @@ -8,6 +8,7 @@ from django.core.urlresolvers import reverse from badges.service import BadgingService from badges.utils import badges_enabled from lms.djangoapps.lms_xblock.models import XBlockAsidesConfig +from lms.djangoapps.completion.services import CompletionService from openedx.core.djangoapps.user_api.course_tag import api as user_course_tag_api from openedx.core.lib.url_utils import quote_slashes from openedx.core.lib.xblock_utils import xblock_local_resource_url @@ -133,15 +134,17 @@ class LmsModuleSystem(ModuleSystem): # pylint: disable=abstract-method """ def __init__(self, **kwargs): request_cache_dict = RequestCache.get_request_cache().data + store = modulestore() + services = kwargs.setdefault('services', {}) + services['completion'] = CompletionService(user=kwargs.get('user'), course_key=kwargs.get('course_id')) services['fs'] = xblock.reference.plugins.FSService() services['i18n'] = ModuleI18nService - services['library_tools'] = LibraryToolsService(modulestore()) + services['library_tools'] = LibraryToolsService(store) services['partitions'] = PartitionService( course_id=kwargs.get('course_id'), cache=request_cache_dict ) - store = modulestore() services['settings'] = SettingsService() services['user_tags'] = UserTagsService(self) if badges_enabled(): diff --git a/lms/templates/vert_module.html b/lms/templates/vert_module.html index 1ac7a09736..7e7f537148 100644 --- a/lms/templates/vert_module.html +++ b/lms/templates/vert_module.html @@ -10,7 +10,11 @@
% for idx, item in enumerate(items): -
+
${HTML(item['content'])}
% endfor diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index e50df6d19b..2f55d5d4da 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -203,7 +203,7 @@ py2neo==3.1.2 # Support for plugins web-fragments==0.2.2 -xblock==1.0.0 +XBlock==1.1.1 # Redis version redis==2.10.6