From eee6250fb24471738292a145f0d8b331bd5990b5 Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Sun, 1 Mar 2015 21:40:54 +0500 Subject: [PATCH] Use RequestCache to cache cohorts information. TNL-1258 --- .../core/djangoapps/course_groups/cohorts.py | 66 +++++++++++-------- .../course_groups/tests/test_cohorts.py | 2 +- 2 files changed, 39 insertions(+), 29 deletions(-) diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py index 2221afe574..863dcd0096 100644 --- a/openedx/core/djangoapps/course_groups/cohorts.py +++ b/openedx/core/djangoapps/course_groups/cohorts.py @@ -14,7 +14,9 @@ from django.utils.translation import ugettext as _ from courseware import courses from eventtracking import tracker +from request_cache.middleware import RequestCache from student.models import get_user_by_username_or_email + from .models import CourseUserGroup, CourseCohort, CourseCohortsSettings, CourseUserGroupPartitionGroup @@ -177,6 +179,10 @@ def get_cohort(user, course_key, assign=True, use_cached=False): Given a Django user and a CourseKey, return the user's cohort in that cohort. + The cohort for the user is cached for the duration of a request. Pass + use_cached=True to use the cached value instead of fetching from the + database. + Arguments: user: a Django User object. course_key: CourseKey @@ -190,35 +196,37 @@ def get_cohort(user, course_key, assign=True, use_cached=False): Raises: ValueError if the CourseKey doesn't exist. """ - # pylint: disable=protected-access - # We cache the cohort on the user object so that we do not have to repeatedly - # query the database during a request. If the cached value exists, just return it. - if use_cached and hasattr(user, '_cohort'): - return user._cohort + request_cache = RequestCache.get_request_cache() + cache_key = u"cohorts.get_cohort.{}.{}".format(user.id, course_key) + + if use_cached and cache_key in request_cache.data: + return request_cache.data[cache_key] + + request_cache.data.pop(cache_key, None) # First check whether the course is cohorted (users shouldn't be in a cohort # in non-cohorted courses, but settings can change after course starts) - course = courses.get_course(course_key) - course_cohort_settings = get_course_cohort_settings(course.id) - + course_cohort_settings = get_course_cohort_settings(course_key) if not course_cohort_settings.is_cohorted: - user._cohort = None - return user._cohort + return request_cache.data.setdefault(cache_key, None) + # If course is cohorted, check if the user already has a cohort. try: - user._cohort = CourseUserGroup.objects.get( + cohort = CourseUserGroup.objects.get( course_id=course_key, group_type=CourseUserGroup.COHORT, users__id=user.id, ) - return user._cohort + return request_cache.data.setdefault(cache_key, cohort) except CourseUserGroup.DoesNotExist: - # Didn't find the group. We'll go on to create one if needed. + # Didn't find the group. If we do not want to assign, return here. if not assign: # Do not cache the cohort here, because in the next call assign # may be True, and we will have to assign the user a cohort. return None + # Otherwise assign the user a cohort. + course = courses.get_course(course_key) cohorts = get_course_cohorts(course, assignment_type=CourseCohort.RANDOM) if cohorts: cohort = local_random().choice(cohorts) @@ -231,8 +239,7 @@ def get_cohort(user, course_key, assign=True, use_cached=False): user.course_groups.add(cohort) - user._cohort = cohort - return user._cohort + return request_cache.data.setdefault(cache_key, cohort) def migrate_cohort_settings(course): @@ -406,23 +413,26 @@ def get_group_info_for_cohort(cohort, use_cached=False): If the cohort has not been linked to any group/partition, both values in the tuple will be None. + + The partition group info is cached for the duration of a request. Pass + use_cached=True to use the cached value instead of fetching from the + database. """ - # pylint: disable=protected-access - # We cache the partition group on the cohort object so that we do not have to repeatedly - # query the database during a request. - if not use_cached and hasattr(cohort, '_partition_group'): - delattr(cohort, '_partition_group') + request_cache = RequestCache.get_request_cache() + cache_key = u"cohorts.get_group_info_for_cohort.{}".format(cohort.id) - if not hasattr(cohort, '_partition_group'): - try: - cohort._partition_group = CourseUserGroupPartitionGroup.objects.get(course_user_group=cohort) - except CourseUserGroupPartitionGroup.DoesNotExist: - cohort._partition_group = None + if use_cached and cache_key in request_cache.data: + return request_cache.data[cache_key] - if cohort._partition_group: - return cohort._partition_group.group_id, cohort._partition_group.partition_id + request_cache.data.pop(cache_key, None) - return None, None + try: + partition_group = CourseUserGroupPartitionGroup.objects.get(course_user_group=cohort) + return request_cache.data.setdefault(cache_key, (partition_group.group_id, partition_group.partition_id)) + except CourseUserGroupPartitionGroup.DoesNotExist: + pass + + return request_cache.data.setdefault(cache_key, (None, None)) def set_assignment_type(user_group, assignment_type): diff --git a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py index d5c80ab64e..a2c98ad802 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py +++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py @@ -176,7 +176,7 @@ class TestCohorts(ModuleStoreTestCase): self.assertEqual(cohorts.get_cohort_id(user, course.id), cohort.id) self.assertRaises( - ValueError, + Http404, lambda: cohorts.get_cohort_id(user, SlashSeparatedCourseKey("course", "does_not", "exist")) )