diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 3990ab1846..8594fdbf17 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -1195,6 +1195,13 @@ class CourseEnrollment(models.Model): """ if not user.is_authenticated(): return False + + # unwrap CCXLocators so that we use the course as the access control + # source + from ccx_keys.locator import CCXLocator + if isinstance(course_key, CCXLocator): + course_key = course_key.to_course_locator() + try: record = CourseEnrollment.objects.get(user=user, course_id=course_key) return record.is_active diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index b6766109a6..2e863bc23f 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -618,14 +618,10 @@ def dashboard(request): ccx_membership_triplets = [] if settings.FEATURES.get('CUSTOM_COURSES_EDX', False): - from ccx import ACTIVE_CCX_KEY from ccx.utils import get_ccx_membership_triplets ccx_membership_triplets = get_ccx_membership_triplets( user, course_org_filter, org_filter_out_set ) - # should we deselect any active CCX at this time so that we don't have - # to change the URL for viewing a course? I think so. - request.session[ACTIVE_CCX_KEY] = None context = { 'enrollment_message': enrollment_message, diff --git a/common/lib/xmodule/xmodule/modulestore/django.py b/common/lib/xmodule/xmodule/modulestore/django.py index 8db59c8ba2..08654425b3 100644 --- a/common/lib/xmodule/xmodule/modulestore/django.py +++ b/common/lib/xmodule/xmodule/modulestore/django.py @@ -193,6 +193,15 @@ def modulestore(): settings.MODULESTORE['default'].get('OPTIONS', {}) ) + if settings.FEATURES.get('CUSTOM_COURSES_EDX'): + # TODO: This import prevents a circular import issue, but is + # symptomatic of a lib having a dependency on code in lms. This + # should be updated to have a setting that enumerates modulestore + # wrappers and then uses that setting to wrap the modulestore in + # appropriate wrappers depending on enabled features. + from ccx.modulestore import CCXModulestoreWrapper # pylint: disable=import-error + _MIXED_MODULESTORE = CCXModulestoreWrapper(_MIXED_MODULESTORE) + return _MIXED_MODULESTORE diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index 54d65aa830..dff8b0d853 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -72,6 +72,9 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): ) self.modulestore = modulestore self.course_entry = course_entry + # set course_id attribute to avoid problems with subsystems that expect + # it here. (grading, for example) + self.course_id = course_entry.course_key self.lazy = lazy self.module_data = module_data self.default_class = default_class diff --git a/lms/djangoapps/ccx/__init__.py b/lms/djangoapps/ccx/__init__.py index 3584f0006a..e69de29bb2 100644 --- a/lms/djangoapps/ccx/__init__.py +++ b/lms/djangoapps/ccx/__init__.py @@ -1,4 +0,0 @@ -""" -we use this to mark the active ccx, for use by ccx middleware and some views -""" -ACTIVE_CCX_KEY = '_ccx_id' diff --git a/lms/djangoapps/ccx/modulestore.py b/lms/djangoapps/ccx/modulestore.py new file mode 100644 index 0000000000..e4ac651cf0 --- /dev/null +++ b/lms/djangoapps/ccx/modulestore.py @@ -0,0 +1,298 @@ +# -*- coding: utf-8 -*- +"""A modulestore wrapper + +It will 'unwrap' ccx keys on the way in and re-wrap them on the way out + +In practical terms this means that when an object is retrieved from modulestore +using a CCXLocator or CCXBlockUsageLocator as the key, the equivalent +CourseLocator or BlockUsageLocator will actually be used. And all objects +returned from the modulestore will have their keys updated to be the CCX +version that was passed in. +""" +from contextlib import contextmanager +from functools import partial +from ccx_keys.locator import CCXLocator, CCXBlockUsageLocator +from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator + + +def strip_ccx(val): + """remove any reference to a CCX from the incoming value + + return a tuple of the stripped value and the id of the ccx + """ + retval = val + ccx_id = None + if isinstance(retval, CCXLocator): + ccx_id = retval.ccx + retval = retval.to_course_locator() + elif isinstance(retval, CCXBlockUsageLocator): + ccx_id = retval.course_key.ccx + retval = retval.to_block_locator() + elif hasattr(retval, 'location'): + retval.location, ccx_id = strip_ccx(retval.location) + return retval, ccx_id + + +def restore_ccx(val, ccx_id): + """restore references to a CCX to the incoming value + + returns the value converted to a CCX-aware state, using the provided ccx_id + """ + if isinstance(val, CourseLocator): + return CCXLocator.from_course_locator(val, ccx_id) + elif isinstance(val, BlockUsageLocator): + ccx_key = restore_ccx(val.course_key, ccx_id) + val = CCXBlockUsageLocator(ccx_key, val.block_type, val.block_id) + if hasattr(val, 'location'): + val.location = restore_ccx(val.location, ccx_id) + if hasattr(val, 'children'): + val.children = restore_ccx_collection(val.children, ccx_id) + return val + + +def restore_ccx_collection(field_value, ccx_id=None): + """restore references to a CCX to collections of incoming values + + returns the original collection with all values converted to a ccx-aware + state, using the provided ccx_id + """ + if ccx_id is None: + return field_value + if isinstance(field_value, list): + field_value = [restore_ccx(fv, ccx_id) for fv in field_value] + elif isinstance(field_value, dict): + for key, val in field_value.iteritems(): + field_value[key] = restore_ccx(val, ccx_id) + else: + field_value = restore_ccx(field_value, ccx_id) + return field_value + + +@contextmanager +def remove_ccx(to_strip): + """A context manager for wrapping modulestore api methods. + + yields a stripped value and a function suitable for restoring it + """ + stripped, ccx = strip_ccx(to_strip) + yield stripped, partial(restore_ccx_collection, ccx_id=ccx) + + +class CCXModulestoreWrapper(object): + """This class wraps a modulestore + + The purpose is to remove ccx-specific identifiers during lookup and restore + it after retrieval so that data can be stored local to a course, but + referenced in app context as ccx-specific + """ + + def __init__(self, modulestore): + """wrap the provided modulestore""" + self.__dict__['_modulestore'] = modulestore + + def __getattr__(self, name): + """look up missing attributes on the wrapped modulestore""" + return getattr(self._modulestore, name) + + def __setattr__(self, name, value): + """set attributes only on the wrapped modulestore""" + setattr(self._modulestore, name, value) + + def __delattr__(self, name): + """delete attributes only on the wrapped modulestore""" + delattr(self._modulestore, name) + + def _clean_locator_for_mapping(self, locator): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(locator) as (locator, restore): + # pylint: disable=protected-access + return restore( + self._modulestore._clean_locator_for_mapping(locator) + ) + + def _get_modulestore_for_courselike(self, locator=None): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + if locator is not None: + locator, _ = strip_ccx(locator) + # pylint: disable=protected-access + return self._modulestore._get_modulestore_for_courselike(locator) + + def fill_in_run(self, course_key): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.fill_in_run(course_key)) + + def has_item(self, usage_key, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + usage_key, _ = strip_ccx(usage_key) + return self._modulestore.has_item(usage_key, **kwargs) + + def get_item(self, usage_key, depth=0, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(usage_key) as (usage_key, restore): + return restore( + self._modulestore.get_item(usage_key, depth, **kwargs) + ) + + def get_items(self, course_key, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.get_items(course_key, **kwargs)) + + def get_course(self, course_key, depth=0, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.get_course( + course_key, depth=depth, **kwargs + )) + + def has_course(self, course_id, ignore_case=False, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_id) as (course_id, restore): + return restore(self._modulestore.has_course( + course_id, ignore_case=ignore_case, **kwargs + )) + + def delete_course(self, course_key, user_id): + """ + See xmodule.modulestore.__init__.ModuleStoreWrite.delete_course + """ + course_key, _ = strip_ccx(course_key) + return self._modulestore.delete_course(course_key, user_id) + + def get_parent_location(self, location, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(location) as (location, restore): + return restore( + self._modulestore.get_parent_location(location, **kwargs) + ) + + def get_block_original_usage(self, usage_key): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(usage_key) as (usage_key, restore): + orig_key, version = self._modulestore.get_block_original_usage(usage_key) + return restore(orig_key), version + + def get_modulestore_type(self, course_id): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_id) as (course_id, restore): + return restore(self._modulestore.get_modulestore_type(course_id)) + + def get_orphans(self, course_key, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.get_orphans(course_key, **kwargs)) + + def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(source_course_id) as (source_course_id, _): + with remove_ccx(dest_course_id) as (dest_course_id, dest_restore): + return dest_restore(self._modulestore.clone_course( + source_course_id, dest_course_id, user_id, fields=fields, **kwargs + )) + + def create_item(self, user_id, course_key, block_type, block_id=None, fields=None, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.create_item( + user_id, course_key, block_type, block_id=block_id, fields=fields, **kwargs + )) + + def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fields=None, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(parent_usage_key) as (parent_usage_key, restore): + return restore(self._modulestore.create_child( + user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, **kwargs + )) + + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.import_xblock( + user_id, course_key, block_type, block_id, fields=fields, runtime=runtime, **kwargs + )) + + def copy_from_template(self, source_keys, dest_key, user_id, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(dest_key) as (dest_key, restore): + return restore(self._modulestore.copy_from_template( + source_keys, dest_key, user_id, **kwargs + )) + + def update_item(self, xblock, user_id, allow_not_found=False, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(xblock) as (xblock, restore): + return restore(self._modulestore.update_item( + xblock, user_id, allow_not_found=allow_not_found, **kwargs + )) + + def delete_item(self, location, user_id, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(location) as (location, restore): + return restore( + self._modulestore.delete_item(location, user_id, **kwargs) + ) + + def revert_to_published(self, location, user_id): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(location) as (location, restore): + return restore( + self._modulestore.revert_to_published(location, user_id) + ) + + def create_xblock(self, runtime, course_key, block_type, block_id=None, fields=None, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.create_xblock( + runtime, course_key, block_type, block_id=block_id, fields=fields, **kwargs + )) + + def has_published_version(self, xblock): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(xblock) as (xblock, restore): + return restore(self._modulestore.has_published_version(xblock)) + + def publish(self, location, user_id, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(location) as (location, restore): + return restore( + self._modulestore.publish(location, user_id, **kwargs) + ) + + def unpublish(self, location, user_id, **kwargs): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(location) as (location, restore): + return restore( + self._modulestore.unpublish(location, user_id, **kwargs) + ) + + def convert_to_draft(self, location, user_id): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(location) as (location, restore): + return restore( + self._modulestore.convert_to_draft(location, user_id) + ) + + def has_changes(self, xblock): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(xblock) as (xblock, restore): + return restore(self._modulestore.has_changes(xblock)) + + def check_supports(self, course_key, method): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + course_key, _ = strip_ccx(course_key) + return self._modulestore.check_supports(course_key, method) + + @contextmanager + def branch_setting(self, branch_setting, course_id=None): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + course_id, _ = strip_ccx(course_id) + with self._modulestore.branch_setting(branch_setting, course_id): + yield + + @contextmanager + def bulk_operations(self, course_id, emit_signals=True): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + course_id, _ = strip_ccx(course_id) + with self._modulestore.bulk_operations(course_id, emit_signals=emit_signals): + yield diff --git a/lms/djangoapps/ccx/overrides.py b/lms/djangoapps/ccx/overrides.py index b9573a027f..cd90098aa3 100644 --- a/lms/djangoapps/ccx/overrides.py +++ b/lms/djangoapps/ccx/overrides.py @@ -3,16 +3,18 @@ API related to providing field overrides for individual students. This is used by the individual custom courses feature. """ import json -import threading - -from contextlib import contextmanager +import logging from django.db import transaction, IntegrityError from courseware.field_overrides import FieldOverrideProvider # pylint: disable=import-error -from ccx import ACTIVE_CCX_KEY # pylint: disable=import-error +from opaque_keys.edx.keys import CourseKey, UsageKey +from ccx_keys.locator import CCXLocator, CCXBlockUsageLocator -from .models import CcxMembership, CcxFieldOverride +from .models import CcxFieldOverride, CustomCourseForEdX + + +log = logging.getLogger(__name__) class CustomCoursesForEdxOverrideProvider(FieldOverrideProvider): @@ -25,43 +27,41 @@ class CustomCoursesForEdxOverrideProvider(FieldOverrideProvider): """ Just call the get_override_for_ccx method if there is a ccx """ - ccx = get_current_ccx() + # The incoming block might be a CourseKey instance of some type, a + # UsageKey instance of some type, or it might be something that has a + # location attribute. That location attribute will be a UsageKey + ccx = course_key = None + identifier = getattr(block, 'id', None) + if isinstance(identifier, CourseKey): + course_key = block.id + elif isinstance(identifier, UsageKey): + course_key = block.id.course_key + elif hasattr(block, 'location'): + course_key = block.location.course_key + else: + msg = "Unable to get course id when calculating ccx overide for block type %r" + log.error(msg, type(block)) + if course_key is not None: + ccx = get_current_ccx(course_key) if ccx: return get_override_for_ccx(ccx, block, name, default) return default -class _CcxContext(threading.local): +def get_current_ccx(course_key): """ - A threading local used to implement the `with_ccx` context manager, that - keeps track of the CCX currently set as the context. + Return the ccx that is active for this course. + + course_key is expected to be an instance of an opaque CourseKey, a + ValueError is raised if this expectation is not met. """ - ccx = None - request = None + if not isinstance(course_key, CourseKey): + raise ValueError("get_current_ccx requires a CourseKey instance") + if not isinstance(course_key, CCXLocator): + return None -_CCX_CONTEXT = _CcxContext() - - -@contextmanager -def ccx_context(ccx): - """ - A context manager which can be used to explicitly set the CCX that is in - play for field overrides. This mechanism overrides the standard mechanism - of looking in the user's session to see if they are enrolled in a CCX and - viewing that CCX. - """ - prev = _CCX_CONTEXT.ccx - _CCX_CONTEXT.ccx = ccx - yield - _CCX_CONTEXT.ccx = prev - - -def get_current_ccx(): - """ - Return the ccx that is active for this request. - """ - return _CCX_CONTEXT.ccx + return CustomCourseForEdX.objects.get(pk=course_key.ccx) def get_override_for_ccx(ccx, block, name, default=None): @@ -85,9 +85,14 @@ def _get_overrides_for_ccx(ccx, block): 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=block.location + location=location ) for override in query: field = block.fields[override.field] @@ -141,35 +146,3 @@ def clear_override_for_ccx(ccx, block, name): except CcxFieldOverride.DoesNotExist: pass - - -class CcxMiddleware(object): - """ - Checks to see if current session is examining a CCX and sets the CCX as - the current CCX for the override machinery if so. - """ - def process_request(self, request): - """ - Do the check. - """ - ccx_id = request.session.get(ACTIVE_CCX_KEY, None) - if ccx_id is not None: - try: - membership = CcxMembership.objects.get( - student=request.user, active=True, ccx__id__exact=ccx_id - ) - _CCX_CONTEXT.ccx = membership.ccx - except CcxMembership.DoesNotExist: - # if there is no membership, be sure to unset the active ccx - _CCX_CONTEXT.ccx = None - request.session.pop(ACTIVE_CCX_KEY) - - _CCX_CONTEXT.request = request - - def process_response(self, request, response): # pylint: disable=unused-argument - """ - Clean up afterwards. - """ - _CCX_CONTEXT.ccx = None - _CCX_CONTEXT.request = None - return response diff --git a/lms/djangoapps/ccx/tests/test_ccx_modulestore.py b/lms/djangoapps/ccx/tests/test_ccx_modulestore.py new file mode 100644 index 0000000000..8e642595b2 --- /dev/null +++ b/lms/djangoapps/ccx/tests/test_ccx_modulestore.py @@ -0,0 +1,136 @@ +""" +Test the CCXModulestoreWrapper +""" +from collections import deque +from ccx_keys.locator import CCXLocator +import datetime +from itertools import izip_longest, chain +import pytz +from student.tests.factories import AdminFactory +from xmodule.modulestore.tests.django_utils import ( + ModuleStoreTestCase, + TEST_DATA_SPLIT_MODULESTORE +) +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory + +from ..models import CustomCourseForEdX + + +class TestCCXModulestoreWrapper(ModuleStoreTestCase): + """tests for a modulestore wrapped by CCXModulestoreWrapper + """ + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + + def setUp(self): + """ + Set up tests + """ + super(TestCCXModulestoreWrapper, self).setUp() + self.course = course = CourseFactory.create() + + # Create instructor account + coach = AdminFactory.create() + + # Create a course outline + self.mooc_start = start = datetime.datetime( + 2010, 5, 12, 2, 42, tzinfo=pytz.UTC) + self.mooc_due = due = datetime.datetime( + 2010, 7, 7, 0, 0, tzinfo=pytz.UTC) + self.chapters = chapters = [ + ItemFactory.create(start=start, parent=course) for _ in xrange(2) + ] + self.sequentials = sequentials = [ + ItemFactory.create(parent=c) for _ in xrange(2) for c in chapters + ] + self.verticals = verticals = [ + ItemFactory.create( + due=due, parent=s, graded=True, format='Homework' + ) for _ in xrange(2) for s in sequentials + ] + self.blocks = [ + ItemFactory.create(parent=v) for _ in xrange(2) for v in verticals + ] + + self.ccx = ccx = CustomCourseForEdX( + course_id=course.id, + display_name='Test CCX', + coach=coach + ) + ccx.save() + + self.ccx_locator = CCXLocator.from_course_locator(course.id, ccx.id) # pylint: disable=no-member + + def get_all_children_bf(self, block): + """traverse the children of block in a breadth-first order""" + queue = deque([block]) + while queue: + item = queue.popleft() + yield item + queue.extend(item.get_children()) + + def get_course(self, key): + """get a course given a key""" + with self.store.bulk_operations(key): + course = self.store.get_course(key) + return course + + def test_get_course(self): + """retrieving a course with a ccx key works""" + expected = self.get_course(self.ccx_locator.to_course_locator()) + actual = self.get_course(self.ccx_locator) + self.assertEqual( + expected.location.course_key, + actual.location.course_key.to_course_locator()) + self.assertEqual(expected.display_name, actual.display_name) + + def test_get_children(self): + """the children of retrieved courses should be the same with course and ccx keys + """ + course_key = self.ccx_locator.to_course_locator() + course = self.get_course(course_key) + ccx = self.get_course(self.ccx_locator) + test_fodder = izip_longest( + self.get_all_children_bf(course), self.get_all_children_bf(ccx) + ) + for expected, actual in test_fodder: + if expected is None: + self.fail('course children exhausted before ccx children') + if actual is None: + self.fail('ccx children exhausted before course children') + self.assertEqual(expected.display_name, actual.display_name) + self.assertEqual(expected.location.course_key, course_key) + self.assertEqual(actual.location.course_key, self.ccx_locator) + + def test_has_item(self): + """can verify that a location exists, using ccx block usage key""" + for item in chain(self.chapters, self.sequentials, self.verticals, self.blocks): + block_key = self.ccx_locator.make_usage_key( + item.location.block_type, item.location.block_id + ) + self.assertTrue(self.store.has_item(block_key)) + + def test_get_item(self): + """can retrieve an item by a location key, using a ccx block usage key + + the retrieved item should be the same as the the one read without ccx + info + """ + for expected in chain(self.chapters, self.sequentials, self.verticals, self.blocks): + block_key = self.ccx_locator.make_usage_key( + expected.location.block_type, expected.location.block_id + ) + actual = self.store.get_item(block_key) + self.assertEqual(expected.display_name, actual.display_name) + self.assertEqual(expected.location, actual.location.to_block_locator()) + + def test_publication_api(self): + """verify that we can correctly discern a published item by ccx key""" + for expected in self.blocks: + block_key = self.ccx_locator.make_usage_key( + expected.location.block_type, expected.location.block_id + ) + self.assertTrue(self.store.has_published_version(expected)) + self.store.unpublish(block_key, self.user.id) + self.assertFalse(self.store.has_published_version(expected)) + self.store.publish(block_key, self.user.id) + self.assertTrue(self.store.has_published_version(expected)) diff --git a/lms/djangoapps/ccx/tests/test_overrides.py b/lms/djangoapps/ccx/tests/test_overrides.py index 37a7a14d36..a7a972eb25 100644 --- a/lms/djangoapps/ccx/tests/test_overrides.py +++ b/lms/djangoapps/ccx/tests/test_overrides.py @@ -9,7 +9,9 @@ from nose.plugins.attrib import attr from courseware.field_overrides import OverrideFieldData # pylint: disable=import-error from django.test.utils import override_settings from student.tests.factories import AdminFactory # pylint: disable=import-error -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.django_utils import ( + ModuleStoreTestCase, + TEST_DATA_SPLIT_MODULESTORE) from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from ..models import CustomCourseForEdX @@ -25,6 +27,8 @@ class TestFieldOverrides(ModuleStoreTestCase): """ Make sure field overrides behave in the expected manner. """ + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + def setUp(self): """ Set up tests @@ -64,7 +68,7 @@ class TestFieldOverrides(ModuleStoreTestCase): # 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 - for block in iter_blocks(course): + for block in iter_blocks(ccx.course): block._field_data = OverrideFieldData.wrap( # pylint: disable=protected-access AdminFactory.create(), block._field_data) # pylint: disable=protected-access @@ -81,7 +85,7 @@ class TestFieldOverrides(ModuleStoreTestCase): Test that overriding start date on a chapter works. """ ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC) - chapter = self.course.get_children()[0] + chapter = self.ccx.course.get_children()[0] override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) self.assertEquals(chapter.start, ccx_start) @@ -90,7 +94,7 @@ class TestFieldOverrides(ModuleStoreTestCase): Test that overriding and accessing a field produce same number of queries. """ ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC) - chapter = self.course.get_children()[0] + chapter = self.ccx.course.get_children()[0] with self.assertNumQueries(4): override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) dummy = chapter.start @@ -100,7 +104,7 @@ class TestFieldOverrides(ModuleStoreTestCase): Test no extra queries when accessing an overriden field more than once. """ ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC) - chapter = self.course.get_children()[0] + chapter = self.ccx.course.get_children()[0] with self.assertNumQueries(4): override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) dummy1 = chapter.start @@ -112,7 +116,7 @@ class TestFieldOverrides(ModuleStoreTestCase): Test that sequentials inherit overridden start date from chapter. """ ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC) - chapter = self.course.get_children()[0] + chapter = self.ccx.course.get_children()[0] override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) self.assertEquals(chapter.get_children()[0].start, ccx_start) self.assertEquals(chapter.get_children()[1].start, ccx_start) @@ -124,7 +128,7 @@ class TestFieldOverrides(ModuleStoreTestCase): the mooc. """ ccx_due = datetime.datetime(2015, 1, 1, 00, 00, tzinfo=pytz.UTC) - chapter = self.course.get_children()[0] + chapter = self.ccx.course.get_children()[0] chapter.display_name = 'itsme!' override_field_for_ccx(self.ccx, chapter, 'due', ccx_due) vertical = chapter.get_children()[0].get_children()[0] diff --git a/lms/djangoapps/ccx/tests/test_utils.py b/lms/djangoapps/ccx/tests/test_utils.py index ca1fb6ac7c..6dd8924967 100644 --- a/lms/djangoapps/ccx/tests/test_utils.py +++ b/lms/djangoapps/ccx/tests/test_utils.py @@ -16,11 +16,12 @@ from student.roles import CourseCcxCoachRole # pylint: disable=import-error from student.tests.factories import ( # pylint: disable=import-error AdminFactory, UserFactory, - CourseEnrollmentFactory, - AnonymousUserFactory, ) -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.django_utils import ( + ModuleStoreTestCase, + TEST_DATA_SPLIT_MODULESTORE) from xmodule.modulestore.tests.factories import CourseFactory +from ccx_keys.locator import CCXLocator @attr('shard_1') @@ -68,9 +69,9 @@ class TestEmailEnrollmentState(ModuleStoreTestCase): """verify behavior for non-user email address """ ee_state = self.create_one(email='nobody@nowhere.com') - for attr in ['user', 'member', 'full_name', 'in_ccx']: - value = getattr(ee_state, attr, 'missing attribute') - self.assertFalse(value, "{}: {}".format(value, attr)) + for attribute in ['user', 'member', 'full_name', 'in_ccx']: + value = getattr(ee_state, attribute, 'missing attribute') + self.assertFalse(value, "{}: {}".format(value, attribute)) def test_enrollment_state_for_non_member_user(self): """verify behavior for email address of user who is not a ccx memeber @@ -88,10 +89,10 @@ class TestEmailEnrollmentState(ModuleStoreTestCase): self.create_user() self.register_user_in_ccx() ee_state = self.create_one() - for attr in ['user', 'in_ccx']: + for attribute in ['user', 'in_ccx']: self.assertTrue( - getattr(ee_state, attr, False), - "attribute {} is missing or False".format(attr) + getattr(ee_state, attribute, False), + "attribute {} is missing or False".format(attribute) ) self.assertEqual(ee_state.member, self.user) self.assertEqual(ee_state.full_name, self.user.profile.name) @@ -128,6 +129,8 @@ class TestEmailEnrollmentState(ModuleStoreTestCase): class TestGetEmailParams(ModuleStoreTestCase): """tests for ccx.utils.get_email_params """ + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + def setUp(self): """ Set up tests @@ -157,7 +160,7 @@ class TestGetEmailParams(ModuleStoreTestCase): self.assertFalse(set(params.keys()) - set(self.all_keys)) def test_ccx_id_in_params(self): - expected_course_id = self.ccx.course_id.to_deprecated_string() + expected_course_id = unicode(CCXLocator.from_course_locator(self.ccx.course_id, self.ccx.id)) params = self.call_fut() self.assertEqual(params['course'], self.ccx) for url_key in self.url_keys: @@ -185,6 +188,8 @@ class TestGetEmailParams(ModuleStoreTestCase): class TestEnrollEmail(ModuleStoreTestCase): """tests for the enroll_email function from ccx.utils """ + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + def setUp(self): super(TestEnrollEmail, self).setUp() # unbind the user created by the parent, so we can create our own when @@ -365,6 +370,8 @@ class TestEnrollEmail(ModuleStoreTestCase): # TODO: deal with changes in behavior for auto_enroll class TestUnenrollEmail(ModuleStoreTestCase): """Tests for the unenroll_email function from ccx.utils""" + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + def setUp(self): super(TestUnenrollEmail, self).setUp() # unbind the user created by the parent, so we can create our own when @@ -511,67 +518,60 @@ class TestUnenrollEmail(ModuleStoreTestCase): @attr('shard_1') -class TestUserCCXList(ModuleStoreTestCase): - """Unit tests for ccx.utils.get_all_ccx_for_user""" +class TestGetMembershipTriplets(ModuleStoreTestCase): + """Verify that get_ccx_membership_triplets functions properly""" + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE def setUp(self): - """Create required infrastructure for tests""" - super(TestUserCCXList, self).setUp() + """Set up a course, coach, ccx and user""" + super(TestGetMembershipTriplets, self).setUp() self.course = CourseFactory.create() coach = AdminFactory.create() role = CourseCcxCoachRole(self.course.id) role.add_users(coach) self.ccx = CcxFactory(course_id=self.course.id, coach=coach) - enrollment = CourseEnrollmentFactory.create(course_id=self.course.id) - self.user = enrollment.user - self.anonymous = AnonymousUserFactory.create() - def register_user_in_ccx(self, active=False): + def make_ccx_membership(self, active=True): """create registration of self.user in self.ccx - registration will be inactive unless active=True + registration will be inactive """ - CcxMembershipFactory(ccx=self.ccx, student=self.user, active=active) + CcxMembershipFactory.create(ccx=self.ccx, student=self.user, active=active) - def get_course_title(self): - """Get course title""" - from courseware.courses import get_course_about_section # pylint: disable=import-error - return get_course_about_section(self.course, 'title') + def call_fut(self, org_filter=None, org_filter_out=()): + """call the function under test in this test case""" + from ccx.utils import get_ccx_membership_triplets + return list( + get_ccx_membership_triplets(self.user, org_filter, org_filter_out) + ) - def call_fut(self, user): - """Call function under test""" - from ccx.utils import get_all_ccx_for_user # pylint: disable=import-error - return get_all_ccx_for_user(user) + def test_no_membership(self): + """verify that no triplets are returned if there are no memberships + """ + triplets = self.call_fut() + self.assertEqual(len(triplets), 0) - def test_anonymous_sees_no_ccx(self): - memberships = self.call_fut(self.anonymous) - self.assertEqual(memberships, []) + def test_has_membership(self): + """verify that a triplet is returned when a membership exists + """ + self.make_ccx_membership() + triplets = self.call_fut() + self.assertEqual(len(triplets), 1) + ccx, membership, course = triplets[0] + self.assertEqual(ccx.id, self.ccx.id) + self.assertEqual(unicode(course.id), unicode(self.course.id)) + self.assertEqual(membership.student, self.user) - def test_unenrolled_sees_no_ccx(self): - memberships = self.call_fut(self.user) - self.assertEqual(memberships, []) + def test_has_membership_org_filtered(self): + """verify that microsite org filter prevents seeing microsite ccx""" + self.make_ccx_membership() + bad_org = self.course.location.org + 'foo' + triplets = self.call_fut(org_filter=bad_org) + self.assertEqual(len(triplets), 0) - def test_enrolled_inactive_sees_no_ccx(self): - self.register_user_in_ccx() - memberships = self.call_fut(self.user) - self.assertEqual(memberships, []) - - def test_enrolled_sees_a_ccx(self): - self.register_user_in_ccx(active=True) - memberships = self.call_fut(self.user) - self.assertEqual(len(memberships), 1) - - def test_data_structure(self): - self.register_user_in_ccx(active=True) - memberships = self.call_fut(self.user) - this_membership = memberships[0] - self.assertTrue(this_membership) - # structure contains the expected keys - for key in ['ccx_name', 'ccx_url']: - self.assertTrue(key in this_membership.keys()) - url_parts = [self.course.id.to_deprecated_string(), str(self.ccx.id)] - # all parts of the ccx url are present - for part in url_parts: - self.assertTrue(part in this_membership['ccx_url']) - actual_name = self.ccx.display_name - self.assertEqual(actual_name, this_membership['ccx_name']) + def test_has_membership_org_filtered_out(self): + """verify that microsite ccxs not seen in non-microsite view""" + self.make_ccx_membership() + filter_list = [self.course.location.org] + triplets = self.call_fut(org_filter_out=filter_list) + self.assertEqual(len(triplets), 0) diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index a217e791cc..f0b949efd1 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -6,11 +6,11 @@ import json import re import pytz import ddt -import unittest from mock import patch, MagicMock from nose.plugins.attrib import attr from capa.tests.response_xml_factory import StringResponseXMLFactory +from courseware.courses import get_course_by_id # pyline: disable=import-error from courseware.field_overrides import OverrideFieldData # pylint: disable=import-error from courseware.tests.factories import StudentModuleFactory # pylint: disable=import-error from courseware.tests.helpers import LoginEnrollmentTestCase # pylint: disable=import-error @@ -26,21 +26,22 @@ from student.tests.factories import ( # pylint: disable=import-error UserFactory, ) -from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.x_module import XModuleMixin -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.django_utils import ( + ModuleStoreTestCase, + TEST_DATA_SPLIT_MODULESTORE) from xmodule.modulestore.tests.factories import ( CourseFactory, ItemFactory, ) -import xmodule.tabs as tabs +from ccx_keys.locator import CCXLocator + from ..models import ( CustomCourseForEdX, CcxMembership, CcxFutureMembership, ) from ..overrides import get_override_for_ccx, override_field_for_ccx -from .. import ACTIVE_CCX_KEY from .factories import ( CcxFactory, CcxMembershipFactory, @@ -78,6 +79,8 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): """ Tests for Custom Courses views. """ + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + def setUp(self): """ Set up tests @@ -139,9 +142,10 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): """ User is not a coach, should get Forbidden response. """ + ccx = self.make_ccx() url = reverse( 'ccx_coach_dashboard', - kwargs={'course_id': self.course.id.to_deprecated_string()}) + kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)}) response = self.client.get(url) self.assertEqual(response.status_code, 403) @@ -182,14 +186,15 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): Get CCX schedule, modify it, save it. """ today.return_value = datetime.datetime(2014, 11, 25, tzinfo=pytz.UTC) - self.test_create_ccx() + self.make_coach() + ccx = self.make_ccx() url = reverse( 'ccx_coach_dashboard', - kwargs={'course_id': self.course.id.to_deprecated_string()}) + kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)}) response = self.client.get(url) schedule = json.loads(response.mako_context['schedule']) # pylint: disable=no-member self.assertEqual(len(schedule), 2) - self.assertEqual(schedule[0]['hidden'], True) + self.assertEqual(schedule[0]['hidden'], False) self.assertEqual(schedule[0]['start'], None) self.assertEqual(schedule[0]['children'][0]['start'], None) self.assertEqual(schedule[0]['due'], None) @@ -200,7 +205,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): url = reverse( 'save_ccx', - kwargs={'course_id': self.course.id.to_deprecated_string()}) + kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)}) def unhide(unit): """ @@ -235,7 +240,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): policy = get_override_for_ccx(ccx, self.course, 'grading_policy', self.course.grading_policy) self.assertEqual(policy['GRADER'][0]['type'], 'Homework') - self.assertEqual(policy['GRADER'][0]['min_count'], 4) + self.assertEqual(policy['GRADER'][0]['min_count'], 8) self.assertEqual(policy['GRADER'][1]['type'], 'Lab') self.assertEqual(policy['GRADER'][1]['min_count'], 0) self.assertEqual(policy['GRADER'][2]['type'], 'Midterm Exam') @@ -255,7 +260,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): url = reverse( 'ccx_invite', - kwargs={'course_id': self.course.id.to_deprecated_string()} + kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)} ) data = { 'enrollment-button': 'Enroll', @@ -288,7 +293,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): url = reverse( 'ccx_invite', - kwargs={'course_id': self.course.id.to_deprecated_string()} + kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)} ) data = { 'enrollment-button': 'Unenroll', @@ -318,7 +323,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): url = reverse( 'ccx_invite', - kwargs={'course_id': self.course.id.to_deprecated_string()} + kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)} ) data = { 'enrollment-button': 'Enroll', @@ -350,7 +355,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): url = reverse( 'ccx_invite', - kwargs={'course_id': self.course.id.to_deprecated_string()} + kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)} ) data = { 'enrollment-button': 'Unenroll', @@ -383,7 +388,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): url = reverse( 'ccx_manage_student', - kwargs={'course_id': self.course.id.to_deprecated_string()} + kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)} ) data = { 'student-action': 'add', @@ -414,7 +419,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): url = reverse( 'ccx_manage_student', - kwargs={'course_id': self.course.id.to_deprecated_string()} + kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)} ) data = { 'student-action': 'revoke', @@ -435,9 +440,10 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): GET_CHILDREN = XModuleMixin.get_children -def patched_get_children(self, usage_key_filter=None): # pylint: disable=missing-docstring - def iter_children(): # pylint: disable=missing-docstring - print self.__dict__ +def patched_get_children(self, usage_key_filter=None): + """Emulate system tools that mask courseware not visible to students""" + def iter_children(): + """skip children not visible to students""" for child in GET_CHILDREN(self, usage_key_filter=usage_key_filter): child._field_data_cache = {} # pylint: disable=protected-access if not child.visible_to_staff_only: @@ -453,16 +459,14 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): """ Tests for Custom Courses views. """ + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + def setUp(self): """ Set up tests """ super(TestCCXGrades, self).setUp() - self.course = course = CourseFactory.create() - - # Create instructor account - self.coach = coach = AdminFactory.create() - self.client.login(username=coach.username, password="test") + course = CourseFactory.create() # Create a course outline self.mooc_start = start = datetime.datetime( @@ -475,31 +479,25 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): category="sequential", metadata={'graded': True, 'format': 'Homework'}) for _ in xrange(4)] - - role = CourseCcxCoachRole(self.course.id) - role.add_users(coach) - self.ccx = ccx = CcxFactory(course_id=self.course.id, coach=self.coach) - - self.student = student = UserFactory.create() - CourseEnrollmentFactory.create(user=student, course_id=self.course.id) - CcxMembershipFactory(ccx=ccx, student=student, active=True) - - for i, section in enumerate(sections): - for j in xrange(4): - item = ItemFactory.create( + # pylint: disable=unused-variable + problems = [ + [ + ItemFactory.create( parent=section, category="problem", data=StringResponseXMLFactory().build_xml(answer='foo'), metadata={'rerandomize': 'always'} - ) + ) for _ in xrange(4) + ] for section in sections + ] - StudentModuleFactory.create( - grade=1 if i < j else 0, - max_grade=1, - student=student, - course_id=self.course.id, - module_state_key=item.location - ) + # Create instructor account + self.coach = coach = AdminFactory.create() + + # Create CCX + role = CourseCcxCoachRole(course.id) + 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 @@ -521,11 +519,7 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): OverrideFieldData.provider_classes = None self.addCleanup(cleanup_provider_classes) - patch_context = patch('ccx.views.get_course_by_id') - get_course = patch_context.start() - get_course.return_value = course - self.addCleanup(patch_context.stop) - + # override course grading policy and make last section invisible to students override_field_for_ccx(ccx, course, 'grading_policy', { 'GRADER': [ {'drop_count': 0, @@ -539,11 +533,35 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): override_field_for_ccx( ccx, sections[-1], 'visible_to_staff_only', True) + # create a ccx locator and retrieve the course structure using that key + # which emulates how a student would get access. + self.ccx_key = CCXLocator.from_course_locator(course.id, ccx.id) + self.course = get_course_by_id(self.ccx_key) + + self.student = student = UserFactory.create() + CourseEnrollmentFactory.create(user=student, course_id=self.course.id) + CcxMembershipFactory(ccx=ccx, student=student, active=True) + + # create grades for self.student as if they'd submitted the ccx + for chapter in self.course.get_children(): + for i, section in enumerate(chapter.get_children()): + for j, problem in enumerate(section.get_children()): + # if not problem.visible_to_staff_only: + StudentModuleFactory.create( + grade=1 if i < j else 0, + max_grade=1, + student=self.student, + course_id=self.course.id, + module_state_key=problem.location + ) + + self.client.login(username=coach.username, password="test") + @patch('ccx.views.render_to_response', intercept_renderer) def test_gradebook(self): url = reverse( 'ccx_gradebook', - kwargs={'course_id': self.course.id.to_deprecated_string()} + kwargs={'course_id': self.ccx_key} ) response = self.client.get(url) self.assertEqual(response.status_code, 200) @@ -558,7 +576,7 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): def test_grades_csv(self): url = reverse( 'ccx_grades_csv', - kwargs={'course_id': self.course.id.to_deprecated_string()} + kwargs={'course_id': self.ccx_key} ) response = self.client.get(url) self.assertEqual(response.status_code, 200) @@ -581,13 +599,9 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): self.addCleanup(patch_context.stop) self.client.login(username=self.student.username, password="test") - session = self.client.session - session[ACTIVE_CCX_KEY] = self.ccx.id # pylint: disable=no-member - session.save() - self.client.session.get(ACTIVE_CCX_KEY) url = reverse( 'progress', - kwargs={'course_id': self.course.id.to_deprecated_string()} + kwargs={'course_id': self.ccx_key} ) response = self.client.get(url) self.assertEqual(response.status_code, 200) @@ -597,182 +611,6 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): self.assertEqual(len(grades['section_breakdown']), 4) -@attr('shard_1') -class TestSwitchActiveCCX(ModuleStoreTestCase, LoginEnrollmentTestCase): - """Verify the view for switching which CCX is active, if any - """ - def setUp(self): - super(TestSwitchActiveCCX, self).setUp() - self.course = course = CourseFactory.create() - coach = AdminFactory.create() - role = CourseCcxCoachRole(course.id) - role.add_users(coach) - self.ccx = CcxFactory(course_id=course.id, coach=coach) - enrollment = CourseEnrollmentFactory.create(course_id=course.id) - self.user = enrollment.user - self.target_url = reverse( - 'course_root', args=[course.id.to_deprecated_string()] - ) - - def register_user_in_ccx(self, active=False): - """create registration of self.user in self.ccx - - registration will be inactive unless active=True - """ - CcxMembershipFactory(ccx=self.ccx, student=self.user, active=active) - - def revoke_ccx_registration(self): - """ - delete membership - """ - membership = CcxMembership.objects.filter( - ccx=self.ccx, student=self.user - ) - membership.delete() - - def verify_active_ccx(self, request, id=None): # pylint: disable=redefined-builtin, invalid-name - """verify that we have the correct active ccx""" - if id: - id = str(id) - self.assertEqual(id, request.session.get(ACTIVE_CCX_KEY, None)) - - def test_unauthorized_cannot_switch_to_ccx(self): - switch_url = reverse( - 'switch_active_ccx', - args=[self.course.id.to_deprecated_string(), self.ccx.id] - ) - response = self.client.get(switch_url) - self.assertEqual(response.status_code, 302) - - def test_unauthorized_cannot_switch_to_mooc(self): - switch_url = reverse( - 'switch_active_ccx', - args=[self.course.id.to_deprecated_string()] - ) - response = self.client.get(switch_url) - self.assertEqual(response.status_code, 302) - - def test_enrolled_inactive_user_cannot_select_ccx(self): - self.register_user_in_ccx(active=False) - self.client.login(username=self.user.username, password="test") - switch_url = reverse( - 'switch_active_ccx', - args=[self.course.id.to_deprecated_string(), self.ccx.id] - ) - response = self.client.get(switch_url) - self.assertEqual(response.status_code, 302) - self.assertTrue(response.get('Location', '').endswith(self.target_url)) # pylint: disable=no-member - # if the ccx were active, we'd need to pass the ID of the ccx here. - self.verify_active_ccx(self.client) - - def test_enrolled_user_can_select_ccx(self): - self.register_user_in_ccx(active=True) - self.client.login(username=self.user.username, password="test") - switch_url = reverse( - 'switch_active_ccx', - args=[self.course.id.to_deprecated_string(), self.ccx.id] - ) - response = self.client.get(switch_url) - self.assertEqual(response.status_code, 302) - self.assertTrue(response.get('Location', '').endswith(self.target_url)) # pylint: disable=no-member - self.verify_active_ccx(self.client, self.ccx.id) - - def test_enrolled_user_can_select_mooc(self): - self.register_user_in_ccx(active=True) - self.client.login(username=self.user.username, password="test") - # pre-seed the session with the ccx id - session = self.client.session - session[ACTIVE_CCX_KEY] = str(self.ccx.id) - session.save() - switch_url = reverse( - 'switch_active_ccx', - args=[self.course.id.to_deprecated_string()] - ) - response = self.client.get(switch_url) - self.assertEqual(response.status_code, 302) - self.assertTrue(response.get('Location', '').endswith(self.target_url)) # pylint: disable=no-member - self.verify_active_ccx(self.client) - - def test_unenrolled_user_cannot_select_ccx(self): - self.client.login(username=self.user.username, password="test") - switch_url = reverse( - 'switch_active_ccx', - args=[self.course.id.to_deprecated_string(), self.ccx.id] - ) - response = self.client.get(switch_url) - self.assertEqual(response.status_code, 302) - self.assertTrue(response.get('Location', '').endswith(self.target_url)) # pylint: disable=no-member - # if the ccx were active, we'd need to pass the ID of the ccx here. - self.verify_active_ccx(self.client) - - def test_unenrolled_user_switched_to_mooc(self): - self.client.login(username=self.user.username, password="test") - # pre-seed the session with the ccx id - session = self.client.session - session[ACTIVE_CCX_KEY] = str(self.ccx.id) - session.save() - switch_url = reverse( - 'switch_active_ccx', - args=[self.course.id.to_deprecated_string(), self.ccx.id] - ) - response = self.client.get(switch_url) - self.assertEqual(response.status_code, 302) - self.assertTrue(response.get('Location', '').endswith(self.target_url)) # pylint: disable=no-member - # we tried to select the ccx but are not registered, so we are switched - # back to the mooc view - self.verify_active_ccx(self.client) - - def test_unassociated_course_and_ccx_not_selected(self): - new_course = CourseFactory.create() - self.client.login(username=self.user.username, password="test") - expected_url = reverse( - 'course_root', args=[new_course.id.to_deprecated_string()] - ) - # the ccx and the course are not related. - switch_url = reverse( - 'switch_active_ccx', - args=[new_course.id.to_deprecated_string(), self.ccx.id] - ) - response = self.client.get(switch_url) - self.assertEqual(response.status_code, 302) - self.assertTrue(response.get('Location', '').endswith(expected_url)) # pylint: disable=no-member - # the mooc should be active - self.verify_active_ccx(self.client) - - def test_missing_ccx_cannot_be_selected(self): - self.register_user_in_ccx() - self.client.login(username=self.user.username, password="test") - switch_url = reverse( - 'switch_active_ccx', - args=[self.course.id.to_deprecated_string(), self.ccx.id] - ) - # delete the ccx - self.ccx.delete() # pylint: disable=no-member - - response = self.client.get(switch_url) - self.assertEqual(response.status_code, 302) - self.assertTrue(response.get('Location', '').endswith(self.target_url)) # pylint: disable=no-member - # we tried to select the ccx it doesn't exist anymore, so we are - # switched back to the mooc view - self.verify_active_ccx(self.client) - - def test_revoking_ccx_membership_revokes_active_ccx(self): - self.register_user_in_ccx(active=True) - self.client.login(username=self.user.username, password="test") - # ensure ccx is active in the request session - switch_url = reverse( - 'switch_active_ccx', - args=[self.course.id.to_deprecated_string(), self.ccx.id] - ) - self.client.get(switch_url) - self.verify_active_ccx(self.client, self.ccx.id) - # unenroll the user from the ccx - self.revoke_ccx_registration() - # request the course root and verify that the ccx is not active - self.client.get(self.target_url) - self.verify_active_ccx(self.client) - - @ddt.ddt class CCXCoachTabTestCase(ModuleStoreTestCase): """ diff --git a/lms/djangoapps/ccx/urls.py b/lms/djangoapps/ccx/urls.py index fcf0342f98..9a2be83e7e 100644 --- a/lms/djangoapps/ccx/urls.py +++ b/lms/djangoapps/ccx/urls.py @@ -24,6 +24,4 @@ urlpatterns = patterns( 'ccx.views.ccx_grades_csv', name='ccx_grades_csv'), url(r'^ccx_set_grading_policy$', 'ccx.views.set_grading_policy', name='ccx_set_grading_policy'), - url(r'^switch_ccx(?:/(?P[\d]+))?$', - 'ccx.views.switch_active_ccx', name='switch_active_ccx'), ) diff --git a/lms/djangoapps/ccx/utils.py b/lms/djangoapps/ccx/utils.py index 3ebf9e4d0b..7d493c4b93 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/lms/djangoapps/ccx/utils.py @@ -4,8 +4,6 @@ CCX Enrollment operations for use by Coach APIs. Does not include any access control, be sure to check access before calling. """ import logging -from courseware.courses import get_course_about_section # pylint: disable=import-error -from courseware.courses import get_course_by_id # pylint: disable=import-error from django.contrib.auth.models import User from django.conf import settings from django.core.urlresolvers import reverse @@ -14,12 +12,12 @@ from edxmako.shortcuts import render_to_string # pylint: disable=import-error from microsite_configuration import microsite # pylint: disable=import-error from xmodule.modulestore.django import modulestore from xmodule.error_module import ErrorDescriptor +from ccx_keys.locator import CCXLocator from .models import ( CcxMembership, CcxFutureMembership, ) -from .overrides import get_current_ccx log = logging.getLogger("edx.ccx") @@ -138,7 +136,6 @@ def get_email_params(ccx, auto_enroll, secure=True): get parameters for enrollment emails """ protocol = 'https' if secure else 'http' - course_id = ccx.course_id stripped_site_name = microsite.get_value( 'SITE_NAME', @@ -154,7 +151,7 @@ def get_email_params(ccx, auto_enroll, secure=True): site=stripped_site_name, path=reverse( 'course_root', - kwargs={'course_id': course_id.to_deprecated_string()} + kwargs={'course_id': CCXLocator.from_course_locator(ccx.course_id, ccx.id)} ) ) @@ -165,7 +162,7 @@ def get_email_params(ccx, auto_enroll, secure=True): site=stripped_site_name, path=reverse( 'about_course', - kwargs={'course_id': course_id.to_deprecated_string()} + kwargs={'course_id': CCXLocator.from_course_locator(ccx.course_id, ccx.id)} ) ) @@ -241,44 +238,6 @@ def send_mail_to_student(student, param_dict): ) -def get_all_ccx_for_user(user): - """return all CCXS to which the user is registered - - Returns a list of dicts: { - ccx_name: - ccx_url: - ccx_active: True if this ccx is currently the 'active' one - mooc_name: - mooc_url: - } - """ - if user.is_anonymous(): - return [] - current_active_ccx = get_current_ccx() - memberships = [] - for membership in CcxMembership.memberships_for_user(user): - course = get_course_by_id(membership.ccx.course_id) - ccx = membership.ccx - ccx_title = ccx.display_name - mooc_title = get_course_about_section(course, 'title') - url = reverse( - 'switch_active_ccx', - args=[course.id.to_deprecated_string(), membership.ccx.id] - ) - mooc_url = reverse( - 'switch_active_ccx', - args=[course.id.to_deprecated_string(), ] - ) - memberships.append({ - 'ccx_name': ccx_title, - 'ccx_url': url, - 'active': membership.ccx == current_active_ccx, - 'mooc_name': mooc_title, - 'mooc_url': mooc_url, - }) - return memberships - - def get_ccx_membership_triplets(user, course_org_filter, org_filter_out_set): """ Get the relevant set of (CustomCourseForEdX, CcxMembership, Course) @@ -300,6 +259,17 @@ def get_ccx_membership_triplets(user, course_org_filter, org_filter_out_set): 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 + yield (ccx, membership, course) else: log.error("User {0} enrolled in {2} course {1}".format( # pylint: disable=logging-format-interpolation diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index d804bc4438..1cd845e24d 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -8,6 +8,7 @@ import json import logging import pytz +from contextlib import contextmanager from copy import deepcopy from cStringIO import StringIO @@ -17,8 +18,10 @@ from django.http import ( HttpResponseForbidden, HttpResponseRedirect, ) +from django.contrib import messages from django.core.exceptions import ValidationError from django.core.validators import validate_email +from django.http import Http404 from django.shortcuts import redirect from django.utils.translation import ugettext as _ from django.views.decorators.cache import cache_control @@ -34,6 +37,7 @@ from courseware.model_data import FieldDataCache # pylint: disable=import-error from courseware.module_render import get_module_for_descriptor # pylint: disable=import-error from edxmako.shortcuts import render_to_response # pylint: disable=import-error from opaque_keys.edx.keys import CourseKey +from ccx_keys.locator import CCXLocator from student.roles import CourseCcxCoachRole # pylint: disable=import-error from instructor.offline_gradecalc import student_grades # pylint: disable=import-error @@ -45,13 +49,11 @@ from .overrides import ( clear_override_for_ccx, get_override_for_ccx, override_field_for_ccx, - ccx_context, ) from .utils import ( enroll_email, unenroll_email, ) -from ccx import ACTIVE_CCX_KEY # pylint: disable=import-error log = logging.getLogger(__name__) @@ -71,43 +73,70 @@ def coach_dashboard(view): and modifying the view's call signature. """ course_key = CourseKey.from_string(course_id) + ccx = None + if isinstance(course_key, CCXLocator): + ccx_id = course_key.ccx + ccx = CustomCourseForEdX.objects.get(pk=ccx_id) + course_key = ccx.course_id + role = CourseCcxCoachRole(course_key) if not role.has_user(request.user): return HttpResponseForbidden( _('You must be a CCX Coach to access this view.')) + course = get_course_by_id(course_key, depth=None) - return view(request, course) + + # if there is a ccx, we must validate that it is the ccx for this coach + if ccx is not None: + coach_ccx = get_ccx_for_coach(course, request.user) + if coach_ccx is None or coach_ccx.id != ccx.id: + return HttpResponseForbidden( + _('You must be the coach for this ccx to access this view') + ) + + return view(request, course, ccx) return wrapper @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @coach_dashboard -def dashboard(request, course): +def dashboard(request, course, ccx=None): """ Display the CCX Coach Dashboard. """ - ccx = get_ccx_for_coach(course, request.user) + # right now, we can only have one ccx per user and course + # so, if no ccx is passed in, we can sefely redirect to that + if ccx is None: + ccx = get_ccx_for_coach(course, request.user) + if ccx: + url = reverse( + 'ccx_coach_dashboard', + kwargs={'course_id': CCXLocator.from_course_locator(course.id, ccx.id)} + ) + return redirect(url) + context = { 'course': course, 'ccx': ccx, } if ccx: + ccx_locator = CCXLocator.from_course_locator(course.id, ccx.id) schedule = get_ccx_schedule(course, ccx) grading_policy = get_override_for_ccx( ccx, course, 'grading_policy', course.grading_policy) context['schedule'] = json.dumps(schedule, indent=4) context['save_url'] = reverse( - 'save_ccx', kwargs={'course_id': course.id}) + 'save_ccx', kwargs={'course_id': ccx_locator}) context['ccx_members'] = CcxMembership.objects.filter(ccx=ccx) context['gradebook_url'] = reverse( - 'ccx_gradebook', kwargs={'course_id': course.id}) + 'ccx_gradebook', kwargs={'course_id': ccx_locator}) context['grades_csv_url'] = reverse( - 'ccx_grades_csv', kwargs={'course_id': course.id}) + 'ccx_grades_csv', kwargs={'course_id': ccx_locator}) context['grading_policy'] = json.dumps(grading_policy, indent=4) context['grading_policy_url'] = reverse( - 'ccx_set_grading_policy', kwargs={'course_id': course.id}) + 'ccx_set_grading_policy', kwargs={'course_id': ccx_locator}) else: context['create_ccx_url'] = reverse( 'create_ccx', kwargs={'course_id': course.id}) @@ -117,11 +146,21 @@ def dashboard(request, course): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @coach_dashboard -def create_ccx(request, course): +def create_ccx(request, course, ccx=None): """ Create a new CCX """ name = request.POST.get('name') + + # prevent CCX objects from being created for deprecated course ids. + if course.id.deprecated: + messages.error(request, _( + "You cannot create a CCX from a course using a deprecated id. " + "Please create a rerun of this course in the studio to allow " + "this action.")) + url = reverse('ccx_coach_dashboard', kwargs={'course_id', course.id}) + return redirect(url) + ccx = CustomCourseForEdX( course_id=course.id, coach=request.user, @@ -142,18 +181,20 @@ def create_ccx(request, course): for vertical in sequential.get_children(): override_field_for_ccx(ccx, vertical, hidden, True) - url = reverse('ccx_coach_dashboard', kwargs={'course_id': course.id}) + ccx_id = CCXLocator.from_course_locator(course.id, ccx.id) # pylint: disable=no-member + url = reverse('ccx_coach_dashboard', kwargs={'course_id': ccx_id}) return redirect(url) @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @coach_dashboard -def save_ccx(request, course): +def save_ccx(request, course, ccx=None): """ Save changes to CCX. """ - ccx = get_ccx_for_coach(course, request.user) + if not ccx: + raise Http404 def override_fields(parent, data, graded, earliest=None): """ @@ -220,15 +261,20 @@ def save_ccx(request, course): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @coach_dashboard -def set_grading_policy(request, course): +def set_grading_policy(request, course, ccx=None): """ Set grading policy for the CCX. """ - ccx = get_ccx_for_coach(course, request.user) + if not ccx: + raise Http404 + override_field_for_ccx( ccx, course, 'grading_policy', json.loads(request.POST['policy'])) - url = reverse('ccx_coach_dashboard', kwargs={'course_id': course.id}) + url = reverse( + 'ccx_coach_dashboard', + kwargs={'course_id': CCXLocator.from_course_locator(course.id, ccx.id)} + ) return redirect(url) @@ -271,12 +317,15 @@ def get_ccx_for_coach(course, coach): Looks to see if user is coach of a CCX for this course. Returns the CCX or None. """ - try: - return CustomCourseForEdX.objects.get( - course_id=course.id, - coach=coach) - except CustomCourseForEdX.DoesNotExist: - return None + ccxs = CustomCourseForEdX.objects.filter( + course_id=course.id, + coach=coach + ) + # XXX: In the future, it would be nice to support more than one ccx per + # coach per course. This is a place where that might happen. + if ccxs.exists(): + return ccxs[0] + return None def get_ccx_schedule(course, ccx): @@ -322,11 +371,13 @@ def get_ccx_schedule(course, ccx): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @coach_dashboard -def ccx_schedule(request, course): +def ccx_schedule(request, course, ccx=None): # pylint: disable=unused-argument """ get json representation of ccx schedule """ - ccx = get_ccx_for_coach(course, request.user) + if not ccx: + raise Http404 + schedule = get_ccx_schedule(course, ccx) json_schedule = json.dumps(schedule, indent=4) return HttpResponse(json_schedule, mimetype='application/json') @@ -335,11 +386,13 @@ def ccx_schedule(request, course): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @coach_dashboard -def ccx_invite(request, course): +def ccx_invite(request, course, ccx=None): """ Invite users to new ccx """ - ccx = get_ccx_for_coach(course, request.user) + if not ccx: + raise Http404 + action = request.POST.get('enrollment-button') identifiers_raw = request.POST.get('student-ids') identifiers = _split_input_list(identifiers_raw) @@ -367,17 +420,22 @@ def ccx_invite(request, course): unenroll_email(ccx, email, email_students=email_students) except ValidationError: log.info('Invalid user name or email when trying to invite students: %s', email) - url = reverse('ccx_coach_dashboard', kwargs={'course_id': course.id}) + url = reverse( + 'ccx_coach_dashboard', + kwargs={'course_id': CCXLocator.from_course_locator(course.id, ccx.id)} + ) return redirect(url) @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @coach_dashboard -def ccx_student_management(request, course): +def ccx_student_management(request, course, ccx=None): """Manage the enrollment of individual students in a CCX """ - ccx = get_ccx_for_coach(course, request.user) + if not ccx: + raise Http404 + action = request.POST.get('student-action', None) student_id = request.POST.get('student-id', '') user = email = None @@ -399,28 +457,44 @@ def ccx_student_management(request, course): except ValidationError: log.info('Invalid user name or email when trying to enroll student: %s', email) - url = reverse('ccx_coach_dashboard', kwargs={'course_id': course.id}) + url = reverse( + 'ccx_coach_dashboard', + kwargs={'course_id': CCXLocator.from_course_locator(course.id, ccx.id)} + ) return redirect(url) -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@coach_dashboard -def ccx_gradebook(request, course): +@contextmanager +def ccx_course(ccx_locator): + """Create a context in which the course identified by course_locator exists """ - Show the gradebook for this CCX. - """ - # Need course module for overrides to function properly + course = get_course_by_id(ccx_locator) + yield course + + +def prep_course_for_grading(course, request): + """Set up course module for overrides to function properly""" field_data_cache = FieldDataCache.cache_for_descriptor_descendents( course.id, request.user, course, depth=2) course = get_module_for_descriptor( request.user, request, course, field_data_cache, course.id) - ccx = get_ccx_for_coach(course, request.user) - with ccx_context(ccx): - # The grading policy for the MOOC is probably already cached. We need - # to make sure we have the CCX grading policy loaded. - course._field_data_cache = {} # pylint: disable=protected-access - course.set_grading_policy(course.grading_policy) + course._field_data_cache = {} # pylint: disable=protected-access + course.set_grading_policy(course.grading_policy) + + +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +@coach_dashboard +def ccx_gradebook(request, course, ccx=None): + """ + Show the gradebook for this CCX. + """ + if not ccx: + raise Http404 + + ccx_key = CCXLocator.from_course_locator(course.id, ccx.id) + with ccx_course(ccx_key) as course: + prep_course_for_grading(course, request) enrolled_students = User.objects.filter( ccxmembership__ccx=ccx, @@ -450,21 +524,16 @@ def ccx_gradebook(request, course): @cache_control(no_cache=True, no_store=True, must_revalidate=True) @coach_dashboard -def ccx_grades_csv(request, course): +def ccx_grades_csv(request, course, ccx=None): """ Download grades as CSV. """ - # Need course module for overrides to function properly - field_data_cache = FieldDataCache.cache_for_descriptor_descendents( - course.id, request.user, course, depth=2) - course = get_module_for_descriptor( - request.user, request, course, field_data_cache, course.id) - ccx = get_ccx_for_coach(course, request.user) - with ccx_context(ccx): - # The grading policy for the MOOC is probably already cached. We need - # to make sure we have the CCX grading policy loaded. - course._field_data_cache = {} # pylint: disable=protected-access - course.set_grading_policy(course.grading_policy) + if not ccx: + raise Http404 + + ccx_key = CCXLocator.from_course_locator(course.id, ccx.id) + with ccx_course(ccx_key) as course: + prep_course_for_grading(course, request) enrolled_students = User.objects.filter( ccxmembership__ccx=ccx, @@ -501,33 +570,3 @@ def ccx_grades_csv(request, course): writer.writerow(row) return HttpResponse(buf.getvalue(), content_type='text/plain') - - -@login_required -def switch_active_ccx(request, course_id, ccx_id=None): - """set the active CCX for the logged-in user - """ - course_key = CourseKey.from_string(course_id) - # will raise Http404 if course_id is bad - course = get_course_by_id(course_key) - course_url = reverse( - 'course_root', args=[course.id.to_deprecated_string()] - ) - if ccx_id is not None: - try: - requested_ccx = CustomCourseForEdX.objects.get(pk=ccx_id) - assert unicode(requested_ccx.course_id) == course_id - if not CcxMembership.objects.filter( - ccx=requested_ccx, student=request.user, active=True - ).exists(): - ccx_id = None - except CustomCourseForEdX.DoesNotExist: - # what to do here? Log the failure? Do we care? - ccx_id = None - except AssertionError: - # what to do here? Log the failure? Do we care? - ccx_id = None - - request.session[ACTIVE_CCX_KEY] = ccx_id - - return HttpResponseRedirect(course_url) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 372cc04c2b..3fa2cda60c 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -43,6 +43,7 @@ from util.milestones_helpers import ( get_pre_requisite_courses_not_completed, any_unfulfilled_milestones, ) +from ccx_keys.locator import CCXLocator import dogstats_wrapper as dog_stats_api @@ -91,6 +92,9 @@ def has_access(user, action, obj, course_key=None): if not user: user = AnonymousUser() + if isinstance(course_key, CCXLocator): + course_key = course_key.to_course_locator() + # delegate the work to type-specific functions. # (start with more specific types, then get more general) if isinstance(obj, CourseDescriptor): @@ -106,6 +110,9 @@ def has_access(user, action, obj, course_key=None): if isinstance(obj, XBlock): return _has_access_descriptor(user, action, obj, course_key) + if isinstance(obj, CCXLocator): + return _has_access_ccx_key(user, action, obj) + if isinstance(obj, CourseKey): return _has_access_course_key(user, action, obj) @@ -488,6 +495,16 @@ def _has_access_course_key(user, action, course_key): return _dispatch(checkers, action, user, course_key) +def _has_access_ccx_key(user, action, ccx_key): + """Check if user has access to the course for this ccx_key + + Delegates checking to _has_access_course_key + Valid actions: same as for that function + """ + course_key = ccx_key.to_course_locator() + return _has_access_course_key(user, action, course_key) + + def _has_access_string(user, action, perm): """ Check if user has certain special access, specified as string. Valid strings: diff --git a/lms/djangoapps/dashboard/sysadmin.py b/lms/djangoapps/dashboard/sysadmin.py index 2b656cecbd..d65fd7dd5b 100644 --- a/lms/djangoapps/dashboard/sysadmin.py +++ b/lms/djangoapps/dashboard/sysadmin.py @@ -41,7 +41,6 @@ from student.models import CourseEnrollment, UserProfile, Registration import track.views from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore -from xmodule.modulestore.xml import XMLModuleStore from opaque_keys.edx.locations import SlashSeparatedCourseKey @@ -60,8 +59,9 @@ class SysadminDashboardView(TemplateView): """ self.def_ms = modulestore() + self.is_using_mongo = True - if isinstance(self.def_ms, XMLModuleStore): + if self.def_ms.get_modulestore_type(None) == 'xml': self.is_using_mongo = False self.msg = u'' self.datatable = [] diff --git a/lms/djangoapps/dashboard/tests/test_sysadmin.py b/lms/djangoapps/dashboard/tests/test_sysadmin.py index 735ffb29f6..7f08e35906 100644 --- a/lms/djangoapps/dashboard/tests/test_sysadmin.py +++ b/lms/djangoapps/dashboard/tests/test_sysadmin.py @@ -32,7 +32,6 @@ from student.tests.factories import UserFactory from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST -from xmodule.modulestore.xml import XMLModuleStore TEST_MONGODB_LOG = { @@ -316,7 +315,8 @@ class TestSysadmin(SysadminBaseTestCase): response = self._add_edx4edx() def_ms = modulestore() - self.assertIn('xml', str(def_ms.__class__)) + + self.assertEqual('xml', def_ms.get_modulestore_type(None)) course = def_ms.courses.get('{0}/edx4edx_lite'.format( os.path.abspath(settings.DATA_DIR)), None) self.assertIsNotNone(course) @@ -460,7 +460,7 @@ class TestSysAdminMongoCourseImport(SysadminBaseTestCase): self._mkdir(getattr(settings, 'GIT_REPO_DIR')) def_ms = modulestore() - self.assertFalse(isinstance(def_ms, XMLModuleStore)) + self.assertFalse('xml' == def_ms.get_modulestore_type(None)) self._add_edx4edx() course = def_ms.get_course(SlashSeparatedCourseKey('MITx', 'edx4edx', 'edx4edx')) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 2cad01d63b..1aa4df0ac3 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -65,7 +65,7 @@ class DiscussionTab(EnrolledTab): return False if settings.FEATURES.get('CUSTOM_COURSES_EDX', False): - if get_current_ccx(): + if get_current_ccx(course.id): return False return settings.FEATURES.get('ENABLE_DISCUSSION_SERVICE') diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 06e9febead..fc1a9770b1 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -612,7 +612,6 @@ ECOMMERCE_API_TIMEOUT = ENV_TOKENS.get('ECOMMERCE_API_TIMEOUT', ECOMMERCE_API_TI ##### Custom Courses for EdX ##### if FEATURES.get('CUSTOM_COURSES_EDX'): INSTALLED_APPS += ('ccx',) - MIDDLEWARE_CLASSES += ('ccx.overrides.CcxMiddleware',) FIELD_OVERRIDE_PROVIDERS += ( 'ccx.overrides.CustomCoursesForEdxOverrideProvider', ) diff --git a/lms/envs/test.py b/lms/envs/test.py index 8519d860b2..38a30b944b 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -475,7 +475,6 @@ FEATURES['CERTIFICATES_HTML_VIEW'] = True ######### custom courses ######### INSTALLED_APPS += ('ccx',) -MIDDLEWARE_CLASSES += ('ccx.overrides.CcxMiddleware',) FEATURES['CUSTOM_COURSES_EDX'] = True # Set dummy values for profile image settings. diff --git a/lms/envs/yaml_config.py b/lms/envs/yaml_config.py index 6e17d439cf..db25bab6dd 100644 --- a/lms/envs/yaml_config.py +++ b/lms/envs/yaml_config.py @@ -307,7 +307,6 @@ GRADES_DOWNLOAD_ROUTING_KEY = HIGH_MEM_QUEUE ##### Custom Courses for EdX ##### if FEATURES.get('CUSTOM_COURSES_EDX'): INSTALLED_APPS += ('ccx',) - MIDDLEWARE_CLASSES += ('ccx.overrides.CcxMiddleware',) FIELD_OVERRIDE_PROVIDERS += ( 'ccx.overrides.CustomCoursesForEdxOverrideProvider', ) diff --git a/lms/templates/ccx/_dashboard_ccx_listing.html b/lms/templates/ccx/_dashboard_ccx_listing.html index 1d58bddd45..5856238498 100644 --- a/lms/templates/ccx/_dashboard_ccx_listing.html +++ b/lms/templates/ccx/_dashboard_ccx_listing.html @@ -4,9 +4,10 @@ from django.utils.translation import ugettext as _ from django.core.urlresolvers import reverse from courseware.courses import course_image_url, get_course_about_section +from ccx_keys.locator import CCXLocator %> <% - ccx_switch_target = reverse('switch_active_ccx', args=[course.id.to_deprecated_string(), ccx.id]) + ccx_target = reverse('info', args=[CCXLocator.from_course_locator(course.id, ccx.id)]) %>
  • @@ -14,7 +15,7 @@ from courseware.courses import course_image_url, get_course_about_section