From adb88e21f3d929cb9745bae328c86efc1f3b91dc Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Wed, 10 May 2017 16:05:32 -0400 Subject: [PATCH 1/2] Bulk-reads and Request caching in Course Grade Report This reverts commit 5388d5d1fcd60723bf14fc4d466eda85c0f253f8. --- common/djangoapps/course_modes/models.py | 13 ++ .../course_modes/tests/test_models.py | 5 +- common/djangoapps/request_cache/__init__.py | 10 ++ common/djangoapps/request_cache/middleware.py | 55 +++++-- common/djangoapps/student/models.py | 32 +++- common/djangoapps/student/roles.py | 32 +++- lms/djangoapps/certificates/models.py | 37 +++-- lms/djangoapps/certificates/tests/tests.py | 9 +- lms/djangoapps/grades/models.py | 30 +++- .../grades/new/course_grade_factory.py | 9 +- lms/djangoapps/grades/tests/test_new.py | 6 +- lms/djangoapps/grades/tests/test_tasks.py | 4 +- .../instructor_task/tasks_helper/grades.py | 145 ++++++++++++------ .../tests/test_tasks_helper.py | 45 +++++- lms/djangoapps/verify_student/models.py | 20 ++- .../core/djangoapps/course_groups/cohorts.py | 72 +++++++-- openedx/core/djangoapps/credit/models.py | 11 ++ .../core/djangoapps/credit/tests/test_api.py | 4 +- .../djangoapps/user_api/course_tag/api.py | 43 ++++++ .../djangoapps/user_api/partition_schemes.py | 2 +- .../user_api/tests/test_partition_schemes.py | 5 + .../verified_track_content/models.py | 18 ++- 22 files changed, 477 insertions(+), 130 deletions(-) diff --git a/common/djangoapps/course_modes/models.py b/common/djangoapps/course_modes/models.py index 3f17afc622..e33ee37222 100644 --- a/common/djangoapps/course_modes/models.py +++ b/common/djangoapps/course_modes/models.py @@ -9,8 +9,11 @@ from config_models.models import ConfigurationModel from django.core.exceptions import ValidationError from django.db import models from django.db.models import Q +from django.dispatch import receiver from django.utils.translation import ugettext_lazy as _ from openedx.core.djangoapps.xmodule_django.models import CourseKeyField +from request_cache.middleware import ns_request_cached, RequestCache + Mode = namedtuple('Mode', [ @@ -141,6 +144,8 @@ class CourseMode(models.Model): DEFAULT_SHOPPINGCART_MODE_SLUG = HONOR DEFAULT_SHOPPINGCART_MODE = Mode(HONOR, _('Honor'), 0, '', 'usd', None, None, None, None) + CACHE_NAMESPACE = u"course_modes.CourseMode.cache." + class Meta(object): unique_together = ('course_id', 'mode_slug', 'currency') @@ -265,6 +270,7 @@ class CourseMode(models.Model): return [mode.to_tuple() for mode in found_course_modes] @classmethod + @ns_request_cached(CACHE_NAMESPACE) def modes_for_course(cls, course_id, include_expired=False, only_selectable=True): """ Returns a list of the non-expired modes for a given course id @@ -666,6 +672,13 @@ class CourseMode(models.Model): ) +@receiver(models.signals.post_save, sender=CourseMode) +@receiver(models.signals.post_delete, sender=CourseMode) +def invalidate_course_mode_cache(sender, **kwargs): # pylint: disable=unused-argument + """Invalidate the cache of course modes. """ + RequestCache.clear_request_cache(name=CourseMode.CACHE_NAMESPACE) + + class CourseModesArchive(models.Model): """ Store the past values of course_mode that a course had in the past. We decided on having diff --git a/common/djangoapps/course_modes/tests/test_models.py b/common/djangoapps/course_modes/tests/test_models.py index f992e54ebc..1dd761cd3e 100644 --- a/common/djangoapps/course_modes/tests/test_models.py +++ b/common/djangoapps/course_modes/tests/test_models.py @@ -16,7 +16,7 @@ from opaque_keys.edx.locator import CourseLocator import pytz from course_modes.helpers import enrollment_mode_display -from course_modes.models import CourseMode, Mode +from course_modes.models import CourseMode, Mode, invalidate_course_mode_cache from course_modes.tests.factories import CourseModeFactory @@ -31,6 +31,9 @@ class CourseModeModelTest(TestCase): self.course_key = SlashSeparatedCourseKey('Test', 'TestCourse', 'TestCourseRun') CourseMode.objects.all().delete() + def tearDown(self): + invalidate_course_mode_cache(sender=None) + def create_mode( self, mode_slug, diff --git a/common/djangoapps/request_cache/__init__.py b/common/djangoapps/request_cache/__init__.py index 8ef6a4d058..419508fd74 100644 --- a/common/djangoapps/request_cache/__init__.py +++ b/common/djangoapps/request_cache/__init__.py @@ -41,6 +41,16 @@ def get_cache(name): return middleware.RequestCache.get_request_cache(name) +def clear_cache(name): + """ + Clears the request cache named ``name``. + + Arguments: + name (str): The name of the request cache to clear + """ + return middleware.RequestCache.clear_request_cache(name) + + def get_request(): """ Return the current request. diff --git a/common/djangoapps/request_cache/middleware.py b/common/djangoapps/request_cache/middleware.py index 246f490b69..f2f3a40fdd 100644 --- a/common/djangoapps/request_cache/middleware.py +++ b/common/djangoapps/request_cache/middleware.py @@ -39,11 +39,14 @@ class RequestCache(object): return crum.get_current_request() @classmethod - def clear_request_cache(cls): + def clear_request_cache(cls, name=None): """ Empty the request cache. """ - REQUEST_CACHE.data = {} + if name is None: + REQUEST_CACHE.data = {} + elif REQUEST_CACHE.data.get(name): + REQUEST_CACHE.data[name] = {} def process_request(self, request): self.clear_request_cache() @@ -82,25 +85,43 @@ def request_cached(f): cache the value it returns, and return that cached value for subsequent calls with the same args/kwargs within a single request """ - def wrapper(*args, **kwargs): + return ns_request_cached()(f) + + +def ns_request_cached(namespace=None): + """ + Same as request_cached above, except an optional namespace can be passed in to compartmentalize the cache. + + Arguments: + namespace (string): An optional namespace to use for the cache. Useful if the caller wants to manage + their own sub-cache by, for example, calling RequestCache.clear_request_cache for their own namespace. + """ + def outer_wrapper(f): """ - Wrapper function to decorate with. + Outer wrapper that decorates the given function + + Arguments: + f (func): the function to wrap """ - # Check to see if we have a result in cache. If not, invoke our wrapped - # function. Cache and return the result to the caller. - rcache = RequestCache.get_request_cache() - cache_key = func_call_cache_key(f, *args, **kwargs) + def inner_wrapper(*args, **kwargs): + """ + Wrapper function to decorate with. + """ + # Check to see if we have a result in cache. If not, invoke our wrapped + # function. Cache and return the result to the caller. + rcache = RequestCache.get_request_cache(namespace) + rcache = rcache.data if namespace is None else rcache + cache_key = func_call_cache_key(f, *args, **kwargs) - if cache_key in rcache.data: - return rcache.data.get(cache_key) - else: - result = f(*args, **kwargs) - rcache.data[cache_key] = result + if cache_key in rcache: + return rcache.get(cache_key) + else: + result = f(*args, **kwargs) + rcache[cache_key] = result + return result - return result - - wrapper.request_cached_contained_func = f - return wrapper + return inner_wrapper + return outer_wrapper def func_call_cache_key(func, *args, **kwargs): diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index e02e0d6c55..d4a38880fe 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -998,7 +998,9 @@ class CourseEnrollment(models.Model): history = HistoricalRecords() # cache key format e.g enrollment...mode = 'honor' - COURSE_ENROLLMENT_CACHE_KEY = u"enrollment.{}.{}.mode" + COURSE_ENROLLMENT_CACHE_KEY = u"enrollment.{}.{}.mode" # TODO Can this be removed? It doesn't seem to be used. + + MODE_CACHE_NAMESPACE = u'CourseEnrollment.mode_and_active' class Meta(object): unique_together = (('user', 'course_id'),) @@ -1697,12 +1699,28 @@ class CourseEnrollment(models.Model): cls._update_enrollment_in_request_cache(user, course_key, enrollment_state) return enrollment_state + @classmethod + def bulk_fetch_enrollment_states(cls, users, course_key): + """ + Bulk pre-fetches the enrollment states for the given users + for the given course. + """ + # before populating the cache with another bulk set of data, + # remove previously cached entries to keep memory usage low. + request_cache.clear_cache(cls.MODE_CACHE_NAMESPACE) + + records = cls.objects.filter(user__in=users, course_id=course_key).select_related('user__id') + cache = cls._get_mode_active_request_cache() + for record in records: + enrollment_state = CourseEnrollmentState(record.mode, record.is_active) + cls._update_enrollment(cache, record.user.id, course_key, enrollment_state) + @classmethod def _get_mode_active_request_cache(cls): """ Returns the request-specific cache for CourseEnrollment """ - return request_cache.get_cache('CourseEnrollment.mode_and_active') + return request_cache.get_cache(cls.MODE_CACHE_NAMESPACE) @classmethod def _get_enrollment_in_request_cache(cls, user, course_key): @@ -1718,7 +1736,15 @@ class CourseEnrollment(models.Model): Updates the cached value for the user's enrollment in the request cache. """ - cls._get_mode_active_request_cache()[(user.id, course_key)] = enrollment_state + cls._update_enrollment(cls._get_mode_active_request_cache(), user.id, course_key, enrollment_state) + + @classmethod + def _update_enrollment(cls, cache, user_id, course_key, enrollment_state): + """ + Updates the cached value for the user's enrollment in the + given cache. + """ + cache[(user_id, course_key)] = enrollment_state @receiver(models.signals.post_save, sender=CourseEnrollment) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index e1bf8189ef..0fd10873c0 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -4,10 +4,12 @@ adding users, removing users, and listing members """ from abc import ABCMeta, abstractmethod +from collections import defaultdict from django.contrib.auth.models import User import logging +from request_cache import get_cache from student.models import CourseAccessRole from openedx.core.djangoapps.xmodule_django.models import CourseKeyField @@ -34,14 +36,38 @@ def register_access_role(cls): return cls +class BulkRoleCache(object): + CACHE_NAMESPACE = u"student.roles.BulkRoleCache" + CACHE_KEY = u'roles_by_user' + + @classmethod + def prefetch(cls, users): + roles_by_user = defaultdict(set) + get_cache(cls.CACHE_NAMESPACE)[cls.CACHE_KEY] = roles_by_user + + for role in CourseAccessRole.objects.filter(user__in=users).select_related('user__id'): + roles_by_user[role.user.id].add(role) + + users_without_roles = filter(lambda u: u.id not in roles_by_user, users) + for user in users_without_roles: + roles_by_user[user.id] = set() + + @classmethod + def get_user_roles(cls, user): + return get_cache(cls.CACHE_NAMESPACE)[cls.CACHE_KEY][user.id] + + class RoleCache(object): """ A cache of the CourseAccessRoles held by a particular user """ def __init__(self, user): - self._roles = set( - CourseAccessRole.objects.filter(user=user).all() - ) + try: + self._roles = BulkRoleCache.get_user_roles(user) + except KeyError: + self._roles = set( + CourseAccessRole.objects.filter(user=user).all() + ) def has_role(self, role, course_id, org): """ diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index 4c2a410594..736a648959 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -493,6 +493,18 @@ def handle_course_cert_awarded(sender, user, course_key, **kwargs): # pylint: d def certificate_status_for_student(student, course_id): + """ + This returns a dictionary with a key for status, and other information. + See certificate_status for more information. + """ + try: + generated_certificate = GeneratedCertificate.objects.get(user=student, course_id=course_id) + except GeneratedCertificate.DoesNotExist: + generated_certificate = None + return certificate_status(generated_certificate) + + +def certificate_status(generated_certificate): ''' This returns a dictionary with a key for status, and other information. The status is one of the following: @@ -527,9 +539,7 @@ def certificate_status_for_student(student, course_id): # the course_modes app is loaded, resulting in a Django deprecation warning. from course_modes.models import CourseMode - try: - generated_certificate = GeneratedCertificate.objects.get( # pylint: disable=no-member - user=student, course_id=course_id) + if generated_certificate: cert_status = { 'status': generated_certificate.status, 'mode': generated_certificate.mode, @@ -539,7 +549,7 @@ def certificate_status_for_student(student, course_id): cert_status['grade'] = generated_certificate.grade if generated_certificate.mode == 'audit': - course_mode_slugs = [mode.slug for mode in CourseMode.modes_for_course(course_id)] + course_mode_slugs = [mode.slug for mode in CourseMode.modes_for_course(generated_certificate.course_id)] # Short term fix to make sure old audit users with certs still see their certs # only do this if there if no honor mode if 'honor' not in course_mode_slugs: @@ -550,31 +560,24 @@ def certificate_status_for_student(student, course_id): cert_status['download_url'] = generated_certificate.download_url return cert_status - - except GeneratedCertificate.DoesNotExist: - pass - return {'status': CertificateStatuses.unavailable, 'mode': GeneratedCertificate.MODES.honor, 'uuid': None} + else: + return {'status': CertificateStatuses.unavailable, 'mode': GeneratedCertificate.MODES.honor, 'uuid': None} -def certificate_info_for_user(user, course_id, grade, user_is_whitelisted=None): +def certificate_info_for_user(user, grade, user_is_whitelisted, user_certificate): """ Returns the certificate info for a user for grade report. """ - if user_is_whitelisted is None: - user_is_whitelisted = CertificateWhitelist.objects.filter( - user=user, course_id=course_id, whitelist=True - ).exists() - certificate_is_delivered = 'N' certificate_type = 'N/A' eligible_for_certificate = 'Y' if (user_is_whitelisted or grade is not None) and user.profile.allow_certificate \ else 'N' - certificate_status = certificate_status_for_student(user, course_id) - certificate_generated = certificate_status['status'] == CertificateStatuses.downloadable + status = certificate_status(user_certificate) + certificate_generated = status['status'] == CertificateStatuses.downloadable if certificate_generated: certificate_is_delivered = 'Y' - certificate_type = certificate_status['mode'] + certificate_type = status['mode'] return [eligible_for_certificate, certificate_is_delivered, certificate_type] diff --git a/lms/djangoapps/certificates/tests/tests.py b/lms/djangoapps/certificates/tests/tests.py index 3ca9c5ef9e..fb82ce92c5 100644 --- a/lms/djangoapps/certificates/tests/tests.py +++ b/lms/djangoapps/certificates/tests/tests.py @@ -54,11 +54,11 @@ class CertificatesModelTest(ModuleStoreTestCase, MilestonesTestCaseMixin): Verify that certificate_info_for_user works. """ student = UserFactory() - course = CourseFactory.create(org='edx', number='verified', display_name='Verified Course') + _ = CourseFactory.create(org='edx', number='verified', display_name='Verified Course') student.profile.allow_certificate = allow_certificate student.profile.save() - certificate_info = certificate_info_for_user(student, course.id, grade, whitelisted) + certificate_info = certificate_info_for_user(student, grade, whitelisted, user_certificate=None) self.assertEqual(certificate_info, output) @unpack @@ -81,14 +81,13 @@ class CertificatesModelTest(ModuleStoreTestCase, MilestonesTestCaseMixin): student.profile.allow_certificate = allow_certificate student.profile.save() - GeneratedCertificateFactory.create( + certificate = GeneratedCertificateFactory.create( user=student, course_id=course.id, status=CertificateStatuses.downloadable, mode='honor' ) - - certificate_info = certificate_info_for_user(student, course.id, grade, whitelisted) + certificate_info = certificate_info_for_user(student, grade, whitelisted, certificate) self.assertEqual(certificate_info, output) def test_course_ids_with_certs_for_user(self): diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index 16c9c4fbf0..716f330db5 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -25,6 +25,7 @@ from track.event_transaction_utils import get_event_transaction_id, get_event_tr from coursewarehistoryextended.fields import UnsignedBigIntAutoField from opaque_keys.edx.keys import CourseKey, UsageKey from openedx.core.djangoapps.xmodule_django.models import CourseKeyField, UsageKeyField +from request_cache import get_cache from .config import waffle @@ -522,6 +523,8 @@ class PersistentCourseGrade(DeleteGradesMixin, TimeStampedModel): # Information related to course completion passed_timestamp = models.DateTimeField(u'Date learner earned a passing grade', blank=True, null=True) + CACHE_NAMESPACE = u"grades.models.PersistentCourseGrade" + def __unicode__(self): """ Returns a string representation of this model. @@ -535,6 +538,21 @@ class PersistentCourseGrade(DeleteGradesMixin, TimeStampedModel): u"passed timestamp: {}".format(self.passed_timestamp), ]) + @classmethod + def _cache_key(cls, course_id): + return u"grades_cache.{}".format(course_id) + + @classmethod + def prefetch(cls, course_id, users): + """ + Prefetches grades for the given users for the given course. + """ + get_cache(cls.CACHE_NAMESPACE)[cls._cache_key(course_id)] = { + grade.user_id: grade + for grade in + cls.objects.filter(user_id__in=[user.id for user in users], course_id=course_id) + } + @classmethod def read(cls, user_id, course_id): """ @@ -546,7 +564,17 @@ class PersistentCourseGrade(DeleteGradesMixin, TimeStampedModel): Raises PersistentCourseGrade.DoesNotExist if applicable """ - return cls.objects.get(user_id=user_id, course_id=course_id) + try: + prefetched_grades = get_cache(cls.CACHE_NAMESPACE)[cls._cache_key(course_id)] + try: + return prefetched_grades[user_id] + except KeyError: + # user's grade is not in the prefetched list, so + # assume they have no grade + raise cls.DoesNotExist + except KeyError: + # grades were not prefetched for the course, so fetch it + return cls.objects.get(user_id=user_id, course_id=course_id) @classmethod def update_or_create(cls, user_id, course_id, **kwargs): diff --git a/lms/djangoapps/grades/new/course_grade_factory.py b/lms/djangoapps/grades/new/course_grade_factory.py index c731f29ef7..716cf13c77 100644 --- a/lms/djangoapps/grades/new/course_grade_factory.py +++ b/lms/djangoapps/grades/new/course_grade_factory.py @@ -81,7 +81,6 @@ class CourseGradeFactory(object): users, course=None, collected_block_structure=None, - course_structure=None, course_key=None, force_update=False, ): @@ -99,7 +98,9 @@ class CourseGradeFactory(object): # compute the grade for all students. # 2. Optimization: the collected course_structure is not # retrieved from the data store multiple times. - course_data = CourseData(None, course, collected_block_structure, course_structure, course_key) + course_data = CourseData( + user=None, course=course, collected_block_structure=collected_block_structure, course_key=course_key, + ) for user in users: with dog_stats_api.timer( 'lms.grades.CourseGradeFactory.iter', @@ -107,7 +108,9 @@ class CourseGradeFactory(object): ): try: method = CourseGradeFactory().update if force_update else CourseGradeFactory().create - course_grade = method(user, course, course_data.collected_structure, course_structure, course_key) + course_grade = method( + user, course_data.course, course_data.collected_structure, course_key=course_key, + ) yield self.GradeResult(user, course_grade, None) except Exception as exc: # pylint: disable=broad-except diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index 56d0caad6e..99549856d6 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -178,10 +178,10 @@ class TestCourseGradeFactory(GradeTestBase): self.assertEqual(course_grade.letter_grade, u'Pass' if expected_pass else None) self.assertEqual(course_grade.percent, 0.5) - with self.assertNumQueries(12), mock_get_score(1, 2): + with self.assertNumQueries(11), mock_get_score(1, 2): _assert_create(expected_pass=True) - with self.assertNumQueries(15), mock_get_score(1, 2): + with self.assertNumQueries(13), mock_get_score(1, 2): grade_factory.update(self.request.user, self.course) with self.assertNumQueries(1): @@ -189,7 +189,7 @@ class TestCourseGradeFactory(GradeTestBase): self._update_grading_policy(passing=0.9) - with self.assertNumQueries(8): + with self.assertNumQueries(6): _assert_create(expected_pass=False) @ddt.data(True, False) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index f803b90877..0f467db1c0 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -409,8 +409,8 @@ class ComputeGradesForCourseTest(HasCourseWithProblemsMixin, ModuleStoreTestCase @ddt.data(*xrange(1, 12, 3)) def test_database_calls(self, batch_size): - per_user_queries = 17 * min(batch_size, 6) # No more than 6 due to offset - with self.assertNumQueries(5 + per_user_queries): + per_user_queries = 15 * min(batch_size, 6) # No more than 6 due to offset + with self.assertNumQueries(6 + per_user_queries): with check_mongo_calls(1): compute_grades_for_course_v2.delay( course_key=six.text_type(self.course.id), diff --git a/lms/djangoapps/instructor_task/tasks_helper/grades.py b/lms/djangoapps/instructor_task/tasks_helper/grades.py index 96030fdb28..0818852f98 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/grades.py +++ b/lms/djangoapps/instructor_task/tasks_helper/grades.py @@ -12,14 +12,19 @@ from time import time from instructor_analytics.basic import list_problem_responses from instructor_analytics.csvs import format_dictlist -from certificates.models import CertificateWhitelist, certificate_info_for_user +from certificates.models import CertificateWhitelist, certificate_info_for_user, GeneratedCertificate from courseware.courses import get_course_by_id -from lms.djangoapps.grades.context import grading_context_for_course +from lms.djangoapps.grades.context import grading_context_for_course, grading_context from lms.djangoapps.grades.new.course_grade_factory import CourseGradeFactory +from lms.djangoapps.grades.models import PersistentCourseGrade from lms.djangoapps.teams.models import CourseTeamMembership from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification -from openedx.core.djangoapps.course_groups.cohorts import get_cohort, is_course_cohorted +from openedx.core.djangoapps.content.block_structure.api import get_course_in_cache +from openedx.core.djangoapps.course_groups.cohorts import get_cohort, is_course_cohorted, bulk_cache_cohorts +from openedx.core.djangoapps.user_api.course_tag.api import BulkCourseTags from student.models import CourseEnrollment +from student.roles import BulkRoleCache +from xmodule.modulestore.django import modulestore from xmodule.partitions.partitions_service import PartitionService from xmodule.split_test_module import get_split_user_partitions @@ -30,7 +35,7 @@ from .utils import upload_csv_to_report_store TASK_LOG = logging.getLogger('edx.celery.task') -class CourseGradeReportContext(object): +class _CourseGradeReportContext(object): """ Internal class that provides a common context to use for a single grade report. When a report is parallelized across multiple processes, @@ -57,6 +62,10 @@ class CourseGradeReportContext(object): def course(self): return get_course_by_id(self.course_id) + @lazy + def course_structure(self): + return get_course_in_cache(self.course_id) + @lazy def course_experiments(self): return get_split_user_partitions(self.course.user_partitions) @@ -75,9 +84,9 @@ class CourseGradeReportContext(object): Returns an OrderedDict that maps an assignment type to a dict of subsection-headers and average-header. """ - grading_context = grading_context_for_course(self.course_id) + grading_cxt = grading_context(self.course_structure) graded_assignments_map = OrderedDict() - for assignment_type_name, subsection_infos in grading_context['all_graded_subsections_by_type'].iteritems(): + for assignment_type_name, subsection_infos in grading_cxt['all_graded_subsections_by_type'].iteritems(): graded_subsections_map = OrderedDict() for subsection_index, subsection_info in enumerate(subsection_infos, start=1): subsection = subsection_info['subsection_block'] @@ -112,17 +121,64 @@ class CourseGradeReportContext(object): return self.task_progress.update_task_state(extra_meta={'step': message}) +class _CertificateBulkContext(object): + def __init__(self, context, users): + certificate_whitelist = CertificateWhitelist.objects.filter(course_id=context.course_id, whitelist=True) + self.whitelisted_user_ids = [entry.user_id for entry in certificate_whitelist] + self.certificates_by_user = { + certificate.user.id: certificate + for certificate in + GeneratedCertificate.objects.filter(course_id=context.course_id, user__in=users) + } + + +class _TeamBulkContext(object): + def __init__(self, context, users): + if context.teams_enabled: + self.teams_by_user = { + membership.user.id: membership.team.name + for membership in + CourseTeamMembership.objects.filter(team__course_id=context.course_id, user__in=users) + } + else: + self.teams_by_user = {} + + +class _EnrollmentBulkContext(object): + def __init__(self, context, users): + CourseEnrollment.bulk_fetch_enrollment_states(users, context.course_id) + self.verified_users = [ + verified.user.id for verified in + SoftwareSecurePhotoVerification.verified_query().filter(user__in=users).select_related('user__id') + ] + + +class _CourseGradeBulkContext(object): + def __init__(self, context, users): + self.certs = _CertificateBulkContext(context, users) + self.teams = _TeamBulkContext(context, users) + self.enrollments = _EnrollmentBulkContext(context, users) + bulk_cache_cohorts(context.course_id, users) + BulkRoleCache.prefetch(users) + PersistentCourseGrade.prefetch(context.course_id, users) + BulkCourseTags.prefetch(context.course_id, users) + + class CourseGradeReport(object): """ Class to encapsulate functionality related to generating Grade Reports. """ + # Batch size for chunking the list of enrollees in the course. + USER_BATCH_SIZE = 100 + @classmethod def generate(cls, _xmodule_instance_args, _entry_id, course_id, _task_input, action_name): """ Public method to generate a grade report. """ - context = CourseGradeReportContext(_xmodule_instance_args, _entry_id, course_id, _task_input, action_name) - return CourseGradeReport()._generate(context) + with modulestore().bulk_operations(course_id): + context = _CourseGradeReportContext(_xmodule_instance_args, _entry_id, course_id, _task_input, action_name) + return CourseGradeReport()._generate(context) def _generate(self, context): """ @@ -166,6 +222,7 @@ class CourseGradeReport(object): A generator of batches of (success_rows, error_rows) for this report. """ for users in self._batch_users(context): + users = filter(lambda u: u is not None, users) yield self._rows_for_users(context, users) def _compile(self, context, batched_rows): @@ -211,10 +268,11 @@ class CourseGradeReport(object): """ Returns a generator of batches of users. """ - def grouper(iterable, chunk_size=1, fillvalue=None): + def grouper(iterable, chunk_size=self.USER_BATCH_SIZE, fillvalue=None): args = [iter(iterable)] * chunk_size return izip_longest(*args, fillvalue=fillvalue) users = CourseEnrollment.objects.users_enrolled_in(context.course_id) + users = users.select_related('profile__allow_certificate') return grouper(users) def _user_grade_results(self, course_grade, context): @@ -249,7 +307,7 @@ class CourseGradeReport(object): """ cohort_group_names = [] if context.cohorts_enabled: - group = get_cohort(user, context.course_id, assign=False) + group = get_cohort(user, context.course_id, assign=False, use_cached=True) cohort_group_names.append(group.name if group else '') return cohort_group_names @@ -264,20 +322,13 @@ class CourseGradeReport(object): experiment_group_names.append(group.name if group else '') return experiment_group_names - def _user_team_names(self, user, context): + def _user_team_names(self, user, bulk_teams): """ Returns a list of names of teams in which the given user belongs. """ - team_names = [] - if context.teams_enabled: - try: - membership = CourseTeamMembership.objects.get(user=user, team__course_id=context.course_id) - team_names.append(membership.team.name) - except CourseTeamMembership.DoesNotExist: - team_names.append('') - return team_names + return [bulk_teams.teams_by_user.get(user.id, '')] - def _user_verification_mode(self, user, context): + def _user_verification_mode(self, user, context, bulk_enrollments): """ Returns a list of enrollment-mode and verification-status for the given user. @@ -286,19 +337,21 @@ class CourseGradeReport(object): verification_status = SoftwareSecurePhotoVerification.verification_status_for_user( user, context.course_id, - enrollment_mode + enrollment_mode, + user_is_verified=user.id in bulk_enrollments.verified_users, ) return [enrollment_mode, verification_status] - def _user_certificate_info(self, user, context, course_grade, whitelisted_user_ids): + def _user_certificate_info(self, user, context, course_grade, bulk_certs): """ Returns the course certification information for the given user. """ + is_whitelisted = user.id in bulk_certs.whitelisted_user_ids certificate_info = certificate_info_for_user( user, - context.course_id, course_grade.letter_grade, - user.id in whitelisted_user_ids + is_whitelisted, + bulk_certs.certificates_by_user.get(user.id), ) TASK_LOG.info( u'Student certificate eligibility: %s ' @@ -311,7 +364,7 @@ class CourseGradeReport(object): course_grade.letter_grade, context.course.grade_cutoffs, user.profile.allow_certificate, - user.id in whitelisted_user_ids, + is_whitelisted, ) return certificate_info @@ -319,24 +372,30 @@ class CourseGradeReport(object): """ Returns a list of rows for the given users for this report. """ - certificate_whitelist = CertificateWhitelist.objects.filter(course_id=context.course_id, whitelist=True) - whitelisted_user_ids = [entry.user_id for entry in certificate_whitelist] - success_rows, error_rows = [], [] - for user, course_grade, error in CourseGradeFactory().iter(users, course_key=context.course_id): - if not course_grade: - # An empty gradeset means we failed to grade a student. - error_rows.append([user.id, user.username, error.message]) - else: - success_rows.append( - [user.id, user.email, user.username] + - self._user_grade_results(course_grade, context) + - self._user_cohort_group_names(user, context) + - self._user_experiment_group_names(user, context) + - self._user_team_names(user, context) + - self._user_verification_mode(user, context) + - self._user_certificate_info(user, context, course_grade, whitelisted_user_ids) - ) - return success_rows, error_rows + with modulestore().bulk_operations(context.course_id): + bulk_context = _CourseGradeBulkContext(context, users) + + success_rows, error_rows = [], [] + for user, course_grade, error in CourseGradeFactory().iter( + users, + course=context.course, + collected_block_structure=context.course_structure, + course_key=context.course_id, + ): + if not course_grade: + # An empty gradeset means we failed to grade a student. + error_rows.append([user.id, user.username, error.message]) + else: + success_rows.append( + [user.id, user.email, user.username] + + self._user_grade_results(course_grade, context) + + self._user_cohort_group_names(user, context) + + self._user_experiment_group_names(user, context) + + self._user_team_names(user, bulk_context.teams) + + self._user_verification_mode(user, context, bulk_context.enrollments) + + self._user_certificate_info(user, context, course_grade, bulk_context.certs) + ) + return success_rows, error_rows class ProblemGradeReport(object): diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index dd230dd320..b2b18e19ab 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -34,9 +34,11 @@ from lms.djangoapps.teams.tests.factories import CourseTeamFactory, CourseTeamMe from lms.djangoapps.verify_student.tests.factories import SoftwareSecurePhotoVerificationFactory from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup, CohortMembership from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory +from openedx.core.djangoapps.credit.tests.factories import CreditCourseFactory import openedx.core.djangoapps.user_api.course_tag.api as course_tag_api from openedx.core.djangoapps.user_api.partition_schemes import RandomUserPartitionScheme from openedx.core.djangoapps.util.testing import ContentGroupTestCase, TestConditionalContent +from request_cache.middleware import RequestCache from shoppingcart.models import ( Order, PaidCourseRegistration, CourseRegistrationCode, Invoice, CourseRegistrationCodeInvoiceItem, InvoiceTransaction, Coupon @@ -44,8 +46,9 @@ from shoppingcart.models import ( from student.models import CourseEnrollment, CourseEnrollmentAllowed, ManualEnrollmentAudit, ALLOWEDTOENROLL_TO_ENROLLED from student.tests.factories import CourseEnrollmentFactory, CourseModeFactory, UserFactory from survey.models import SurveyForm, SurveyAnswer +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls from xmodule.partitions.partitions import Group, UserPartition from ..models import ReportStore @@ -321,6 +324,44 @@ class TestInstructorGradeReport(InstructorGradeReportTestCase): result = CourseGradeReport.generate(None, None, self.course.id, None, 'graded') self.assertDictContainsSubset({'attempted': 1, 'succeeded': 1, 'failed': 0}, result) + @ddt.data( + (ModuleStoreEnum.Type.mongo, 4), + (ModuleStoreEnum.Type.split, 3), + ) + @ddt.unpack + def test_query_counts(self, store_type, mongo_count): + with self.store.default_store(store_type): + experiment_group_a = Group(2, u'Expériment Group A') + experiment_group_b = Group(3, u'Expériment Group B') + experiment_partition = UserPartition( + 1, + u'Content Expériment Configuration', + u'Group Configuration for Content Expériments', + [experiment_group_a, experiment_group_b], + scheme_id='random' + ) + course = CourseFactory.create( + cohort_config={'cohorted': True, 'auto_cohort': True, 'auto_cohort_groups': ['cohort 1', 'cohort 2']}, + user_partitions=[experiment_partition], + teams_configuration={ + 'max_size': 2, 'topics': [{'topic-id': 'topic', 'name': 'Topic', 'description': 'A Topic'}] + }, + ) + _ = CreditCourseFactory(course_key=course.id) + + num_users = 5 + for _ in range(num_users): + user = UserFactory.create() + CourseEnrollment.enroll(user, course.id, mode='verified') + SoftwareSecurePhotoVerificationFactory.create(user=user, status='approved') + + RequestCache.clear_request_cache() + + with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'): + with check_mongo_calls(mongo_count): + with self.assertNumQueries(41): + CourseGradeReport.generate(None, None, course.id, None, 'graded') + class TestTeamGradeReport(InstructorGradeReportTestCase): """ Test that teams appear correctly in the grade report when it is enabled for the course. """ @@ -1783,7 +1824,7 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase): 'failed': 3, 'skipped': 2 } - with self.assertNumQueries(186): + with self.assertNumQueries(171): self.assertCertificatesGenerated(task_input, expected_results) expected_results = { diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 02e953389f..5373347e58 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -210,12 +210,19 @@ class PhotoVerification(StatusModel): This will check for the user's *initial* verification. """ + return cls.verified_query(earliest_allowed_date).filter(user=user).exists() + + @classmethod + def verified_query(cls, earliest_allowed_date=None): + """ + Return a query set for all records with 'approved' state + that are still valid according to the earliest_allowed_date + value or policy settings. + """ return cls.objects.filter( - user=user, status="approved", - created_at__gte=(earliest_allowed_date - or cls._earliest_allowed_date()) - ).exists() + created_at__gte=(earliest_allowed_date or cls._earliest_allowed_date()), + ) @classmethod def verification_valid_or_pending(cls, user, earliest_allowed_date=None, queryset=None): @@ -951,14 +958,15 @@ class SoftwareSecurePhotoVerification(PhotoVerification): return response @classmethod - def verification_status_for_user(cls, user, course_id, user_enrollment_mode): + def verification_status_for_user(cls, user, course_id, user_enrollment_mode, user_is_verified=None): """ Returns the verification status for use in grade report. """ if user_enrollment_mode not in CourseMode.VERIFIED_MODES: return 'N/A' - user_is_verified = cls.user_is_verified(user) + if user_is_verified is None: + user_is_verified = cls.user_is_verified(user) if not user_is_verified: return 'Not ID Verified' diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py index 8e69f239e8..963934d625 100644 --- a/openedx/core/djangoapps/course_groups/cohorts.py +++ b/openedx/core/djangoapps/course_groups/cohorts.py @@ -14,7 +14,8 @@ from django.utils.translation import ugettext as _ from courseware import courses from eventtracking import tracker -from request_cache.middleware import RequestCache, request_cached +import request_cache +from request_cache.middleware import request_cached from student.models import get_user_by_username_or_email from .models import ( @@ -146,8 +147,45 @@ def get_cohorted_commentables(course_key): return ans +COHORT_CACHE_NAMESPACE = u"cohorts.get_cohort" + + +def _cohort_cache_key(user_id, course_key): + """ + Returns the cache key for the given user_id and course_key. + """ + return u"{}.{}".format(user_id, course_key) + + +def bulk_cache_cohorts(course_key, users): + """ + Pre-fetches and caches the cohort assignments for the + given users, for later fast retrieval by get_cohort. + """ + # before populating the cache with another bulk set of data, + # remove previously cached entries to keep memory usage low. + request_cache.clear_cache(COHORT_CACHE_NAMESPACE) + cache = request_cache.get_cache(COHORT_CACHE_NAMESPACE) + + if is_course_cohorted(course_key): + cohorts_by_user = { + membership.user: membership + for membership in + CohortMembership.objects.filter(user__in=users, course_id=course_key).select_related('user__id') + } + for user, membership in cohorts_by_user.iteritems(): + cache[_cohort_cache_key(user.id, course_key)] = membership.course_user_group + uncohorted_users = filter(lambda u: u not in cohorts_by_user, users) + else: + uncohorted_users = users + + for user in uncohorted_users: + cache[_cohort_cache_key(user.id, course_key)] = None + + def get_cohort(user, course_key, assign=True, use_cached=False): - """Returns the user's cohort for the specified course. + """ + Returns the user's cohort for the specified course. 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 @@ -166,19 +204,19 @@ def get_cohort(user, course_key, assign=True, use_cached=False): Raises: ValueError if the CourseKey doesn't exist. """ - request_cache = RequestCache.get_request_cache() - cache_key = u"cohorts.get_cohort.{}.{}".format(user.id, course_key) + cache = request_cache.get_cache(COHORT_CACHE_NAMESPACE) + cache_key = _cohort_cache_key(user.id, course_key) - if use_cached and cache_key in request_cache.data: - return request_cache.data[cache_key] + if use_cached and cache_key in cache: + return cache[cache_key] - request_cache.data.pop(cache_key, None) + cache.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_cohort_settings = get_course_cohort_settings(course_key) if not course_cohort_settings.is_cohorted: - return request_cache.data.setdefault(cache_key, None) + return cache.setdefault(cache_key, None) # If course is cohorted, check if the user already has a cohort. try: @@ -186,7 +224,7 @@ def get_cohort(user, course_key, assign=True, use_cached=False): course_id=course_key, user_id=user.id, ) - return request_cache.data.setdefault(cache_key, membership.course_user_group) + return cache.setdefault(cache_key, membership.course_user_group) except CohortMembership.DoesNotExist: # Didn't find the group. If we do not want to assign, return here. if not assign: @@ -201,7 +239,7 @@ def get_cohort(user, course_key, assign=True, use_cached=False): user=user, course_user_group=get_random_cohort(course_key) ) - return request_cache.data.setdefault(cache_key, membership.course_user_group) + return cache.setdefault(cache_key, membership.course_user_group) except IntegrityError as integrity_error: # An IntegrityError is raised when multiple workers attempt to # create the same row in one of the cohort model entries: @@ -419,21 +457,21 @@ def get_group_info_for_cohort(cohort, use_cached=False): use_cached=True to use the cached value instead of fetching from the database. """ - request_cache = RequestCache.get_request_cache() - cache_key = u"cohorts.get_group_info_for_cohort.{}".format(cohort.id) + cache = request_cache.get_cache(u"cohorts.get_group_info_for_cohort") + cache_key = unicode(cohort.id) - if use_cached and cache_key in request_cache.data: - return request_cache.data[cache_key] + if use_cached and cache_key in cache: + return cache[cache_key] - request_cache.data.pop(cache_key, None) + cache.pop(cache_key, 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)) + return cache.setdefault(cache_key, (partition_group.group_id, partition_group.partition_id)) except CourseUserGroupPartitionGroup.DoesNotExist: pass - return request_cache.data.setdefault(cache_key, (None, None)) + return cache.setdefault(cache_key, (None, None)) def set_assignment_type(user_group, assignment_type): diff --git a/openedx/core/djangoapps/credit/models.py b/openedx/core/djangoapps/credit/models.py index 67cfb4ee72..a6829f2fbe 100644 --- a/openedx/core/djangoapps/credit/models.py +++ b/openedx/core/djangoapps/credit/models.py @@ -22,6 +22,7 @@ from model_utils.models import TimeStampedModel import pytz from simple_history.models import HistoricalRecords from openedx.core.djangoapps.xmodule_django.models import CourseKeyField +from request_cache.middleware import ns_request_cached, RequestCache CREDIT_PROVIDER_ID_REGEX = r"[a-z,A-Z,0-9,\-]+" @@ -290,6 +291,8 @@ class CreditRequirement(TimeStampedModel): criteria = JSONField() active = models.BooleanField(default=True) + CACHE_NAMESPACE = u"credit.CreditRequirement.cache." + class Meta(object): unique_together = ('namespace', 'name', 'course') ordering = ["order"] @@ -331,6 +334,7 @@ class CreditRequirement(TimeStampedModel): return credit_requirement, created @classmethod + @ns_request_cached(CACHE_NAMESPACE) def get_course_requirements(cls, course_key, namespace=None, name=None): """ Get credit requirements of a given course. @@ -392,6 +396,13 @@ class CreditRequirement(TimeStampedModel): return None +@receiver(models.signals.post_save, sender=CreditRequirement) +@receiver(models.signals.post_delete, sender=CreditRequirement) +def invalidate_credit_requirement_cache(sender, **kwargs): # pylint: disable=unused-argument + """Invalidate the cache of credit requirements. """ + RequestCache.clear_request_cache(name=CreditRequirement.CACHE_NAMESPACE) + + class CreditRequirementStatus(TimeStampedModel): """ This model represents the status of each requirement. diff --git a/openedx/core/djangoapps/credit/tests/test_api.py b/openedx/core/djangoapps/credit/tests/test_api.py index ac61d1c921..ee57fbe99c 100644 --- a/openedx/core/djangoapps/credit/tests/test_api.py +++ b/openedx/core/djangoapps/credit/tests/test_api.py @@ -664,7 +664,7 @@ class CreditRequirementApiTests(CreditApiTestBase): self.assertFalse(api.is_user_eligible_for_credit(user.username, self.course_key)) # Satisfy the other requirement - with self.assertNumQueries(25): + with self.assertNumQueries(24): api.set_credit_requirement_status( user, self.course_key, @@ -718,7 +718,7 @@ class CreditRequirementApiTests(CreditApiTestBase): # Delete the eligibility entries and satisfy the user's eligibility # requirement again to trigger eligibility notification CreditEligibility.objects.all().delete() - with self.assertNumQueries(17): + with self.assertNumQueries(16): api.set_credit_requirement_status( user, self.course_key, diff --git a/openedx/core/djangoapps/user_api/course_tag/api.py b/openedx/core/djangoapps/user_api/course_tag/api.py index 6aa9d7d948..218954faf3 100644 --- a/openedx/core/djangoapps/user_api/course_tag/api.py +++ b/openedx/core/djangoapps/user_api/course_tag/api.py @@ -7,6 +7,8 @@ Stores global metadata using the UserPreference model, and per-course metadata u UserCourseTag model. """ +from collections import defaultdict +from request_cache import get_cache from ..models import UserCourseTag # Scopes @@ -15,6 +17,42 @@ from ..models import UserCourseTag COURSE_SCOPE = 'course' +class BulkCourseTags(object): + CACHE_NAMESPACE = u'user_api.course_tag.api' + + @classmethod + def prefetch(cls, course_id, users): + """ + Prefetches the value of the course tags for the specified users + for the specified course_id. + + Args: + users: iterator of User objects + course_id: course identifier (CourseKey) + + Returns: + course_tags: a dict of dicts, + where the primary key is the user's id + and the secondary key is the course tag's key + """ + course_tags = defaultdict(dict) + for tag in UserCourseTag.objects.filter(user__in=users, course_id=course_id).select_related('user__id'): + course_tags[tag.user.id][tag.key] = tag.value + get_cache(cls.CACHE_NAMESPACE)[cls._cache_key(course_id)] = course_tags + + @classmethod + def get_course_tag(cls, user_id, course_id, key): + return get_cache(cls.CACHE_NAMESPACE)[cls._cache_key(course_id)][user_id][key] + + @classmethod + def is_prefetched(cls, course_id): + return cls._cache_key(course_id) in get_cache(cls.CACHE_NAMESPACE) + + @classmethod + def _cache_key(cls, course_id): + return u'course_tag.{}'.format(course_id) + + def get_course_tag(user, course_id, key): """ Gets the value of the user's course tag for the specified key in the specified @@ -28,6 +66,11 @@ def get_course_tag(user, course_id, key): Returns: string value, or None if there is no value saved """ + if BulkCourseTags.is_prefetched(course_id): + try: + return BulkCourseTags.get_course_tag(user.id, course_id, key) + except KeyError: + return None try: record = UserCourseTag.objects.get( user=user, diff --git a/openedx/core/djangoapps/user_api/partition_schemes.py b/openedx/core/djangoapps/user_api/partition_schemes.py index a772a1260b..070acbd0cc 100644 --- a/openedx/core/djangoapps/user_api/partition_schemes.py +++ b/openedx/core/djangoapps/user_api/partition_schemes.py @@ -70,7 +70,7 @@ class RandomUserPartitionScheme(object): exc_info=True ) - if group is None and assign: + if group is None and assign and not course_tag_api.BulkCourseTags.is_prefetched(course_key): if not user_partition.groups: raise UserPartitionError('Cannot assign user to an empty user partition') diff --git a/openedx/core/djangoapps/user_api/tests/test_partition_schemes.py b/openedx/core/djangoapps/user_api/tests/test_partition_schemes.py index ba80846cea..d12bc3f8c6 100644 --- a/openedx/core/djangoapps/user_api/tests/test_partition_schemes.py +++ b/openedx/core/djangoapps/user_api/tests/test_partition_schemes.py @@ -26,6 +26,11 @@ class MemoryCourseTagAPI(object): """Gets the value of ``key``""" self._tags[course_id][key] = value + class BulkCourseTags(object): + @classmethod + def is_prefetched(self, course_id): + return False + class TestRandomUserPartitionScheme(PartitionTestCase): """ diff --git a/openedx/core/djangoapps/verified_track_content/models.py b/openedx/core/djangoapps/verified_track_content/models.py index 213c53bf5b..8c96960261 100644 --- a/openedx/core/djangoapps/verified_track_content/models.py +++ b/openedx/core/djangoapps/verified_track_content/models.py @@ -5,17 +5,17 @@ from django.db import models from django.utils.translation import ugettext_lazy from django.dispatch import receiver from django.db.models.signals import post_save, pre_save +import logging -from openedx.core.djangoapps.xmodule_django.models import CourseKeyField -from student.models import CourseEnrollment from lms.djangoapps.courseware.courses import get_course_by_id - +from openedx.core.djangoapps.xmodule_django.models import CourseKeyField from openedx.core.djangoapps.verified_track_content.tasks import sync_cohort_with_mode from openedx.core.djangoapps.course_groups.cohorts import ( get_course_cohorts, CourseCohort, is_course_cohorted, get_random_cohort ) +from request_cache.middleware import ns_request_cached, RequestCache +from student.models import CourseEnrollment -import logging log = logging.getLogger(__name__) @@ -97,6 +97,8 @@ class VerifiedTrackCohortedCourse(models.Model): enabled = models.BooleanField() + CACHE_NAMESPACE = u"verified_track_content.VerifiedTrackCohortedCourse.cache." + def __unicode__(self): return u"Course: {}, enabled: {}".format(unicode(self.course_key), self.enabled) @@ -119,6 +121,7 @@ class VerifiedTrackCohortedCourse(models.Model): return None @classmethod + @ns_request_cached(CACHE_NAMESPACE) def is_verified_track_cohort_enabled(cls, course_key): """ Checks whether or not verified track cohort is enabled for the given course. @@ -134,3 +137,10 @@ class VerifiedTrackCohortedCourse(models.Model): return cls.objects.get(course_key=course_key).enabled except cls.DoesNotExist: return False + + +@receiver(models.signals.post_save, sender=VerifiedTrackCohortedCourse) +@receiver(models.signals.post_delete, sender=VerifiedTrackCohortedCourse) +def invalidate_verified_track_cache(sender, **kwargs): # pylint: disable=unused-argument + """Invalidate the cache of VerifiedTrackCohortedCourse. """ + RequestCache.clear_request_cache(name=VerifiedTrackCohortedCourse.CACHE_NAMESPACE) From 4430695b1c771e5e701b5fbcf2166f5e80c0eef7 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Wed, 10 May 2017 16:05:44 -0400 Subject: [PATCH 2/2] Fix column alignment issue when Teams is disabled This reverts commit b6b704019f35aa4e81f077e5c6bb65dd42af56a0. --- .../instructor_task/tasks_helper/grades.py | 8 ++++-- .../tests/test_tasks_helper.py | 27 ++++++++++++------- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/lms/djangoapps/instructor_task/tasks_helper/grades.py b/lms/djangoapps/instructor_task/tasks_helper/grades.py index 0818852f98..69963b001a 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/grades.py +++ b/lms/djangoapps/instructor_task/tasks_helper/grades.py @@ -134,7 +134,8 @@ class _CertificateBulkContext(object): class _TeamBulkContext(object): def __init__(self, context, users): - if context.teams_enabled: + self.enabled = context.teams_enabled + if self.enabled: self.teams_by_user = { membership.user.id: membership.team.name for membership in @@ -326,7 +327,10 @@ class CourseGradeReport(object): """ Returns a list of names of teams in which the given user belongs. """ - return [bulk_teams.teams_by_user.get(user.id, '')] + team_names = [] + if bulk_teams.enabled: + team_names = [bulk_teams.teams_by_user.get(user.id, '')] + return team_names def _user_verification_mode(self, user, context, bulk_enrollments): """ diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index b2b18e19ab..88a82f4ecb 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -96,10 +96,13 @@ class InstructorGradeReportTestCase(TestReportMixin, InstructorTaskCourseTestCas report_store = ReportStore.from_config(config_name='GRADES_DOWNLOAD') report_csv_filename = report_store.links_for(course_id)[0][0] report_path = report_store.path_to(course_id, report_csv_filename) + found_user = False with report_store.storage.open(report_path) as csv_file: for row in unicodecsv.DictReader(csv_file): - if row.get('username') == username: + if row.get('Username') == username: self.assertEqual(row[column_header], expected_cell_content) + found_user = True + self.assertTrue(found_user) @ddt.ddt @@ -301,7 +304,7 @@ class TestInstructorGradeReport(InstructorGradeReportTestCase): user_b.username, course.id, cohort_name_header, - u'Default Group', + u'', ) @patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task') @@ -1229,13 +1232,11 @@ class TestTeamStudentReport(TestReportMixin, InstructorTaskCourseTestCase): """ Run the upload_students_csv task and verify that the correct team was added to the CSV. """ current_task = Mock() current_task.update_state = Mock() - task_input = { - 'features': [ - 'id', 'username', 'name', 'email', 'language', 'location', - 'year_of_birth', 'gender', 'level_of_education', 'mailing_address', - 'goals', 'team' - ] - } + task_input = [ + 'id', 'username', 'name', 'email', 'language', 'location', + 'year_of_birth', 'gender', 'level_of_education', 'mailing_address', + 'goals', 'team' + ] with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task') as mock_current_task: mock_current_task.return_value = current_task result = upload_students_csv(None, None, self.course.id, task_input, 'calculated') @@ -1243,10 +1244,13 @@ class TestTeamStudentReport(TestReportMixin, InstructorTaskCourseTestCase): report_store = ReportStore.from_config(config_name='GRADES_DOWNLOAD') report_csv_filename = report_store.links_for(self.course.id)[0][0] report_path = report_store.path_to(self.course.id, report_csv_filename) + found_user = False with report_store.storage.open(report_path) as csv_file: for row in unicodecsv.DictReader(csv_file): if row.get('username') == username: self.assertEqual(row['team'], expected_team) + found_user = True + self.assertTrue(found_user) def test_team_column_no_teams(self): self._generate_and_verify_teams_column(self.student1.username, UNAVAILABLE) @@ -1698,11 +1702,14 @@ class TestGradeReportEnrollmentAndCertificateInfo(TestReportMixin, InstructorTas report_store = ReportStore.from_config(config_name='GRADES_DOWNLOAD') report_csv_filename = report_store.links_for(self.course.id)[0][0] report_path = report_store.path_to(self.course.id, report_csv_filename) + found_user = False with report_store.storage.open(report_path) as csv_file: for row in unicodecsv.DictReader(csv_file): - if row.get('username') == username: + if row.get('Username') == username: csv_row_data = [row[column] for column in self.columns_to_check] self.assertEqual(csv_row_data, expected_data) + found_user = True + self.assertTrue(found_user) def _create_user_data(self, user_enroll_mode,