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.
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user