From 569c86de74d0cf972c6e85cbe225771e902b47eb Mon Sep 17 00:00:00 2001 From: cahrens Date: Thu, 14 Nov 2013 16:27:50 -0500 Subject: [PATCH] Code review feedback. --- CHANGELOG.rst | 2 ++ cms/djangoapps/contentstore/views/item.py | 26 ++++++++----------- cms/djangoapps/contentstore/views/tabs.py | 8 +++--- cms/urls.py | 2 +- common/djangoapps/util/string_utils.py | 11 +++----- .../util/tests/test_string_utils.py | 13 +++++++--- .../xmodule/modulestore/mongo/draft.py | 6 +---- 7 files changed, 34 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 2a6b19afa8..dbb096a60d 100644 --- a/CHANGELOG.rst +++ b/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) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index cd3a68cb24..448da9d8f4 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/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, diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index f184024f42..277445e3b9 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/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! diff --git a/cms/urls.py b/cms/urls.py index 714975d8bf..99e9cbfaba 100644 --- a/cms/urls.py +++ b/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\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.+)?$'.format(parsers.URL_RE_SOURCE), 'assets_handler'), url(r'(?ix)^import/{}$'.format(parsers.URL_RE_SOURCE), 'import_handler'), url(r'(?ix)^import_status/{}/(?P.+)$'.format(parsers.URL_RE_SOURCE), 'import_status_handler'), diff --git a/common/djangoapps/util/string_utils.py b/common/djangoapps/util/string_utils.py index 1fd7a8b646..b14498d400 100644 --- a/common/djangoapps/util/string_utils.py +++ b/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" diff --git a/common/djangoapps/util/tests/test_string_utils.py b/common/djangoapps/util/tests/test_string_utils.py index 542ea562c1..b3bdea3411 100644 --- a/common/djangoapps/util/tests/test_string_utils.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 41e75efe0a..554b13190c 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/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)