From 47bf2679e3637e0f8e4cf27d125bd031637e3445 Mon Sep 17 00:00:00 2001 From: kingoftech-v01 Date: Fri, 10 Apr 2026 05:29:02 +0000 Subject: [PATCH] fix(olx): persist subsection prerequisites across export/import Subsection gating metadata (required prereq, min_score, min_completion, and the is_prerequisite flag) lives in the external edx-milestones tables and was previously not serialized by OLX export. Re-importing a course therefore silently lost every configured learning-path gate. Serialize the gating metadata as attributes on the element in SequenceBlock.definition_to_xml so it survives a full round trip. On import, rebuild the milestone relationships via openedx.core.lib.gating.api, verify the referenced prerequisite block actually exists in the target course, and raise CourseImportException with a clear message otherwise. Re-imports first clear any stale "requires" milestone so the target matches the OLX exactly (round-trip fidelity). Both paths no-op when the course-level enable_subsection_gating flag is off. Closes #36995 --- .../modulestore/tests/test_xml_importer.py | 72 ++++++++++++ xmodule/modulestore/xml_importer.py | 104 ++++++++++++++++++ xmodule/seq_block.py | 44 ++++++++ 3 files changed, 220 insertions(+) diff --git a/xmodule/modulestore/tests/test_xml_importer.py b/xmodule/modulestore/tests/test_xml_importer.py index b8e82814cf08..f35de6c42654 100644 --- a/xmodule/modulestore/tests/test_xml_importer.py +++ b/xmodule/modulestore/tests/test_xml_importer.py @@ -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', + }) diff --git a/xmodule/modulestore/xml_importer.py b/xmodule/modulestore/xml_importer.py index 88c88fa42fd2..0db45aebbea1 100644 --- a/xmodule/modulestore/xml_importer.py +++ b/xmodule/modulestore/xml_importer.py @@ -60,6 +60,10 @@ from .inheritance import own_metadata from .store_utilities import rewrite_nonportable_content_links +# XML attributes on 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' @@ -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 ```` 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, @@ -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. @@ -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) @@ -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. diff --git a/xmodule/seq_block.py b/xmodule/seq_block.py index ba482a285571..515dbf49426a 100644 --- a/xmodule/seq_block.py +++ b/xmodule/seq_block.py @@ -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 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 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): """