Merge pull request #1910 from edx/christina/locations

Create explicit Locations.

cms/djangoapps/auth/tests/test_authz.py

@@ -5,6 +5,7 @@ import mock
 
 from django.test import TestCase
 from django.contrib.auth.models import User
+from xmodule.modulestore import Location
 from django.core.exceptions import PermissionDenied
 
 from auth.authz import add_user_to_creator_group, remove_user_from_creator_group, is_user_in_creator_group,\
@@ -129,7 +130,7 @@ class CourseGroupTest(TestCase):
         """ Test case setup """
         self.creator = User.objects.create_user('testcreator', 'testcreator+courses@edx.org', 'foo')
         self.staff = User.objects.create_user('teststaff', 'teststaff+courses@edx.org', 'foo')
-        self.location = 'i4x', 'mitX', '101', 'course', 'test'
+        self.location = Location('i4x', 'mitX', '101', 'course', 'test')
 
     def test_add_user_to_course_group(self):
         """
@@ -181,7 +182,7 @@ class CourseGroupTest(TestCase):
         create_all_course_groups(self.creator, self.location)
         add_user_to_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME)
 
-        location2 = 'i4x', 'mitX', '103', 'course', 'test2'
+        location2 = Location('i4x', 'mitX', '103', 'course', 'test2')
         staff2 = User.objects.create_user('teststaff2', 'teststaff2+courses@edx.org', 'foo')
         create_all_course_groups(self.creator, location2)
         add_user_to_course_group(self.creator, staff2, location2, STAFF_ROLE_NAME)
@@ -193,7 +194,7 @@ class CourseGroupTest(TestCase):
         create_all_course_groups(self.creator, self.location)
         add_user_to_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME)
 
-        location2 = 'i4x', 'mitX', '103', 'course', 'test2'
+        location2 = Location('i4x', 'mitX', '103', 'course', 'test2')
         creator2 = User.objects.create_user('testcreator2', 'testcreator2+courses@edx.org', 'foo')
         staff2 = User.objects.create_user('teststaff2', 'teststaff2+courses@edx.org', 'foo')
         create_all_course_groups(creator2, location2)

cms/djangoapps/contentstore/management/commands/check_course.py

@@ -2,6 +2,7 @@ from django.core.management.base import BaseCommand, CommandError
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.xml_importer import check_module_metadata_editability
 from xmodule.course_module import CourseDescriptor
+from xmodule.modulestore import Location
 
 
 class Command(BaseCommand):
@@ -54,8 +55,16 @@ class Command(BaseCommand):
         discussion_items = _get_discussion_items(course)
 
         # now query all discussion items via get_items() and compare with the tree-traversal
-        queried_discussion_items = store.get_items(['i4x', course.location.org, course.location.course,
-                                                    'discussion', None, None])
+        queried_discussion_items = store.get_items(
+            Location(
+                'i4x',
+                course.location.org,
+                course.location.course,
+                'discussion',
+                None,
+                None
+            )
+        )
 
         for item in queried_discussion_items:
             if item.location.url() not in discussion_items:

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -202,20 +202,20 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         draft_store = modulestore('draft')
         import_from_xml(store, 'common/test/data/', ['simple'])
 
-        html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None])
+        html_module = draft_store.get_item(Location('i4x', 'edX', 'simple', 'html', 'test_html', None))
 
         draft_store.convert_to_draft(html_module.location)
 
         # now query get_items() to get this location with revision=None, this should just
         # return back a single item (not 2)
 
-        items = store.get_items(['i4x', 'edX', 'simple', 'html', 'test_html', None])
+        items = store.get_items(Location('i4x', 'edX', 'simple', 'html', 'test_html', None))
         self.assertEqual(len(items), 1)
         self.assertFalse(getattr(items[0], 'is_draft', False))
 
         # now refetch from the draft store. Note that even though we pass
         # None in the revision field, the draft store will replace that with 'draft'
-        items = draft_store.get_items(['i4x', 'edX', 'simple', 'html', 'test_html', None])
+        items = draft_store.get_items(Location('i4x', 'edX', 'simple', 'html', 'test_html', None))
         self.assertEqual(len(items), 1)
         self.assertTrue(getattr(items[0], 'is_draft', False))
 
@@ -229,9 +229,9 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         draft_store = modulestore('draft')
         import_from_xml(store, 'common/test/data/', ['simple'])
 
-        course = draft_store.get_item(Location(['i4x', 'edX', 'simple',
-                                               'course', '2012_Fall', None]), depth=None)
-        html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None])
+        course = draft_store.get_item(Location('i4x', 'edX', 'simple',
+                                               'course', '2012_Fall', None), depth=None)
+        html_module = draft_store.get_item(Location('i4x', 'edX', 'simple', 'html', 'test_html', None))
 
         self.assertEqual(html_module.graceperiod, course.graceperiod)
         self.assertNotIn('graceperiod', own_metadata(html_module))
@@ -239,7 +239,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         draft_store.convert_to_draft(html_module.location)
 
         # refetch to check metadata
-        html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None])
+        html_module = draft_store.get_item(Location('i4x', 'edX', 'simple', 'html', 'test_html', None))
 
         self.assertEqual(html_module.graceperiod, course.graceperiod)
         self.assertNotIn('graceperiod', own_metadata(html_module))
@@ -248,14 +248,14 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         draft_store.publish(html_module.location, 0)
 
         # refetch to check metadata
-        html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None])
+        html_module = draft_store.get_item(Location('i4x', 'edX', 'simple', 'html', 'test_html', None))
 
         self.assertEqual(html_module.graceperiod, course.graceperiod)
         self.assertNotIn('graceperiod', own_metadata(html_module))
 
         # put back in draft and change metadata and see if it's now marked as 'own_metadata'
         draft_store.convert_to_draft(html_module.location)
-        html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None])
+        html_module = draft_store.get_item(Location('i4x', 'edX', 'simple', 'html', 'test_html', None))
 
         new_graceperiod = timedelta(hours=1)
 
@@ -270,7 +270,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         draft_store.update_metadata(html_module.location, own_metadata(html_module))
 
         # read back to make sure it reads as 'own-metadata'
-        html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None])
+        html_module = draft_store.get_item(Location('i4x', 'edX', 'simple', 'html', 'test_html', None))
 
         self.assertIn('graceperiod', own_metadata(html_module))
         self.assertEqual(html_module.graceperiod, new_graceperiod)
@@ -280,7 +280,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         # and re-read and verify 'own-metadata'
         draft_store.convert_to_draft(html_module.location)
-        html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None])
+        html_module = draft_store.get_item(Location('i4x', 'edX', 'simple', 'html', 'test_html', None))
 
         self.assertIn('graceperiod', own_metadata(html_module))
         self.assertEqual(html_module.graceperiod, new_graceperiod)
@@ -289,7 +289,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         import_from_xml(modulestore('direct'), 'common/test/data/', ['simple'])
 
         course = modulestore('draft').get_item(
-            Location(['i4x', 'edX', 'simple', 'course', '2012_Fall', None]),
+            Location('i4x', 'edX', 'simple', 'course', '2012_Fall', None),
             depth=None
         )
 
@@ -298,7 +298,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         self.assertEqual(num_drafts, 0)
 
         problem = modulestore('draft').get_item(
-            Location(['i4x', 'edX', 'simple', 'problem', 'ps01-simple', None])
+            Location('i4x', 'edX', 'simple', 'problem', 'ps01-simple', None)
         )
 
         # put into draft
@@ -306,13 +306,13 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         # make sure we can query that item and verify that it is a draft
         draft_problem = modulestore('draft').get_item(
-            Location(['i4x', 'edX', 'simple', 'problem', 'ps01-simple', None])
+            Location('i4x', 'edX', 'simple', 'problem', 'ps01-simple', None)
         )
         self.assertTrue(getattr(draft_problem, 'is_draft', False))
 
         # now requery with depth
         course = modulestore('draft').get_item(
-            Location(['i4x', 'edX', 'simple', 'course', '2012_Fall', None]),
+            Location('i4x', 'edX', 'simple', 'course', '2012_Fall', None),
             depth=None
         )
 
@@ -324,10 +324,10 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         module_store = modulestore('direct')
         import_from_xml(module_store, 'common/test/data/', ['toy'])
 
-        handouts = module_store.get_item(Location(['i4x', 'edX', 'toy', 'course_info', 'handouts', None]))
+        handouts = module_store.get_item(Location('i4x', 'edX', 'toy', 'course_info', 'handouts', None))
         self.assertIn('/static/', handouts.data)
 
-        handouts = module_store.get_item(Location(['i4x', 'edX', 'toy', 'html', 'toyhtml', None]))
+        handouts = module_store.get_item(Location('i4x', 'edX', 'toy', 'html', 'toyhtml', None))
         self.assertIn('/static/', handouts.data)
 
     @mock.patch('xmodule.course_module.requests.get')
@@ -341,14 +341,14 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         module_store = modulestore('direct')
         import_from_xml(module_store, 'common/test/data/', ['toy'])
 
-        course = module_store.get_item(Location(['i4x', 'edX', 'toy', 'course', '2012_Fall', None]))
+        course = module_store.get_item(Location('i4x', 'edX', 'toy', 'course', '2012_Fall', None))
 
         self.assertGreater(len(course.textbooks), 0)
 
     def test_default_tabs_on_create_course(self):
         module_store = modulestore('direct')
         CourseFactory.create(org='edX', course='999', display_name='Robot Super Course')
-        course_location = Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None])
+        course_location = Location('i4x', 'edX', '999', 'course', 'Robot_Super_Course', None)
 
         course = module_store.get_item(course_location)
 
@@ -365,7 +365,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
     def test_create_static_tab_and_rename(self):
         module_store = modulestore('direct')
         CourseFactory.create(org='edX', course='999', display_name='Robot Super Course')
-        course_location = Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None])
+        course_location = Location('i4x', 'edX', '999', 'course', 'Robot_Super_Course', None)
 
         item = ItemFactory.create(parent_location=course_location, category='static_tab', display_name="My Tab")
 
@@ -455,7 +455,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         """ Creates two static tabs in a dummy course. """
         module_store = modulestore('direct')
         CourseFactory.create(org='edX', course='999', display_name='Robot Super Course')
-        course_location = Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None])
+        course_location = Location('i4x', 'edX', '999', 'course', 'Robot_Super_Course', None)
         new_location = loc_mapper().translate_location(course_location.course_id, course_location, False, True)
 
         ItemFactory.create(
@@ -473,7 +473,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         module_store = modulestore('direct')
         import_from_xml(module_store, 'common/test/data/', ['toy'])
 
-        items = module_store.get_items(['i4x', 'edX', 'toy', 'poll_question', None, None])
+        items = module_store.get_items(Location('i4x', 'edX', 'toy', 'poll_question', None, None))
         found = len(items) > 0
 
         self.assertTrue(found)
@@ -489,7 +489,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         """
         Tests the ajax callback to render an XModule
         """
-        resp = self._test_preview(Location(['i4x', 'edX', 'toy', 'vertical', 'vertical_test', None]))
+        resp = self._test_preview(Location('i4x', 'edX', 'toy', 'vertical', 'vertical_test', None))
         # These are the data-ids of the xblocks contained in the vertical.
         # Ultimately, these must be converted to new locators.
         self.assertContains(resp, 'i4x://edX/toy/video/sample_video')
@@ -501,7 +501,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         """
         This verifies that a video caption url is as we expect it to be
         """
-        resp = self._test_preview(Location(['i4x', 'edX', 'toy', 'video', 'sample_video', None]))
+        resp = self._test_preview(Location('i4x', 'edX', 'toy', 'video', 'sample_video', None))
         self.assertContains(resp, 'data-caption-asset-path="/c4x/edX/toy/asset/subs_"')
 
     def _test_preview(self, location):
@@ -522,13 +522,13 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
     def test_delete(self):
         direct_store = modulestore('direct')
         CourseFactory.create(org='edX', course='999', display_name='Robot Super Course')
-        course_location = Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None])
+        course_location = Location('i4x', 'edX', '999', 'course', 'Robot_Super_Course', None)
 
         chapterloc = ItemFactory.create(parent_location=course_location, display_name="Chapter").location
         ItemFactory.create(parent_location=chapterloc, category='sequential', display_name="Sequential")
 
-        sequential = direct_store.get_item(Location(['i4x', 'edX', '999', 'sequential', 'Sequential', None]))
-        chapter = direct_store.get_item(Location(['i4x', 'edX', '999', 'chapter', 'Chapter', None]))
+        sequential = direct_store.get_item(Location('i4x', 'edX', '999', 'sequential', 'Sequential', None))
+        chapter = direct_store.get_item(Location('i4x', 'edX', '999', 'chapter', 'Chapter', None))
 
         # make sure the parent points to the child object which is to be deleted
         self.assertTrue(sequential.location.url() in chapter.children)
@@ -1124,7 +1124,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         # create a new video module and add it as a child to a vertical
         # this re-creates a bug whereby since the video template doesn't have
         # anything in 'data' field, the export was blowing up
-        verticals = module_store.get_items(['i4x', 'edX', 'toy', 'vertical', None, None])
+        verticals = module_store.get_items(Location('i4x', 'edX', 'toy', 'vertical', None, None))
 
         self.assertGreater(len(verticals), 0)
 
@@ -1152,7 +1152,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         import_from_xml(module_store, 'common/test/data/', ['word_cloud'])
         location = CourseDescriptor.id_to_location('HarvardX/ER22x/2013_Spring')
 
-        verticals = module_store.get_items(['i4x', 'HarvardX', 'ER22x', 'vertical', None, None])
+        verticals = module_store.get_items(Location('i4x', 'HarvardX', 'ER22x', 'vertical', None, None))
 
         self.assertGreater(len(verticals), 0)
 
@@ -1181,7 +1181,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         import_from_xml(module_store, 'common/test/data/', ['toy'])
         location = CourseDescriptor.id_to_location('edX/toy/2012_Fall')
 
-        verticals = module_store.get_items(['i4x', 'edX', 'toy', 'vertical', None, None])
+        verticals = module_store.get_items(Location('i4x', 'edX', 'toy', 'vertical', None, None))
 
         self.assertGreater(len(verticals), 0)
 
@@ -1198,7 +1198,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         # Reimport and get the video back
         import_from_xml(module_store, root_dir)
-        imported_word_cloud = module_store.get_item(Location(['i4x', 'edX', 'toy', 'word_cloud', 'untitled', None]))
+        imported_word_cloud = module_store.get_item(Location('i4x', 'edX', 'toy', 'word_cloud', 'untitled', None))
 
         # It should now contain empty data
         self.assertEquals(imported_word_cloud.data, '')
@@ -1224,14 +1224,14 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         # get the sample HTML with styling information
         html_module = module_store.get_instance(
             'edX/toy/2012_Fall',
-            Location(['i4x', 'edX', 'toy', 'html', 'with_styling'])
+            Location('i4x', 'edX', 'toy', 'html', 'with_styling')
         )
         self.assertIn('<p style="font:italic bold 72px/30px Georgia, serif; color: red; ">', html_module.data)
 
         # get the sample HTML with just a simple <img> tag information
         html_module = module_store.get_instance(
             'edX/toy/2012_Fall',
-            Location(['i4x', 'edX', 'toy', 'html', 'just_img'])
+            Location('i4x', 'edX', 'toy', 'html', 'just_img')
         )
         self.assertIn('<img src="/static/foo_bar.jpg" />', html_module.data)
 
@@ -1790,7 +1790,7 @@ class ContentStoreTest(ModuleStoreTestCase):
 
         course = module_store.get_item(Location(['i4x', 'edX', 'toy', 'course', '2012_Fall', None]))
 
-        verticals = module_store.get_items(['i4x', 'edX', 'toy', 'vertical', None, None])
+        verticals = module_store.get_items(Location('i4x', 'edX', 'toy', 'vertical', None, None))
 
         # let's assert on the metadata_inheritance on an existing vertical
         for vertical in verticals:

cms/djangoapps/contentstore/tests/test_utils.py

 """ Tests for utils. """
 from contentstore import utils
 import mock
-import unittest
 import collections
 import copy
-import json
-from uuid import uuid4
 
 from django.test import TestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 from django.test.utils import override_settings
-from xmodule.modulestore.tests.factories import CourseFactory
 
-from xmodule.contentstore.content import StaticContent
-from xmodule.contentstore.django import contentstore
-from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
-from xmodule.exceptions import NotFoundError
+from xmodule.modulestore import Location
 
 
 class LMSLinksTestCase(TestCase):
@@ -64,12 +57,12 @@ class LMSLinksTestCase(TestCase):
 
     def get_about_page_link(self):
         """ create mock course and return the about page link """
-        location = 'i4x', 'mitX', '101', 'course', 'test'
+        location = Location('i4x', 'mitX', '101', 'course', 'test')
         return utils.get_lms_link_for_about_page(location)
 
     def lms_link_test(self):
         """ Tests get_lms_link_for_item. """
-        location = 'i4x', 'mitX', '101', 'vertical', 'contacting_us'
+        location = Location('i4x', 'mitX', '101', 'vertical', 'contacting_us')
         link = utils.get_lms_link_for_item(location, False, "mitX/101/test")
         self.assertEquals(link, "//localhost:8000/courses/mitX/101/test/jump_to/i4x://mitX/101/vertical/contacting_us")
         link = utils.get_lms_link_for_item(location, True, "mitX/101/test")
@@ -80,7 +73,7 @@ class LMSLinksTestCase(TestCase):
 
         # If no course_id is passed in, it is obtained from the location. This is the case for
         # Studio dashboard.
-        location = 'i4x', 'mitX', '101', 'course', 'test'
+        location = Location('i4x', 'mitX', '101', 'course', 'test')
         link = utils.get_lms_link_for_item(location)
         self.assertEquals(
             link,

cms/djangoapps/contentstore/utils.py

@@ -79,7 +79,7 @@ def get_course_location_for_item(location):
         # @hack! We need to find the course location however, we don't
         # know the 'name' parameter in this context, so we have
         # to assume there's only one item in this query even though we are not specifying a name
-        course_search_location = ['i4x', item_loc.org, item_loc.course, 'course', None]
+        course_search_location = Location('i4x', item_loc.org, item_loc.course, 'course', None)
         courses = modulestore().get_items(course_search_location)
 
         # make sure we found exactly one match on this above course search
@@ -107,7 +107,7 @@ def get_course_for_item(location):
     # @hack! We need to find the course location however, we don't
     # know the 'name' parameter in this context, so we have
     # to assume there's only one item in this query even though we are not specifying a name
-    course_search_location = ['i4x', item_loc.org, item_loc.course, 'course', None]
+    course_search_location = Location('i4x', item_loc.org, item_loc.course, 'course', None)
     courses = modulestore().get_items(course_search_location)
 
     # make sure we found exactly one match on this above course search

cms/djangoapps/contentstore/views/access.py

 from auth.authz import STAFF_ROLE_NAME, INSTRUCTOR_ROLE_NAME
 from auth.authz import is_user_in_course_group_role
-from django.core.exceptions import PermissionDenied
 from ..utils import get_course_location_for_item
-from xmodule.modulestore import Location
 from xmodule.modulestore.locator import CourseLocator
 
 
-def get_location_and_verify_access(request, org, course, name):
-    """
-    Create the location, verify that the user has permissions
-    to view the location. Returns the location as a Location
-    """
-    location = ['i4x', org, course, 'course', name]
-
-    # check that logged in user has permissions to this item
-    if not has_access(request.user, location):
-        raise PermissionDenied()
-
-    return Location(location)
-
-
 def has_access(user, location, role=STAFF_ROLE_NAME):
-    '''
+    """
     Return True if user allowed to access this piece of data
     Note that the CMS permissions model is with respect to courses
     There is a super-admin permissions if user.is_staff is set
@@ -29,7 +13,7 @@ def has_access(user, location, role=STAFF_ROLE_NAME):
     I'm presuming that the course instructor (formally known as admin)
     will not be in both INSTRUCTOR and STAFF groups, so we have to cascade our
     queries here as INSTRUCTOR has all the rights that STAFF do
-    '''
+    """
     if not isinstance(location, CourseLocator):
         location = get_course_location_for_item(location)
     _has_access = is_user_in_course_group_role(user, location, role)

cms/djangoapps/contentstore/views/course.py

@@ -128,7 +128,7 @@ def course_listing(request):
     """
     List all courses available to the logged in user
     """
-    courses = modulestore('direct').get_items(['i4x', None, None, 'course', None])
+    courses = modulestore('direct').get_items(Location('i4x', None, None, 'course', None))
 
     # filter out courses that we don't have access too
     def course_filter(course):