diff --git a/common/djangoapps/course_modes/models.py b/common/djangoapps/course_modes/models.py index 9f80c4e733..c2645fdbee 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=None, include_expired=False, only_selectable=True, course=None): + def modes_for_course(cls, course_id, include_expired=False, only_selectable=True): """ Returns a list of the non-expired modes for a given course id @@ -330,25 +330,11 @@ 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 """ - 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 + found_course_modes = cls.objects.filter(course_id=course_id) # Filter out expired course modes if include_expired is not set if not include_expired: @@ -361,10 +347,7 @@ 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: - 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) + 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: @@ -373,7 +356,7 @@ class CourseMode(models.Model): return modes @classmethod - def modes_for_course_dict(cls, course_id=None, modes=None, **kwargs): + def modes_for_course_dict(cls, course_id, modes=None, **kwargs): """Returns the non-expired modes for a particular course. Arguments: @@ -435,7 +418,7 @@ class CourseMode(models.Model): return None @classmethod - def verified_mode_for_course(cls, course_id=None, modes=None, include_expired=False, course=None): + def verified_mode_for_course(cls, course_id, modes=None, include_expired=False): """Find a verified mode for a particular course. Since we have multiple modes that can go through the verify flow, @@ -456,12 +439,7 @@ class CourseMode(models.Model): Mode or None """ - modes_dict = cls.modes_for_course_dict( - course_id=course_id, - modes=modes, - include_expired=include_expired, - course=course - ) + modes_dict = cls.modes_for_course_dict(course_id, modes=modes, include_expired=include_expired) verified_mode = modes_dict.get('verified', None) professional_mode = modes_dict.get('professional', None) # we prefer professional over verify @@ -725,7 +703,7 @@ class CourseMode(models.Model): Takes a mode model and turns it into a model named tuple. Returns: - A 'Mode' namedtuple with all the same attributes as the model. + A 'Model' namedtuple with all the same attributes as the model. """ return Mode( diff --git a/lms/djangoapps/branding/__init__.py b/lms/djangoapps/branding/__init__.py index e467d96ef4..534c0c4faf 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): """ - Yield the CourseOverviews that should be visible in this branded + Return the set of CourseOverviews that should be visible in this branded instance. Arguments: @@ -27,10 +27,9 @@ 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: @@ -41,7 +40,7 @@ def get_visible_courses(org=None, filter_=None): else: courses = CourseOverview.get_all_courses(filter_=filter_) - courses = courses.order_by('id') + courses = sorted(courses, key=lambda course: course.number) # Filtering can stop here. if current_site_orgs: @@ -58,11 +57,11 @@ def get_visible_courses(org=None, filter_=None): ) if filtered_visible_ids: - return courses.filter(id__in=filtered_visible_ids) + return [course for course in courses if course.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 courses.exclude(org__in=orgs) + return [course for course in courses if course.location.org not in orgs] def get_university_for_request(): diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index ef25e61399..79a3b38796 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): """ - Yield all available courses. + Return a list of 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: - Yield `CourseOverview` objects representing the collection of courses. + List of `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 7670acbfa1..cef5d4c739 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(list(courses)), 0) + self.assertEqual(len(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(list(courses)), 0) + self.assertEqual(len(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 641a7c915d..974752be37 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -1,7 +1,6 @@ """ Tests for Course API views. """ -from datetime import datetime import ddt from hashlib import md5 @@ -13,14 +12,8 @@ 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 @@ -290,7 +283,6 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear """ ENABLED_SIGNALS = ['course_published'] - ENABLED_CACHES = ModuleStoreTestCase.ENABLED_CACHES + ['configuration'] def setUp(self): super(CourseListSearchViewTest, self).setUp() @@ -306,7 +298,6 @@ 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): """ @@ -356,56 +347,4 @@ 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'], []) - # 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): - """ - 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 = [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() - - 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}) - - 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] - ) + self.assertEqual(res.data['pagination']['count'], 1) # Should list a single course diff --git a/lms/djangoapps/course_api/views.py b/lms/djangoapps/course_api/views.py index 26d7986342..66743ea079 100644 --- a/lms/djangoapps/course_api/views.py +++ b/lms/djangoapps/course_api/views.py @@ -10,7 +10,6 @@ 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 @@ -244,7 +243,7 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): def get_queryset(self): """ - Yield courses visible to the user. + Return a list of courses visible to the user. """ form = CourseListGetForm(self.request.query_params, initial={'requesting_user': self.request.user}) if not form.is_valid(): @@ -265,12 +264,9 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): size=self.results_size_infinity, ) - search_courses_ids = {course['data']['id'] for course in search_courses['results']} + search_courses_ids = {course['data']['id']: True for course in search_courses['results']} - return LazySequence( - ( - course for course in db_courses - if unicode(course.id) in search_courses_ids - ), - est_len=len(db_courses) - ) + return [ + course for course in db_courses + if unicode(course.id) in search_courses_ids + ] diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 30461cbf1f..caccff1313 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -23,9 +23,7 @@ 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 @@ -37,7 +35,6 @@ 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 @@ -454,29 +451,19 @@ def get_course_syllabus_section(course, section_key): def get_courses(user, org=None, filter_=None): """ - Return a LazySequence of courses available, optionally filtered by org code (case-insensitive). + Returns a list of courses available, sorted by course.number and optionally + filtered by org code (case-insensitive). """ - 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', - ), - 'image_set', - ) + courses = branding.get_visible_courses(org=org, filter_=filter_) permission_name = configuration_helpers.get_value( 'COURSE_CATALOG_VISIBILITY_PERMISSION', settings.COURSE_CATALOG_VISIBILITY_PERMISSION ) - return LazySequence( - (c for c in courses if has_access(user, permission_name, c)), - est_len=courses.count() - ) + courses = [c for c in courses if has_access(user, permission_name, c)] + + return courses 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 12d694524c..c54d98e4e2 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(list(no_courses), []) + self.assertEqual(no_courses, []) # Request filtering for an org matching the designated org. site_courses = get_courses(user, org=alternate) diff --git a/openedx/core/djangoapps/config_model_utils/models.py b/openedx/core/djangoapps/config_model_utils/models.py index 2d29ac3605..7e47db7d1b 100644 --- a/openedx/core/djangoapps/config_model_utils/models.py +++ b/openedx/core/djangoapps/config_model_utils/models.py @@ -22,7 +22,6 @@ 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): @@ -253,9 +252,7 @@ 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/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index 6d1998c92f..e852cce1b0 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -409,8 +409,7 @@ class CourseOverview(TimeStampedModel): migrate and test switching to display_name_with_default, which is no longer escaped. """ - # pylint: disable=line-too-long - return block_metadata_utils.display_name_with_default_escaped(self) # xss-lint: disable=python-deprecated-display-name + return block_metadata_utils.display_name_with_default_escaped(self) @property def dashboard_start_display(self): @@ -561,7 +560,7 @@ class CourseOverview(TimeStampedModel): @classmethod def get_all_courses(cls, orgs=None, filter_=None): """ - Return a queryset containing all CourseOverview objects in the database. + Returns all CourseOverview objects in the database. Arguments: orgs (list[string]): Optional parameter that allows case-insensitive diff --git a/openedx/core/djangolib/testing/utils.py b/openedx/core/djangolib/testing/utils.py index 3d620385e8..710f5a35ba 100644 --- a/openedx/core/djangolib/testing/utils.py +++ b/openedx/core/djangolib/testing/utils.py @@ -73,9 +73,6 @@ 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 }) diff --git a/openedx/core/lib/api/view_utils.py b/openedx/core/lib/api/view_utils.py index 61c8dbd319..ef67c4eb82 100644 --- a/openedx/core/lib/api/view_utils.py +++ b/openedx/core/lib/api/view_utils.py @@ -1,7 +1,6 @@ """ 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 _ @@ -206,100 +205,3 @@ 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, - ) diff --git a/openedx/core/lib/cache_utils.py b/openedx/core/lib/cache_utils.py index 3910d33f7c..ba4324143d 100644 --- a/openedx/core/lib/cache_utils.py +++ b/openedx/core/lib/cache_utils.py @@ -9,7 +9,6 @@ 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): @@ -48,32 +47,38 @@ 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. """ - @wrapt.decorator - def decorator(wrapped, instance, args, kwargs): + def decorator(f): """ Arguments: - args, kwargs: values passed into the wrapped function + f (func): the function to wrap """ - # 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) + @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) - 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 + 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 - result = wrapped(*args, **kwargs) + result = f(*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 aca44f17bc..e51a16b529 100644 --- a/openedx/features/content_type_gating/tests/test_models.py +++ b/openedx/features/content_type_gating/tests/test_models.py @@ -6,7 +6,6 @@ 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 @@ -234,14 +233,10 @@ 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) @@ -249,8 +244,6 @@ 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) @@ -260,14 +253,10 @@ 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) @@ -275,8 +264,6 @@ 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) @@ -284,8 +271,6 @@ 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) @@ -296,14 +281,10 @@ 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) @@ -311,8 +292,6 @@ 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) @@ -320,8 +299,6 @@ 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) @@ -329,8 +306,6 @@ 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) @@ -341,14 +316,10 @@ 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) @@ -356,8 +327,6 @@ 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) @@ -365,8 +334,6 @@ 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) @@ -374,8 +341,6 @@ 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) @@ -383,8 +348,6 @@ 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/access.py b/openedx/features/course_duration_limits/access.py index 7d4f1c3abe..a024475a23 100644 --- a/openedx/features/course_duration_limits/access.py +++ b/openedx/features/course_duration_limits/access.py @@ -66,9 +66,7 @@ def get_user_course_expiration_date(user, course): access_duration = MIN_DURATION - verified_mode = CourseMode.verified_mode_for_course(course=course, include_expired=True) - - if not verified_mode: + if not CourseMode.verified_mode_for_course(course.id, include_expired=True): 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 fb957ab697..a87abe0825 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, CourseOverview.get_from_id(self.course.id)) + result = get_user_course_expiration_date(self.user, self.course) self.assertEqual(result, None) @mock.patch("openedx.features.course_duration_limits.access.get_course_run_details") @@ -94,10 +94,7 @@ 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, - CourseOverview.get_from_id(self.course.id), - ) + result = get_user_course_expiration_date(self.user, self.course) self.assertEqual(result, enrollment.created + access_duration) @mock.patch("openedx.features.course_duration_limits.access.get_course_run_details") @@ -112,17 +109,11 @@ 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, - CourseOverview.get_from_id(past_course.id), - ) + result = get_user_course_expiration_date(self.user, past_course) self.assertEqual(result, None) add_course_mode(past_course, upgrade_deadline_expired=False) - result = get_user_course_expiration_date( - self.user, - CourseOverview.get_from_id(past_course.id), - ) + result = get_user_course_expiration_date(self.user, past_course) content_availability_date = enrollment.created self.assertEqual(result, content_availability_date + access_duration) @@ -130,18 +121,12 @@ 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, - CourseOverview.get_from_id(future_course.id), - ) + result = get_user_course_expiration_date(self.user, future_course) self.assertEqual(result, None) add_course_mode(future_course, upgrade_deadline_expired=False) - result = get_user_course_expiration_date( - self.user, - CourseOverview.get_from_id(future_course.id), - ) - content_availability_date = start_date.replace(microsecond=0) + result = get_user_course_expiration_date(self.user, future_course) + content_availability_date = start_date self.assertEqual(result, content_availability_date + access_duration) @mock.patch("openedx.features.course_duration_limits.access.get_course_run_details") @@ -156,10 +141,7 @@ 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, - CourseOverview.get_from_id(course.id), - ) + result = get_user_course_expiration_date(self.user, course) content_availability_date = enrollment.created self.assertEqual(result, content_availability_date + access_duration) diff --git a/openedx/features/course_duration_limits/tests/test_models.py b/openedx/features/course_duration_limits/tests/test_models.py index 584a4b210b..6a6db087cd 100644 --- a/openedx/features/course_duration_limits/tests/test_models.py +++ b/openedx/features/course_duration_limits/tests/test_models.py @@ -10,7 +10,6 @@ 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 @@ -267,14 +266,10 @@ 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) @@ -282,8 +277,6 @@ 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) @@ -293,14 +286,10 @@ 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) @@ -308,8 +297,6 @@ 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) @@ -317,8 +304,6 @@ 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) @@ -329,14 +314,10 @@ 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) @@ -344,8 +325,6 @@ 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) @@ -353,8 +332,6 @@ 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) @@ -362,8 +339,6 @@ 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) @@ -374,14 +349,10 @@ 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) @@ -389,8 +360,6 @@ 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) @@ -398,8 +367,6 @@ 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) @@ -407,8 +374,6 @@ 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) @@ -416,8 +381,6 @@ 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) 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 9975da81f9..048ebcf8c4 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(89, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with self.assertNumQueries(87, 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 d3fe698ade..5c1449c3f8 100644 --- a/openedx/features/course_experience/views/course_home.py +++ b/openedx/features/course_experience/views/course_home.py @@ -25,7 +25,6 @@ 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 @@ -154,10 +153,7 @@ 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, - CourseOverview.get_from_id(course.id) - ) + course_expiration_fragment = generate_course_expired_fragment(request.user, course) elif allow_public_outline or allow_public: outline_fragment = CourseOutlineFragmentView().render_to_fragment( request, course_id=course_id, user_is_enrolled=False, **kwargs