From a9c4774fe70c9b0a43adfbdc48f7e5d153b2423d Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Mon, 20 Jul 2015 10:29:37 -0400 Subject: [PATCH 1/9] PLA-749 Fix student dashboard for users enrolled in CCX courses --- lms/djangoapps/ccx/tests/test_utils.py | 4 +- lms/djangoapps/ccx/tests/test_views.py | 40 ++++++++++++ lms/djangoapps/ccx/utils.py | 65 +++++++++++-------- lms/templates/ccx/_dashboard_ccx_listing.html | 2 +- lms/templates/dashboard.html | 4 +- 5 files changed, 82 insertions(+), 33 deletions(-) diff --git a/lms/djangoapps/ccx/tests/test_utils.py b/lms/djangoapps/ccx/tests/test_utils.py index 6dd8924967..7fa024c924 100644 --- a/lms/djangoapps/ccx/tests/test_utils.py +++ b/lms/djangoapps/ccx/tests/test_utils.py @@ -557,9 +557,9 @@ class TestGetMembershipTriplets(ModuleStoreTestCase): self.make_ccx_membership() triplets = self.call_fut() self.assertEqual(len(triplets), 1) - ccx, membership, course = triplets[0] + ccx, membership, course_overview = triplets[0] self.assertEqual(ccx.id, self.ccx.id) - self.assertEqual(unicode(course.id), unicode(self.course.id)) + self.assertEqual(unicode(course_overview.id), unicode(self.course.id)) self.assertEqual(membership.student, self.user) def test_has_membership_org_filtered(self): diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 7111afda46..78300ab5de 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -16,9 +16,11 @@ from courseware.tests.factories import StudentModuleFactory # pylint: disable=i from courseware.tests.helpers import LoginEnrollmentTestCase # pylint: disable=import-error from courseware.tabs import get_course_tab_list from django.core.urlresolvers import reverse +from django.utils.timezone import UTC from django.test.utils import override_settings from django.test import RequestFactory from edxmako.shortcuts import render_to_response # pylint: disable=import-error +from student.models import CourseEnrollment from student.roles import CourseCcxCoachRole # pylint: disable=import-error from student.tests.factories import ( # pylint: disable=import-error AdminFactory, @@ -27,6 +29,7 @@ from student.tests.factories import ( # pylint: disable=import-error ) from xmodule.x_module import XModuleMixin +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import ( ModuleStoreTestCase, TEST_DATA_SPLIT_MODULESTORE) @@ -655,6 +658,43 @@ class CCXCoachTabTestCase(ModuleStoreTestCase): ) +class TestStudentDashboardWithCCX(ModuleStoreTestCase): + """ + Test to ensure that the student dashboard works for users enrolled in CCX + courses. + """ + + def setUp(self): + """ + Set up courses and enrollments. + """ + super(TestStudentDashboardWithCCX, self).setUp() + + # Create a Draft Mongo and a Split Mongo course and enroll a student user in them. + self.student_password = "foobar" + self.student = UserFactory.create(username="test", password=self.student_password, is_staff=False) + self.draft_course = CourseFactory.create(default_store=ModuleStoreEnum.Type.mongo) + self.split_course = CourseFactory.create(default_store=ModuleStoreEnum.Type.split) + CourseEnrollment.enroll(self.student, self.draft_course.id) + CourseEnrollment.enroll(self.student, self.split_course.id) + + # Create a CCX coach. + self.coach = AdminFactory.create() + role = CourseCcxCoachRole(self.split_course.id) + role.add_users(self.coach) + + # Create a CCX course and enroll the user in it. + self.ccx = CcxFactory(course_id=self.split_course.id, coach=self.coach) + last_week = datetime.datetime.now(UTC()) - datetime.timedelta(days=7) + override_field_for_ccx(self.ccx, self.split_course, 'start', last_week) # Required by self.ccx.has_started(). + CcxMembershipFactory(ccx=self.ccx, student=self.student, active=True) + + def test_load_student_dashboard(self): + self.client.login(username=self.student.username, password=self.student_password) + response = self.client.get(reverse('dashboard')) + self.assertEqual(response.status_code, 200) + + def flatten(seq): """ For [[1, 2], [3, 4]] returns [1, 2, 3, 4]. Does not recurse. diff --git a/lms/djangoapps/ccx/utils.py b/lms/djangoapps/ccx/utils.py index 7d493c4b93..5b69cd794b 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/lms/djangoapps/ccx/utils.py @@ -10,6 +10,7 @@ from django.core.urlresolvers import reverse from django.core.mail import send_mail from edxmako.shortcuts import render_to_string # pylint: disable=import-error from microsite_configuration import microsite # pylint: disable=import-error +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from xmodule.modulestore.django import modulestore from xmodule.error_module import ErrorDescriptor from ccx_keys.locator import CCXLocator @@ -240,38 +241,46 @@ def send_mail_to_student(student, param_dict): def get_ccx_membership_triplets(user, course_org_filter, org_filter_out_set): """ - Get the relevant set of (CustomCourseForEdX, CcxMembership, Course) + Get the relevant set of (CustomCourseForEdX, CcxMembership, CourseOverview) triplets to be displayed on a student's dashboard. """ + error_message_format = "User {0} enrolled in {2} course {1}" + # only active memberships for now for membership in CcxMembership.memberships_for_user(user): ccx = membership.ccx - store = modulestore() - with store.bulk_operations(ccx.course_id): - course = store.get_course(ccx.course_id) - if course and not isinstance(course, ErrorDescriptor): - # if we are in a Microsite, then filter out anything that is not - # attributed (by ORG) to that Microsite - if course_org_filter and course_org_filter != course.location.org: - continue - # Conversely, if we are not in a Microsite, then let's filter out any enrollments - # with courses attributed (by ORG) to Microsites - elif course.location.org in org_filter_out_set: - continue - # If, somehow, we've got a ccx that has been created for a - # course with a deprecated ID, we must filter it out. Emit a - # warning to the log so we can clean up. - if course.location.deprecated: - log.warning( - "CCX %s exists for course %s with deprecated id", - ccx, - ccx.course_id - ) - continue + try: + course_overview = CourseOverview.get_from_id(ccx.course_id) + except CourseOverview.DoesNotExist: + log.error(error_message_format.format( # pylint: disable=logging-format-interpolation + user.username, ccx.course_id, "non-existent" + )) + continue + except IOError: + log.error(error_message_format.format( # pylint: disable=logging-format-interpolation + user.username, ccx.course_id, "broken" + )) + continue - yield (ccx, membership, course) - else: - log.error("User {0} enrolled in {2} course {1}".format( # pylint: disable=logging-format-interpolation - user.username, ccx.course_id, "broken" if course else "non-existent" - )) + # if we are in a Microsite, then filter out anything that is not + # attributed (by ORG) to that Microsite + if course_org_filter and course_org_filter != course_overview.location.org: + continue + # Conversely, if we are not in a Microsite, then let's filter out any enrollments + # with courses attributed (by ORG) to Microsites + elif course_overview.location.org in org_filter_out_set: + continue + + # If, somehow, we've got a ccx that has been created for a + # course with a deprecated ID, we must filter it out. Emit a + # warning to the log so we can clean up. + if course_overview.location.deprecated: + log.warning( + "CCX %s exists for course %s with deprecated id", + ccx, + ccx.course_id + ) + continue + + yield (ccx, membership, course_overview) diff --git a/lms/templates/ccx/_dashboard_ccx_listing.html b/lms/templates/ccx/_dashboard_ccx_listing.html index c9d1c36f4b..1eca7c6eb5 100644 --- a/lms/templates/ccx/_dashboard_ccx_listing.html +++ b/lms/templates/ccx/_dashboard_ccx_listing.html @@ -42,7 +42,7 @@ from ccx_keys.locator import CCXLocator % endif
- ${get_course_about_section(course, 'university')} - + ${get_course_about_section(course_overview, 'university')} - ${course_overview.display_number_with_default | h} % if ccx.has_ended(): diff --git a/lms/templates/dashboard.html b/lms/templates/dashboard.html index 5d1ffc4c88..9fe38d18c3 100644 --- a/lms/templates/dashboard.html +++ b/lms/templates/dashboard.html @@ -88,10 +88,10 @@ from django.core.urlresolvers import reverse % endfor % if settings.FEATURES.get('CUSTOM_COURSES_EDX', False): - % for ccx, membership, course in ccx_membership_triplets: + % for ccx, membership, course_overview in ccx_membership_triplets: <% show_courseware_link = ccx.has_started() %> <% is_course_blocked = False %> - <%include file='ccx/_dashboard_ccx_listing.html' args="ccx=ccx, membership=membership, course_overview=enrollment.course_overview, show_courseware_link=show_courseware_link, is_course_blocked=is_course_blocked" /> + <%include file='ccx/_dashboard_ccx_listing.html' args="ccx=ccx, membership=membership, course_overview=course_overview, show_courseware_link=show_courseware_link, is_course_blocked=is_course_blocked" /> % endfor % endif From 7b0c22afe99d611c33213c4ae69765492fdb2140 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 10 Jul 2015 14:34:09 -0400 Subject: [PATCH 2/9] Make request_cache easier to use --- common/djangoapps/request_cache/__init__.py | 28 +++++++++++++++ common/djangoapps/request_cache/middleware.py | 36 +++++++++++++------ 2 files changed, 53 insertions(+), 11 deletions(-) diff --git a/common/djangoapps/request_cache/__init__.py b/common/djangoapps/request_cache/__init__.py index e69de29bb2..7d8bf77ef6 100644 --- a/common/djangoapps/request_cache/__init__.py +++ b/common/djangoapps/request_cache/__init__.py @@ -0,0 +1,28 @@ +""" +A cache that is cleared after every request. + +This module requires that :class:`request_cache.middleware.RequestCache` +is installed in order to clear the cache after each request. +""" + + +from request_cache import middleware + + +def get_cache(name): + """ + Return the request cache named ``name``. + + Arguments: + name (str): The name of the request cache to load + + Returns: dict + """ + return middleware.RequestCache.get_request_cache(name) + + +def get_request(): + """ + Return the current request. + """ + return middleware.RequestCache.get_current_request() diff --git a/common/djangoapps/request_cache/middleware.py b/common/djangoapps/request_cache/middleware.py index 7690eac5ba..ecb85633ef 100644 --- a/common/djangoapps/request_cache/middleware.py +++ b/common/djangoapps/request_cache/middleware.py @@ -1,34 +1,48 @@ import threading -_request_cache_threadlocal = threading.local() -_request_cache_threadlocal.data = {} -_request_cache_threadlocal.request = None + +class _RequestCache(threading.local): + """ + A thread-local for storing the per-request cache. + """ + def __init__(self): + super(_RequestCache, self).__init__() + self.data = {} + self.request = None + + +REQUEST_CACHE = _RequestCache() class RequestCache(object): @classmethod - def get_request_cache(cls): - return _request_cache_threadlocal + def get_request_cache(cls, name=None): + """ + This method is deprecated. Please use :func:`request_cache.get_cache`. + """ + if name is None: + return REQUEST_CACHE + else: + return REQUEST_CACHE.data.setdefault(name, {}) @classmethod def get_current_request(cls): """ - Get a reference to the HttpRequest object, if we are presently - servicing one. + This method is deprecated. Please use :func:`request_cache.get_request`. """ - return _request_cache_threadlocal.request + return REQUEST_CACHE.request @classmethod def clear_request_cache(cls): """ Empty the request cache. """ - _request_cache_threadlocal.data = {} - _request_cache_threadlocal.request = None + REQUEST_CACHE.data = {} + REQUEST_CACHE.request = None def process_request(self, request): self.clear_request_cache() - _request_cache_threadlocal.request = request + REQUEST_CACHE.request = request return None def process_response(self, request, response): From 1e84ad99418656066d89a5c51bf94f7665bd24e5 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 10 Jul 2015 13:19:00 -0400 Subject: [PATCH 3/9] Make CcxFactory fill in a coach if one isn't supplied --- lms/djangoapps/ccx/tests/factories.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lms/djangoapps/ccx/tests/factories.py b/lms/djangoapps/ccx/tests/factories.py index 36c976970f..507df6b44a 100644 --- a/lms/djangoapps/ccx/tests/factories.py +++ b/lms/djangoapps/ccx/tests/factories.py @@ -1,7 +1,9 @@ """ Dummy factories for tests """ +from factory import SubFactory from factory.django import DjangoModelFactory +from student.tests.factories import UserFactory from ccx.models import CustomCourseForEdX # pylint: disable=import-error from ccx.models import CcxMembership # pylint: disable=import-error from ccx.models import CcxFutureMembership # pylint: disable=import-error @@ -11,6 +13,7 @@ class CcxFactory(DjangoModelFactory): # pylint: disable=missing-docstring FACTORY_FOR = CustomCourseForEdX display_name = "Test CCX" id = None # pylint: disable=redefined-builtin, invalid-name + coach = SubFactory(UserFactory) class CcxMembershipFactory(DjangoModelFactory): # pylint: disable=missing-docstring From 63ddbd9b037093a92a92d144381f0fe8c0d5c7f3 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 10 Jul 2015 13:43:00 -0400 Subject: [PATCH 4/9] Cache CustomCourseForEdX object lookup per-request --- lms/djangoapps/ccx/overrides.py | 8 +- .../tests/test_field_override_performance.py | 120 +++++++++++++----- lms/djangoapps/ccx/tests/test_overrides.py | 3 + 3 files changed, 95 insertions(+), 36 deletions(-) diff --git a/lms/djangoapps/ccx/overrides.py b/lms/djangoapps/ccx/overrides.py index 4f63fc6324..a2098da6a0 100644 --- a/lms/djangoapps/ccx/overrides.py +++ b/lms/djangoapps/ccx/overrides.py @@ -7,6 +7,8 @@ import logging from django.db import transaction, IntegrityError +import request_cache + from courseware.field_overrides import FieldOverrideProvider # pylint: disable=import-error from opaque_keys.edx.keys import CourseKey, UsageKey from ccx_keys.locator import CCXLocator, CCXBlockUsageLocator @@ -69,7 +71,11 @@ def get_current_ccx(course_key): if not isinstance(course_key, CCXLocator): return None - return CustomCourseForEdX.objects.get(pk=course_key.ccx) + ccx_cache = request_cache.get_cache('ccx') + if course_key not in ccx_cache: + ccx_cache[course_key] = CustomCourseForEdX.objects.get(pk=course_key.ccx) + + return ccx_cache[course_key] def get_override_for_ccx(ccx, block, name, default=None): diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index e012523c28..b9977b8657 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -5,6 +5,7 @@ Performance tests for field overrides. import ddt import itertools import mock +from nose.plugins.skip import SkipTest from courseware.views import progress # pylint: disable=import-error from courseware.field_overrides import OverrideFieldData @@ -25,6 +26,8 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, \ TEST_DATA_SPLIT_MODULESTORE, TEST_DATA_MONGO_MODULESTORE from xmodule.modulestore.tests.factories import check_mongo_calls, CourseFactory, check_sum_of_calls from xmodule.modulestore.tests.utils import ProceduralCourseTestMixin +from ccx_keys.locator import CCXLocator +from ccx.tests.factories import CcxFactory, CcxMembershipFactory @attr('shard_1') @@ -58,6 +61,7 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, self.request = self.request_factory.get("foo") self.request.user = self.student self.course = None + self.ccx = None MakoMiddleware().process_request(self.request) @@ -113,17 +117,42 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, self.course.id ) - def grade_course(self, course): + if enable_ccx: + self.ccx = CcxFactory.create() + CcxMembershipFactory.create( + student=self.student, + ccx=self.ccx + ) + + def grade_course(self, course, view_as_ccx): """ Renders the progress page for the given course. """ + course_key = course.id + if view_as_ccx: + course_key = CCXLocator.from_course_locator(course_key, self.ccx.id) + return progress( self.request, - course_id=course.id.to_deprecated_string(), + course_id=unicode(course_key), student_id=self.student.id ) - def instrument_course_progress_render(self, course_width, enable_ccx, queries, reads, xblocks): + def assertMongoCallCount(self, calls): + """ + Assert that mongodb is queried ``calls`` times in the surrounded + context. + """ + return check_mongo_calls(calls) + + def assertXBlockInstantiations(self, instantiations): + """ + Assert that exactly ``instantiations`` XBlocks are instantiated in + the surrounded context. + """ + return check_sum_of_calls(XBlock, ['__init__'], instantiations, instantiations, include_arguments=False) + + def instrument_course_progress_render(self, course_width, enable_ccx, view_as_ccx, queries, reads, xblocks): """ Renders the progress page, instrumenting Mongo reads and SQL queries. """ @@ -146,16 +175,16 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, OverrideFieldData.provider_classes = None with self.assertNumQueries(queries): - with check_mongo_calls(reads): - with check_sum_of_calls(XBlock, ['__init__'], xblocks, xblocks, include_arguments=False): - self.grade_course(self.course) + with self.assertMongoCallCount(reads): + with self.assertXBlockInstantiations(xblocks): + self.grade_course(self.course, view_as_ccx) - @ddt.data(*itertools.product(('no_overrides', 'ccx'), range(1, 4), (True, False))) + @ddt.data(*itertools.product(('no_overrides', 'ccx'), range(1, 4), (True, False), (True, False))) @ddt.unpack @override_settings( FIELD_OVERRIDE_PROVIDERS=(), ) - def test_field_overrides(self, overrides, course_width, enable_ccx): + def test_field_overrides(self, overrides, course_width, enable_ccx, view_as_ccx): """ Test without any field overrides. """ @@ -163,9 +192,18 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, 'no_overrides': (), 'ccx': ('ccx.overrides.CustomCoursesForEdxOverrideProvider',) } + if overrides == 'no_overrides' and view_as_ccx: + raise SkipTest("Can't view a ccx course if field overrides are disabled.") + + if not enable_ccx and view_as_ccx: + raise SkipTest("Can't view a ccx course if ccx is disabled on the course") + + if self.MODULESTORE == TEST_DATA_MONGO_MODULESTORE and view_as_ccx: + raise SkipTest("Can't use a MongoModulestore test as a CCX course") + with self.settings(FIELD_OVERRIDE_PROVIDERS=providers[overrides]): - queries, reads, xblocks = self.TEST_DATA[(overrides, course_width, enable_ccx)] - self.instrument_course_progress_render(course_width, enable_ccx, queries, reads, xblocks) + queries, reads, xblocks = self.TEST_DATA[(overrides, course_width, enable_ccx, view_as_ccx)] + self.instrument_course_progress_render(course_width, enable_ccx, view_as_ccx, queries, reads, xblocks) class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): @@ -176,19 +214,25 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): __test__ = True TEST_DATA = { - # (providers, course_width, enable_ccx): # of sql queries, # of mongo queries, # of xblocks - ('no_overrides', 1, True): (23, 7, 14), - ('no_overrides', 2, True): (68, 7, 85), - ('no_overrides', 3, True): (263, 7, 336), - ('ccx', 1, True): (23, 7, 14), - ('ccx', 2, True): (68, 7, 85), - ('ccx', 3, True): (263, 7, 336), - ('no_overrides', 1, False): (23, 7, 14), - ('no_overrides', 2, False): (68, 7, 85), - ('no_overrides', 3, False): (263, 7, 336), - ('ccx', 1, False): (23, 7, 14), - ('ccx', 2, False): (68, 7, 85), - ('ccx', 3, False): (263, 7, 336), + # (providers, course_width, enable_ccx, view_as_ccx): # of sql queries, # of mongo queries, # of xblocks + ('no_overrides', 1, True, False): (23, 7, 14), + ('no_overrides', 2, True, False): (68, 7, 85), + ('no_overrides', 3, True, False): (263, 7, 336), + ('ccx', 1, True, False): (23, 7, 14), + ('ccx', 2, True, False): (68, 7, 85), + ('ccx', 3, True, False): (263, 7, 336), + ('ccx', 1, True, True): (23, 7, 14), + ('ccx', 2, True, True): (68, 7, 85), + ('ccx', 3, True, True): (263, 7, 336), + ('no_overrides', 1, False, False): (23, 7, 14), + ('no_overrides', 2, False, False): (68, 7, 85), + ('no_overrides', 3, False, False): (263, 7, 336), + ('ccx', 1, False, False): (23, 7, 14), + ('ccx', 2, False, False): (68, 7, 85), + ('ccx', 3, False, False): (263, 7, 336), + ('ccx', 1, False, True): (23, 7, 14), + ('ccx', 2, False, True): (68, 7, 85), + ('ccx', 3, False, True): (263, 7, 336), } @@ -200,16 +244,22 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True TEST_DATA = { - ('no_overrides', 1, True): (23, 4, 9), - ('no_overrides', 2, True): (68, 19, 54), - ('no_overrides', 3, True): (263, 84, 215), - ('ccx', 1, True): (23, 4, 9), - ('ccx', 2, True): (68, 19, 54), - ('ccx', 3, True): (263, 84, 215), - ('no_overrides', 1, False): (23, 4, 9), - ('no_overrides', 2, False): (68, 19, 54), - ('no_overrides', 3, False): (263, 84, 215), - ('ccx', 1, False): (23, 4, 9), - ('ccx', 2, False): (68, 19, 54), - ('ccx', 3, False): (263, 84, 215), + ('no_overrides', 1, True, False): (23, 4, 9), + ('no_overrides', 2, True, False): (68, 19, 54), + ('no_overrides', 3, True, False): (263, 84, 215), + ('ccx', 1, True, False): (23, 4, 9), + ('ccx', 2, True, False): (68, 19, 54), + ('ccx', 3, True, False): (263, 84, 215), + ('ccx', 1, True, True): (33, 4, 13), + ('ccx', 2, True, True): (68, 19, 84), + ('ccx', 3, True, True): (263, 84, 335), + ('no_overrides', 1, False, False): (23, 4, 9), + ('no_overrides', 2, False, False): (68, 19, 54), + ('no_overrides', 3, False, False): (263, 84, 215), + ('ccx', 1, False, False): (23, 4, 9), + ('ccx', 2, False, False): (68, 19, 54), + ('ccx', 3, False, False): (263, 84, 215), + ('ccx', 1, False, True): (23, 4, 9), + ('ccx', 2, False, True): (68, 19, 54), + ('ccx', 3, False, True): (263, 84, 215), } diff --git a/lms/djangoapps/ccx/tests/test_overrides.py b/lms/djangoapps/ccx/tests/test_overrides.py index 8421dce8f8..01ff0db111 100644 --- a/lms/djangoapps/ccx/tests/test_overrides.py +++ b/lms/djangoapps/ccx/tests/test_overrides.py @@ -9,6 +9,7 @@ from nose.plugins.attrib import attr from courseware.field_overrides import OverrideFieldData # pylint: disable=import-error from django.test.utils import override_settings +from request_cache.middleware import RequestCache from student.tests.factories import AdminFactory # pylint: disable=import-error from xmodule.modulestore.tests.django_utils import ( ModuleStoreTestCase, @@ -66,6 +67,8 @@ class TestFieldOverrides(ModuleStoreTestCase): get_ccx.return_value = ccx self.addCleanup(patch.stop) + self.addCleanup(RequestCache.clear_request_cache) + # Apparently the test harness doesn't use LmsFieldStorage, and I'm not # sure if there's a way to poke the test harness to do so. So, we'll # just inject the override field storage in this brute force manner. From 0e8b303d2ea0702d32978263953813f2c30a93fb Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 10 Jul 2015 13:58:34 -0400 Subject: [PATCH 5/9] Cache CcxFieldOverrides on a per-request basis --- lms/djangoapps/ccx/overrides.py | 68 ++++++++++--------- .../tests/test_field_override_performance.py | 6 +- lms/djangoapps/ccx/tests/test_overrides.py | 4 +- lms/djangoapps/ccx/tests/test_views.py | 29 +++----- 4 files changed, 50 insertions(+), 57 deletions(-) diff --git a/lms/djangoapps/ccx/overrides.py b/lms/djangoapps/ccx/overrides.py index a2098da6a0..462bf73735 100644 --- a/lms/djangoapps/ccx/overrides.py +++ b/lms/djangoapps/ccx/overrides.py @@ -84,35 +84,40 @@ def get_override_for_ccx(ccx, block, name, default=None): specify the block and the name of the field. If the field is not overridden for the given ccx, returns `default`. """ - if not hasattr(block, '_ccx_overrides'): - block._ccx_overrides = {} # pylint: disable=protected-access - overrides = block._ccx_overrides.get(ccx.id) # pylint: disable=protected-access - if overrides is None: - overrides = _get_overrides_for_ccx(ccx, block) - block._ccx_overrides[ccx.id] = overrides # pylint: disable=protected-access - return overrides.get(name, default) + overrides = _get_overrides_for_ccx(ccx) + + if isinstance(block.location, CCXBlockUsageLocator): + non_ccx_key = block.location.to_block_locator() + else: + non_ccx_key = block.location + + block_overrides = overrides.get(non_ccx_key, {}) + if name in block_overrides: + return block.fields[name].from_json(block_overrides[name]) + else: + return default -def _get_overrides_for_ccx(ccx, block): +def _get_overrides_for_ccx(ccx): """ Returns a dictionary mapping field name to overriden value for any overrides set on this block for this CCX. """ - overrides = {} - # block as passed in may have a location specific to a CCX, we must strip - # that for this query - location = block.location - if isinstance(block.location, CCXBlockUsageLocator): - location = block.location.to_block_locator() - query = CcxFieldOverride.objects.filter( - ccx=ccx, - location=location - ) - for override in query: - field = block.fields[override.field] - value = field.from_json(json.loads(override.value)) - overrides[override.field] = value - return overrides + overrides_cache = request_cache.get_cache('ccx-overrides') + + if ccx not in overrides_cache: + overrides = {} + query = CcxFieldOverride.objects.filter( + ccx=ccx, + ) + + for override in query: + block_overrides = overrides.setdefault(override.location, {}) + block_overrides[override.field] = json.loads(override.value) + + overrides_cache[ccx] = overrides + + return overrides_cache[ccx] @transaction.commit_on_success @@ -123,23 +128,25 @@ def override_field_for_ccx(ccx, block, name, value): value to set for the given field. """ field = block.fields[name] - value = json.dumps(field.to_json(value)) + value_json = field.to_json(value) + serialized_value = json.dumps(value_json) try: override = CcxFieldOverride.objects.create( ccx=ccx, location=block.location, field=name, - value=value) + value=serialized_value + ) except IntegrityError: transaction.commit() override = CcxFieldOverride.objects.get( ccx=ccx, location=block.location, - field=name) - override.value = value + field=name + ) + override.value = serialized_value override.save() - if hasattr(block, '_ccx_overrides'): - del block._ccx_overrides[ccx.id] # pylint: disable=protected-access + _get_overrides_for_ccx(ccx).setdefault(block.location, {})[name] = value_json def clear_override_for_ccx(ccx, block, name): @@ -155,8 +162,7 @@ def clear_override_for_ccx(ccx, block, name): location=block.location, field=name).delete() - if hasattr(block, '_ccx_overrides'): - del block._ccx_overrides[ccx.id] # pylint: disable=protected-access + _get_overrides_for_ccx(ccx).setdefault(block.location, {}).pop(name) except CcxFieldOverride.DoesNotExist: pass diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index b9977b8657..77247650fb 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -250,9 +250,9 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): ('ccx', 1, True, False): (23, 4, 9), ('ccx', 2, True, False): (68, 19, 54), ('ccx', 3, True, False): (263, 84, 215), - ('ccx', 1, True, True): (33, 4, 13), - ('ccx', 2, True, True): (68, 19, 84), - ('ccx', 3, True, True): (263, 84, 335), + ('ccx', 1, True, True): (25, 4, 13), + ('ccx', 2, True, True): (70, 19, 84), + ('ccx', 3, True, True): (265, 84, 335), ('no_overrides', 1, False, False): (23, 4, 9), ('no_overrides', 2, False, False): (68, 19, 54), ('no_overrides', 3, False, False): (263, 84, 215), diff --git a/lms/djangoapps/ccx/tests/test_overrides.py b/lms/djangoapps/ccx/tests/test_overrides.py index 01ff0db111..d473c0aafa 100644 --- a/lms/djangoapps/ccx/tests/test_overrides.py +++ b/lms/djangoapps/ccx/tests/test_overrides.py @@ -100,7 +100,7 @@ class TestFieldOverrides(ModuleStoreTestCase): """ ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC) chapter = self.ccx.course.get_children()[0] - with self.assertNumQueries(4): + with self.assertNumQueries(3): override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) dummy = chapter.start @@ -110,7 +110,7 @@ class TestFieldOverrides(ModuleStoreTestCase): """ ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC) chapter = self.ccx.course.get_children()[0] - with self.assertNumQueries(4): + with self.assertNumQueries(3): override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) dummy1 = chapter.start dummy2 = chapter.start diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 78300ab5de..d9a8182017 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -21,6 +21,7 @@ from django.test.utils import override_settings from django.test import RequestFactory from edxmako.shortcuts import render_to_response # pylint: disable=import-error from student.models import CourseEnrollment +from request_cache.middleware import RequestCache from student.roles import CourseCcxCoachRole # pylint: disable=import-error from student.tests.factories import ( # pylint: disable=import-error AdminFactory, @@ -503,26 +504,6 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): role.add_users(coach) ccx = CcxFactory(course_id=course.id, coach=self.coach) - # Apparently the test harness doesn't use LmsFieldStorage, and I'm not - # sure if there's a way to poke the test harness to do so. So, we'll - # just inject the override field storage in this brute force manner. - OverrideFieldData.provider_classes = None - # pylint: disable=protected-access - for block in iter_blocks(course): - block._field_data = OverrideFieldData.wrap(coach, course, block._field_data) - new_cache = {'tabs': [], 'discussion_topics': []} - if 'grading_policy' in block._field_data_cache: - new_cache['grading_policy'] = block._field_data_cache['grading_policy'] - block._field_data_cache = new_cache - - def cleanup_provider_classes(): - """ - After everything is done, clean up by un-doing the change to the - OverrideFieldData object that is done during the wrap method. - """ - OverrideFieldData.provider_classes = None - self.addCleanup(cleanup_provider_classes) - # override course grading policy and make last section invisible to students override_field_for_ccx(ccx, course, 'grading_policy', { 'GRADER': [ @@ -561,9 +542,13 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): self.client.login(username=coach.username, password="test") + self.addCleanup(RequestCache.clear_request_cache) + @patch('ccx.views.render_to_response', intercept_renderer) def test_gradebook(self): self.course.enable_ccx = True + RequestCache.clear_request_cache() + url = reverse( 'ccx_gradebook', kwargs={'course_id': self.ccx_key} @@ -580,6 +565,8 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): def test_grades_csv(self): self.course.enable_ccx = True + RequestCache.clear_request_cache() + url = reverse( 'ccx_grades_csv', kwargs={'course_id': self.ccx_key} @@ -591,11 +578,11 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): response.content.strip().split('\n') ) data = dict(zip(headers, row)) + self.assertTrue('HW 04' not in data) self.assertEqual(data['HW 01'], '0.75') self.assertEqual(data['HW 02'], '0.5') self.assertEqual(data['HW 03'], '0.25') self.assertEqual(data['HW Avg'], '0.5') - self.assertTrue('HW 04' not in data) @patch('courseware.views.render_to_response', intercept_renderer) def test_student_progress(self): From abcdc05af4613b250850f959055033bd50800955 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 13 Jul 2015 11:10:57 -0400 Subject: [PATCH 6/9] Return just the CourseDescriptor in discussions get_course() Instead of calling the modulestore's get_course(), return the descriptor via self.runtime.get_block(). Doing this means that bulk ops caching works within CCX courses. Note: The long term fix for this is to have some course-level scope so that we're not peeking at the top of the tree just for some basic discussion-specific configuration information (in this case the blackout dates). TNL-2697 --- common/lib/xmodule/xmodule/discussion_module.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/discussion_module.py b/common/lib/xmodule/xmodule/discussion_module.py index ed787fb296..0d2d29b6c1 100644 --- a/common/lib/xmodule/xmodule/discussion_module.py +++ b/common/lib/xmodule/xmodule/discussion_module.py @@ -66,9 +66,11 @@ class DiscussionModule(DiscussionFields, XModule): def get_course(self): """ - Return course by course id. + Return the CourseDescriptor by course id. """ - return self.descriptor.runtime.modulestore.get_course(self.course_id) + course_key = self.location.course_key + root_course_loc = course_key.make_usage_key(u'course', u'course') + return self.runtime.get_block(root_course_loc) class DiscussionDescriptor(DiscussionFields, MetadataOnlyEditingDescriptor, RawDescriptor): From 693901529c0ec4672785b73d618eba59be55a41f Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 13 Jul 2015 17:54:01 -0400 Subject: [PATCH 7/9] Instead of calculating the course's usage key, just follow the parent chain. --- common/lib/xmodule/xmodule/discussion_module.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/discussion_module.py b/common/lib/xmodule/xmodule/discussion_module.py index 0d2d29b6c1..157f6141e2 100644 --- a/common/lib/xmodule/xmodule/discussion_module.py +++ b/common/lib/xmodule/xmodule/discussion_module.py @@ -66,11 +66,13 @@ class DiscussionModule(DiscussionFields, XModule): def get_course(self): """ - Return the CourseDescriptor by course id. + Return the CourseDescriptor at the root of the tree we're in. """ - course_key = self.location.course_key - root_course_loc = course_key.make_usage_key(u'course', u'course') - return self.runtime.get_block(root_course_loc) + block = self + while block.parent: + block = block.get_parent() + + return block class DiscussionDescriptor(DiscussionFields, MetadataOnlyEditingDescriptor, RawDescriptor): From 274e6de22bcbbf8cd4d6eeb346622037761d6e5b Mon Sep 17 00:00:00 2001 From: utkjad Date: Tue, 14 Jul 2015 20:19:04 +0000 Subject: [PATCH 8/9] PLAT-734 Fix slow transaction due to multiple calls to unpickling --- .../contentstore/views/tests/test_assets.py | 27 ++++++------------- .../xmodule/xmodule/assetstore/assetmgr.py | 20 +++++++------- 2 files changed, 17 insertions(+), 30 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_assets.py b/cms/djangoapps/contentstore/views/tests/test_assets.py index 3fc5646ac7..78ce5b4050 100644 --- a/cms/djangoapps/contentstore/views/tests/test_assets.py +++ b/cms/djangoapps/contentstore/views/tests/test_assets.py @@ -6,13 +6,12 @@ from io import BytesIO from pytz import UTC from PIL import Image import json - +from mock import patch from django.conf import settings from contentstore.tests.utils import CourseTestCase from contentstore.views import assets from contentstore.utils import reverse_course_url -from xmodule.assetstore.assetmgr import AssetMetadataFoundTemporary from xmodule.assetstore import AssetMetadata from xmodule.contentstore.content import StaticContent from xmodule.contentstore.django import contentstore @@ -331,23 +330,13 @@ class DownloadTestCase(AssetsTestCase): resp = self.client.get(url, HTTP_ACCEPT='text/html') self.assertEquals(resp.status_code, 404) - def test_metadata_found_in_modulestore(self): - # Insert asset metadata into the modulestore (with no accompanying asset). - asset_key = self.course.id.make_asset_key(AssetMetadata.GENERAL_ASSET_TYPE, 'pic1.jpg') - asset_md = AssetMetadata(asset_key, { - 'internal_name': 'EKMND332DDBK', - 'basename': 'pix/archive', - 'locked': False, - 'curr_version': '14', - 'prev_version': '13' - }) - modulestore().save_asset_metadata(asset_md, 15) - # Get the asset metadata and have it be found in the modulestore. - # Currently, no asset metadata should be found in the modulestore. The code is not yet storing it there. - # If asset metadata *is* found there, an exception is raised. This test ensures the exception is indeed raised. - # THIS IS TEMPORARY. Soon, asset metadata *will* be stored in the modulestore. - with self.assertRaises((AssetMetadataFoundTemporary, NameError)): - self.client.get(unicode(asset_key), HTTP_ACCEPT='text/html') + @patch('xmodule.modulestore.mixed.MixedModuleStore.find_asset_metadata') + def test_pickling_calls(self, patched_find_asset_metadata): + """ Tests if assets are not calling find_asset_metadata + """ + patched_find_asset_metadata.return_value = None + self.client.get(self.uploaded_url, HTTP_ACCEPT='text/html') + self.assertFalse(patched_find_asset_metadata.called) class AssetToJsonTestCase(AssetsTestCase): diff --git a/common/lib/xmodule/xmodule/assetstore/assetmgr.py b/common/lib/xmodule/xmodule/assetstore/assetmgr.py index 601e827c77..fef81b05f1 100644 --- a/common/lib/xmodule/xmodule/assetstore/assetmgr.py +++ b/common/lib/xmodule/xmodule/assetstore/assetmgr.py @@ -9,11 +9,12 @@ Handles: Phase 1: Checks to see if an asset's metadata can be found in the course's modulestore. If not found, fails over to access the asset from the contentstore. At first, the asset metadata will never be found, since saving isn't implemented yet. +Note: Hotfix (PLAT-734) No asset calls find_asset_metadata, and directly accesses from contentstore. + """ from contracts import contract, new_contract from opaque_keys.edx.keys import AssetKey -from xmodule.modulestore.django import modulestore from xmodule.contentstore.django import contentstore @@ -49,14 +50,11 @@ class AssetManager(object): @contract(asset_key='AssetKey', throw_on_not_found='bool', as_stream='bool') def find(asset_key, throw_on_not_found=True, as_stream=False): """ - Finds a course asset either in the assetstore -or- in the deprecated contentstore. + Finds course asset in the deprecated contentstore. + This method was previously searching for the course asset in the assetstore first, then in the deprecated + contentstore. However, the asset was never found in the assetstore since an asset's metadata is + not yet stored there.(removed calls to modulestore().find_asset_metadata(asset_key)) + The assetstore search was removed due to performance issues caused by each call unpickling the pickled and + compressed course structure from the structure cache. """ - content_md = modulestore().find_asset_metadata(asset_key) - - # If found, raise an exception. - if content_md: - # For now, no asset metadata should be found in the modulestore. - raise AssetMetadataFoundTemporary() - else: - # If not found, load the asset via the contentstore. - return contentstore().find(asset_key, throw_on_not_found, as_stream) + return contentstore().find(asset_key, throw_on_not_found, as_stream) From f50f045bcf92207c1ce2d1ab4cbd4446e4c7a4b1 Mon Sep 17 00:00:00 2001 From: Renzo Lucioni Date: Fri, 17 Jul 2015 14:48:36 -0400 Subject: [PATCH 9/9] Update edx-oauth2-provider to 0.5.4 Conflicts: requirements/edx/github.txt --- requirements/edx/github.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index c06889992e..d77c1dbd0a 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -44,7 +44,7 @@ git+https://github.com/hmarr/django-debug-toolbar-mongo.git@b0686a76f1ce3532088c -e git+https://github.com/edx/opaque-keys.git@27dc382ea587483b1e3889a3d19cbd90b9023a06#egg=opaque-keys git+https://github.com/edx/ease.git@release-2015-07-14#egg=ease==0.1.3 -e git+https://github.com/edx/i18n-tools.git@v0.1.1#egg=i18n-tools -git+https://github.com/edx/edx-oauth2-provider.git@0.5.2#egg=oauth2-provider==0.5.2 +git+https://github.com/edx/edx-oauth2-provider.git@0.5.4#egg=oauth2-provider==0.5.4 -e git+https://github.com/edx/edx-val.git@v0.0.5#egg=edx-val -e git+https://github.com/pmitros/RecommenderXBlock.git@518234bc354edbfc2651b9e534ddb54f96080779#egg=recommender-xblock -e git+https://github.com/edx/edx-search.git@release-2015-07-03#egg=edx-search