Merge pull request #1728 from edx/christina/components

Convert component.py to use RESTful URLs.

CHANGELOG.rst

@@ -7,6 +7,10 @@ the top.  Include a label indicating the component affected.
 
 Blades: Enabled several Video Jasmine tests. BLD-463.
 
+Studio: Continued modification of Studio pages to follow a RESTful framework.
+includes Settings pages, edit page for Subsection and Unit, and interfaces
+for updating xblocks (xmodules) and getting their editing HTML.
+
 Blades: Put 2nd "Hide output" button at top of test box & increase text size for
 code response questions. BLD-126.
 

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -42,6 +42,7 @@ from xmodule.capa_module import CapaDescriptor
 from xmodule.course_module import CourseDescriptor
 from xmodule.seq_module import SequenceDescriptor
 from xmodule.modulestore.exceptions import ItemNotFoundError
+from xmodule.modulestore.locator import BlockUsageLocator
 
 from contentstore.views.component import ADVANCED_COMPONENT_TYPES
 from xmodule.exceptions import NotFoundError
@@ -133,10 +134,10 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         # just pick one vertical
         descriptor = store.get_items(Location('i4x', 'edX', 'simple', 'vertical', None, None))[0]
-
-        resp = self.client.get_html(reverse('edit_unit', kwargs={'location': descriptor.location.url()}))
+        locator = loc_mapper().translate_location(course.location.course_id, descriptor.location, False, True)
+        resp = self.client.get_html(locator.url_reverse('unit'))
         self.assertEqual(resp.status_code, 200)
-        # TODO: uncomment after edit_unit no longer using locations.
+        # TODO: uncomment when video transcripts no longer require IDs.
         # _test_no_locations(self, resp)
 
         for expected in expected_types:
@@ -155,29 +156,22 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
     def test_malformed_edit_unit_request(self):
         store = modulestore('direct')
-        import_from_xml(store, 'common/test/data/', ['simple'])
+        _, course_items = import_from_xml(store, 'common/test/data/', ['simple'])
 
         # just pick one vertical
         descriptor = store.get_items(Location('i4x', 'edX', 'simple', 'vertical', None, None))[0]
         location = descriptor.location.replace(name='.' + descriptor.location.name)
+        locator = loc_mapper().translate_location(course_items[0].location.course_id, location, False, True)
 
-        resp = self.client.get_html(reverse('edit_unit', kwargs={'location': location.url()}))
+        resp = self.client.get_html(locator.url_reverse('unit'))
         self.assertEqual(resp.status_code, 400)
         _test_no_locations(self, resp, status_code=400)
 
     def check_edit_unit(self, test_course_name):
-        import_from_xml(modulestore('direct'), 'common/test/data/', [test_course_name])
+        _, course_items = import_from_xml(modulestore('direct'), 'common/test/data/', [test_course_name])
 
         items = modulestore().get_items(Location('i4x', 'edX', test_course_name, 'vertical', None, None))
-        # Assert is here to make sure that the course being tested actually has verticals.
-        self.assertGreater(len(items), 0)
-        for descriptor in items:
-            print "Checking ", descriptor.location.url()
-            print descriptor.__class__, descriptor.location
-            resp = self.client.get_html(reverse('edit_unit', kwargs={'location': descriptor.location.url()}))
-            self.assertEqual(resp.status_code, 200)
-            # TODO: uncomment after edit_unit not using locations.
-            # _test_no_locations(self, resp)
+        self._check_verticals(items, course_items[0].location.course_id)
 
     def _lock_an_asset(self, content_store, course_location):
         """
@@ -1065,14 +1059,11 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
             target_location_namespace=course_location
         )
 
+        # Unit test fails in Jenkins without this.
+        loc_mapper().translate_location(course_location.course_id, course_location, False, True)
+
         items = module_store.get_items(stub_location.replace(category='vertical', name=None))
-        self.assertGreater(len(items), 0)
-        for descriptor in items:
-            print "Checking {0}....".format(descriptor.location.url())
-            resp = self.client.get_html(reverse('edit_unit', kwargs={'location': descriptor.location.url()}))
-            self.assertEqual(resp.status_code, 200)
-            # TODO: uncomment when edit_unit no longer has locations.
-            # _test_no_locations(self, resp)
+        self._check_verticals(items, course_location.course_id)
 
         # verify that we have the content in the draft store as well
         vertical = draft_store.get_item(
@@ -1355,6 +1346,17 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         items = module_store.get_items(stub_location)
         self.assertEqual(len(items), 1)
 
+    def _check_verticals(self, items, course_id):
+        """ Test getting the editing HTML for each vertical. """
+        # Assert is here to make sure that the course being tested actually has verticals (units) to check.
+        self.assertGreater(len(items), 0)
+        for descriptor in items:
+            unit_locator = loc_mapper().translate_location(course_id, descriptor.location, False, True)
+            resp = self.client.get_html(unit_locator.url_reverse('unit'))
+            self.assertEqual(resp.status_code, 200)
+            # TODO: uncomment when video transcripts no longer require IDs.
+            # _test_no_locations(self, resp)
+
 
 @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE, MODULESTORE=TEST_MODULESTORE)
 class ContentStoreTest(ModuleStoreTestCase):
@@ -1598,12 +1600,13 @@ class ContentStoreTest(ModuleStoreTestCase):
         }
 
         resp = self.client.ajax_post('/xblock', section_data)
+        _test_no_locations(self, resp, html=False)
 
         self.assertEqual(resp.status_code, 200)
         data = parse_json(resp)
         self.assertRegexpMatches(
-            data['id'],
-            r"^i4x://MITx/999/chapter/([0-9]|[a-f]){32}$"
+            data['locator'],
+            r"^MITx.999.Robot_Super_Course/branch/draft/block/chapter([0-9]|[a-f]){3}$"
         )
 
     def test_capa_module(self):
@@ -1619,7 +1622,7 @@ class ContentStoreTest(ModuleStoreTestCase):
 
         self.assertEqual(resp.status_code, 200)
         payload = parse_json(resp)
-        problem_loc = Location(payload['id'])
+        problem_loc = loc_mapper().translate_locator_to_location(BlockUsageLocator(payload['locator']))
         problem = get_modulestore(problem_loc).get_item(problem_loc)
         # should be a CapaDescriptor
         self.assertIsInstance(problem, CapaDescriptor, "New problem is not a CapaDescriptor")
@@ -1677,19 +1680,17 @@ class ContentStoreTest(ModuleStoreTestCase):
 
         # go look at a subsection page
         subsection_location = loc.replace(category='sequential', name='test_sequence')
-        resp = self.client.get_html(
-            reverse('edit_subsection', kwargs={'location': subsection_location.url()})
-        )
+        subsection_locator = loc_mapper().translate_location(loc.course_id, subsection_location, False, True)
+        resp = self.client.get_html(subsection_locator.url_reverse('subsection'))
         self.assertEqual(resp.status_code, 200)
-        # TODO: uncomment when grading and outline not using old locations.
-        # _test_no_locations(self, resp)
+        _test_no_locations(self, resp)
 
         # go look at the Edit page
         unit_location = loc.replace(category='vertical', name='test_vertical')
-        resp = self.client.get_html(
-            reverse('edit_unit', kwargs={'location': unit_location.url()}))
+        unit_locator = loc_mapper().translate_location(loc.course_id, unit_location, False, True)
+        resp = self.client.get_html(unit_locator.url_reverse('unit'))
         self.assertEqual(resp.status_code, 200)
-        # TODO: uncomment when edit_unit not using old locations.
+        # TODO: uncomment when video transcripts no longer require IDs.
         # _test_no_locations(self, resp)
 
         def delete_item(category, name):
@@ -1899,8 +1900,7 @@ class ContentStoreTest(ModuleStoreTestCase):
         """
         new_location = loc_mapper().translate_location(location.course_id, location, False, True)
         resp = self.client.get_html(new_location.url_reverse('course/', ''))
-        # TODO: uncomment when i4x no longer in overview.
-        # _test_no_locations(self, resp)
+        _test_no_locations(self, resp)
         return resp
 
 
@@ -2033,7 +2033,5 @@ def _test_no_locations(test, resp, status_code=200, html=True):
         # it checks that the HTML properly parses. However, it won't find i4x usages
         # in JavaScript blocks.
         content = resp.content
-        num_jump_to = len(re.findall(r"8000(\S)*jump_to/i4x", content))
-        total_i4x = len(re.findall(r"i4x", content))
-
-        test.assertEqual(total_i4x - num_jump_to, 0, "i4x found outside of LMS jump-to links")
+        hits = len(re.findall(r"(?<!jump_to/)i4x://", content))
+        test.assertEqual(hits, 0, "i4x found outside of LMS jump-to links")

cms/djangoapps/contentstore/tests/test_import_export.py

@@ -263,7 +263,7 @@ class ExportTestCase(CourseTestCase):
             parent_location=vertical.location,
             category='aawefawef'
         )
-        self._verify_export_failure('/edit/i4x://MITx/999/vertical/foo')
+        self._verify_export_failure(u'/unit/MITx.999.Robot_Super_Course/branch/draft/block/foo')
 
     def _verify_export_failure(self, expectedText):
         """ Export failure helper method. """

cms/djangoapps/contentstore/tests/test_item.py

@@ -9,6 +9,7 @@ from xmodule.capa_module import CapaDescriptor
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.django import loc_mapper
 from xmodule.modulestore.locator import BlockUsageLocator
+from xmodule.modulestore.exceptions import ItemNotFoundError
 
 
 class ItemTest(CourseTestCase):
@@ -30,7 +31,7 @@ class ItemTest(CourseTestCase):
         """
         Get the item referenced by the locator from the modulestore
         """
-        store = modulestore('draft') if draft else modulestore()
+        store = modulestore('draft') if draft else modulestore('direct')
         return store.get_item(self.get_old_id(locator))
 
     def response_locator(self, response):
@@ -251,3 +252,105 @@ class TestEditItem(ItemTest):
         self.assertEqual(self.get_old_id(self.problem_locator).url(), children[0])
         self.assertEqual(self.get_old_id(unit1_locator).url(), children[2])
         self.assertEqual(self.get_old_id(unit2_locator).url(), children[1])
+
+    def test_make_public(self):
+        """ Test making a private problem public (publishing it). """
+        # When the problem is first created, it is only in draft (because of its category).
+        with self.assertRaises(ItemNotFoundError):
+            self.get_item_from_modulestore(self.problem_locator, False)
+        self.client.ajax_post(
+            self.problem_update_url,
+            data={'publish': 'make_public'}
+        )
+        self.assertIsNotNone(self.get_item_from_modulestore(self.problem_locator, False))
+
+    def test_make_private(self):
+        """ Test making a public problem private (un-publishing it). """
+        # Make problem public.
+        self.client.ajax_post(
+            self.problem_update_url,
+            data={'publish': 'make_public'}
+        )
+        self.assertIsNotNone(self.get_item_from_modulestore(self.problem_locator, False))
+        # Now make it private
+        self.client.ajax_post(
+            self.problem_update_url,
+            data={'publish': 'make_private'}
+        )
+        with self.assertRaises(ItemNotFoundError):
+            self.get_item_from_modulestore(self.problem_locator, False)
+
+    def test_make_draft(self):
+        """ Test creating a draft version of a public problem. """
+        # Make problem public.
+        self.client.ajax_post(
+            self.problem_update_url,
+            data={'publish': 'make_public'}
+        )
+        self.assertIsNotNone(self.get_item_from_modulestore(self.problem_locator, False))
+        # Now make it draft, which means both versions will exist.
+        self.client.ajax_post(
+            self.problem_update_url,
+            data={'publish': 'create_draft'}
+        )
+        # Update the draft version and check that published is different.
+        self.client.ajax_post(
+            self.problem_update_url,
+            data={'metadata': {'due': '2077-10-10T04:00Z'}}
+        )
+        published = self.get_item_from_modulestore(self.problem_locator, False)
+        self.assertIsNone(published.due)
+        draft = self.get_item_from_modulestore(self.problem_locator, True)
+        self.assertEqual(draft.due, datetime.datetime(2077, 10, 10, 4, 0, tzinfo=UTC))
+
+    def test_make_public_with_update(self):
+        """ Update a problem and make it public at the same time. """
+        self.client.ajax_post(
+            self.problem_update_url,
+            data={
+                'metadata': {'due': '2077-10-10T04:00Z'},
+                'publish': 'make_public'
+            }
+        )
+        published = self.get_item_from_modulestore(self.problem_locator, False)
+        self.assertEqual(published.due, datetime.datetime(2077, 10, 10, 4, 0, tzinfo=UTC))
+
+    def test_make_private_with_update(self):
+        """ Make a problem private and update it at the same time. """
+        # Make problem public.
+        self.client.ajax_post(
+            self.problem_update_url,
+            data={'publish': 'make_public'}
+        )
+        self.client.ajax_post(
+            self.problem_update_url,
+            data={
+                'metadata': {'due': '2077-10-10T04:00Z'},
+                'publish': 'make_private'
+            }
+        )
+        with self.assertRaises(ItemNotFoundError):
+            self.get_item_from_modulestore(self.problem_locator, False)
+        draft = self.get_item_from_modulestore(self.problem_locator, True)
+        self.assertEqual(draft.due, datetime.datetime(2077, 10, 10, 4, 0, tzinfo=UTC))
+
+    def test_create_draft_with_update(self):
+        """ Create a draft and update it at the same time. """
+        # Make problem public.
+        self.client.ajax_post(
+            self.problem_update_url,
+            data={'publish': 'make_public'}
+        )
+        self.assertIsNotNone(self.get_item_from_modulestore(self.problem_locator, False))
+        # Now make it draft, which means both versions will exist.
+        self.client.ajax_post(
+            self.problem_update_url,
+            data={
+                'metadata': {'due': '2077-10-10T04:00Z'},
+                'publish': 'create_draft'
+            }
+        )
+        published = self.get_item_from_modulestore(self.problem_locator, False)
+        self.assertIsNone(published.due)
+        draft = self.get_item_from_modulestore(self.problem_locator, True)
+        self.assertEqual(draft.due, datetime.datetime(2077, 10, 10, 4, 0, tzinfo=UTC))

cms/djangoapps/contentstore/tests/test_transcripts.py

@@ -20,6 +20,7 @@ from xmodule.contentstore.django import contentstore, _CONTENTSTORE
 from xmodule.contentstore.content import StaticContent
 from xmodule.exceptions import NotFoundError
 from xmodule.modulestore.django import loc_mapper
+from xmodule.modulestore.locator import BlockUsageLocator
 
 from contentstore.tests.modulestore_config import TEST_MODULESTORE
 TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE)
@@ -59,7 +60,7 @@ class Basetranscripts(CourseTestCase):
             'type': 'video'
         }
         resp = self.client.ajax_post('/xblock', data)
-        self.item_location = json.loads(resp.content).get('id')
+        self.item_location = self._get_location(resp)
         self.assertEqual(resp.status_code, 200)
 
         # hI10vDNYz4M - valid Youtube ID with transcripts.
@@ -72,6 +73,11 @@ class Basetranscripts(CourseTestCase):
         # Remove all transcripts for current module.
         self.clear_subs_content()
 
+    def _get_location(self, resp):
+        """ Returns the location (as a string) from the response returned by a create operation. """
+        locator = json.loads(resp.content).get('locator')
+        return loc_mapper().translate_locator_to_location(BlockUsageLocator(locator)).url()
+
     def get_youtube_ids(self):
         """Return youtube speeds and ids."""
         item = modulestore().get_item(self.item_location)
@@ -205,7 +211,7 @@ class TestUploadtranscripts(Basetranscripts):
             'type': 'non_video'
         }
         resp = self.client.ajax_post('/xblock', data)
-        item_location = json.loads(resp.content).get('id')
+        item_location = self._get_location(resp)
         data = '<non_video youtube="0.75:JMD_ifUUfsU,1.0:hI10vDNYz4M" />'
         modulestore().update_item(item_location, data)
 
@@ -416,7 +422,7 @@ class TestDownloadtranscripts(Basetranscripts):
             'type': 'videoalpha'
         }
         resp = self.client.ajax_post('/xblock', data)
-        item_location = json.loads(resp.content).get('id')
+        item_location = self._get_location(resp)
         subs_id = str(uuid4())
         data = textwrap.dedent("""
             <videoalpha youtube="" sub="{}">
@@ -666,7 +672,7 @@ class TestChecktranscripts(Basetranscripts):
             'type': 'not_video'
         }
         resp = self.client.ajax_post('/xblock', data)
-        item_location = json.loads(resp.content).get('id')
+        item_location = self._get_location(resp)
         subs_id = str(uuid4())
         data = textwrap.dedent("""
             <not_video youtube="" sub="{}">

cms/djangoapps/contentstore/views/component.py

@@ -2,26 +2,27 @@ import json
 import logging
 from collections import defaultdict
 
-from django.http import HttpResponse, HttpResponseBadRequest
+from django.http import HttpResponseBadRequest
 from django.contrib.auth.decorators import login_required
+from django.views.decorators.http import require_http_methods
 from django.core.exceptions import PermissionDenied
+from django_future.csrf import ensure_csrf_cookie
 from django.conf import settings
-from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError
+from xmodule.modulestore.exceptions import ItemNotFoundError
 from mitxmako.shortcuts import render_to_response
 
-from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 from xmodule.util.date_utils import get_default_time_display
 from xmodule.modulestore.django import loc_mapper
+from xmodule.modulestore.locator import BlockUsageLocator
 
 from xblock.fields import Scope
-from util.json_request import expect_json
+from util.json_request import expect_json, JsonResponse
 
 from contentstore.utils import get_lms_link_for_item, compute_unit_state, UnitState, get_course_for_item
 
 from models.settings.course_grading import CourseGradingModel
 
-from .helpers import _xmodule_recurse
 from .access import has_access
 from xmodule.x_module import XModuleDescriptor
 from xblock.plugin import PluginMissingError
@@ -29,16 +30,13 @@ from xblock.runtime import Mixologist
 
 __all__ = ['OPEN_ENDED_COMPONENT_TYPES',
            'ADVANCED_COMPONENT_POLICY_KEY',
-           'edit_subsection',
-           'edit_unit',
-           'create_draft',
-           'publish_draft',
-           'unpublish_unit',
+           'subsection_handler',
+           'unit_handler'
            ]
 
 log = logging.getLogger(__name__)
 
-# NOTE: edit_unit assumes this list is disjoint from ADVANCED_COMPONENT_TYPES
+# NOTE: unit_handler assumes this list is disjoint from ADVANCED_COMPONENT_TYPES
 COMPONENT_TYPES = ['discussion', 'html', 'problem', 'video']
 
 OPEN_ENDED_COMPONENT_TYPES = ["combinedopenended", "peergrading"]
@@ -53,38 +51,37 @@ ADVANCED_COMPONENT_CATEGORY = 'advanced'
 ADVANCED_COMPONENT_POLICY_KEY = 'advanced_modules'
 
 
+@require_http_methods(["GET"])
 @login_required
-def edit_subsection(request, location):
-    "Edit the subsection of a course"
-    # check that we have permissions to edit this item
-    try:
-        course = get_course_for_item(location)
-    except InvalidLocationError:
-        return HttpResponseBadRequest()
-
-    if not has_access(request.user, course.location):
-        raise PermissionDenied()
+def subsection_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None):
+    """
+    The restful handler for subsection-specific requests.
 
+    GET
+        html: return html page for editing a subsection
+        json: not currently supported
+    """
+    if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'):
+        locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
         try:
-        item = modulestore().get_item(location, depth=1)
+            old_location, course, item, lms_link = _get_item_in_course(request, locator)
         except ItemNotFoundError:
             return HttpResponseBadRequest()
 
-    lms_link = get_lms_link_for_item(location, course_id=course.location.course_id)
-    preview_link = get_lms_link_for_item(location, course_id=course.location.course_id, preview=True)
+        preview_link = get_lms_link_for_item(old_location, course_id=course.location.course_id, preview=True)
 
         # make sure that location references a 'sequential', otherwise return
         # BadRequest
         if item.location.category != 'sequential':
             return HttpResponseBadRequest()
 
-    parent_locs = modulestore().get_parent_locations(location, None)
+        parent_locs = modulestore().get_parent_locations(old_location, None)
 
         # we're for now assuming a single parent
         if len(parent_locs) != 1:
             logging.error(
                 'Multiple (or none) parents have been found for %s',
-            location
+                unicode(locator)
             )
 
         # this should blow up if we don't find any parents, which would be erroneous
@@ -99,8 +96,7 @@ def edit_subsection(request, location):
             (field.name, field.read_from(item))
             for field
             in fields.values()
-        if field.name not in ['display_name', 'start', 'due', 'format']
-        and field.scope == Scope.settings
+            if field.name not in ['display_name', 'start', 'due', 'format'] and field.scope == Scope.settings
         )
 
         can_view_live = False
@@ -114,9 +110,6 @@ def edit_subsection(request, location):
         course_locator = loc_mapper().translate_location(
             course.location.course_id, course.location, False, True
         )
-    locator = loc_mapper().translate_location(
-        course.location.course_id, item.location, False, True
-    )
 
         return render_to_response(
             'edit_subsection.html',
@@ -134,9 +127,11 @@ def edit_subsection(request, location):
                 'can_view_live': can_view_live
             }
         )
+    else:
+        return HttpResponseBadRequest("Only supports html requests")
 
 
-def load_mixed_class(category):
+def _load_mixed_class(category):
     """
     Load an XBlock by category name, and apply all defined mixins
     """
@@ -145,41 +140,26 @@ def load_mixed_class(category):
     return mixologist.mix(component_class)
 
 
+@require_http_methods(["GET"])
 @login_required
-def edit_unit(request, location):
+def unit_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None):
     """
-    Display an editing page for the specified module.
+    The restful handler for unit-specific requests.
 
-    Expects a GET request with the parameter `id`.
-
-    id: A Location URL
+    GET
+        html: return html page for editing a unit
+        json: not currently supported
     """
+    if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'):
+        locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
         try:
-        course = get_course_for_item(location)
-    except InvalidLocationError:
-        return HttpResponseBadRequest()
-
-    if not has_access(request.user, course.location):
-        raise PermissionDenied()
-
-    try:
-        item = modulestore().get_item(location, depth=1)
+            old_location, course, item, lms_link = _get_item_in_course(request, locator)
         except ItemNotFoundError:
             return HttpResponseBadRequest()
-    lms_link = get_lms_link_for_item(
-        item.location,
-        course_id=course.location.course_id
-    )
-
-    # Note that the unit_state (draft, public, private) does not match up with the published value
-    # passed to translate_location. The two concepts are different at this point.
-    unit_locator = loc_mapper().translate_location(
-        course.location.course_id, Location(location), False, True
-    )
 
         component_templates = defaultdict(list)
         for category in COMPONENT_TYPES:
-        component_class = load_mixed_class(category)
+            component_class = _load_mixed_class(category)
             # add the default template
             # TODO: Once mixins are defined per-application, rather than per-runtime,
             # this should use a cms mixed-in class. (cpennington)
@@ -219,14 +199,16 @@ def edit_unit(request, location):
                     # class? i.e., can an advanced have more than one entry in the
                     # menu? one for default and others for prefilled boilerplates?
                     try:
-                    component_class = load_mixed_class(category)
+                        component_class = _load_mixed_class(category)
 
-                    component_templates['advanced'].append((
+                        component_templates['advanced'].append(
+                            (
                                 component_class.display_name.default or category,
                                 category,
                                 False,
                                 None  # don't override default data
-                    ))
+                            )
+                        )
                     except PluginMissingError:
                         # dhm: I got this once but it can happen any time the
                         # course author configures an advanced component which does
@@ -242,6 +224,7 @@ def edit_unit(request, location):
 
         components = [
             [
+                # TODO: old location needed for video transcripts.
                 component.location.url(),
                 loc_mapper().translate_location(
                     course.location.course_id, component.location, False, True
@@ -255,7 +238,7 @@ def edit_unit(request, location):
         # this will need to change to check permissions correctly so as
         # to pick the correct parent subsection
 
-    containing_subsection_locs = modulestore().get_parent_locations(location, None)
+        containing_subsection_locs = modulestore().get_parent_locations(old_location, None)
         containing_subsection = modulestore().get_item(containing_subsection_locs[0])
         containing_section_locs = modulestore().get_parent_locations(
             containing_subsection.location, None
@@ -292,9 +275,7 @@ def edit_unit(request, location):
         return render_to_response('unit.html', {
             'context_course': course,
             'unit': item,
-        # Still needed for creating a draft.
-        'unit_location': location,
-        'unit_locator': unit_locator,
+            'unit_locator': locator,
             'components': components,
             'component_templates': component_templates,
             'draft_preview_link': preview_lms_link,
@@ -312,57 +293,25 @@ def edit_unit(request, location):
                 if item.published_date is not None else None
             ),
         })
+    else:
+        return HttpResponseBadRequest("Only supports html requests")
 
 
 @login_required
-@expect_json
-def create_draft(request):
-    "Create a draft"
-    location = request.json['id']
-
-    # check permissions for this user within this course
-    if not has_access(request.user, location):
-        raise PermissionDenied()
-
-    # This clones the existing item location to a draft location (the draft is
-    # implicit, because modulestore is a Draft modulestore)
-    modulestore().convert_to_draft(location)
-
-    return HttpResponse()
-
-
-@login_required
-@expect_json
-def publish_draft(request):
-    """
-    Publish a draft
+def _get_item_in_course(request, locator):
     """
-    location = request.json['id']
+    Helper method for getting the old location, containing course,
+    item, and lms_link for a given locator.
 
-    # check permissions for this user within this course
-    if not has_access(request.user, location):
-        raise PermissionDenied()
-
-    item = modulestore().get_item(location)
-    _xmodule_recurse(
-        item,
-        lambda i: modulestore().publish(i.location, request.user.id)
-    )
-
-    return HttpResponse()
-
-
-@login_required
-@expect_json
-def unpublish_unit(request):
-    "Unpublish a unit"
-    location = request.json['id']
-
-    # check permissions for this user within this course
-    if not has_access(request.user, location):
+    Verifies that the caller has permission to access this item.
+    """
+    if not has_access(request.user, locator):
         raise PermissionDenied()
 
-    item = modulestore().get_item(location)
-    _xmodule_recurse(item, lambda i: modulestore().unpublish(i.location))
+    old_location = loc_mapper().translate_locator_to_location(locator)
+    course_location = loc_mapper().translate_locator_to_location(locator, True)
+    course = modulestore().get_item(course_location)
+    item = modulestore().get_item(old_location, depth=1)
+    lms_link = get_lms_link_for_item(old_location, course_id=course.location.course_id)
 
-    return HttpResponse()
+    return old_location, course, item, lms_link

cms/djangoapps/contentstore/views/import_export.py

@@ -14,7 +14,6 @@ from django.conf import settings
 from django.http import HttpResponse
 from django.contrib.auth.decorators import login_required
 from django_future.csrf import ensure_csrf_cookie
-from django.core.urlresolvers import reverse
 from django.core.servers.basehttp import FileWrapper
 from django.core.files.temp import NamedTemporaryFile
 from django.core.exceptions import SuspiciousOperation, PermissionDenied
@@ -140,7 +139,7 @@ def import_handler(request, tag=None, course_id=None, branch=None, version_guid=
                                   "size": size,
                                   "deleteUrl": "",
                                   "deleteType": "",
-                                  "url": location.url_reverse('import/', ''),
+                                  "url": location.url_reverse('import'),
                                   "thumbnailUrl": ""
                               }]
                 })
@@ -252,8 +251,8 @@ def import_handler(request, tag=None, course_id=None, branch=None, version_guid=
         course_module = modulestore().get_item(old_location)
         return render_to_response('import.html', {
             'context_course': course_module,
-            'successful_import_redirect_url': location.url_reverse("course/", ""),
-            'import_status_url': location.url_reverse("import_status/", "fillerName"),
+            'successful_import_redirect_url': location.url_reverse("course"),
+            'import_status_url': location.url_reverse("import_status", "fillerName"),
         })
     else:
         return HttpResponseNotFound()
@@ -313,7 +312,7 @@ def export_handler(request, tag=None, course_id=None, branch=None, version_guid=
     # an _accept URL parameter will be preferred over HTTP_ACCEPT in the header.
     requested_format = request.REQUEST.get('_accept', request.META.get('HTTP_ACCEPT', 'text/html'))
 
-    export_url = location.url_reverse('export/', '') + '?_accept=application/x-tgz'
+    export_url = location.url_reverse('export') + '?_accept=application/x-tgz'
     if 'application/x-tgz' in requested_format:
         name = old_location.name
         export_file = NamedTemporaryFile(prefix=name + '.', suffix=".tar.gz")
@@ -339,16 +338,16 @@ def export_handler(request, tag=None, course_id=None, branch=None, version_guid=
                 # if we have a nested exception, then we'll show the more generic error message
                 pass
 
+            unit_locator = loc_mapper().translate_location(old_location.course_id, parent.location, False, True)
+
             return render_to_response('export.html', {
                 'context_course': course_module,
                 'in_err': True,
                 'raw_err_msg': str(e),
                 'failed_module': failed_item,
                 'unit': unit,
-                'edit_unit_url': reverse('edit_unit', kwargs={
-                    'location': parent.location
-                }) if parent else '',
-                'course_home_url': location.url_reverse("course/", ""),
+                'edit_unit_url': unit_locator.url_reverse("unit") if parent else "",
+                'course_home_url': location.url_reverse("course"),
                 'export_url': export_url
 
             })
@@ -359,7 +358,7 @@ def export_handler(request, tag=None, course_id=None, branch=None, version_guid=
                 'in_err': True,
                 'unit': None,
                 'raw_err_msg': str(e),
-                'course_home_url': location.url_reverse("course/", ""),
+                'course_home_url': location.url_reverse("course"),
                 'export_url': export_url
             })
 

cms/djangoapps/contentstore/views/item.py

@@ -31,7 +31,6 @@ from django.http import HttpResponseBadRequest
 from xblock.fields import Scope
 from preview import handler_prefix, get_preview_html
 from mitxmako.shortcuts import render_to_response, render_to_string
-from django.views.decorators.csrf import ensure_csrf_cookie
 from models.settings.course_grading import CourseGradingModel
 
 __all__ = ['orphan_handler', 'xblock_handler']
@@ -68,6 +67,7 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid=
                        to None! Absent ones will be left alone.
                 :nullout: which metadata fields to set to None
                 :graderType: change how this unit is graded
+                :publish: can be one of three values, 'make_public, 'make_private', or 'create_draft'         
               The JSON representation on the updated xblock (minus children) is returned.
 
               if xblock locator is not specified, create a new xblock instance. The json playload can contain
@@ -118,13 +118,15 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid=
             return _delete_item_at_location(old_location, delete_children, delete_all_versions)
         else:  # Since we have a course_id, we are updating an existing xblock.
             return _save_item(
+                request,
                 locator,
                 old_location,
                 data=request.json.get('data'),
                 children=request.json.get('children'),
                 metadata=request.json.get('metadata'),
                 nullout=request.json.get('nullout'),
-                grader_type=request.json.get('graderType')
+                grader_type=request.json.get('graderType'),
+                publish=request.json.get('publish'),
             )
     elif request.method in ('PUT', 'POST'):
         return _create_item(request)
@@ -135,11 +137,10 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid=
         )
 
 
-def _save_item(usage_loc, item_location, data=None, children=None, metadata=None, nullout=None,
-        grader_type=None
-    ):
+def _save_item(request, usage_loc, item_location, data=None, children=None, metadata=None, nullout=None,
+               grader_type=None, publish=None):
     """
-    Saves xblock w/ its fields. Has special processing for grader_type and nullout and Nones in metadata.
+    Saves xblock w/ its fields. Has special processing for grader_type, publish, and nullout and Nones in metadata.
     nullout means to truly set the field to None whereas nones in metadata mean to unset them (so they revert
     to default).
 
@@ -161,6 +162,14 @@ def _save_item(usage_loc, item_location, data=None, children=None, metadata=None
         log.error("Can't find item by location.")
         return JsonResponse({"error": "Can't find item by location: " + str(item_location)}, 404)
 
+    if publish:
+        if publish == 'make_private':
+            _xmodule_recurse(existing_item, lambda i: modulestore().unpublish(i.location))
+        elif publish == 'create_draft':
+            # This clones the existing item location to a draft location (the draft is
+            # implicit, because modulestore is a Draft modulestore)
+            modulestore().convert_to_draft(item_location)
+
     if data:
         store.update_item(item_location, data)
     else:
@@ -213,9 +222,18 @@ def _save_item(usage_loc, item_location, data=None, children=None, metadata=None
         'data': data,
         'metadata': own_metadata(existing_item)
     }
+
     if grader_type is not None:
         result.update(CourseGradingModel.update_section_grader_type(existing_item, grader_type))
 
+    # Make public after updating the xblock, in case the caller asked
+    # for both an update and a publish.
+    if publish and publish == 'make_public':
+        _xmodule_recurse(
+            existing_item,
+            lambda i: modulestore().publish(i.location, request.user.id)
+        )
+
     # Note that children aren't being returned until we have a use case.
     return JsonResponse(result)
 
@@ -234,10 +252,7 @@ def _create_item(request):
         raise PermissionDenied()
 
     parent = get_modulestore(category).get_item(parent_location)
-    # Necessary to set revision=None or else metadata inheritance does not work
-    # (the ID with @draft will be used as the key in the inherited metadata map,
-    # and that is not expected by the code that later references it).
-    dest_location = parent_location.replace(category=category, name=uuid4().hex, revision=None)
+    dest_location = parent_location.replace(category=category, name=uuid4().hex)
 
     # get the metadata, display_name, and definition from the request
     metadata = {}
@@ -266,7 +281,7 @@ def _create_item(request):
 
     course_location = loc_mapper().translate_locator_to_location(parent_locator, get_course=True)
     locator = loc_mapper().translate_location(course_location.course_id, dest_location, False, True)
-    return JsonResponse({'id': dest_location.url(), "locator": unicode(locator)})
+    return JsonResponse({"locator": unicode(locator)})
 
 
 def _delete_item_at_location(item_location, delete_children=False, delete_all_versions=False):

cms/static/coffee/src/views/unit.coffee

@@ -166,7 +166,7 @@ define ["jquery", "jquery.ui", "gettext", "backbone",
       @wait(true)
       $.ajax({
           type: 'DELETE',
-          url: @model.urlRoot + "/" + @$el.data('locator') + "?" + $.param({recurse: true})
+          url: @model.url() + "?" + $.param({recurse: true})
       }).success(=>
 
           analytics.track "Deleted Draft",
@@ -179,8 +179,8 @@ define ["jquery", "jquery.ui", "gettext", "backbone",
     createDraft: (event) ->
       @wait(true)
 
-      $.postJSON('/create_draft', {
-        id: @$el.data('id')
+      $.postJSON(@model.url(), {
+        publish: 'create_draft'
       }, =>
         analytics.track "Created Draft",
           course: course_location_analytics
@@ -193,8 +193,8 @@ define ["jquery", "jquery.ui", "gettext", "backbone",
       @wait(true)
       @saveDraft()
 
-      $.postJSON('/publish_draft', {
-        id: @$el.data('id')
+      $.postJSON(@model.url(), {
+        publish: 'make_public'
       }, =>
         analytics.track "Published Draft",
           course: course_location_analytics
@@ -205,16 +205,16 @@ define ["jquery", "jquery.ui", "gettext", "backbone",
 
     setVisibility: (event) ->
       if @$('.visibility-select').val() == 'private'
-        target_url = '/unpublish_unit'
+        action = 'make_private'
         visibility = "private"
       else
-        target_url = '/publish_draft'
+        action = 'make_public'
         visibility = "public"
 
       @wait(true)
 
-      $.postJSON(target_url, {
-        id: @$el.data('id')
+      $.postJSON(@model.url(), {
+        publish: action
       }, =>
         analytics.track "Set Unit Visibility",
           course: course_location_analytics

cms/static/js/base.js

@@ -237,7 +237,7 @@ function createNewUnit(e) {
 
     function(data) {
         // redirect to the edit page
-        window.location = "/edit/" + data['id'];
+        window.location = "/unit/" + data['locator'];
     });
 }
 

cms/templates/overview.html

@@ -207,7 +207,7 @@ require(["domReady!", "jquery", "js/models/location", "js/models/section", "js/v
                   <div class="section-item">
                     <div class="details">
                       <a href="#" data-tooltip="${_('Expand/collapse this subsection')}" class="expand-collapse-icon expand"></a>
-                      <a href="${reverse('edit_subsection', args=[subsection.location])}">
+                      <a href="${subsection_locator.url_reverse('subsection')}">
                         <span class="folder-icon"></span>
                         <span class="subsection-name"><span class="subsection-name-value">${subsection.display_name_with_default}</span></span>
                       </a>

cms/templates/unit.html

@@ -34,7 +34,7 @@ require(["domReady!", "jquery", "js/models/module_info", "coffee/src/views/unit"
 
 </%block>
 <%block name="content">
-  <div class="main-wrapper edit-state-${unit_state}" data-id="${unit_location}" data-locator="${unit_locator}">
+  <div class="main-wrapper edit-state-${unit_state}" data-locator="${unit_locator}">
     <div class="inner-wrapper">
       <div class="alert editing-draft-alert">
         <p class="alert-message"><strong>${_("You are editing a draft.")}</strong>
@@ -135,6 +135,13 @@ require(["domReady!", "jquery", "js/models/module_info", "coffee/src/views/unit"
         </article>
       </div>
 
+      <%
+        ctx_loc = context_course.location
+        index_url = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True).url_reverse('course')
+        subsection_url = loc_mapper().translate_location(
+            ctx_loc.course_id, subsection.location, False, True
+        ).url_reverse('subsection')
+      %>
       <div class="sidebar">
         <div class="unit-settings window">
           <h4 class="header">${_("Unit Settings")}</h4>
@@ -157,7 +164,7 @@ require(["domReady!", "jquery", "js/models/module_info", "coffee/src/views/unit"
               % endif
               ${_("with the subsection {link_start}{name}{link_end}").format(
                   name=subsection.display_name_with_default,
-                  link_start='<a href="{url}">'.format(url=reverse('edit_subsection', kwargs={'location': subsection.location})),
+                  link_start='<a href="{url}">'.format(url=subsection_url),
                   link_end='</a>',
                 )}
               </p>
@@ -180,14 +187,10 @@ require(["domReady!", "jquery", "js/models/module_info", "coffee/src/views/unit"
             </div>
             <ol>
               <li>
-                <%
-                  ctx_loc = context_course.location
-                  index_url = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True).url_reverse('course/', '')
-                %>
                 <a href="${index_url}" class="section-item">${section.display_name_with_default}</a>
                 <ol>
                   <li>
-                    <a href="${reverse('edit_subsection', args=[subsection.location])}" class="section-item">
+                    <a href="${subsection_url}" class="section-item">
                       <span class="folder-icon"></span>
                       <span class="subsection-name"><span class="subsection-name-value">${subsection.display_name_with_default}</span></span>
                     </a>

cms/templates/widgets/units.html

@@ -31,7 +31,7 @@ This def will enumerate through a passed in subsection and list all of the units
         selected_class = ''
     %>
     <div class="section-item ${selected_class}">
-      <a href="${reverse('edit_unit', args=[unit.location])}" class="${unit_state}-item">
+      <a href="${unit_locator.url_reverse('unit')}" class="${unit_state}-item">
         <span class="${unit.scope_ids.block_type}-icon"></span>
         <span class="unit-name">${unit.display_name_with_default}</span>
       </a>

cms/urls.py

@@ -11,8 +11,6 @@ from ratelimitbackend import admin
 admin.autodiscover()
 
 urlpatterns = patterns('',  # nopep8
-    url(r'^edit/(?P<location>.*?)$', 'contentstore.views.edit_unit', name='edit_unit'),
-    url(r'^subsection/(?P<location>.*?)$', 'contentstore.views.edit_subsection', name='edit_subsection'),
 
     url(r'^transcripts/upload$', 'contentstore.views.upload_transcripts', name='upload_transcripts'),
     url(r'^transcripts/download$', 'contentstore.views.download_transcripts', name='download_transcripts'),
@@ -22,10 +20,6 @@ urlpatterns = patterns('',  # nopep8
     url(r'^transcripts/rename$', 'contentstore.views.rename_transcripts', name='rename_transcripts'),
     url(r'^transcripts/save$', 'contentstore.views.save_transcripts', name='save_transcripts'),
 
-    url(r'^create_draft$', 'contentstore.views.create_draft', name='create_draft'),
-    url(r'^publish_draft$', 'contentstore.views.publish_draft', name='publish_draft'),
-    url(r'^unpublish_unit$', 'contentstore.views.unpublish_unit', name='unpublish_unit'),
-
     url(r'^preview/xblock/(?P<usage_id>.*?)/handler/(?P<handler>[^/]*)(?:/(?P<suffix>[^/]*))?$',
         'contentstore.views.preview_handler', name='preview_handler'),
 
@@ -89,6 +83,8 @@ urlpatterns += patterns(
         'course_info_update_handler'
         ),
     url(r'(?ix)^course($|/){}$'.format(parsers.URL_RE_SOURCE), 'course_handler'),
+    url(r'(?ix)^subsection($|/){}$'.format(parsers.URL_RE_SOURCE), 'subsection_handler'),
+    url(r'(?ix)^unit($|/){}$'.format(parsers.URL_RE_SOURCE), 'unit_handler'),
     url(r'(?ix)^checklists/{}(/)?(?P<checklist_index>\d+)?$'.format(parsers.URL_RE_SOURCE), 'checklists_handler'),
     url(r'(?ix)^orphan/{}$'.format(parsers.URL_RE_SOURCE), 'orphan_handler'),
     url(r'(?ix)^assets/{}(/)?(?P<asset_id>.+)?$'.format(parsers.URL_RE_SOURCE), 'assets_handler'),

common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py

@@ -204,21 +204,16 @@ class LocMapperStore(object):
                                 self._decode_from_mongo(old_name),
                                 None)
                     elif usage_id == locator.usage_id:
-                        # figure out revision
-                        # enforce the draft only if category in [..] logic
-                        if category in draft.DIRECT_ONLY_CATEGORIES:
-                            revision = None
-                        elif locator.branch == candidate['draft_branch']:
-                            revision = draft.DRAFT
-                        else:
-                            revision = None
+                        # Always return revision=None because the
+                        # old draft module store wraps locations as draft before
+                        # trying to access things.
                         return Location(
                             'i4x',
                             candidate['_id']['org'],
                             candidate['_id']['course'],
                             category,
                             self._decode_from_mongo(old_name),
-                            revision)
+                            None)
         return None
 
     def add_block_location_translator(self, location, old_course_id=None, usage_id=None):

common/lib/xmodule/xmodule/modulestore/mongo/base.py

@@ -778,11 +778,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
         children: A list of child item identifiers
         """
 
-        # We expect the children IDs to always be the non-draft version. With view refactoring
-        # for split, we are now passing the draft version in some cases.
-        children_ids = [Location(child).replace(revision=None).url() for child in children]
-
-        self._update_single_item(location, {'definition.children': children_ids})
+        self._update_single_item(location, {'definition.children': children})
         # recompute (and update) the metadata inheritance tree which is cached
         self.refresh_cached_metadata_inheritance_tree(Location(location))
         # fire signal that we've written to DB

common/lib/xmodule/xmodule/modulestore/mongo/draft.py

@@ -81,7 +81,7 @@ class DraftModuleStore(MongoModuleStore):
         try:
             return wrap_draft(super(DraftModuleStore, self).get_item(as_draft(location), depth=depth))
         except ItemNotFoundError:
-            return wrap_draft(super(DraftModuleStore, self).get_item(as_published(location), depth=depth))
+            return wrap_draft(super(DraftModuleStore, self).get_item(location, depth=depth))
 
     def get_instance(self, course_id, location, depth=0):
         """
@@ -169,7 +169,7 @@ class DraftModuleStore(MongoModuleStore):
         try:
             draft_item = self.get_item(location)
             if not getattr(draft_item, 'is_draft', False):
-                self.convert_to_draft(as_published(location))
+                self.convert_to_draft(location)
         except ItemNotFoundError, e:
             if not allow_not_found:
                 raise e
@@ -187,7 +187,7 @@ class DraftModuleStore(MongoModuleStore):
         draft_loc = as_draft(location)
         draft_item = self.get_item(location)
         if not getattr(draft_item, 'is_draft', False):
-            self.convert_to_draft(as_published(location))
+            self.convert_to_draft(location)
 
         return super(DraftModuleStore, self).update_children(draft_loc, children)
 
@@ -203,7 +203,7 @@ class DraftModuleStore(MongoModuleStore):
         draft_item = self.get_item(location)
 
         if not getattr(draft_item, 'is_draft', False):
-            self.convert_to_draft(as_published(location))
+            self.convert_to_draft(location)
 
         if 'is_draft' in metadata:
             del metadata['is_draft']
@@ -262,7 +262,7 @@ class DraftModuleStore(MongoModuleStore):
         """
         Turn the published version into a draft, removing the published version
         """
-        self.convert_to_draft(as_published(location))
+        self.convert_to_draft(location)
         super(DraftModuleStore, self).delete_item(location)
 
     def _query_children_for_cache_children(self, items):

common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py

@@ -274,7 +274,9 @@ class TestLocationMapper(unittest.TestCase):
             course_id=prob_locator.course_id, branch='draft', usage_id=prob_locator.usage_id
         )
         prob_location = loc_mapper().translate_locator_to_location(prob_locator)
-        self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123', 'draft'))
+        # Even though the problem was set as draft, we always return revision=None to work
+        # with old mongo/draft modulestores.
+        self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123', None))
         prob_locator = BlockUsageLocator(
             course_id=new_style_course_id, usage_id='problem2', branch='production'
         )

common/lib/xmodule/xmodule/modulestore/xml_exporter.py

@@ -97,7 +97,6 @@ def export_to_xml(modulestore, contentstore, course_location, root_dir, course_d
         if len(draft_verticals) > 0:
             draft_course_dir = export_fs.makeopendir('drafts')
             for draft_vertical in draft_verticals:
-                if getattr(draft_vertical, 'is_draft', False):
                 parent_locs = draft_modulestore.get_parent_locations(draft_vertical.location, course.location.course_id)
                 # Don't try to export orphaned items.
                 if len(parent_locs) > 0: