Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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) {
Expand All @@ -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
})
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I view a sequential like "Lesson 1 - Getting Started" in the demo course and click from the first vertical to another vertical and then back to the first, I see that it still POSTs a publish_completion event to the server - it has "forgotten" that it already marked the block as complete.

I was going to suggest calling delete block.dataset.completableByViewing; after the AJAX request completes successfully, but I think that each vertical gets rendered from an HTML string when switching verticals in a sequential, so that approach wouldn't work.

If you think it's worth doing, maybe add a global variable to track already-submitted completion events? Or we can ignore the duplicates, as it's not a huge optimization.

SEEN_COMPLETABLES.add(blockKey);
}
}
);
};
1 change: 1 addition & 0 deletions common/lib/xmodule/xmodule/library_content_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ def student_view(self, context):
'items': contents,
'xblock_context': context,
'show_bookmark_button': False,
'watched_completable_blocks': set(),
}))
return fragment

Expand Down
2 changes: 2 additions & 0 deletions common/lib/xmodule/xmodule/seq_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
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
Expand Down Expand Up @@ -40,6 +41,7 @@

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.
Expand Down
110 changes: 101 additions & 9 deletions common/lib/xmodule/xmodule/tests/test_vertical.py
Original file line number Diff line number Diff line change
@@ -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):
Expand All @@ -33,12 +79,15 @@ def setUp(self):
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}

Expand Down Expand Up @@ -66,8 +115,8 @@ def test_render_student_view(self, context):
"""
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
Expand All @@ -76,6 +125,38 @@ def test_render_student_view(self, context):
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):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test could use a docstring - it's not immediately obvious exactly what it's doing.

"""
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
Expand All @@ -97,3 +178,14 @@ def test_render_studio_view(self):
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})
76 changes: 73 additions & 3 deletions common/lib/xmodule/xmodule/vertical_block.py
Original file line number Diff line number Diff line change
@@ -1,28 +1,51 @@
"""
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
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
# OBSOLETE: This obsoletes 'type'
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.
Expand All @@ -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.
Expand Down Expand Up @@ -66,7 +109,7 @@ def student_view(self, context):
fragment.add_frag_resources(rendered_child)

contents.append({
'id': child.location.to_deprecated_string(),
'id': six.text_type(child.location),
'content': rendered_child.content
})

Expand All @@ -76,7 +119,8 @@ def student_view(self, context):
'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'))
Expand Down Expand Up @@ -177,3 +221,29 @@ def index_dictionary(self):
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'}
Loading