Skip to content

E
edx-platform
Commit cb3f8584 by noraiz-anwar
Options

use CourseSummary for course listing on studio for non-global staff users

parent 725edbbc
Showing with 179 additions and 143 deletions
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,37 +153,11 @@ 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)
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')
)
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 = _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')
)
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
course_keys = course_keys.values()
if course is not None and not isinstance(course, ErrorDescriptor):
# ignore deleted, errored or ccx courses
courses_list[course_key] = course
if course_keys:
courses_list = modulestore().get_course_summaries(course_keys=course_keys)
return courses_list.values(), in_process_course_actions
return courses_list, []
def _accessible_libraries_iter(user, org=None):
......
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),
......
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'}
if course_org_filter:
query['_id.org'] = course_org_filter
course_org_filter = kwargs.get('org')
course_keys = kwargs.get('course_keys')
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()
......
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
......
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
......
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()
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment