From 17506638f438a5485bf7daa9777d702e0c515d8d Mon Sep 17 00:00:00 2001 From: Muhammad Arslan Abdul Rauf Date: Tue, 9 Sep 2025 18:25:54 +0500 Subject: [PATCH 1/5] fix: always generate block structure from published-only branch --- .../content/block_structure/manager.py | 17 ++++-- .../content/block_structure/tests/helpers.py | 7 +++ .../block_structure/tests/test_manager.py | 59 +++++++++++++++++++ 3 files changed, 78 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/content/block_structure/manager.py b/openedx/core/djangoapps/content/block_structure/manager.py index 49f423ce7ac3..b6a447bdf201 100644 --- a/openedx/core/djangoapps/content/block_structure/manager.py +++ b/openedx/core/djangoapps/content/block_structure/manager.py @@ -6,6 +6,8 @@ from contextlib import contextmanager +from xmodule.modulestore import ModuleStoreEnum + from .exceptions import BlockStructureNotFound, TransformerDataIncompatible, UsageKeyNotInBlockStructure from .factory import BlockStructureFactory from .store import BlockStructureStore @@ -104,7 +106,6 @@ def get_collected(self, user=None): self.store, ) BlockStructureTransformers.verify_versions(block_structure) - except (BlockStructureNotFound, TransformerDataIncompatible): if user and getattr(user, "known", True): # This bypasses the runtime access checks. When we are populating the course blocks cache, @@ -133,10 +134,16 @@ def _update_collected(self): the modulestore. """ with self._bulk_operations(): - block_structure = BlockStructureFactory.create_from_modulestore( - self.root_block_usage_key, - self.modulestore, - ) + # Always uses published-only branch regardless of CMS or LMS context. + with self.modulestore.branch_setting( + ModuleStoreEnum.Branch.published_only, + self.root_block_usage_key.course_key + ): + block_structure = BlockStructureFactory.create_from_modulestore( + self.root_block_usage_key, + self.modulestore, + ) + BlockStructureTransformers.collect(block_structure) self.store.add(block_structure) return block_structure diff --git a/openedx/core/djangoapps/content/block_structure/tests/helpers.py b/openedx/core/djangoapps/content/block_structure/tests/helpers.py index f0a20554e6d2..df53b33d7b2e 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/helpers.py +++ b/openedx/core/djangoapps/content/block_structure/tests/helpers.py @@ -110,6 +110,13 @@ def bulk_operations(self, ignore): # pylint: disable=unused-argument """ yield + @contextmanager + def branch_setting(self, branch_settings, course_id=None): # pylint: disable=unused-argument + """ + A context manager for temporarily setting a store's branch value on the current thread. + """ + yield + class MockCache: """ diff --git a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py index 493e4c34714f..97b82e816a45 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py +++ b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py @@ -4,8 +4,11 @@ import pytest import ddt +from unittest.mock import patch, MagicMock from django.test import TestCase +from xmodule.modulestore import ModuleStoreEnum + from ..block_structure import BlockStructureBlockData from ..exceptions import UsageKeyNotInBlockStructure from ..manager import BlockStructureManager @@ -13,6 +16,7 @@ from .helpers import ( ChildrenMapTestMixin, MockCache, + MockModulestore, MockModulestoreFactory, MockTransformer, UsageKeyFactoryMixin, @@ -216,3 +220,58 @@ def test_clear(self): self.bs_manager.clear() self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True) assert TestTransformer1.collect_call_count == 2 + + @patch.object(MockModulestore, 'branch_setting') + def test_update_collected_uses_published_only_branch(self, mock_branch_setting): + """ + Test that _update_collected always uses published-only branch. + """ + # Set up the mock to return a proper context manager + mock_context_manager = MagicMock() + mock_context_manager.__enter__ = MagicMock(return_value=None) + mock_context_manager.__exit__ = MagicMock(return_value=None) + mock_branch_setting.return_value = mock_context_manager + + # Call _update_collected + with mock_registered_transformers(self.registered_transformers): + self.bs_manager._update_collected() + + # Verify branch_setting was called with published_only + mock_branch_setting.assert_called_once_with( + ModuleStoreEnum.Branch.published_only, + self.block_key_factory(0).course_key + ) + + def test_update_collected_branch_context_integration(self): + """ + Integration test to verify the published-only branch context works end-to-end. + """ + # Track branch setting calls on our mock modulestore + original_branch_setting = getattr(self.modulestore, 'branch_setting', None) + branch_setting_calls = [] + + def mock_branch_setting(branch, course_key): + branch_setting_calls.append((branch, course_key)) + # Return a proper context manager that does nothing + return MagicMock(__enter__=MagicMock(), __exit__=MagicMock()) + + # Add the branch_setting method to our mock modulestore + self.modulestore.branch_setting = mock_branch_setting + + try: + # Call _update_collected + with mock_registered_transformers(self.registered_transformers): + self.bs_manager._update_collected() + + # Verify branch_setting was called with the correct parameters + self.assertEqual(len(branch_setting_calls), 1) + branch, course_key = branch_setting_calls[0] + self.assertEqual(branch, ModuleStoreEnum.Branch.published_only) + self.assertEqual(course_key, self.block_key_factory(0).course_key) + + finally: + # Restore original method if it existed + if original_branch_setting: + self.modulestore.branch_setting = original_branch_setting + elif hasattr(self.modulestore, 'branch_setting'): + delattr(self.modulestore, 'branch_setting') From 6d151a1e42c1752c54efe94673a471367bbf499e Mon Sep 17 00:00:00 2001 From: Muhammad Arslan Abdul Rauf Date: Tue, 9 Sep 2025 20:34:54 +0500 Subject: [PATCH 2/5] fix: pylint error fixed --- .../block_structure/tests/test_manager.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py index 97b82e816a45..66553a35958d 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py +++ b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py @@ -232,9 +232,8 @@ def test_update_collected_uses_published_only_branch(self, mock_branch_setting): mock_context_manager.__exit__ = MagicMock(return_value=None) mock_branch_setting.return_value = mock_context_manager - # Call _update_collected with mock_registered_transformers(self.registered_transformers): - self.bs_manager._update_collected() + self.bs_manager.get_collected() # Verify branch_setting was called with published_only mock_branch_setting.assert_called_once_with( @@ -247,7 +246,8 @@ def test_update_collected_branch_context_integration(self): Integration test to verify the published-only branch context works end-to-end. """ # Track branch setting calls on our mock modulestore - original_branch_setting = getattr(self.modulestore, 'branch_setting', None) + attr_name = 'branch_setting' + original_branch_setting = getattr(self.modulestore, attr_name, None) branch_setting_calls = [] def mock_branch_setting(branch, course_key): @@ -256,12 +256,11 @@ def mock_branch_setting(branch, course_key): return MagicMock(__enter__=MagicMock(), __exit__=MagicMock()) # Add the branch_setting method to our mock modulestore - self.modulestore.branch_setting = mock_branch_setting + setattr(self.modulestore, attr_name, mock_branch_setting) try: - # Call _update_collected with mock_registered_transformers(self.registered_transformers): - self.bs_manager._update_collected() + self.bs_manager.get_collected() # Verify branch_setting was called with the correct parameters self.assertEqual(len(branch_setting_calls), 1) @@ -271,7 +270,7 @@ def mock_branch_setting(branch, course_key): finally: # Restore original method if it existed - if original_branch_setting: - self.modulestore.branch_setting = original_branch_setting - elif hasattr(self.modulestore, 'branch_setting'): - delattr(self.modulestore, 'branch_setting') + if original_branch_setting is not None: + setattr(self.modulestore, attr_name, original_branch_setting) + elif hasattr(self.modulestore, attr_name): + delattr(self.modulestore, attr_name) From 302efa806c5290f1fff1cc7f970bf63c74e618e7 Mon Sep 17 00:00:00 2001 From: Muhammad Arslan Abdul Rauf Date: Wed, 10 Sep 2025 16:38:08 +0500 Subject: [PATCH 3/5] test: fix grades test, course_structure will only get published-only content now --- lms/djangoapps/grades/tests/test_tasks.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 347879105aad..8f32991272e2 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -156,8 +156,8 @@ def test_block_structure_created_only_once(self): assert mock_block_structure_create.call_count == 1 @ddt.data( - (ModuleStoreEnum.Type.split, 2, 42, True), - (ModuleStoreEnum.Type.split, 2, 42, False), + (ModuleStoreEnum.Type.split, 1, 42, True), + (ModuleStoreEnum.Type.split, 1, 42, False), ) @ddt.unpack def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections): @@ -168,7 +168,7 @@ def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, creat self._apply_recalculate_subsection_grade() @ddt.data( - (ModuleStoreEnum.Type.split, 2, 42), + (ModuleStoreEnum.Type.split, 1, 42), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -200,16 +200,17 @@ def test_other_inaccessible_subsection(self, mock_subsection_signal): with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, self.course.id): self.store.update_item(sequential, self.user.id) - # Make sure the signal is sent for only the 2 accessible sequentials. + # Make sure the signal is sent for only the 1 accessible sequentials. + # Update: draft branch content shouldn't be accessible self._apply_recalculate_subsection_grade() - assert mock_subsection_signal.call_count == 2 + assert mock_subsection_signal.call_count == 1 sequentials_signalled = { args[1]['subsection_grade'].location for args in mock_subsection_signal.call_args_list } self.assertSetEqual( sequentials_signalled, - {self.sequential.location, accessible_seq.location}, + {self.sequential.location}, ) @patch('lms.djangoapps.grades.signals.signals.SUBSECTION_SCORE_CHANGED.send') @@ -255,7 +256,7 @@ def test_problem_block_with_restricted_access(self, mock_subsection_signal): UserPartition.scheme_extensions = None @ddt.data( - (ModuleStoreEnum.Type.split, 2, 42), + (ModuleStoreEnum.Type.split, 1, 42), ) @ddt.unpack def test_persistent_grades_on_course(self, default_store, num_mongo_queries, num_sql_queries): From 5a60679787dc22b6d21c43dd1bd0aa0d48cb2830 Mon Sep 17 00:00:00 2001 From: Muhammad Arslan Abdul Rauf Date: Wed, 10 Sep 2025 17:29:57 +0500 Subject: [PATCH 4/5] test: query count is 1 when we get from published-only branch --- lms/djangoapps/courseware/tests/test_courses.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py index 804f9037791f..72465b299ade 100644 --- a/lms/djangoapps/courseware/tests/test_courses.py +++ b/lms/djangoapps/courseware/tests/test_courses.py @@ -88,7 +88,7 @@ def test_get_course_func_with_access_error(self, course_access_func_name): assert not error.value.access_response.has_access @ddt.data( - (GET_COURSE_WITH_ACCESS, 2), + (GET_COURSE_WITH_ACCESS, 1), (GET_COURSE_OVERVIEW_WITH_ACCESS, 0), ) @ddt.unpack From e3ad27335339d93880ad6f0f1c56aee892c97fa1 Mon Sep 17 00:00:00 2001 From: Muhammad Arslan Abdul Rauf Date: Thu, 18 Sep 2025 14:19:59 +0500 Subject: [PATCH 5/5] test: remove redundant mocked test in favor of existing e2e test --- .../block_structure/tests/test_manager.py | 23 +------------------ 1 file changed, 1 insertion(+), 22 deletions(-) diff --git a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py index 66553a35958d..1c4060871c97 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py +++ b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py @@ -4,7 +4,7 @@ import pytest import ddt -from unittest.mock import patch, MagicMock +from unittest.mock import MagicMock from django.test import TestCase from xmodule.modulestore import ModuleStoreEnum @@ -16,7 +16,6 @@ from .helpers import ( ChildrenMapTestMixin, MockCache, - MockModulestore, MockModulestoreFactory, MockTransformer, UsageKeyFactoryMixin, @@ -221,26 +220,6 @@ def test_clear(self): self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True) assert TestTransformer1.collect_call_count == 2 - @patch.object(MockModulestore, 'branch_setting') - def test_update_collected_uses_published_only_branch(self, mock_branch_setting): - """ - Test that _update_collected always uses published-only branch. - """ - # Set up the mock to return a proper context manager - mock_context_manager = MagicMock() - mock_context_manager.__enter__ = MagicMock(return_value=None) - mock_context_manager.__exit__ = MagicMock(return_value=None) - mock_branch_setting.return_value = mock_context_manager - - with mock_registered_transformers(self.registered_transformers): - self.bs_manager.get_collected() - - # Verify branch_setting was called with published_only - mock_branch_setting.assert_called_once_with( - ModuleStoreEnum.Branch.published_only, - self.block_key_factory(0).course_key - ) - def test_update_collected_branch_context_integration(self): """ Integration test to verify the published-only branch context works end-to-end.