From 7b7a0d3773ef57be492e41e7fb1f33af5b718747 Mon Sep 17 00:00:00 2001 From: Andy Shultz Date: Wed, 17 Jan 2024 09:41:24 -0500 Subject: [PATCH] fix: make reindexing wait for the data using on_commit includes a very long comment explaining this for future developers --- cms/djangoapps/contentstore/signals/handlers.py | 16 +++++++++++++--- .../contentstore/tests/test_courseware_index.py | 4 +++- .../views/tests/test_course_index.py | 7 ++++--- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/signals/handlers.py b/cms/djangoapps/contentstore/signals/handlers.py index 20c14089e0..e0bc9fcc95 100644 --- a/cms/djangoapps/contentstore/signals/handlers.py +++ b/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], diff --git a/cms/djangoapps/contentstore/tests/test_courseware_index.py b/cms/djangoapps/contentstore/tests/test_courseware_index.py index 7d7a0d533b..98a60dce90 100644 --- a/cms/djangoapps/contentstore/tests/test_courseware_index.py +++ b/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 diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index 73db1a10b9..b30f8c95a6 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/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()