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 <pdpinch@mit.edu>

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

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):

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

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:
     """

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)