From 0d273e950666be3a0e69f0b34b9c63789c74ed6e Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 22 Jan 2019 22:19:19 -0500 Subject: [PATCH] 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