fix: Update on_commit_changes_to of modulestore to check MySQL transaction [FC-0097] (#37485)

- `handle_update_xblock_upstream_link` is called asynchronously with celery. In `update_upstream_downstream_link_handler`, the xblock has the updated version, but when calling `handle_update_xblock_upstream_link` inside Celery, the xblock is outdated in a previous version, which is why the error occurs. This happens because `on_commit_changes_to` is executed before the MySQL transaction ends.
- Added `ImmediateOnCommitMixin` to be used in tests that need to call `on_commit_changes_to`. See #37485 (comment)  for more info

Author: ChrisChV

cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py

@@ -13,12 +13,12 @@
 from openedx.core.djangoapps.content_libraries.tests import ContentLibrariesRestApiTest
 from cms.djangoapps.contentstore.xblock_storage_handlers.xblock_helpers import get_block_key_string
 from xmodule.modulestore.django import modulestore
-from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, ImmediateOnCommitMixin
 from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory
 
 
 @ddt.ddt
-class CourseToLibraryTestCase(ContentLibrariesRestApiTest, ModuleStoreTestCase):
+class CourseToLibraryTestCase(ContentLibrariesRestApiTest, ImmediateOnCommitMixin, ModuleStoreTestCase):
     """
     Tests that involve syncing content from libraries to courses.
     """

cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py

@@ -23,7 +23,7 @@
 from common.djangoapps.student.tests.factories import UserFactory
 from openedx.core.djangoapps.content_libraries import api as lib_api
 from xmodule.modulestore.django import modulestore
-from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
+from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase, ImmediateOnCommitMixin
 from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory
 
 from .. import downstreams as downstreams_views
@@ -406,7 +406,7 @@ def test_400(self, sync: str):
         assert video_after.upstream is None
 
 
-class DeleteDownstreamViewTest(SharedErrorTestCases, SharedModuleStoreTestCase):
+class DeleteDownstreamViewTest(SharedErrorTestCases, ImmediateOnCommitMixin, SharedModuleStoreTestCase):
     """
     Test that `DELETE /api/v2/contentstore/downstreams/...` severs a downstream's link to an upstream.
     """
@@ -596,6 +596,7 @@ def test_204(self, mock_decline_sync):
 @ddt.ddt
 class GetUpstreamViewTest(
     _BaseDownstreamViewTestMixin,
+    ImmediateOnCommitMixin,
     SharedModuleStoreTestCase,
 ):
     """
@@ -1424,6 +1425,7 @@ def test_200_get_ready_to_sync_duplicated_top_level_parents(self):
 
 class GetDownstreamSummaryViewTest(
     _BaseDownstreamViewTestMixin,
+    ImmediateOnCommitMixin,
     SharedModuleStoreTestCase,
 ):
     """

cms/djangoapps/contentstore/tests/test_upstream_downstream_links.py

@@ -14,7 +14,7 @@
 
 from common.djangoapps.student.tests.factories import UserFactory
 from openedx.core.djangolib.testing.utils import skip_unless_cms
-from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, ImmediateOnCommitMixin
 from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory
 
 from ..models import ContainerLink, LearningContextLinksStatus, LearningContextLinksStatusChoices, ComponentLink
@@ -265,7 +265,12 @@ def test_call_for_nonexistent_course(self):
 
 
 @skip_unless_cms
-class TestUpstreamLinksEvents(ModuleStoreTestCase, OpenEdxEventsTestMixin, BaseUpstreamLinksHelpers):
+class TestUpstreamLinksEvents(
+    ImmediateOnCommitMixin,
+    ModuleStoreTestCase,
+    OpenEdxEventsTestMixin,
+    BaseUpstreamLinksHelpers,
+):
     """
     Test signals related to managing upstream->downstream links.
     """

cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py

@@ -16,7 +16,7 @@
 from openedx_tagging.core.tagging.models import Tag
 from organizations.models import Organization
 from xmodule.modulestore.django import contentstore, modulestore
-from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course, ImmediateOnCommitMixin
 from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, ToyCourseFactory, LibraryFactory
 
 from cms.djangoapps.contentstore.utils import reverse_usage_url
@@ -400,7 +400,7 @@ def test_paste_with_assets(self):
         assert source_pic2_hash != dest_pic2_hash  # Because there was a conflict, this file was unchanged.
 
 
-class ClipboardPasteFromV2LibraryTestCase(OpenEdxEventsTestMixin, ModuleStoreTestCase):
+class ClipboardPasteFromV2LibraryTestCase(OpenEdxEventsTestMixin, ImmediateOnCommitMixin, ModuleStoreTestCase):
     """
     Test Clipboard Paste functionality with a "new" (as of Sumac) library
     """

openedx/core/djangoapps/content/course_overviews/tests/test_signals.py

@@ -13,7 +13,11 @@
 
 from xmodule.data import CertificatesDisplayBehaviors
 from xmodule.modulestore import ModuleStoreEnum
-from xmodule.modulestore.tests.django_utils import TEST_DATA_ONLY_SPLIT_MODULESTORE_DRAFT_PREFERRED, ModuleStoreTestCase
+from xmodule.modulestore.tests.django_utils import (
+    TEST_DATA_ONLY_SPLIT_MODULESTORE_DRAFT_PREFERRED,
+    ModuleStoreTestCase,
+    ImmediateOnCommitMixin,
+)
 from xmodule.modulestore.tests.factories import CourseFactory, check_mongo_calls
 
 from ..models import CourseOverview
@@ -23,7 +27,7 @@
 
 
 @ddt.ddt
-class CourseOverviewSignalsTestCase(ModuleStoreTestCase):
+class CourseOverviewSignalsTestCase(ImmediateOnCommitMixin, ModuleStoreTestCase):
     """
     Tests for CourseOverview signals.
     """
@@ -43,16 +47,14 @@ def assert_changed_signal_sent(self, changes, mock_signal):
         )
 
         # changing display name doesn't fire the signal
-        with self.captureOnCommitCallbacks(execute=True) as callbacks:
-            course.display_name = course.display_name + 'changed'
-            course = self.store.update_item(course, ModuleStoreEnum.UserID.test)
+        course.display_name = course.display_name + 'changed'
+        course = self.store.update_item(course, ModuleStoreEnum.UserID.test)
         assert not mock_signal.called
 
         # changing the given field fires the signal
-        with self.captureOnCommitCallbacks(execute=True) as callbacks:
-            for change in changes:
-                setattr(course, change.field_name, change.changed_value)
-            self.store.update_item(course, ModuleStoreEnum.UserID.test)
+        for change in changes:
+            setattr(course, change.field_name, change.changed_value)
+        self.store.update_item(course, ModuleStoreEnum.UserID.test)
         assert mock_signal.called
 
     def test_caching(self):

openedx/core/djangoapps/content/search/tests/test_handlers.py

@@ -11,7 +11,11 @@
 from common.djangoapps.student.tests.factories import UserFactory
 from openedx.core.djangoapps.content_libraries import api as library_api
 from openedx.core.djangolib.testing.utils import skip_unless_cms
-from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase
+from xmodule.modulestore.tests.django_utils import (
+    TEST_DATA_SPLIT_MODULESTORE,
+    ModuleStoreTestCase,
+    ImmediateOnCommitMixin,
+)
 
 
 try:
@@ -26,7 +30,7 @@
 @patch("openedx.core.djangoapps.content.search.api.MeilisearchClient")
 @override_settings(MEILISEARCH_ENABLED=True)
 @skip_unless_cms
-class TestUpdateIndexHandlers(ModuleStoreTestCase, LiveServerTestCase):
+class TestUpdateIndexHandlers(ImmediateOnCommitMixin, ModuleStoreTestCase, LiveServerTestCase):
     """
     Test that the search index is updated when XBlocks and Library Blocks are modified
     """
@@ -80,7 +84,9 @@ def test_create_delete_xblock(self, meilisearch_client):
             "access_id": course_access.id,
             "modified": created_date.timestamp(),
         }
+
         meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_sequential])
+
         with freeze_time(created_date):
             vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical")
         doc_vertical = {
@@ -119,6 +125,7 @@ def test_create_delete_xblock(self, meilisearch_client):
         doc_sequential["display_name"] = "Updated Sequential"
         doc_vertical["breadcrumbs"][1]["display_name"] = "Updated Sequential"
         doc_sequential["modified"] = modified_date.timestamp()
+
         meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([
             doc_sequential,
             doc_vertical,

openedx/core/djangoapps/content_tagging/tests/test_tasks.py

@@ -13,7 +13,11 @@
 
 from common.djangoapps.student.tests.factories import UserFactory
 from openedx.core.djangolib.testing.utils import skip_unless_cms
-from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase
+from xmodule.modulestore.tests.django_utils import (
+    TEST_DATA_SPLIT_MODULESTORE,
+    ModuleStoreTestCase,
+    ImmediateOnCommitMixin,
+)
 from openedx.core.djangoapps.content_libraries.api import (
     create_library, create_library_block, delete_library_block, restore_library_block
 )
@@ -59,6 +63,7 @@ def setUp(self):
 @override_waffle_flag(CONTENT_TAGGING_AUTO, active=True)
 class TestAutoTagging(  # type: ignore[misc]
     LanguageTaxonomyTestMixin,
+    ImmediateOnCommitMixin,
     ModuleStoreTestCase,
     LiveServerTestCase
 ):

xmodule/modulestore/__init__.py

@@ -12,6 +12,7 @@
 from collections import defaultdict
 from contextlib import contextmanager
 from operator import itemgetter
+from django.db import transaction
 
 from opaque_keys.edx.keys import AssetKey, CourseKey
 from opaque_keys.edx.locations import Location  # For import backwards compatibility
@@ -322,16 +323,20 @@ def on_commit_changes_to(self, course_key, fn):
         """
         Call some callback when the currently active bulk operation has saved
         """
+        # If we're in a MySQL transaction, so the new version will only be committed to the
+        # SplitModulestoreCourseIndex table after the MySQL transaction is closed.
+        def wrapped_fn():
+            transaction.on_commit(fn)
         # Check if a bulk op is active. If so, defer fn(); otherwise call it immediately.
         # Note: calling _get_bulk_ops_record() here and then checking .active can have side-effects in some cases
         # because it creates an entry in the defaultdict if none exists, so we check if the record is active using
         # the same code as _clear_bulk_ops_record(), which doesn't modify the defaultdict.
         # so we check it this way:
         if course_key and course_key.for_branch(None) in self._active_bulk_ops.records:
             bulk_ops_record = self._active_bulk_ops.records[course_key.for_branch(None)]
-            bulk_ops_record.defer_until_commit(fn)
+            bulk_ops_record.defer_until_commit(wrapped_fn)
         else:
-            fn()  # There is no active bulk operation - call fn() now.
+            wrapped_fn()  # There is no active bulk operation - call wrapped_fn() now.
 
     def _is_in_bulk_operation(self, course_key, ignore_case=False):
         """

xmodule/modulestore/tests/django_utils.py

@@ -613,6 +613,35 @@ def update_course(self, course, user_id):
         return updated_course
 
 
+class ImmediateOnCommitMixin:
+    """
+    Mixin for tests that want `on_commit` callbacks to run immediately,
+    even under TestCase (which normally wraps tests in a transaction
+    that never commits).
+    Especially useful when the test needs to execute an event that occurs after an `on_commit`
+    """
+
+    @classmethod
+    def setUpClass(cls):
+        super_cls = super()
+        if hasattr(super_cls, 'setUpClass'):
+            super_cls.setUpClass()
+        # Patch `transaction.on_commit` so that callbacks run immediately
+        cls._on_commit_patcher = patch(
+            'django.db.transaction.on_commit',
+            side_effect=lambda func, **kwargs: func()
+        )
+        cls._on_commit_patcher.start()
+
+    @classmethod
+    def tearDownClass(cls):
+        # Stop patching, restore original behavior
+        cls._on_commit_patcher.stop()
+        super_cls = super()
+        if hasattr(super_cls, 'tearDownClass'):
+            super_cls.tearDownClass()
+
+
 def upload_file_to_course(course_key, contentstore, source_file, target_filename):
     '''
     Uploads the given source file to the given course, and returns the content of the file.

xmodule/tests/__init__.py

@@ -13,7 +13,7 @@
 from functools import wraps
 from unittest.mock import Mock
 
-from django.test import TestCase
+from django.test import TransactionTestCase
 
 from opaque_keys.edx.keys import CourseKey
 from path import Path as path
@@ -306,7 +306,7 @@ def __getitem__(self, index):
         return str(self)[index]
 
 
-class CourseComparisonTest(TestCase):
+class CourseComparisonTest(TransactionTestCase):
     """
     Mixin that has methods for comparing courses for equality.
     """