diff --git a/common/djangoapps/django_comment_common/models.py b/common/djangoapps/django_comment_common/models.py index 7d9d56aef4..a05facd66c 100644 --- a/common/djangoapps/django_comment_common/models.py +++ b/common/djangoapps/django_comment_common/models.py @@ -121,7 +121,14 @@ class Role(models.Model): """ Returns True if the user has one of the given roles for the given course """ - return Role.objects.filter(course_id=course_id, name__in=role_names, users=user).exists() + if 'roles' in getattr(user, '_prefetched_objects_cache', {}): + # Don't blow up the prefetch cache + return any( + role.course_id == course_id and role.name in role_names + for role in user.roles.all() + ) + else: + return user.roles.filter(course_id=course_id, name__in=role_names).exists() class Permission(models.Model): diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 49f036f940..863a0d0016 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -1195,13 +1195,19 @@ class CourseEnrollment(models.Model): if user.is_anonymous: return None try: - query = cls.objects - if select_related is not None: - query = query.select_related(*select_related) - return query.get( - user=user, - course_id=course_key - ) + if 'courseenrollment' in getattr(user, '_prefetched_objects_cache', {}): + for enrollment in user.courseenrollment_set.all(): + if enrollment.course_id == course_key: + return enrollment + return None + else: + query = cls.objects + if select_related is not None: + query = query.select_related(*select_related) + return query.get( + user=user, + course_id=course_key + ) except cls.DoesNotExist: return None diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index 0f84860206..f64c4df6af 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -277,6 +277,7 @@ class CourseDetailViewTestCase(CourseApiTestViewMixin, SharedModuleStoreTestCase }) @override_settings(SEARCH_ENGINE="search.tests.mock_search_engine.MockSearchEngine") @override_settings(COURSEWARE_INDEX_NAME=TEST_INDEX_NAME) +@ddt.ddt class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, SearcherMixin): """ Tests the search functionality of the courses API. @@ -355,7 +356,8 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear 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): + @ddt.data(True, False) + def test_too_many_courses(self, with_username): """ Test that search results are limited to 100 courses, and that they don't blow up the database. @@ -394,8 +396,14 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear 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}) + expected_query_count = query_counts[page - 1] + if with_username: + expected_query_count += 4 + with self.assertNumQueries(expected_query_count): + params = {'page': page, 'page_size': 30} + if with_username: + params['username'] = self.audit_user.username + response = self.verify_response(params=params) self.assertIn('results', response.data) self.assertEqual(response.data['pagination']['count'], 303) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index cc6ee05f2e..e3da3d3a66 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -25,7 +25,7 @@ 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.db.models import Prefetch, prefetch_related_objects from django.urls import reverse from django.http import Http404, QueryDict from enrollment.api import get_course_enrollment_details @@ -474,7 +474,8 @@ def get_courses(user, org=None, filter_=None): 'COURSE_CATALOG_VISIBILITY_PERMISSION', settings.COURSE_CATALOG_VISIBILITY_PERMISSION ) - + if user.is_authenticated: + prefetch_related_objects([user], 'roles', 'courseenrollment_set', 'experimentdata_set') return LazySequence( (c for c in courses if has_access(user, permission_name, c)), est_len=courses.count() diff --git a/openedx/core/djangoapps/config_model_utils/models.py b/openedx/core/djangoapps/config_model_utils/models.py index 2d29ac3605..909099c328 100644 --- a/openedx/core/djangoapps/config_model_utils/models.py +++ b/openedx/core/djangoapps/config_model_utils/models.py @@ -43,6 +43,11 @@ class StackedConfigurationModel(ConfigurationModel): """ class Meta(object): abstract = True + indexes = [ + # This index optimizes the .object.current_set() query + # by preventing a filesort + models.Index(fields=['site', 'org', 'course']) + ] KEY_FIELDS = ('site', 'org', 'course') STACKABLE_FIELDS = ('enabled',) @@ -134,23 +139,18 @@ class StackedConfigurationModel(ConfigurationModel): site_override_q | org_override_q | course_override_q - ).order_by( - # Sort nulls first, and in reverse specificity order - # so that the overrides are in the order of general to specific. - # - # Site | Org | Course - # -------------------- - # Null | Null | Null - # site | Null | Null - # Null | org | Null - # Null | Null | Course - F('course').desc(nulls_first=True), - F('org').desc(nulls_first=True), - F('site').desc(nulls_first=True), ) provenances = defaultdict(lambda: Provenance.default) - for override in overrides: + # We are sorting in python to avoid doing a filesort in the database for + # what will only be 4 rows at maximum + for override in sorted( + overrides, + # This particular sort order sorts None before not-None (because False < True) + # It sorts global first (because all entries are None), then site entries + # (because course_id and org are None), then org and course (by the same logic) + key=lambda o: (o.course_id is not None, o.org is not None, o.site_id is not None) + ): for field in stackable_fields: value = field.value_from_object(override) if value != field_defaults[field.name]: diff --git a/openedx/core/djangoapps/config_model_utils/utils.py b/openedx/core/djangoapps/config_model_utils/utils.py index eae11e1814..b97974ad96 100644 --- a/openedx/core/djangoapps/config_model_utils/utils.py +++ b/openedx/core/djangoapps/config_model_utils/utils.py @@ -12,14 +12,24 @@ def is_in_holdback(user): """ in_holdback = False if user and user.is_authenticated: - try: - holdback_value = ExperimentData.objects.get( - user=user, - experiment_id=EXPERIMENT_ID, - key=EXPERIMENT_DATA_HOLDBACK_KEY, - ).value - in_holdback = holdback_value == 'True' - except ExperimentData.DoesNotExist: - pass + if 'experimentdata' in getattr(user, '_prefetched_objects_cache', {}): + for experiment_data in user.experimentdata_set.all(): + if ( + experiment_data.experiment_id == EXPERIMENT_ID and + experiment_data.key == EXPERIMENT_DATA_HOLDBACK_KEY + ): + in_holdback = experiment_data.value == 'True' + break + else: + try: + holdback_value = ExperimentData.objects.get( + user=user, + experiment_id=EXPERIMENT_ID, + key=EXPERIMENT_DATA_HOLDBACK_KEY, + ).value + except ExperimentData.DoesNotExist: + pass + else: + in_holdback = holdback_value == 'True' return in_holdback diff --git a/openedx/features/content_type_gating/migrations/0006_auto_20190308_1447.py b/openedx/features/content_type_gating/migrations/0006_auto_20190308_1447.py new file mode 100644 index 0000000000..6e3792e462 --- /dev/null +++ b/openedx/features/content_type_gating/migrations/0006_auto_20190308_1447.py @@ -0,0 +1,19 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-03-08 14:47 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('content_type_gating', '0005_auto_20190306_1547'), + ] + + operations = [ + migrations.AddIndex( + model_name='contenttypegatingconfig', + index=models.Index(fields=['site', 'org', 'course'], name='content_typ_site_id_e91576_idx'), + ), + ] diff --git a/openedx/features/course_duration_limits/migrations/0006_auto_20190308_1447.py b/openedx/features/course_duration_limits/migrations/0006_auto_20190308_1447.py new file mode 100644 index 0000000000..f2ae47015f --- /dev/null +++ b/openedx/features/course_duration_limits/migrations/0006_auto_20190308_1447.py @@ -0,0 +1,19 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-03-08 14:47 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('course_duration_limits', '0005_auto_20190306_1546'), + ] + + operations = [ + migrations.AddIndex( + model_name='coursedurationlimitconfig', + index=models.Index(fields=['site', 'org', 'course'], name='course_dura_site_id_424016_idx'), + ), + ] diff --git a/openedx/features/course_duration_limits/tests/test_models.py b/openedx/features/course_duration_limits/tests/test_models.py index 3e14a2fbed..90e8d30be0 100644 --- a/openedx/features/course_duration_limits/tests/test_models.py +++ b/openedx/features/course_duration_limits/tests/test_models.py @@ -138,10 +138,10 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): @ddt.data( # Generate all combinations of setting each configuration level to True/False/None - *itertools.product(*[(True, False, None)] * 4) + *itertools.product(*([(True, False, None)] * 4 + [(True, False)])) ) @ddt.unpack - def test_config_overrides(self, global_setting, site_setting, org_setting, course_setting): + def test_config_overrides(self, global_setting, site_setting, org_setting, course_setting, reverse_order): """ Test that the stacked configuration overrides happen in the correct order and priority. @@ -170,10 +170,16 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): test_course = CourseOverviewFactory.create(org='test-org') test_site_cfg = SiteConfigurationFactory.create(values={'course_org_filter': test_course.org}) - CourseDurationLimitConfig.objects.create(enabled=global_setting) - CourseDurationLimitConfig.objects.create(course=test_course, enabled=course_setting) - CourseDurationLimitConfig.objects.create(org=test_course.org, enabled=org_setting) - CourseDurationLimitConfig.objects.create(site=test_site_cfg.site, enabled=site_setting) + if reverse_order: + CourseDurationLimitConfig.objects.create(site=test_site_cfg.site, enabled=site_setting) + CourseDurationLimitConfig.objects.create(org=test_course.org, enabled=org_setting) + CourseDurationLimitConfig.objects.create(course=test_course, enabled=course_setting) + CourseDurationLimitConfig.objects.create(enabled=global_setting) + else: + CourseDurationLimitConfig.objects.create(enabled=global_setting) + CourseDurationLimitConfig.objects.create(course=test_course, enabled=course_setting) + CourseDurationLimitConfig.objects.create(org=test_course.org, enabled=org_setting) + CourseDurationLimitConfig.objects.create(site=test_site_cfg.site, enabled=site_setting) expected_global_setting = self._resolve_settings([global_setting]) expected_site_setting = self._resolve_settings([global_setting, site_setting])