From 552c0bc7345f30d8611a00ed5b62b7ad2cf00cce Mon Sep 17 00:00:00 2001 From: Martyn James Date: Fri, 1 May 2015 15:58:11 -0400 Subject: [PATCH] Move location identifying strings into index --- .../contentstore/courseware_index.py | 35 ++++++++ .../tests/test_courseware_index.py | 62 ++++++++++++- .../courseware_search/lms_result_processor.py | 47 ---------- .../test/test_lms_result_processor.py | 86 +------------------ 4 files changed, 95 insertions(+), 135 deletions(-) diff --git a/cms/djangoapps/contentstore/courseware_index.py b/cms/djangoapps/contentstore/courseware_index.py index 3ac410e2ba..aaa8f932ad 100644 --- a/cms/djangoapps/contentstore/courseware_index.py +++ b/cms/djangoapps/contentstore/courseware_index.py @@ -190,6 +190,7 @@ class SearchIndexerBase(object): item_index['id'] = item_id if item.start: item_index['start_date'] = item.start + item_index.update(cls.supplemental_fields(item)) searcher.index(cls.DOCUMENT_TYPE, item_index) indexed_count["count"] += 1 @@ -271,6 +272,14 @@ class SearchIndexerBase(object): """ pass + @classmethod + def supplemental_fields(cls, item): # pylint: disable=unused-argument + """ + Any supplemental fields that get added to the index for the specified + item. Base implementation returns an empty dictionary + """ + return {} + class CoursewareSearchIndexer(SearchIndexerBase): """ @@ -285,6 +294,8 @@ class CoursewareSearchIndexer(SearchIndexerBase): 'category': 'courseware_index' } + UNNAMED_MODULE_NAME = _("(Unnamed)") + @classmethod def normalize_structure_key(cls, structure_key): """ Normalizes structure key for use in indexing """ @@ -314,6 +325,30 @@ class CoursewareSearchIndexer(SearchIndexerBase): """ CourseAboutSearchIndexer.index_about_information(modulestore, structure) + @classmethod + def supplemental_fields(cls, item): + """ + Add location path to the item object + + Once we've established the path of names, the first name is the course + name, and the next 3 names are the navigable path within the edx + application. Notice that we stop at that level because a full path to + deep children would be confusing. + """ + location_path = [] + parent = item + while parent is not None: + path_component_name = parent.display_name + if not path_component_name: + path_component_name = cls.UNNAMED_MODULE_NAME + location_path.append(path_component_name) + parent = parent.get_parent() + location_path.reverse() + return { + "course_name": location_path[0], + "location": location_path[1:4] + } + class LibrarySearchIndexer(SearchIndexerBase): """ diff --git a/cms/djangoapps/contentstore/tests/test_courseware_index.py b/cms/djangoapps/contentstore/tests/test_courseware_index.py index 270a2f9fde..a5978ee1e4 100644 --- a/cms/djangoapps/contentstore/tests/test_courseware_index.py +++ b/cms/djangoapps/contentstore/tests/test_courseware_index.py @@ -145,10 +145,10 @@ class MixedWithOptionsTestCase(MixedSplitTestCase): """ Returns field_dictionary for default search """ return {} - def search(self, field_dictionary=None): + def search(self, field_dictionary=None, query_string=None): """ Performs index search according to passed parameters """ fields = field_dictionary if field_dictionary else self._get_default_search() - return self.searcher.search(field_dictionary=fields, doc_type=self.DOCUMENT_TYPE) + return self.searcher.search(query_string=query_string, field_dictionary=fields, doc_type=self.DOCUMENT_TYPE) def _perform_test_using_store(self, store_type, test_to_perform): """ Helper method to run a test function that uses a specific store """ @@ -220,7 +220,11 @@ class TestCoursewareSearchIndexer(MixedWithOptionsTestCase): """ Set up the for the course outline tests. """ - self.course = CourseFactory.create(modulestore=store, start=datetime(2015, 3, 1, tzinfo=UTC)) + self.course = CourseFactory.create( + modulestore=store, + start=datetime(2015, 3, 1, tzinfo=UTC), + display_name="Search Index Test Course" + ) self.chapter = ItemFactory.create( parent_location=self.course.location, @@ -485,6 +489,50 @@ class TestCoursewareSearchIndexer(MixedWithOptionsTestCase): self.assertIn(CourseMode.HONOR, response["results"][0]["data"]["modes"]) self.assertIn(CourseMode.VERIFIED, response["results"][0]["data"]["modes"]) + def _test_course_location_info(self, store): + """ Test that course location information is added to index """ + self.publish_item(store, self.vertical.location) + self.reindex_course(store) + response = self.search(query_string="Html Content") + self.assertEqual(response["total"], 1) + + result = response["results"][0]["data"] + self.assertEqual(result["course_name"], "Search Index Test Course") + self.assertEqual(result["location"], ["Week 1", "Lesson 1", "Subsection 1"]) + + def _test_course_location_null(self, store): + """ Test that course location information is added to index """ + sequential2 = ItemFactory.create( + parent_location=self.chapter.location, + category='sequential', + display_name=None, + modulestore=store, + publish_item=True, + start=datetime(2015, 3, 1, tzinfo=UTC), + ) + # add a new vertical + vertical2 = ItemFactory.create( + parent_location=sequential2.location, + category='vertical', + display_name='Subsection 2', + modulestore=store, + publish_item=True, + ) + ItemFactory.create( + parent_location=vertical2.location, + category="html", + display_name="Find Me", + publish_item=True, + modulestore=store, + ) + self.reindex_course(store) + response = self.search(query_string="Find Me") + self.assertEqual(response["total"], 1) + + result = response["results"][0]["data"] + self.assertEqual(result["course_name"], "Search Index Test Course") + self.assertEqual(result["location"], ["Week 1", CoursewareSearchIndexer.UNNAMED_MODULE_NAME, "Subsection 2"]) + @patch('django.conf.settings.SEARCH_ENGINE', 'search.tests.utils.ErroringIndexEngine') def _test_exception(self, store): """ Test that exception within indexing yields a SearchIndexingError """ @@ -536,6 +584,14 @@ class TestCoursewareSearchIndexer(MixedWithOptionsTestCase): def test_course_about_mode_index(self, store_type): self._perform_test_using_store(store_type, self._test_course_about_mode_index) + @ddt.data(*WORKS_WITH_STORES) + def test_course_location_info(self, store_type): + self._perform_test_using_store(store_type, self._test_course_location_info) + + @ddt.data(*WORKS_WITH_STORES) + def test_course_location_null(self, store_type): + self._perform_test_using_store(store_type, self._test_course_location_null) + @patch('django.conf.settings.SEARCH_ENGINE', 'search.tests.utils.ForceRefreshElasticSearchEngine') @ddt.ddt diff --git a/lms/lib/courseware_search/lms_result_processor.py b/lms/lib/courseware_search/lms_result_processor.py index c19be41b24..78625661e1 100644 --- a/lms/lib/courseware_search/lms_result_processor.py +++ b/lms/lib/courseware_search/lms_result_processor.py @@ -13,8 +13,6 @@ from xmodule.modulestore.search import path_to_location, navigation_index from courseware.access import has_access -UNNAMED_MODULE_NAME = _("(Unnamed)") - class LmsSearchResultProcessor(SearchResultProcessor): @@ -62,51 +60,6 @@ class LmsSearchResultProcessor(SearchResultProcessor): kwargs={"course_id": self._results_fields["course"], "location": self._results_fields["id"]} ) - @property - def course_name(self): - """ - Display the course name when searching multiple courses - retain result for subsequent uses - """ - if self._course_name is None: - course = self.get_module_store().get_course(self.get_course_key()) - self._course_name = course.display_name_with_default - return self._course_name - - @property - def location(self): - """ - Blend "location" property into the resultset, so that the path to the found component can be shown within the UI - """ - # TODO: update whern changes to "cohorted-courseware" branch are merged in - (course_key, chapter, section, position) = path_to_location(self.get_module_store(), self.get_usage_key()) - - def get_display_name(item_key): - """ gets display name from object's key """ - item = self.get_item(item_key) - display_name = getattr(item, "display_name", None) - return display_name if display_name else UNNAMED_MODULE_NAME - - def get_position_name(section, position): - """ helper to fetch name corresponding to the position therein """ - if position: - section_item = self.get_item(course_key.make_usage_key("sequential", section)) - if section_item.has_children and len(section_item.children) >= position: - return get_display_name(section_item.children[position - 1]) - return None - - location_description = [] - if chapter: - location_description.append(get_display_name(course_key.make_usage_key("chapter", chapter))) - if section: - location_description.append(get_display_name(course_key.make_usage_key("sequential", section))) - if position: - # We're only wanting to show the first vertical, so we use the - # navigation_index function to display the same location to which one - # would be sent if navigating - location_description.append(get_position_name(section, navigation_index(position))) - - return location_description - def should_remove(self, user): """ Test to see if this result should be removed due to access restriction """ return not has_access( diff --git a/lms/lib/courseware_search/test/test_lms_result_processor.py b/lms/lib/courseware_search/test/test_lms_result_processor.py index c07352cbeb..ef85771b72 100644 --- a/lms/lib/courseware_search/test/test_lms_result_processor.py +++ b/lms/lib/courseware_search/test/test_lms_result_processor.py @@ -6,7 +6,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from courseware.tests.factories import UserFactory -from lms.lib.courseware_search.lms_result_processor import LmsSearchResultProcessor, UNNAMED_MODULE_NAME +from lms.lib.courseware_search.lms_result_processor import LmsSearchResultProcessor class LmsSearchResultProcessorTestCase(ModuleStoreTestCase): @@ -85,71 +85,6 @@ class LmsSearchResultProcessorTestCase(ModuleStoreTestCase): self.assertEqual( srp.url, "/courses/{}/jump_to/{}".format(unicode(self.course.id), unicode(self.html.scope_ids.usage_id))) - def test_course_name_parameter(self): - srp = LmsSearchResultProcessor( - { - "course": unicode(self.course.id), - "id": unicode(self.html.scope_ids.usage_id), - "content": {"text": "This is the html text"} - }, - "test" - ) - self.assertEqual(srp.course_name, self.course.display_name) - - def test_location_parameter(self): - srp = LmsSearchResultProcessor( - { - "course": unicode(self.course.id), - "id": unicode(self.html.scope_ids.usage_id), - "content": {"text": "This is html test text"} - }, - "test" - ) - - self.assertEqual(len(srp.location), 3) - self.assertEqual(srp.location[0], 'Test Section') - self.assertEqual(srp.location[1], 'Test Subsection') - self.assertEqual(srp.location[2], 'Test Unit') - - srp = LmsSearchResultProcessor( - { - "course": unicode(self.course.id), - "id": unicode(self.vertical.scope_ids.usage_id), - "content": {"text": "This is html test text"} - }, - "test" - ) - - self.assertEqual(len(srp.location), 3) - self.assertEqual(srp.location[0], 'Test Section') - self.assertEqual(srp.location[1], 'Test Subsection') - self.assertEqual(srp.location[2], 'Test Unit') - - srp = LmsSearchResultProcessor( - { - "course": unicode(self.course.id), - "id": unicode(self.subsection.scope_ids.usage_id), - "content": {"text": "This is html test text"} - }, - "test" - ) - - self.assertEqual(len(srp.location), 2) - self.assertEqual(srp.location[0], 'Test Section') - self.assertEqual(srp.location[1], 'Test Subsection') - - srp = LmsSearchResultProcessor( - { - "course": unicode(self.course.id), - "id": unicode(self.section.scope_ids.usage_id), - "content": {"text": "This is html test text"} - }, - "test" - ) - - self.assertEqual(len(srp.location), 1) - self.assertEqual(srp.location[0], 'Test Section') - def test_should_remove(self): """ Tests that "visible_to_staff_only" overrides start date. @@ -164,22 +99,3 @@ class LmsSearchResultProcessorTestCase(ModuleStoreTestCase): ) self.assertEqual(srp.should_remove(self.global_staff), False) - - def test_missing_display_name(self): - """ - Tests that we get the correct name to include when there is an empty or null display name - """ - srp = LmsSearchResultProcessor( - { - "course": unicode(self.course.id), - "id": unicode(self.ghost_html.scope_ids.usage_id), - "content": {"text": "This is html test text"} - }, - "test" - ) - - location = srp.location - self.assertEqual(len(location), 3) - self.assertEqual(location[0], self.section.display_name) - self.assertEqual(location[1], UNNAMED_MODULE_NAME) - self.assertEqual(location[2], UNNAMED_MODULE_NAME)