diff --git a/cms/djangoapps/contentstore/courseware_index.py b/cms/djangoapps/contentstore/courseware_index.py index 06a204742ff1..61bd2a6a19ea 100644 --- a/cms/djangoapps/contentstore/courseware_index.py +++ b/cms/djangoapps/contentstore/courseware_index.py @@ -16,7 +16,6 @@ from common.djangoapps.course_modes.models import CourseMode from openedx.core.lib.courses import course_image_url, course_organization_image_url from xmodule.annotator_mixin import html_to_text # pylint: disable=wrong-import-order -from xmodule.library_tools import normalize_key_for_search # pylint: disable=wrong-import-order from xmodule.modulestore import ModuleStoreEnum # pylint: disable=wrong-import-order # REINDEX_AGE is the default amount of time that we look back for changes @@ -443,46 +442,6 @@ def supplemental_fields(cls, item): } -class LibrarySearchIndexer(SearchIndexerBase): - """ - Base class to perform indexing for library search from different modulestores - """ - INDEX_NAME = "library_index" - ENABLE_INDEXING_KEY = 'ENABLE_LIBRARY_INDEX' - - INDEX_EVENT = { - 'name': 'edx.library.index.reindexed', - 'category': 'library_index' - } - - @classmethod - def normalize_structure_key(cls, structure_key): - """ Normalizes structure key for use in indexing """ - return normalize_key_for_search(structure_key) - - @classmethod - def _fetch_top_level(cls, modulestore, structure_key): - """ Fetch the item from the modulestore location """ - return modulestore.get_library(structure_key, depth=None) - - @classmethod - def _get_location_info(cls, normalized_structure_key): - """ Builds location info dictionary """ - return {"library": str(normalized_structure_key)} - - @classmethod - def _id_modifier(cls, usage_id): - """ Modifies usage_id to submit to index """ - return usage_id.replace(library_key=(usage_id.library_key.replace(version_guid=None, branch=None))) - - @classmethod - def do_library_reindex(cls, modulestore, library_key): - """ - (Re)index all content within the given library, tracking the fact that a full reindex has taken place - """ - return cls._do_reindex(modulestore, library_key) - - class AboutInfo: """ About info structure to contain 1) Property name to use diff --git a/cms/djangoapps/contentstore/management/commands/reindex_library.py b/cms/djangoapps/contentstore/management/commands/reindex_library.py deleted file mode 100644 index a1707c49845a..000000000000 --- a/cms/djangoapps/contentstore/management/commands/reindex_library.py +++ /dev/null @@ -1,66 +0,0 @@ -""" Management command to update libraries' search index """ - - -from textwrap import dedent - -from django.core.management import BaseCommand, CommandError -from opaque_keys.edx.keys import CourseKey -from opaque_keys.edx.locator import LibraryLocator - -from cms.djangoapps.contentstore.courseware_index import LibrarySearchIndexer -from xmodule.modulestore.django import modulestore # pylint: disable=wrong-import-order - -from .prompt import query_yes_no - - -class Command(BaseCommand): - """ - Command to reindex content libraries (single, multiple or all available) - - Examples: - - ./manage.py reindex_library lib1 lib2 - reindexes libraries with keys lib1 and lib2 - ./manage.py reindex_library --all - reindexes all available libraries - """ - help = dedent(__doc__) - CONFIRMATION_PROMPT = "Reindexing all libraries might be a time consuming operation. Do you want to continue?" - - def add_arguments(self, parser): - parser.add_argument('library_ids', nargs='*') - parser.add_argument( - '--all', - action='store_true', - dest='all', - help='Reindex all libraries' - ) - - def _parse_library_key(self, raw_value): - """ Parses library key from string """ - result = CourseKey.from_string(raw_value) - - if not isinstance(result, LibraryLocator): - raise CommandError(f"Argument {raw_value} is not a library key") - - return result - - def handle(self, *args, **options): - """ - By convention set by django developers, this method actually executes command's actions. - So, there could be no better docstring than emphasize this once again. - """ - if (not options['library_ids'] and not options['all']) or (options['library_ids'] and options['all']): - raise CommandError("reindex_library requires one or more s or the --all flag.") - - store = modulestore() - - if options['all']: - if query_yes_no(self.CONFIRMATION_PROMPT, default="no"): - library_keys = [library.location.library_key.replace(branch=None) for library in store.get_libraries()] - else: - return - else: - library_keys = list(map(self._parse_library_key, options['library_ids'])) - - for library_key in library_keys: - print(f"Indexing library {library_key}") - LibrarySearchIndexer.do_library_reindex(store, library_key) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_reindex_library.py b/cms/djangoapps/contentstore/management/commands/tests/test_reindex_library.py deleted file mode 100644 index 3147060cb651..000000000000 --- a/cms/djangoapps/contentstore/management/commands/tests/test_reindex_library.py +++ /dev/null @@ -1,133 +0,0 @@ -""" Tests for library reindex command """ - - -from unittest import mock - -import ddt -from django.core.management import CommandError, call_command -from opaque_keys import InvalidKeyError - -from cms.djangoapps.contentstore.courseware_index import SearchIndexingError -from cms.djangoapps.contentstore.management.commands.reindex_library import Command as ReindexCommand -from xmodule.modulestore import ModuleStoreEnum # pylint: disable=wrong-import-order -from xmodule.modulestore.django import modulestore # pylint: disable=wrong-import-order -from xmodule.modulestore.tests.django_utils import ( - ModuleStoreTestCase, # pylint: disable=wrong-import-order -) -from xmodule.modulestore.tests.factories import ( # pylint: disable=wrong-import-order - CourseFactory, - LibraryFactory, -) - - -@ddt.ddt -class TestReindexLibrary(ModuleStoreTestCase): - """ Tests for library reindex command """ - def setUp(self): - """ Setup method - create libraries and courses """ - super().setUp() - self.store = modulestore() - self.first_lib = LibraryFactory.create( - org="test", library="lib1", display_name="run1", default_store=ModuleStoreEnum.Type.split - ) - self.second_lib = LibraryFactory.create( - org="test", library="lib2", display_name="run2", default_store=ModuleStoreEnum.Type.split - ) - - self.first_course = CourseFactory.create( - org="test", course="course1", display_name="run1", default_store=ModuleStoreEnum.Type.split - ) - self.second_course = CourseFactory.create( - org="test", course="course2", display_name="run1", default_store=ModuleStoreEnum.Type.split - ) - - REINDEX_PATH_LOCATION = ( - 'cms.djangoapps.contentstore.management.commands.reindex_library.LibrarySearchIndexer.do_library_reindex' - ) - MODULESTORE_PATCH_LOCATION = 'cms.djangoapps.contentstore.management.commands.reindex_library.modulestore' - YESNO_PATCH_LOCATION = 'cms.djangoapps.contentstore.management.commands.reindex_library.query_yes_no' - - def _get_lib_key(self, library): - """ Get's library key as it is passed to indexer """ - return library.location.library_key - - def _build_calls(self, *libraries): - """ BUilds a list of mock.call instances representing calls to reindexing method """ - return [mock.call(self.store, self._get_lib_key(lib)) for lib in libraries] - - def test_given_no_arguments_raises_command_error(self): - """ Test that raises CommandError for incorrect arguments """ - with self.assertRaisesRegex(CommandError, ".* requires one or more *"): # noqa: PT027 - call_command('reindex_library') - - @ddt.data('qwerty', 'invalid_key', 'xblock-v1:qwe+rty') - def test_given_invalid_lib_key_raises_not_found(self, invalid_key): - """ Test that raises InvalidKeyError for invalid keys """ - with self.assertRaises(InvalidKeyError): # noqa: PT027 - call_command('reindex_library', invalid_key) - - def test_given_course_key_raises_command_error(self): - """ Test that raises CommandError if course key is passed """ - with self.assertRaisesRegex(CommandError, ".* is not a library key"): # noqa: PT027 - call_command('reindex_library', str(self.first_course.id)) - - with self.assertRaisesRegex(CommandError, ".* is not a library key"): # noqa: PT027 - call_command('reindex_library', str(self.second_course.id)) - - with self.assertRaisesRegex(CommandError, ".* is not a library key"): # noqa: PT027 - call_command( - 'reindex_library', - str(self.second_course.id), - str(self._get_lib_key(self.first_lib)) - ) - - def test_given_id_list_indexes_libraries(self): - """ Test that reindexes libraries when given single library key or a list of library keys """ - with mock.patch(self.REINDEX_PATH_LOCATION) as patched_index, \ - mock.patch(self.MODULESTORE_PATCH_LOCATION, mock.Mock(return_value=self.store)): - call_command('reindex_library', str(self._get_lib_key(self.first_lib))) - self.assertEqual(patched_index.mock_calls, self._build_calls(self.first_lib)) # noqa: PT009 - patched_index.reset_mock() - - call_command('reindex_library', str(self._get_lib_key(self.second_lib))) - self.assertEqual(patched_index.mock_calls, self._build_calls(self.second_lib)) # noqa: PT009 - patched_index.reset_mock() - - call_command( - 'reindex_library', - str(self._get_lib_key(self.first_lib)), - str(self._get_lib_key(self.second_lib)) - ) - expected_calls = self._build_calls(self.first_lib, self.second_lib) - self.assertEqual(patched_index.mock_calls, expected_calls) # noqa: PT009 - - def test_given_all_key_prompts_and_reindexes_all_libraries(self): - """ Test that reindexes all libraries when --all key is given and confirmed """ - with mock.patch(self.YESNO_PATCH_LOCATION) as patched_yes_no: - patched_yes_no.return_value = True - with mock.patch(self.REINDEX_PATH_LOCATION) as patched_index, \ - mock.patch(self.MODULESTORE_PATCH_LOCATION, mock.Mock(return_value=self.store)): - call_command('reindex_library', all=True) - - patched_yes_no.assert_called_once_with(ReindexCommand.CONFIRMATION_PROMPT, default='no') - expected_calls = self._build_calls(self.first_lib, self.second_lib) - self.assertCountEqual(patched_index.mock_calls, expected_calls) # noqa: PT009 - - def test_given_all_key_prompts_and_reindexes_all_libraries_cancelled(self): - """ Test that does not reindex anything when --all key is given and cancelled """ - with mock.patch(self.YESNO_PATCH_LOCATION) as patched_yes_no: - patched_yes_no.return_value = False - with mock.patch(self.REINDEX_PATH_LOCATION) as patched_index, \ - mock.patch(self.MODULESTORE_PATCH_LOCATION, mock.Mock(return_value=self.store)): - call_command('reindex_library', all=True) - - patched_yes_no.assert_called_once_with(ReindexCommand.CONFIRMATION_PROMPT, default='no') - patched_index.assert_not_called() - - def test_fail_fast_if_reindex_fails(self): - """ Test that fails on first reindexing exception """ - with mock.patch(self.REINDEX_PATH_LOCATION) as patched_index: - patched_index.side_effect = SearchIndexingError("message", []) - - with self.assertRaises(SearchIndexingError): # noqa: PT027 - call_command('reindex_library', str(self._get_lib_key(self.second_lib))) diff --git a/cms/djangoapps/contentstore/signals/handlers.py b/cms/djangoapps/contentstore/signals/handlers.py index d603c0e583db..e52c79dcc695 100644 --- a/cms/djangoapps/contentstore/signals/handlers.py +++ b/cms/djangoapps/contentstore/signals/handlers.py @@ -34,7 +34,6 @@ from cms.djangoapps.contentstore.courseware_index import ( CourseAboutSearchIndexer, CoursewareSearchIndexer, - LibrarySearchIndexer, ) from common.djangoapps.track.event_transaction_utils import get_event_transaction_id, get_event_transaction_type from common.djangoapps.util.block_utils import yield_dynamic_block_descendants @@ -176,19 +175,6 @@ def listen_for_course_delete(sender, course_key, **kwargs): # pylint: disable=u CourseAboutSearchIndexer.remove_deleted_items(course_key) -@receiver(SignalHandler.library_updated) -def listen_for_library_update(sender, library_key, **kwargs): # pylint: disable=unused-argument - """ - Receives signal and kicks off celery task to update search index - """ - - if LibrarySearchIndexer.indexing_is_enabled(): - # import here, because signal is registered at startup, but items in tasks are not yet able to be loaded - from cms.djangoapps.contentstore.tasks import update_library_index - - update_library_index.delay(str(library_key), datetime.now(UTC).isoformat()) - - @receiver(SignalHandler.pre_item_delete) def handle_item_deleted(**kwargs) -> None: """ diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index d16e941b09e1..7f15a69f182f 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -49,7 +49,6 @@ import cms.djangoapps.contentstore.errors as UserErrors from cms.djangoapps.contentstore.courseware_index import ( CoursewareSearchIndexer, - LibrarySearchIndexer, SearchIndexingError, ) from cms.djangoapps.contentstore.storage import course_import_export_storage @@ -292,20 +291,6 @@ def update_search_index(course_id, triggered_time_isoformat): LOGGER.debug('Search indexing successful for complete course %s', course_id) -@shared_task -@set_code_owner_attribute -def update_library_index(library_id, triggered_time_isoformat): - """ Updates course search index. """ - try: - library_key = CourseKey.from_string(library_id) - LibrarySearchIndexer.index(modulestore(), library_key, triggered_at=(_parse_time(triggered_time_isoformat))) - - except SearchIndexingError as exc: - LOGGER.error('Search indexing error for library %s - %s', library_id, str(exc)) - else: - LOGGER.debug('Search indexing successful for library %s', library_id) - - @shared_task @set_code_owner_attribute def update_special_exams_and_publish(course_key_str): diff --git a/cms/djangoapps/contentstore/tests/test_courseware_index.py b/cms/djangoapps/contentstore/tests/test_courseware_index.py index 7c59773bd871..8b0c75e383ae 100644 --- a/cms/djangoapps/contentstore/tests/test_courseware_index.py +++ b/cms/djangoapps/contentstore/tests/test_courseware_index.py @@ -17,17 +17,15 @@ from cms.djangoapps.contentstore.courseware_index import ( CourseAboutSearchIndexer, CoursewareSearchIndexer, - LibrarySearchIndexer, SearchIndexingError, ) -from cms.djangoapps.contentstore.signals.handlers import listen_for_course_publish, listen_for_library_update +from cms.djangoapps.contentstore.signals.handlers import listen_for_course_publish from cms.djangoapps.contentstore.tasks import update_search_index from cms.djangoapps.contentstore.tests.utils import CourseTestCase from cms.djangoapps.contentstore.utils import reverse_course_url, reverse_usage_url from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.course_modes.tests.factories import CourseModeFactory from openedx.core.djangoapps.models.course_details import CourseDetails -from xmodule.library_tools import normalize_key_for_search # pylint: disable=wrong-import-order from xmodule.modulestore import ModuleStoreEnum # pylint: disable=wrong-import-order from xmodule.modulestore.django import SignalHandler, modulestore # pylint: disable=wrong-import-order from xmodule.modulestore.tests.django_utils import ( # pylint: disable=wrong-import-order @@ -38,7 +36,6 @@ from xmodule.modulestore.tests.factories import ( # pylint: disable=wrong-import-order BlockFactory, CourseFactory, - LibraryFactory, ) from xmodule.partitions.partitions import UserPartition # pylint: disable=wrong-import-order @@ -608,7 +605,6 @@ class TestTaskExecution(SharedModuleStoreTestCase): def setUpClass(cls): super().setUpClass() SignalHandler.course_published.disconnect(listen_for_course_publish) - SignalHandler.library_updated.disconnect(listen_for_library_update) cls.course = CourseFactory.create(start=datetime(2015, 3, 1, tzinfo=UTC)) cls.chapter = BlockFactory.create( @@ -640,26 +636,9 @@ def setUpClass(cls): publish_item=False, ) - cls.library = LibraryFactory.create() - - cls.library_block1 = BlockFactory.create( - parent_location=cls.library.location, - category="html", - display_name="Html Content", - publish_item=False, - ) - - cls.library_block2 = BlockFactory.create( - parent_location=cls.library.location, - category="html", - display_name="Html Content 2", - publish_item=False, - ) - @classmethod def tearDownClass(cls): SignalHandler.course_published.connect(listen_for_course_publish) - SignalHandler.library_updated.connect(listen_for_library_update) super().tearDownClass() def test_task_indexing_course(self): @@ -681,19 +660,6 @@ def test_task_indexing_course(self): ) self.assertEqual(response["total"], 3) # noqa: PT009 - def test_task_library_update(self): - """ Making sure that the receiver correctly fires off the task when invoked by signal """ - searcher = SearchEngine.get_search_engine(LibrarySearchIndexer.INDEX_NAME) - library_search_key = str(normalize_key_for_search(self.library.location.library_key)) - response = searcher.search(field_dictionary={"library": library_search_key}) - self.assertEqual(response["total"], 0) # noqa: PT009 - - listen_for_library_update(self, self.library.location.library_key) - - # Note that this test will only succeed if celery is working in inline mode - response = searcher.search(field_dictionary={"library": library_search_key}) - self.assertEqual(response["total"], 2) # noqa: PT009 - def test_ignore_ccx(self): """Test that we ignore CCX courses (it's too slow now).""" # We're relying on our CCX short circuit to just stop execution as soon @@ -709,156 +675,6 @@ def test_ignore_ccx(self): self.assertFalse(mock_index.called) # noqa: PT009 -@pytest.mark.django_db -@ddt.ddt -class TestLibrarySearchIndexer(MixedWithOptionsTestCase): - """ Tests the operation of the CoursewareSearchIndexer """ - - # libraries work only with split, so do library indexer - WORKS_WITH_STORES = (ModuleStoreEnum.Type.split, ) - - def setUp(self): - super().setUp() - - self.library = None - self.html_unit1 = None - self.html_unit2 = None - - def setup_course_base(self, store): - """ - Set up the for the course outline tests. - """ - self.library = LibraryFactory.create(modulestore=store) - - self.html_unit1 = BlockFactory.create( - parent_location=self.library.location, - category="html", - display_name="Html Content", - modulestore=store, - publish_item=False, - ) - - self.html_unit2 = BlockFactory.create( - parent_location=self.library.location, - category="html", - display_name="Html Content 2", - modulestore=store, - publish_item=False, - ) - - INDEX_NAME = LibrarySearchIndexer.INDEX_NAME - - def _get_default_search(self): - """ Returns field_dictionary for default search """ - return {"library": str(self.library.location.library_key.replace(version_guid=None, branch=None))} - - def reindex_library(self, store): - """ kick off complete reindex of the course """ - return LibrarySearchIndexer.do_library_reindex(store, self.library.location.library_key) - - def _get_contents(self, response): - """ Extracts contents from search response """ - return [item['data']['content'] for item in response['results']] - - def _test_indexing_library(self, store): - """ indexing course tests """ - self.reindex_library(store) - response = self.search() - self.assertEqual(response["total"], 2) # noqa: PT009 - - added_to_index = self.reindex_library(store) - self.assertEqual(added_to_index, 2) # noqa: PT009 - response = self.search() - self.assertEqual(response["total"], 2) # noqa: PT009 - - def _test_creating_item(self, store): - """ test updating an item """ - self.reindex_library(store) - response = self.search() - self.assertEqual(response["total"], 2) # noqa: PT009 - - # updating a library item causes immediate reindexing - data = "Some data" - BlockFactory.create( - parent_location=self.library.location, - category="html", - display_name="Html Content 3", - data=data, - modulestore=store, - publish_item=False, - ) - - self.reindex_library(store) - response = self.search() - self.assertEqual(response["total"], 3) # noqa: PT009 - html_contents = [cont['html_content'] for cont in self._get_contents(response)] - self.assertIn(data, html_contents) # noqa: PT009 - - def _test_updating_item(self, store): - """ test updating an item """ - self.reindex_library(store) - response = self.search() - self.assertEqual(response["total"], 2) # noqa: PT009 - - # updating a library item causes immediate reindexing - new_data = "I'm new data" - self.html_unit1.data = new_data - self.update_item(store, self.html_unit1) - self.reindex_library(store) - response = self.search() - self.assertEqual(response["total"], 2) # noqa: PT009 - html_contents = [cont['html_content'] for cont in self._get_contents(response)] - self.assertIn(new_data, html_contents) # noqa: PT009 - - def _test_deleting_item(self, store): - """ test deleting an item """ - self.reindex_library(store) - response = self.search() - self.assertEqual(response["total"], 2) # noqa: PT009 - - # deleting a library item causes immediate reindexing - self.delete_item(store, self.html_unit1.location) - self.reindex_library(store) - response = self.search() - self.assertEqual(response["total"], 1) # noqa: PT009 - - @patch('django.conf.settings.SEARCH_ENGINE', None) - def _test_search_disabled(self, store): - """ if search setting has it as off, confirm that nothing is indexed """ - indexed_count = self.reindex_library(store) - self.assertFalse(indexed_count) # noqa: PT009 - - @patch('django.conf.settings.SEARCH_ENGINE', 'search.tests.utils.ErroringIndexEngine') - def _test_exception(self, store): - """ Test that exception within indexing yields a SearchIndexingError """ - with self.assertRaises(SearchIndexingError): # noqa: PT027 - self.reindex_library(store) - - @ddt.data(*WORKS_WITH_STORES) - def test_indexing_library(self, store_type): - self._perform_test_using_store(store_type, self._test_indexing_library) - - @ddt.data(*WORKS_WITH_STORES) - def test_updating_item(self, store_type): - self._perform_test_using_store(store_type, self._test_updating_item) - - @ddt.data(*WORKS_WITH_STORES) - def test_creating_item(self, store_type): - self._perform_test_using_store(store_type, self._test_creating_item) - - @ddt.data(*WORKS_WITH_STORES) - def test_deleting_item(self, store_type): - self._perform_test_using_store(store_type, self._test_deleting_item) - - @ddt.data(*WORKS_WITH_STORES) - def test_search_disabled(self, store_type): - self._perform_test_using_store(store_type, self._test_search_disabled) - - @ddt.data(*WORKS_WITH_STORES) - def test_exception(self, store_type): - self._perform_test_using_store(store_type, self._test_exception) - - class GroupConfigurationSearchSplit(CourseTestCase, MixedWithOptionsTestCase): """ Tests indexing of content groups on course blocks using split modulestore. diff --git a/xmodule/library_tools.py b/xmodule/library_tools.py index 1bee31a68755..66f1c84f6670 100644 --- a/xmodule/library_tools.py +++ b/xmodule/library_tools.py @@ -15,11 +15,6 @@ from xmodule.modulestore.exceptions import ItemNotFoundError -def normalize_key_for_search(library_key): - """ Normalizes library key for use with search indexing """ - return library_key.replace(version_guid=None, branch=None) - - class LegacyLibraryToolsService: """ Service for LegacyLibraryContentBlock.