Skip to content
Snippets Groups Projects
Unverified Commit d507a5cd authored by David Ormsbee's avatar David Ormsbee Committed by GitHub
Browse files

Merge pull request #23265 from open-craft/guruprasad/BB-112-fix-randomized-content-block-order 

[BB-112] Fix the unpredictable order randomization issue with randomized content blocks
parents 304b000b a7bd0050
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 261 additions and 50 deletions
common/lib/xmodule/xmodule/library_content_module.py
+ 21
− 22
View file @ d507a5cd
......@@ -158,37 +158,40 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule):
"""
rand = random.Random()
selected = set(tuple(k) for k in selected) # set of (block_type, block_id) tuples assigned to this student
selected_keys = set(tuple(k) for k in selected) # set of (block_type, block_id) tuples assigned to this student
# Determine which of our children we will show:
valid_block_keys = set([(c.block_type, c.block_id) for c in children])
# Remove any selected blocks that are no longer valid:
invalid_block_keys = (selected - valid_block_keys)
invalid_block_keys = (selected_keys - valid_block_keys)
if invalid_block_keys:
selected -= invalid_block_keys
selected_keys -= invalid_block_keys
# If max_count has been decreased, we may have to drop some previously selected blocks:
overlimit_block_keys = set()
if len(selected) > max_count:
num_to_remove = len(selected) - max_count
overlimit_block_keys = set(rand.sample(selected, num_to_remove))
selected -= overlimit_block_keys
if len(selected_keys) > max_count:
num_to_remove = len(selected_keys) - max_count
overlimit_block_keys = set(rand.sample(selected_keys, num_to_remove))
selected_keys -= overlimit_block_keys
# Do we have enough blocks now?
num_to_add = max_count - len(selected)
num_to_add = max_count - len(selected_keys)
added_block_keys = None
if num_to_add > 0:
# We need to select [more] blocks to display to this user:
pool = valid_block_keys - selected
pool = valid_block_keys - selected_keys
if mode == "random":
num_to_add = min(len(pool), num_to_add)
added_block_keys = set(rand.sample(pool, num_to_add))
# We now have the correct n random children to show for this user.
else:
raise NotImplementedError("Unsupported mode.")
selected |= added_block_keys
selected_keys |= added_block_keys
if any([invalid_block_keys, overlimit_block_keys, added_block_keys]):
selected = selected_keys
return {
'selected': selected,
......@@ -268,19 +271,15 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule):
def selected_children(self):
"""
Returns a set() of block_ids indicating which of the possible children
Returns a list() of block_ids indicating which of the possible children
have been selected to display to the current user.
This reads and updates the "selected" field, which has user_state scope.
Note: self.selected and the return value contain block_ids. To get
Note: the return value (self.selected) contains block_ids. To get
actual BlockUsageLocators, it is necessary to use self.children,
because the block_ids alone do not specify the block type.
"""
if hasattr(self, "_selected_set"):
# Already done:
return self._selected_set # pylint: disable=access-member-before-definition
block_keys = self.make_selection(self.selected, self.children, self.max_count, "random") # pylint: disable=no-member
# Publish events for analytics purposes:
......@@ -292,13 +291,13 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule):
self._publish_event,
)
# Save our selections to the user state, to ensure consistency:
selected = block_keys['selected']
self.selected = list(selected) # TODO: this doesn't save from the LMS "Progress" page.
# Cache the results
self._selected_set = selected # pylint: disable=attribute-defined-outside-init
if any(block_keys[changed] for changed in ('invalid', 'overlimit', 'added')):
# Save our selections to the user state, to ensure consistency:
selected = list(block_keys['selected'])
random.shuffle(selected)
self.selected = selected # TODO: this doesn't save from the LMS "Progress" page.
return selected
return self.selected
def _get_selected_child_blocks(self):
"""
......
common/lib/xmodule/xmodule/tests/test_library_content.py
+ 5
− 14
View file @ d507a5cd
......@@ -272,9 +272,8 @@ class LibraryContentModuleTestMixin(object):
Helper method that changes the max_count of self.lc_block, refreshes
children, and asserts that the number of selected children equals the count provided.
"""
# Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access
if hasattr(self.lc_block._xmodule, '_selected_set'):
del self.lc_block._xmodule._selected_set
# Construct the XModule for the descriptor, if not present already.
self.lc_block._xmodule # pylint: disable=pointless-statement,protected-access
self.lc_block.max_count = count
selected = self.lc_block.get_child_descriptors()
self.assertEqual(len(selected), count)
......@@ -287,7 +286,7 @@ class TestLibraryContentModuleNoSearchIndex(LibraryContentModuleTestMixin, Libra
Tests for library container when no search index is available.
Tests fallback low-level CAPA problem introspection
"""
pass
pass # pylint: disable=unnecessary-pass
search_index_mock = Mock(spec=SearchEngine) # pylint: disable=invalid-name
......@@ -365,8 +364,8 @@ class TestLibraryContentAnalytics(LibraryContentTest):
Check that a LibraryContentModule analytics event was published by self.lc_block.
"""
self.assertTrue(self.publisher.called)
self.assertTrue(len(self.publisher.call_args[0]), 3)
_, event_name, event_data = self.publisher.call_args[0]
self.assertTrue(len(self.publisher.call_args[0]), 3) # pylint: disable=unsubscriptable-object
_, event_name, event_data = self.publisher.call_args[0] # pylint: disable=unsubscriptable-object
self.assertEqual(event_name, "edx.librarycontentblock.content.{}".format(event_type))
self.assertEqual(event_data["location"], six.text_type(self.lc_block.location))
return event_data
......@@ -397,8 +396,6 @@ class TestLibraryContentAnalytics(LibraryContentTest):
# Now increase max_count so that one more child will be added:
self.lc_block.max_count = 2
# Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access
del self.lc_block._xmodule._selected_set
children = self.lc_block.get_child_descriptors()
self.assertEqual(len(children), 2)
child, new_child = children if children[0].location == child.location else reversed(children)
......@@ -478,8 +475,6 @@ class TestLibraryContentAnalytics(LibraryContentTest):
self.lc_block.get_child_descriptors() # This line is needed in the test environment or the change has no effect
self.publisher.reset_mock() # Clear the "assigned" event that was just published.
self.lc_block.max_count = 0
# Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access
del self.lc_block._xmodule._selected_set
# Check that the event says that one block was removed, leaving no blocks left:
children = self.lc_block.get_child_descriptors()
......@@ -497,8 +492,6 @@ class TestLibraryContentAnalytics(LibraryContentTest):
# Start by assigning two blocks to the student:
self.lc_block.get_child_descriptors() # This line is needed in the test environment or the change has no effect
self.lc_block.max_count = 2
# Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access
del self.lc_block._xmodule._selected_set
initial_blocks_assigned = self.lc_block.get_child_descriptors()
self.assertEqual(len(initial_blocks_assigned), 2)
self.publisher.reset_mock() # Clear the "assigned" event that was just published.
......@@ -512,8 +505,6 @@ class TestLibraryContentAnalytics(LibraryContentTest):
self.library.children = [keep_block_lib_usage_key]
self.store.update_item(self.library, self.user_id)
self.lc_block.refresh_children()
# Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access
del self.lc_block._xmodule._selected_set
# Check that the event says that one block was removed, leaving one block left:
children = self.lc_block.get_child_descriptors()
......
lms/djangoapps/course_blocks/api.py
+ 1
− 0
View file @ d507a5cd
......@@ -40,6 +40,7 @@ def get_course_block_access_transformers(user):
"""
course_block_access_transformers = [
library_content.ContentLibraryTransformer(),
library_content.ContentLibraryOrderTransformer(),
start_date.StartDateTransformer(),
ContentTypeGateTransformer(),
user_partitions.UserPartitionTransformer(),
......
lms/djangoapps/course_blocks/transformers/library_content.py
+ 104
− 13
View file @ d507a5cd
......@@ -4,6 +4,8 @@ Content Library Transformer.
import json
import logging
import random
import six
from eventtracking import tracker
......@@ -19,6 +21,8 @@ from xmodule.modulestore.django import modulestore
from ..utils import get_student_module_as_dict
logger = logging.getLogger(__name__)
class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransformer):
"""
......@@ -40,17 +44,8 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo
return "library_content"
@classmethod
def collect(cls, block_structure):
"""
Collects any information that's necessary to execute this
transformer's transform method.
"""
block_structure.request_xblock_fields('mode')
block_structure.request_xblock_fields('max_count')
block_structure.request_xblock_fields('category')
store = modulestore()
# needed for analytics purposes
def set_block_analytics_summary(cls, block_structure, store):
"""Set the block analytics summary information in the children fields."""
def summarize_block(usage_key):
""" Basic information about the given block """
orig_key, orig_version = store.get_block_original_usage(usage_key)
......@@ -71,6 +66,20 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo
summary = summarize_block(child_key)
block_structure.set_transformer_block_field(child_key, cls, 'block_analytics_summary', summary)
@classmethod
def collect(cls, block_structure):
"""
Collects any information that's necessary to execute this
transformer's transform method.
"""
block_structure.request_xblock_fields('mode')
block_structure.request_xblock_fields('max_count')
block_structure.request_xblock_fields('category')
store = modulestore()
# needed for analytics purposes
cls.set_block_analytics_summary(block_structure, store)
def transform_block_filters(self, usage_info, block_structure):
all_library_children = set()
all_selected_children = set()
......@@ -102,6 +111,7 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo
# Save back any changes
if any(block_keys[changed] for changed in ('invalid', 'overlimit', 'added')):
state_dict['selected'] = list(selected)
random.shuffle(state_dict['selected'])
StudentModule.save_state(
student=usage_info.user,
course_id=usage_info.course_key,
......@@ -112,7 +122,7 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo
)
# publish events for analytics
self._publish_events(
self.publish_events(
block_structure,
block_key,
previous_count,
......@@ -137,7 +147,8 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo
return [block_structure.create_removal_filter(check_child_removal)]
def _publish_events(self, block_structure, location, previous_count, max_count, block_keys, user_id):
@staticmethod
def publish_events(block_structure, location, previous_count, max_count, block_keys, user_id):
"""
Helper method to publish events for analytics purposes
"""
......@@ -177,3 +188,83 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo
format_block_keys,
publish_event,
)
class ContentLibraryOrderTransformer(BlockStructureTransformer):
"""
A transformer that manipulates the block structure by modifying the order of the
selected blocks within a library_content module to match the order of the selections
made by the ContentLibraryTransformer or the corresponding XBlock. So this transformer
requires the selections for the randomized content block to be already
made either by the ContentLibraryTransformer or the XBlock.
Staff users are *not* exempted from library content pathways/
"""
WRITE_VERSION = 1
READ_VERSION = 1
@classmethod
def name(cls):
"""
Unique identifier for the transformer's class;
same identifier used in setup.py
"""
return "library_content_randomize"
@classmethod
def collect(cls, block_structure):
"""
Collects any information that's necessary to execute this
transformer's transform method.
"""
block_structure.request_xblock_fields('mode')
block_structure.request_xblock_fields('max_count')
ContentLibraryTransformer.set_block_analytics_summary(block_structure, modulestore())
def transform(self, usage_info, block_structure):
"""
Transforms the order of the children of the randomized content block
to match the order of the selections made and stored in the XBlock 'selected' field.
"""
for block_key in block_structure:
if block_key.block_type != 'library_content':
continue
library_children = block_structure.get_children(block_key)
if library_children:
state_dict = get_student_module_as_dict(usage_info.user, usage_info.course_key, block_key)
current_children_blocks = set(block.block_id for block in library_children)
current_selected_blocks = set(item[1] for item in state_dict['selected'])
# Ensure that the current children blocks are the same as the stored selections
# before ordering the blocks.
if current_children_blocks != current_selected_blocks:
logger.info(
u'Mismatch between the children of %s in the stored state and the actual children for user %s',
str(block_key),
usage_info.user.username
)
previous_count = len(current_selected_blocks.intersection(current_children_blocks))
mode = block_structure.get_xblock_field(block_key, 'mode')
max_count = block_structure.get_xblock_field(block_key, 'max_count')
block_keys = LibraryContentModule.make_selection(
state_dict['selected'], library_children, max_count, mode
)
state_dict['selected'] = block_keys['selected']
random.shuffle(state_dict['selected'])
StudentModule.save_state(
student=usage_info.user,
course_id=usage_info.course_key,
module_state_key=block_key,
defaults={
'state': json.dumps(state_dict)
},
)
ContentLibraryTransformer.publish_events(
block_structure, block_key, previous_count, max_count, block_keys, usage_info.user.id
)
ordering_data = {block[1]: position for position, block in enumerate(state_dict['selected'])}
library_children.sort(key=lambda block, data=ordering_data: data[block.block_id])
lms/djangoapps/course_blocks/transformers/tests/test_library_content.py
+ 129
− 1
View file @ d507a5cd
......@@ -4,13 +4,14 @@ Tests for ContentLibraryTransformer.
from six.moves import range
import mock
from openedx.core.djangoapps.content.block_structure.api import clear_course_from_cache
from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers
from student.tests.factories import CourseEnrollmentFactory
from ...api import get_course_blocks
from ..library_content import ContentLibraryTransformer
from ..library_content import ContentLibraryTransformer, ContentLibraryOrderTransformer
from .helpers import CourseStructureTestCase
......@@ -167,3 +168,130 @@ class ContentLibraryTransformerTestCase(CourseStructureTestCase):
),
u"Expected 'selected' equality failed in iteration {}.".format(i)
)
class ContentLibraryOrderTransformerTestCase(CourseStructureTestCase):
"""
ContentLibraryOrderTransformer Test
"""
TRANSFORMER_CLASS_TO_TEST = ContentLibraryOrderTransformer
def setUp(self):
"""
Setup course structure and create user for content library order transformer test.
"""
super(ContentLibraryOrderTransformerTestCase, self).setUp()
self.course_hierarchy = self.get_course_hierarchy()
self.blocks = self.build_course(self.course_hierarchy)
self.course = self.blocks['course']
clear_course_from_cache(self.course.id)
# Enroll user in course.
CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id, is_active=True)
def get_course_hierarchy(self):
"""
Get a course hierarchy to test with.
"""
return [{
'org': 'ContentLibraryTransformer',
'course': 'CL101F',
'run': 'test_run',
'#type': 'course',
'#ref': 'course',
'#children': [
{
'#type': 'chapter',
'#ref': 'chapter1',
'#children': [
{
'#type': 'sequential',
'#ref': 'lesson1',
'#children': [
{
'#type': 'vertical',
'#ref': 'vertical1',
'#children': [
{
'metadata': {'category': 'library_content'},
'#type': 'library_content',
'#ref': 'library_content1',
'#children': [
{
'metadata': {'display_name': "CL Vertical 2"},
'#type': 'vertical',
'#ref': 'vertical2',
'#children': [
{
'metadata': {'display_name': "HTML1"},
'#type': 'html',
'#ref': 'html1',
}
]
},
{
'metadata': {'display_name': "CL Vertical 3"},
'#type': 'vertical',
'#ref': 'vertical3',
'#children': [
{
'metadata': {'display_name': "HTML2"},
'#type': 'html',
'#ref': 'html2',
}
]
},
{
'metadata': {'display_name': "CL Vertical 4"},
'#type': 'vertical',
'#ref': 'vertical4',
'#children': [
{
'metadata': {'display_name': "HTML3"},
'#type': 'html',
'#ref': 'html3',
}
]
}
]
}
],
}
],
}
],
}
]
}]
@mock.patch('lms.djangoapps.course_blocks.transformers.library_content.get_student_module_as_dict')
def test_content_library_randomize(self, mocked):
"""
Test whether the order of the children blocks matches the order of the selected blocks when
course has content library section
"""
mocked.return_value = {
'selected': [
['vertical', 'vertical_vertical3'],
['vertical', 'vertical_vertical2'],
['vertical', 'vertical_vertical4'],
]
}
for i in range(5):
trans_block_structure = get_course_blocks(
self.user,
self.course.location,
self.transformers,
)
children = []
for block_key in trans_block_structure.topological_traversal():
if block_key.block_type == 'library_content':
children = trans_block_structure.get_children(block_key)
break
expected_children = ['vertical_vertical3', 'vertical_vertical2', 'vertical_vertical4']
self.assertEqual(
expected_children,
[child.block_id for child in children],
u"Expected 'selected' equality failed in iteration {}.".format(i)
)
setup.py
+ 1
− 0
View file @ d507a5cd
......@@ -53,6 +53,7 @@ setup(
],
"openedx.block_structure_transformer": [
"library_content = lms.djangoapps.course_blocks.transformers.library_content:ContentLibraryTransformer",
"library_content_randomize = lms.djangoapps.course_blocks.transformers.library_content:ContentLibraryOrderTransformer",
"split_test = lms.djangoapps.course_blocks.transformers.split_test:SplitTestTransformer",
"start_date = lms.djangoapps.course_blocks.transformers.start_date:StartDateTransformer",
"user_partitions = lms.djangoapps.course_blocks.transformers.user_partitions:UserPartitionTransformer",
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment