Skip to content

Commit

Permalink
Revert "Fix the unpredictable order randomization issue with randomiz…
Browse files Browse the repository at this point in the history 
…ed content blocks"
  • Loading branch information
David Ormsbee authored May 8, 2019
1 parent 47292e5 commit d2af1d4
Show file tree
Hide file tree
Showing 6 changed files with 34 additions and 201 deletions.
43 changes: 22 additions & 21 deletions common/lib/xmodule/xmodule/library_content_module.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,40 +154,37 @@ def make_selection(cls, selected, children, max_count, mode):
"""
rand = random.Random()

selected_keys = set(tuple(k) for k in selected) # set of (block_type, block_id) tuples assigned to this student
selected = 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_keys - valid_block_keys)
invalid_block_keys = (selected - valid_block_keys)
if invalid_block_keys:
selected_keys -= invalid_block_keys
selected -= 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_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
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

# Do we have enough blocks now?
num_to_add = max_count - len(selected_keys)
num_to_add = max_count - len(selected)

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_keys
pool = valid_block_keys - selected
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_keys |= added_block_keys

if any([invalid_block_keys, overlimit_block_keys, added_block_keys]):
selected = selected_keys
selected |= added_block_keys

return {
'selected': selected,
Expand Down Expand Up @@ -267,15 +264,19 @@ def publish_selected_children_events(cls, block_keys, format_block_keys, publish

def selected_children(self):
"""
Returns a list() of block_ids indicating which of the possible children
Returns a set() 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: the return value (self.selected) contains block_ids. To get
Note: self.selected and the return value contain 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:
Expand All @@ -287,13 +288,13 @@ def selected_children(self):
self._publish_event,
)

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.
# 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

return self.selected
return selected

def _get_selected_child_blocks(self):
"""
Expand Down
13 changes: 11 additions & 2 deletions common/lib/xmodule/xmodule/tests/test_library_content.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,9 @@ def _change_count_and_refresh_children(self, count):
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.
"""
# Construct the XModule for the descriptor, if not present already present pylint: disable=protected-access
self.lc_block._xmodule
# 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
self.lc_block.max_count = count
selected = self.lc_block.get_child_descriptors()
self.assertEqual(len(selected), count)
Expand Down Expand Up @@ -393,6 +394,8 @@ def test_assigned_event(self):

# 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)
Expand Down Expand Up @@ -472,6 +475,8 @@ def test_removed_overlimit(self):
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()
Expand All @@ -489,6 +494,8 @@ def test_removed_invalid(self):
# 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.
Expand All @@ -502,6 +509,8 @@ def test_removed_invalid(self):
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()
Expand Down
1 change: 0 additions & 1 deletion lms/djangoapps/course_blocks/api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ def get_course_block_access_transformers(user):
"""
course_block_access_transformers = [
library_content.ContentLibraryTransformer(),
library_content.ContentLibraryOrderTransformer(),
start_date.StartDateTransformer(),
ContentTypeGateTransformer(),
user_partitions.UserPartitionTransformer(),
Expand Down
47 changes: 0 additions & 47 deletions lms/djangoapps/course_blocks/transformers/library_content.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
Content Library Transformer.
"""
import json
import random

from eventtracking import tracker

Expand Down Expand Up @@ -100,7 +99,6 @@ def transform_block_filters(self, usage_info, block_structure):
# 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,
Expand Down Expand Up @@ -178,48 +176,3 @@ def publish_event(event_name, result, **kwargs):
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.
"""
# There is nothing to collect.
pass

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

state_dict = get_student_module_as_dict(usage_info.user, usage_info.course_key, block_key)
library_children = block_structure.get_children(block_key)
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])
130 changes: 1 addition & 129 deletions lms/djangoapps/course_blocks/transformers/tests/test_library_content.py
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
"""
Tests for ContentLibraryTransformer.
"""
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, ContentLibraryOrderTransformer
from ..library_content import ContentLibraryTransformer
from .helpers import CourseStructureTestCase


Expand Down Expand Up @@ -181,130 +180,3 @@ def test_staff_access_to_library_content(self):
transformers=self.transformers
)
self.assertEqual(len(list(transformed_blocks.get_block_keys())), len(self.blocks))


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)
)
1 change: 0 additions & 1 deletion setup.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
],
"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",
Expand Down

0 comments on commit d2af1d4

Please sign in to comment.