diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9b3452eb50..46c7260724 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,7 +5,7 @@ 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 course overview page, checklists, and course staff management +Studio: Change course overview page, checklists, assets, and course staff management page URLs to a RESTful interface. Also removed "\listing", which duplicated "\index". diff --git a/cms/djangoapps/contentstore/tests/test_assets.py b/cms/djangoapps/contentstore/tests/test_assets.py index 4169e41d42..9ed9650b45 100644 --- a/cms/djangoapps/contentstore/tests/test_assets.py +++ b/cms/djangoapps/contentstore/tests/test_assets.py @@ -13,26 +13,24 @@ import json import re from unittest import TestCase, skip from .utils import CourseTestCase -from django.core.urlresolvers import reverse from contentstore.views import assets from xmodule.contentstore.content import StaticContent, XASSET_LOCATION_TAG from xmodule.modulestore import Location from xmodule.contentstore.django import contentstore from xmodule.modulestore.django import modulestore from xmodule.modulestore.xml_importer import import_from_xml +from xmodule.modulestore.django import loc_mapper +from xmodule.modulestore.mongo.base import location_to_query class AssetsTestCase(CourseTestCase): def setUp(self): super(AssetsTestCase, self).setUp() - self.url = reverse("asset_index", kwargs={ - 'org': self.course.location.org, - 'course': self.course.location.course, - 'name': self.course.location.name, - }) + location = loc_mapper().translate_location(self.course.location.course_id, self.course.location, False, True) + self.url = location.url_reverse('assets/', '') def test_basic(self): - resp = self.client.get(self.url) + resp = self.client.get(self.url, HTTP_ACCEPT='text/html') self.assertEquals(resp.status_code, 200) def test_static_url_generation(self): @@ -43,14 +41,22 @@ class AssetsTestCase(CourseTestCase): class AssetsToyCourseTestCase(CourseTestCase): """ - Tests the assets returned from asset_index for the toy test course. + Tests the assets returned from assets_handler (full page content) for the toy test course. """ def test_toy_assets(self): module_store = modulestore('direct') - import_from_xml(module_store, 'common/test/data/', ['toy'], static_content_store=contentstore(), verbose=True) - url = reverse("asset_index", kwargs={'org': 'edX', 'course': 'toy', 'name': '2012_Fall'}) + _, course_items = import_from_xml( + module_store, + 'common/test/data/', + ['toy'], + static_content_store=contentstore(), + verbose=True + ) + course = course_items[0] + location = loc_mapper().translate_location(course.location.course_id, course.location, False, True) + url = location.url_reverse('assets/', '') - resp = self.client.get(url) + resp = self.client.get(url, HTTP_ACCEPT='text/html') # Test a small portion of the asset data passed to the client. self.assertContains(resp, "new AssetCollection([{") self.assertContains(resp, "/c4x/edX/toy/asset/handouts_sample_handout.txt") @@ -62,11 +68,8 @@ class UploadTestCase(CourseTestCase): """ def setUp(self): super(UploadTestCase, self).setUp() - self.url = reverse("upload_asset", kwargs={ - 'org': self.course.location.org, - 'course': self.course.location.course, - 'coursename': self.course.location.name, - }) + location = loc_mapper().translate_location(self.course.location.course_id, self.course.location, False, True) + self.url = location.url_reverse('assets/', '') @skip("CorruptGridFile error on continuous integration server") def test_happy_path(self): @@ -76,12 +79,12 @@ class UploadTestCase(CourseTestCase): self.assertEquals(resp.status_code, 200) def test_no_file(self): - resp = self.client.post(self.url, {"name": "file.txt"}) + resp = self.client.post(self.url, {"name": "file.txt"}, "application/json") self.assertEquals(resp.status_code, 400) def test_get(self): - resp = self.client.get(self.url) - self.assertEquals(resp.status_code, 405) + with self.assertRaises(NotImplementedError): + self.client.get(self.url) class AssetToJsonTestCase(TestCase): @@ -127,16 +130,28 @@ class LockAssetTestCase(CourseTestCase): def post_asset_update(lock): """ Helper method for posting asset update. """ upload_date = datetime(2013, 6, 1, 10, 30, tzinfo=UTC) - location = Location(['c4x', 'edX', 'toy', 'asset', 'sample_static.txt']) - url = reverse('update_asset', kwargs={'org': 'edX', 'course': 'toy', 'name': '2012_Fall'}) + asset_location = Location(['c4x', 'edX', 'toy', 'asset', 'sample_static.txt']) + location = loc_mapper().translate_location(course.location.course_id, course.location, False, True) + url = location.url_reverse('assets/', '') - resp = self.client.post(url, json.dumps(assets._get_asset_json("sample_static.txt", upload_date, location, None, lock)), "application/json") + resp = self.client.post( + url, + json.dumps(assets._get_asset_json("sample_static.txt", upload_date, asset_location, None, lock)), + "application/json" + ) self.assertEqual(resp.status_code, 201) return json.loads(resp.content) # Load the toy course. module_store = modulestore('direct') - import_from_xml(module_store, 'common/test/data/', ['toy'], static_content_store=contentstore(), verbose=True) + _, course_items = import_from_xml( + module_store, + 'common/test/data/', + ['toy'], + static_content_store=contentstore(), + verbose=True + ) + course = course_items[0] verify_asset_locked_state(False) # Lock the asset @@ -160,6 +175,8 @@ class TestAssetIndex(CourseTestCase): """ super(TestAssetIndex, self).setUp() self.entry_filter = self.create_asset_entries(contentstore(), 100) + location = loc_mapper().translate_location(self.course.location.course_id, self.course.location, False, True) + self.url = location.url_reverse('assets/', '') def tearDown(self): """ @@ -175,7 +192,7 @@ class TestAssetIndex(CourseTestCase): XASSET_LOCATION_TAG, category='asset', course=self.course.location.course, org=self.course.location.org ) # purge existing entries (a bit brutal but hopefully tests are independent enuf to not trip on this) - cstore.fs_files.remove(course_filter.dict()) + cstore.fs_files.remove(location_to_query(course_filter)) base_entry = { 'displayname': 'foo.jpg', 'chunkSize': 262144, @@ -215,19 +232,11 @@ class TestAssetIndex(CourseTestCase): The actual test """ # get all - asset_url = reverse( - 'asset_index', - kwargs={ - 'org': self.course.location.org, - 'course': self.course.location.course, - 'name': self.course.location.name - } - ) - resp = self.client.get(asset_url) + resp = self.client.get(self.url, HTTP_ACCEPT='text/html') self.check_page_content(resp.content, 100) # get first page of 10 - resp = self.client.get(asset_url + "/max/10") + resp = self.client.get(self.url + "?max=10", HTTP_ACCEPT='text/html') last_date = self.check_page_content(resp.content, 10) # get next of 20 - resp = self.client.get(asset_url + "/start/10/max/20") - last_date = self.check_page_content(resp.content, 20, last_date) + resp = self.client.get(self.url + "?start=10&max=20", HTTP_ACCEPT='text/html') + self.check_page_content(resp.content, 20, last_date) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 91b9049584..3ba3e30493 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -585,29 +585,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): ''' This test will exercise the soft delete/restore functionality of the assets ''' - content_store = contentstore() - trash_store = contentstore('trashcan') - module_store = modulestore('direct') - import_from_xml(module_store, 'common/test/data/', ['toy'], static_content_store=content_store) - - # look up original (and thumbnail) in content store, should be there after import - location = StaticContent.get_location_from_path('/c4x/edX/toy/asset/sample_static.txt') - content = content_store.find(location, throw_on_not_found=False) - thumbnail_location = content.thumbnail_location - self.assertIsNotNone(content) - - # - # cdodge: temporarily comment out assertion on thumbnails because many environments - # will not have the jpeg converter installed and this test will fail - # - # self.assertIsNotNone(thumbnail_location) - - # go through the website to do the delete, since the soft-delete logic is in the view - - url = reverse('update_asset', kwargs={'org': 'edX', 'course': 'toy', 'name': '2012_Fall', 'asset_id': '/c4x/edX/toy/asset/sample_static.txt'}) - resp = self.client.delete(url) - self.assertEqual(resp.status_code, 204) - + content_store, trash_store, thumbnail_location = self._delete_asset_in_course() asset_location = StaticContent.get_location_from_path('/c4x/edX/toy/asset/sample_static.txt') # now try to find it in store, but they should not be there any longer @@ -637,29 +615,49 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): thumbnail = content_store.find(thumbnail_location, throw_on_not_found=False) self.assertIsNotNone(thumbnail) + def _delete_asset_in_course (self): + """ + Helper method for: + 1) importing course from xml + 2) finding asset in course (verifying non-empty) + 3) computing thumbnail location of asset + 4) deleting the asset from the course + """ + + content_store = contentstore() + trash_store = contentstore('trashcan') + module_store = modulestore('direct') + _, course_items = import_from_xml(module_store, 'common/test/data/', ['toy'], static_content_store=content_store) + + # look up original (and thumbnail) in content store, should be there after import + location = StaticContent.get_location_from_path('/c4x/edX/toy/asset/sample_static.txt') + content = content_store.find(location, throw_on_not_found=False) + thumbnail_location = content.thumbnail_location + self.assertIsNotNone(content) + + # + # cdodge: temporarily comment out assertion on thumbnails because many environments + # will not have the jpeg converter installed and this test will fail + # + # self.assertIsNotNone(thumbnail_location) + + # go through the website to do the delete, since the soft-delete logic is in the view + course = course_items[0] + location = loc_mapper().translate_location(course.location.course_id, course.location, False, True) + url = location.url_reverse('assets/', '/c4x/edX/toy/asset/sample_static.txt') + resp = self.client.delete(url) + self.assertEqual(resp.status_code, 204) + + return content_store, trash_store, thumbnail_location + def test_empty_trashcan(self): ''' This test will exercise the emptying of the asset trashcan ''' - content_store = contentstore() - trash_store = contentstore('trashcan') - module_store = modulestore('direct') - - import_from_xml(module_store, 'common/test/data/', ['toy'], static_content_store=content_store) - - course_location = CourseDescriptor.id_to_location('edX/toy/6.002_Spring_2012') - - location = StaticContent.get_location_from_path('/c4x/edX/toy/asset/sample_static.txt') - content = content_store.find(location, throw_on_not_found=False) - self.assertIsNotNone(content) - - # go through the website to do the delete, since the soft-delete logic is in the view - - url = reverse('update_asset', kwargs={'org': 'edX', 'course': 'toy', 'name': '2012_Fall', 'asset_id': '/c4x/edX/toy/asset/sample_static.txt'}) - resp = self.client.delete(url) - self.assertEqual(resp.status_code, 204) + _, trash_store, _ = self._delete_asset_in_course() # make sure there's something in the trashcan + course_location = CourseDescriptor.id_to_location('edX/toy/6.002_Spring_2012') all_assets = trash_store.get_all_content_for_course(course_location) self.assertGreater(len(all_assets), 0) @@ -1633,11 +1631,11 @@ class ContentStoreTest(ModuleStoreTestCase): 'name': loc.name})) self.assertEqual(resp.status_code, 200) - # asset_index - resp = self.client.get(reverse('asset_index', - kwargs={'org': loc.org, - 'course': loc.course, - 'name': loc.name})) + # assets_handler (HTML for full page content) + new_location = loc_mapper().translate_location(loc.course_id, loc, False, True) + url = new_location.url_reverse('assets/', '') + + resp = self.client.get(url, HTTP_ACCEPT='text/html') self.assertEqual(resp.status_code, 200) # go look at a subsection page diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index f55ccac37e..7f2fab96f3 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -5,7 +5,6 @@ from django.http import HttpResponseBadRequest from django.contrib.auth.decorators import login_required from django.views.decorators.http import require_http_methods from django_future.csrf import ensure_csrf_cookie -from django.core.urlresolvers import reverse from django.views.decorators.http import require_POST from mitxmako.shortcuts import render_to_response @@ -18,39 +17,67 @@ from xmodule.contentstore.content import StaticContent from xmodule.util.date_utils import get_default_time_display from xmodule.modulestore import InvalidLocationError from xmodule.exceptions import NotFoundError +from django.core.exceptions import PermissionDenied +from xmodule.modulestore.django import loc_mapper +from .access import has_access +from xmodule.modulestore.locator import BlockUsageLocator -from .access import get_location_and_verify_access from util.json_request import JsonResponse +from django.http import HttpResponseNotFound import json from django.utils.translation import ugettext as _ from pymongo import DESCENDING -__all__ = ['asset_index', 'upload_asset'] +__all__ = ['assets_handler'] @login_required @ensure_csrf_cookie -def asset_index(request, org, course, name, start=None, maxresults=None): +def assets_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None, asset_id=None): """ - Display an editable asset library + The restful handler for assets. + It allows retrieval of all the assets (as an HTML page), as well as uploading new assets, + deleting assets, and changing the "locked" state of an asset. - org, course, name: Attributes of the Location for the item to edit - - :param start: which index of the result list to start w/, used for paging results - :param maxresults: maximum results + GET + html: return html page of all course assets (note though that a range of assets can be requested using start + and max query parameters) + json: not currently supported + POST + json: create (or update?) an asset. The only updating that can be done is changing the lock state. + PUT + json: update the locked state of an asset + DELETE + json: delete an asset """ - location = get_location_and_verify_access(request, org, course, name) + location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) + if not has_access(request.user, location): + raise PermissionDenied() - upload_asset_callback_url = reverse('upload_asset', kwargs={ - 'org': org, - 'course': course, - 'coursename': name - }) + if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): + if request.method == 'GET': + raise NotImplementedError('coming soon') + else: + return _update_asset(request, location, asset_id) + elif request.method == 'GET': # assume html + return _asset_index(request, location) + else: + return HttpResponseNotFound() - course_module = modulestore().get_item(location) - course_reference = StaticContent.compute_location(org, course, name) +def _asset_index(request, location): + """ + Display an editable asset library. + + Supports start (0-based index into the list of assets) and max query parameters. + """ + old_location = loc_mapper().translate_locator_to_location(location) + + course_module = modulestore().get_item(old_location) + maxresults = request.REQUEST.get('max', None) + start = request.REQUEST.get('start', None) + course_reference = StaticContent.compute_location(old_location.org, old_location.course, old_location.name) if maxresults is not None: maxresults = int(maxresults) start = int(start) if start else 0 @@ -77,36 +104,27 @@ def asset_index(request, org, course, name, start=None, maxresults=None): return render_to_response('asset_index.html', { 'context_course': course_module, 'asset_list': json.dumps(asset_json), - 'upload_asset_callback_url': upload_asset_callback_url, - 'update_asset_callback_url': reverse('update_asset', kwargs={ - 'org': org, - 'course': course, - 'name': name - }) + 'asset_callback_url': location.url_reverse('assets/', '') }) @require_POST @ensure_csrf_cookie @login_required -def upload_asset(request, org, course, coursename): +def _upload_asset(request, location): ''' This method allows for POST uploading of files into the course asset library, which will be supported by GridFS in MongoDB. ''' - # construct a location from the passed in path - location = get_location_and_verify_access(request, org, course, coursename) + old_location = loc_mapper().translate_locator_to_location(location) # Does the course actually exist?!? Get anything from it to prove its # existence try: - modulestore().get_item(location) + modulestore().get_item(old_location) except: # no return it as a Bad Request response - logging.error('Could not find course' + location) - return HttpResponseBadRequest() - - if 'file' not in request.FILES: + logging.error('Could not find course' + old_location) return HttpResponseBadRequest() # compute a 'filename' which is similar to the location formatting, we're @@ -117,7 +135,7 @@ def upload_asset(request, org, course, coursename): filename = upload_file.name mime_type = upload_file.content_type - content_loc = StaticContent.compute_location(org, course, filename) + content_loc = StaticContent.compute_location(old_location.org, old_location.course, filename) chunked = upload_file.multiple_chunks() sc_partial = partial(StaticContent, content_loc, filename, mime_type) @@ -160,12 +178,11 @@ def upload_asset(request, org, course, coursename): @require_http_methods(("DELETE", "POST", "PUT")) @login_required @ensure_csrf_cookie -def update_asset(request, org, course, name, asset_id): +def _update_asset(request, location, asset_id): """ restful CRUD operations for a course asset. Currently only DELETE, POST, and PUT methods are implemented. - org, course, name: Attributes of the Location for the item to edit asset_id: the URL of the asset (used by Backbone as the id) """ def get_asset_location(asset_id): @@ -176,8 +193,6 @@ def update_asset(request, org, course, name, asset_id): # return a 'Bad Request' to browser as we have a malformed Location return JsonResponse({"error": err.message}, status=400) - get_location_and_verify_access(request, org, course, name) - if request.method == 'DELETE': loc = get_asset_location(asset_id) # Make sure the item to delete actually exists. @@ -208,16 +223,20 @@ def update_asset(request, org, course, name, asset_id): return JsonResponse() elif request.method in ('PUT', 'POST'): - # We don't support creation of new assets through this - # method-- just changing the locked state. - modified_asset = json.loads(request.body) - asset_id = modified_asset['url'] - location = get_asset_location(asset_id) - contentstore().set_attr(location, 'locked', modified_asset['locked']) - # Delete the asset from the cache so we check the lock status the next time it is requested. - del_cached_content(location) - - return JsonResponse(modified_asset, status=201) + if 'file' in request.FILES: + return _upload_asset(request, location) + else: + # Update existing asset + try: + modified_asset = json.loads(request.body) + except ValueError: + return HttpResponseBadRequest() + asset_id = modified_asset['url'] + asset_location = get_asset_location(asset_id) + contentstore().set_attr(asset_location, 'locked', modified_asset['locked']) + # Delete the asset from the cache so we check the lock status the next time it is requested. + del_cached_content(asset_location) + return JsonResponse(modified_asset, status=201) def _get_asset_json(display_name, date, location, thumbnail_location, locked): diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index fbb001ac25..020c983c0b 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -124,12 +124,6 @@ def course_index(request, course_id, branch, version_guid, block): lms_link = get_lms_link_for_item(old_location) - upload_asset_callback_url = reverse('upload_asset', kwargs={ - 'org': old_location.org, - 'course': old_location.course, - 'coursename': old_location.name - }) - course = modulestore().get_item(old_location, depth=3) sections = course.get_children() @@ -143,7 +137,6 @@ def course_index(request, course_id, branch, version_guid, block): 'parent_location': course.location, 'new_section_category': 'chapter', 'new_subsection_category': 'sequential', - 'upload_asset_callback_url': upload_asset_callback_url, 'new_unit_category': 'vertical', 'category': 'vertical' }) @@ -334,6 +327,9 @@ def get_course_settings(request, org, course, name): course_module = modulestore().get_item(location) + new_loc = loc_mapper().translate_location(location.course_id, location, False, True) + upload_asset_url = new_loc.url_reverse('assets/', '') + return render_to_response('settings.html', { 'context_course': course_module, 'course_location': location, @@ -345,11 +341,7 @@ def get_course_settings(request, org, course, name): 'about_page_editable': not settings.MITX_FEATURES.get( 'ENABLE_MKTG_SITE', False ), - 'upload_asset_url': reverse('upload_asset', kwargs={ - 'org': org, - 'course': course, - 'coursename': name, - }) + 'upload_asset_url': upload_asset_url }) @@ -656,11 +648,8 @@ def textbook_index(request, org, course, name): ) return JsonResponse(course_module.pdf_textbooks) else: - upload_asset_url = reverse('upload_asset', kwargs={ - 'org': org, - 'course': course, - 'coursename': name, - }) + new_loc = loc_mapper().translate_location(location.course_id, location, False, True) + upload_asset_url = new_loc.url_reverse('assets/', '') textbook_url = reverse('textbook_index', kwargs={ 'org': org, 'course': course, diff --git a/cms/templates/asset_index.html b/cms/templates/asset_index.html index 909fc73c5e..4a7655fbbe 100644 --- a/cms/templates/asset_index.html +++ b/cms/templates/asset_index.html @@ -22,7 +22,7 @@ require(["domReady", "jquery", "gettext", "js/models/asset", "js/collections/ass function(domReady, $, gettext, AssetModel, AssetCollection, AssetsView, PromptView, NotificationView, ModalUtils) { var assets = new AssetCollection(${asset_list}); - assets.url = "${update_asset_callback_url}"; + assets.url = "${asset_callback_url}"; var assetsView = new AssetsView({collection: assets, el: $('#asset_table_body')}); var hideModal = function (e) { @@ -192,7 +192,7 @@ require(["domReady", "jquery", "gettext", "js/models/asset", "js/collections/ass -
${_("Choose File")} diff --git a/cms/templates/settings.html b/cms/templates/settings.html index 10b9055979..4aa5912c6e 100644 --- a/cms/templates/settings.html +++ b/cms/templates/settings.html @@ -24,7 +24,7 @@ CMS.URL.UPLOAD_ASSET = '${upload_asset_url}'; require(["domReady!", "jquery", "js/models/settings/course_details", "js/views/settings/main"], function(doc, $, CourseDetailsModel, MainView) { - // hilighting labels when fields are focused in + // highlighting labels when fields are focused in $("form :input").focus(function() { $("label[for='" + this.id + "']").addClass("is-focused"); }).blur(function() { @@ -215,7 +215,7 @@ require(["domReady!", "jquery", "js/models/settings/course_details", "js/views/s <% ctx_loc = context_course.location %> - ${_("You can manage this image along with all of your other")} ${_("files & uploads")} + ${_("You can manage this image along with all of your other")} ${_("files & uploads")} % else: diff --git a/cms/templates/widgets/header.html b/cms/templates/widgets/header.html index 8b22ad4812..764b1429dc 100644 --- a/cms/templates/widgets/header.html +++ b/cms/templates/widgets/header.html @@ -19,6 +19,7 @@ index_url = location.url_reverse('course/', '') checklists_url = location.url_reverse('checklists/', '') course_team_url = location.url_reverse('course_team/', '') + assets_url = location.url_reverse('assets/', '') %>

${_("Current Course:")} @@ -47,7 +48,7 @@ ${_("Static Pages")}