diff --git a/cms/djangoapps/contentstore/features/static-pages.feature b/cms/djangoapps/contentstore/features/static-pages.feature index 54d23d985d..39399fb207 100644 --- a/cms/djangoapps/contentstore/features/static-pages.feature +++ b/cms/djangoapps/contentstore/features/static-pages.feature @@ -9,10 +9,8 @@ Feature: CMS.Static Pages Then I should see a static page named "Empty" Scenario: Users can delete static pages - Given I have opened a new course in Studio - And I go to the static pages page - And I add a new page - And I "delete" the static page + Given I have created a static page + When I "delete" the static page Then I am shown a prompt When I confirm the prompt Then I should not see any static pages @@ -20,9 +18,16 @@ Feature: CMS.Static Pages # Safari won't update the name properly @skip_safari Scenario: Users can edit static pages - Given I have opened a new course in Studio - And I go to the static pages page - And I add a new page + Given I have created a static page When I "edit" the static page And I change the name to "New" Then I should see a static page named "New" + + # Safari won't update the name properly + @skip_safari + Scenario: Users can reorder static pages + Given I have created two different static pages + When I reorder the tabs + Then the tabs are in the reverse order + And I reload the page + Then the tabs are in the reverse order diff --git a/cms/djangoapps/contentstore/features/static-pages.py b/cms/djangoapps/contentstore/features/static-pages.py index 58932ad8e2..0adb4b1e54 100644 --- a/cms/djangoapps/contentstore/features/static-pages.py +++ b/cms/djangoapps/contentstore/features/static-pages.py @@ -48,3 +48,47 @@ def change_name(step, new_name): world.trigger_event(input_css) save_button = 'a.save-button' world.css_click(save_button) + + +@step(u'I reorder the tabs') +def reorder_tabs(_step): + # For some reason, the drag_and_drop method did not work in this case. + draggables = world.css_find('.drag-handle') + source = draggables.first + target = draggables.last + source.action_chains.click_and_hold(source._element).perform() + source.action_chains.move_to_element_with_offset(target._element, 0, 50).perform() + source.action_chains.release().perform() + + +@step(u'I have created a static page') +def create_static_page(step): + step.given('I have opened a new course in Studio') + step.given('I go to the static pages page') + step.given('I add a new page') + + +@step(u'I have created two different static pages') +def create_two_pages(step): + step.given('I have created a static page') + step.given('I "edit" the static page') + step.given('I change the name to "First"') + step.given('I add a new page') + # Verify order of tabs + _verify_tab_names('First', 'Empty') + + +@step(u'the tabs are in the reverse order') +def tabs_in_reverse_order(step): + _verify_tab_names('Empty', 'First') + + +def _verify_tab_names(first, second): + world.wait_for( + func=lambda _: len(world.css_find('.xmodule_StaticTabModule')) == 2, + timeout=200, + timeout_msg="Timed out waiting for two tabs to be present" + ) + tabs = world.css_find('.xmodule_StaticTabModule') + assert tabs[0].text == first + assert tabs[1].text == second diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 786d6ddf89..831800e27b 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -179,7 +179,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # TODO: uncomment after edit_unit not using locations. # _test_no_locations(self, resp) - def lockAnAsset(self, content_store, course_location): + def _lock_an_asset(self, content_store, course_location): """ Lock an arbitrary asset in the course :param course_location: @@ -407,15 +407,63 @@ 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, course_location, new_location = self._create_static_tabs() + 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, unicode(self._get_tab_locator(course, tab))) + + self.client.ajax_post(new_location.url_reverse('tabs'), {'tabs': reverse_tabs}) + + 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(unicode(self._get_tab_locator(course, tab))) + + self.assertEqual(reverse_tabs, course_tabs) + + def test_static_tab_deletion(self): + module_store, course_location, _ = self._create_static_tabs() + + course = module_store.get_item(course_location) + num_tabs = len(course.tabs) + last_tab = course.tabs[num_tabs - 1] + url_slug = last_tab['url_slug'] + delete_url = self._get_tab_locator(course, last_tab).url_reverse('xblock') + + self.client.delete(delete_url) + + course = module_store.get_item(course_location) + self.assertEqual(num_tabs - 1, len(course.tabs)) + + def tab_matches(tab): + """ Checks if the tab matches the one we deleted """ + return tab['type'] == 'static_tab' and tab['url_slug'] == url_slug + + tab_found = any(tab_matches(tab) for tab in course.tabs) + + self.assertFalse(tab_found, "tab should have been deleted") + + def _get_tab_locator(self, course, tab): + """ Returns the locator for a given tab. """ + tab_location = 'i4x://MITx/999/static_tab/{0}'.format(tab['url_slug']) + return loc_mapper().translate_location( + course.location.course_id, Location(tab_location), False, True + ) + + def _create_static_tabs(self): + """ Creates two static tabs in a dummy course. """ module_store = modulestore('direct') - locator = _course_factory_create_course() - course_location = loc_mapper().translate_locator_to_location(locator) + CourseFactory.create(org='edX', course='999', display_name='Robot Super Course') + course_location = Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None]) + new_location = loc_mapper().translate_location(course_location.course_id, course_location, False, True) ItemFactory.create( parent_location=course_location, @@ -426,25 +474,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): category="static_tab", display_name="Static_2") - 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, get_tab_locator(tab)) - - self.client.ajax_post(reverse('reorder_static_tabs'), {'tabs': reverse_tabs}) - - 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(get_tab_locator(tab)) - - self.assertEqual(reverse_tabs, course_tabs) + return module_store, course_location, new_location def test_import_polls(self): module_store = modulestore('direct') @@ -633,7 +663,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): thumbnail = content_store.find(thumbnail_location, throw_on_not_found=False) self.assertIsNotNone(thumbnail) - def _delete_asset_in_course (self): + def _delete_asset_in_course(self): """ Helper method for: 1) importing course from xml @@ -972,7 +1002,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertIn(private_location_no_draft.url(), sequential.children) - locked_asset = self.lockAnAsset(content_store, location) + locked_asset = self._lock_an_asset(content_store, location) locked_asset_attrs = content_store.get_attrs(locked_asset) # the later import will reupload del locked_asset_attrs['uploadDate'] @@ -1027,7 +1057,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): shutil.rmtree(root_dir) def check_import(self, module_store, root_dir, draft_store, content_store, stub_location, course_location, - locked_asset, locked_asset_attrs): + locked_asset, locked_asset_attrs): # reimport import_from_xml( module_store, root_dir, ['test_export'], draft_store=draft_store, @@ -1226,7 +1256,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): handouts_locator = loc_mapper().translate_location('edX/toy/2012_Fall', handout_location) # get module info (json) - resp = self.client.get(handouts_locator.url_reverse('/xblock', '')) + resp = self.client.get(handouts_locator.url_reverse('/xblock')) # make sure we got a successful response self.assertEqual(resp.status_code, 200) @@ -1403,7 +1433,7 @@ class ContentStoreTest(ModuleStoreTestCase): second_course_data = self.assert_created_course(number_suffix=uuid4().hex) # unseed the forums for the first course - course_id =_get_course_id(test_course_data) + course_id = _get_course_id(test_course_data) delete_course_and_groups(course_id, commit=True) self.assertFalse(are_permissions_roles_seeded(course_id)) @@ -1625,6 +1655,7 @@ class ContentStoreTest(ModuleStoreTestCase): test_get_html('course_info') test_get_html('checklists') test_get_html('assets') + test_get_html('tabs') # settings_details resp = self.client.get_html(reverse('settings_details', @@ -1677,13 +1708,6 @@ class ContentStoreTest(ModuleStoreTestCase): # TODO: uncomment when edit_unit not using old locations. # _test_no_locations(self, resp) - resp = self.client.get_html(reverse('edit_tabs', - kwargs={'org': loc.org, - 'course': loc.course, - 'coursename': loc.name})) - self.assertEqual(resp.status_code, 200) - _test_no_locations(self, resp) - def delete_item(category, name): """ Helper method for testing the deletion of an xblock item. """ del_loc = loc.replace(category=category, name=name) @@ -1997,7 +2021,7 @@ def _create_course(test, course_data): test.assertEqual(response.status_code, 200) data = parse_json(response) test.assertNotIn('ErrMsg', data) - test.assertEqual(data['url'], new_location.url_reverse("course/", "")) + test.assertEqual(data['url'], new_location.url_reverse("course")) def _course_factory_create_course(): diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index 40ea9bfd6b..46791ddc26 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/cms/djangoapps/contentstore/views/tabs.py @@ -2,12 +2,13 @@ Views related to course tabs """ from access import has_access -from util.json_request import expect_json +from util.json_request import expect_json, JsonResponse -from django.http import HttpResponse, HttpResponseBadRequest +from django.http import HttpResponseNotFound from django.contrib.auth.decorators import login_required from django.core.exceptions import PermissionDenied from django_future.csrf import ensure_csrf_cookie +from django.views.decorators.http import require_http_methods from mitxmako.shortcuts import render_to_response from xmodule.modulestore import Location from xmodule.modulestore.inheritance import own_metadata @@ -19,7 +20,7 @@ from ..utils import get_modulestore from django.utils.translation import ugettext as _ -__all__ = ['edit_tabs', 'reorder_static_tabs'] +__all__ = ['tabs_handler'] def initialize_course_tabs(course): @@ -43,104 +44,113 @@ def initialize_course_tabs(course): modulestore('direct').update_metadata(course.location.url(), own_metadata(course)) - -@login_required @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_location = loc_mapper().translate_locator_to_location(BlockUsageLocator(tabs[0]), get_course=True) - - if not has_access(request.user, course_location): - raise PermissionDenied() - - course = get_modulestore(course_location).get_item(course_location) - - # get list of existing static tabs in course - # make sure they are the same lengths (i.e. the number of passed in tabs equals the number - # that we know about) otherwise we can drop some! - - existing_static_tabs = [t for t in course.tabs if t['type'] == 'static_tab'] - if len(existing_static_tabs) != len(tabs): - return HttpResponseBadRequest() - - # 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(get_location_for_tab(tab)) - if item is None: - return HttpResponseBadRequest() - - tab_items.append(item) - - # now just go through the existing course_tabs and re-order the static tabs - reordered_tabs = [] - static_tab_idx = 0 - for tab in course.tabs: - if tab['type'] == 'static_tab': - reordered_tabs.append({'type': 'static_tab', - 'name': tab_items[static_tab_idx].display_name, - 'url_slug': tab_items[static_tab_idx].location.name}) - static_tab_idx += 1 - else: - reordered_tabs.append(tab) - - # OK, re-assemble the static tabs in the new order - course.tabs = reordered_tabs - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - course.save() - modulestore('direct').update_metadata(course.location, own_metadata(course)) - # TODO: above two lines are used for the primitive-save case. Maybe factor them out? - return HttpResponse() - - @login_required @ensure_csrf_cookie -def edit_tabs(request, org, course, coursename): - "Edit tabs" - location = ['i4x', org, course, 'course', coursename] - store = get_modulestore(location) - course_item = store.get_item(location) +@require_http_methods(("GET", "POST", "PUT")) +def tabs_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): + """ + The restful handler for static tabs. - # check that logged in user has permissions to this item - if not has_access(request.user, location): + GET + html: return page for editing static tabs + json: not supported + PUT or POST + json: update the tab order. It is expected that the request body contains a JSON-encoded dict with entry "tabs". + The value for "tabs" is an array of tab locators, indicating the desired order of the tabs. + + Creating a tab, deleting a tab, or changing its contents is not supported through this method. + Instead use the general xblock URL (see item.xblock_handler). + """ + locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) + if not has_access(request.user, locator): raise PermissionDenied() - # see tabs have been uninitialized (e.g. supporing courses created before tab support in studio) - if course_item.tabs is None or len(course_item.tabs) == 0: - initialize_course_tabs(course_item) + old_location = loc_mapper().translate_locator_to_location(locator) + store = get_modulestore(old_location) + course_item = store.get_item(old_location) - # first get all static tabs from the tabs list - # we do this because this is also the order in which items are displayed in the LMS - static_tabs_refs = [t for t in course_item.tabs if t['type'] == 'static_tab'] + if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): + if request.method == 'GET': + raise NotImplementedError('coming soon') + else: + if 'tabs' in request.json: + def get_location_for_tab(tab): + """ Returns the location (old-style) for a tab. """ + return loc_mapper().translate_locator_to_location(BlockUsageLocator(tab)) - static_tabs = [] - for static_tab_ref in static_tabs_refs: - static_tab_loc = Location(location)._replace(category='static_tab', name=static_tab_ref['url_slug']) - static_tabs.append(modulestore('direct').get_item(static_tab_loc)) + tabs = request.json['tabs'] - components = [ - loc_mapper().translate_location( - course_item.location.course_id, static_tab.location, False, True - ) - for static_tab - in static_tabs - ] + # get list of existing static tabs in course + # make sure they are the same lengths (i.e. the number of passed in tabs equals the number + # that we know about) otherwise we will inadvertently drop some! + existing_static_tabs = [t for t in course_item.tabs if t['type'] == 'static_tab'] + if len(existing_static_tabs) != len(tabs): + return JsonResponse( + {"error": "number of tabs must be {}".format(len(existing_static_tabs))}, status=400 + ) - course_locator = loc_mapper().translate_location( - course_item.location.course_id, course_item.location, False, True - ) + # 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(get_location_for_tab(tab)) + if item is None: + return JsonResponse( + {"error": "no tab for found location {}".format(tab)}, status=400 + ) - return render_to_response('edit-tabs.html', { - 'context_course': course_item, - 'components': components, - 'locator': course_locator - }) + tab_items.append(item) + + # now just go through the existing course_tabs and re-order the static tabs + reordered_tabs = [] + static_tab_idx = 0 + for tab in course_item.tabs: + if tab['type'] == 'static_tab': + reordered_tabs.append( + {'type': 'static_tab', + 'name': tab_items[static_tab_idx].display_name, + 'url_slug': tab_items[static_tab_idx].location.name, + } + ) + static_tab_idx += 1 + else: + reordered_tabs.append(tab) + + # OK, re-assemble the static tabs in the new order + course_item.tabs = reordered_tabs + modulestore('direct').update_metadata(course_item.location, own_metadata(course_item)) + return JsonResponse() + else: + raise NotImplementedError('Creating or changing tab content is not supported.') + elif request.method == 'GET': # assume html + # see tabs have been uninitialized (e.g. supporting courses created before tab support in studio) + if course_item.tabs is None or len(course_item.tabs) == 0: + initialize_course_tabs(course_item) + + # first get all static tabs from the tabs list + # we do this because this is also the order in which items are displayed in the LMS + static_tabs_refs = [t for t in course_item.tabs if t['type'] == 'static_tab'] + + static_tabs = [] + for static_tab_ref in static_tabs_refs: + static_tab_loc = old_location.replace(category='static_tab', name=static_tab_ref['url_slug']) + static_tabs.append(modulestore('direct').get_item(static_tab_loc)) + + components = [ + loc_mapper().translate_location( + course_item.location.course_id, static_tab.location, False, True + ) + for static_tab + in static_tabs + ] + + return render_to_response('edit-tabs.html', { + 'context_course': course_item, + 'components': components, + 'course_locator': locator + }) + else: + return HttpResponseNotFound() # "primitive" tab edit functions driven by the command line. @@ -164,7 +174,7 @@ def primitive_delete(course, num): # Note for future implementations: if you delete a static_tab, then Chris Dodge # points out that there's other stuff to delete beyond this element. # This code happens to not delete static_tab so it doesn't come up. - primitive_save(course) + modulestore('direct').update_metadata(course.location, own_metadata(course)) def primitive_insert(course, num, tab_type, name): @@ -173,11 +183,5 @@ def primitive_insert(course, num, tab_type, name): new_tab = {u'type': unicode(tab_type), u'name': unicode(name)} tabs = course.tabs tabs.insert(num, new_tab) - primitive_save(course) - - -def primitive_save(course): - "Saves the course back to modulestore." - # This code copied from reorder_static_tabs above - course.save() modulestore('direct').update_metadata(course.location, own_metadata(course)) + diff --git a/cms/static/coffee/spec/main.coffee b/cms/static/coffee/spec/main.coffee index 2ee8d17979..c84b60be61 100644 --- a/cms/static/coffee/spec/main.coffee +++ b/cms/static/coffee/spec/main.coffee @@ -197,7 +197,8 @@ define([ "js/spec/transcripts/videolist_spec", "js/spec/transcripts/message_manager_spec", "js/spec/transcripts/file_uploader_spec", - "js/spec/utils/module_spec" + "js/spec/utils/module_spec", + "js/spec/models/explicit_url_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/src/views/tabs.coffee b/cms/static/coffee/src/views/tabs.coffee index a85b3b7863..83ca7dc2fe 100644 --- a/cms/static/coffee/src/views/tabs.coffee +++ b/cms/static/coffee/src/views/tabs.coffee @@ -37,14 +37,17 @@ define ["jquery", "jquery.ui", "backbone", "js/views/feedback_prompt", "js/views analytics.track "Reordered Static Pages", course: course_location_analytics + saving = new NotificationView.Mini({title: gettext("Saving…")}) + saving.show() + $.ajax({ type:'POST', - url: '/reorder_static_tabs', + url: @model.url(), data: JSON.stringify({ tabs : tabs }), contentType: 'application/json' - }) + }).success(=> saving.hide()) addNewTab: (event) => event.preventDefault() diff --git a/cms/static/js/models/explicit_url.js b/cms/static/js/models/explicit_url.js new file mode 100644 index 0000000000..aae69608af --- /dev/null +++ b/cms/static/js/models/explicit_url.js @@ -0,0 +1,14 @@ +/** + * A model that simply allows the update URL to be passed + * in as an argument. + */ +define(["backbone"], function(Backbone){ + return Backbone.Model.extend({ + defaults: { + "explicit_url": "" + }, + url: function() { + return this.get("explicit_url"); + } + }); +}); diff --git a/cms/static/js/spec/models/explicit_url_spec.js b/cms/static/js/spec/models/explicit_url_spec.js new file mode 100644 index 0000000000..df70d47b63 --- /dev/null +++ b/cms/static/js/spec/models/explicit_url_spec.js @@ -0,0 +1,12 @@ +define(['js/models/explicit_url'], + function (Model) { + describe('Model ', function () { + it('allows url to be passed in constructor', function () { + expect(new Model({'explicit_url': '/fancy/url'}).url()).toBe('/fancy/url'); + }); + it('returns empty string if url not set', function () { + expect(new Model().url()).toBe(''); + }); + }); + } +); diff --git a/cms/templates/edit-tabs.html b/cms/templates/edit-tabs.html index ca6e0259ab..70c3e61801 100644 --- a/cms/templates/edit-tabs.html +++ b/cms/templates/edit-tabs.html @@ -9,12 +9,15 @@ <%block name="jsextra">