Opaque-Keys: Response to code review

cms/djangoapps/contentstore/tests/test_permissions.py

@@ -11,7 +11,7 @@ from contentstore.tests.modulestore_config import TEST_MODULESTORE
 from contentstore.tests.utils import AjaxEnabledTestClient
 from xmodule.modulestore.locations import SlashSeparatedCourseKey
 from contentstore.utils import reverse_url, reverse_course_url
-from student.roles import CourseInstructorRole, CourseStaffRole
+from student.roles import CourseInstructorRole, CourseStaffRole, OrgStaffRole, OrgInstructorRole
 from contentstore.views.access import has_course_access
 from student import auth
 
@@ -95,10 +95,15 @@ class TestCourseAccess(ModuleStoreTestCase):
         # doesn't use role.users_with_role b/c it's verifying the roles.py behavior
         user_by_role = {}
         # add the misc users to the course in different groups
-        for role in [CourseInstructorRole, CourseStaffRole]:
+        for role in [CourseInstructorRole, CourseStaffRole, OrgStaffRole, OrgInstructorRole]:
             user_by_role[role] = []
-            # pylint: disable=protected-access
-            group = role(self.course_key)
+            # Org-based roles are created via org name, rather than course_key
+            if (role is OrgStaffRole) or (role is OrgInstructorRole):
+                # pylint: disable=protected-access
+                group = role(self.course_key.org)
+            else:
+                # pylint: disable=protected-access
+                group = role(self.course_key)
             # NOTE: this loop breaks the roles.py abstraction by purposely assigning
             # users to one of each possible groupname in order to test that has_course_access
             # and remove_user work
@@ -109,21 +114,27 @@ class TestCourseAccess(ModuleStoreTestCase):
 
         course_team_url = reverse_course_url('course_team_handler', self.course_key)
         response = self.client.get_html(course_team_url)
-        for role in [CourseInstructorRole, CourseStaffRole]:
+        for role in [CourseInstructorRole, CourseStaffRole]:  # Global and org-based roles don't appear on this page
             for user in user_by_role[role]:
                 self.assertContains(response, user.email)
 
         # test copying course permissions
         copy_course_key = SlashSeparatedCourseKey('copyu', 'copydept.mycourse', 'myrun')
-        for role in [CourseInstructorRole, CourseStaffRole]:
-            auth.add_users(
-                self.user,
-                role(copy_course_key),
-                *role(self.course_key).users_with_role()
-            )
+        for role in [CourseInstructorRole, CourseStaffRole, OrgStaffRole, OrgInstructorRole]:
+            if (role is OrgStaffRole) or (role is OrgInstructorRole):
+                auth.add_users(
+                    self.user,
+                    role(copy_course_key.org),
+                    *role(self.course_key.org).users_with_role())
+            else:
+                auth.add_users(
+                    self.user,
+                    role(copy_course_key),
+                    *role(self.course_key).users_with_role()
+                )
         # verify access in copy course and verify that removal from source course w/ the various
         # groupnames works
-        for role in [CourseInstructorRole, CourseStaffRole]:
+        for role in [CourseInstructorRole, CourseStaffRole, OrgStaffRole, OrgInstructorRole]:
             for user in user_by_role[role]:
                 # forcefully decache the groups: premise is that any real request will not have
                 # multiple objects repr the same user but this test somehow uses different instance
@@ -132,5 +143,8 @@ class TestCourseAccess(ModuleStoreTestCase):
                     del user._roles
 
                 self.assertTrue(has_course_access(user, copy_course_key), "{} no copy access".format(user))
-                auth.remove_users(self.user, role(self.course_key), user)
+                if (role is OrgStaffRole) or (role is OrgInstructorRole):
+                    auth.remove_users(self.user, role(self.course_key.org), user)
+                else:
+                    auth.remove_users(self.user, role(self.course_key), user)
                 self.assertFalse(has_course_access(user, self.course_key), "{} remove didn't work".format(user))

cms/djangoapps/contentstore/views/course.py

@@ -204,7 +204,8 @@ def _accessible_courses_list_from_groups(request):
         try:
             course = modulestore('direct').get_course(course_key)
         except ItemNotFoundError:
-            raise ItemNotFoundError
+            # If a user has access to a course that doesn't exist, don't do anything with that course
+            pass
         if course is not None and not isinstance(course, ErrorDescriptor):
             # ignore deleted or errored courses
             courses_list[course_key] = course

common/djangoapps/student/roles.py

@@ -7,6 +7,7 @@ from abc import ABCMeta, abstractmethod
 
 from django.contrib.auth.models import User
 from student.models import CourseAccessRole
+from xmodule_django.models import CourseKeyField
 
 
 class AccessRole(object):
@@ -129,6 +130,9 @@ class RoleBase(AccessRole):
         """
         Return a django QuerySet for all of the users with this role
         """
+        # Org roles don't query by CourseKey, so use CourseKeyField.Empty for that query
+        if self.course_key is None:
+            self.course_key = CourseKeyField.Empty
         entries = User.objects.filter(
             courseaccessrole__role=self._role_name,
             courseaccessrole__org=self.org,

common/lib/xmodule/xmodule/contentstore/mongo.py

@@ -63,10 +63,11 @@ class MongoContentStore(ContentStore):
 
         return content
 
-    def delete(self, content_id):
-        if self.fs.exists({"_id": content_id}):
-            self.fs.delete(content_id)
-            assert not self.fs.exists({"_id": content_id})
+    def delete(self, location_or_id):
+        if isinstance(location_or_id, AssetLocation):
+            location_or_id = self.asset_db_key(location_or_id)
+        if self.fs.exists({"_id": location_or_id}):
+            self.fs.delete(location_or_id)
 
     def find(self, location, throw_on_not_found=True, as_stream=False):
         content_id = self.asset_db_key(location)
@@ -282,4 +283,8 @@ class MongoContentStore(ContentStore):
         """
         Returns the database query to find the given asset location.
         """
-        return location.to_deprecated_son(tag=XASSET_LOCATION_TAG, prefix='_id.')
+        # codifying the original order which pymongo used for the dicts coming out of location_to_dict
+        # stability of order is more important than sanity of order as any changes to order make things
+        # unfindable
+        ordered_key_fields = ['category', 'name', 'course', 'tag', 'org', 'revision']
+        return SON((field_name, getattr(location, field_name)) for field_name in ordered_key_fields)

requirements/edx/base.txt

@@ -56,7 +56,7 @@ polib==1.0.3
 pycrypto>=2.6
 pygments==1.6
 pygraphviz==1.1
-# pymongo==2.4.1
+pymongo==2.4.1
 pyparsing==2.0.1
 python-memcached==1.48
 python-openid==2.2.5

requirements/edx/github.txt

@@ -16,9 +16,6 @@
 -e git+https://github.com/un33k/django-ipware.git@42cb1bb1dc680a60c6452e8bb2b843c2a0382c90#egg=django-ipware
 -e git+https://github.com/appliedsec/pygeoip.git@95e69341cebf5a6a9fbf7c4f5439d458898bdc3b#egg=pygeoip
 
-# Temporary update to 3rd party libs:
--e git+https://github.com/edx/mongo-python-driver.git@dd01bf051dd5373146f668b9c78da9cb15c82331#egg=pymongo
-
 # Our libraries:
 -e git+https://github.com/edx/XBlock.git@fc5fea25c973ec66d8db63cf69a817ce624f5ef5#egg=XBlock
 -e git+https://github.com/edx/codejail.git@71f5c5616e2a73ae8cecd1ff2362774a773d3665#egg=codejail