Skip to content
Closed
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
72 changes: 72 additions & 0 deletions xmodule/modulestore/tests/test_xml_importer.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,3 +257,75 @@ def test_import_static_file(self):
)
mock_file.assert_called_with(full_file_path, 'rb')
self.mocked_content_store.generate_thumbnail.assert_called_once()


class TestApplySequentialGating(unittest.TestCase):
"""
Unit tests for ``_apply_sequential_gating`` — bug #36995 regression.

Subsection gating metadata must round-trip through OLX export/import.
"""

def setUp(self):
super().setUp()
self.course_key = CourseLocator('org', 'course', 'run')
self.seq_loc = self.course_key.make_usage_key('sequential', 'gated_seq')
self.prereq_loc = self.course_key.make_usage_key('sequential', 'prereq_seq')
self.block = mock.Mock()
self.block.location = self.seq_loc

def _run(self, attrs, gating_enabled=True, prereq_exists=True):
"""Invoke _apply_sequential_gating with the given config and return the mocks."""
from xmodule.modulestore.xml_importer import _apply_sequential_gating # noqa: PLC0415
course = mock.Mock(enable_subsection_gating=gating_enabled)
with mock.patch('xmodule.modulestore.xml_importer.modulestore') as ms, \
mock.patch('openedx.core.lib.gating.api.is_prerequisite', return_value=False), \
mock.patch('openedx.core.lib.gating.api.add_prerequisite') as add_p, \
mock.patch('openedx.core.lib.gating.api.set_required_content') as set_r:
ms.return_value.get_course.return_value = course
ms.return_value.has_item.return_value = prereq_exists
_apply_sequential_gating(self.block, self.course_key, attrs)
return add_p, set_r

def test_unit_noop_when_gating_disabled_on_course(self):
"""If the course has gating disabled, do nothing."""
add_p, set_r = self._run(
{
'required_prereq': str(self.prereq_loc),
'min_score': '80',
'min_completion': '90',
},
gating_enabled=False,
)
add_p.assert_not_called()
set_r.assert_not_called()

def test_unit_is_prerequisite_flag_only(self):
"""is_prerequisite='true' alone marks the subsection as an eligible prereq source."""
add_p, set_r = self._run({'is_prerequisite': 'true'})
add_p.assert_called_once_with(self.course_key, self.seq_loc)
# set_required_content is still called once to clear stale state.
assert set_r.call_count == 1

def test_integration_missing_prereq_raises(self):
"""Referencing a non-existent prereq must raise CourseImportException."""
from xmodule.modulestore.xml_importer import CourseImportException # noqa: PLC0415
with self.assertRaises(CourseImportException):
self._run(
{
'required_prereq': str(self.prereq_loc),
'min_score': '80',
'min_completion': '90',
},
prereq_exists=False,
)

def test_bug_36995_regression_invalid_prereq_key_raises(self):
"""Regression for #36995: invalid key string must raise CourseImportException."""
from xmodule.modulestore.xml_importer import CourseImportException # noqa: PLC0415
with self.assertRaises(CourseImportException):
self._run({
'required_prereq': 'not-a-usage-key',
'min_score': '80',
'min_completion': '90',
})
104 changes: 104 additions & 0 deletions xmodule/modulestore/xml_importer.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@
from .inheritance import own_metadata
from .store_utilities import rewrite_nonportable_content_links

# XML attributes on <sequential> used to carry subsection gating metadata
# across OLX export/import. See issue #36995.
GATING_XML_ATTRS = ('is_prerequisite', 'required_prereq', 'min_score', 'min_completion')

log = logging.getLogger(__name__)

DEFAULT_STATIC_CONTENT_SUBDIR = 'static'
Expand Down Expand Up @@ -834,6 +838,86 @@ def import_library_from_xml(*args, **kwargs):
return list(manager.run_imports())


def _apply_sequential_gating(block, dest_course_id, gating_xml_attrs):
"""
Persist subsection gating metadata carried on a ``<sequential>`` OLX
element into the external edx-milestones tables.

Called after the sequential has been written to the modulestore so the
block exists when milestones reference it. Clears any stale "requires"
relationship on the target so re-imports stay idempotent: if the OLX no
longer defines a prereq, existing milestones for this subsection are
removed first. See issue #36995.

Raises:
CourseImportException: if the OLX references a prereq subsection
that does not exist in the imported course, or carries an
invalid prerequisite key.
"""
# Local imports to avoid pulling LMS-only code at module import time.
from opaque_keys import InvalidKeyError # noqa: PLC0415
from openedx.core.lib.gating import api as gating_api # noqa: PLC0415
from openedx.core.lib.gating.exceptions import GatingValidationError # noqa: PLC0415

from xmodule.modulestore.django import modulestore # noqa: PLC0415

course = modulestore().get_course(dest_course_id)
if course is None or not getattr(course, 'enable_subsection_gating', False):
log.info(
'Skipping gating import for %s: enable_subsection_gating is off',
block.location,
)
return

# Idempotency: wipe any existing "requires" relationship so re-imports
# reflect the source OLX exactly.
gating_api.set_required_content(dest_course_id, block.location, None)

if str(gating_xml_attrs.get('is_prerequisite', '')).lower() == 'true':
gating_api.add_prerequisite(dest_course_id, block.location)

required_prereq = gating_xml_attrs.get('required_prereq')
if not required_prereq:
return

try:
prereq_key = UsageKey.from_string(required_prereq).map_into_course(dest_course_id)
except InvalidKeyError as err:
raise CourseImportException(
_('Invalid prerequisite key {key} on subsection {loc}').format(
key=required_prereq, loc=block.location,
)
) from err

if not modulestore().has_item(prereq_key):
raise CourseImportException(
_(
'Subsection {loc} references prerequisite {prereq} which was '
'not found in the imported course.'
).format(loc=block.location, prereq=prereq_key)
)

# The target subsection must itself be registered as a prereq source
# before set_required_content can succeed.
if not gating_api.is_prerequisite(dest_course_id, prereq_key):
gating_api.add_prerequisite(dest_course_id, prereq_key)

try:
gating_api.set_required_content(
dest_course_id,
block.location,
prereq_key,
gating_xml_attrs.get('min_score', ''),
gating_xml_attrs.get('min_completion', ''),
)
except GatingValidationError as err:
raise CourseImportException(
_('Invalid gating thresholds on {loc}: {err}').format(
loc=block.location, err=err,
)
) from err


def _update_and_import_block( # pylint: disable=too-many-statements
block, store, user_id,
source_course_id, dest_course_id,
Expand All @@ -844,6 +928,18 @@ def _update_and_import_block( # pylint: disable=too-many-statements
"""
logging.debug('processing import of blocks %s...', str(block.location))

# Snapshot gating xml_attributes before _update_block_references strips
# them, so we can re-apply the data to the milestones tables after the
# sequential has been imported. See issue #36995.
gating_xml_attrs_snapshot = None
if block.location.block_type == 'sequential':
raw_xml_attrs = getattr(block, 'xml_attributes', None) or {}
gating_xml_attrs_snapshot = {
key: raw_xml_attrs[key]
for key in GATING_XML_ATTRS
if key in raw_xml_attrs
}

def _update_block_references(block, source_course_id, dest_course_id):
"""
Move the block to a new course.
Expand Down Expand Up @@ -892,6 +988,11 @@ def _convert_ref_fields_to_new_namespace(reference):

if 'index_in_children_list' in value:
del value['index_in_children_list']
# Strip gating xml_attributes — they are re-applied
# against the milestones tables by
# _apply_sequential_gating below (see issue #36995).
for _gate_attr in GATING_XML_ATTRS:
value.pop(_gate_attr, None)
fields[field_name] = value
else:
fields[field_name] = field.read_from(block)
Expand All @@ -918,6 +1019,9 @@ def _convert_ref_fields_to_new_namespace(reference):
block.location.block_id, fields, runtime, asides=asides
)

if gating_xml_attrs_snapshot:
_apply_sequential_gating(block, dest_course_id, gating_xml_attrs_snapshot)

# TODO: Move this code once the following condition is met.
# Get to the point where XML import is happening inside the
# modulestore that is eventually going to store the data.
Expand Down
44 changes: 44 additions & 0 deletions xmodule/seq_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -969,8 +969,52 @@ def definition_to_xml(self, resource_fs):
xml_object = etree.Element('sequential')
for child in self.get_children():
self.runtime.add_block_as_child_node(child, xml_object)
self._add_gating_info_to_xml(xml_object)
return xml_object

def _add_gating_info_to_xml(self, xml_object):
"""
Serialize subsection gating metadata onto the <sequential> element so
it survives OLX export/import. No-op if the course has
``enable_subsection_gating`` disabled.

Subsection gating data lives in the external ``edx-milestones``
tables (outside the modulestore), so without this serialization
step every prerequisite is lost on export/import. See issue #36995.

Writes up to three attributes on the <sequential> element:

* ``is_prerequisite="true"`` — this subsection is marked as
available to gate other subsections.
* ``required_prereq`` — usage key string of the subsection that
must be completed before this one is accessible.
* ``min_score`` / ``min_completion`` — thresholds on that prereq.
"""
# Local import: avoid pulling LMS-only code at module load time.
from openedx.core.lib.gating import api as gating_api # noqa: PLC0415

try:
course = self.runtime.modulestore.get_course(self.location.course_key)
except Exception: # pylint: disable=broad-except
return
if course is None or not getattr(course, 'enable_subsection_gating', False):
return

if gating_api.is_prerequisite(self.location.course_key, self.location):
xml_object.set('is_prerequisite', 'true')

prereq_key_str, min_score, min_completion = gating_api.get_required_content(
self.location.course_key,
self.location,
)
if not prereq_key_str:
return
xml_object.set('required_prereq', prereq_key_str)
if min_score not in (None, ''):
xml_object.set('min_score', str(min_score))
if min_completion not in (None, ''):
xml_object.set('min_completion', str(min_completion))

@property
def non_editable_metadata_fields(self):
"""
Expand Down