From e3459cd3cb474fb8018848a5f4476df280559496 Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Thu, 5 Jun 2014 17:33:07 +0000 Subject: [PATCH] Opaque-Keys: Response to code review --- .../contentstore/tests/test_permissions.py | 40 +++++++++++++------ cms/djangoapps/contentstore/views/course.py | 3 +- common/djangoapps/student/roles.py | 4 ++ .../lib/xmodule/xmodule/contentstore/mongo.py | 15 ++++--- requirements/edx/base.txt | 2 +- requirements/edx/github.txt | 3 -- 6 files changed, 44 insertions(+), 23 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_permissions.py b/cms/djangoapps/contentstore/tests/test_permissions.py index afa357a9cd..dd4903d74c 100644 --- a/cms/djangoapps/contentstore/tests/test_permissions.py +++ b/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)) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index fbd75c7dae..468c16e023 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/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 diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 89043b2c3a..6c56b934b9 100644 --- a/common/djangoapps/student/roles.py +++ b/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, diff --git a/common/lib/xmodule/xmodule/contentstore/mongo.py b/common/lib/xmodule/xmodule/contentstore/mongo.py index 9cad2f1014..6aac2a43b0 100644 --- a/common/lib/xmodule/xmodule/contentstore/mongo.py +++ b/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) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 7955137333..16854aad36 100644 --- a/requirements/edx/base.txt +++ b/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 diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 726e7c0692..ac135296f4 100644 --- a/requirements/edx/github.txt +++ b/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