From 16766a5ee9165b6f32f68e27271e91d910cabdd4 Mon Sep 17 00:00:00 2001 From: cahrens Date: Fri, 1 Nov 2013 14:39:23 -0400 Subject: [PATCH] Change delete_item to RESTful URL. Part of STUD-847. --- cms/djangoapps/auth/authz.py | 8 ++- .../contentstore/tests/test_contentstore.py | 34 +++++------ .../contentstore/tests/test_item.py | 9 +-- .../contentstore/views/component.py | 19 +++++-- cms/djangoapps/contentstore/views/item.py | 56 ++++++++++++------- cms/djangoapps/contentstore/views/tabs.py | 8 ++- .../coffee/src/views/module_edit.coffee | 1 + cms/static/coffee/src/views/tabs.coffee | 7 ++- cms/static/coffee/src/views/unit.coffee | 23 ++++---- cms/static/js/base.js | 17 +++--- cms/templates/edit-tabs.html | 4 +- cms/templates/overview.html | 17 +++++- cms/templates/unit.html | 6 +- cms/templates/widgets/units.html | 11 +++- cms/urls.py | 3 +- common/djangoapps/util/json_request.py | 2 +- .../xmodule/modulestore/loc_mapper_store.py | 16 +++++- .../xmodule/xmodule/modulestore/locator.py | 2 +- .../xmodule/modulestore/mongo/draft.py | 10 ++-- .../modulestore/tests/test_location_mapper.py | 6 +- .../xmodule/modulestore/xml_exporter.py | 19 ++++--- 21 files changed, 172 insertions(+), 106 deletions(-) diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index d58c2ba3fd..c4d1a9ddff 100644 --- a/cms/djangoapps/auth/authz.py +++ b/cms/djangoapps/auth/authz.py @@ -11,6 +11,7 @@ from django.conf import settings from xmodule.modulestore import Location from xmodule.modulestore.locator import CourseLocator, Locator from xmodule.modulestore.django import loc_mapper +from xmodule.modulestore.exceptions import InvalidLocationError # define a couple of simple roles, we just need ADMIN and EDITOR now for our purposes @@ -32,11 +33,14 @@ def get_course_groupname_for_role(location, role): # if it exists, then use that one, otherwise use a _ which contains # more information groupnames = [] - groupnames.append('{0}_{1}'.format(role, location.course_id)) + try: + groupnames.append('{0}_{1}'.format(role, location.course_id)) + except InvalidLocationError: # will occur on old locations where location is not of category course + pass if isinstance(location, Location): groupnames.append('{0}_{1}'.format(role, location.course)) elif isinstance(location, CourseLocator): - old_location = loc_mapper().translate_locator_to_location(location) + old_location = loc_mapper().translate_locator_to_location(location, get_course=True) if old_location: groupnames.append('{0}_{1}'.format(role, old_location.course_id)) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 226b36aeaf..a207b9283f 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -488,11 +488,8 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # make sure the parent points to the child object which is to be deleted self.assertTrue(sequential.location.url() in chapter.children) - self.client.post( - reverse('delete_item'), - json.dumps({'id': sequential.location.url(), 'delete_children': 'true', 'delete_all_versions': 'true'}), - "application/json" - ) + location = loc_mapper().translate_location(course_location.course_id, sequential.location, False, True) + self.client.delete(location.url_reverse('xblock'), {'recurse': True, 'all_versions': True}) found = False try: @@ -1640,29 +1637,24 @@ class ContentStoreTest(ModuleStoreTestCase): kwargs={'location': unit_location.url()})) self.assertEqual(resp.status_code, 200) + def delete_item(category, name): + """ Helper method for testing the deletion of an xblock item. """ + del_loc = loc.replace(category=category, name=name) + del_location = loc_mapper().translate_location(loc.course_id, del_loc, False, True) + resp = self.client.delete(del_location.url_reverse('xblock')) + self.assertEqual(resp.status_code, 204) + # delete a component - del_loc = loc.replace(category='html', name='test_html') - resp = self.client.post(reverse('delete_item'), - json.dumps({'id': del_loc.url()}), "application/json") - self.assertEqual(resp.status_code, 204) + delete_item(category='html', name='test_html') # delete a unit - del_loc = loc.replace(category='vertical', name='test_vertical') - resp = self.client.post(reverse('delete_item'), - json.dumps({'id': del_loc.url()}), "application/json") - self.assertEqual(resp.status_code, 204) + delete_item(category='vertical', name='test_vertical') # delete a unit - del_loc = loc.replace(category='sequential', name='test_sequence') - resp = self.client.post(reverse('delete_item'), - json.dumps({'id': del_loc.url()}), "application/json") - self.assertEqual(resp.status_code, 204) + delete_item(category='sequential', name='test_sequence') # delete a chapter - del_loc = loc.replace(category='chapter', name='chapter_2') - resp = self.client.post(reverse('delete_item'), - json.dumps({'id': del_loc.url()}), "application/json") - self.assertEqual(resp.status_code, 204) + delete_item(category='chapter', name='chapter_2') def test_import_into_new_course_id(self): module_store = modulestore('direct') diff --git a/cms/djangoapps/contentstore/tests/test_item.py b/cms/djangoapps/contentstore/tests/test_item.py index 1317e76a10..6d42d400fc 100644 --- a/cms/djangoapps/contentstore/tests/test_item.py +++ b/cms/djangoapps/contentstore/tests/test_item.py @@ -12,7 +12,7 @@ from xmodule.modulestore.django import modulestore class DeleteItem(CourseTestCase): - """Tests for '/delete_item' url.""" + """Tests for '/xblock' DELETE url.""" def setUp(self): """ Creates the test course with a static page in it. """ super(DeleteItem, self).setUp() @@ -33,11 +33,8 @@ class DeleteItem(CourseTestCase): self.assertEqual(resp.status_code, 200) # Now delete it. There was a bug that the delete was failing (static tabs do not exist in draft modulestore). - resp = self.client.post( - reverse('delete_item'), - resp.content, - "application/json" - ) + resp_content = json.loads(resp.content) + resp = self.client.delete(resp_content['update_url']) self.assertEqual(resp.status_code, 204) diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index f51789739c..2cc31bcea0 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -16,6 +16,7 @@ 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 xblock.fields import Scope from util.json_request import expect_json, JsonResponse @@ -174,6 +175,12 @@ def edit_unit(request, 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_update_url = loc_mapper().translate_location( + course.location.course_id, Location(location), False, True + ).url_reverse("xblock", "") + component_templates = defaultdict(list) for category in COMPONENT_TYPES: component_class = load_mixed_class(category) @@ -238,7 +245,12 @@ def edit_unit(request, location): ) components = [ - component.location.url() + [ + component.location.url(), + loc_mapper().translate_location( + course.location.course_id, component.location, False, True + ).url_reverse("xblock") + ] for component in item.get_children() ] @@ -283,12 +295,11 @@ def edit_unit(request, location): index=index ) - unit_state = compute_unit_state(item) - return render_to_response('unit.html', { 'context_course': course, 'unit': item, 'unit_location': location, + 'unit_update_url': unit_update_url, 'components': components, 'component_templates': component_templates, 'draft_preview_link': preview_lms_link, @@ -300,7 +311,7 @@ def edit_unit(request, location): ), 'section': containing_section, 'new_unit_category': 'vertical', - 'unit_state': unit_state, + 'unit_state': compute_unit_state(item), 'published_date': ( get_default_time_display(item.published_date) if item.published_date is not None else None diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 34c22ca1bd..41da5586b0 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -16,16 +16,16 @@ from util.json_request import expect_json, JsonResponse from ..transcripts_utils import manage_video_subtitles_save -from ..utils import get_modulestore +from ..utils import get_modulestore, get_course_for_item from .access import has_access from .helpers import _xmodule_recurse from xmodule.x_module import XModuleDescriptor from django.views.decorators.http import require_http_methods -from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator +from xmodule.modulestore.locator import BlockUsageLocator from student.models import CourseEnrollment -__all__ = ['save_item', 'create_item', 'delete_item', 'orphan'] +__all__ = ['save_item', 'create_item', 'orphan', 'xblock_handler'] log = logging.getLogger(__name__) @@ -33,6 +33,31 @@ log = logging.getLogger(__name__) DETACHED_CATEGORIES = ['about', 'static_tab', 'course_info'] +@require_http_methods(("DELETE")) +@login_required +@expect_json +def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): + """ + The restful handler for xblock requests. + + DELETE + json: delete this xblock instance from the course. Supports query parameters "recurse" to delete + all children and "all_versions" to delete from all (mongo) versions. + """ + if request.method == 'DELETE': + location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) + if not has_access(request.user, location): + raise PermissionDenied() + + old_location = loc_mapper().translate_locator_to_location(location) + + delete_children = bool(request.REQUEST.get('recurse', False)) + delete_all_versions = bool(request.REQUEST.get('all_versions', False)) + + _delete_item_at_location(old_location, delete_children, delete_all_versions) + return JsonResponse() + + @login_required @expect_json def save_item(request): @@ -171,24 +196,18 @@ def create_item(request): if category not in DETACHED_CATEGORIES: get_modulestore(parent.location).update_children(parent_location, parent.children + [dest_location.url()]) - return JsonResponse({'id': dest_location.url()}) + locator = loc_mapper().translate_location( + get_course_for_item(parent_location).location.course_id, dest_location, False, True + ) + return JsonResponse({'id': dest_location.url(), "update_url": locator.url_reverse("xblock")}) -@login_required -@expect_json -def delete_item(request): - """View for removing items.""" - item_location = request.json['id'] - item_location = Location(item_location) - - # check permissions for this user within this course - if not has_access(request.user, item_location): - raise PermissionDenied() - - # optional parameter to delete all children (default False) - delete_children = request.json.get('delete_children', False) - delete_all_versions = request.json.get('delete_all_versions', False) +def _delete_item_at_location(item_location, delete_children=False, delete_all_versions=False): + """ + Deletes the item at with the given Location. + It is assumed that course permissions have already been checked. + """ store = get_modulestore(item_location) item = store.get_item(item_location) @@ -211,7 +230,6 @@ def delete_item(request): parent.children = children modulestore('direct').update_children(parent.location, parent.children) - return JsonResponse() # pylint: disable=W0613 @login_required diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index db3432045a..ed1a8b16de 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/cms/djangoapps/contentstore/views/tabs.py @@ -12,6 +12,7 @@ from mitxmako.shortcuts import render_to_response from xmodule.modulestore import Location from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.django import modulestore +from xmodule.modulestore.django import loc_mapper from ..utils import get_course_for_item, get_modulestore @@ -117,7 +118,12 @@ def edit_tabs(request, org, course, coursename): static_tabs.append(modulestore('direct').get_item(static_tab_loc)) components = [ - static_tab.location.url() + [ + static_tab.location.url(), + loc_mapper().translate_location( + course_item.location.course_id, static_tab.location, False, True + ).url_reverse("xblock") + ] for static_tab in static_tabs ] diff --git a/cms/static/coffee/src/views/module_edit.coffee b/cms/static/coffee/src/views/module_edit.coffee index af577bbb47..e82a668b52 100644 --- a/cms/static/coffee/src/views/module_edit.coffee +++ b/cms/static/coffee/src/views/module_edit.coffee @@ -70,6 +70,7 @@ define ["backbone", "jquery", "underscore", "gettext", "xblock/runtime.v1", (data) => @model.set(id: data.id) @$el.data('id', data.id) + @$el.data('update_url', data.update_url) @render() ) diff --git a/cms/static/coffee/src/views/tabs.coffee b/cms/static/coffee/src/views/tabs.coffee index 88b47b6733..5fd9b3f7f5 100644 --- a/cms/static/coffee/src/views/tabs.coffee +++ b/cms/static/coffee/src/views/tabs.coffee @@ -82,9 +82,10 @@ define ["jquery", "jquery.ui", "backbone", "js/views/feedback_prompt", "js/views deleting = new NotificationView.Mini title: gettext('Deleting…') deleting.show() - $.postJSON('/delete_item', { - id: $component.data('id') - }, => + $.ajax({ + type: 'DELETE', + url: $component.data('update_url') + }).success(=> $component.remove() deleting.hide() ) diff --git a/cms/static/coffee/src/views/unit.coffee b/cms/static/coffee/src/views/unit.coffee index 83b4f02897..294c52a208 100644 --- a/cms/static/coffee/src/views/unit.coffee +++ b/cms/static/coffee/src/views/unit.coffee @@ -134,9 +134,10 @@ define ["jquery", "jquery.ui", "gettext", "backbone", title: gettext('Deleting…'), deleting.show() $component = $(event.currentTarget).parents('.component') - $.postJSON('/delete_item', { - id: $component.data('id') - }, => + $.ajax({ + type: 'DELETE', + url: $component.data('update_url') + }).success(=> deleting.hide() analytics.track "Deleted a Component", course: course_location_analytics @@ -162,16 +163,16 @@ define ["jquery", "jquery.ui", "gettext", "backbone", deleteDraft: (event) -> @wait(true) + $.ajax({ + type: 'DELETE', + url: @$el.data('update_url') + "?" + $.param({recurse: true}) + }).success(=> - $.postJSON('/delete_item', { - id: @$el.data('id') - delete_children: true - }, => - analytics.track "Deleted Draft", - course: course_location_analytics - unit_id: unit_location_analytics + analytics.track "Deleted Draft", + course: course_location_analytics + unit_id: unit_location_analytics - window.location.reload() + window.location.reload() ) createDraft: (event) -> diff --git a/cms/static/js/base.js b/cms/static/js/base.js index bc155d7413..4f392fdd98 100644 --- a/cms/static/js/base.js +++ b/cms/static/js/base.js @@ -279,15 +279,14 @@ function _deleteItem($el, type) { }); deleting.show(); - $.postJSON('/delete_item', - {'id': id, - 'delete_children': true, - 'delete_all_versions': true}, - function(data) { - $el.remove(); - deleting.hide(); - } - ); + $.ajax({ + type: 'DELETE', + url: $el.data('update_url')+'?'+ $.param({recurse: true, all_versions: true}), + success: function () { + $el.remove(); + deleting.hide(); + } + }); } }, secondary: { diff --git a/cms/templates/edit-tabs.html b/cms/templates/edit-tabs.html index ae597f0377..502d8fdc46 100644 --- a/cms/templates/edit-tabs.html +++ b/cms/templates/edit-tabs.html @@ -61,8 +61,8 @@ require(["coffee/src/views/tabs", "coffee/src/models/module"], function(TabsEdit
    - % for id in components: -
  1. + % for id, update_url in components: +
  2. % endfor
  3. diff --git a/cms/templates/overview.html b/cms/templates/overview.html index 30e9552160..66da1909e0 100644 --- a/cms/templates/overview.html +++ b/cms/templates/overview.html @@ -4,6 +4,7 @@ from xmodule.util import date_utils from django.utils.translation import ugettext as _ from django.core.urlresolvers import reverse + from xmodule.modulestore.django import loc_mapper %> <%block name="title">${_("Course Outline")} <%block name="bodyclass">is-signedin course view-outline @@ -141,7 +142,13 @@ require(["domReady!", "jquery", "js/models/location", "js/models/section", "js/v
    % for section in sections: -
    + <% + section_update_url = loc_mapper().translate_location( + context_course.location.course_id, section.location, False, True + ).url_reverse('xblock') + %> +
    <%include file="widgets/_ui-dnd-indicator-before.html" /> @@ -184,7 +191,13 @@ require(["domReady!", "jquery", "js/models/location", "js/models/section", "js/v
      % for subsection in section.get_children(): -