From 50128cfb5ca5d7c0402d0c072277d4843923cad9 Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 20 Nov 2013 14:17:26 -0500 Subject: [PATCH 1/2] Convert edit_subsection, edit_unit, and publishing to RESTful URLs. STUD-844 --- CHANGELOG.rst | 4 + .../contentstore/tests/test_contentstore.py | 79 +-- .../contentstore/tests/test_import_export.py | 2 +- .../contentstore/tests/test_item.py | 105 +++- .../contentstore/tests/test_transcripts.py | 14 +- .../contentstore/views/component.py | 511 ++++++++---------- .../contentstore/views/import_export.py | 19 +- cms/djangoapps/contentstore/views/item.py | 39 +- cms/static/coffee/src/views/unit.coffee | 18 +- cms/static/js/base.js | 2 +- cms/templates/overview.html | 2 +- cms/templates/unit.html | 17 +- cms/templates/widgets/units.html | 2 +- cms/urls.py | 8 +- .../xmodule/modulestore/loc_mapper_store.py | 13 +- .../xmodule/xmodule/modulestore/mongo/base.py | 6 +- .../modulestore/tests/test_location_mapper.py | 4 +- 17 files changed, 458 insertions(+), 387 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 0a5b25f469..0f633be99e 100644 --- a/CHANGELOG.rst +++ b/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. diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 237a60809e..bc25687342 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/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,19 @@ 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) + print "Checking {0}....".format(unicode(unit_locator)) + print descriptor.__class__, descriptor.location + 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 +1602,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 +1624,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 +1682,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 +1902,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 +2035,8 @@ 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)) + num_jump_to_preview = len(re.findall(r"/preview/(\S)*jump_to/i4x", content)) + num_jump_to_live = len(re.findall(r":8000/(\S)*jump_to/i4x", content)) + hits = len(re.findall(r"i4x", content)) - num_jump_to_preview - num_jump_to_live - test.assertEqual(total_i4x - num_jump_to, 0, "i4x found outside of LMS jump-to links") + test.assertEqual(hits, 0, "i4x found outside of LMS jump-to links") diff --git a/cms/djangoapps/contentstore/tests/test_import_export.py b/cms/djangoapps/contentstore/tests/test_import_export.py index 20957a3508..85df894cd4 100644 --- a/cms/djangoapps/contentstore/tests/test_import_export.py +++ b/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. """ diff --git a/cms/djangoapps/contentstore/tests/test_item.py b/cms/djangoapps/contentstore/tests/test_item.py index b34dab0a6d..4922b4888a 100644 --- a/cms/djangoapps/contentstore/tests/test_item.py +++ b/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)) diff --git a/cms/djangoapps/contentstore/tests/test_transcripts.py b/cms/djangoapps/contentstore/tests/test_transcripts.py index 695fdcd09f..4c481383ab 100644 --- a/cms/djangoapps/contentstore/tests/test_transcripts.py +++ b/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 = '' 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(""" @@ -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(""" diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index fd3344f76b..5c558a83d3 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/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,90 +51,87 @@ 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() +def subsection_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): + """ + The restful handler for subsection-specific requests. - if not has_access(request.user, course.location): - raise PermissionDenied() + 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: + old_location, course, item, lms_link = _get_item_in_course(request, locator) + except ItemNotFoundError: + return HttpResponseBadRequest() - try: - item = modulestore().get_item(location, depth=1) - except ItemNotFoundError: - return HttpResponseBadRequest() + preview_link = get_lms_link_for_item(old_location, course_id=course.location.course_id, preview=True) - 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) + # make sure that location references a 'sequential', otherwise return + # BadRequest + if item.location.category != 'sequential': + return HttpResponseBadRequest() - # make sure that location references a 'sequential', otherwise return - # BadRequest - if item.location.category != 'sequential': - return HttpResponseBadRequest() + parent_locs = modulestore().get_parent_locations(old_location, None) - parent_locs = modulestore().get_parent_locations(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', + old_location + ) - # 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 + # this should blow up if we don't find any parents, which would be erroneous + parent = modulestore().get_item(parent_locs[0]) + + # remove all metadata from the generic dictionary that is presented in a + # more normalized UI. We only want to display the XBlocks fields, not + # the fields from any mixins that have been added + fields = getattr(item, 'unmixed_class', item.__class__).fields + + policy_metadata = dict( + (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 ) - # this should blow up if we don't find any parents, which would be erroneous - parent = modulestore().get_item(parent_locs[0]) + can_view_live = False + subsection_units = item.get_children() + for unit in subsection_units: + state = compute_unit_state(unit) + if state == UnitState.public or state == UnitState.draft: + can_view_live = True + break - # remove all metadata from the generic dictionary that is presented in a - # more normalized UI. We only want to display the XBlocks fields, not - # the fields from any mixins that have been added - fields = getattr(item, 'unmixed_class', item.__class__).fields + course_locator = loc_mapper().translate_location( + course.location.course_id, course.location, False, True + ) - policy_metadata = dict( - (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 - ) - - can_view_live = False - subsection_units = item.get_children() - for unit in subsection_units: - state = compute_unit_state(unit) - if state == UnitState.public or state == UnitState.draft: - can_view_live = True - break - - 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', - { - 'subsection': item, - 'context_course': course, - 'new_unit_category': 'vertical', - 'lms_link': lms_link, - 'preview_link': preview_link, - 'course_graders': json.dumps(CourseGradingModel.fetch(course_locator).graders), - 'parent_item': parent, - 'locator': locator, - 'policy_metadata': policy_metadata, - 'subsection_units': subsection_units, - 'can_view_live': can_view_live - } - ) + return render_to_response( + 'edit_subsection.html', + { + 'subsection': item, + 'context_course': course, + 'new_unit_category': 'vertical', + 'lms_link': lms_link, + 'preview_link': preview_link, + 'course_graders': json.dumps(CourseGradingModel.fetch(course_locator).graders), + 'parent_item': parent, + 'locator': locator, + 'policy_metadata': policy_metadata, + 'subsection_units': subsection_units, + '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,224 +140,178 @@ 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 """ - try: - course = get_course_for_item(location) - except InvalidLocationError: - return HttpResponseBadRequest() + 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: + old_location, course, item, lms_link = _get_item_in_course(request, locator) + except ItemNotFoundError: + return HttpResponseBadRequest() - if not has_access(request.user, course.location): - raise PermissionDenied() + component_templates = defaultdict(list) + for category in COMPONENT_TYPES: + 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) + if hasattr(component_class, 'display_name'): + display_name = component_class.display_name.default or 'Blank' + else: + display_name = 'Blank' + component_templates[category].append(( + display_name, + category, + False, # No defaults have markdown (hardcoded current default) + None # no boilerplate for overrides + )) + # add boilerplates + if hasattr(component_class, 'templates'): + for template in component_class.templates(): + filter_templates = getattr(component_class, 'filter_templates', None) + if not filter_templates or filter_templates(template, course): + component_templates[category].append(( + template['metadata'].get('display_name'), + category, + template['metadata'].get('markdown') is not None, + template.get('template_id') + )) - try: - item = modulestore().get_item(location, depth=1) - except ItemNotFoundError: - return HttpResponseBadRequest() - lms_link = get_lms_link_for_item( - item.location, - course_id=course.location.course_id - ) + # Check if there are any advanced modules specified in the course policy. + # These modules should be specified as a list of strings, where the strings + # are the names of the modules in ADVANCED_COMPONENT_TYPES that should be + # enabled for the course. + course_advanced_keys = course.advanced_modules - # 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 - ) + # Set component types according to course policy file + if isinstance(course_advanced_keys, list): + for category in course_advanced_keys: + if category in ADVANCED_COMPONENT_TYPES: + # Do I need to allow for boilerplates or just defaults on the + # 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_templates = defaultdict(list) - for category in COMPONENT_TYPES: - 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) - if hasattr(component_class, 'display_name'): - display_name = component_class.display_name.default or 'Blank' + 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 + # not exist on the server. This code here merely + # prevents any authors from trying to instantiate the + # non-existent component type by not showing it in the menu + pass else: - display_name = 'Blank' - component_templates[category].append(( - display_name, - category, - False, # No defaults have markdown (hardcoded current default) - None # no boilerplate for overrides - )) - # add boilerplates - if hasattr(component_class, 'templates'): - for template in component_class.templates(): - filter_templates = getattr(component_class, 'filter_templates', None) - if not filter_templates or filter_templates(template, course): - component_templates[category].append(( - template['metadata'].get('display_name'), - category, - template['metadata'].get('markdown') is not None, - template.get('template_id') - )) + log.error( + "Improper format for course advanced keys! %s", + course_advanced_keys + ) - # Check if there are any advanced modules specified in the course policy. - # These modules should be specified as a list of strings, where the strings - # are the names of the modules in ADVANCED_COMPONENT_TYPES that should be - # enabled for the course. - course_advanced_keys = course.advanced_modules + components = [ + [ + # TODO: old location needed for video transcripts. + component.location.url(), + loc_mapper().translate_location( + course.location.course_id, component.location, False, True + ) + ] + for component + in item.get_children() + ] - # Set component types according to course policy file - if isinstance(course_advanced_keys, list): - for category in course_advanced_keys: - if category in ADVANCED_COMPONENT_TYPES: - # Do I need to allow for boilerplates or just defaults on the - # 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) + # TODO (cpennington): If we share units between courses, + # this will need to change to check permissions correctly so as + # to pick the correct parent subsection - 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 - # not exist on the server. This code here merely - # prevents any authors from trying to instantiate the - # non-existent component type by not showing it in the menu - pass - else: - log.error( - "Improper format for course advanced keys! %s", - course_advanced_keys + 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 + ) + containing_section = modulestore().get_item(containing_section_locs[0]) + + # cdodge hack. We're having trouble previewing drafts via jump_to redirect + # so let's generate the link url here + + # need to figure out where this item is in the list of children as the + # preview will need this + index = 1 + for child in containing_subsection.get_children(): + if child.location == item.location: + break + index = index + 1 + + preview_lms_base = settings.MITX_FEATURES.get('PREVIEW_LMS_BASE') + + preview_lms_link = ( + '//{preview_lms_base}/courses/{org}/{course}/' + '{course_name}/courseware/{section}/{subsection}/{index}' + ).format( + preview_lms_base=preview_lms_base, + lms_base=settings.LMS_BASE, + org=course.location.org, + course=course.location.course, + course_name=course.location.name, + section=containing_section.location.name, + subsection=containing_subsection.location.name, + index=index ) - components = [ - [ - component.location.url(), - loc_mapper().translate_location( - course.location.course_id, component.location, False, True - ) - ] - for component - in item.get_children() - ] - - # TODO (cpennington): If we share units between courses, - # 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 = modulestore().get_item(containing_subsection_locs[0]) - containing_section_locs = modulestore().get_parent_locations( - containing_subsection.location, None - ) - containing_section = modulestore().get_item(containing_section_locs[0]) - - # cdodge hack. We're having trouble previewing drafts via jump_to redirect - # so let's generate the link url here - - # need to figure out where this item is in the list of children as the - # preview will need this - index = 1 - for child in containing_subsection.get_children(): - if child.location == item.location: - break - index = index + 1 - - preview_lms_base = settings.MITX_FEATURES.get('PREVIEW_LMS_BASE') - - preview_lms_link = ( - '//{preview_lms_base}/courses/{org}/{course}/' - '{course_name}/courseware/{section}/{subsection}/{index}' - ).format( - preview_lms_base=preview_lms_base, - lms_base=settings.LMS_BASE, - org=course.location.org, - course=course.location.course, - course_name=course.location.name, - section=containing_section.location.name, - subsection=containing_subsection.location.name, - index=index - ) - - return render_to_response('unit.html', { - 'context_course': course, - 'unit': item, - # Still needed for creating a draft. - 'unit_location': location, - 'unit_locator': unit_locator, - 'components': components, - 'component_templates': component_templates, - 'draft_preview_link': preview_lms_link, - 'published_preview_link': lms_link, - 'subsection': containing_subsection, - 'release_date': ( - get_default_time_display(containing_subsection.start) - if containing_subsection.start is not None else None - ), - 'section': containing_section, - 'new_unit_category': 'vertical', - 'unit_state': compute_unit_state(item), - 'published_date': ( - get_default_time_display(item.published_date) - if item.published_date is not None else None - ), - }) + return render_to_response('unit.html', { + 'context_course': course, + 'unit': item, + 'unit_locator': locator, + 'components': components, + 'component_templates': component_templates, + 'draft_preview_link': preview_lms_link, + 'published_preview_link': lms_link, + 'subsection': containing_subsection, + 'release_date': ( + get_default_time_display(containing_subsection.start) + if containing_subsection.start is not None else None + ), + 'section': containing_section, + 'new_unit_category': 'vertical', + 'unit_state': compute_unit_state(item), + 'published_date': ( + get_default_time_display(item.published_date) + 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): +def _get_item_in_course(request, locator): """ - Publish a draft + Helper method for getting the old location, containing course, + item, and lms_link for a given locator. + + Verifies that the caller has permission to access this item. """ - location = request.json['id'] - - # check permissions for this user within this course - if not has_access(request.user, location): + if not has_access(request.user, locator): raise PermissionDenied() - item = modulestore().get_item(location) - _xmodule_recurse( - item, - lambda i: modulestore().publish(i.location, request.user.id) - ) + 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() - - -@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): - raise PermissionDenied() - - item = modulestore().get_item(location) - _xmodule_recurse(item, lambda i: modulestore().unpublish(i.location)) - - return HttpResponse() + return old_location, course, item, lms_link diff --git a/cms/djangoapps/contentstore/views/import_export.py b/cms/djangoapps/contentstore/views/import_export.py index 5d1b26ec3d..f740d10707 100644 --- a/cms/djangoapps/contentstore/views/import_export.py +++ b/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 }) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 5d6f560dcc..220da038a7 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/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'] @@ -57,7 +56,7 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid= all children and "all_versions" to delete from all (mongo) versions. GET json: returns representation of the xblock (locator id, data, and metadata). - if ?fields=graderType, it returns the graderType for the unit instead of the above. + if ?fields=graderType, it returns the graderType for the unit instead of the above. html: returns HTML for rendering the xblock (which includes both the "preview" view and the "editor" view) PUT or POST json: if xblock locator is specified, update the xblock instance. The json payload can contain @@ -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): diff --git a/cms/static/coffee/src/views/unit.coffee b/cms/static/coffee/src/views/unit.coffee index e6ba0e1382..c4ff25c309 100644 --- a/cms/static/coffee/src/views/unit.coffee +++ b/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 diff --git a/cms/static/js/base.js b/cms/static/js/base.js index bc7260cf64..174ab10c89 100644 --- a/cms/static/js/base.js +++ b/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']; }); } diff --git a/cms/templates/overview.html b/cms/templates/overview.html index c260e987b2..61b00278a4 100644 --- a/cms/templates/overview.html +++ b/cms/templates/overview.html @@ -207,7 +207,7 @@ require(["domReady!", "jquery", "js/models/location", "js/models/section", "js/v
- + ${subsection.display_name_with_default} diff --git a/cms/templates/unit.html b/cms/templates/unit.html index 233b3fbd16..07ab8de693 100644 --- a/cms/templates/unit.html +++ b/cms/templates/unit.html @@ -34,7 +34,7 @@ require(["domReady!", "jquery", "js/models/module_info", "coffee/src/views/unit" <%block name="content"> -
+

${_("You are editing a draft.")} @@ -135,6 +135,13 @@ require(["domReady!", "jquery", "js/models/module_info", "coffee/src/views/unit"

+ <% + 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') + %>

${_("Unit Settings")}

@@ -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=''.format(url=reverse('edit_subsection', kwargs={'location': subsection.location})), + link_start=''.format(url=subsection_url), link_end='', )}

@@ -180,14 +187,10 @@ require(["domReady!", "jquery", "js/models/module_info", "coffee/src/views/unit"
  1. - <% - ctx_loc = context_course.location - index_url = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True).url_reverse('course/', '') - %> ${section.display_name_with_default}
    1. - + ${subsection.display_name_with_default} diff --git a/cms/templates/widgets/units.html b/cms/templates/widgets/units.html index 58d9c50194..edc32ed947 100644 --- a/cms/templates/widgets/units.html +++ b/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 = '' %>
      - + ${unit.display_name_with_default} diff --git a/cms/urls.py b/cms/urls.py index f46e04c4a1..ac50c623b8 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -11,8 +11,6 @@ from ratelimitbackend import admin admin.autodiscover() urlpatterns = patterns('', # nopep8 - url(r'^edit/(?P.*?)$', 'contentstore.views.edit_unit', name='edit_unit'), - url(r'^subsection/(?P.*?)$', '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.*?)/handler/(?P[^/]*)(?:/(?P[^/]*))?$', '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\d+)?$'.format(parsers.URL_RE_SOURCE), 'checklists_handler'), url(r'(?ix)^orphan/{}$'.format(parsers.URL_RE_SOURCE), 'orphan_handler'), url(r'(?ix)^assets/{}(/)?(?P.+)?$'.format(parsers.URL_RE_SOURCE), 'assets_handler'), diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index 33b65b8b10..c7355c2f29 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/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): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index f368032f17..2f6fabfce0 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py index 2a1c165993..2fbff10423 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py +++ b/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' ) From b5d72a38d38d32440cda00c9fbbdebca328d396f Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 25 Nov 2013 13:54:45 -0500 Subject: [PATCH 2/2] Updates from code review. Includes backing out the "as_published" changes I previously put in, as now we are not adding @draft to locations converted from locators. --- .../contentstore/tests/test_contentstore.py | 7 +------ .../contentstore/views/component.py | 2 +- .../xmodule/modulestore/mongo/draft.py | 10 +++++----- .../xmodule/modulestore/xml_exporter.py | 19 +++++++++---------- 4 files changed, 16 insertions(+), 22 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index bc25687342..aaf2cf7660 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1352,8 +1352,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertGreater(len(items), 0) for descriptor in items: unit_locator = loc_mapper().translate_location(course_id, descriptor.location, False, True) - print "Checking {0}....".format(unicode(unit_locator)) - print descriptor.__class__, descriptor.location 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. @@ -2035,8 +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_preview = len(re.findall(r"/preview/(\S)*jump_to/i4x", content)) - num_jump_to_live = len(re.findall(r":8000/(\S)*jump_to/i4x", content)) - hits = len(re.findall(r"i4x", content)) - num_jump_to_preview - num_jump_to_live - + hits = len(re.findall(r"(? 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: - logging.debug('parent_locs = {0}'.format(parent_locs)) - draft_vertical.xml_attributes['parent_sequential_url'] = Location(parent_locs[0]).url() - sequential = modulestore.get_item(Location(parent_locs[0])) - index = sequential.children.index(draft_vertical.location.url()) - draft_vertical.xml_attributes['index_in_children_list'] = str(index) - draft_vertical.export_to_xml(draft_course_dir) + 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: + logging.debug('parent_locs = {0}'.format(parent_locs)) + draft_vertical.xml_attributes['parent_sequential_url'] = Location(parent_locs[0]).url() + sequential = modulestore.get_item(Location(parent_locs[0])) + index = sequential.children.index(draft_vertical.location.url()) + draft_vertical.xml_attributes['index_in_children_list'] = str(index) + draft_vertical.export_to_xml(draft_course_dir) def export_extra_content(export_fs, modulestore, course_id, course_location, category_type, dirname, file_suffix=''):