Code review feedback.

CHANGELOG.rst

@@ -5,6 +5,8 @@ These are notable changes in edx-platform.  This is a rolling list of changes,
 in roughly chronological order, most recent first.  Add your entries at or near
 the top.  Include a label indicating the component affected.
 
+Studio: change create_item, delete_item, and save_item to RESTful API (STUD-847).
+
 Blades: Fix answer choices rearranging if user tries to stylize something in the
 text like with bold or italics. (BLD-449)
 

cms/djangoapps/contentstore/views/item.py

@@ -16,7 +16,7 @@ from util.string_utils import str_to_bool
 
 from ..transcripts_utils import manage_video_subtitles_save
 
-from ..utils import get_modulestore, get_course_for_item
+from ..utils import get_modulestore
 
 from .access import has_access
 from .helpers import _xmodule_recurse
@@ -27,7 +27,7 @@ from student.models import CourseEnrollment
 from django.http import HttpResponseBadRequest
 from xblock.fields import Scope
 
-__all__ = ['orphan', 'xblock_handler']
+__all__ = ['orphan_handler', 'xblock_handler']
 
 log = logging.getLogger(__name__)
 
@@ -75,12 +75,11 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid=
         old_location = loc_mapper().translate_locator_to_location(location)
 
         if request.method == 'GET':
-            rewrite_static_links = str_to_bool(request.GET.get('rewrite_url_links', 'True'))
-            rsp = _get_module_info(location, rewrite_static_links=rewrite_static_links)
+            rsp = _get_module_info(location)
             return JsonResponse(rsp)
         elif request.method == 'DELETE':
-            delete_children = str_to_bool(request.REQUEST.get('recurse', False))
-            delete_all_versions = str_to_bool(request.REQUEST.get('all_versions', False))
+            delete_children = str_to_bool(request.REQUEST.get('recurse', 'False'))
+            delete_all_versions = str_to_bool(request.REQUEST.get('all_versions', 'False'))
 
             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.
@@ -170,8 +169,7 @@ def _save_item(usage_loc, item_location, data=None, children=None, metadata=None
         if existing_item.category == 'video':
             manage_video_subtitles_save(existing_item, existing_item)
 
-    # Note that children aren't returned because it is currently expensive to get the
-    # containing course for an xblock (and that is necessary to convert to locators).
+    # Note that children aren't being returned until we have a use case.
     return JsonResponse({
         'id': unicode(usage_loc),
         'data': data,
@@ -223,9 +221,8 @@ def _create_item(request):
     if category not in DETACHED_CATEGORIES:
         get_modulestore(parent.location).update_children(parent_location, parent.children + [dest_location.url()])
 
-    locator = loc_mapper().translate_location(
-        get_course_for_item(parent_location).location.course_id, dest_location, False, True
-    )
+    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)})
 
 
@@ -263,7 +260,7 @@ def _delete_item_at_location(item_location, delete_children=False, delete_all_ve
 # pylint: disable=W0613
 @login_required
 @require_http_methods(("GET", "DELETE"))
-def orphan(request, tag=None, course_id=None, branch=None, version_guid=None, block=None):
+def orphan_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None):
     """
     View for handling orphan related requests. GET gets all of the current orphans.
     DELETE removes all orphans (requires is_staff access)
@@ -292,7 +289,7 @@ def orphan(request, tag=None, course_id=None, branch=None, version_guid=None, bl
             raise PermissionDenied()
 
 
-def _get_module_info(usage_loc, rewrite_static_links=False):
+def _get_module_info(usage_loc, rewrite_static_links=True):
     """
     metadata, data, id representation of a leaf module fetcher.
     :param usage_loc: A BlockUsageLocator
@@ -319,8 +316,7 @@ def _get_module_info(usage_loc, rewrite_static_links=False):
             course_id=module.location.org + '/' + module.location.course + '/BOGUS_RUN_REPLACE_WHEN_AVAILABLE'
         )
 
-    # Note that children aren't returned because it is currently expensive to get the
-    # containing course for an xblock (and that is necessary to convert to locators).
+    # Note that children aren't being returned until we have a use case.
     return {
         'id': unicode(usage_loc),
         'data': data,

cms/djangoapps/contentstore/views/tabs.py

@@ -15,7 +15,7 @@ from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.django import loc_mapper
 from xmodule.modulestore.locator import BlockUsageLocator
 
-from ..utils import get_course_for_item, get_modulestore
+from ..utils import get_modulestore
 
 from django.utils.translation import ugettext as _
 
@@ -53,11 +53,13 @@ def reorder_static_tabs(request):
         return loc_mapper().translate_locator_to_location(tab_locator)
 
     tabs = request.json['tabs']
-    course = get_course_for_item(get_location_for_tab(tabs[0]))
+    course_location = loc_mapper().translate_locator_to_location(BlockUsageLocator(tabs[0]), get_course=True)
 
-    if not has_access(request.user, course.location):
+    if not has_access(request.user, course_location):
         raise PermissionDenied()
 
+    course = get_modulestore(course_location).get_item(course_location)
+
     # get list of existing static tabs in course
     # make sure they are the same lengths (i.e. the number of passed in tabs equals the number
     # that we know about) otherwise we can drop some!

cms/urls.py

@@ -108,7 +108,7 @@ urlpatterns += patterns(
         ),
     url(r'(?ix)^course($|/){}$'.format(parsers.URL_RE_SOURCE), 'course_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'),
+    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'),
     url(r'(?ix)^import/{}$'.format(parsers.URL_RE_SOURCE), 'import_handler'),
     url(r'(?ix)^import_status/{}/(?P<filename>.+)$'.format(parsers.URL_RE_SOURCE), 'import_status_handler'),

common/djangoapps/util/string_utils.py

@@ -2,14 +2,11 @@
 Utilities for string manipulation.
 """
 
-import ast
-
 def str_to_bool(str):
     """
     Converts "true" (case-insensitive) to the boolean True.
-    Everything else will return False.
+    Everything else will return False (including None).
+
+    An error will be thrown for non-string input (besides None).
     """
-    try:
-        return ast.literal_eval(str.title())
-    except:
-        return False
+    return False if str is None else str.lower() == "true"

common/djangoapps/util/tests/test_string_utils.py

@@ -21,6 +21,13 @@ class StringUtilsTest(TestCase):
         self.assertFalse(str_to_bool(''))
         self.assertFalse(str_to_bool(None))
         self.assertFalse(str_to_bool('anything'))
-        self.assertFalse(str_to_bool([]))
-        self.assertFalse(str_to_bool({}))
-        self.assertFalse(str_to_bool(1))
+
+    def test_str_to_bool_errors(self):
+        def test_raises_error(val):
+            with self.assertRaises(AttributeError):
+                self.assertFalse(str_to_bool(val))
+
+        test_raises_error({})
+        test_raises_error([])
+        test_raises_error(1)
+        test_raises_error(True)

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

@@ -187,11 +187,7 @@ class DraftModuleStore(MongoModuleStore):
 
         # 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
-        ]
+        children_ids = [as_published(child).url() for child in children]
 
         draft_loc = as_draft(location)
         draft_item = self.get_item(location)