Skip to content

E
edx-platform
Commit 8a4bb2e6 by Cliff Dyer Committed by GitHub
Options

Merge pull request #15184 from edx/neem/cms-dash-inefficiencies 

Cleanup inefficiencies in Studio dashboard
parents 8bb56fa4 6c6f271e
Showing with 98 additions and 79 deletions
cms/djangoapps/contentstore/tests/test_course_listing.py
......@@ -17,11 +17,11 @@ from common.test.utils import XssTestMixin
from xmodule.course_module import CourseSummary
from contentstore.views.course import (
_accessible_courses_list,
_accessible_courses_iter,
_accessible_courses_list_from_groups,
AccessListFallback,
get_courses_accessible_to_user,
_accessible_courses_summary_list,
_accessible_courses_summary_iter,
)
from contentstore.utils import delete_course_and_groups
from contentstore.tests.utils import AjaxEnabledTestClient
......@@ -132,11 +132,12 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
self._create_course_with_access_groups(course_location, self.user)
# get courses through iterating all courses
courses_list, __ = _accessible_courses_list(self.request)
courses_iter, __ = _accessible_courses_iter(self.request)
courses_list = list(courses_iter)
self.assertEqual(len(courses_list), 1)
courses_summary_list, __ = _accessible_courses_summary_list(self.request)
self.assertEqual(len(courses_summary_list), 1)
courses_summary_list, __ = _accessible_courses_summary_iter(self.request)
self.assertEqual(len(list(courses_summary_list)), 1)
# get courses by reversing group name formats
courses_list_by_groups, __ = _accessible_courses_list_from_groups(self.request)
......@@ -179,8 +180,8 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
# 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_list, __ = _accessible_courses_list(self.request)
self.assertEqual(len(courses_list), 0)
courses_iter, __ = _accessible_courses_iter(self.request)
self.assertEqual(len(list(courses_iter)), 0)
@ddt.data(
(ModuleStoreEnum.Type.split, 'xmodule.modulestore.split_mongo.split_mongo_kvs.SplitMongoKVS'),
......@@ -201,8 +202,8 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
self.assertIsInstance(self.store.get_course(course_key), ErrorDescriptor)
# get courses through iterating all courses
courses_list, __ = _accessible_courses_list(self.request)
self.assertEqual(courses_list, [])
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)
......@@ -231,14 +232,14 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
# Fetch accessible courses list & verify their count
courses_list_by_staff, __ = get_courses_accessible_to_user(self.request)
self.assertEqual(len(courses_list_by_staff), TOTAL_COURSES_COUNT)
self.assertEqual(len(list(courses_list_by_staff)), TOTAL_COURSES_COUNT)
# Verify fetched accessible courses list is a list of CourseSummery instances
self.assertTrue(all(isinstance(course, CourseSummary) for course in courses_list_by_staff))
# Now count the db queries for staff
with check_mongo_calls(mongo_calls):
_accessible_courses_summary_list(self.request)
list(_accessible_courses_summary_iter(self.request))
@ddt.data(
(ModuleStoreEnum.Type.split, 'xmodule.modulestore.split_mongo.split_mongo_kvs.SplitMongoKVS'),
......@@ -261,7 +262,8 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
self.assertIsInstance(self.store.get_course(course_key), ErrorDescriptor)
# get courses through iterating all courses
courses_list, __ = _accessible_courses_list(self.request)
courses_iter, __ = _accessible_courses_iter(self.request)
courses_list = list(courses_iter)
self.assertEqual(courses_list, [])
# get courses by reversing group name formats
......@@ -279,10 +281,12 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
self._create_course_with_access_groups(course_key, self.user, store)
# get courses through iterating all courses
courses_list, __ = _accessible_courses_list(self.request)
courses_iter, __ = _accessible_courses_iter(self.request)
courses_list = list(courses_iter)
self.assertEqual(len(courses_list), 1)
courses_summary_list, __ = _accessible_courses_summary_list(self.request)
courses_summary_iter, __ = _accessible_courses_summary_iter(self.request)
courses_summary_list = list(courses_summary_iter)
# Verify fetched accessible courses list is a list of CourseSummery instances and only one course
# is returned
......@@ -302,17 +306,17 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
CourseInstructorRole(course_key).add_users(self.user)
# Get courses through iterating all courses
courses_list, __ = _accessible_courses_list(self.request)
courses_iter, __ = _accessible_courses_iter(self.request)
# Get course summaries by iterating all courses
courses_summary_list, __ = _accessible_courses_summary_list(self.request)
courses_summary_iter, __ = _accessible_courses_summary_iter(self.request)
# Get courses by reversing group name formats
courses_list_by_groups, __ = _accessible_courses_list_from_groups(self.request)
# Test that course list returns no course
self.assertEqual(
[len(courses_list), len(courses_list_by_groups), len(courses_summary_list)],
[len(list(courses_iter)), len(courses_list_by_groups), len(list(courses_summary_iter))],
[0, 0, 0]
)
......@@ -344,13 +348,13 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
# time the get courses by iterating through all courses
with Timer() as iteration_over_courses_time_1:
courses_list, __ = _accessible_courses_list(self.request)
self.assertEqual(len(courses_list), USER_COURSES_COUNT)
courses_iter, __ = _accessible_courses_iter(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_list, __ = _accessible_courses_list(self.request)
self.assertEqual(len(courses_list), USER_COURSES_COUNT)
courses_iter, __ = _accessible_courses_iter(self.request)
self.assertEqual(len(list(courses_iter)), USER_COURSES_COUNT)
# time the get courses by reversing django groups
with Timer() as iteration_over_groups_time_1:
......@@ -362,17 +366,25 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
courses_list, __ = _accessible_courses_list_from_groups(self.request)
self.assertEqual(len(courses_list), USER_COURSES_COUNT)
# 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)
# 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)
# 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):
_accessible_courses_list(self.request)
list(_accessible_courses_iter(self.request))
# Calls:
# 1) query old mongo
# 2) get_more on old mongo
......@@ -424,7 +436,7 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
# Verify fetched accessible courses list is a list of CourseSummery instances and test expacted
# course count is returned
self.assertEqual(len(courses_list), 2)
self.assertEqual(len(list(courses_list)), 2)
self.assertTrue(all(isinstance(course, CourseSummary) for course in courses_list))
def test_course_listing_with_actions_in_progress(self):
......@@ -432,11 +444,17 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
num_courses_to_create = 3
courses = [
self._create_course_with_access_groups(CourseLocator('Org', 'CreatedCourse' + str(num), 'Run'), self.user)
self._create_course_with_access_groups(
CourseLocator('Org', 'CreatedCourse' + str(num), 'Run'),
self.user,
)
for num in range(num_courses_to_create)
]
courses_in_progress = [
self._create_course_with_access_groups(CourseLocator('Org', 'InProgressCourse' + str(num), 'Run'), self.user)
self._create_course_with_access_groups(
CourseLocator('Org', 'InProgressCourse' + str(num), 'Run'),
self.user,
)
for num in range(num_courses_to_create)
]
......@@ -447,7 +465,7 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
)
# verify return values
for method in (_accessible_courses_list_from_groups, _accessible_courses_list):
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)
......
cms/djangoapps/contentstore/views/course.py
......@@ -17,6 +17,7 @@ import django.utils
from django.utils.translation import ugettext as _
from django.views.decorators.http import require_http_methods, require_GET
from django.views.decorators.csrf import ensure_csrf_cookie
import six
from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import CourseKey
......@@ -357,13 +358,14 @@ def get_in_process_course_actions(request):
return [
course for course in
CourseRerunState.objects.find_all(
exclude_args={'state': CourseRerunUIStateManager.State.SUCCEEDED}, should_display=True
exclude_args={'state': CourseRerunUIStateManager.State.SUCCEEDED},
should_display=True,
)
if has_studio_read_access(request.user, course.course_key)
]
def _accessible_courses_summary_list(request):
def _accessible_courses_summary_iter(request):
"""
List all courses available to the logged in user by iterating through all the courses
"""
......@@ -378,12 +380,12 @@ def _accessible_courses_summary_list(request):
return has_studio_read_access(request.user, course_summary.id)
courses_summary = filter(course_filter, modulestore().get_course_summaries())
courses_summary = six.moves.filter(course_filter, modulestore().get_course_summaries())
in_process_course_actions = get_in_process_course_actions(request)
return courses_summary, in_process_course_actions
def _accessible_courses_list(request):
def _accessible_courses_iter(request):
"""
List all courses available to the logged in user by iterating through all the courses
"""
......@@ -406,7 +408,7 @@ def _accessible_courses_list(request):
return has_studio_read_access(request.user, course.id)
courses = filter(course_filter, modulestore().get_courses())
courses = six.moves.filter(course_filter, modulestore().get_courses())
in_process_course_actions = get_in_process_course_actions(request)
return courses, in_process_course_actions
......@@ -455,12 +457,12 @@ def _accessible_courses_list_from_groups(request):
return courses_list.values(), in_process_course_actions
def _accessible_libraries_list(user):
def _accessible_libraries_iter(user):
"""
List all libraries available to the logged in user by iterating through all libraries
"""
# No need to worry about ErrorDescriptors - split's get_libraries() never returns them.
return [lib for lib in modulestore().get_libraries() if has_studio_read_access(user, lib.location.library_key)]
return (lib for lib in modulestore().get_libraries() if has_studio_read_access(user, lib.location.library_key))
@login_required
......@@ -469,29 +471,29 @@ def course_listing(request):
"""
List all courses available to the logged in user
"""
courses, in_process_course_actions = get_courses_accessible_to_user(request)
courses_iter, in_process_course_actions = get_courses_accessible_to_user(request)
user = request.user
libraries = _accessible_libraries_list(request.user) if LIBRARIES_ENABLED else []
libraries = _accessible_libraries_iter(request.user) if LIBRARIES_ENABLED else []
def format_in_process_course_view(uca):
"""
Return a dict of the data which the view requires for each unsucceeded course
"""
return {
'display_name': uca.display_name,
'course_key': unicode(uca.course_key),
'org': uca.course_key.org,
'number': uca.course_key.course,
'run': uca.course_key.run,
'is_failed': True if uca.state == CourseRerunUIStateManager.State.FAILED else False,
'is_in_progress': True if uca.state == CourseRerunUIStateManager.State.IN_PROGRESS else False,
'dismiss_link': reverse_course_url(
'course_notifications_handler',
u'display_name': uca.display_name,
u'course_key': unicode(uca.course_key),
u'org': uca.course_key.org,
u'number': uca.course_key.course,
u'run': uca.course_key.run,
u'is_failed': True if uca.state == CourseRerunUIStateManager.State.FAILED else False,
u'is_in_progress': True if uca.state == CourseRerunUIStateManager.State.IN_PROGRESS else False,
u'dismiss_link': reverse_course_url(
u'course_notifications_handler',
uca.course_key,
kwargs={
'action_state_id': uca.id,
u'action_state_id': uca.id,
},
) if uca.state == CourseRerunUIStateManager.State.FAILED else ''
) if uca.state == CourseRerunUIStateManager.State.FAILED else u''
}
def format_library_for_view(library):
......@@ -499,29 +501,29 @@ def course_listing(request):
Return a dict of the data which the view requires for each library
"""
return {
'display_name': library.display_name,
'library_key': unicode(library.location.library_key),
'url': reverse_library_url('library_handler', unicode(library.location.library_key)),
'org': library.display_org_with_default,
'number': library.display_number_with_default,
'can_edit': has_studio_write_access(request.user, library.location.library_key),
u'display_name': library.display_name,
u'library_key': unicode(library.location.library_key),
u'url': reverse_library_url(u'library_handler', unicode(library.location.library_key)),
u'org': library.display_org_with_default,
u'number': library.display_number_with_default,
u'can_edit': has_studio_write_access(request.user, library.location.library_key),
}
courses = _remove_in_process_courses(courses, in_process_course_actions)
courses_iter = _remove_in_process_courses(courses_iter, in_process_course_actions)
in_process_course_actions = [format_in_process_course_view(uca) for uca in in_process_course_actions]
return render_to_response('index.html', {
'courses': courses,
'in_process_course_actions': in_process_course_actions,
'libraries_enabled': LIBRARIES_ENABLED,
'libraries': [format_library_for_view(lib) for lib in libraries],
'show_new_library_button': get_library_creator_status(user),
'user': user,
'request_course_creator_url': reverse('contentstore.views.request_course_creator'),
'course_creator_status': _get_course_creator_status(user),
'rerun_creator_status': GlobalStaff().has_user(user),
'allow_unicode_course_id': settings.FEATURES.get('ALLOW_UNICODE_COURSE_ID', False),
'allow_course_reruns': settings.FEATURES.get('ALLOW_COURSE_RERUNS', True),
return render_to_response(u'index.html', {
u'courses': list(courses_iter),
u'in_process_course_actions': in_process_course_actions,
u'libraries_enabled': LIBRARIES_ENABLED,
u'libraries': [format_library_for_view(lib) for lib in libraries],
u'show_new_library_button': get_library_creator_status(user),
u'user': user,
u'request_course_creator_url': reverse(u'contentstore.views.request_course_creator'),
u'course_creator_status': _get_course_creator_status(user),
u'rerun_creator_status': GlobalStaff().has_user(user),
u'allow_unicode_course_id': settings.FEATURES.get(u'ALLOW_UNICODE_COURSE_ID', False),
u'allow_course_reruns': settings.FEATURES.get(u'ALLOW_COURSE_RERUNS', True),
})
......@@ -623,18 +625,18 @@ def get_courses_accessible_to_user(request):
"""
if GlobalStaff().has_user(request.user):
# user has global access so no need to get courses from django groups
courses, in_process_course_actions = _accessible_courses_summary_list(request)
courses, in_process_course_actions = _accessible_courses_summary_iter(request)
else:
try:
courses, in_process_course_actions = _accessible_courses_list_from_groups(request)
except AccessListFallback:
# user have some old groups or there was some error getting courses from django groups
# so fallback to iterating through all courses
courses, in_process_course_actions = _accessible_courses_summary_list(request)
courses, in_process_course_actions = _accessible_courses_summary_iter(request)
return courses, in_process_course_actions
def _remove_in_process_courses(courses, in_process_course_actions):
def _remove_in_process_courses(courses_iter, in_process_course_actions):
"""
removes any in-process courses in courses list. in-process actually refers to courses
that are in the process of being generated for re-run
......@@ -654,13 +656,12 @@ def _remove_in_process_courses(courses, in_process_course_actions):
'run': course.location.run
}
in_process_action_course_keys = [uca.course_key for uca in in_process_course_actions]
courses = [
in_process_action_course_keys = {uca.course_key for uca in in_process_course_actions}
return (
format_course_for_view(course)
for course in courses
for course in courses_iter
if not isinstance(course, ErrorDescriptor) and (course.id not in in_process_action_course_keys)
]
return courses
)
def course_outline_initial_state(locator_to_show, course_structure):
......@@ -1014,10 +1015,10 @@ def settings_handler(request, course_key_string):
if is_prerequisite_courses_enabled():
courses, in_process_course_actions = get_courses_accessible_to_user(request)
# exclude current course from the list of available courses
courses = [course for course in courses if course.id != course_key]
courses = (course for course in courses if course.id != course_key)
if courses:
courses = _remove_in_process_courses(courses, in_process_course_actions)
settings_context.update({'possible_pre_requisite_courses': courses})
settings_context.update({'possible_pre_requisite_courses': list(courses)})
if credit_eligibility_enabled:
if is_credit_course(course_key):
......
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