Skip to content

E
edx-platform
Commit 2021ba63 by Nimisha Asthagiri
Options

fixup! user_partitions unit tests, DAG errors, etc.

parent 2d64c54c
Showing with 191 additions and 102 deletions
lms/djangoapps/course_blocks/transformers/start_date.py
...@@ -3,6 +3,9 @@ ...@@ -3,6 +3,9 @@
""" """
from openedx.core.lib.block_cache.transformer import BlockStructureTransformer from openedx.core.lib.block_cache.transformer import BlockStructureTransformer
from courseware.access_utils import check_start_date from courseware.access_utils import check_start_date
from xmodule.course_metadata_utils import DEFAULT_START_DATE
from .utils import get_field_on_block
class StartDateTransformer(BlockStructureTransformer): class StartDateTransformer(BlockStructureTransformer):
...@@ -36,13 +39,21 @@ class StartDateTransformer(BlockStructureTransformer): ...@@ -36,13 +39,21 @@ class StartDateTransformer(BlockStructureTransformer):
) if parents else None ) if parents else None
# set the merged value for this block # set the merged value for this block
block_start = block_structure.get_xblock(block_key).start block_start = get_field_on_block(block_structure.get_xblock(block_key), 'start')
if min_all_parents_start_date is None:
# no parents so just use value on block or default
merged_start_value = block_start or DEFAULT_START_DATE
elif not block_start:
# no value on this block so take value from parents
merged_start_value = min_all_parents_start_date
else:
# max of merged-start-from-all-parents and this block
merged_start_value = max(min_all_parents_start_date, block_start)
block_structure.set_transformer_block_data( block_structure.set_transformer_block_data(
block_key, block_key,
cls, cls,
cls.MERGED_START_DATE, cls.MERGED_START_DATE,
# max of merged-start-from-all-parents and this block merged_start_value
max(min_all_parents_start_date or block_start, block_start)
) )
def transform(self, user_info, block_structure): def transform(self, user_info, block_structure):
......
lms/djangoapps/course_blocks/transformers/tests/test_library_content.py
...@@ -47,10 +47,11 @@ class ContentLibraryTransformerTestCase(CourseStructureTestCase): ...@@ -47,10 +47,11 @@ class ContentLibraryTransformerTestCase(CourseStructureTestCase):
""" """
Get a course hierarchy to test with. Get a course hierarchy to test with.
""" """
return { return [{
'org': 'ContentLibraryTransformer', 'org': 'ContentLibraryTransformer',
'course': 'CL101F', 'course': 'CL101F',
'run': 'test_run', 'run': 'test_run',
'#type': 'course',
'#ref': 'course', '#ref': 'course',
'#children': [ '#children': [
{ {
...@@ -103,7 +104,7 @@ class ContentLibraryTransformerTestCase(CourseStructureTestCase): ...@@ -103,7 +104,7 @@ class ContentLibraryTransformerTestCase(CourseStructureTestCase):
], ],
} }
] ]
} }]
def test_course_structure_with_user_course_library(self): def test_course_structure_with_user_course_library(self):
""" """
...@@ -128,7 +129,7 @@ class ContentLibraryTransformerTestCase(CourseStructureTestCase): ...@@ -128,7 +129,7 @@ class ContentLibraryTransformerTestCase(CourseStructureTestCase):
self.assertEqual( self.assertEqual(
set(trans_block_structure.get_block_keys()), set(trans_block_structure.get_block_keys()),
self.get_block_key_set('course', 'chapter1', 'lesson1', 'vertical1', 'library_content1') self.get_block_key_set(self.blocks, 'course', 'chapter1', 'lesson1', 'vertical1', 'library_content1')
) )
# Check course structure again, with mocked selected modules for a user. # Check course structure again, with mocked selected modules for a user.
...@@ -145,6 +146,7 @@ class ContentLibraryTransformerTestCase(CourseStructureTestCase): ...@@ -145,6 +146,7 @@ class ContentLibraryTransformerTestCase(CourseStructureTestCase):
self.assertEqual( self.assertEqual(
set(trans_block_structure.get_block_keys()), set(trans_block_structure.get_block_keys()),
self.get_block_key_set( self.get_block_key_set(
self.blocks,
'course', 'course',
'chapter1', 'chapter1',
'lesson1', 'lesson1',
...@@ -156,4 +158,4 @@ class ContentLibraryTransformerTestCase(CourseStructureTestCase): ...@@ -156,4 +158,4 @@ class ContentLibraryTransformerTestCase(CourseStructureTestCase):
) )
def test_staff_user(self): def test_staff_user(self):
self.assert_staff_access_to_all_blocks(self.staff, self.course, self.blocks, self.transformer) self.assert_staff_access_to_all_blocks(self.course, self.blocks, self.transformer)
lms/djangoapps/course_blocks/transformers/tests/test_split_test.py
...@@ -51,11 +51,12 @@ class SplitTestTransformerTestCase(CourseStructureTestCase): ...@@ -51,11 +51,12 @@ class SplitTestTransformerTestCase(CourseStructureTestCase):
Returns: dict[course_structure] Returns: dict[course_structure]
""" """
return { return [{
'org': 'SplitTestTransformer', 'org': 'SplitTestTransformer',
'course': 'ST101F', 'course': 'ST101F',
'run': 'test_run', 'run': 'test_run',
'user_partitions': [self.split_test_user_partition], 'user_partitions': [self.split_test_user_partition],
'#type': 'course',
'#ref': 'course', '#ref': 'course',
'#children': [ '#children': [
{ {
...@@ -113,15 +114,9 @@ class SplitTestTransformerTestCase(CourseStructureTestCase): ...@@ -113,15 +114,9 @@ class SplitTestTransformerTestCase(CourseStructureTestCase):
], ],
} }
] ]
} }]
def test_user(self): def test_user(self):
trans_block_structure = get_course_blocks(
self.user,
self.course.location,
transformers={self.transformer},
)
# user was randomly assigned to one of the groups # user was randomly assigned to one of the groups
user_groups = get_user_partition_groups( user_groups = get_user_partition_groups(
self.course.id, [self.split_test_user_partition], self.user self.course.id, [self.split_test_user_partition], self.user
...@@ -129,19 +124,18 @@ class SplitTestTransformerTestCase(CourseStructureTestCase): ...@@ -129,19 +124,18 @@ class SplitTestTransformerTestCase(CourseStructureTestCase):
self.assertEquals(len(user_groups), 1) self.assertEquals(len(user_groups), 1)
group = user_groups[self.split_test_user_partition_id] group = user_groups[self.split_test_user_partition_id]
# determine expected blocks
expected_blocks = ['course', 'chapter1', 'lesson1', 'vertical1'] expected_blocks = ['course', 'chapter1', 'lesson1', 'vertical1']
if group.id == 3: expected_blocks += (['vertical2', 'html1'] if group.id == 3 else ['vertical3', 'html2'])
expected_blocks += ['vertical2', 'html1']
else:
expected_blocks += ['vertical3', 'html2']
self.assertEqual(set(trans_block_structure.get_block_keys()), set(self.get_block_key_set(*expected_blocks)))
# calling again should result in the same block set
reloaded_structure = get_course_blocks(
self.user,
self.course.location,
transformers={self.transformer}
)
self.assertEqual(set(reloaded_structure.get_block_keys()), set(self.get_block_key_set(*expected_blocks)))
# calling twice should result in the same block set
for _ in range(2):
trans_block_structure = get_course_blocks(
self.user,
self.course.location,
transformers={self.transformer},
)
self.assertEqual(
set(trans_block_structure.get_block_keys()),
set(self.get_block_key_set(self.blocks, *expected_blocks))
)
lms/djangoapps/course_blocks/transformers/tests/test_start_date.py
...@@ -7,10 +7,8 @@ from datetime import timedelta ...@@ -7,10 +7,8 @@ from datetime import timedelta
from django.utils.timezone import now from django.utils.timezone import now
from mock import patch from mock import patch
from xmodule.course_metadata_utils import DEFAULT_START_DATE from ..start_date import StartDateTransformer, DEFAULT_START_DATE
from .test_helpers import BlockParentsMapTestCase, update_block
from ..start_date import StartDateTransformer
from .test_helpers import BlockParentsMapTestCase
@ddt.ddt @ddt.ddt
...@@ -43,10 +41,19 @@ class StartDateTransformerTestCase(BlockParentsMapTestCase): ...@@ -43,10 +41,19 @@ class StartDateTransformerTestCase(BlockParentsMapTestCase):
@patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False}) @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False})
@ddt.data( @ddt.data(
({}, {}, {}), ({}, {}, {}),
({0: StartDateType.default}, {}, {}),
({0: StartDateType.future}, {}, {}),
({0: StartDateType.released}, {0, 1, 2, 3, 4, 5, 6}, {}), ({0: StartDateType.released}, {0, 1, 2, 3, 4, 5, 6}, {}),
# has_access checks on block directly and doesn't follow negative access set on parent/ancestor (i.e., 0)
({1: StartDateType.released}, {}, {1, 3, 4, 6}), ({1: StartDateType.released}, {}, {1, 3, 4, 6}),
({2: StartDateType.released}, {}, {2, 5, 6}),
({1: StartDateType.released, 2: StartDateType.released}, {}, {1, 2, 3, 4, 5, 6}), ({1: StartDateType.released, 2: StartDateType.released}, {}, {1, 2, 3, 4, 5, 6}),
({0: StartDateType.released, 4: StartDateType.future}, {0, 1, 2, 3, 5}, {}),
# DAG conflicts: has_access relies on field inheritance so it takes only the value from the first parent-chain
({0: StartDateType.released, 4: StartDateType.future}, {0, 1, 2, 3, 5, 6}, {6}),
({0: StartDateType.released, 2: StartDateType.released, 4: StartDateType.future}, {0, 1, 2, 3, 5, 6}, {6}),
({0: StartDateType.released, 2: StartDateType.future, 4: StartDateType.released}, {0, 1, 3, 4, 6}, {}),
) )
@ddt.unpack @ddt.unpack
def test_block_start_date( def test_block_start_date(
...@@ -55,7 +62,7 @@ class StartDateTransformerTestCase(BlockParentsMapTestCase): ...@@ -55,7 +62,7 @@ class StartDateTransformerTestCase(BlockParentsMapTestCase):
for i, start_date_type in start_date_type_values.iteritems(): for i, start_date_type in start_date_type_values.iteritems():
block = self.get_block(i) block = self.get_block(i)
block.start = self.StartDateType.start(start_date_type) block.start = self.StartDateType.start(start_date_type)
self.update_block(block) update_block(block)
self.check_transformer_results( self.check_transformer_results(
expected_student_visible_blocks, blocks_with_differing_student_access, [StartDateTransformer()] expected_student_visible_blocks, blocks_with_differing_student_access, [StartDateTransformer()]
......
lms/djangoapps/course_blocks/transformers/tests/test_user_partitions.py
...@@ -18,6 +18,8 @@ class UserPartitionTransformerTestCase(CourseStructureTestCase): ...@@ -18,6 +18,8 @@ class UserPartitionTransformerTestCase(CourseStructureTestCase):
""" """
UserPartitionTransformer Test UserPartitionTransformer Test
""" """
TEST_PARTITION_ID = 0
def setUp(self): def setUp(self):
""" """
Setup course structure and create user for user partition transformer test. Setup course structure and create user for user partition transformer test.
...@@ -25,10 +27,9 @@ class UserPartitionTransformerTestCase(CourseStructureTestCase): ...@@ -25,10 +27,9 @@ class UserPartitionTransformerTestCase(CourseStructureTestCase):
super(UserPartitionTransformerTestCase, self).setUp() super(UserPartitionTransformerTestCase, self).setUp()
# Set up user partitions and groups. # Set up user partitions and groups.
self.groups = [Group(1, 'Group 1'), Group(2, 'Group 2')] self.groups = [Group(1, 'Group 1'), Group(2, 'Group 2'), Group(3, 'Group 3'), Group(4, 'Group 4')]
self.content_groups = [1, 2]
self.user_partition = UserPartition( self.user_partition = UserPartition(
id=0, id=self.TEST_PARTITION_ID,
name='Partition 1', name='Partition 1',
description='This is partition 1', description='This is partition 1',
groups=self.groups, groups=self.groups,
...@@ -46,8 +47,18 @@ class UserPartitionTransformerTestCase(CourseStructureTestCase): ...@@ -46,8 +47,18 @@ class UserPartitionTransformerTestCase(CourseStructureTestCase):
# Set up cohorts. # Set up cohorts.
config_course_cohorts(self.course, is_cohorted=True) config_course_cohorts(self.course, is_cohorted=True)
self.cohorts = [CohortFactory(course_id=self.course.id) for __ in enumerate(self.groups)] self.cohorts = []
self.add_user_to_cohort_group(self.cohorts[0], self.groups[0]) for group in self.groups:
cohort = CohortFactory(course_id=self.course.id)
self.cohorts.append(cohort)
link_cohort_to_partition_group(
cohort,
self.user_partition.id,
group.id,
)
add_user_to_cohort(self.cohorts[0], self.user.username)
self.transformer = UserPartitionTransformer() self.transformer = UserPartitionTransformer()
def get_course_hierarchy(self): def get_course_hierarchy(self):
...@@ -56,62 +67,104 @@ class UserPartitionTransformerTestCase(CourseStructureTestCase): ...@@ -56,62 +67,104 @@ class UserPartitionTransformerTestCase(CourseStructureTestCase):
Assumes self.user_partition has already been initialized. Assumes self.user_partition has already been initialized.
""" """
return { # course
'org': 'UserPartitionTransformer', # / \
'course': 'UP101F', # / \
'run': 'test_run', # A[1, 2, 3] B
'user_partitions': [self.user_partition], # / | \ |
'#ref': 'course', # / | \ |
'#children': [ # / | \ |
{ # C[1, 2] D[2, 3] E /
'#type': 'chapter', # / | \ | / \ /
'#ref': 'chapter1', # / | \ | / \ /
'#children': [ # / | \ | / \ /
{ # F G[1] H[2] I J K[4] /
'metadata': { # / \ / / \ /
'group_access': {0: [0, 1, 2]}, # / \ / / \ /
}, # / \ / / \/
'#type': 'sequential', # L[1, 2] M[1, 2, 3] N O
'#ref': 'lesson1', #
'#children': [ return [
{ {
'#type': 'vertical', 'org': 'UserPartitionTransformer',
'#ref': 'vertical1', 'course': 'UP101F',
'#children': [ 'run': 'test_run',
{ 'user_partitions': [self.user_partition],
'metadata': {'group_access': {0: [0]}}, '#type': 'course',
'#type': 'html', '#ref': 'course',
'#ref': 'html1', '#children': [
}, {
{ '#type': 'vertical',
'metadata': {'group_access': {0: [1]}}, '#ref': 'A',
'#type': 'html', 'metadata': {'group_access': {self.TEST_PARTITION_ID: [0, 1, 2, 3]}},
'#ref': 'html2', },
} {'#type': 'vertical', '#ref': 'B'},
], ],
} },
], {
} '#type': 'vertical',
], '#ref': 'C',
} '#parents': ['A'],
] 'metadata': {'group_access': {self.TEST_PARTITION_ID: [1, 2]}},
} '#children': [
{'#type': 'vertical', '#ref': 'F'},
def add_user_to_cohort_group(self, cohort, group): {
""" '#type': 'vertical',
Add user to cohort, link cohort to content group, and update blocks. '#ref': 'G',
""" 'metadata': {'group_access': {self.TEST_PARTITION_ID: [1]}},
add_user_to_cohort(cohort, self.user.username) },
link_cohort_to_partition_group( {
cohort, '#type': 'vertical',
self.user_partition.id, '#ref': 'H',
group.id, 'metadata': {'group_access': {self.TEST_PARTITION_ID: [2]}},
) },
],
},
{
'#type': 'vertical',
'#ref': 'D',
'#parents': ['A'],
'metadata': {'group_access': {self.TEST_PARTITION_ID: [2, 3]}},
'#children': [{'#type': 'vertical', '#ref': 'I'}],
},
{
'#type': 'vertical',
'#ref': 'E',
'#parents': ['A'],
'#children': [{'#type': 'vertical', '#ref': 'J'}],
},
{
'#type': 'vertical',
'#ref': 'K',
'#parents': ['E'],
'metadata': {'group_access': {self.TEST_PARTITION_ID: [4]}},
'#children': [{'#type': 'vertical', '#ref': 'N'}],
},
{
'#type': 'vertical',
'#ref': 'L',
'#parents': ['G'],
'metadata': {'group_access': {self.TEST_PARTITION_ID: [1, 2]}},
},
{
'#type': 'vertical',
'#ref': 'M',
'#parents': ['G', 'H'],
'metadata': {'group_access': {self.TEST_PARTITION_ID: [1, 2, 3]}},
},
{
'#type': 'vertical',
'#ref': 'O',
'#parents': ['K', 'B'],
},
]
def test_user_assigned(self): def test_user_assigned(self):
""" """
Test when user is assigned to group in user partition. Test when user is assigned to group in user partition.
""" """
# TODO ddt with testing user in different groups
trans_block_structure = get_course_blocks( trans_block_structure = get_course_blocks(
self.user, self.user,
self.course.location, self.course.location,
...@@ -119,8 +172,8 @@ class UserPartitionTransformerTestCase(CourseStructureTestCase): ...@@ -119,8 +172,8 @@ class UserPartitionTransformerTestCase(CourseStructureTestCase):
) )
self.assertSetEqual( self.assertSetEqual(
set(trans_block_structure.get_block_keys()), set(trans_block_structure.get_block_keys()),
self.get_block_key_set('course', 'chapter1', 'lesson1', 'vertical1', 'html2') self.get_block_key_set(self.blocks, 'course', 'A', 'B', 'C', 'E', 'F', 'G', 'J', 'L', 'M', 'O')
) )
def test_staff_user(self): def test_staff_user(self):
self.assert_staff_access_to_all_blocks(self.staff, self.course, self.blocks, self.transformer) self.assert_staff_access_to_all_blocks(self.course, self.blocks, self.transformer)
lms/djangoapps/course_blocks/transformers/tests/test_visibility.py
...@@ -4,7 +4,7 @@ Tests for VisibilityTransformer. ...@@ -4,7 +4,7 @@ Tests for VisibilityTransformer.
import ddt import ddt
from course_blocks.transformers.visibility import VisibilityTransformer from course_blocks.transformers.visibility import VisibilityTransformer
from .test_helpers import BlockParentsMapTestCase from .test_helpers import BlockParentsMapTestCase, update_block
@ddt.ddt @ddt.ddt
...@@ -33,7 +33,7 @@ class VisibilityTransformerTestCase(BlockParentsMapTestCase): ...@@ -33,7 +33,7 @@ class VisibilityTransformerTestCase(BlockParentsMapTestCase):
for i, _ in enumerate(self.parents_map): for i, _ in enumerate(self.parents_map):
block = self.get_block(i) block = self.get_block(i)
block.visible_to_staff_only = (i in staff_only_blocks) block.visible_to_staff_only = (i in staff_only_blocks)
self.update_block(block) update_block(block)
self.check_transformer_results( self.check_transformer_results(
expected_student_visible_blocks, blocks_with_differing_student_access, [VisibilityTransformer()] expected_student_visible_blocks, blocks_with_differing_student_access, [VisibilityTransformer()]
......
lms/djangoapps/course_blocks/transformers/user_partitions.py
...@@ -4,7 +4,7 @@ User Partitions Transformer ...@@ -4,7 +4,7 @@ User Partitions Transformer
from openedx.core.lib.block_cache.transformer import BlockStructureTransformer from openedx.core.lib.block_cache.transformer import BlockStructureTransformer
from .split_test import SplitTestTransformer from .split_test import SplitTestTransformer
from .utils import get_field_on_block
class MergedGroupAccess(object): class MergedGroupAccess(object):
""" """
...@@ -36,19 +36,26 @@ class MergedGroupAccess(object): ...@@ -36,19 +36,26 @@ class MergedGroupAccess(object):
# Note that a user must have access to all partitions in group_access # Note that a user must have access to all partitions in group_access
# or _access in order to access a block. # or _access in order to access a block.
block_group_access = getattr(xblock, 'group_access', {})
self._access = {} # { partition.id: set(IDs of groups that can access partition } self._access = {} # { partition.id: set(IDs of groups that can access partition }
# Get the group_access value that is directly set on the xblock.
# Do not get the inherited value since field inheritance doesn't
# take a union of them for DAGs.
block_group_access = get_field_on_block(xblock, 'group_access', default_value={})
for partition in user_partitions: for partition in user_partitions:
# Within this loop, None <=> Universe set <=> "No access restriction" # Within this loop, None <=> Universe set <=> "No access restriction"
block_group_ids = set(block_group_access.get(partition.id, [])) or None block_group_ids = set(block_group_access.get(partition.id, [])) or None
parents_group_ids = [ parents_group_ids = []
merged_parent_access._access[partition.id] for merged_parent_access in merged_parent_access_list:
for merged_parent_access in merged_parent_access_list if partition.id in merged_parent_access._access:
if partition.id in merged_parent_access._access parents_group_ids.append(merged_parent_access._access[partition.id])
] else:
parents_group_ids = []
break
merged_parent_group_ids = ( merged_parent_group_ids = (
set().union(*parents_group_ids) set().union(*parents_group_ids)
if parents_group_ids != [] if parents_group_ids != []
...@@ -193,6 +200,7 @@ class UserPartitionTransformer(BlockStructureTransformer): ...@@ -193,6 +200,7 @@ class UserPartitionTransformer(BlockStructureTransformer):
user_groups = get_user_partition_groups( user_groups = get_user_partition_groups(
user_info.course_key, user_partitions, user_info.user user_info.course_key, user_partitions, user_info.user
) )
# TODO test this when deserializing across processes
block_structure.remove_block_if( block_structure.remove_block_if(
lambda block_key: not block_structure.get_transformer_block_data( lambda block_key: not block_structure.get_transformer_block_data(
block_key, self, 'merged_group_access' block_key, self, 'merged_group_access'
......
lms/djangoapps/course_blocks/transformers/utils.py 0 → 100644
"""
Common Helper utilities for transformers
"""
def get_field_on_block(block, field_name, default_value=None):
# Get the field value that is directly set on the xblock.
# Do not get the inherited value since field inheritance
# returns value from only a single parent chain
# (e.g., doesn't take a union in DAGs).
if block.fields[field_name].is_set_on(block):
return getattr(block, field_name)
else:
return default_value
Markdown is supported
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