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/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): 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) diff --git a/common/lib/xmodule/xmodule/discussion_module.py b/common/lib/xmodule/xmodule/discussion_module.py index 0e418deed7..8ae7f9ad93 100644 --- a/common/lib/xmodule/xmodule/discussion_module.py +++ b/common/lib/xmodule/xmodule/discussion_module.py @@ -65,9 +65,13 @@ class DiscussionModule(DiscussionFields, XModule): def get_course(self): """ - Return course by course id. + Return the CourseDescriptor at the root of the tree we're in. """ - return self.descriptor.runtime.modulestore.get_course(self.course_id) + block = self + while block.parent: + block = block.get_parent() + + return block class DiscussionDescriptor(DiscussionFields, MetadataOnlyEditingDescriptor, RawDescriptor): diff --git a/lms/djangoapps/ccx/overrides.py b/lms/djangoapps/ccx/overrides.py index 4f63fc6324..462bf73735 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): @@ -78,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 @@ -117,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): @@ -149,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/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 diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index e012523c28..77247650fb 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): (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), + ('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..d473c0aafa 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. @@ -97,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 @@ -107,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_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..d9a8182017 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -16,9 +16,12 @@ 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 request_cache.middleware import RequestCache from student.roles import CourseCcxCoachRole # pylint: disable=import-error from student.tests.factories import ( # pylint: disable=import-error AdminFactory, @@ -27,6 +30,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) @@ -500,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': [ @@ -558,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} @@ -577,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} @@ -588,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): @@ -655,6 +645,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 07181b65d5..ed5d369a42 100644 --- a/lms/templates/dashboard.html +++ b/lms/templates/dashboard.html @@ -90,10 +90,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