Move location identifying strings into index

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):
     """

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

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(

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)