From 8423ecb1c870f368b4fa511b70d4f255677d0f9d Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 20 Feb 2017 11:27:50 -0500 Subject: [PATCH] Student Dashboard CourseOverviews with one query. Pre-load the course overviews attached to CourseEnrollments on the Student Dashboard, if possible. This will only grab the CourseOverviews that already exist, and will not generate new ones. Any missing CourseOverviews fall back to the lazily-created one-at-a-time behavior they've always had. That's mostly because I wanted to optimize for the common case in the least invasive way possible, and I don't want to get caught up in locking issues. --- common/djangoapps/student/models.py | 25 ++++++++++++++++ common/djangoapps/student/views.py | 2 +- .../content/course_overviews/models.py | 21 ++++++++++++++ .../content/course_overviews/tests.py | 29 +++++++++++++++++++ 4 files changed, 76 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 2552fbedad..228067a67c 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -1441,6 +1441,31 @@ class CourseEnrollment(models.Model): def enrollments_for_user(cls, user): return cls.objects.filter(user=user, is_active=1).select_related('user') + @classmethod + def enrollments_for_user_with_overviews_preload(cls, user): # pylint: disable=invalid-name + """ + List of user's CourseEnrollments, CourseOverviews preloaded if possible. + + We try to preload all CourseOverviews, which are usually lazily loaded + as the .course_overview property. This is to avoid making an extra + query for every enrollment when displaying something like the student + dashboard. If some of the CourseOverviews are not found, we make no + attempt to initialize them -- we just fall back to existing lazy-load + behavior. The goal is to optimize the most common case as simply as + possible, without changing any of the existing contracts. + + The name of this method is long, but was the end result of hashing out a + number of alternatives, so pylint can stuff it (disable=invalid-name) + """ + enrollments = list(cls.enrollments_for_user(user)) + overviews = CourseOverview.get_from_ids_if_exists( + enrollment.course_id for enrollment in enrollments + ) + for enrollment in enrollments: + enrollment._course_overview = overviews.get(enrollment.course_id) # pylint: disable=protected-access + + return enrollments + @classmethod def enrollment_status_hash_cache_key(cls, user): """ Returns the cache key for the cached enrollment status hash. diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 5f47eb11c7..dd24d47d9b 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -301,7 +301,7 @@ def get_course_enrollments(user, orgs_to_include, orgs_to_exclude): generator[CourseEnrollment]: a sequence of enrollments to be displayed on the user's dashboard. """ - for enrollment in CourseEnrollment.enrollments_for_user(user): + for enrollment in CourseEnrollment.enrollments_for_user_with_overviews_preload(user): # If the course is missing or broken, log an error and skip it. course_overview = enrollment.course_overview diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index ccd4698db8..346afd26ec 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -274,6 +274,27 @@ class CourseOverview(TimeStampedModel): return course_overview or cls.load_from_module_store(course_id) + @classmethod + def get_from_ids_if_exists(cls, course_ids): + """ + Return a dict mapping course_ids to CourseOverviews, if they exist. + + This method will *not* generate new CourseOverviews or delete outdated + ones. It exists only as a small optimization used when CourseOverviews + are known to exist, for common situations like the student dashboard. + + Callers should assume that this list is incomplete and fall back to + get_from_id if they need to guarantee CourseOverview generation. + """ + return { + overview.id: overview + for overview + in cls.objects.select_related('image_set').filter( + id__in=course_ids, + version__gte=cls.VERSION + ) + } + def clean_id(self, padding_char='='): """ Returns a unique deterministic base32-encoded ID for the course. diff --git a/openedx/core/djangoapps/content/course_overviews/tests.py b/openedx/core/djangoapps/content/course_overviews/tests.py index a8dff4fe9f..6fcfb45336 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests.py +++ b/openedx/core/djangoapps/content/course_overviews/tests.py @@ -515,6 +515,35 @@ class CourseOverviewTestCase(ModuleStoreTestCase): "testing CourseOverview.get_all_courses with filter_={}".format(filter_), ) + def test_get_from_ids_if_exists(self): + course_with_overview_1 = CourseFactory.create(emit_signals=True) + course_with_overview_2 = CourseFactory.create(emit_signals=True) + course_without_overview = CourseFactory.create(emit_signals=False) + + courses = [course_with_overview_1, course_with_overview_2, course_without_overview] + course_ids_to_overviews = CourseOverview.get_from_ids_if_exists( + course.id for course in courses + ) + + # We should see the ones that have published CourseOverviews + # (when the signals were emitted), but not the one that didn't issue + # a publish signal. + self.assertEqual(len(course_ids_to_overviews), 2) + self.assertIn(course_with_overview_1.id, course_ids_to_overviews) + self.assertIn(course_with_overview_2.id, course_ids_to_overviews) + self.assertNotIn(course_without_overview.id, course_ids_to_overviews) + + # But if we set a CourseOverview to be an old version, it shouldn't be + # returned by get_from_ids_if_exists() + overview_2 = course_ids_to_overviews[course_with_overview_2.id] + overview_2.version = CourseOverview.VERSION - 1 + overview_2.save() + course_ids_to_overviews = CourseOverview.get_from_ids_if_exists( + course.id for course in courses + ) + self.assertEqual(len(course_ids_to_overviews), 1) + self.assertIn(course_with_overview_1.id, course_ids_to_overviews) + @attr(shard=3) @ddt.ddt