diff --git a/common/lib/xmodule/xmodule/modulestore/django.py b/common/lib/xmodule/xmodule/modulestore/django.py index db816a05fb..08654425b3 100644 --- a/common/lib/xmodule/xmodule/modulestore/django.py +++ b/common/lib/xmodule/xmodule/modulestore/django.py @@ -194,7 +194,12 @@ def modulestore(): ) if settings.FEATURES.get('CUSTOM_COURSES_EDX'): - from ccx.modulestore import CCXModulestoreWrapper + # TODO: This import prevents a circular import issue, but is + # symptomatic of a lib having a dependency on code in lms. This + # should be updated to have a setting that enumerates modulestore + # wrappers and then uses that setting to wrap the modulestore in + # appropriate wrappers depending on enabled features. + from ccx.modulestore import CCXModulestoreWrapper # pylint: disable=import-error _MIXED_MODULESTORE = CCXModulestoreWrapper(_MIXED_MODULESTORE) return _MIXED_MODULESTORE diff --git a/lms/djangoapps/ccx/modulestore.py b/lms/djangoapps/ccx/modulestore.py index d8ac7d7169..e4ac651cf0 100644 --- a/lms/djangoapps/ccx/modulestore.py +++ b/lms/djangoapps/ccx/modulestore.py @@ -16,12 +16,16 @@ from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator def strip_ccx(val): + """remove any reference to a CCX from the incoming value + + return a tuple of the stripped value and the id of the ccx + """ retval = val ccx_id = None if isinstance(retval, CCXLocator): ccx_id = retval.ccx retval = retval.to_course_locator() - elif isinstance(object, CCXBlockUsageLocator): + elif isinstance(retval, CCXBlockUsageLocator): ccx_id = retval.course_key.ccx retval = retval.to_block_locator() elif hasattr(retval, 'location'): @@ -30,6 +34,10 @@ def strip_ccx(val): def restore_ccx(val, ccx_id): + """restore references to a CCX to the incoming value + + returns the value converted to a CCX-aware state, using the provided ccx_id + """ if isinstance(val, CourseLocator): return CCXLocator.from_course_locator(val, ccx_id) elif isinstance(val, BlockUsageLocator): @@ -43,6 +51,11 @@ def restore_ccx(val, ccx_id): def restore_ccx_collection(field_value, ccx_id=None): + """restore references to a CCX to collections of incoming values + + returns the original collection with all values converted to a ccx-aware + state, using the provided ccx_id + """ if ccx_id is None: return field_value if isinstance(field_value, list): @@ -57,226 +70,229 @@ def restore_ccx_collection(field_value, ccx_id=None): @contextmanager def remove_ccx(to_strip): + """A context manager for wrapping modulestore api methods. + + yields a stripped value and a function suitable for restoring it + """ stripped, ccx = strip_ccx(to_strip) yield stripped, partial(restore_ccx_collection, ccx_id=ccx) class CCXModulestoreWrapper(object): + """This class wraps a modulestore + + The purpose is to remove ccx-specific identifiers during lookup and restore + it after retrieval so that data can be stored local to a course, but + referenced in app context as ccx-specific + """ def __init__(self, modulestore): - self._modulestore = modulestore + """wrap the provided modulestore""" + self.__dict__['_modulestore'] = modulestore def __getattr__(self, name): - """pass missing attributes through to _modulestore - """ + """look up missing attributes on the wrapped modulestore""" return getattr(self._modulestore, name) + def __setattr__(self, name, value): + """set attributes only on the wrapped modulestore""" + setattr(self._modulestore, name, value) + + def __delattr__(self, name): + """delete attributes only on the wrapped modulestore""" + delattr(self._modulestore, name) + def _clean_locator_for_mapping(self, locator): - with remove_ccx(locator) as stripped: - locator, restore = stripped - retval = self._modulestore._clean_locator_for_mapping(locator) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(locator) as (locator, restore): + # pylint: disable=protected-access + return restore( + self._modulestore._clean_locator_for_mapping(locator) + ) def _get_modulestore_for_courselike(self, locator=None): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" if locator is not None: locator, _ = strip_ccx(locator) + # pylint: disable=protected-access return self._modulestore._get_modulestore_for_courselike(locator) def fill_in_run(self, course_key): - """ - Some course_keys are used without runs. This function calls the corresponding - fill_in_run function on the appropriate modulestore. - """ - with remove_ccx(course_key) as stripped: - course_key, restore = stripped - retval = self._modulestore.fill_in_run(course_key) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.fill_in_run(course_key)) def has_item(self, usage_key, **kwargs): - """ - Does the course include the xblock who's id is reference? - """ - usage_key, ccx = strip_ccx(usage_key) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + usage_key, _ = strip_ccx(usage_key) return self._modulestore.has_item(usage_key, **kwargs) def get_item(self, usage_key, depth=0, **kwargs): - """ - see parent doc - """ - with remove_ccx(usage_key) as stripped: - usage_key, restore = stripped - retval = self._modulestore.get_item(usage_key, depth, **kwargs) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(usage_key) as (usage_key, restore): + return restore( + self._modulestore.get_item(usage_key, depth, **kwargs) + ) def get_items(self, course_key, **kwargs): - with remove_ccx(course_key) as stripped: - course_key, restore = stripped - retval = self._modulestore.get_items(course_key, **kwargs) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.get_items(course_key, **kwargs)) def get_course(self, course_key, depth=0, **kwargs): - with remove_ccx(course_key) as stripped: - course_key, restore = stripped - retval = self._modulestore.get_course( + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.get_course( course_key, depth=depth, **kwargs - ) - return restore(retval) + )) def has_course(self, course_id, ignore_case=False, **kwargs): - with remove_ccx(course_id) as stripped: - course_id, restore = stripped - retval = self._modulestore.has_course( + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_id) as (course_id, restore): + return restore(self._modulestore.has_course( course_id, ignore_case=ignore_case, **kwargs - ) - return restore(retval) + )) def delete_course(self, course_key, user_id): """ See xmodule.modulestore.__init__.ModuleStoreWrite.delete_course """ - course_key, ccx = strip_ccx(course_key) + course_key, _ = strip_ccx(course_key) return self._modulestore.delete_course(course_key, user_id) def get_parent_location(self, location, **kwargs): - with remove_ccx(location) as stripped: - location, restore = stripped - retval = self._modulestore.get_parent_location(location, **kwargs) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(location) as (location, restore): + return restore( + self._modulestore.get_parent_location(location, **kwargs) + ) def get_block_original_usage(self, usage_key): - with remove_ccx(usage_key) as stripped: - usage_key, restore = stripped + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(usage_key) as (usage_key, restore): orig_key, version = self._modulestore.get_block_original_usage(usage_key) return restore(orig_key), version def get_modulestore_type(self, course_id): - with remove_ccx(course_id) as stripped: - course_id, restore = stripped - retval = self._modulestore.get_modulestore_type(course_id) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_id) as (course_id, restore): + return restore(self._modulestore.get_modulestore_type(course_id)) def get_orphans(self, course_key, **kwargs): - with remove_ccx(course_key) as stripped: - course_key, restore = stripped - retval = self._modulestore.get_orphans(course_key, **kwargs) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.get_orphans(course_key, **kwargs)) def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs): - 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 + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(source_course_id) as (source_course_id, _): + with remove_ccx(dest_course_id) as (dest_course_id, dest_restore): + return dest_restore(self._modulestore.clone_course( + source_course_id, dest_course_id, user_id, fields=fields, **kwargs + )) def create_item(self, user_id, course_key, block_type, block_id=None, fields=None, **kwargs): - with remove_ccx(course_key) as stripped: - course_key, restore = stripped - retval = self._modulestore.create_item( + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.create_item( user_id, course_key, block_type, block_id=block_id, fields=fields, **kwargs - ) - return restore(retval) + )) def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fields=None, **kwargs): - with remove_ccx(parent_usage_key) as stripped: - parent_usage_key, restore = stripped - retval = self._modulestore.create_child( + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(parent_usage_key) as (parent_usage_key, restore): + return restore(self._modulestore.create_child( user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, **kwargs - ) - return restore(retval) + )) def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None, **kwargs): - with remove_ccx(course_key) as stripped: - course_key, restore = stripped - retval = self._modulestore.import_xblock( + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.import_xblock( user_id, course_key, block_type, block_id, fields=fields, runtime=runtime, **kwargs - ) - return restore(retval) + )) def copy_from_template(self, source_keys, dest_key, user_id, **kwargs): - with remove_ccx(dest_key) as stripped: - dest_key, restore = stripped - retval = self._modulestore.copy_from_template( + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(dest_key) as (dest_key, restore): + return restore(self._modulestore.copy_from_template( source_keys, dest_key, user_id, **kwargs - ) - return restore(retval) + )) def update_item(self, xblock, user_id, allow_not_found=False, **kwargs): - with remove_ccx(xblock) as stripped: - xblock, restore = stripped - retval = self._modulestore.update_item( + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(xblock) as (xblock, restore): + return restore(self._modulestore.update_item( xblock, user_id, allow_not_found=allow_not_found, **kwargs - ) - return restore(retval) + )) def delete_item(self, location, user_id, **kwargs): - with remove_ccx(location) as stripped: - location, restore = stripped - retval = self._modulestore.delete_item(location, user_id, **kwargs) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(location) as (location, restore): + return restore( + self._modulestore.delete_item(location, user_id, **kwargs) + ) def revert_to_published(self, location, user_id): - with remove_ccx(location) as stripped: - location, restore = stripped - retval = self._modulestore.revert_to_published(location, user_id) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(location) as (location, restore): + return restore( + self._modulestore.revert_to_published(location, user_id) + ) def create_xblock(self, runtime, course_key, block_type, block_id=None, fields=None, **kwargs): - with remove_ccx(course_key) as stripped: - course_key, restore = stripped - retval = self._modulestore.create_xblock( + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(course_key) as (course_key, restore): + return restore(self._modulestore.create_xblock( runtime, course_key, block_type, block_id=block_id, fields=fields, **kwargs - ) - return restore(retval) + )) def has_published_version(self, xblock): - with remove_ccx(xblock) as stripped: - xblock, restore = stripped - retval = self._modulestore.has_published_version(xblock) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(xblock) as (xblock, restore): + return restore(self._modulestore.has_published_version(xblock)) def publish(self, location, user_id, **kwargs): - with remove_ccx(location) as stripped: - location, restore = stripped - retval = self._modulestore.publish(location, user_id, **kwargs) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(location) as (location, restore): + return restore( + self._modulestore.publish(location, user_id, **kwargs) + ) def unpublish(self, location, user_id, **kwargs): - with remove_ccx(location) as stripped: - location, restore = stripped - retval = self._modulestore.unpublish(location, user_id, **kwargs) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(location) as (location, restore): + return restore( + self._modulestore.unpublish(location, user_id, **kwargs) + ) def convert_to_draft(self, location, user_id): - with remove_ccx(location) as stripped: - location, restore = stripped - retval = self._modulestore.convert_to_draft(location, user_id) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(location) as (location, restore): + return restore( + self._modulestore.convert_to_draft(location, user_id) + ) def has_changes(self, xblock): - with remove_ccx(xblock) as stripped: - xblock, restore = stripped - retval = self._modulestore.has_changes(xblock) - return restore(retval) + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + with remove_ccx(xblock) as (xblock, restore): + return restore(self._modulestore.has_changes(xblock)) def check_supports(self, course_key, method): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" course_key, _ = strip_ccx(course_key) return self._modulestore.check_supports(course_key, method) @contextmanager def branch_setting(self, branch_setting, course_id=None): - """ - 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. - """ + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" course_id, _ = strip_ccx(course_id) with self._modulestore.branch_setting(branch_setting, course_id): yield @contextmanager def bulk_operations(self, course_id, emit_signals=True): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" course_id, _ = strip_ccx(course_id) with self._modulestore.bulk_operations(course_id, emit_signals=emit_signals): yield diff --git a/lms/djangoapps/ccx/overrides.py b/lms/djangoapps/ccx/overrides.py index a1d25fab3b..cd90098aa3 100644 --- a/lms/djangoapps/ccx/overrides.py +++ b/lms/djangoapps/ccx/overrides.py @@ -11,7 +11,7 @@ from courseware.field_overrides import FieldOverrideProvider # pylint: disable= from opaque_keys.edx.keys import CourseKey, UsageKey from ccx_keys.locator import CCXLocator, CCXBlockUsageLocator -from .models import CcxMembership, CcxFieldOverride, CustomCourseForEdX +from .models import CcxFieldOverride, CustomCourseForEdX log = logging.getLogger(__name__) @@ -30,39 +30,38 @@ class CustomCoursesForEdxOverrideProvider(FieldOverrideProvider): # 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 + ccx = course_key = None identifier = getattr(block, 'id', None) if isinstance(identifier, CourseKey): - course_id = block.id + course_key = block.id elif isinstance(identifier, UsageKey): - course_id = block.id.course_key + course_key = block.id.course_key elif hasattr(block, 'location'): - course_id = block.location.course_key + course_key = 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) + msg = "Unable to get course id when calculating ccx overide for block type %r" + log.error(msg, type(block)) + if course_key is not None: + ccx = get_current_ccx(course_key) if ccx: return get_override_for_ccx(ccx, block, name, default) return default -def get_current_ccx(course_id): +def get_current_ccx(course_key): """ 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 - if isinstance(course_key, CCXLocator): - ccx_id = course_key.ccx - ccx = CustomCourseForEdX.objects.get(pk=ccx_id) - return ccx + course_key is expected to be an instance of an opaque CourseKey, a + ValueError is raised if this expectation is not met. + """ + if not isinstance(course_key, CourseKey): + raise ValueError("get_current_ccx requires a CourseKey instance") + + if not isinstance(course_key, CCXLocator): + return None + + return CustomCourseForEdX.objects.get(pk=course_key.ccx) def get_override_for_ccx(ccx, block, name, default=None): diff --git a/lms/djangoapps/ccx/tests/test_ccx_modulestore.py b/lms/djangoapps/ccx/tests/test_ccx_modulestore.py index 79129a38d7..8e642595b2 100644 --- a/lms/djangoapps/ccx/tests/test_ccx_modulestore.py +++ b/lms/djangoapps/ccx/tests/test_ccx_modulestore.py @@ -2,30 +2,20 @@ Test the CCXModulestoreWrapper """ from collections import deque -from ccx_keys.locator import CCXLocator, CCXBlockUsageLocator +from ccx_keys.locator import CCXLocator import datetime -from itertools import izip_longest +from itertools import izip_longest, chain import pytz -from student.tests.factories import ( # pylint: disable=import-error - AdminFactory, - CourseEnrollmentFactory, - UserFactory, -) +from student.tests.factories import AdminFactory from xmodule.modulestore.tests.django_utils import ( ModuleStoreTestCase, - TEST_DATA_SPLIT_MODULESTORE) + 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 """ @@ -36,7 +26,7 @@ class TestCCXModulestoreWrapper(ModuleStoreTestCase): Set up tests """ super(TestCCXModulestoreWrapper, self).setUp() - course = CourseFactory.create() + self.course = course = CourseFactory.create() # Create instructor account coach = AdminFactory.create() @@ -46,17 +36,18 @@ class TestCCXModulestoreWrapper(ModuleStoreTestCase): 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 = [ + self.chapters = chapters = [ + ItemFactory.create(start=start, parent=course) for _ in xrange(2) + ] + self.sequentials = sequentials = [ ItemFactory.create(parent=c) for _ in xrange(2) for c in chapters ] - verticals = [ + self.verticals = verticals = [ ItemFactory.create( due=due, parent=s, graded=True, format='Homework' ) for _ in xrange(2) for s in sequentials ] - blocks = [ + self.blocks = [ ItemFactory.create(parent=v) for _ in xrange(2) for v in verticals ] @@ -67,9 +58,10 @@ class TestCCXModulestoreWrapper(ModuleStoreTestCase): ) ccx.save() - self.ccx_locator = CCXLocator.from_course_locator(course.id, ccx.id) + self.ccx_locator = CCXLocator.from_course_locator(course.id, ccx.id) # pylint: disable=no-member def get_all_children_bf(self, block): + """traverse the children of block in a breadth-first order""" queue = deque([block]) while queue: item = queue.popleft() @@ -97,9 +89,10 @@ class TestCCXModulestoreWrapper(ModuleStoreTestCase): 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( + test_fodder = izip_longest( self.get_all_children_bf(course), self.get_all_children_bf(ccx) - ): + ) + for expected, actual in test_fodder: if expected is None: self.fail('course children exhausted before ccx children') if actual is None: @@ -108,3 +101,36 @@ class TestCCXModulestoreWrapper(ModuleStoreTestCase): self.assertEqual(expected.location.course_key, course_key) self.assertEqual(actual.location.course_key, self.ccx_locator) + def test_has_item(self): + """can verify that a location exists, using ccx block usage key""" + for item in chain(self.chapters, self.sequentials, self.verticals, self.blocks): + block_key = self.ccx_locator.make_usage_key( + item.location.block_type, item.location.block_id + ) + self.assertTrue(self.store.has_item(block_key)) + + def test_get_item(self): + """can retrieve an item by a location key, using a ccx block usage key + + the retrieved item should be the same as the the one read without ccx + info + """ + for expected in chain(self.chapters, self.sequentials, self.verticals, self.blocks): + block_key = self.ccx_locator.make_usage_key( + expected.location.block_type, expected.location.block_id + ) + actual = self.store.get_item(block_key) + self.assertEqual(expected.display_name, actual.display_name) + self.assertEqual(expected.location, actual.location.to_block_locator()) + + def test_publication_api(self): + """verify that we can correctly discern a published item by ccx key""" + for expected in self.blocks: + block_key = self.ccx_locator.make_usage_key( + expected.location.block_type, expected.location.block_id + ) + self.assertTrue(self.store.has_published_version(expected)) + self.store.unpublish(block_key, self.user.id) + self.assertFalse(self.store.has_published_version(expected)) + self.store.publish(block_key, self.user.id) + self.assertTrue(self.store.has_published_version(expected)) diff --git a/lms/djangoapps/ccx/tests/test_overrides.py b/lms/djangoapps/ccx/tests/test_overrides.py index e3fe76c6ab..a7a972eb25 100644 --- a/lms/djangoapps/ccx/tests/test_overrides.py +++ b/lms/djangoapps/ccx/tests/test_overrides.py @@ -28,6 +28,7 @@ class TestFieldOverrides(ModuleStoreTestCase): Make sure field overrides behave in the expected manner. """ MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + def setUp(self): """ Set up tests diff --git a/lms/djangoapps/ccx/tests/test_utils.py b/lms/djangoapps/ccx/tests/test_utils.py index 25325059de..6dd8924967 100644 --- a/lms/djangoapps/ccx/tests/test_utils.py +++ b/lms/djangoapps/ccx/tests/test_utils.py @@ -130,6 +130,7 @@ class TestGetEmailParams(ModuleStoreTestCase): """tests for ccx.utils.get_email_params """ MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + def setUp(self): """ Set up tests @@ -188,6 +189,7 @@ 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 @@ -369,6 +371,7 @@ class TestEnrollEmail(ModuleStoreTestCase): 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 @@ -512,3 +515,63 @@ 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 TestGetMembershipTriplets(ModuleStoreTestCase): + """Verify that get_ccx_membership_triplets functions properly""" + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + + def setUp(self): + """Set up a course, coach, ccx and user""" + super(TestGetMembershipTriplets, self).setUp() + self.course = CourseFactory.create() + coach = AdminFactory.create() + role = CourseCcxCoachRole(self.course.id) + role.add_users(coach) + self.ccx = CcxFactory(course_id=self.course.id, coach=coach) + + def make_ccx_membership(self, active=True): + """create registration of self.user in self.ccx + + registration will be inactive + """ + CcxMembershipFactory.create(ccx=self.ccx, student=self.user, active=active) + + def call_fut(self, org_filter=None, org_filter_out=()): + """call the function under test in this test case""" + from ccx.utils import get_ccx_membership_triplets + return list( + get_ccx_membership_triplets(self.user, org_filter, org_filter_out) + ) + + def test_no_membership(self): + """verify that no triplets are returned if there are no memberships + """ + triplets = self.call_fut() + self.assertEqual(len(triplets), 0) + + def test_has_membership(self): + """verify that a triplet is returned when a membership exists + """ + self.make_ccx_membership() + triplets = self.call_fut() + self.assertEqual(len(triplets), 1) + ccx, membership, course = triplets[0] + self.assertEqual(ccx.id, self.ccx.id) + self.assertEqual(unicode(course.id), unicode(self.course.id)) + self.assertEqual(membership.student, self.user) + + def test_has_membership_org_filtered(self): + """verify that microsite org filter prevents seeing microsite ccx""" + self.make_ccx_membership() + bad_org = self.course.location.org + 'foo' + triplets = self.call_fut(org_filter=bad_org) + self.assertEqual(len(triplets), 0) + + def test_has_membership_org_filtered_out(self): + """verify that microsite ccxs not seen in non-microsite view""" + self.make_ccx_membership() + filter_list = [self.course.location.org] + triplets = self.call_fut(org_filter_out=filter_list) + self.assertEqual(len(triplets), 0) diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 2e0e9e100c..f0b949efd1 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -6,7 +6,6 @@ import json import re import pytz import ddt -import unittest from mock import patch, MagicMock from nose.plugins.attrib import attr @@ -27,7 +26,6 @@ from student.tests.factories import ( # pylint: disable=import-error UserFactory, ) -from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.x_module import XModuleMixin from xmodule.modulestore.tests.django_utils import ( ModuleStoreTestCase, @@ -36,7 +34,6 @@ from xmodule.modulestore.tests.factories import ( CourseFactory, ItemFactory, ) -import xmodule.tabs as tabs from ccx_keys.locator import CCXLocator from ..models import ( @@ -83,6 +80,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): Tests for Custom Courses views. """ MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + def setUp(self): """ Set up tests @@ -481,6 +479,7 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): category="sequential", metadata={'graded': True, 'format': 'Homework'}) for _ in xrange(4)] + # pylint: disable=unused-variable problems = [ [ ItemFactory.create( diff --git a/lms/djangoapps/ccx/utils.py b/lms/djangoapps/ccx/utils.py index 88f5ea187e..7d493c4b93 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/lms/djangoapps/ccx/utils.py @@ -4,8 +4,6 @@ CCX Enrollment operations for use by Coach APIs. Does not include any access control, be sure to check access before calling. """ import logging -from courseware.courses import get_course_about_section # pylint: disable=import-error -from courseware.courses import get_course_by_id # pylint: disable=import-error from django.contrib.auth.models import User from django.conf import settings from django.core.urlresolvers import reverse @@ -20,7 +18,6 @@ from .models import ( CcxMembership, CcxFutureMembership, ) -from .overrides import get_current_ccx log = logging.getLogger("edx.ccx") @@ -154,7 +151,7 @@ def get_email_params(ccx, auto_enroll, secure=True): site=stripped_site_name, path=reverse( 'course_root', - kwargs={'course_id': CCXLocator.from_course_locator(ccx.course_id, ccx.id)} + kwargs={'course_id': CCXLocator.from_course_locator(ccx.course_id, ccx.id)} ) ) @@ -165,7 +162,7 @@ def get_email_params(ccx, auto_enroll, secure=True): site=stripped_site_name, path=reverse( 'about_course', - kwargs={'course_id': CCXLocator.from_course_locator(ccx.course_id, ccx.id)} + kwargs={'course_id': CCXLocator.from_course_locator(ccx.course_id, ccx.id)} ) ) @@ -267,9 +264,9 @@ def get_ccx_membership_triplets(user, course_org_filter, org_filter_out_set): # warning to the log so we can clean up. if course.location.deprecated: log.warning( - "CCX {} exists for course {} with deprecated id".format( - ccx, ccx.course_id - ) + "CCX %s exists for course %s with deprecated id", + ccx, + ccx.course_id ) continue diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index e07049f784..1cd845e24d 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -110,7 +110,10 @@ def dashboard(request, course, ccx=None): 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)}) + url = reverse( + 'ccx_coach_dashboard', + kwargs={'course_id': CCXLocator.from_course_locator(course.id, ccx.id)} + ) return redirect(url) context = { @@ -151,11 +154,10 @@ def create_ccx(request, course, ccx=None): # prevent CCX objects from being created for deprecated course ids. if course.id.deprecated: - messages.error(_( + messages.error(request, _( "You cannot create a CCX from a course using a deprecated id. " "Please create a rerun of this course in the studio to allow " - "this action.") - ) + "this action.")) url = reverse('ccx_coach_dashboard', kwargs={'course_id', course.id}) return redirect(url) @@ -179,7 +181,7 @@ def create_ccx(request, course, ccx=None): for vertical in sequential.get_children(): override_field_for_ccx(ccx, vertical, hidden, True) - ccx_id = CCXLocator.from_course_locator(course.id, ccx.id) + ccx_id = CCXLocator.from_course_locator(course.id, ccx.id) # pylint: disable=no-member url = reverse('ccx_coach_dashboard', kwargs={'course_id': ccx_id}) return redirect(url) @@ -369,7 +371,7 @@ 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, ccx=None): +def ccx_schedule(request, course, ccx=None): # pylint: disable=unused-argument """ get json representation of ccx schedule """ @@ -464,6 +466,8 @@ def ccx_student_management(request, course, ccx=None): @contextmanager def ccx_course(ccx_locator): + """Create a context in which the course identified by course_locator exists + """ course = get_course_by_id(ccx_locator) yield course diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 0d66aa17e3..3fa2cda60c 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -494,7 +494,13 @@ def _has_access_course_key(user, action, course_key): return _dispatch(checkers, action, user, course_key) + def _has_access_ccx_key(user, action, ccx_key): + """Check if user has access to the course for this ccx_key + + Delegates checking to _has_access_course_key + Valid actions: same as for that function + """ course_key = ccx_key.to_course_locator() return _has_access_course_key(user, action, course_key) diff --git a/lms/djangoapps/dashboard/sysadmin.py b/lms/djangoapps/dashboard/sysadmin.py index da10935ca0..d65fd7dd5b 100644 --- a/lms/djangoapps/dashboard/sysadmin.py +++ b/lms/djangoapps/dashboard/sysadmin.py @@ -41,9 +41,7 @@ from student.models import CourseEnrollment, UserProfile, Registration import track.views from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore -from xmodule.modulestore.xml import XMLModuleStore from opaque_keys.edx.locations import SlashSeparatedCourseKey -from ccx.modulestore import CCXModulestoreWrapper log = logging.getLogger(__name__) @@ -60,13 +58,10 @@ class SysadminDashboardView(TemplateView): modulestore_type and return msg """ - self.def_ms = check_ms = modulestore() - # if the modulestore is wrapped by CCX, unwrap it for checking purposes - if isinstance(check_ms, CCXModulestoreWrapper): - check_ms = check_ms._modulestore + self.def_ms = modulestore() self.is_using_mongo = True - if isinstance(check_ms, XMLModuleStore): + if self.def_ms.get_modulestore_type(None) == 'xml': self.is_using_mongo = False self.msg = u'' self.datatable = [] diff --git a/lms/djangoapps/dashboard/tests/test_sysadmin.py b/lms/djangoapps/dashboard/tests/test_sysadmin.py index 8ed5c07d56..7f08e35906 100644 --- a/lms/djangoapps/dashboard/tests/test_sysadmin.py +++ b/lms/djangoapps/dashboard/tests/test_sysadmin.py @@ -32,8 +32,6 @@ from student.tests.factories import UserFactory from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST -from xmodule.modulestore.xml import XMLModuleStore -from ccx.modulestore import CCXModulestoreWrapper TEST_MONGODB_LOG = { @@ -316,12 +314,9 @@ class TestSysadmin(SysadminBaseTestCase): # Create git loaded course response = self._add_edx4edx() - def_ms = check_ms = modulestore() - # if the modulestore is wrapped by CCX, unwrap it for testing purposes - if isinstance(check_ms, CCXModulestoreWrapper): - check_ms = check_ms._modulestore + def_ms = modulestore() - self.assertIn('xml', str(check_ms.__class__)) + self.assertEqual('xml', def_ms.get_modulestore_type(None)) course = def_ms.courses.get('{0}/edx4edx_lite'.format( os.path.abspath(settings.DATA_DIR)), None) self.assertIsNotNone(course) @@ -465,7 +460,7 @@ class TestSysAdminMongoCourseImport(SysadminBaseTestCase): self._mkdir(getattr(settings, 'GIT_REPO_DIR')) def_ms = modulestore() - self.assertFalse(isinstance(def_ms, XMLModuleStore)) + self.assertFalse('xml' == def_ms.get_modulestore_type(None)) self._add_edx4edx() course = def_ms.get_course(SlashSeparatedCourseKey('MITx', 'edx4edx', 'edx4edx')) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 1e6c7d4a1a..44b429f789 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -54,7 +54,6 @@ git+https://github.com/edx/edx-lint.git@ed8c8d2a0267d4d42f43642d193e25f8bd575d9b -e git+https://github.com/edx-solutions/xblock-google-drive.git@138e6fa0bf3a2013e904a085b9fed77dab7f3f21#egg=xblock-google-drive -e git+https://github.com/edx/edx-reverification-block.git@6e2834c5f7e998ad9b81170e7ceb4d8a64900eb0#egg=edx-reverification-block git+https://github.com/edx/ecommerce-api-client.git@1.0.0#egg=ecommerce-api-client==1.0.0 --e git+https://github.com/jazkarta/ccx-keys.git@e6b03704b1bb97c1d2f31301ecb4e3a687c536ea#egg=ccx-keys # Third Party XBlocks -e git+https://github.com/mitodl/edx-sga@172a90fd2738f8142c10478356b2d9ed3e55334a#egg=edx-sga