Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: AA-802: Update the BlockCompletionTransformer

Browse Source 
We discovered a subsection that contained a unit without any content
inside, but because of our logic requiring children, it would never be
marked complete (meaning the subsection, section, and course could thus
never be marked complete). This fixes that by removing the children
check from setting completion, but first gating that code path on the
xblock being an aggregator (to prevent leaves from marking as true
simply because there are no children).

Test fixes include adding a test for the empty aggregator case as
well as some changes to not have an entire course marked complete
because they are all empty aggregators.
This commit is contained in:
Dillon Dumesnil
2021-05-13 14:06:11 -04:00
parent 128c53e50b
commit 6f773d1eca
4 changed files with 94 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
lms/djangoapps/course_api/blocks/api.py
View File

@@ -88,6 +88,13 @@ def get_blocks(
HiddenContentTransformer()
]
# Note: A change to the BlockCompletionTransformer (https://github.com/edx/edx-platform/pull/27622/)
# will be introducing a bug if hide_access_denials is True. I'm accepting this risk because in
# the AccessDeniedMessageFilterTransformer, there is note about deleting it and I believe it is
# technically deprecated functionality. The only use case where hide_access_denials is True
# (outside of explicitly setting the temporary waffle flag) is in lms/djangoapps/course_api/blocks/urls.py
# for a v1 api that I also believe should have been deprecated and removed. When this code is removed,
# please also remove this comment. Thanks!
if hide_access_denials:
transformers += [AccessDeniedMessageFilterTransformer()]

18
lms/djangoapps/course_api/blocks/transformers/block_completion.py
View File

@@ -71,17 +71,17 @@ class BlockCompletionTransformer(BlockStructureTransformer):
block_structure.override_xblock_field(block_key, self.COMPLETE, True)
if str(block_key) == str(latest_complete_block_key):
block_structure.override_xblock_field(block_key, self.RESUME_BLOCK, True)
elif block_structure.get_xblock_field(block_key, 'completion_mode') == CompletionMode.AGGREGATOR:
children = block_structure.get_children(block_key)
all_children_complete = all(block_structure.get_xblock_field(child_key, self.COMPLETE)
for child_key in children
if not self._is_block_excluded(block_structure, child_key))
children = block_structure.get_children(block_key)
all_children_complete = all(block_structure.get_xblock_field(child_key, self.COMPLETE)
for child_key in children
if not self._is_block_excluded(block_structure, child_key))
if all_children_complete:
block_structure.override_xblock_field(block_key, self.COMPLETE, True)
if children and all_children_complete:
block_structure.override_xblock_field(block_key, self.COMPLETE, True)
if any(block_structure.get_xblock_field(child_key, self.RESUME_BLOCK) for child_key in children):
block_structure.override_xblock_field(block_key, self.RESUME_BLOCK, True)
if any(block_structure.get_xblock_field(child_key, self.RESUME_BLOCK) for child_key in children):
block_structure.override_xblock_field(block_key, self.RESUME_BLOCK, True)
def transform(self, usage_info, block_structure):
"""

88
lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py
View File

@@ -21,6 +21,7 @@ class StubAggregatorXBlock(XBlock):
when transforming aggregator XBlock.
"""
completion_mode = XBlockCompletionMode.AGGREGATOR
has_children = True
class StubExcludedXBlock(XBlock):
@@ -36,7 +37,6 @@ class StubCompletableXBlock(XBlock, CompletableXBlockMixin):
XBlock to test behaviour of BlockCompletionTransformer
when transforming completable XBlock.
"""
pass # lint-amnesty, pylint: disable=unnecessary-pass
class BlockCompletionTransformerTestCase(TransformerRegistryTestMixin, CompletionWaffleTestMixin, ModuleStoreTestCase):
@@ -44,7 +44,9 @@ class BlockCompletionTransformerTestCase(TransformerRegistryTestMixin, Completio
Tests behaviour of BlockCompletionTransformer
"""
TRANSFORMER_CLASS_TO_TEST = BlockCompletionTransformer
COMPLETION_TEST_VALUE = 0.4
# Has to be 1.0 for 'complete' to function properly. The Completion api only uses 0.0 and 1.0 right now
# so this better reflects reality anyway. Should be updated if Completion api ever supports more.
COMPLETION_TEST_VALUE = 1.0
def setUp(self):
super().setUp()
@@ -53,31 +55,54 @@ class BlockCompletionTransformerTestCase(TransformerRegistryTestMixin, Completio
self.override_waffle_switch(True)
@XBlock.register_temp_plugin(StubAggregatorXBlock, identifier='aggregator')
def test_transform_gives_none_for_aggregator(self):
@XBlock.register_temp_plugin(StubCompletableXBlock, identifier='comp')
def test_transform_aggregators(self):
"""
If there is an aggregator (such as a unit) that contains no children for some reason
(likely a mistake by the course team), it should be marked 'complete'. 'completion' should
still be None.
We mark 'complete' so learners are still able to have their subsections/sections/course
marked as complete and are not blocked by this one empty aggregator.
"""
course = CourseFactory.create()
block = ItemFactory.create(category='aggregator', parent=course)
block_structure = get_course_blocks(
self.user, course.location, self.transformers
# Have to have at least one complete block to trigger entering the marking 'complete' flow
filled_aggregator = ItemFactory.create(category='aggregator', parent=course)
block = ItemFactory.create(category='comp', parent=filled_aggregator)
BlockCompletion.objects.submit_completion(
user=self.user,
block_key=block.location,
completion=self.COMPLETION_TEST_VALUE,
)
empty_aggregator = ItemFactory.create(category='aggregator', parent=course)
block_structure = get_course_blocks(self.user, course.location, self.transformers)
self._assert_block_has_proper_completion_value(
block_structure, block.location, None
self._assert_block_has_proper_completion_values(
block_structure, block.location, self.COMPLETION_TEST_VALUE, True
)
self._assert_block_has_proper_completion_values(
block_structure, filled_aggregator.location, None, True
)
self._assert_block_has_proper_completion_values(
block_structure, empty_aggregator.location, None, True
)
@XBlock.register_temp_plugin(StubExcludedXBlock, identifier='excluded')
def test_transform_gives_none_for_excluded(self):
"""
Excluded blocks always receive None for 'completion' and False for 'complete'
"""
course = CourseFactory.create()
block = ItemFactory.create(category='excluded', parent=course)
block_structure = get_course_blocks(
self.user, course.location, self.transformers
)
block_structure = get_course_blocks(self.user, course.location, self.transformers)
self._assert_block_has_proper_completion_value(
block_structure, block.location, None
)
self._assert_block_has_proper_completion_values(block_structure, block.location, None, False)
@XBlock.register_temp_plugin(StubCompletableXBlock, identifier='comp')
def test_transform_gives_value_for_completable(self):
"""
If a block is actually complete, make sure that value shows up in the transformed fields.
'completion' should have the value and 'complete' should be True in these cases.
"""
course = CourseFactory.create()
block = ItemFactory.create(category='comp', parent=course)
BlockCompletion.objects.submit_completion(
@@ -85,34 +110,33 @@ class BlockCompletionTransformerTestCase(TransformerRegistryTestMixin, Completio
block_key=block.location,
completion=self.COMPLETION_TEST_VALUE,
)
block_structure = get_course_blocks(
self.user, course.location, self.transformers
)
block_structure = get_course_blocks(self.user, course.location, self.transformers)
self._assert_block_has_proper_completion_value(
block_structure, block.location, self.COMPLETION_TEST_VALUE
self._assert_block_has_proper_completion_values(
block_structure, block.location, self.COMPLETION_TEST_VALUE, True
)
def test_transform_gives_zero_for_ordinary_block(self):
"""
I _think_ this is testing 'completion' is 0.0 before an ordinary block is viewed.
'html' blocks end up receiving a 'completion' of 1.0 after being viewed.
"""
course = CourseFactory.create()
block = ItemFactory.create(category='html', parent=course)
block_structure = get_course_blocks(
self.user, course.location, self.transformers
)
block_structure = get_course_blocks(self.user, course.location, self.transformers)
self._assert_block_has_proper_completion_value(
block_structure, block.location, 0.0
)
self._assert_block_has_proper_completion_values(block_structure, block.location, 0.0, False)
def _assert_block_has_proper_completion_value(
self, block_structure, block_key, expected_value
def _assert_block_has_proper_completion_values(
self, block_structure, block_key, expected_completion, expected_complete
):
"""
Checks whether block's completion has expected value.
Checks whether block's completion and complete have expected values.
"""
block_data = block_structure.get_transformer_block_data(
block_key, self.TRANSFORMER_CLASS_TO_TEST
)
block_data = block_structure.get_transformer_block_data(block_key, self.TRANSFORMER_CLASS_TO_TEST)
completion_value = block_data.fields['completion']
# complete isn't saved as a transformer field, but just a regular xblock field /shrug
complete_value = block_structure.get_xblock_field(block_key, 'complete', False)
assert completion_value == expected_value
assert completion_value == expected_completion
assert complete_value == expected_complete

38
openedx/features/course_experience/tests/views/test_course_outline.py
View File

@@ -462,6 +462,9 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase, CompletionWaffleT
vertical2 = ItemFactory.create(category='vertical', parent_location=sequential2.location)
vertical3 = ItemFactory.create(category='vertical', parent_location=sequential3.location)
vertical4 = ItemFactory.create(category='vertical', parent_location=sequential4.location)
problem = ItemFactory.create(category='problem', parent_location=vertical.location)
problem2 = ItemFactory.create(category='problem', parent_location=vertical2.location)
problem3 = ItemFactory.create(category='problem', parent_location=vertical3.location)
course.children = [chapter, chapter2]
chapter.children = [sequential, sequential2]
chapter2.children = [sequential3, sequential4]
@@ -469,6 +472,9 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase, CompletionWaffleT
sequential2.children = [vertical2]
sequential3.children = [vertical3]
sequential4.children = [vertical4]
vertical.children = [problem]
vertical2.children = [problem2]
vertical3.children = [problem3]
if hasattr(cls, 'user'):
CourseEnrollment.enroll(cls.user, course.id)
return course
@@ -545,8 +551,8 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase, CompletionWaffleT
response = self.client.get(course_home_url(course))
content = pq(response.content)
# Subsection should be checked
assert len(content('.fa-check')) == 1
# Subsection should be checked. Subsection 4 is also checked because it contains a vertical with no content
assert len(content('.fa-check')) == 2
def test_start_course(self):
"""
@@ -561,8 +567,8 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase, CompletionWaffleT
response = self.visit_course_home(course, start_count=1, resume_count=0)
content = pq(response.content)
vertical = course.children[0].children[0].children[0]
assert content('.action-resume-course').attr('href').endswith('/vertical/' + vertical.url_name)
problem = course.children[0].children[0].children[0].children[0]
assert content('.action-resume-course').attr('href').endswith('/problem/' + problem.url_name)
@override_settings(LMS_BASE='test_url:9999')
def test_resume_course_with_completion_api(self):
@@ -573,33 +579,33 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase, CompletionWaffleT
# Course tree
course = self.course
vertical1 = course.children[0].children[0].children[0]
vertical2 = course.children[0].children[1].children[0]
problem1 = course.children[0].children[0].children[0].children[0]
problem2 = course.children[0].children[1].children[0].children[0]
self.complete_sequential(self.course, vertical1)
self.complete_sequential(self.course, problem1)
# Test for 'resume' link
response = self.visit_course_home(course, resume_count=1)
# Test for 'resume' link URL - should be vertical 1
# Test for 'resume' link URL - should be problem 1
content = pq(response.content)
assert content('.action-resume-course').attr('href').endswith('/vertical/' + vertical1.url_name)
assert content('.action-resume-course').attr('href').endswith('/problem/' + problem1.url_name)
self.complete_sequential(self.course, vertical2)
self.complete_sequential(self.course, problem2)
# Test for 'resume' link
response = self.visit_course_home(course, resume_count=1)
# Test for 'resume' link URL - should be vertical 2
# Test for 'resume' link URL - should be problem 2
content = pq(response.content)
assert content('.action-resume-course').attr('href').endswith('/vertical/' + vertical2.url_name)
assert content('.action-resume-course').attr('href').endswith('/problem/' + problem2.url_name)
# visit sequential 1, make sure 'Resume Course' URL is robust against 'Last Visited'
# (even though I visited seq1/vert1, 'Resume Course' still points to seq2/vert2)
self.visit_sequential(course, course.children[0], course.children[0].children[0])
# Test for 'resume' link URL - should be vertical 2 (last completed block, NOT last visited)
# Test for 'resume' link URL - should be problem 2 (last completed block, NOT last visited)
response = self.visit_course_home(course, resume_count=1)
content = pq(response.content)
assert content('.action-resume-course').attr('href').endswith('/vertical/' + vertical2.url_name)
assert content('.action-resume-course').attr('href').endswith('/problem/' + problem2.url_name)
def test_resume_course_deleted_sequential(self):
"""
@@ -662,8 +668,8 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase, CompletionWaffleT
CourseEnrollment.get_enrollment(self.user, course.id).delete()
response = self.visit_course_home(course, start_count=1, resume_count=0)
content = pq(response.content)
vertical = course.children[0].children[0].children[0]
assert content('.action-resume-course').attr('href').endswith('/vertical/' + vertical.url_name)
problem = course.children[0].children[0].children[0].children[0]
assert content('.action-resume-course').attr('href').endswith('/problem/' + problem.url_name)
@override_waffle_switch(ENABLE_COMPLETION_TRACKING_SWITCH, active=True)
def test_course_outline_auto_open(self):