From ee75db27031089d639ee7f3f9be0bd5306a5b66a Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 22 Jan 2019 14:50:11 -0500 Subject: [PATCH 1/5] Add a test that shows how bad course api query counts are --- lms/djangoapps/course_api/tests/test_views.py | 58 +++++++++++++++++++ openedx/core/djangolib/testing/utils.py | 3 + 2 files changed, 61 insertions(+) diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index 974752be37..92a4fd9da9 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -1,6 +1,7 @@ """ Tests for Course API views. """ +from datetime import datetime import ddt from hashlib import md5 @@ -12,8 +13,14 @@ from search.tests.test_course_discovery import DemoCourse from search.tests.tests import TEST_INDEX_NAME from search.tests.utils import SearcherMixin +from course_modes.models import CourseMode +from course_modes.tests.factories import CourseModeFactory +from edx_django_utils.cache import RequestCache from openedx.core.lib.tests import attr +from openedx.features.content_type_gating.models import ContentTypeGatingConfig +from openedx.features.course_duration_limits.models import CourseDurationLimitConfig from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory from waffle.testutils import override_switch from ..views import CourseDetailView, CourseListUserThrottle @@ -283,6 +290,7 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear """ ENABLED_SIGNALS = ['course_published'] + ENABLED_CACHES = ModuleStoreTestCase.ENABLED_CACHES + ['configuration'] def setUp(self): super(CourseListSearchViewTest, self).setUp() @@ -298,6 +306,7 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear self.url = reverse('course-list') self.staff_user = self.create_user(username='staff', is_staff=True) self.honor_user = self.create_user(username='honor', is_staff=False) + self.audit_user = self.create_user(username='audit', is_staff=False) def create_and_index_course(self, org_code, short_description): """ @@ -348,3 +357,52 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear self.assertIn('results', res.data) self.assertNotEqual(res.data['results'], []) self.assertEqual(res.data['pagination']['count'], 1) # Should list a single course + + def test_too_many_courses(self): + """ + Test that search results are limited to 100 courses, and that they don't + blow up the database. + """ + + ContentTypeGatingConfig.objects.create( + enabled=True, + enabled_as_of=datetime(2018, 1, 1), + ) + + CourseDurationLimitConfig.objects.create( + enabled=True, + enabled_as_of=datetime(2018, 1, 1), + ) + + course_ids = [] + + # Create 300 courses across 30 organizations + for org_num in range(10): + org_id = 'org{}'.format(org_num) + for course_num in range(30): + course_name = 'course{}.{}'.format(org_num, course_num) + course_run_name = 'run{}.{}'.format(org_num, course_num) + course = CourseFactory.create(org=org_id, number=course_name, run=course_run_name, emit_signals=True) + CourseModeFactory.create(course_id=course.id, mode_slug=CourseMode.AUDIT) + CourseModeFactory.create(course_id=course.id, mode_slug=CourseMode.VERIFIED) + course_ids.append(course.id) + + self.setup_user(self.audit_user) + + # These query counts were found empirically + query_counts = [1266, 349, 349, 349, 349, 349, 349, 349, 349, 349, 322] + ordered_course_ids = sorted([str(cid) for cid in (course_ids + [c.id for c in self.courses])]) + + self.clear_caches() + + for page in range(1, 12): + with self.assertNumQueries(query_counts[page - 1]): + response = self.verify_response(params={'page': page, 'page_size': 30}) + + self.assertIn('results', response.data) + self.assertEqual(response.data['pagination']['count'], 303) + self.assertEqual(len(response.data['results']), 30 if page < 11 else 3) + self.assertEqual( + [c['id'] for c in response.data['results']], + ordered_course_ids[(page - 1) * 30:page * 30] + ) diff --git a/openedx/core/djangolib/testing/utils.py b/openedx/core/djangolib/testing/utils.py index 710f5a35ba..3d620385e8 100644 --- a/openedx/core/djangolib/testing/utils.py +++ b/openedx/core/djangolib/testing/utils.py @@ -73,6 +73,9 @@ class CacheIsolationMixin(object): 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', 'LOCATION': cache_name, 'KEY_FUNCTION': 'util.memcache.safe_key', + 'OPTIONS': { + 'MAX_ENTRIES': 1000, + }, } for cache_name in cls.ENABLED_CACHES }) From a3541d6e46f118bd34c813560f7df78edfeb5ef3 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 22 Jan 2019 14:50:45 -0500 Subject: [PATCH 2/5] Allow courses api to return data incrementally Prior to this commit, the course api (/api/courses/v1/courses/) performed all the work necessary to return all courses available to the user, and then only actually returned on page's worth of those courses. With this change, the api now does the work incrementally, computing only the data needed to fetch the courses up to and including the page being returned. This still increases approximately linearly as the page number accessed being increases, but should be more cache-friendly. One side effect of this is that the max_page reported by pagination will be an overestimate (it will include pages that are removed due to a users access restrictions). This change also changes the sort-order of courses being returned by the course_api. By sorting by course-id, rather than course-number, we can sort in the database, rather than in Python, and defer loading data from the end of the list until it is requested. REVMI-90 --- lms/djangoapps/branding/__init__.py | 11 ++- lms/djangoapps/course_api/api.py | 4 +- lms/djangoapps/course_api/tests/test_api.py | 4 +- lms/djangoapps/course_api/tests/test_views.py | 6 +- lms/djangoapps/course_api/views.py | 16 +-- lms/djangoapps/courseware/courses.py | 11 ++- .../courseware/tests/test_courses.py | 2 +- .../content/course_overviews/models.py | 5 +- openedx/core/lib/api/view_utils.py | 98 +++++++++++++++++++ 9 files changed, 132 insertions(+), 25 deletions(-) diff --git a/lms/djangoapps/branding/__init__.py b/lms/djangoapps/branding/__init__.py index 534c0c4faf..e467d96ef4 100644 --- a/lms/djangoapps/branding/__init__.py +++ b/lms/djangoapps/branding/__init__.py @@ -15,7 +15,7 @@ from openedx.core.djangoapps.site_configuration import helpers as configuration_ def get_visible_courses(org=None, filter_=None): """ - Return the set of CourseOverviews that should be visible in this branded + Yield the CourseOverviews that should be visible in this branded instance. Arguments: @@ -27,9 +27,10 @@ def get_visible_courses(org=None, filter_=None): # Import is placed here to avoid model import at project startup. from openedx.core.djangoapps.content.course_overviews.models import CourseOverview - courses = [] current_site_orgs = configuration_helpers.get_current_site_orgs() + courses = CourseOverview.objects.none() + if org: # Check the current site's orgs to make sure the org's courses should be displayed if not current_site_orgs or org in current_site_orgs: @@ -40,7 +41,7 @@ def get_visible_courses(org=None, filter_=None): else: courses = CourseOverview.get_all_courses(filter_=filter_) - courses = sorted(courses, key=lambda course: course.number) + courses = courses.order_by('id') # Filtering can stop here. if current_site_orgs: @@ -57,11 +58,11 @@ def get_visible_courses(org=None, filter_=None): ) if filtered_visible_ids: - return [course for course in courses if course.id in filtered_visible_ids] + return courses.filter(id__in=filtered_visible_ids) else: # Filter out any courses based on current org, to avoid leaking these. orgs = configuration_helpers.get_all_orgs() - return [course for course in courses if course.location.org not in orgs] + return courses.exclude(org__in=orgs) def get_university_for_request(): diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index 79a3b38796..ef25e61399 100644 --- a/lms/djangoapps/course_api/api.py +++ b/lms/djangoapps/course_api/api.py @@ -57,7 +57,7 @@ def course_detail(request, username, course_key): def list_courses(request, username, org=None, filter_=None): """ - Return a list of available courses. + Yield all available courses. The courses returned are all be visible to the user identified by `username` and the logged in user should have permission to view courses @@ -81,7 +81,7 @@ def list_courses(request, username, org=None, filter_=None): by the given key-value pairs. Return value: - List of `CourseOverview` objects representing the collection of courses. + Yield `CourseOverview` objects representing the collection of courses. """ user = get_effective_user(request.user, username) return get_courses(user, org=org, filter_=filter_) diff --git a/lms/djangoapps/course_api/tests/test_api.py b/lms/djangoapps/course_api/tests/test_api.py index cef5d4c739..7670acbfa1 100644 --- a/lms/djangoapps/course_api/tests/test_api.py +++ b/lms/djangoapps/course_api/tests/test_api.py @@ -231,12 +231,12 @@ class TestGetCourseListExtras(CourseListTestMixin, ModuleStoreTestCase): def test_no_courses(self): courses = self._make_api_call(self.honor_user, self.honor_user) - self.assertEqual(len(courses), 0) + self.assertEqual(len(list(courses)), 0) def test_hidden_course_for_honor(self): self.create_course(visible_to_staff_only=True) courses = self._make_api_call(self.honor_user, self.honor_user) - self.assertEqual(len(courses), 0) + self.assertEqual(len(list(courses)), 0) def test_hidden_course_for_staff(self): self.create_course(visible_to_staff_only=True) diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index 92a4fd9da9..5541e511ca 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -356,7 +356,9 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear res = self.verify_response(params={'search_term': 'unique search term'}) self.assertIn('results', res.data) self.assertNotEqual(res.data['results'], []) - self.assertEqual(res.data['pagination']['count'], 1) # Should list a single course + # Returns a count of 3 courses because that's the estimate before filtering + self.assertEqual(res.data['pagination']['count'], 3) + self.assertEqual(len(res.data['results']), 1) # Should return a single course def test_too_many_courses(self): """ @@ -390,7 +392,7 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear self.setup_user(self.audit_user) # These query counts were found empirically - query_counts = [1266, 349, 349, 349, 349, 349, 349, 349, 349, 349, 322] + query_counts = [174, 196, 226, 256, 286, 316, 346, 376, 406, 436, 331] ordered_course_ids = sorted([str(cid) for cid in (course_ids + [c.id for c in self.courses])]) self.clear_caches() diff --git a/lms/djangoapps/course_api/views.py b/lms/djangoapps/course_api/views.py index 66743ea079..26d7986342 100644 --- a/lms/djangoapps/course_api/views.py +++ b/lms/djangoapps/course_api/views.py @@ -10,6 +10,7 @@ from rest_framework.throttling import UserRateThrottle from edx_rest_framework_extensions.paginators import NamespacedPageNumberPagination from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes +from openedx.core.lib.api.view_utils import LazySequence from . import USE_RATE_LIMIT_2_FOR_COURSE_LIST_API, USE_RATE_LIMIT_10_FOR_COURSE_LIST_API from .api import course_detail, list_courses @@ -243,7 +244,7 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): def get_queryset(self): """ - Return a list of courses visible to the user. + Yield courses visible to the user. """ form = CourseListGetForm(self.request.query_params, initial={'requesting_user': self.request.user}) if not form.is_valid(): @@ -264,9 +265,12 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): size=self.results_size_infinity, ) - search_courses_ids = {course['data']['id']: True for course in search_courses['results']} + search_courses_ids = {course['data']['id'] for course in search_courses['results']} - return [ - course for course in db_courses - if unicode(course.id) in search_courses_ids - ] + return LazySequence( + ( + course for course in db_courses + if unicode(course.id) in search_courses_ids + ), + est_len=len(db_courses) + ) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index caccff1313..9fc73ec0a8 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -35,6 +35,7 @@ from lms.djangoapps.courseware.exceptions import CourseAccessRedirect from opaque_keys.edx.keys import UsageKey from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from openedx.core.lib.api.view_utils import LazySequence from path import Path as path from six import text_type from static_replace import replace_static_urls @@ -451,8 +452,7 @@ def get_course_syllabus_section(course, section_key): def get_courses(user, org=None, filter_=None): """ - Returns a list of courses available, sorted by course.number and optionally - filtered by org code (case-insensitive). + Return a LazySequence of courses available, optionally filtered by org code (case-insensitive). """ courses = branding.get_visible_courses(org=org, filter_=filter_) @@ -461,9 +461,10 @@ def get_courses(user, org=None, filter_=None): settings.COURSE_CATALOG_VISIBILITY_PERMISSION ) - courses = [c for c in courses if has_access(user, permission_name, c)] - - return courses + return LazySequence( + (c for c in courses if has_access(user, permission_name, c)), + est_len=courses.count() + ) def get_permission_for_course_about(): diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py index c54d98e4e2..12d694524c 100644 --- a/lms/djangoapps/courseware/tests/test_courses.py +++ b/lms/djangoapps/courseware/tests/test_courses.py @@ -141,7 +141,7 @@ class CoursesTest(ModuleStoreTestCase): # Request filtering for an org distinct from the designated org. no_courses = get_courses(user, org=primary) - self.assertEqual(no_courses, []) + self.assertEqual(list(no_courses), []) # Request filtering for an org matching the designated org. site_courses = get_courses(user, org=alternate) diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index e852cce1b0..6d1998c92f 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -409,7 +409,8 @@ class CourseOverview(TimeStampedModel): migrate and test switching to display_name_with_default, which is no longer escaped. """ - return block_metadata_utils.display_name_with_default_escaped(self) + # pylint: disable=line-too-long + return block_metadata_utils.display_name_with_default_escaped(self) # xss-lint: disable=python-deprecated-display-name @property def dashboard_start_display(self): @@ -560,7 +561,7 @@ class CourseOverview(TimeStampedModel): @classmethod def get_all_courses(cls, orgs=None, filter_=None): """ - Returns all CourseOverview objects in the database. + Return a queryset containing all CourseOverview objects in the database. Arguments: orgs (list[string]): Optional parameter that allows case-insensitive diff --git a/openedx/core/lib/api/view_utils.py b/openedx/core/lib/api/view_utils.py index ef67c4eb82..61c8dbd319 100644 --- a/openedx/core/lib/api/view_utils.py +++ b/openedx/core/lib/api/view_utils.py @@ -1,6 +1,7 @@ """ Utilities related to API views """ +from collections import Sequence from django.core.exceptions import NON_FIELD_ERRORS, ObjectDoesNotExist, ValidationError from django.http import Http404 from django.utils.translation import ugettext as _ @@ -205,3 +206,100 @@ class RetrievePatchAPIView(RetrieveModelMixin, UpdateModelMixin, GenericAPIView) # PATCH requests where the object does not exist should still # return a 404 response. raise + + +class LazySequence(Sequence): + """ + This class provides an immutable Sequence interface on top of an existing + iterable. + + It is immutable, and accepts an estimated length in order to support __len__ + without exhausting the underlying sequence + """ + def __init__(self, iterable, est_len=None): # pylint: disable=super-init-not-called + self.iterable = iterable + self.est_len = est_len + self._data = [] + self._exhausted = False + + def __len__(self): + # Return the actual data length if we know it exactly (because + # the underlying sequence is exhausted), or it's greater than + # the initial estimated length + if len(self._data) > self.est_len or self._exhausted: + return len(self._data) + else: + return self.est_len + + def __iter__(self): + # Yield all the known data first + for item in self._data: + yield item + + # Capture and yield data from the underlying iterator + # until it is exhausted + while True: + try: + item = next(self.iterable) + self._data.append(item) + yield item + except StopIteration: + self._exhausted = True + return + + def __getitem__(self, index): + if isinstance(index, int): + # For a single index, if we haven't already loaded enough + # data, we can load data until we have enough, and then + # return the value from the loaded data + if index < 0: + raise IndexError("Negative indexes aren't supported") + + while len(self._data) <= index: + try: + self._data.append(next(self.iterable)) + except StopIteration: + self._exhausted = True + raise IndexError("Underlying sequence exhausted") + + return self._data[index] + elif isinstance(index, slice): + # For a slice, we can load data until we reach 'stop'. + # Once we have data including 'stop', then we can use + # the underlying list to actually understand the mechanics + # of the slicing operation. + if index.start is not None and index.start < 0: + raise IndexError("Negative indexes aren't supported") + if index.stop is not None and index.stop < 0: + raise IndexError("Negative indexes aren't supported") + + if index.step is not None and index.step < 0: + largest_value = index.start + 1 + else: + largest_value = index.stop + + if largest_value is not None: + while len(self._data) <= largest_value: + try: + self._data.append(next(self.iterable)) + except StopIteration: + self._exhausted = True + break + else: + self._data.extend(self.iterable) + return self._data[index] + else: + raise TypeError("Unsupported index type") + + def __repr__(self): + if self._exhausted: + return "LazySequence({!r}, {!r})".format( + self._data, + self.est_len, + ) + else: + return "LazySequence(itertools.chain({!r}, {!r}), {!r})".format( + self._data, + self.iterable, + self.est_len, + ) From a842921e1fe756e84f5525db56225556c2b404ef Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 22 Jan 2019 21:21:27 -0500 Subject: [PATCH 3/5] Cache org-site lookups in the RequestCache --- lms/djangoapps/course_api/tests/test_views.py | 3 +- .../djangoapps/config_model_utils/models.py | 3 ++ openedx/core/lib/cache_utils.py | 43 ++++++++----------- .../content_type_gating/tests/test_models.py | 37 ++++++++++++++++ .../tests/test_models.py | 37 ++++++++++++++++ 5 files changed, 98 insertions(+), 25 deletions(-) diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index 5541e511ca..0acba1b764 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -392,12 +392,13 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear self.setup_user(self.audit_user) # These query counts were found empirically - query_counts = [174, 196, 226, 256, 286, 316, 346, 376, 406, 436, 331] + query_counts = [122, 140, 170, 200, 230, 260, 290, 320, 350, 380, 327] ordered_course_ids = sorted([str(cid) for cid in (course_ids + [c.id for c in self.courses])]) self.clear_caches() for page in range(1, 12): + RequestCache.clear_all_namespaces() with self.assertNumQueries(query_counts[page - 1]): response = self.verify_response(params={'page': page, 'page_size': 30}) diff --git a/openedx/core/djangoapps/config_model_utils/models.py b/openedx/core/djangoapps/config_model_utils/models.py index 7e47db7d1b..2d29ac3605 100644 --- a/openedx/core/djangoapps/config_model_utils/models.py +++ b/openedx/core/djangoapps/config_model_utils/models.py @@ -22,6 +22,7 @@ import crum from config_models.models import ConfigurationModel, cache from openedx.core.djangoapps.site_configuration.models import SiteConfiguration from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.lib.cache_utils import request_cached class Provenance(Enum): @@ -252,7 +253,9 @@ class StackedConfigurationModel(ConfigurationModel): return course_key.org @classmethod + @request_cached() def _site_from_org(cls, org): + configuration = SiteConfiguration.get_configuration_for_org(org, select_related=['site']) if configuration is None: try: diff --git a/openedx/core/lib/cache_utils.py b/openedx/core/lib/cache_utils.py index ba4324143d..3910d33f7c 100644 --- a/openedx/core/lib/cache_utils.py +++ b/openedx/core/lib/cache_utils.py @@ -9,6 +9,7 @@ import zlib from django.utils.encoding import force_text from edx_django_utils.cache import RequestCache +import wrapt def request_cached(namespace=None, arg_map_function=None, request_cache_getter=None): @@ -47,38 +48,32 @@ def request_cached(namespace=None, arg_map_function=None, request_cache_getter=N cache the value it returns, and return that cached value for subsequent calls with the same args/kwargs within a single request. """ - def decorator(f): + @wrapt.decorator + def decorator(wrapped, instance, args, kwargs): """ Arguments: - f (func): the function to wrap + args, kwargs: values passed into the wrapped function """ - @functools.wraps(f) - def _decorator(*args, **kwargs): - """ - Arguments: - args, kwargs: values passed into the wrapped function - """ - # Check to see if we have a result in cache. If not, invoke our wrapped - # function. Cache and return the result to the caller. - if request_cache_getter: - request_cache = request_cache_getter(args, kwargs) - else: - request_cache = RequestCache(namespace) + # Check to see if we have a result in cache. If not, invoke our wrapped + # function. Cache and return the result to the caller. + if request_cache_getter: + request_cache = request_cache_getter(args if instance is None else (instance,) + args, kwargs) + else: + request_cache = RequestCache(namespace) - if request_cache: - cache_key = _func_call_cache_key(f, arg_map_function, *args, **kwargs) - cached_response = request_cache.get_cached_response(cache_key) - if cached_response.is_found: - return cached_response.value + if request_cache: + cache_key = _func_call_cache_key(wrapped, arg_map_function, *args, **kwargs) + cached_response = request_cache.get_cached_response(cache_key) + if cached_response.is_found: + return cached_response.value - result = f(*args, **kwargs) + result = wrapped(*args, **kwargs) - if request_cache: - request_cache.set(cache_key, result) + if request_cache: + request_cache.set(cache_key, result) - return result + return result - return _decorator return decorator diff --git a/openedx/features/content_type_gating/tests/test_models.py b/openedx/features/content_type_gating/tests/test_models.py index e51a16b529..aca44f17bc 100644 --- a/openedx/features/content_type_gating/tests/test_models.py +++ b/openedx/features/content_type_gating/tests/test_models.py @@ -6,6 +6,7 @@ from django.utils import timezone from mock import Mock import pytz +from edx_django_utils.cache import RequestCache from opaque_keys.edx.locator import CourseLocator from openedx.core.djangoapps.config_model_utils.models import Provenance from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory @@ -233,10 +234,14 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): global_config = ContentTypeGatingConfig(enabled=True, enabled_as_of=datetime(2018, 1, 1)) global_config.save() + RequestCache.clear_all_namespaces() + # Check that the global value is not retrieved from cache after save with self.assertNumQueries(1): self.assertTrue(ContentTypeGatingConfig.current().enabled) + RequestCache.clear_all_namespaces() + # Check that the global value can be retrieved from cache after read with self.assertNumQueries(0): self.assertTrue(ContentTypeGatingConfig.current().enabled) @@ -244,6 +249,8 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): global_config.enabled = False global_config.save() + RequestCache.clear_all_namespaces() + # Check that the global value in cache was deleted on save with self.assertNumQueries(1): self.assertFalse(ContentTypeGatingConfig.current().enabled) @@ -253,10 +260,14 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): site_config = ContentTypeGatingConfig(site=site_cfg.site, enabled=True, enabled_as_of=datetime(2018, 1, 1)) site_config.save() + RequestCache.clear_all_namespaces() + # Check that the site value is not retrieved from cache after save with self.assertNumQueries(1): self.assertTrue(ContentTypeGatingConfig.current(site=site_cfg.site).enabled) + RequestCache.clear_all_namespaces() + # Check that the site value can be retrieved from cache after read with self.assertNumQueries(0): self.assertTrue(ContentTypeGatingConfig.current(site=site_cfg.site).enabled) @@ -264,6 +275,8 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): site_config.enabled = False site_config.save() + RequestCache.clear_all_namespaces() + # Check that the site value in cache was deleted on save with self.assertNumQueries(1): self.assertFalse(ContentTypeGatingConfig.current(site=site_cfg.site).enabled) @@ -271,6 +284,8 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): global_config = ContentTypeGatingConfig(enabled=True, enabled_as_of=datetime(2018, 1, 1)) global_config.save() + RequestCache.clear_all_namespaces() + # Check that the site value is not updated in cache by changing the global value with self.assertNumQueries(0): self.assertFalse(ContentTypeGatingConfig.current(site=site_cfg.site).enabled) @@ -281,10 +296,14 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): org_config = ContentTypeGatingConfig(org=course.org, enabled=True, enabled_as_of=datetime(2018, 1, 1)) org_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not retrieved from cache after save with self.assertNumQueries(2): self.assertTrue(ContentTypeGatingConfig.current(org=course.org).enabled) + RequestCache.clear_all_namespaces() + # Check that the org value can be retrieved from cache after read with self.assertNumQueries(0): self.assertTrue(ContentTypeGatingConfig.current(org=course.org).enabled) @@ -292,6 +311,8 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): org_config.enabled = False org_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value in cache was deleted on save with self.assertNumQueries(2): self.assertFalse(ContentTypeGatingConfig.current(org=course.org).enabled) @@ -299,6 +320,8 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): global_config = ContentTypeGatingConfig(enabled=True, enabled_as_of=datetime(2018, 1, 1)) global_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not updated in cache by changing the global value with self.assertNumQueries(0): self.assertFalse(ContentTypeGatingConfig.current(org=course.org).enabled) @@ -306,6 +329,8 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): site_config = ContentTypeGatingConfig(site=site_cfg.site, enabled=True, enabled_as_of=datetime(2018, 1, 1)) site_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not updated in cache by changing the site value with self.assertNumQueries(0): self.assertFalse(ContentTypeGatingConfig.current(org=course.org).enabled) @@ -316,10 +341,14 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): course_config = ContentTypeGatingConfig(course=course, enabled=True, enabled_as_of=datetime(2018, 1, 1)) course_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not retrieved from cache after save with self.assertNumQueries(2): self.assertTrue(ContentTypeGatingConfig.current(course_key=course.id).enabled) + RequestCache.clear_all_namespaces() + # Check that the org value can be retrieved from cache after read with self.assertNumQueries(0): self.assertTrue(ContentTypeGatingConfig.current(course_key=course.id).enabled) @@ -327,6 +356,8 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): course_config.enabled = False course_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value in cache was deleted on save with self.assertNumQueries(2): self.assertFalse(ContentTypeGatingConfig.current(course_key=course.id).enabled) @@ -334,6 +365,8 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): global_config = ContentTypeGatingConfig(enabled=True, enabled_as_of=datetime(2018, 1, 1)) global_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not updated in cache by changing the global value with self.assertNumQueries(0): self.assertFalse(ContentTypeGatingConfig.current(course_key=course.id).enabled) @@ -341,6 +374,8 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): site_config = ContentTypeGatingConfig(site=site_cfg.site, enabled=True, enabled_as_of=datetime(2018, 1, 1)) site_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not updated in cache by changing the site value with self.assertNumQueries(0): self.assertFalse(ContentTypeGatingConfig.current(course_key=course.id).enabled) @@ -348,6 +383,8 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): org_config = ContentTypeGatingConfig(org=course.org, enabled=True, enabled_as_of=datetime(2018, 1, 1)) org_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not updated in cache by changing the site value with self.assertNumQueries(0): self.assertFalse(ContentTypeGatingConfig.current(course_key=course.id).enabled) diff --git a/openedx/features/course_duration_limits/tests/test_models.py b/openedx/features/course_duration_limits/tests/test_models.py index 6a6db087cd..584a4b210b 100644 --- a/openedx/features/course_duration_limits/tests/test_models.py +++ b/openedx/features/course_duration_limits/tests/test_models.py @@ -10,6 +10,7 @@ from django.utils import timezone from mock import Mock import pytz +from edx_django_utils.cache import RequestCache from opaque_keys.edx.locator import CourseLocator from openedx.core.djangoapps.config_model_utils.models import Provenance from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory @@ -266,10 +267,14 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): global_config = CourseDurationLimitConfig(enabled=True, enabled_as_of=datetime(2018, 1, 1)) global_config.save() + RequestCache.clear_all_namespaces() + # Check that the global value is not retrieved from cache after save with self.assertNumQueries(1): self.assertTrue(CourseDurationLimitConfig.current().enabled) + RequestCache.clear_all_namespaces() + # Check that the global value can be retrieved from cache after read with self.assertNumQueries(0): self.assertTrue(CourseDurationLimitConfig.current().enabled) @@ -277,6 +282,8 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): global_config.enabled = False global_config.save() + RequestCache.clear_all_namespaces() + # Check that the global value in cache was deleted on save with self.assertNumQueries(1): self.assertFalse(CourseDurationLimitConfig.current().enabled) @@ -286,10 +293,14 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): site_config = CourseDurationLimitConfig(site=site_cfg.site, enabled=True, enabled_as_of=datetime(2018, 1, 1)) site_config.save() + RequestCache.clear_all_namespaces() + # Check that the site value is not retrieved from cache after save with self.assertNumQueries(1): self.assertTrue(CourseDurationLimitConfig.current(site=site_cfg.site).enabled) + RequestCache.clear_all_namespaces() + # Check that the site value can be retrieved from cache after read with self.assertNumQueries(0): self.assertTrue(CourseDurationLimitConfig.current(site=site_cfg.site).enabled) @@ -297,6 +308,8 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): site_config.enabled = False site_config.save() + RequestCache.clear_all_namespaces() + # Check that the site value in cache was deleted on save with self.assertNumQueries(1): self.assertFalse(CourseDurationLimitConfig.current(site=site_cfg.site).enabled) @@ -304,6 +317,8 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): global_config = CourseDurationLimitConfig(enabled=True, enabled_as_of=datetime(2018, 1, 1)) global_config.save() + RequestCache.clear_all_namespaces() + # Check that the site value is not updated in cache by changing the global value with self.assertNumQueries(0): self.assertFalse(CourseDurationLimitConfig.current(site=site_cfg.site).enabled) @@ -314,10 +329,14 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): org_config = CourseDurationLimitConfig(org=course.org, enabled=True, enabled_as_of=datetime(2018, 1, 1)) org_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not retrieved from cache after save with self.assertNumQueries(2): self.assertTrue(CourseDurationLimitConfig.current(org=course.org).enabled) + RequestCache.clear_all_namespaces() + # Check that the org value can be retrieved from cache after read with self.assertNumQueries(0): self.assertTrue(CourseDurationLimitConfig.current(org=course.org).enabled) @@ -325,6 +344,8 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): org_config.enabled = False org_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value in cache was deleted on save with self.assertNumQueries(2): self.assertFalse(CourseDurationLimitConfig.current(org=course.org).enabled) @@ -332,6 +353,8 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): global_config = CourseDurationLimitConfig(enabled=True, enabled_as_of=datetime(2018, 1, 1)) global_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not updated in cache by changing the global value with self.assertNumQueries(0): self.assertFalse(CourseDurationLimitConfig.current(org=course.org).enabled) @@ -339,6 +362,8 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): site_config = CourseDurationLimitConfig(site=site_cfg.site, enabled=True, enabled_as_of=datetime(2018, 1, 1)) site_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not updated in cache by changing the site value with self.assertNumQueries(0): self.assertFalse(CourseDurationLimitConfig.current(org=course.org).enabled) @@ -349,10 +374,14 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): course_config = CourseDurationLimitConfig(course=course, enabled=True, enabled_as_of=datetime(2018, 1, 1)) course_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not retrieved from cache after save with self.assertNumQueries(2): self.assertTrue(CourseDurationLimitConfig.current(course_key=course.id).enabled) + RequestCache.clear_all_namespaces() + # Check that the org value can be retrieved from cache after read with self.assertNumQueries(0): self.assertTrue(CourseDurationLimitConfig.current(course_key=course.id).enabled) @@ -360,6 +389,8 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): course_config.enabled = False course_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value in cache was deleted on save with self.assertNumQueries(2): self.assertFalse(CourseDurationLimitConfig.current(course_key=course.id).enabled) @@ -367,6 +398,8 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): global_config = CourseDurationLimitConfig(enabled=True, enabled_as_of=datetime(2018, 1, 1)) global_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not updated in cache by changing the global value with self.assertNumQueries(0): self.assertFalse(CourseDurationLimitConfig.current(course_key=course.id).enabled) @@ -374,6 +407,8 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): site_config = CourseDurationLimitConfig(site=site_cfg.site, enabled=True, enabled_as_of=datetime(2018, 1, 1)) site_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not updated in cache by changing the site value with self.assertNumQueries(0): self.assertFalse(CourseDurationLimitConfig.current(course_key=course.id).enabled) @@ -381,6 +416,8 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): org_config = CourseDurationLimitConfig(org=course.org, enabled=True, enabled_as_of=datetime(2018, 1, 1)) org_config.save() + RequestCache.clear_all_namespaces() + # Check that the org value is not updated in cache by changing the site value with self.assertNumQueries(0): self.assertFalse(CourseDurationLimitConfig.current(course_key=course.id).enabled) From 0d273e950666be3a0e69f0b34b9c63789c74ed6e Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 22 Jan 2019 22:19:19 -0500 Subject: [PATCH 4/5] Prefetch course modes used in has_access calls in course api --- common/djangoapps/course_modes/models.py | 36 +++++++++++++++---- lms/djangoapps/course_api/tests/test_views.py | 2 +- lms/djangoapps/courseware/courses.py | 13 ++++++- .../features/course_duration_limits/access.py | 4 ++- .../tests/test_course_expiration.py | 34 +++++++++++++----- .../tests/views/test_course_home.py | 2 +- .../course_experience/views/course_home.py | 6 +++- 7 files changed, 77 insertions(+), 20 deletions(-) diff --git a/common/djangoapps/course_modes/models.py b/common/djangoapps/course_modes/models.py index c2645fdbee..9f80c4e733 100644 --- a/common/djangoapps/course_modes/models.py +++ b/common/djangoapps/course_modes/models.py @@ -312,7 +312,7 @@ class CourseMode(models.Model): @classmethod @request_cached(CACHE_NAMESPACE) - def modes_for_course(cls, course_id, include_expired=False, only_selectable=True): + def modes_for_course(cls, course_id=None, include_expired=False, only_selectable=True, course=None): """ Returns a list of the non-expired modes for a given course id @@ -330,11 +330,25 @@ class CourseMode(models.Model): aren't available to users until they complete the course, so they are hidden in track selection.) + course (CourseOverview): The course to select course modes from. + Returns: list of `Mode` tuples """ - found_course_modes = cls.objects.filter(course_id=course_id) + if course_id is None and course is None: + raise ValueError("One of course_id or course must not be None.") + + if course is not None and not isinstance(course, CourseOverview): + # CourseModules don't have the data needed to pull related modes, + # so we'll fall back on course_id-based lookup instead + course_id = course.id + course = None + + if course_id is not None: + found_course_modes = cls.objects.filter(course_id=course_id) + else: + found_course_modes = course.modes # Filter out expired course modes if include_expired is not set if not include_expired: @@ -347,7 +361,10 @@ class CourseMode(models.Model): # we exclude them from the list if we're only looking for selectable modes # (e.g. on the track selection page or in the payment/verification flows). if only_selectable: - found_course_modes = found_course_modes.exclude(mode_slug__in=cls.CREDIT_MODES) + if course is not None and hasattr(course, 'selectable_modes'): + found_course_modes = course.selectable_modes + else: + found_course_modes = found_course_modes.exclude(mode_slug__in=cls.CREDIT_MODES) modes = ([mode.to_tuple() for mode in found_course_modes]) if not modes: @@ -356,7 +373,7 @@ class CourseMode(models.Model): return modes @classmethod - def modes_for_course_dict(cls, course_id, modes=None, **kwargs): + def modes_for_course_dict(cls, course_id=None, modes=None, **kwargs): """Returns the non-expired modes for a particular course. Arguments: @@ -418,7 +435,7 @@ class CourseMode(models.Model): return None @classmethod - def verified_mode_for_course(cls, course_id, modes=None, include_expired=False): + def verified_mode_for_course(cls, course_id=None, modes=None, include_expired=False, course=None): """Find a verified mode for a particular course. Since we have multiple modes that can go through the verify flow, @@ -439,7 +456,12 @@ class CourseMode(models.Model): Mode or None """ - modes_dict = cls.modes_for_course_dict(course_id, modes=modes, include_expired=include_expired) + modes_dict = cls.modes_for_course_dict( + course_id=course_id, + modes=modes, + include_expired=include_expired, + course=course + ) verified_mode = modes_dict.get('verified', None) professional_mode = modes_dict.get('professional', None) # we prefer professional over verify @@ -703,7 +725,7 @@ class CourseMode(models.Model): Takes a mode model and turns it into a model named tuple. Returns: - A 'Model' namedtuple with all the same attributes as the model. + A 'Mode' namedtuple with all the same attributes as the model. """ return Mode( diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index 0acba1b764..21f7a82cd7 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -392,7 +392,7 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear self.setup_user(self.audit_user) # These query counts were found empirically - query_counts = [122, 140, 170, 200, 230, 260, 290, 320, 350, 380, 327] + query_counts = [93, 81, 81, 81, 81, 81, 81, 81, 81, 81, 25] ordered_course_ids = sorted([str(cid) for cid in (course_ids + [c.id for c in self.courses])]) self.clear_caches() diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 9fc73ec0a8..fd14a84774 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -23,7 +23,9 @@ from courseware.date_summary import ( from courseware.masquerade import check_content_start_date_for_masquerade_user from courseware.model_data import FieldDataCache from courseware.module_render import get_module +from course_modes.models import CourseMode from django.conf import settings +from django.db.models import Prefetch from django.urls import reverse from django.http import Http404, QueryDict from enrollment.api import get_course_enrollment_details @@ -454,7 +456,16 @@ def get_courses(user, org=None, filter_=None): """ Return a LazySequence of courses available, optionally filtered by org code (case-insensitive). """ - courses = branding.get_visible_courses(org=org, filter_=filter_) + courses = branding.get_visible_courses( + org=org, + filter_=filter_, + ).prefetch_related( + Prefetch( + 'modes', + queryset=CourseMode.objects.exclude(mode_slug__in=CourseMode.CREDIT_MODES), + to_attr='selectable_modes', + ) + ) permission_name = configuration_helpers.get_value( 'COURSE_CATALOG_VISIBILITY_PERMISSION', diff --git a/openedx/features/course_duration_limits/access.py b/openedx/features/course_duration_limits/access.py index a024475a23..7d4f1c3abe 100644 --- a/openedx/features/course_duration_limits/access.py +++ b/openedx/features/course_duration_limits/access.py @@ -66,7 +66,9 @@ def get_user_course_expiration_date(user, course): access_duration = MIN_DURATION - if not CourseMode.verified_mode_for_course(course.id, include_expired=True): + verified_mode = CourseMode.verified_mode_for_course(course=course, include_expired=True) + + if not verified_mode: return None enrollment = CourseEnrollment.get_enrollment(user, course.id) diff --git a/openedx/features/course_duration_limits/tests/test_course_expiration.py b/openedx/features/course_duration_limits/tests/test_course_expiration.py index a87abe0825..fb957ab697 100644 --- a/openedx/features/course_duration_limits/tests/test_course_expiration.py +++ b/openedx/features/course_duration_limits/tests/test_course_expiration.py @@ -63,7 +63,7 @@ class CourseExpirationTestCase(ModuleStoreTestCase): def test_enrollment_mode(self): """Tests that verified enrollments do not have an expiration""" CourseEnrollment.enroll(self.user, self.course.id, CourseMode.VERIFIED) - result = get_user_course_expiration_date(self.user, self.course) + result = get_user_course_expiration_date(self.user, CourseOverview.get_from_id(self.course.id)) self.assertEqual(result, None) @mock.patch("openedx.features.course_duration_limits.access.get_course_run_details") @@ -94,7 +94,10 @@ class CourseExpirationTestCase(ModuleStoreTestCase): self.course.self_paced = True mock_get_course_run_details.return_value = {'weeks_to_complete': weeks_to_complete} enrollment = CourseEnrollment.enroll(self.user, self.course.id, CourseMode.AUDIT) - result = get_user_course_expiration_date(self.user, self.course) + result = get_user_course_expiration_date( + self.user, + CourseOverview.get_from_id(self.course.id), + ) self.assertEqual(result, enrollment.created + access_duration) @mock.patch("openedx.features.course_duration_limits.access.get_course_run_details") @@ -109,11 +112,17 @@ class CourseExpirationTestCase(ModuleStoreTestCase): start_date = now() - timedelta(weeks=10) past_course = CourseFactory(start=start_date) enrollment = CourseEnrollment.enroll(self.user, past_course.id, CourseMode.AUDIT) - result = get_user_course_expiration_date(self.user, past_course) + result = get_user_course_expiration_date( + self.user, + CourseOverview.get_from_id(past_course.id), + ) self.assertEqual(result, None) add_course_mode(past_course, upgrade_deadline_expired=False) - result = get_user_course_expiration_date(self.user, past_course) + result = get_user_course_expiration_date( + self.user, + CourseOverview.get_from_id(past_course.id), + ) content_availability_date = enrollment.created self.assertEqual(result, content_availability_date + access_duration) @@ -121,12 +130,18 @@ class CourseExpirationTestCase(ModuleStoreTestCase): start_date = now() + timedelta(weeks=10) future_course = CourseFactory(start=start_date) enrollment = CourseEnrollment.enroll(self.user, future_course.id, CourseMode.AUDIT) - result = get_user_course_expiration_date(self.user, future_course) + result = get_user_course_expiration_date( + self.user, + CourseOverview.get_from_id(future_course.id), + ) self.assertEqual(result, None) add_course_mode(future_course, upgrade_deadline_expired=False) - result = get_user_course_expiration_date(self.user, future_course) - content_availability_date = start_date + result = get_user_course_expiration_date( + self.user, + CourseOverview.get_from_id(future_course.id), + ) + content_availability_date = start_date.replace(microsecond=0) self.assertEqual(result, content_availability_date + access_duration) @mock.patch("openedx.features.course_duration_limits.access.get_course_run_details") @@ -141,7 +156,10 @@ class CourseExpirationTestCase(ModuleStoreTestCase): course = CourseFactory(start=start_date) enrollment = CourseEnrollment.enroll(self.user, course.id, CourseMode.AUDIT) add_course_mode(course, upgrade_deadline_expired=True) - result = get_user_course_expiration_date(self.user, course) + result = get_user_course_expiration_date( + self.user, + CourseOverview.get_from_id(course.id), + ) content_availability_date = enrollment.created self.assertEqual(result, content_availability_date + access_duration) diff --git a/openedx/features/course_experience/tests/views/test_course_home.py b/openedx/features/course_experience/tests/views/test_course_home.py index 048ebcf8c4..9975da81f9 100644 --- a/openedx/features/course_experience/tests/views/test_course_home.py +++ b/openedx/features/course_experience/tests/views/test_course_home.py @@ -204,7 +204,7 @@ class TestCourseHomePage(CourseHomePageTestCase): # Fetch the view and verify the query counts # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(87, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with self.assertNumQueries(89, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): with check_mongo_calls(4): url = course_home_url(self.course) self.client.get(url) diff --git a/openedx/features/course_experience/views/course_home.py b/openedx/features/course_experience/views/course_home.py index 5c1449c3f8..d3fe698ade 100644 --- a/openedx/features/course_experience/views/course_home.py +++ b/openedx/features/course_experience/views/course_home.py @@ -25,6 +25,7 @@ from lms.djangoapps.courseware.exceptions import CourseAccessRedirect from lms.djangoapps.courseware.views.views import CourseTabView from openedx.core.djangoapps.plugin_api.views import EdxFragmentView from openedx.core.djangoapps.util.maintenance_banner import add_maintenance_banner +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.features.course_experience.course_tools import CourseToolsPluginManager from openedx.features.course_duration_limits.access import generate_course_expired_fragment from student.models import CourseEnrollment @@ -153,7 +154,10 @@ class CourseHomeFragmentView(EdxFragmentView): course_sock_fragment = CourseSockFragmentView().render_to_fragment(request, course=course, **kwargs) has_visited_course, resume_course_url = self._get_resume_course_info(request, course_id) handouts_html = self._get_course_handouts(request, course) - course_expiration_fragment = generate_course_expired_fragment(request.user, course) + course_expiration_fragment = generate_course_expired_fragment( + request.user, + CourseOverview.get_from_id(course.id) + ) elif allow_public_outline or allow_public: outline_fragment = CourseOutlineFragmentView().render_to_fragment( request, course_id=course_id, user_is_enrolled=False, **kwargs From 2527efd82203b499382106ccfe9ebf8d0a54a1bb Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 22 Jan 2019 22:23:50 -0500 Subject: [PATCH 5/5] Prefetch CourseOverview.image_set when loading the course_api --- lms/djangoapps/course_api/tests/test_views.py | 2 +- lms/djangoapps/courseware/courses.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index 21f7a82cd7..641a7c915d 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -392,7 +392,7 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear self.setup_user(self.audit_user) # These query counts were found empirically - query_counts = [93, 81, 81, 81, 81, 81, 81, 81, 81, 81, 25] + query_counts = [65, 52, 52, 52, 52, 52, 52, 52, 52, 52, 22] ordered_course_ids = sorted([str(cid) for cid in (course_ids + [c.id for c in self.courses])]) self.clear_caches() diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index fd14a84774..30461cbf1f 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -464,7 +464,8 @@ def get_courses(user, org=None, filter_=None): 'modes', queryset=CourseMode.objects.exclude(mode_slug__in=CourseMode.CREDIT_MODES), to_attr='selectable_modes', - ) + ), + 'image_set', ) permission_name = configuration_helpers.get_value(