Merge pull request #10455 from edx/search/optimization

Search performance (SOL-1030, SOL-1031)

common/test/acceptance/pages/lms/courseware_search.py

@@ -29,7 +29,7 @@ class CoursewareSearchPage(CoursePage):
     def search(self):
         """ execute the search """
         self.q(css=self.search_bar_selector + ' [type="submit"]').click()
-        self.wait_for_element_visibility('.search-info', 'Search results are shown')
+        self.wait_for_ajax()
 
     def search_for_term(self, text):
         """

common/test/acceptance/tests/lms/test_lms_cohorted_courseware_search.py

@@ -30,8 +30,6 @@ class CoursewareSearchCohortTest(ContainerBase):
     """
     Test courseware search.
     """
-    USERNAME = 'STUDENT_TESTER'
-    EMAIL = 'student101@example.com'
 
     TEST_INDEX_FILENAME = "test_root/index_file.dat"
 
@@ -71,6 +69,14 @@ class CoursewareSearchCohortTest(ContainerBase):
             self.browser, username=self.cohort_b_student_username, email=self.cohort_b_student_email, no_login=True
         ).visit()
 
+        # Create a student who will end up in the default cohort group
+        self.cohort_default_student_username = "cohort_default_student"
+        self.cohort_default_student_email = "cohort_default_student@example.com"
+        StudioAutoAuthPage(
+            self.browser, username=self.cohort_default_student_username,
+            email=self.cohort_default_student_email, no_login=True
+        ).visit()
+
         self.courseware_search_page = CoursewareSearchPage(self.browser, self.course_id)
 
         # Enable Cohorting and assign cohorts and content groups
@@ -183,6 +189,7 @@ class CoursewareSearchCohortTest(ContainerBase):
         set_visibility(1, self.content_group_a)
         set_visibility(2, self.content_group_b)
         set_visibility(3, self.content_group_a, self.content_group_b)
+        set_visibility(4, 'All Students and Staff')  # Does not work without this
 
         container_page.publish_action.click()
 
@@ -213,7 +220,7 @@ class CoursewareSearchCohortTest(ContainerBase):
         """
         Make sure that the page is accessible.
         """
-        self._auto_auth(self.USERNAME, self.EMAIL, False)
+        self._auto_auth(self.cohort_default_student_username, self.cohort_default_student_email, False)
         self.courseware_search_page.visit()
 
     def test_cohorted_search_user_a_a_content(self):
@@ -234,20 +241,20 @@ class CoursewareSearchCohortTest(ContainerBase):
         self.courseware_search_page.search_for_term(self.group_a_html)
         assert self.group_a_html not in self.courseware_search_page.search_results.html[0]
 
-    def test_cohorted_search_user_c_ab_content(self):
+    def test_cohorted_search_user_default_ab_content(self):
         """
         Test user not enrolled in any cohorts can't see any of restricted content.
         """
-        self._auto_auth(self.USERNAME, self.EMAIL, False)
+        self._auto_auth(self.cohort_default_student_username, self.cohort_default_student_email, False)
         self.courseware_search_page.visit()
         self.courseware_search_page.search_for_term(self.group_a_and_b_html)
         assert self.group_a_and_b_html not in self.courseware_search_page.search_results.html[0]
 
-    def test_cohorted_search_user_c_all_content(self):
+    def test_cohorted_search_user_default_all_content(self):
         """
         Test user can search public content if cohorts used on course.
         """
-        self._auto_auth(self.USERNAME, self.EMAIL, False)
+        self._auto_auth(self.cohort_default_student_username, self.cohort_default_student_email, False)
         self.courseware_search_page.visit()
         self.courseware_search_page.search_for_term(self.visible_to_all_html)
         assert self.visible_to_all_html in self.courseware_search_page.search_results.html[0]

common/test/acceptance/tests/lms/test_lms_split_test_courseware_search.py

@@ -52,7 +52,7 @@ class SplitTestCoursewareSearchTest(ContainerBase):
             self.course_info['run']
         )
 
-        self._add_and_configure_split_test()
+        self._create_group_configuration()
         self._studio_reindex()
 
     def _auto_auth(self, username, email, staff):
@@ -72,41 +72,24 @@ class SplitTestCoursewareSearchTest(ContainerBase):
         self.course_outline.start_reindex()
         self.course_outline.wait_for_ajax()
 
-    def _add_and_configure_split_test(self):
+    def _create_group_configuration(self):
         """
-        Add a split test and a configuration to a test course fixture
+        Create a group configuration for course
         """
-        # Create a new group configurations
-        # pylint: disable=W0212
+        # pylint: disable=protected-access
         self.course_fixture._update_xblock(self.course_fixture._course_location, {
             "metadata": {
                 u"user_partitions": [
                     create_user_partition_json(
                         0,
-                        "Name",
-                        "Description.",
+                        "Configuration A/B",
+                        "Content Group Partition.",
                         [Group("0", "Group A"), Group("1", "Group B")]
-                    ),
-                    create_user_partition_json(
-                        456,
-                        "Name 2",
-                        "Description 2.",
-                        [Group("2", "Group C"), Group("3", "Group D")]
-                    ),
-                ],
-            },
+                    )
+                ]
+            }
         })
 
-        # Add a split test module to the 'Test Unit' vertical in the course tree
-        split_test_1 = XBlockFixtureDesc('split_test', 'Test Content Experiment 1', metadata={'user_partition_id': 0})
-        split_test_1_parent_vertical = self.course_fixture.get_nested_xblocks(category="vertical")[1]
-        self.course_fixture.create_xblock(split_test_1_parent_vertical.locator, split_test_1)
-
-        # Add a split test module to the 'Test 2 Unit' vertical in the course tree
-        split_test_2 = XBlockFixtureDesc('split_test', 'Test Content Experiment 2', metadata={'user_partition_id': 456})
-        split_test_2_parent_vertical = self.course_fixture.get_nested_xblocks(category="vertical")[2]
-        self.course_fixture.create_xblock(split_test_2_parent_vertical.locator, split_test_2)
-
     def populate_course_fixture(self, course_fixture):
         """
         Populate the children of the test course fixture.
@@ -116,28 +99,26 @@ class SplitTestCoursewareSearchTest(ContainerBase):
         })
 
         course_fixture.add_children(
-            XBlockFixtureDesc('chapter', 'Content Section').add_children(
-                XBlockFixtureDesc('sequential', 'Content Subsection').add_children(
-                    XBlockFixtureDesc('vertical', 'Content Unit').add_children(
-                        XBlockFixtureDesc('html', 'VISIBLETOALLCONTENT', data='<html>VISIBLETOALLCONTENT</html>')
-                    )
-                )
-            ),
             XBlockFixtureDesc('chapter', 'Test Section').add_children(
                 XBlockFixtureDesc('sequential', 'Test Subsection').add_children(
-                    XBlockFixtureDesc('vertical', 'Test Unit')
-                )
-            ),
-            XBlockFixtureDesc('chapter', 'X Section').add_children(
-                XBlockFixtureDesc('sequential', 'X Subsection').add_children(
-                    XBlockFixtureDesc('vertical', 'X Unit')
+                    XBlockFixtureDesc('vertical', 'Test Unit').add_children(
+                        XBlockFixtureDesc(
+                            'html',
+                            'VISIBLE TO A',
+                            data='<html>VISIBLE TO A</html>',
+                            metadata={"group_access": {0: [0]}}
+                        ),
+                        XBlockFixtureDesc(
+                            'html',
+                            'VISIBLE TO B',
+                            data='<html>VISIBLE TO B</html>',
+                            metadata={"group_access": {0: [1]}}
+                        )
+                    )
                 )
-            ),
+            )
         )
 
-        self.test_1_breadcrumb = "Test Section \xe2\x96\xb8 Test Subsection \xe2\x96\xb8 Test Unit".decode("utf-8")
-        self.test_2_breadcrumb = "X Section \xe2\x96\xb8 X Subsection \xe2\x96\xb8 X Unit".decode("utf-8")
-
     def test_page_existence(self):
         """
         Make sure that the page is accessible.
@@ -145,15 +126,6 @@ class SplitTestCoursewareSearchTest(ContainerBase):
         self._auto_auth(self.USERNAME, self.EMAIL, False)
         self.courseware_search_page.visit()
 
-    def test_search_for_experiment_content_user_not_assigned(self):
-        """
-        Test user can't search for experiment content if not assigned to a group.
-        """
-        self._auto_auth(self.USERNAME, self.EMAIL, False)
-        self.courseware_search_page.visit()
-        self.courseware_search_page.search_for_term("Group")
-        assert "Sorry, no results were found." in self.courseware_search_page.search_results.html[0]
-
     def test_search_for_experiment_content_user_assigned_to_one_group(self):
         """
         Test user can search for experiment content restricted to his group
@@ -161,8 +133,5 @@ class SplitTestCoursewareSearchTest(ContainerBase):
         """
         self._auto_auth(self.USERNAME, self.EMAIL, False)
         self.courseware_search_page.visit()
-        self.course_navigation_page.go_to_section("Test Section", "Test Subsection")
-        self.courseware_search_page.search_for_term("Group")
+        self.courseware_search_page.search_for_term("VISIBLE TO")
         assert "1 result" in self.courseware_search_page.search_results.html[0]
-        assert self.test_1_breadcrumb in self.courseware_search_page.search_results.html[0]
-        assert self.test_2_breadcrumb not in self.courseware_search_page.search_results.html[0]

lms/lib/courseware_search/lms_filter_generator.py

@@ -5,15 +5,9 @@ This file contains implementation override of SearchFilterGenerator which will a
 from microsite_configuration import microsite
 
 from student.models import CourseEnrollment
-from opaque_keys import InvalidKeyError
-from opaque_keys.edx.keys import CourseKey
-from opaque_keys.edx.locations import SlashSeparatedCourseKey
-from xmodule.modulestore.django import modulestore
-
 from search.filter_generator import SearchFilterGenerator
 from openedx.core.djangoapps.user_api.partition_schemes import RandomUserPartitionScheme
 from openedx.core.djangoapps.course_groups.partition_scheme import CohortPartitionScheme
-from courseware.access import get_user_role
 
 
 INCLUDE_SCHEMES = [CohortPartitionScheme, RandomUserPartitionScheme, ]
@@ -31,67 +25,6 @@ class LmsSearchFilterGenerator(SearchFilterGenerator):
             self._user_enrollments[user] = CourseEnrollment.enrollments_for_user(user)
         return self._user_enrollments[user]
 
-    def filter_dictionary(self, **kwargs):
-        """ LMS implementation, adds filtering by user partition, course id and user """
-
-        def get_group_for_user_partition(user_partition, course_key, user):
-            """ Returns the specified user's group for user partition """
-            if user_partition.scheme in SCHEME_SUPPORTS_ASSIGNMENT:
-                return user_partition.scheme.get_group_for_user(
-                    course_key,
-                    user,
-                    user_partition,
-                    assign=False,
-                )
-            else:
-                return user_partition.scheme.get_group_for_user(
-                    course_key,
-                    user,
-                    user_partition,
-                )
-
-        def get_group_ids_for_user(course, user):
-            """ Collect user partition group ids for user for this course """
-            partition_groups = []
-            for user_partition in course.user_partitions:
-                if user_partition.scheme in INCLUDE_SCHEMES:
-                    group = get_group_for_user_partition(user_partition, course.id, user)
-                    if group:
-                        partition_groups.append(group)
-            partition_group_ids = [unicode(partition_group.id) for partition_group in partition_groups]
-            return partition_group_ids if partition_group_ids else None
-
-        filter_dictionary = super(LmsSearchFilterGenerator, self).filter_dictionary(**kwargs)
-        if 'user' in kwargs:
-            user = kwargs['user']
-
-            if 'course_id' in kwargs and kwargs['course_id']:
-                try:
-                    course_key = CourseKey.from_string(kwargs['course_id'])
-                except InvalidKeyError:
-                    course_key = SlashSeparatedCourseKey.from_deprecated_string(kwargs['course_id'])
-
-                # Staff user looking at course as staff user
-                if get_user_role(user, course_key) in ('instructor', 'staff'):
-                    return filter_dictionary
-                # Need to check course exist (if course gets deleted enrollments don't get cleaned up)
-                course = modulestore().get_course(course_key)
-                if course:
-                    filter_dictionary['content_groups'] = get_group_ids_for_user(course, user)
-            else:
-                user_enrollments = self._enrollments_for_user(user)
-                content_groups = []
-                for enrollment in user_enrollments:
-                    course = modulestore().get_course(enrollment.course_id)
-                    if course:
-                        enrollment_group_ids = get_group_ids_for_user(course, user)
-                        if enrollment_group_ids:
-                            content_groups.extend(enrollment_group_ids)
-
-                filter_dictionary['content_groups'] = content_groups if content_groups else None
-
-        return filter_dictionary
-
     def field_dictionary(self, **kwargs):
         """ add course if provided otherwise add courses in which the user is enrolled in """
         field_dictionary = super(LmsSearchFilterGenerator, self).field_dictionary(**kwargs)

lms/lib/courseware_search/lms_result_processor.py

@@ -8,18 +8,16 @@ from django.core.urlresolvers import reverse
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from search.result_processor import SearchResultProcessor
 from xmodule.modulestore.django import modulestore
-
-from courseware.access import has_access
+from lms.djangoapps.course_blocks.api import get_course_blocks
+from lms.djangoapps.courseware.access import has_access
 
 
 class LmsSearchResultProcessor(SearchResultProcessor):
-
     """ SearchResultProcessor for LMS Search """
     _course_key = None
-    _course_name = None
     _usage_key = None
     _module_store = None
-    _module_temp_dictionary = {}
+    _course_blocks = {}
 
     def get_course_key(self):
         """ fetch course key object from string representation - retain result for subsequent uses """
@@ -39,11 +37,13 @@ class LmsSearchResultProcessor(SearchResultProcessor):
             self._module_store = modulestore()
         return self._module_store
 
-    def get_item(self, usage_key):
-        """ fetch item from the modulestore - don't refetch if we've already retrieved it beforehand """
-        if usage_key not in self._module_temp_dictionary:
-            self._module_temp_dictionary[usage_key] = self.get_module_store().get_item(usage_key)
-        return self._module_temp_dictionary[usage_key]
+    def get_course_blocks(self, user):
+        """ fetch cached blocks for course - retain for subsequent use """
+        course_key = self.get_course_key()
+        if course_key not in self._course_blocks:
+            root_block_usage_key = self.get_module_store().make_course_usage_key(course_key)
+            self._course_blocks[course_key] = get_course_blocks(user, root_block_usage_key)
+        return self._course_blocks[course_key]
 
     @property
     def url(self):
@@ -60,10 +60,6 @@ class LmsSearchResultProcessor(SearchResultProcessor):
 
     def should_remove(self, user):
         """ Test to see if this result should be removed due to access restriction """
-        user_has_access = has_access(
-            user,
-            "load",
-            self.get_item(self.get_usage_key()),
-            self.get_course_key()
-        )
-        return not user_has_access
+        if has_access(user, 'staff', self.get_course_key()):
+            return False
+        return self.get_usage_key() not in self.get_course_blocks(user).get_block_keys()

lms/lib/courseware_search/test/test_lms_search_initializer.py

-"""
-Tests for the lms_search_initializer
-"""
-
-from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
-from xmodule.partitions.partitions import Group, UserPartition
-from xmodule.modulestore.django import modulestore
-
-from courseware.tests.factories import UserFactory
-from courseware.tests.test_masquerade import StaffMasqueradeTestCase
-from courseware.masquerade import handle_ajax
-
-from lms.lib.courseware_search.lms_search_initializer import LmsSearchInitializer
-from lms.lib.courseware_search.lms_filter_generator import LmsSearchFilterGenerator
-
-
-class LmsSearchInitializerTestCase(StaffMasqueradeTestCase):
-    """ Test case class to test search initializer """
-
-    def build_course(self):
-        """
-        Build up a course tree with an html control
-        """
-        self.global_staff = UserFactory(is_staff=True)
-
-        self.course = CourseFactory.create(
-            org='Elasticsearch',
-            course='ES101',
-            run='test_run',
-            display_name='Elasticsearch test course',
-        )
-        self.section = ItemFactory.create(
-            parent=self.course,
-            category='chapter',
-            display_name='Test Section',
-        )
-        self.subsection = ItemFactory.create(
-            parent=self.section,
-            category='sequential',
-            display_name='Test Subsection',
-        )
-        self.vertical = ItemFactory.create(
-            parent=self.subsection,
-            category='vertical',
-            display_name='Test Unit',
-        )
-        self.html = ItemFactory.create(
-            parent=self.vertical,
-            category='html',
-            display_name='Test Html control 1',
-        )
-        self.html = ItemFactory.create(
-            parent=self.vertical,
-            category='html',
-            display_name='Test Html control 2',
-        )
-
-    def setUp(self):
-        super(LmsSearchInitializerTestCase, self).setUp()
-        self.build_course()
-        self.user_partition = UserPartition(
-            id=0,
-            name='Test User Partition',
-            description='',
-            groups=[Group(0, 'Group 1'), Group(1, 'Group 2')],
-            scheme_id='cohort'
-        )
-        self.course.user_partitions.append(self.user_partition)
-        modulestore().update_item(self.course, self.global_staff.id)  # pylint: disable=no-member
-
-    def test_staff_masquerading_added_to_group(self):
-        """
-        Tests that initializer sets masquerading for a staff user in a group.
-        """
-        # Verify that there is no masquerading group initially
-        _, filter_directory, _ = LmsSearchFilterGenerator.generate_field_filters(  # pylint: disable=unused-variable
-            user=self.global_staff,
-            course_id=unicode(self.course.id)
-        )
-        # User is staff by default, no content groups filter is set - see all
-        self.assertNotIn('content_groups', filter_directory)
-
-        # Install a masquerading group
-        request = self._create_mock_json_request(
-            self.global_staff,
-            body='{"role": "student", "user_partition_id": 0, "group_id": 1}'
-        )
-        handle_ajax(request, unicode(self.course.id))
-
-        # Call initializer
-        LmsSearchInitializer.set_search_enviroment(
-            request=request,
-            course_id=unicode(self.course.id)
-        )
-
-        # Verify that there is masquerading group after masquerade
-        _, filter_directory, _ = LmsSearchFilterGenerator.generate_field_filters(  # pylint: disable=unused-variable
-            user=self.global_staff,
-            course_id=unicode(self.course.id)
-        )
-        self.assertEqual(filter_directory['content_groups'], [unicode(1)])
-
-    def test_staff_masquerading_as_a_staff_user(self):
-        """
-        Tests that initializer sets masquerading for a staff user as staff.
-        """
-
-        # Install a masquerading group
-        request = self._create_mock_json_request(
-            self.global_staff,
-            body='{"role": "staff"}'
-        )
-        handle_ajax(request, unicode(self.course.id))
-
-        # Call initializer
-        LmsSearchInitializer.set_search_enviroment(
-            request=request,
-            course_id=unicode(self.course.id)
-        )
-
-        # Verify that there is masquerading group after masquerade
-        _, filter_directory, _ = LmsSearchFilterGenerator.generate_field_filters(  # pylint: disable=unused-variable
-            user=self.global_staff,
-            course_id=unicode(self.course.id)
-        )
-        self.assertNotIn('content_groups', filter_directory)
-
-    def test_staff_masquerading_as_a_student_user(self):
-        """
-        Tests that initializer sets masquerading for a staff user as student.
-        """
-
-        # Install a masquerading group
-        request = self._create_mock_json_request(
-            self.global_staff,
-            body='{"role": "student"}'
-        )
-        handle_ajax(request, unicode(self.course.id))
-
-        # Call initializer
-        LmsSearchInitializer.set_search_enviroment(
-            request=request,
-            course_id=unicode(self.course.id)
-        )
-
-        # Verify that there is masquerading group after masquerade
-        _, filter_directory, _ = LmsSearchFilterGenerator.generate_field_filters(  # pylint: disable=unused-variable
-            user=self.global_staff,
-            course_id=unicode(self.course.id)
-        )
-        self.assertEqual(filter_directory['content_groups'], None)