From a170c6f4e74538ed4e117a8b5fb987c7fe08421c Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 6 Nov 2013 10:50:00 -0500 Subject: [PATCH] Change save_item and create_item to RESTful URL. Part of STUD-847. --- .../contentstore/features/common.py | 15 + .../contentstore/features/component.py | 9 +- .../features/problem-editor.feature | 15 + .../contentstore/features/problem-editor.py | 10 +- .../contentstore/tests/test_contentstore.py | 44 ++- .../contentstore/tests/test_item.py | 268 ++++++++--------- .../contentstore/tests/test_transcripts.py | 20 +- .../contentstore/views/component.py | 15 +- cms/djangoapps/contentstore/views/course.py | 2 + cms/djangoapps/contentstore/views/item.py | 279 ++++++++---------- cms/djangoapps/contentstore/views/tabs.py | 18 +- cms/static/coffee/spec/main.coffee | 10 +- .../coffee/spec/models/module_spec.coffee | 7 - .../coffee/spec/models/section_spec.coffee | 9 +- .../coffee/spec/views/module_edit_spec.coffee | 5 +- .../coffee/spec/views/overview_spec.coffee | 24 +- cms/static/coffee/src/models/module.coffee | 4 - .../coffee/src/views/module_edit.coffee | 13 +- cms/static/coffee/src/views/tabs.coffee | 20 +- cms/static/coffee/src/views/unit.coffee | 20 +- cms/static/js/base.js | 23 +- cms/static/js/models/module_info.js | 4 +- cms/static/js/models/section.js | 7 +- .../spec}/transcripts/editor_spec.js | 0 .../spec}/transcripts/file_uploader_spec.js | 0 .../spec}/transcripts/message_manager_spec.js | 0 .../spec}/transcripts/utils_spec.js | 0 .../spec}/transcripts/videolist_spec.js | 0 cms/static/js/spec/utils/module_spec.js | 17 ++ cms/static/js/utils/module.js | 26 ++ cms/static/js/views/overview.js | 55 ++-- cms/static/js_spec/helpers.js | 0 cms/static/js_test.yml | 2 +- cms/templates/edit-tabs.html | 10 +- cms/templates/edit_subsection.html | 2 +- cms/templates/overview.html | 36 ++- cms/templates/textbooks.html | 1 - cms/templates/unit.html | 14 +- cms/templates/widgets/units.html | 14 +- cms/urls.py | 4 +- common/djangoapps/terrain/ui_helpers.py | 2 +- .../xmodule/modulestore/mongo/draft.py | 11 +- 42 files changed, 546 insertions(+), 489 deletions(-) delete mode 100644 cms/static/coffee/spec/models/module_spec.coffee delete mode 100644 cms/static/coffee/src/models/module.coffee rename cms/static/{js_spec => js/spec}/transcripts/editor_spec.js (100%) rename cms/static/{js_spec => js/spec}/transcripts/file_uploader_spec.js (100%) rename cms/static/{js_spec => js/spec}/transcripts/message_manager_spec.js (100%) rename cms/static/{js_spec => js/spec}/transcripts/utils_spec.js (100%) rename cms/static/{js_spec => js/spec}/transcripts/videolist_spec.js (100%) create mode 100644 cms/static/js/spec/utils/module_spec.js create mode 100644 cms/static/js/utils/module.js delete mode 100644 cms/static/js_spec/helpers.js diff --git a/cms/djangoapps/contentstore/features/common.py b/cms/djangoapps/contentstore/features/common.py index a54a421de3..4a500c9e16 100644 --- a/cms/djangoapps/contentstore/features/common.py +++ b/cms/djangoapps/contentstore/features/common.py @@ -385,3 +385,18 @@ def create_other_user(_step, name, has_extra_perms, role_name): @step('I log out') def log_out(_step): world.visit('logout') + + +@step(u'I click on "edit a draft"$') +def i_edit_a_draft(_step): + world.css_click("a.create-draft") + + +@step(u'I click on "replace with draft"$') +def i_edit_a_draft(_step): + world.css_click("a.publish-draft") + + +@step(u'I publish the unit$') +def publish_unit(_step): + world.select_option('visibility-select', 'public') diff --git a/cms/djangoapps/contentstore/features/component.py b/cms/djangoapps/contentstore/features/component.py index 14c9644fab..505ac9bad3 100644 --- a/cms/djangoapps/contentstore/features/component.py +++ b/cms/djangoapps/contentstore/features/component.py @@ -87,13 +87,18 @@ def add_component_category(step, component, category): @step(u'I delete all components$') def delete_all_components(step): + count = len(world.css_find('ol.components li.component')) + step.given('I delete "' + str(count) + '" component') + + +@step(u'I delete "([^"]*)" component$') +def delete_components(step, number): world.wait_for_xmodule() delete_btn_css = 'a.delete-button' prompt_css = 'div#prompt-warning' btn_css = '{} a.button.action-primary'.format(prompt_css) saving_mini_css = 'div#page-notification .wrapper-notification-mini' - count = len(world.css_find('ol.components li.component')) - for _ in range(int(count)): + for _ in range(int(number)): world.css_click(delete_btn_css) assert_true( world.is_css_present('{}.is-shown'.format(prompt_css)), diff --git a/cms/djangoapps/contentstore/features/problem-editor.feature b/cms/djangoapps/contentstore/features/problem-editor.feature index 843161ee0d..eb66b8ffd9 100644 --- a/cms/djangoapps/contentstore/features/problem-editor.feature +++ b/cms/djangoapps/contentstore/features/problem-editor.feature @@ -81,6 +81,21 @@ Feature: CMS.Problem Editor When I edit and select Settings Then Edit High Level Source is visible + # This is a very specific scenario that was failing with some of the + # DB rearchitecture changes. It had to do with children IDs being stored + # with @draft at the end. To reproduce, must update children while in draft mode. + Scenario: Problems can be deleted after being public + Given I have created a Blank Common Problem + And I have created another Blank Common Problem + When I publish the unit + And I click on "edit a draft" + And I delete "1" component + And I click on "replace with draft" + And I click on "edit a draft" + And I delete "1" component + Then I see no components + + # Disabled 11/13/2013 after failing in master # The screenshot showed that the LaTeX editor had the text "hi", # but Selenium timed out waiting for the text to appear. diff --git a/cms/djangoapps/contentstore/features/problem-editor.py b/cms/djangoapps/contentstore/features/problem-editor.py index 758ac299be..2265b5010e 100644 --- a/cms/djangoapps/contentstore/features/problem-editor.py +++ b/cms/djangoapps/contentstore/features/problem-editor.py @@ -19,6 +19,11 @@ SHOW_ANSWER = "Show Answer" @step('I have created a Blank Common Problem$') def i_created_blank_common_problem(step): world.create_course_with_unit() + step.given("I have created another Blank Common Problem") + + +@step('I have created another Blank Common Problem$') +def i_create_new_common_problem(step): world.create_component_instance( step=step, category='problem', @@ -218,11 +223,6 @@ def i_import_the_file(_step, filename): import_file(filename) -@step(u'I click on "edit a draft"$') -def i_edit_a_draft(_step): - world.css_click("a.create-draft") - - @step(u'I go to the vertical "([^"]*)"$') def i_go_to_vertical(_step, vertical): world.css_click("span:contains('{0}')".format(vertical)) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index a6fa4e77b3..58cf3be70b 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -398,9 +398,15 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertEqual(course.tabs, expected_tabs) def test_static_tab_reordering(self): + def get_tab_locator(tab): + tab_location = 'i4x://MITx/999/static_tab/{0}'.format(tab['url_slug']) + return unicode(loc_mapper().translate_location( + course.location.course_id, Location(tab_location), False, True + )) + module_store = modulestore('direct') - CourseFactory.create(org='edX', course='999', display_name='Robot Super Course') - course_location = Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None]) + locator = _course_factory_create_course() + course_location = loc_mapper().translate_locator_to_location(locator) ItemFactory.create( parent_location=course_location, @@ -411,23 +417,23 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): category="static_tab", display_name="Static_2") - course = module_store.get_item(Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None])) + course = module_store.get_item(course_location) # reverse the ordering reverse_tabs = [] for tab in course.tabs: if tab['type'] == 'static_tab': - reverse_tabs.insert(0, 'i4x://edX/999/static_tab/{0}'.format(tab['url_slug'])) + reverse_tabs.insert(0, get_tab_locator(tab)) self.client.ajax_post(reverse('reorder_static_tabs'), {'tabs': reverse_tabs}) - course = module_store.get_item(Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None])) + course = module_store.get_item(course_location) # compare to make sure that the tabs information is in the expected order after the server call course_tabs = [] for tab in course.tabs: if tab['type'] == 'static_tab': - course_tabs.append('i4x://edX/999/static_tab/{0}'.format(tab['url_slug'])) + course_tabs.append(get_tab_locator(tab)) self.assertEqual(reverse_tabs, course_tabs) @@ -1528,22 +1534,22 @@ class ContentStoreTest(ModuleStoreTestCase): resp = self._show_course_overview(loc) self.assertContains( resp, - '
', + '
', status_code=200, html=True ) def test_create_item(self): - """Test cloning an item. E.g. creating a new section""" - CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course') + """Test creating a new xblock instance.""" + locator = _course_factory_create_course() section_data = { - 'parent_location': 'i4x://MITx/999/course/Robot_Super_Course', + 'parent_locator': unicode(locator), 'category': 'chapter', 'display_name': 'Section One', } - resp = self.client.ajax_post(reverse('create_item'), section_data) + resp = self.client.ajax_post('/xblock', section_data) self.assertEqual(resp.status_code, 200) data = parse_json(resp) @@ -1554,14 +1560,14 @@ class ContentStoreTest(ModuleStoreTestCase): def test_capa_module(self): """Test that a problem treats markdown specially.""" - CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course') + locator = _course_factory_create_course() problem_data = { - 'parent_location': 'i4x://MITx/999/course/Robot_Super_Course', + 'parent_locator': unicode(locator), 'category': 'problem' } - resp = self.client.ajax_post(reverse('create_item'), problem_data) + resp = self.client.ajax_post('/xblock', problem_data) self.assertEqual(resp.status_code, 200) payload = parse_json(resp) @@ -1911,7 +1917,7 @@ class MetadataSaveTestCase(ModuleStoreTestCase): def _create_course(test, course_data): """ - Creates a course and verifies the URL returned in the response.. + Creates a course via an AJAX request and verifies the URL returned in the response. """ course_id = _get_course_id(course_data) new_location = loc_mapper().translate_location(course_id, CourseDescriptor.id_to_location(course_id), False, True) @@ -1923,6 +1929,14 @@ def _create_course(test, course_data): test.assertEqual(data['url'], new_location.url_reverse("course/", "")) +def _course_factory_create_course(): + """ + Creates a course via the CourseFactory and returns the locator for it. + """ + course = CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course') + return loc_mapper().translate_location(course.location.course_id, course.location, False, True) + + def _get_course_id(test_course_data): """Returns the course ID (org/number/run).""" return "{org}/{number}/{run}".format(**test_course_data) diff --git a/cms/djangoapps/contentstore/tests/test_item.py b/cms/djangoapps/contentstore/tests/test_item.py index 6d42d400fc..e14a343fcf 100644 --- a/cms/djangoapps/contentstore/tests/test_item.py +++ b/cms/djangoapps/contentstore/tests/test_item.py @@ -3,109 +3,110 @@ import json import datetime from pytz import UTC -from django.core.urlresolvers import reverse from contentstore.tests.utils import CourseTestCase -from xmodule.modulestore.tests.factories import CourseFactory 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 -class DeleteItem(CourseTestCase): - """Tests for '/xblock' DELETE url.""" +class ItemTest(CourseTestCase): + """ Base test class for create, save, and delete """ def setUp(self): - """ Creates the test course with a static page in it. """ - super(DeleteItem, self).setUp() - self.course = CourseFactory.create(org='mitX', number='333', display_name='Dummy Course') + super(ItemTest, self).setUp() + self.unicode_locator = unicode(loc_mapper().translate_location( + self.course.location.course_id, self.course.location, False, True + )) + + def get_old_id(self, locator): + """ + Converts new locator to old id format. + """ + return loc_mapper().translate_locator_to_location(BlockUsageLocator(locator)) + + def get_item_from_modulestore(self, locator, draft=False): + """ + Get the item referenced by the locator from the modulestore + """ + store = modulestore('draft') if draft else modulestore() + return store.get_item(self.get_old_id(locator)) + + def response_locator(self, response): + """ + Get the locator (unicode representation) from the response payload + :param response: + """ + parsed = json.loads(response.content) + return parsed['locator'] + + def create_xblock(self, parent_locator=None, display_name=None, category=None, boilerplate=None): + data = { + 'parent_locator': self.unicode_locator if parent_locator is None else parent_locator, + 'category': category + } + if display_name is not None: + data['display_name'] = display_name + if boilerplate is not None: + data['boilerplate'] = boilerplate + return self.client.ajax_post('/xblock', json.dumps(data)) + + +class DeleteItem(ItemTest): + """Tests for '/xblock' DELETE url.""" def test_delete_static_page(self): # Add static tab - data = json.dumps({ - 'parent_location': 'i4x://mitX/333/course/Dummy_Course', - 'category': 'static_tab' - }) - - resp = self.client.post( - reverse('create_item'), - data, - content_type="application/json" - ) + resp = self.create_xblock(category='static_tab') 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_content = json.loads(resp.content) - resp = self.client.delete(resp_content['update_url']) + resp = self.client.delete('/xblock/' + resp_content['locator']) self.assertEqual(resp.status_code, 204) -class TestCreateItem(CourseTestCase): +class TestCreateItem(ItemTest): """ Test the create_item handler thoroughly """ - def response_id(self, response): - """ - Get the id from the response payload - :param response: - """ - parsed = json.loads(response.content) - return parsed['id'] - def test_create_nicely(self): """ Try the straightforward use cases """ # create a chapter display_name = 'Nicely created' - resp = self.client.post( - reverse('create_item'), - json.dumps({ - 'parent_location': self.course.location.url(), - 'display_name': display_name, - 'category': 'chapter' - }), - content_type="application/json" - ) + resp = self.create_xblock(display_name=display_name, category='chapter') self.assertEqual(resp.status_code, 200) # get the new item and check its category and display_name - chap_location = self.response_id(resp) - new_obj = modulestore().get_item(chap_location) + chap_locator = self.response_locator(resp) + new_obj = self.get_item_from_modulestore(chap_locator) self.assertEqual(new_obj.scope_ids.block_type, 'chapter') self.assertEqual(new_obj.display_name, display_name) self.assertEqual(new_obj.location.org, self.course.location.org) self.assertEqual(new_obj.location.course, self.course.location.course) # get the course and ensure it now points to this one - course = modulestore().get_item(self.course.location) - self.assertIn(chap_location, course.children) + course = self.get_item_from_modulestore(self.unicode_locator) + self.assertIn(self.get_old_id(chap_locator).url(), course.children) # use default display name - resp = self.client.post( - reverse('create_item'), - json.dumps({ - 'parent_location': chap_location, - 'category': 'vertical' - }), - content_type="application/json" - ) + resp = self.create_xblock(parent_locator=chap_locator, category='vertical') self.assertEqual(resp.status_code, 200) - vert_location = self.response_id(resp) + vert_locator = self.response_locator(resp) # create problem w/ boilerplate template_id = 'multiplechoice.yaml' - resp = self.client.post( - reverse('create_item'), - json.dumps({ - 'parent_location': vert_location, - 'category': 'problem', - 'boilerplate': template_id - }), - content_type="application/json" + resp = self.create_xblock( + parent_locator=vert_locator, + category='problem', + boilerplate=template_id ) self.assertEqual(resp.status_code, 200) - prob_location = self.response_id(resp) - problem = modulestore('draft').get_item(prob_location) + prob_locator = self.response_locator(resp) + problem = self.get_item_from_modulestore(prob_locator, True) # ensure it's draft self.assertTrue(problem.is_draft) # check against the template @@ -119,133 +120,102 @@ class TestCreateItem(CourseTestCase): Negative tests for create_item """ # non-existent boilerplate: creates a default - resp = self.client.post( - reverse('create_item'), - json.dumps( - {'parent_location': self.course.location.url(), - 'category': 'problem', - 'boilerplate': 'nosuchboilerplate.yaml' - }), - content_type="application/json" - ) + resp = self.create_xblock(category='problem', boilerplate='nosuchboilerplate.yaml') self.assertEqual(resp.status_code, 200) -class TestEditItem(CourseTestCase): +class TestEditItem(ItemTest): """ - Test contentstore.views.item.save_item + Test xblock update. """ - def response_id(self, response): - """ - Get the id from the response payload - :param response: - """ - parsed = json.loads(response.content) - return parsed['id'] - def setUp(self): """ Creates the test course structure and a couple problems to 'edit'. """ super(TestEditItem, self).setUp() # create a chapter display_name = 'chapter created' - resp = self.client.post( - reverse('create_item'), - json.dumps( - {'parent_location': self.course.location.url(), - 'display_name': display_name, - 'category': 'chapter' - }), - content_type="application/json" - ) - chap_location = self.response_id(resp) - resp = self.client.post( - reverse('create_item'), - json.dumps({ - 'parent_location': chap_location, - 'category': 'sequential', - }), - content_type="application/json" - ) - self.seq_location = self.response_id(resp) + resp = self.create_xblock(display_name=display_name, category='chapter') + chap_locator = self.response_locator(resp) + resp = self.create_xblock(parent_locator=chap_locator, category='sequential') + self.seq_locator = self.response_locator(resp) + self.seq_update_url = '/xblock/' + self.seq_locator + # create problem w/ boilerplate template_id = 'multiplechoice.yaml' - resp = self.client.post( - reverse('create_item'), - json.dumps({ - 'parent_location': self.seq_location, - 'category': 'problem', - 'boilerplate': template_id, - }), - content_type="application/json" - ) - self.problems = [self.response_id(resp)] + resp = self.create_xblock(parent_locator=self.seq_locator, category='problem', boilerplate=template_id) + self.problem_locator = self.response_locator(resp) + self.problem_update_url = '/xblock/' + self.problem_locator + + self.course_update_url = '/xblock/' + self.unicode_locator def test_delete_field(self): """ Sending null in for a field 'deletes' it """ - self.client.post( - reverse('save_item'), - json.dumps({ - 'id': self.problems[0], - 'metadata': {'rerandomize': 'onreset'} - }), - content_type="application/json" + self.client.ajax_post( + self.problem_update_url, + data={'metadata': {'rerandomize': 'onreset'}} ) - problem = modulestore('draft').get_item(self.problems[0]) + problem = self.get_item_from_modulestore(self.problem_locator, True) self.assertEqual(problem.rerandomize, 'onreset') - self.client.post( - reverse('save_item'), - json.dumps({ - 'id': self.problems[0], - 'metadata': {'rerandomize': None} - }), - content_type="application/json" + self.client.ajax_post( + self.problem_update_url, + data={'metadata': {'rerandomize': None}} ) - problem = modulestore('draft').get_item(self.problems[0]) + problem = self.get_item_from_modulestore(self.problem_locator, True) self.assertEqual(problem.rerandomize, 'never') def test_null_field(self): """ Sending null in for a field 'deletes' it """ - problem = modulestore('draft').get_item(self.problems[0]) + problem = self.get_item_from_modulestore(self.problem_locator, True) self.assertIsNotNone(problem.markdown) - self.client.post( - reverse('save_item'), - json.dumps({ - 'id': self.problems[0], - 'nullout': ['markdown'] - }), - content_type="application/json" + self.client.ajax_post( + self.problem_update_url, + data={'nullout': ['markdown']} ) - problem = modulestore('draft').get_item(self.problems[0]) + problem = self.get_item_from_modulestore(self.problem_locator, True) self.assertIsNone(problem.markdown) def test_date_fields(self): """ Test setting due & start dates on sequential """ - sequential = modulestore().get_item(self.seq_location) + sequential = self.get_item_from_modulestore(self.seq_locator) self.assertIsNone(sequential.due) - self.client.post( - reverse('save_item'), - json.dumps({ - 'id': self.seq_location, - 'metadata': {'due': '2010-11-22T04:00Z'} - }), - content_type="application/json" + self.client.ajax_post( + self.seq_update_url, + data={'metadata': {'due': '2010-11-22T04:00Z'}} ) - sequential = modulestore().get_item(self.seq_location) + sequential = self.get_item_from_modulestore(self.seq_locator) self.assertEqual(sequential.due, datetime.datetime(2010, 11, 22, 4, 0, tzinfo=UTC)) - self.client.post( - reverse('save_item'), - json.dumps({ - 'id': self.seq_location, - 'metadata': {'start': '2010-09-12T14:00Z'} - }), - content_type="application/json" + self.client.ajax_post( + self.seq_update_url, + data={'metadata': {'start': '2010-09-12T14:00Z'}} ) - sequential = modulestore().get_item(self.seq_location) + sequential = self.get_item_from_modulestore(self.seq_locator) self.assertEqual(sequential.due, datetime.datetime(2010, 11, 22, 4, 0, tzinfo=UTC)) self.assertEqual(sequential.start, datetime.datetime(2010, 9, 12, 14, 0, tzinfo=UTC)) + + def test_children(self): + # Create 2 children of main course. + resp_1 = self.create_xblock(display_name='child 1', category='chapter') + resp_2 = self.create_xblock(display_name='child 2', category='chapter') + chapter1_locator = self.response_locator(resp_1) + chapter2_locator = self.response_locator(resp_2) + + course = self.get_item_from_modulestore(self.unicode_locator) + self.assertIn(self.get_old_id(chapter1_locator).url(), course.children) + self.assertIn(self.get_old_id(chapter2_locator).url(), course.children) + + # Remove one child from the course. + resp = self.client.ajax_post( + self.course_update_url, + data={'children': [chapter2_locator]} + ) + self.assertEqual(resp.status_code, 200) + + # Verify that the child is removed. + course = self.get_item_from_modulestore(self.unicode_locator) + self.assertNotIn(self.get_old_id(chapter1_locator).url(), course.children) + self.assertIn(self.get_old_id(chapter2_locator).url(), course.children) diff --git a/cms/djangoapps/contentstore/tests/test_transcripts.py b/cms/djangoapps/contentstore/tests/test_transcripts.py index a8d1c33370..695fdcd09f 100644 --- a/cms/djangoapps/contentstore/tests/test_transcripts.py +++ b/cms/djangoapps/contentstore/tests/test_transcripts.py @@ -19,6 +19,7 @@ from xmodule.modulestore.django import modulestore 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 contentstore.tests.modulestore_config import TEST_MODULESTORE TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) @@ -47,14 +48,17 @@ class Basetranscripts(CourseTestCase): def setUp(self): """Create initial data.""" super(Basetranscripts, self).setUp() + self.unicode_locator = unicode(loc_mapper().translate_location( + self.course.location.course_id, self.course.location, False, True + )) # Add video module data = { - 'parent_location': str(self.course_location), + 'parent_locator': self.unicode_locator, 'category': 'video', 'type': 'video' } - resp = self.client.ajax_post(reverse('create_item'), data) + resp = self.client.ajax_post('/xblock', data) self.item_location = json.loads(resp.content).get('id') self.assertEqual(resp.status_code, 200) @@ -196,11 +200,11 @@ class TestUploadtranscripts(Basetranscripts): def test_fail_for_non_video_module(self): # non_video module: setup data = { - 'parent_location': str(self.course_location), + 'parent_locator': self.unicode_locator, 'category': 'non_video', 'type': 'non_video' } - resp = self.client.ajax_post(reverse('create_item'), data) + resp = self.client.ajax_post('/xblock', data) item_location = json.loads(resp.content).get('id') data = '' modulestore().update_item(item_location, data) @@ -407,11 +411,11 @@ class TestDownloadtranscripts(Basetranscripts): def test_fail_for_non_video_module(self): # Video module: setup data = { - 'parent_location': str(self.course_location), + 'parent_locator': self.unicode_locator, 'category': 'videoalpha', 'type': 'videoalpha' } - resp = self.client.ajax_post(reverse('create_item'), data) + resp = self.client.ajax_post('/xblock', data) item_location = json.loads(resp.content).get('id') subs_id = str(uuid4()) data = textwrap.dedent(""" @@ -657,11 +661,11 @@ class TestChecktranscripts(Basetranscripts): def test_fail_for_non_video_module(self): # Not video module: setup data = { - 'parent_location': str(self.course_location), + 'parent_locator': self.unicode_locator, 'category': 'not_video', 'type': 'not_video' } - resp = self.client.ajax_post(reverse('create_item'), data) + resp = self.client.ajax_post('/xblock', data) item_location = json.loads(resp.content).get('id') subs_id = str(uuid4()) data = textwrap.dedent(""" diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 69d6a9e352..327e75c7f4 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -120,6 +120,10 @@ def edit_subsection(request, location): can_view_live = True break + locator = loc_mapper().translate_location( + course.location.course_id, item.location, False, True + ) + return render_to_response( 'edit_subsection.html', { @@ -129,8 +133,10 @@ def edit_subsection(request, location): 'lms_link': lms_link, 'preview_link': preview_link, 'course_graders': json.dumps(CourseGradingModel.fetch(course.location).graders), + # For grader, which is not yet converted 'parent_location': course.location, 'parent_item': parent, + 'locator': locator, 'policy_metadata': policy_metadata, 'subsection_units': subsection_units, 'can_view_live': can_view_live @@ -175,9 +181,9 @@ def edit_unit(request, location): # 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( + unit_locator = loc_mapper().translate_location( course.location.course_id, Location(location), False, True - ).url_reverse("xblock", "") + ) component_templates = defaultdict(list) for category in COMPONENT_TYPES: @@ -247,7 +253,7 @@ def edit_unit(request, location): component.location.url(), loc_mapper().translate_location( course.location.course_id, component.location, False, True - ).url_reverse("xblock") + ) ] for component in item.get_children() @@ -296,8 +302,9 @@ def edit_unit(request, location): return render_to_response('unit.html', { 'context_course': course, 'unit': item, + # Still needed for creating a draft. 'unit_location': location, - 'unit_update_url': unit_update_url, + 'unit_locator': unit_locator, 'components': components, 'component_templates': component_templates, 'draft_preview_link': preview_lms_link, diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 872eaf22c5..187ee9343b 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -192,7 +192,9 @@ def course_index(request, course_id, branch, version_guid, block): 'course_graders': json.dumps( CourseGradingModel.fetch(course.location).graders ), + # This is used by course grader, which has not yet been updated. 'parent_location': course.location, + 'parent_locator': location, 'new_section_category': 'chapter', 'new_subsection_category': 'sequential', 'new_unit_category': 'vertical', diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 72f7aa32a4..a91ec4fad8 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -7,7 +7,6 @@ from static_replace import replace_static_urls from django.core.exceptions import PermissionDenied from django.contrib.auth.decorators import login_required -from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore, loc_mapper from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError @@ -24,15 +23,18 @@ from xmodule.x_module import XModuleDescriptor from django.views.decorators.http import require_http_methods from xmodule.modulestore.locator import BlockUsageLocator from student.models import CourseEnrollment +from django.http import HttpResponseBadRequest from xblock.fields import Scope -__all__ = ['save_item', 'create_item', 'orphan', 'xblock_handler'] +__all__ = ['orphan', 'xblock_handler'] log = logging.getLogger(__name__) # cdodge: these are categories which should not be parented, they are detached from the hierarchy DETACHED_CATEGORIES = ['about', 'static_tab', 'course_info'] +CREATE_IF_NOT_FOUND = ['course_info'] + # pylint: disable=unused-argument @require_http_methods(("DELETE", "GET", "PUT", "POST")) @@ -45,106 +47,118 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid= 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. + GET + json: returns representation of the xblock (locator id, data, and metadata). + PUT or POST + json: if xblock location is specified, update the xblock instance. The json payload can contain + these fields, all optional: + :data: the new value for the data. + :children: the locator ids of children for this xblock. + :metadata: new values for the metadata fields. Any whose values are None will be deleted not set + to None! Absent ones will be left alone. + :nullout: which metadata fields to set to None + The JSON representation on the updated xblock (minus children) is returned. + + if xblock location is not specified, create a new xblock instance. The json playload can contain + these fields: + :parent_locator: parent for new xblock, required + :category: type of xblock, required + :display_name: name for new xblock, optional + :boilerplate: template name for populating fields, optional + The locator (and old-style id) for the created xblock (minus children) is returned. """ - location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) - if not has_access(request.user, location): - raise PermissionDenied() - - if request.method == 'GET': - rewrite_static_links = request.GET.get('rewrite_url_links', 'True') in ['True', 'true'] - rsp = _get_module_info(location, rewrite_static_links=rewrite_static_links) - return JsonResponse(rsp) - elif request.method in ("POST", "PUT"): - # Replace w/ save_item from below - rsp = _set_module_info(location, request.json) - return JsonResponse(rsp) - elif request.method == 'DELETE': - + if course_id is not None: + 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)) + if request.method == 'GET': + rewrite_static_links = request.GET.get('rewrite_url_links', 'True') in ['True', 'true'] + rsp = _get_module_info(location, rewrite_static_links=rewrite_static_links) + return JsonResponse(rsp) + elif request.method == 'DELETE': + 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): - """ - Will carry a json payload with these possible fields - :id (required): the id - :data (optional): the new value for the data - :metadata (optional): new values for the metadata fields. - Any whose values are None will be deleted not set to None! Absent ones will be left alone - :nullout (optional): which metadata fields to set to None - """ - # The nullout is a bit of a temporary copout until we can make module_edit.coffee and the metadata editors a - # little smarter and able to pass something more akin to {unset: [field, field]} - - try: - item_location = request.json['id'] - except KeyError: - import inspect - - log.exception( - '''Request missing required attribute 'id'. - Request info: - %s - Caller: - Function %s in file %s - ''', - request.META, - inspect.currentframe().f_back.f_code.co_name, - inspect.currentframe().f_back.f_code.co_filename + 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( + location, + old_location, + data=request.json.get('data'), + children=request.json.get('children'), + metadata=request.json.get('metadata'), + nullout=request.json.get('nullout') + ) + elif request.method in ('PUT', 'POST'): + return _create_item(request) + else: + return HttpResponseBadRequest( + "Only instance creation is supported without a course_id.", + content_type="text/plain" ) - return JsonResponse({"error": "Request missing required attribute 'id'."}, 400) + + +def _save_item(usage_loc, item_location, data=None, children=None, metadata=None, nullout=None): + """ + Saves certain properties (data, children, metadata, nullout) for a given xblock item. + + The item_location is still the old-style location. + """ + store = get_modulestore(item_location) try: - old_item = modulestore().get_item(item_location) - except (ItemNotFoundError, InvalidLocationError): + existing_item = store.get_item(item_location) + except ItemNotFoundError: + if item_location.category in CREATE_IF_NOT_FOUND: + # New module at this location, for pages that are not pre-created. + # Used for course info handouts. + store.create_and_save_xmodule(item_location) + existing_item = store.get_item(item_location) + else: + raise + except InvalidLocationError: log.error("Can't find item by location.") - return JsonResponse({"error": "Can't find item by location"}, 404) + return JsonResponse({"error": "Can't find item by location: " + str(item_location)}, 404) - # check permissions for this user within this course - if not has_access(request.user, item_location): - raise PermissionDenied() - - store = get_modulestore(Location(item_location)) - - if request.json.get('data'): - data = request.json['data'] + if data: store.update_item(item_location, data) + else: + data = existing_item.get_explicitly_set_fields_by_scope(Scope.content) - if request.json.get('children') is not None: - children = request.json['children'] - store.update_children(item_location, children) + if children is not None: + children_ids = [ + loc_mapper().translate_locator_to_location(BlockUsageLocator(child_locator)).url() + for child_locator + in children + ] + store.update_children(item_location, children_ids) # cdodge: also commit any metadata which might have been passed along - if request.json.get('nullout') is not None or request.json.get('metadata') is not None: + if nullout is not None or metadata is not None: # the postback is not the complete metadata, as there's system metadata which is - # not presented to the end-user for editing. So let's fetch the original and - # 'apply' the submitted metadata, so we don't end up deleting system metadata - existing_item = modulestore().get_item(item_location) - for metadata_key in request.json.get('nullout', []): - setattr(existing_item, metadata_key, None) + # not presented to the end-user for editing. So let's use the original (existing_item) and + # 'apply' the submitted metadata, so we don't end up deleting system metadata. + if nullout is not None: + for metadata_key in nullout: + setattr(existing_item, metadata_key, None) # update existing metadata with submitted metadata (which can be partial) # IMPORTANT NOTE: if the client passed 'null' (None) for a piece of metadata that means 'remove it'. If # the intent is to make it None, use the nullout field - for metadata_key, value in request.json.get('metadata', {}).items(): - field = existing_item.fields[metadata_key] - - if value is None: - field.delete_from(existing_item) - else: - try: - value = field.from_json(value) - except ValueError: - return JsonResponse({"error": "Invalid data"}, 400) - field.write_to(existing_item, value) + if metadata is not None: + for metadata_key, value in metadata.items(): + field = existing_item.fields[metadata_key] + if value is None: + field.delete_from(existing_item) + else: + try: + value = field.from_json(value) + except ValueError: + return JsonResponse({"error": "Invalid data"}, 400) + field.write_to(existing_item, value) # Save the data that we've just changed to the underlying # MongoKeyValueStore before we update the mongo datastore. @@ -153,16 +167,23 @@ def save_item(request): store.update_metadata(item_location, own_metadata(existing_item)) if existing_item.category == 'video': - manage_video_subtitles_save(old_item, existing_item) + manage_video_subtitles_save(existing_item, existing_item) - return JsonResponse() + # Note that children aren't returned because it is currently expensive to get the + # containing course for an xblock (and that is necessary to convert to locators). + return JsonResponse({ + 'id': unicode(usage_loc), + 'data': data, + 'metadata': own_metadata(existing_item) + }) @login_required @expect_json -def create_item(request): +def _create_item(request): """View for create items.""" - parent_location = Location(request.json['parent_location']) + parent_locator = BlockUsageLocator(request.json['parent_locator']) + parent_location = loc_mapper().translate_locator_to_location(parent_locator) category = request.json['category'] display_name = request.json.get('display_name') @@ -171,7 +192,10 @@ def create_item(request): raise PermissionDenied() parent = get_modulestore(category).get_item(parent_location) - dest_location = parent_location.replace(category=category, name=uuid4().hex) + # 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) # get the metadata, display_name, and definition from the request metadata = {} @@ -201,7 +225,7 @@ def create_item(request): 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")}) + return JsonResponse({'id': dest_location.url(), "locator": unicode(locator)}) def _delete_item_at_location(item_location, delete_children=False, delete_all_versions=False): @@ -232,6 +256,8 @@ def _delete_item_at_location(item_location, delete_children=False, delete_all_ve parent.children = children modulestore('direct').update_children(parent.location, parent.children) + return JsonResponse() + # pylint: disable=W0613 @login_required @@ -275,8 +301,8 @@ def _get_module_info(usage_loc, rewrite_static_links=False): try: module = store.get_item(old_location) except ItemNotFoundError: - if old_location.category in ['course_info']: - # create a new one + if old_location.category in CREATE_IF_NOT_FOUND: + # Create a new one for certain categories only. Used for course info handouts. store.create_and_save_xmodule(old_location) module = store.get_item(old_location) else: @@ -292,75 +318,10 @@ def _get_module_info(usage_loc, rewrite_static_links=False): course_id=module.location.org + '/' + module.location.course + '/BOGUS_RUN_REPLACE_WHEN_AVAILABLE' ) + # Note that children aren't returned because it is currently expensive to get the + # containing course for an xblock (and that is necessary to convert to locators). return { 'id': unicode(usage_loc), 'data': data, - 'metadata': module.get_explicitly_set_fields_by_scope(Scope.settings) - } - - -def _set_module_info(usage_loc, post_data): - """ - Old metadata, data, id representation leaf module updater. - :param usage_loc: a BlockUsageLocator - :param post_data: the payload with data, metadata, and possibly children (even tho the getter - doesn't support children) - """ - # TODO replace with save_item: differences - # - this doesn't handle nullout - # - this returns the new model - old_location = loc_mapper().translate_locator_to_location(usage_loc) - store = get_modulestore(old_location) - module = None - try: - module = store.get_item(old_location) - except ItemNotFoundError: - # new module at this location: almost always used for the course about pages; thus, no parent. (there - # are quite a handful of about page types available for a course and only the overview is pre-created) - store.create_and_save_xmodule(old_location) - module = store.get_item(old_location) - - if post_data.get('data') is not None: - data = post_data['data'] - store.update_item(old_location, data) - else: - data = module.get_explicitly_set_fields_by_scope(Scope.content) - - if post_data.get('children') is not None: - children = post_data['children'] - store.update_children(old_location, children) - - # cdodge: also commit any metadata which might have been passed along in the - # POST from the client, if it is there - # NOTE, that the postback is not the complete metadata, as there's system metadata which is - # not presented to the end-user for editing. So let's fetch the original and - # 'apply' the submitted metadata, so we don't end up deleting system metadata - if post_data.get('metadata') is not None: - posted_metadata = post_data['metadata'] - - # update existing metadata with submitted metadata (which can be partial) - # IMPORTANT NOTE: if the client passed pack 'null' (None) for a piece of metadata that means 'remove it' - for metadata_key, value in posted_metadata.items(): - field = module.fields[metadata_key] - - if value is None: - # remove both from passed in collection as well as the collection read in from the modulestore - field.delete_from(module) - else: - try: - value = field.from_json(value) - except ValueError: - return JsonResponse({"error": "Invalid data"}, 400) - field.write_to(module, value) - - # commit to datastore - metadata = module.get_explicitly_set_fields_by_scope(Scope.settings) - store.update_metadata(old_location, metadata) - else: - metadata = module.get_explicitly_set_fields_by_scope(Scope.settings) - - return { - 'id': unicode(usage_loc), - 'data': data, - 'metadata': metadata + 'metadata': own_metadata(module) } diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index ed1a8b16de..f184024f42 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/cms/djangoapps/contentstore/views/tabs.py @@ -13,6 +13,7 @@ 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 xmodule.modulestore.locator import BlockUsageLocator from ..utils import get_course_for_item, get_modulestore @@ -47,8 +48,12 @@ def initialize_course_tabs(course): @expect_json def reorder_static_tabs(request): "Order the static tabs in the requested order" + def get_location_for_tab(tab): + tab_locator = BlockUsageLocator(tab) + return loc_mapper().translate_locator_to_location(tab_locator) + tabs = request.json['tabs'] - course = get_course_for_item(tabs[0]) + course = get_course_for_item(get_location_for_tab(tabs[0])) if not has_access(request.user, course.location): raise PermissionDenied() @@ -64,7 +69,7 @@ def reorder_static_tabs(request): # load all reference tabs, return BadRequest if we can't find any of them tab_items = [] for tab in tabs: - item = modulestore('direct').get_item(Location(tab)) + item = modulestore('direct').get_item(get_location_for_tab(tab)) if item is None: return HttpResponseBadRequest() @@ -122,15 +127,20 @@ def edit_tabs(request, org, course, coursename): 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 ] + course_locator = loc_mapper().translate_location( + course_item.location.course_id, course_item.location, False, True + ) + return render_to_response('edit-tabs.html', { 'context_course': course_item, - 'components': components + 'components': components, + 'locator': course_locator }) diff --git a/cms/static/coffee/spec/main.coffee b/cms/static/coffee/spec/main.coffee index 379e5efc17..2ee8d17979 100644 --- a/cms/static/coffee/spec/main.coffee +++ b/cms/static/coffee/spec/main.coffee @@ -182,7 +182,7 @@ define([ "coffee/spec/main_spec", "coffee/spec/models/course_spec", "coffee/spec/models/metadata_spec", - "coffee/spec/models/module_spec", "coffee/spec/models/section_spec", + "coffee/spec/models/section_spec", "coffee/spec/models/settings_course_grader_spec", "coffee/spec/models/settings_grading_spec", "coffee/spec/models/textbook_spec", "coffee/spec/models/upload_spec", @@ -193,9 +193,11 @@ define([ "coffee/spec/views/overview_spec", "coffee/spec/views/textbook_spec", "coffee/spec/views/upload_spec", - "js_spec/transcripts/utils_spec", "js_spec/transcripts/editor_spec", - "js_spec/transcripts/videolist_spec", "js_spec/transcripts/message_manager_spec", - "js_spec/transcripts/file_uploader_spec" + "js/spec/transcripts/utils_spec", "js/spec/transcripts/editor_spec", + "js/spec/transcripts/videolist_spec", "js/spec/transcripts/message_manager_spec", + "js/spec/transcripts/file_uploader_spec", + + "js/spec/utils/module_spec" # these tests are run separate in the cms-squire suite, due to process # isolation issues with Squire.js diff --git a/cms/static/coffee/spec/models/module_spec.coffee b/cms/static/coffee/spec/models/module_spec.coffee deleted file mode 100644 index 92586abd90..0000000000 --- a/cms/static/coffee/spec/models/module_spec.coffee +++ /dev/null @@ -1,7 +0,0 @@ -define ["coffee/src/models/module"], (Module) -> - describe "Module", -> - it "set the correct URL", -> - expect(new Module().url).toEqual("/save_item") - - it "set the correct default", -> - expect(new Module().defaults).toEqual(undefined) diff --git a/cms/static/coffee/spec/models/section_spec.coffee b/cms/static/coffee/spec/models/section_spec.coffee index a97b90e269..ebea69adc7 100644 --- a/cms/static/coffee/spec/models/section_spec.coffee +++ b/cms/static/coffee/spec/models/section_spec.coffee @@ -1,9 +1,9 @@ -define ["js/models/section", "sinon"], (Section, sinon) -> +define ["js/models/section", "sinon", "js/utils/module"], (Section, sinon, ModuleUtils) -> describe "Section", -> describe "basic", -> beforeEach -> @model = new Section({ - id: 42, + id: 42 name: "Life, the Universe, and Everything" }) @@ -14,11 +14,10 @@ define ["js/models/section", "sinon"], (Section, sinon) -> expect(@model.get("name")).toEqual("Life, the Universe, and Everything") it "should have a URL set", -> - expect(@model.url).toEqual("/save_item") + expect(@model.url()).toEqual(ModuleUtils.getUpdateUrl(42)) it "should serialize to JSON correctly", -> expect(@model.toJSON()).toEqual({ - id: 42, metadata: { display_name: "Life, the Universe, and Everything" @@ -30,7 +29,7 @@ define ["js/models/section", "sinon"], (Section, sinon) -> spyOn(Section.prototype, 'showNotification') spyOn(Section.prototype, 'hideNotification') @model = new Section({ - id: 42, + id: 42 name: "Life, the Universe, and Everything" }) @requests = requests = [] diff --git a/cms/static/coffee/spec/views/module_edit_spec.coffee b/cms/static/coffee/spec/views/module_edit_spec.coffee index f108e5046e..22d1052fa3 100644 --- a/cms/static/coffee/spec/views/module_edit_spec.coffee +++ b/cms/static/coffee/spec/views/module_edit_spec.coffee @@ -4,6 +4,9 @@ define ["coffee/src/views/module_edit", "xmodule"], (ModuleEdit) -> beforeEach -> @stubModule = jasmine.createSpy("Module") @stubModule.id = 'stub-id' + @stubModule.get = (param)-> + if param == 'old_id' + return 'stub-old-id' setFixtures """
  • @@ -59,7 +62,7 @@ define ["coffee/src/views/module_edit", "xmodule"], (ModuleEdit) -> @moduleEdit.render() it "loads the module preview and editor via ajax on the view element", -> - expect(@moduleEdit.$el.load).toHaveBeenCalledWith("/preview_component/#{@moduleEdit.model.id}", jasmine.any(Function)) + expect(@moduleEdit.$el.load).toHaveBeenCalledWith("/preview_component/#{@moduleEdit.model.get('old_id')}", jasmine.any(Function)) @moduleEdit.$el.load.mostRecentCall.args[1]() expect(@moduleEdit.loadDisplay).toHaveBeenCalled() expect(@moduleEdit.delegateEvents).toHaveBeenCalled() diff --git a/cms/static/coffee/spec/views/overview_spec.coffee b/cms/static/coffee/spec/views/overview_spec.coffee index 1dea609f12..9ece1b0059 100644 --- a/cms/static/coffee/spec/views/overview_spec.coffee +++ b/cms/static/coffee/spec/views/overview_spec.coffee @@ -8,7 +8,7 @@ define ["js/views/overview", "js/views/feedback_notification", "sinon", "js/base Will Release: 06/12/2013 at 04:00 UTC - Edit + Edit """ @@ -35,8 +35,8 @@ define ["js/views/overview", "js/views/feedback_notification", "sinon", "js/base """ appendSetFixtures """ -
    -