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 <[email protected]>

Author: marslanabdulrauf

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

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

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

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

openedx/core/djangoapps/content/block_structure/tests/test_manager.py

@@ -4,8 +4,11 @@
 
 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 @@ 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
+
+    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)