From 6a0c9aee9deb10661aeea2e681c0d956c050f14b Mon Sep 17 00:00:00 2001 From: cewing Date: Wed, 20 May 2015 19:41:25 -0700 Subject: [PATCH 1/3] MIT CCX: Use CCX Keys Implement the use of CCX opaque keys throughout the ccx code base include the new custom ccx id package in the github checkouts list update the coach dashboard wrapper to get CCX information from the incoming course_id, if possible update function signatures for all view functions to expect CCX as passed by the dashboard wrapper (default to None), remove calls to get_ccx_for_coach as the ccx is passed in. update reverse calls in python view code to use a CCXLocator for the URL instead of a CourseLocator use CCXLocator where necessary use course id to find ccx, instead of thread local remove unused method and related tests use course id for getting ccx provide course id to the get_current_ccx method ensure the course id passed in is a CourseKey instance of some type whether it starts out as a string or not use the provided block to figure out what the course_id should be, then get the ccx for that redirect to ccx dashboard using coach ccx if no ccx is passed in update student dashboard listing for ccx to build an appropriate url from a CCXLocator, not from the course locator. refactor building the ccx locator so we don't have to do it repeatedly begin test refactoring after ccx_keys introduction Ensure that when access checking happens, the course_locator form of a ccx locator is used. This ensures that the access check happens against the course and it is not necesarry to duplicate the entire access control structure for the course. pick up api change in ccx-keys create and conditionally use a wrapper for the mixed modulestore returned by xmodule.modulestore.django.modulestore the wrapper will strip and restore ccx values from CourseKey and UsageKey objects fix return values on a few methods remove unused symbol pull updated ccx-keys package set course_id on the caching descriptor system to avoid api incompatibilities in some subsystems use ccx.course instead of self.course fix get method to find course keys from blocks that are not themselves keys but have a location attribute (which will be a key) if an item coming out of the db has children, restore the ccx to them as well if the block passed in has a CCX key, unwrap that before we try to look up the override, otherwise it will never be found. pick up a change in the ccx keys package that allows for stripping CCX identity from a usage key begin writing tests to cover this modulestore wrapper remove the switch_pocs view, the url pattern for it, and the tests that covered it remove the ccx context and the middleware responsible for setting the current CCX. These are no longer needed all dashboard views should raise 404 if a ccx is not provided by the coach_dashboard decorator code quality prevent errors resulting from trying to `get` a ccx based on non-unique criteria. remove obsolete usage of ACTIVE_CCX_KEY fix setUp method for grading tests to properly create grades for the ccx rather than for the course. clean up reverse calls code quality adding docstrings to clarify purpose of this patch fix bug in getting ccx for coach fix grading views to properly fetch a ccx-ified course so that grades for that version will be calculated fix small errors in modulestore implementation fix errant merge marker update call to get_current_ccx after key refactoring merged with tab changes --- common/djangoapps/student/models.py | 7 + common/djangoapps/student/views.py | 4 - .../lib/xmodule/xmodule/modulestore/django.py | 4 + .../split_mongo/caching_descriptor_system.py | 3 + lms/djangoapps/ccx/__init__.py | 4 - lms/djangoapps/ccx/modulestore.py | 312 ++++++++++++++++++ lms/djangoapps/ccx/overrides.py | 108 +++--- .../ccx/tests/test_ccx_modulestore.py | 116 +++++++ lms/djangoapps/ccx/tests/test_overrides.py | 17 +- lms/djangoapps/ccx/tests/test_utils.py | 79 +---- lms/djangoapps/ccx/tests/test_views.py | 299 ++++------------- lms/djangoapps/ccx/urls.py | 2 - lms/djangoapps/ccx/utils.py | 44 +-- lms/djangoapps/ccx/views.py | 186 ++++++----- lms/djangoapps/courseware/access.py | 6 + .../django_comment_client/forum/views.py | 2 +- lms/envs/test.py | 1 - lms/templates/ccx/_dashboard_ccx_listing.html | 11 +- lms/templates/navigation.html | 2 +- requirements/edx/github.txt | 1 + 20 files changed, 689 insertions(+), 519 deletions(-) create mode 100644 lms/djangoapps/ccx/modulestore.py create mode 100644 lms/djangoapps/ccx/tests/test_ccx_modulestore.py 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