From 51ac947caf9dd576abd57f2b0998b85330f149ce Mon Sep 17 00:00:00 2001 From: Muhammad Arslan Date: Sat, 27 Sep 2025 05:58:12 +0500 Subject: [PATCH] fix: always generate block structure from "published-only" branch (#37335) Block structures are meant to be an optimization for the LMS, meaning that they should always be collecting from the published branch of modulestore. This is what happens by default when it's run from the LMS celery process, but this code is sometimes invoked from a Studio worker (e.g. development mode celery, running in immediate in-proc mode). --------- Co-authored-by: Peter Pinch --- .../courseware/tests/test_courses.py | 2 +- lms/djangoapps/grades/tests/test_tasks.py | 15 ++++---- .../content/block_structure/manager.py | 17 ++++++--- .../content/block_structure/tests/helpers.py | 7 ++++ .../block_structure/tests/test_manager.py | 37 +++++++++++++++++++ 5 files changed, 65 insertions(+), 13 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py index 804f903779..72465b299a 100644 --- a/lms/djangoapps/courseware/tests/test_courses.py +++ b/lms/djangoapps/courseware/tests/test_courses.py @@ -88,7 +88,7 @@ class CoursesTest(ModuleStoreTestCase): 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 diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 347879105a..8f32991272 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -156,8 +156,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest 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 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest 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 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest 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 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest 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): diff --git a/openedx/core/djangoapps/content/block_structure/manager.py b/openedx/core/djangoapps/content/block_structure/manager.py index 49f423ce7a..b6a447bdf2 100644 --- a/openedx/core/djangoapps/content/block_structure/manager.py +++ b/openedx/core/djangoapps/content/block_structure/manager.py @@ -6,6 +6,8 @@ BlockStructures. 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 @@ class BlockStructureManager: 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 @@ class BlockStructureManager: 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 f0a20554e6..df53b33d7b 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 @@ class MockModulestore: """ 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 493e4c3471..1c4060871c 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 @@ Tests for manager.py import pytest import ddt +from unittest.mock import MagicMock from django.test import TestCase +from xmodule.modulestore import ModuleStoreEnum + from ..block_structure import BlockStructureBlockData from ..exceptions import UsageKeyNotInBlockStructure from ..manager import BlockStructureManager @@ -216,3 +219,37 @@ class TestBlockStructureManager(UsageKeyFactoryMixin, ChildrenMapTestMixin, Test self.bs_manager.clear() self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True) assert TestTransformer1.collect_call_count == 2 + + 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 + attr_name = 'branch_setting' + original_branch_setting = getattr(self.modulestore, attr_name, 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 + setattr(self.modulestore, attr_name, mock_branch_setting) + + try: + with mock_registered_transformers(self.registered_transformers): + self.bs_manager.get_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 is not None: + setattr(self.modulestore, attr_name, original_branch_setting) + elif hasattr(self.modulestore, attr_name): + delattr(self.modulestore, attr_name)