MIT CCX: Use CCX Keys: further revisions in response to code review

only require ccx-keys once

get_current_ccx will now expect a CourseKey instance as its argument, and will raise a value error if this expectation is not met.

document reason for local import

add special methods to pass attribute setting and deletion through to the wrapped modulestore

add __setattr__ and __delattr__ per code review, update __init__ to work with new methods

style change per code review

clean up context manager usage as recommended by code review

remove unused code and imports

convert modulestore type tests to use the `get_modulestore_type` api, remove unused imports

code quality: add docstrings

increase coverage for utils tests

fix bug found in testing.

increase test coverage on modulestore wrapper

code quality fixes

code-quality: ignore import error, but mark site for future consideration

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

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.
+
+    course_key is expected to be an instance of an opaque CourseKey, a
+    ValueError is raised if this expectation is not met.
     """
-    # 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
+    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):

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))

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

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)

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(

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
 

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
 

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)
 

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 = []

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'))

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