Merge pull request #34070 from openedx/ashultz0/reindex-on-commit

Make re-indexing wait for the data using on_commit

cms/djangoapps/contentstore/signals/handlers.py

@@ -127,6 +127,17 @@ def listen_for_course_publish(sender, course_key, **kwargs):  # pylint: disable=
         dump_course_to_neo4j
     )
 
+    # DEVELOPER README: probably all tasks here should use transaction.on_commit
+    # to avoid stale data, but the tasks are owned by many teams and are often
+    # working well enough. Several instead use a waiting strategy.
+    # If you are in here trying to figure out why your task is not working correctly,
+    # consider whether it is getting stale data and if so choose to wait for the transaction
+    # like exams or put your task to sleep for a while like discussions.
+    # You will not be able to replicate these errors in an environment where celery runs
+    # in process because it will be inside the transaction. Use the settings from
+    # devstack_with_worker.py, and consider adding a time.sleep into send_bulk_published_signal
+    # if you really want to make sure that the task happens before the data is ready.
+
     # register special exams asynchronously after the data is ready
     course_key_str = str(course_key)
     transaction.on_commit(lambda: update_special_exams_and_publish.delay(course_key_str))
@@ -139,10 +150,9 @@ def listen_for_course_publish(sender, course_key, **kwargs):  # pylint: disable=
         # Push the course out to CourseGraph asynchronously.
         dump_course_to_neo4j.delay(course_key_str)
 
-    # Finally, call into the course search subsystem
-    # to kick off an indexing action
+    # Kick off a courseware indexing action after the data is ready
     if CoursewareSearchIndexer.indexing_is_enabled() and CourseAboutSearchIndexer.indexing_is_enabled():
-        update_search_index.delay(course_key_str, datetime.now(UTC).isoformat())
+        transaction.on_commit(lambda: update_search_index.delay(course_key_str, datetime.now(UTC).isoformat()))
 
     update_discussions_settings_from_course_task.apply_async(
         args=[course_key_str],

cms/djangoapps/contentstore/tests/test_courseware_index.py

@@ -5,7 +5,7 @@ import json
 import time
 from datetime import datetime
 from unittest import skip
-from unittest.mock import patch
+from unittest.mock import patch, Mock
 
 import ddt
 import pytest
@@ -585,6 +585,8 @@ class TestLargeCourseDeletions(MixedWithOptionsTestCase):
         self._test_large_course_deletion(self.store)
 
 
+@patch('cms.djangoapps.contentstore.signals.handlers.transaction.on_commit',
+       new=Mock(side_effect=lambda func: func()),)  # run right away
 class TestTaskExecution(SharedModuleStoreTestCase):
     """
     Set of tests to ensure that the task code will do the right thing when

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

@@ -6,7 +6,6 @@ Unit tests for getting the list of courses and the course outline.
 import datetime
 import json
 from unittest import mock, skip
-from unittest.mock import patch
 
 import ddt
 import lxml
@@ -644,7 +643,7 @@ class TestCourseOutline(CourseTestCase):
         )
 
     @override_settings(FEATURES={'ENABLE_EXAM_SETTINGS_HTML_VIEW': True})
-    @patch('cms.djangoapps.models.settings.course_metadata.CourseMetadata.validate_proctoring_settings')
+    @mock.patch('cms.djangoapps.models.settings.course_metadata.CourseMetadata.validate_proctoring_settings')
     def test_proctoring_link_is_visible(self, mock_validate_proctoring_settings):
         """
         Test to check proctored exam settings mfe url is rendering properly
@@ -685,9 +684,11 @@ class TestCourseReIndex(CourseTestCase):
 
     ENABLED_SIGNALS = ['course_published']
 
+    @mock.patch('cms.djangoapps.contentstore.signals.handlers.transaction.on_commit',
+                new=mock.Mock(side_effect=lambda func: func()), )  # run index right away
     def setUp(self):
         """
-        Set up the for the course outline tests.
+        Set up the for the course reindex tests.
         """
 
         super().setUp()