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..db816a05fb 100644 --- a/common/lib/xmodule/xmodule/modulestore/django.py +++ b/common/lib/xmodule/xmodule/modulestore/django.py @@ -193,6 +193,10 @@ def modulestore(): settings.MODULESTORE['default'].get('OPTIONS', {}) ) + if settings.FEATURES.get('CUSTOM_COURSES_EDX'): + from ccx.modulestore import CCXModulestoreWrapper + _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..ed5172abff --- /dev/null +++ b/lms/djangoapps/ccx/modulestore.py @@ -0,0 +1,312 @@ +# -*- 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 ccx_keys.locator import CCXLocator, CCXBlockUsageLocator +from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator + + +def ccx_locator_to_course_locator(ccx_locator): + return CourseLocator( + org=ccx_locator.org, + course=ccx_locator.course, + run=ccx_locator.run, + branch=ccx_locator.branch, + version_guid=ccx_locator.version_guid, + ) + + +def strip_ccx(val): + retval = val + ccx_id = None + if hasattr(retval, 'ccx'): + if isinstance(retval, CCXLocator): + ccx_id = retval.ccx + retval = ccx_locator_to_course_locator(retval) + elif isinstance(object, CCXBlockUsageLocator): + ccx_locator = retval.course_key + ccx_id = ccx_locator.ccx + course_locator = ccx_locator_to_course_locator(ccx_locator) + retval = BlockUsageLocator( + course_locator, retval.block_type, retval.block_id + ) + if hasattr(retval, 'location'): + retval.location, ccx_id = strip_ccx(retval.location) + return retval, ccx_id + + +def restore_ccx(val, 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): + 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 + + +class CCXModulestoreWrapper(object): + + def __init__(self, modulestore): + self._modulestore = modulestore + + def __getattr__(self, name): + """pass missing attributes through to _modulestore + """ + return getattr(self._modulestore, name) + + def _clean_locator_for_mapping(self, locator): + locator, ccx = strip_ccx(locator) + retval = self._modulestore._clean_locator_for_mapping(locator) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def _get_modulestore_for_courselike(self, locator=None): + if locator is not None: + locator, _ = strip_ccx(locator) + return self._modulestore._get_modulestore_for_courselike(locator) + + def fill_in_run(self, course_key): + """ + Some course_keys are used without runs. This function calls the corresponding + fill_in_run function on the appropriate modulestore. + """ + course_key, ccx = strip_ccx(course_key) + retval = self._modulestore.fill_in_run(course_key) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def has_item(self, usage_key, **kwargs): + """ + Does the course include the xblock who's id is reference? + """ + usage_key, ccx = strip_ccx(usage_key) + return self._modulestore.has_item(usage_key, **kwargs) + + def get_item(self, usage_key, depth=0, **kwargs): + """ + see parent doc + """ + usage_key, ccx = strip_ccx(usage_key) + retval = self._modulestore.get_item(usage_key, depth, **kwargs) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def get_items(self, course_key, **kwargs): + course_key, ccx = strip_ccx(course_key) + retval = self._modulestore.get_items(course_key, **kwargs) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def get_course(self, course_key, depth=0, **kwargs): + # from nose.tools import set_trace; set_trace() + course_key, ccx = strip_ccx(course_key) + retval = self._modulestore.get_course(course_key, depth=depth, **kwargs) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def has_course(self, course_id, ignore_case=False, **kwargs): + course_id, ccx = strip_ccx(course_id) + retval = self._modulestore.has_course( + course_id, ignore_case=ignore_case, **kwargs + ) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def delete_course(self, course_key, user_id): + """ + See xmodule.modulestore.__init__.ModuleStoreWrite.delete_course + """ + course_key, ccx = strip_ccx(course_key) + return self._modulestore.delete_course(course_key, user_id) + + def get_parent_location(self, location, **kwargs): + location, ccx = strip_ccx(location) + retval = self._modulestore.get_parent_location(location, **kwargs) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def get_block_original_usage(self, usage_key): + usage_key, ccx = strip_ccx(usage_key) + orig_key, version = self._modulestore.get_block_original_usage(usage_key) + if orig_key and ccx: + orig_key = restore_ccx_collection(orig_key, ccx) + return orig_key, version + + def get_modulestore_type(self, course_id): + course_id, ccx = strip_ccx(course_id) + retval = self._modulestore.get_modulestore_type(course_id) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def get_orphans(self, course_key, **kwargs): + course_key, ccx = strip_ccx(course_key) + retval = self._modulestore.get_orphans(course_key, **kwargs) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs): + source_course_id, source_ccx = strip_ccx(source_course_id) + dest_course_id, dest_ccx = strip_ccx(dest_course_id) + retval = self._modulestore.clone_course( + source_course_id, dest_course_id, user_id, fields=fields, **kwargs + ) + if dest_ccx: + retval = restore_ccx_collection(retval, dest_ccx) + return retval + + def create_item(self, user_id, course_key, block_type, block_id=None, fields=None, **kwargs): + course_key, ccx = strip_ccx(course_key) + retval = self._modulestore.create_item( + user_id, course_key, block_type, block_id=block_id, fields=fields, **kwargs + ) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fields=None, **kwargs): + parent_usage_key, ccx = strip_ccx(parent_usage_key) + retval = self._modulestore.create_child( + user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, **kwargs + ) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None, **kwargs): + course_key, ccx = strip_ccx(course_key) + retval = self._modulestore.import_xblock( + user_id, course_key, block_type, block_id, fields=fields, runtime=runtime, **kwargs + ) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def copy_from_template(self, source_keys, dest_key, user_id, **kwargs): + dest_key, ccx = strip_ccx(dest_key) + retval = self._modulestore.copy_from_template( + source_keys, dest_key, user_id, **kwargs + ) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def update_item(self, xblock, user_id, allow_not_found=False, **kwargs): + xblock, ccx = strip_ccx(xblock) + retval = self._modulestore.update_item( + xblock, user_id, allow_not_found=allow_not_found, **kwargs + ) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def delete_item(self, location, user_id, **kwargs): + location, ccx = strip_ccx(location) + retval = self._modulestore.delete_item(location, user_id, **kwargs) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def revert_to_published(self, location, user_id): + location, ccx = strip_ccx(location) + retval = self._modulestore.revert_to_published(location, user_id) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def create_xblock(self, runtime, course_key, block_type, block_id=None, fields=None, **kwargs): + course_key, ccx = strip_ccx(course_key) + retval = self._modulestore.create_xblock( + runtime, course_key, block_type, block_id=block_id, fields=fields, **kwargs + ) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def has_published_version(self, xblock): + xblock.scope_ids.usage_id.course_key, ccx = strip_ccx( + xblock.scope_ids.usage_id.course_key + ) + retval = self._modulestore.has_published_version(xblock) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def publish(self, location, user_id, **kwargs): + location, ccx = strip_ccx(location) + retval = self._modulestore.publish(location, user_id, **kwargs) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def unpublish(self, location, user_id, **kwargs): + location, ccx = strip_ccx(location) + retval = self._modulestore.unpublish(location, user_id, **kwargs) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def convert_to_draft(self, location, user_id): + location, ccx = strip_ccx(location) + retval = self._modulestore.convert_to_draft(location, user_id) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def has_changes(self, xblock): + xblock.location, ccx = strip_ccx(xblock.location) + retval = self._modulestore.has_changes(xblock) + if ccx: + retval = restore_ccx_collection(retval, ccx) + return retval + + def check_supports(self, course_key, method): + course_key, _ = strip_ccx(course_key) + return self._modulestore.check_supports(course_key, method) + + @contextmanager + def branch_setting(self, branch_setting, course_id=None): + """ + A context manager for temporarily setting the branch value for the given course' store + to the given branch_setting. If course_id is None, the default store is used. + """ + 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): + 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..a1d25fab3b 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 CcxMembership, CcxFieldOverride, CustomCourseForEdX + + +log = logging.getLogger(__name__) class CustomCoursesForEdxOverrideProvider(FieldOverrideProvider): @@ -25,43 +27,42 @@ 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_id = None + identifier = getattr(block, 'id', None) + if isinstance(identifier, CourseKey): + course_id = block.id + elif isinstance(identifier, UsageKey): + course_id = block.id.course_key + elif hasattr(block, 'location'): + course_id = block.location.course_key + else: + msg = "Unable to get course id when calculating ccx overide for block type {}" + log.error(msg.format(type(block))) + if course_id is not None: + ccx = get_current_ccx(course_id) if ccx: return get_override_for_ccx(ccx, block, name, default) return default -class _CcxContext(threading.local): +def get_current_ccx(course_id): """ - 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. """ + # ensure that the ID passed in is a CourseKey instance of some type. + if isinstance(course_id, CourseKey): + course_key = course_id + else: + course_key = CourseKey.from_string(course_id) + ccx = None - request = 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 + if isinstance(course_key, CCXLocator): + ccx_id = course_key.ccx + ccx = CustomCourseForEdX.objects.get(pk=ccx_id) + return ccx def get_override_for_ccx(ccx, block, name, default=None): @@ -85,9 +86,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 +147,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..c8fc0858b6 --- /dev/null +++ b/lms/djangoapps/ccx/tests/test_ccx_modulestore.py @@ -0,0 +1,116 @@ +""" +Test the CCXModulestoreWrapper +""" +from collections import deque +from ccx_keys.locator import CCXLocator, CCXBlockUsageLocator +import datetime +from itertools import izip_longest +import pytz +from student.tests.factories import ( # pylint: disable=import-error + AdminFactory, + CourseEnrollmentFactory, + UserFactory, +) +from xmodule.modulestore.tests.django_utils import ( + ModuleStoreTestCase, + TEST_DATA_SPLIT_MODULESTORE) +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory + +from ..models import CustomCourseForEdX + + +def flatten(seq): + """ + For [[1, 2], [3, 4]] returns [1, 2, 3, 4]. Does not recurse. + """ + return [x for sub in seq for x in sub] + + +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() + 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) + chapters = [ItemFactory.create(start=start, parent=course) + for _ in xrange(2)] + sequentials = flatten([ + [ + ItemFactory.create(parent=chapter) for _ in xrange(2) + ] for chapter in chapters + ]) + verticals = flatten([ + [ + ItemFactory.create( + due=due, parent=sequential, graded=True, format='Homework' + ) for _ in xrange(2) + ] for sequential in sequentials + ]) + blocks = flatten([ # pylint: disable=unused-variable + [ + ItemFactory.create(parent=vertical) for _ in xrange(2) + ] for vertical 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) + + def get_all_children_bf(self, block): + 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) + for expected, actual in izip_longest( + self.get_all_children_bf(course), self.get_all_children_bf(ccx) + ): + 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) + diff --git a/lms/djangoapps/ccx/tests/test_overrides.py b/lms/djangoapps/ccx/tests/test_overrides.py index 37a7a14d36..e3fe76c6ab 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,7 @@ 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 +67,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 +84,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 +93,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 +103,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 +115,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 +127,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..d390ca4af0 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') @@ -128,6 +129,7 @@ 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 +159,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 +187,7 @@ 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 +368,7 @@ 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 @@ -508,70 +512,3 @@ class TestUnenrollEmail(ModuleStoreTestCase): self.check_enrollment_state(before, True, self.user, True) # no email was sent to the student self.assertEqual(self.outbox, []) - - -@attr('shard_1') -class TestUserCCXList(ModuleStoreTestCase): - """Unit tests for ccx.utils.get_all_ccx_for_user""" - - def setUp(self): - """Create required infrastructure for tests""" - super(TestUserCCXList, 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): - """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 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, 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_anonymous_sees_no_ccx(self): - memberships = self.call_fut(self.anonymous) - self.assertEqual(memberships, []) - - def test_unenrolled_sees_no_ccx(self): - memberships = self.call_fut(self.user) - self.assertEqual(memberships, []) - - 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']) diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index a217e791cc..2e0e9e100c 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -11,6 +11,7 @@ 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 @@ -28,19 +29,22 @@ from student.tests.factories import ( # pylint: disable=import-error 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 +82,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): """ Tests for Custom Courses views. """ + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE def setUp(self): """ Set up tests @@ -139,9 +144,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 +188,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 +207,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 +242,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 +262,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 +295,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 +325,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 +357,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 +390,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 +421,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 +442,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 +461,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 +481,24 @@ 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( + 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 +520,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 +534,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 +577,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 +600,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 +612,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..72a8391ffb 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/lms/djangoapps/ccx/utils.py @@ -14,6 +14,7 @@ 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, @@ -138,7 +139,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 +154,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 +165,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 +241,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) diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index d804bc4438..67f6e9ba5c 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 @@ -19,6 +20,7 @@ from django.http import ( ) 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 +36,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 +48,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 +72,60 @@ 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): + # is there a security leak here in not checking that this user is + # the coach for this ccx? + 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) + 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,7 +135,7 @@ 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 """ @@ -142,18 +160,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) + 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 +240,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 +296,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 +350,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): """ 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 +365,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 +399,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 +436,42 @@ 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): - """ - Show the gradebook for this CCX. - """ - # Need course module for overrides to function properly +@contextmanager +def ccx_course(ccx_locator): + 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 +501,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 +547,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..4f1963ee72 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): @@ -107,6 +111,8 @@ def has_access(user, action, obj, course_key=None): return _has_access_descriptor(user, action, obj, course_key) if isinstance(obj, CourseKey): + if isinstance(obj, CCXLocator): + obj = obj.to_course_locator() return _has_access_course_key(user, action, obj) if isinstance(obj, UsageKey): 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/test.py b/lms/envs/test.py index 323fa9bfe8..2bf2e856c4 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -473,7 +473,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/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