diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index c771fbd74f..f4491cdcd3 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -16,7 +16,7 @@ from contentstore.tests.utils import AjaxEnabledTestClient from contentstore.utils import delete_course from contentstore.views.course import ( AccessListFallback, - _accessible_courses_iter, + _accessible_courses_iter_for_tests, _accessible_courses_list_from_groups, _accessible_courses_summary_iter, get_courses_accessible_to_user @@ -103,7 +103,7 @@ class TestCourseListing(ModuleStoreTestCase): self._create_course_with_access_groups(course_location, self.user) # get courses through iterating all courses - courses_iter, __ = _accessible_courses_iter(self.request) + courses_iter, __ = _accessible_courses_iter_for_tests(self.request) courses_list = list(courses_iter) self.assertEqual(len(courses_list), 1) @@ -115,7 +115,10 @@ class TestCourseListing(ModuleStoreTestCase): self.assertEqual(len(courses_list_by_groups), 1) # check both course lists have same courses - self.assertEqual(courses_list, courses_list_by_groups) + course_keys_in_course_list = [course.id for course in courses_list] + course_keys_in_courses_list_by_groups = [course.id for course in courses_list_by_groups] + + self.assertEqual(course_keys_in_course_list, course_keys_in_courses_list_by_groups) def test_courses_list_with_ccx_courses(self): """ @@ -150,36 +153,10 @@ class TestCourseListing(ModuleStoreTestCase): # Verify that CCX courses are filtered out while iterating over all courses mocked_ccx_course = Mock(id=ccx_course_key) - with patch('xmodule.modulestore.mixed.MixedModuleStore.get_courses', return_value=[mocked_ccx_course]): - courses_iter, __ = _accessible_courses_iter(self.request) + with patch('xmodule.modulestore.mixed.MixedModuleStore.get_course_summaries', return_value=[mocked_ccx_course]): + courses_iter, __ = _accessible_courses_iter_for_tests(self.request) self.assertEqual(len(list(courses_iter)), 0) - @ddt.data( - (ModuleStoreEnum.Type.split, 'xmodule.modulestore.split_mongo.split_mongo_kvs.SplitMongoKVS'), - (ModuleStoreEnum.Type.mongo, 'xmodule.modulestore.mongo.base.MongoKeyValueStore') - ) - @ddt.unpack - def test_errored_course_global_staff(self, store, path_to_patch): - """ - Test the course list for global staff when get_course returns an ErrorDescriptor - """ - GlobalStaff().add_users(self.user) - - with self.store.default_store(store): - course_key = self.store.make_course_key('Org1', 'Course1', 'Run1') - self._create_course_with_access_groups(course_key, self.user, store=store) - - with patch(path_to_patch, Mock(side_effect=Exception)): - self.assertIsInstance(self.store.get_course(course_key), ErrorDescriptor) - - # get courses through iterating all courses - courses_iter, __ = _accessible_courses_iter(self.request) - self.assertEqual(list(courses_iter), []) - - # get courses by reversing group name formats - courses_list_by_groups, __ = _accessible_courses_list_from_groups(self.request) - self.assertEqual(courses_list_by_groups, []) - @ddt.data( (ModuleStoreEnum.Type.split, 3), (ModuleStoreEnum.Type.mongo, 2) @@ -212,36 +189,6 @@ class TestCourseListing(ModuleStoreTestCase): with check_mongo_calls(mongo_calls): list(_accessible_courses_summary_iter(self.request)) - @ddt.data( - (ModuleStoreEnum.Type.split, 'xmodule.modulestore.split_mongo.split_mongo_kvs.SplitMongoKVS'), - (ModuleStoreEnum.Type.mongo, 'xmodule.modulestore.mongo.base.MongoKeyValueStore') - ) - @ddt.unpack - def test_errored_course_regular_access(self, store, path_to_patch): - """ - Test the course list for regular staff when get_course returns an ErrorDescriptor - """ - GlobalStaff().remove_users(self.user) - - with self.store.default_store(store): - CourseStaffRole(self.store.make_course_key('Non', 'Existent', 'Course')).add_users(self.user) - - course_key = self.store.make_course_key('Org1', 'Course1', 'Run1') - self._create_course_with_access_groups(course_key, self.user, store) - - with patch(path_to_patch, Mock(side_effect=Exception)): - self.assertIsInstance(self.store.get_course(course_key), ErrorDescriptor) - - # get courses through iterating all courses - courses_iter, __ = _accessible_courses_iter(self.request) - courses_list = list(courses_iter) - self.assertEqual(courses_list, []) - - # get courses by reversing group name formats - courses_list_by_groups, __ = _accessible_courses_list_from_groups(self.request) - self.assertEqual(courses_list_by_groups, []) - self.assertEqual(courses_list, courses_list_by_groups) - @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo) def test_get_course_list_with_invalid_course_location(self, store): """ @@ -252,7 +199,7 @@ class TestCourseListing(ModuleStoreTestCase): self._create_course_with_access_groups(course_key, self.user, store) # get courses through iterating all courses - courses_iter, __ = _accessible_courses_iter(self.request) + courses_iter, __ = _accessible_courses_iter_for_tests(self.request) courses_list = list(courses_iter) self.assertEqual(len(courses_list), 1) @@ -268,16 +215,17 @@ class TestCourseListing(ModuleStoreTestCase): courses_list_by_groups, __ = _accessible_courses_list_from_groups(self.request) self.assertEqual(len(courses_list_by_groups), 1) + course_keys_in_course_list = [course.id for course in courses_list] + course_keys_in_courses_list_by_groups = [course.id for course in courses_list_by_groups] # check course lists have same courses - self.assertEqual(courses_list, courses_list_by_groups) - + self.assertEqual(course_keys_in_course_list, course_keys_in_courses_list_by_groups) # now delete this course and re-add user to instructor group of this course delete_course(course_key, self.user.id) CourseInstructorRole(course_key).add_users(self.user) # Get courses through iterating all courses - courses_iter, __ = _accessible_courses_iter(self.request) + courses_iter, __ = _accessible_courses_iter_for_tests(self.request) # Get course summaries by iterating all courses courses_summary_iter, __ = _accessible_courses_summary_iter(self.request) @@ -292,8 +240,8 @@ class TestCourseListing(ModuleStoreTestCase): ) @ddt.data( - (ModuleStoreEnum.Type.split, 4, 23), - (ModuleStoreEnum.Type.mongo, USER_COURSES_COUNT, 2) + (ModuleStoreEnum.Type.split, 3, 3), + (ModuleStoreEnum.Type.mongo, 2, 2) ) @ddt.unpack def test_course_listing_performance(self, store, courses_list_from_group_calls, courses_list_calls): @@ -319,12 +267,12 @@ class TestCourseListing(ModuleStoreTestCase): # time the get courses by iterating through all courses with Timer() as iteration_over_courses_time_1: - courses_iter, __ = _accessible_courses_iter(self.request) + courses_iter, __ = _accessible_courses_iter_for_tests(self.request) self.assertEqual(len(list(courses_iter)), USER_COURSES_COUNT) # time again the get courses by iterating through all courses with Timer() as iteration_over_courses_time_2: - courses_iter, __ = _accessible_courses_iter(self.request) + courses_iter, __ = _accessible_courses_iter_for_tests(self.request) self.assertEqual(len(list(courses_iter)), USER_COURSES_COUNT) # time the get courses by reversing django groups @@ -337,25 +285,18 @@ class TestCourseListing(ModuleStoreTestCase): courses_list, __ = _accessible_courses_list_from_groups(self.request) self.assertEqual(len(courses_list), USER_COURSES_COUNT) - # TODO (cdyer) : iteration over courses was optimized, and is now - # sometimes faster than iteration over groups. One of the following - # should be done to resolve this: - # * Iteration over groups should be sped up. - # * Iteration over groups should be removed, as it no longer saves time. - # * Or this part of the test should be removed. - # Test that the time taken by getting courses through reversing django # groups is lower then the time taken by traversing through all courses # (if accessible courses are relatively small). - #self.assertGreaterEqual(iteration_over_courses_time_1.elapsed, iteration_over_groups_time_1.elapsed) - #self.assertGreaterEqual(iteration_over_courses_time_2.elapsed, iteration_over_groups_time_2.elapsed) + self.assertGreaterEqual(iteration_over_courses_time_1.elapsed, iteration_over_groups_time_1.elapsed) + self.assertGreaterEqual(iteration_over_courses_time_2.elapsed, iteration_over_groups_time_2.elapsed) # Now count the db queries with check_mongo_calls(courses_list_from_group_calls): _accessible_courses_list_from_groups(self.request) with check_mongo_calls(courses_list_calls): - list(_accessible_courses_iter(self.request)) + list(_accessible_courses_iter_for_tests(self.request)) # Calls: # 1) query old mongo # 2) get_more on old mongo @@ -436,13 +377,12 @@ class TestCourseListing(ModuleStoreTestCase): ) # verify return values - for method in (_accessible_courses_list_from_groups, _accessible_courses_iter): - def set_of_course_keys(course_list, key_attribute_name='id'): - """Returns a python set of course keys by accessing the key with the given attribute name.""" - return set(getattr(c, key_attribute_name) for c in course_list) + def _set_of_course_keys(course_list, key_attribute_name='id'): + """Returns a python set of course keys by accessing the key with the given attribute name.""" + return set(getattr(c, key_attribute_name) for c in course_list) - found_courses, unsucceeded_course_actions = method(self.request) - self.assertSetEqual(set_of_course_keys(courses + courses_in_progress), set_of_course_keys(found_courses)) - self.assertSetEqual( - set_of_course_keys(courses_in_progress), set_of_course_keys(unsucceeded_course_actions, 'course_key') - ) + found_courses, unsucceeded_course_actions = _accessible_courses_iter_for_tests(self.request) + self.assertSetEqual(_set_of_course_keys(courses + courses_in_progress), _set_of_course_keys(found_courses)) + self.assertSetEqual( + _set_of_course_keys(courses_in_progress), _set_of_course_keys(unsucceeded_course_actions, 'course_key') + ) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index a5381cc239..e45ed5e8d8 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -390,7 +390,6 @@ def _accessible_courses_summary_iter(request, org=None): def _accessible_courses_iter(request): """ List all courses available to the logged in user by iterating through all the courses. - This method is only used by tests. """ def course_filter(course): """ @@ -417,6 +416,35 @@ def _accessible_courses_iter(request): return courses, in_process_course_actions +def _accessible_courses_iter_for_tests(request): + """ + List all courses available to the logged in user by iterating through all the courses. + CourseSummary objects are used for listing purposes. + This method is only used by tests. + """ + def course_filter(course): + """ + Filter out unusable and inaccessible courses + """ + + # Custom Courses for edX (CCX) is an edX feature for re-using course content. + # CCXs cannot be edited in Studio (aka cms) and should not be shown in this dashboard. + if isinstance(course.id, CCXLocator): + return False + + # pylint: disable=fixme + # TODO remove this condition when templates purged from db + if course.location.course == 'templates': + return False + + return has_studio_read_access(request.user, course.id) + + courses = six.moves.filter(course_filter, modulestore().get_course_summaries()) + + in_process_course_actions = get_in_process_course_actions(request) + return courses, in_process_course_actions + + def _accessible_courses_list_from_groups(request): """ List all courses available to the logged in user by reversing access group names @@ -425,39 +453,23 @@ def _accessible_courses_list_from_groups(request): """ CCXs cannot be edited in Studio and should not be shown in this dashboard """ return not isinstance(course_access.course_id, CCXLocator) - courses_list = {} - in_process_course_actions = [] - instructor_courses = UserBasedRole(request.user, CourseInstructorRole.ROLE).courses_with_role() staff_courses = UserBasedRole(request.user, CourseStaffRole.ROLE).courses_with_role() all_courses = filter(filter_ccx, instructor_courses | staff_courses) + courses_list = [] + course_keys = {} for course_access in all_courses: - course_key = course_access.course_id - if course_key is None: - # If the course_access does not have a course_id, it's an org-based role, so we fall back + if course_access.course_id is None: raise AccessListFallback - if course_key not in courses_list: - # check for any course action state for this course - in_process_course_actions.extend( - CourseRerunState.objects.find_all( - exclude_args={'state': CourseRerunUIStateManager.State.SUCCEEDED}, - should_display=True, - course_key=course_key, - ) - ) - # check for the course itself - try: - course = modulestore().get_course(course_key) - except ItemNotFoundError: - # If a user has access to a course that doesn't exist, don't do anything with that course - pass + course_keys[course_access.course_id] = course_access.course_id - if course is not None and not isinstance(course, ErrorDescriptor): - # ignore deleted, errored or ccx courses - courses_list[course_key] = course + course_keys = course_keys.values() - return courses_list.values(), in_process_course_actions + if course_keys: + courses_list = modulestore().get_course_summaries(course_keys=course_keys) + + return courses_list, [] def _accessible_libraries_iter(user, org=None): diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index 09b16486d0..99122e7715 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -392,8 +392,8 @@ class TestCourseIndexArchived(CourseTestCase): @ddt.data( # Staff user has course staff access - (True, 'staff', None, 4, 21), - (False, 'staff', None, 4, 21), + (True, 'staff', None, 3, 17), + (False, 'staff', None, 3, 17), # Base user has global staff access (True, 'user', ORG, 3, 17), (False, 'user', ORG, 3, 17), diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index aa9edf474a..44eb2a8fee 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -1010,11 +1010,23 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo if field in course['metadata'] } - course_org_filter = kwargs.get('org') + course_records = [] query = {'_id.category': 'course'} + course_org_filter = kwargs.get('org') + course_keys = kwargs.get('course_keys') - if course_org_filter: - query['_id.org'] = course_org_filter + if course_keys: + course_queries = [] + for course_key in course_keys: + course_query = { + '_id.{}'.format(value_attr): getattr(course_key, key_attr) + for key_attr, value_attr in {'org': 'org', 'course': 'course', 'run': 'name'}.iteritems() + } + course_query.update(query) + course_queries.append(course_query) + query = {'$or': course_queries} + elif course_org_filter: + query['_id.org'] = course_org_filter course_records = self.collection.find(query, {'metadata': True}) @@ -1028,6 +1040,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo courses_summaries.append( CourseSummary(locator, **course_summary) ) + return courses_summaries @autoretry_read() diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index f8be210202..f12ff5ab01 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -451,7 +451,15 @@ class MongoConnection(object): } return self.course_index.find_one(query) - def find_matching_course_indexes(self, branch=None, search_targets=None, org_target=None, course_context=None): + def find_matching_course_indexes( + self, + branch=None, + search_targets=None, + org_target=None, + course_context=None, + course_keys=None + + ): """ Find the course_index matching particular conditions. @@ -464,18 +472,41 @@ class MongoConnection(object): """ with TIMER.timer("find_matching_course_indexes", course_context): query = {} - if branch is not None: - query['versions.{}'.format(branch)] = {'$exists': True} + if course_keys: + courses_queries = self._generate_query_from_course_keys(branch, course_keys) + query['$or'] = courses_queries + else: + if branch is not None: + query['versions.{}'.format(branch)] = {'$exists': True} - if search_targets: - for key, value in search_targets.iteritems(): - query['search_targets.{}'.format(key)] = value + if search_targets: + for key, value in search_targets.iteritems(): + query['search_targets.{}'.format(key)] = value - if org_target: - query['org'] = org_target + if org_target: + query['org'] = org_target return self.course_index.find(query) + def _generate_query_from_course_keys(self, branch, course_keys): + """ + Generate query for courses using course keys + """ + courses_queries = [] + query = {} + if branch: + query = {'versions.{}'.format(branch): {'$exists': True}} + + for course_key in course_keys: + course_query = { + key_attr: getattr(course_key, key_attr) + for key_attr in ('org', 'course', 'run') + } + course_query.update(query) + courses_queries.append(course_query) + + return courses_queries + def insert_course_index(self, course_index, course_context=None): """ Create the course_index in the db diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 16c238ec4b..d78e8ee0ad 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -497,7 +497,7 @@ class SplitBulkWriteMixin(BulkOperationsMixin): block_data.edit_info.original_usage = original_usage block_data.edit_info.original_usage_version = original_usage_version - def find_matching_course_indexes(self, branch=None, search_targets=None, org_target=None): + def find_matching_course_indexes(self, branch=None, search_targets=None, org_target=None, course_keys=None): """ Find the course_indexes which have the specified branch and search_targets. An optional org_target can be specified to apply an ORG filter to return only the courses that are part of @@ -506,19 +506,44 @@ class SplitBulkWriteMixin(BulkOperationsMixin): Returns: a Cursor if there are no changes in flight or a list if some have changed in current bulk op """ - indexes = self.db_connection.find_matching_course_indexes(branch, search_targets, org_target) + indexes = self.db_connection.find_matching_course_indexes( + branch, + search_targets, + org_target, + course_keys=course_keys) + indexes = self._add_indexes_from_active_records( + indexes, + branch, + search_targets, + org_target, + course_keys=course_keys + ) + + return indexes + + def _add_indexes_from_active_records( + self, + course_indexes, + branch=None, + search_targets=None, + org_target=None, + course_keys=None + ): + """ + + Add any being built but not yet persisted or in the process of being updated + """ def _replace_or_append_index(altered_index): """ If the index is already in indexes, replace it. Otherwise, append it. """ - for index, existing in enumerate(indexes): + for index, existing in enumerate(course_indexes): if all(existing[attr] == altered_index[attr] for attr in ['org', 'course', 'run']): - indexes[index] = altered_index + course_indexes[index] = altered_index return - indexes.append(altered_index) + course_indexes.append(altered_index) - # add any being built but not yet persisted or in the process of being updated for _, record in self._active_records: if branch and branch not in record.index.get('versions', {}): continue @@ -531,7 +556,6 @@ class SplitBulkWriteMixin(BulkOperationsMixin): for field, value in search_targets.iteritems() ): continue - # if we've specified a filter by org, # make sure we've honored that filter when # integrating in-transit records @@ -539,12 +563,22 @@ class SplitBulkWriteMixin(BulkOperationsMixin): if record.index['org'] != org_target: continue - if not hasattr(indexes, 'append'): # Just in time conversion to list from cursor - indexes = list(indexes) + if course_keys: + index_exists_in_active_records = False + for course_key in course_keys: + if all(record.index[key_attr] == getattr(course_key, key_attr) + for key_attr in ['org', 'course', 'run']): + index_exists_in_active_records = True + break + if not index_exists_in_active_records: + continue + + if not hasattr(course_indexes, 'append'): # Just in time conversion to list from cursor + course_indexes = list(course_indexes) _replace_or_append_index(record.index) - return indexes + return course_indexes def find_course_blocks_by_id(self, ids): """ @@ -905,15 +939,15 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): def collect_ids_from_matching_indexes(self, branch, **kwargs): """ - Find the course_indexes which have the specified branch. if `kwargs` contains `org` - to apply an ORG filter to return only the courses that are part of that ORG. Extract `version_guids` + Find the course_indexes which have the specified branch. Extract `version_guids` from the course_indexes. """ matching_indexes = self.find_matching_course_indexes( branch, search_targets=None, - org_target=kwargs.get('org') + org_target=kwargs.get('org'), + course_keys=kwargs.get('course_keys') ) # collect ids and then query for those diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py index 7b845d304a..e70b0dc1a6 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py @@ -302,7 +302,13 @@ class TestBulkWriteMixinFindMethods(TestBulkWriteMixin): org_targets = None self.conn.find_matching_course_indexes.return_value = [Mock(name='result')] result = self.bulk.find_matching_course_indexes(branch, search_targets) - self.assertConnCalls(call.find_matching_course_indexes(branch, search_targets, org_targets)) + self.assertConnCalls(call.find_matching_course_indexes( + branch, + search_targets, + org_targets, + course_keys=None + ) + ) self.assertEqual(result, self.conn.find_matching_course_indexes.return_value) self.assertCacheNotCleared()