diff --git a/xmodule/modulestore/tests/test_xml_importer.py b/xmodule/modulestore/tests/test_xml_importer.py index 8f18907f2c2f..ada64e88229b 100644 --- a/xmodule/modulestore/tests/test_xml_importer.py +++ b/xmodule/modulestore/tests/test_xml_importer.py @@ -2,7 +2,6 @@ Tests for XML importer. """ - import importlib import os import unittest @@ -16,10 +15,17 @@ from xblock.fields import List, Scope, ScopeIds, String from xblock.runtime import DictKeyValueStore, KvsFieldData, Runtime +from openedx.core.lib.gating.exceptions import GatingValidationError from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.modulestore.tests.mongo_connection import MONGO_HOST, MONGO_PORT_NUM -from xmodule.modulestore.xml_importer import StaticContentImporter, _update_and_import_block, _update_block_location +from xmodule.modulestore.xml_importer import ( + StaticContentImporter, + _apply_gating_relationships, + _process_sequential_prerequisites, + _update_and_import_block, + _update_block_location, +) from xmodule.tests import DATA_DIR from xmodule.x_module import XModuleMixin @@ -398,3 +404,132 @@ def test_object_does_not_exist_during_sync_is_handled(self): # Outer else runs even though inner sync failed → publish is called store.publish.assert_called_once_with(published_block.location, 1) + + +class TestSequentialPrerequisitesImport(unittest.TestCase): + """ + Verifies sequential blocks with prerequisite attributes in OLX/XML are processed correctly + by _process_sequential_prerequisites and later persisted by _apply_gating_relationships. + """ + + def setUp(self): + """ + Set up test course and sequential. + """ + self.course_key = CourseLocator('test_org', 'test_course', 'test_run') + self.sequential_location = self.course_key.make_usage_key('sequential', 'gated_sequential') + + # Mock sequential block + self.mock_sequential = mock.Mock() + self.mock_sequential.location = self.sequential_location + + def test_gating_information_extraction(self): + """ + Verify that _process_sequential_prerequisites correctly processes valid prerequisite data + """ + self.mock_sequential.xml_attributes = { + 'required_content': 'required_sequential', + 'min_score': '80', + 'min_completion': '90' + } + + result = _process_sequential_prerequisites(self.mock_sequential) + expected_prereq_key = self.course_key.make_usage_key('sequential', 'required_sequential') + + assert result is not None + block_location, prereq_key, min_score, min_completion = result + assert block_location == self.sequential_location + assert prereq_key == expected_prereq_key + assert min_score == 80 + assert min_completion == 90 + + + + def test_gating_information_extraction_invalid_min_score(self): + """ + Verify that _process_sequential_prerequisites correctly processes invalid min_score prerequisite data + """ + self.mock_sequential.xml_attributes = { + 'required_content': 'required_sequential', + 'min_score': None, + 'min_completion': '90' + } + + result = _process_sequential_prerequisites(self.mock_sequential) + + assert result is None + + + def test_gating_information_extraction_invalid_min_completion(self): + """ + Verify that _process_sequential_prerequisites correctly processes invalid min_completion prerequisite data + """ + self.mock_sequential.xml_attributes = { + 'required_content': 'required_sequential', + 'min_score': '80', + 'min_completion': 'NotANumber' + } + + result = _process_sequential_prerequisites(self.mock_sequential) + + assert result is None + + + def test_gating_information_extraction_missing_required_content(self): + """ + Verify that _process_sequential_prerequisites correctly processes missing required_content prerequisite data + """ + self.mock_sequential.xml_attributes = { + 'min_score': '80', + 'min_completion': 'NotANumber' + } + + result = _process_sequential_prerequisites(self.mock_sequential) + + assert result is None + + def test_gating_information_valid_relationship(self): + """ + Verify valid gating information is persisted. + """ + mock_store = mock.Mock() + mock_store.get_item.return_value = mock.Mock() # block exists + + relationships = [ + (self.sequential_location, self.course_key.make_usage_key('sequential', 'required_sequential'), 80, 90), + ] + + with mock.patch('xmodule.modulestore.xml_importer.gating_api') as mock_gating_api: + _apply_gating_relationships(mock_store, self.course_key, relationships) + + mock_gating_api.add_prerequisite.assert_called_once_with( + self.course_key, + self.course_key.make_usage_key('sequential', 'required_sequential'), + ) + mock_gating_api.set_required_content.assert_called_once_with( + self.course_key, + self.sequential_location, + self.course_key.make_usage_key('sequential', 'required_sequential'), + 80, + 90, + ) + + def test_gating_api_error(self): + """ + If GatingValidationError is raised, the error should be logged and the relationship should be ignored + """ + mock_store = mock.Mock() + mock_store.get_item.return_value = mock.Mock() + + relationships = [ + (self.sequential_location, self.course_key.make_usage_key('sequential', 'required_sequential'), 80, 90), + ] + + with mock.patch('xmodule.modulestore.xml_importer.gating_api') as mock_gating_api: + mock_gating_api.set_required_content.side_effect = GatingValidationError("Invalid") + with mock.patch('xmodule.modulestore.xml_importer.logging') as mock_logging: + _apply_gating_relationships(mock_store, self.course_key, relationships) + + mock_gating_api.add_prerequisite.assert_called_once() + mock_gating_api.set_required_content.assert_called_once() + mock_logging.error.assert_called() diff --git a/xmodule/modulestore/xml_importer.py b/xmodule/modulestore/xml_importer.py index 237d9e0eff15..b18ffb744a0e 100644 --- a/xmodule/modulestore/xml_importer.py +++ b/xmodule/modulestore/xml_importer.py @@ -44,12 +44,14 @@ from common.djangoapps.util.monitoring import monitor_import_failure from openedx.core.djangoapps.content_tagging.api import import_course_tags_from_csv +from openedx.core.lib.gating import api as gating_api +from openedx.core.lib.gating.exceptions import GatingValidationError from xmodule.assetstore import AssetMetadata from xmodule.contentstore.content import StaticContent from xmodule.errortracker import make_error_tracker from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import ASSET_IGNORE_REGEX -from xmodule.modulestore.exceptions import DuplicateCourseError +from xmodule.modulestore.exceptions import DuplicateCourseError, ItemNotFoundError from xmodule.modulestore.mongo.base import MongoRevisionKey from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots from xmodule.modulestore.xml import LibraryXMLModuleStore, XMLImportingModuleStoreRuntime, XMLModuleStore @@ -297,6 +299,10 @@ def __init__( ) self.logger, self.errors = make_error_tracker() + # Here we introduce an approach where we collect all gating relationship information in a course + # and persist at the end. For this we have introduced a new gating_relationships array property. + self.gating_relationships = [] + def preflight(self): """ Perform any pre-import sanity checks. @@ -437,6 +443,7 @@ def import_courselike(self, runtime, courselike_key, dest_id, source_courselike) dest_id, do_import_static=self.do_import_static, runtime=runtime, + gating_relationships=self.gating_relationships, ) self.static_updater(course, source_courselike, courselike_key, dest_id, runtime) self.store.update_item(course, self.user_id) @@ -519,6 +526,7 @@ def depth_first(subtree): dest_id, do_import_static=self.do_import_static, runtime=courselike.runtime, + gating_relationships=self.gating_relationships, ) except Exception: log.exception( @@ -543,6 +551,7 @@ def depth_first(subtree): dest_id, do_import_static=self.do_import_static, runtime=courselike.runtime, + gating_relationships=self.gating_relationships, ) except Exception: log.exception( @@ -567,6 +576,10 @@ def run_imports(self): except DuplicateCourseError: continue + # Prerequisite records are specific to a course, therefore, we need to reset + # the relationship information that has been collected on the previous course. + self.gating_relationships = [] + # This bulk operation wraps all the operations to populate the published branch. with self.store.bulk_operations(dest_id): # Retrieve the course itself. @@ -597,6 +610,15 @@ def run_imports(self): logging.info(f'Course import {dest_id}: No tags.csv file present.') except ValueError as e: logging.info(f'Course import {dest_id}: {str(e)}') + + # After collecting all gating relationships, here we will try to persist + # the relationship records ensuring all validation rules are applied. + # If the there is invalid information, we can take measures (skip registering + # the relationship.) + if self.gating_relationships: + _apply_gating_relationships(self.store, dest_id, self.gating_relationships) + self.gating_relationships = [] + self.post_course_import(dest_id) yield courselike @@ -711,7 +733,8 @@ def import_drafts(self, courselike, courselike_key, data_path, dest_id): data_path, courselike_key, dest_id, - courselike.runtime + courselike.runtime, + self.gating_relationships, ) # Importing the drafts potentially triggered a new structure version. @@ -834,10 +857,86 @@ def import_library_from_xml(*args, **kwargs): return list(manager.run_imports()) +def _process_sequential_prerequisites(block): + """ + Extracts sequential prerequisite information, does basic validation and + returns the relationship information as a tuple. + + Args: + block: The sequential block + """ + if not hasattr(block, "xml_attributes") or not block.xml_attributes: + return None + + try: + required_content = block.xml_attributes.get('required_content') + if not isinstance(required_content, str): + return None + + min_score = block.xml_attributes.get('min_score') + min_completion = block.xml_attributes.get('min_completion') + if not min_score or not min_completion: + return None + + min_score = int(min_score) + min_completion = int(min_completion) + + except (ValueError, TypeError) as e: + logging.debug('Failed to extract valid required_content, min_score, and/or min_completion: %s', e) + return None + + course_key = block.location.course_key + prerequisite_usage_key = course_key.make_usage_key('sequential', required_content) + + return (block.location, prerequisite_usage_key, min_score, min_completion) + + +def _apply_gating_relationships(store, course_key, gating_relationships): + """ + Validates that each prerequisite block exists before calling the gating API to persist the gating relationship, + logs errors and ignores invalid gating relationship. + + Args: + store: modulestore + course_key: Course key + gating_relationships: List of tuples (block_location, prereq_usage_key, min_score, min_completion) + """ + for block_location, prereq_usage_key, min_score, min_completion in gating_relationships: + # Here we validate and store records that are valid, in this case validate if + # sequantial with the specified ID exists. + try: + store.get_item(prereq_usage_key) # raises ItemNotFoundError if missing + except ItemNotFoundError: + logging.error( + "Prerequisite block '%s' referenced by '%s' does not exist.", + prereq_usage_key, block_location + ) + continue + + try: + gating_api.add_prerequisite(course_key, prereq_usage_key) + gating_api.set_required_content( + course_key, + block_location, + prereq_usage_key, + min_score, + min_completion + ) + except GatingValidationError as e: + logging.error( + "Error invalid block %s with prerequisite %s", + block_location, prereq_usage_key + ) + logging.error( + "Error : %s", + e + ) + + def _update_and_import_block( # pylint: disable=too-many-statements block, store, user_id, source_course_id, dest_course_id, - do_import_static=True, runtime=None): + do_import_static=True, runtime=None, gating_relationships=None): """ Update all the block reference fields to the destination course id, then import the block into the destination course. @@ -918,6 +1017,15 @@ def _convert_ref_fields_to_new_namespace(reference): block.location.block_id, fields, runtime, asides=asides ) + # As the order of course blocks is unpredictable during import, we only collect sequantials + # and store the gating relationship at the end. This is to enfirce validation and + # persist relationship if only the referenced sequentials exist in our database. + # We do this only if the current block is sequential and gating_relationships is iitialized. + if block.location.block_type == "sequential" and gating_relationships is not None: + gating_relationship_data = _process_sequential_prerequisites(block) + if gating_relationship_data: + gating_relationships.append(gating_relationship_data) + # 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. @@ -980,7 +1088,8 @@ def _import_course_draft( course_data_path, source_course_id, target_id, - mongo_runtime + mongo_runtime, + gating_relationships=None ): """ This method will import all the content inside of the 'drafts' folder, if content exists. @@ -1050,6 +1159,7 @@ def _import_block(block): source_course_id, target_id, runtime=mongo_runtime, + gating_relationships=gating_relationships, ) for child in block.get_children(): _import_block(child) diff --git a/xmodule/seq_block.py b/xmodule/seq_block.py index 71987f4d616f..e4e31c4384d2 100644 --- a/xmodule/seq_block.py +++ b/xmodule/seq_block.py @@ -969,8 +969,27 @@ 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_prerequisite_to_xml(xml_object) return xml_object + def add_prerequisite_to_xml(self, xml_object): + """ + Add prerequisite information to sequential XML for export. + """ + from openedx.core.lib.gating import api as gating_api + + prereq_info = gating_api.get_required_content( + self.location.course_key, + self.location + ) + prereq_usage_key_str, min_score, min_completion = prereq_info + if not isinstance(prereq_usage_key_str, str): + return + prereq_usage_key = UsageKey.from_string(prereq_usage_key_str) + xml_object.set('required_content', prereq_usage_key.block_id) + xml_object.set('min_score', str(min_score)) + xml_object.set('min_completion', str(min_completion)) + @property def non_editable_metadata_fields(self): """ diff --git a/xmodule/tests/test_sequence.py b/xmodule/tests/test_sequence.py index 3901fc7a09e5..a3c862514899 100644 --- a/xmodule/tests/test_sequence.py +++ b/xmodule/tests/test_sequence.py @@ -15,6 +15,7 @@ from django.utils.timezone import now from edx_toggles.toggles.testutils import override_waffle_flag from freezegun import freeze_time +from lxml import etree from web_fragments.fragment import Fragment from openedx.features.content_type_gating.models import ContentTypeGatingConfig @@ -572,3 +573,43 @@ def test_prereqs_met_content_rendered_normally(self): assert metadata["items"] == "rendered_blocks" assert metadata["next_url"] == "next_url" assert metadata["prev_url"] == "prev_url" + + @patch("openedx.core.lib.gating.api.get_required_content") + def test_export_sequence_with_prerequisites(self, mock_get_required_content): + """ + Test that add_prerequisite_to_xml adds prerequisite information correctly. + """ + xml_object = etree.Element('sequential') + + mock_get_required_content.return_value = ( + "block-v1:TestX+TestCourse+1+type@sequential+block@required_sequential", + 80, + 90, + ) + self.sequence_3_1.add_prerequisite_to_xml(xml_object) + mock_get_required_content.assert_called_once_with( + self.sequence_3_1.location.course_key, + self.sequence_3_1.location + ) + assert xml_object.get('required_content') == 'required_sequential' + assert xml_object.get('min_score') == '80' + assert xml_object.get('min_completion') == '90' + + @patch("openedx.core.lib.gating.api.get_required_content") + def test_export_sequence_no_prerequisites(self, mock_get_required_content): + """ + Test that add_prerequisite_to_xml adds No prerequisite information if + current sequential is not gated. + """ + xml_object = etree.Element('sequential') + + mock_get_required_content.return_value = (None, None, None) + + self.sequence_3_1.add_prerequisite_to_xml(xml_object) + mock_get_required_content.assert_called_once_with( + self.sequence_3_1.location.course_key, + self.sequence_3_1.location + ) + assert 'required_content' not in xml_object.attrib + assert 'min_score' not in xml_object.attrib + assert 'min_completion' not in xml_object.attrib