Merge pull request #1675 from edx/christina/tab-new

Convert tabs to use RESTful API

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

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

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,24 +407,7 @@ 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')
-        locator = _course_factory_create_course()
-        course_location = loc_mapper().translate_locator_to_location(locator)
-
-        ItemFactory.create(
-            parent_location=course_location,
-            category="static_tab",
-            display_name="Static_1")
-        ItemFactory.create(
-            parent_location=course_location,
-            category="static_tab",
-            display_name="Static_2")
+        module_store, course_location, new_location = self._create_static_tabs()
 
         course = module_store.get_item(course_location)
 
@@ -432,9 +415,9 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         reverse_tabs = []
         for tab in course.tabs:
             if tab['type'] == 'static_tab':
-                reverse_tabs.insert(0, get_tab_locator(tab))
+                reverse_tabs.insert(0, unicode(self._get_tab_locator(course, tab)))
 
-        self.client.ajax_post(reverse('reorder_static_tabs'), {'tabs': reverse_tabs})
+        self.client.ajax_post(new_location.url_reverse('tabs'), {'tabs': reverse_tabs})
 
         course = module_store.get_item(course_location)
 
@@ -442,10 +425,57 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         course_tabs = []
         for tab in course.tabs:
             if tab['type'] == 'static_tab':
-                course_tabs.append(get_tab_locator(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')
+        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,
+            category="static_tab",
+            display_name="Static_1")
+        ItemFactory.create(
+            parent_location=course_location,
+            category="static_tab",
+            display_name="Static_2")
+
+        return module_store, course_location, new_location
+
     def test_import_polls(self):
         module_store = modulestore('direct')
         import_from_xml(module_store, 'common/test/data/', ['toy'])
@@ -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']
@@ -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():

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,75 +44,86 @@ 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)
+@login_required
+@ensure_csrf_cookie
+@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.
 
-    tabs = request.json['tabs']
-    course_location = loc_mapper().translate_locator_to_location(BlockUsageLocator(tabs[0]), get_course=True)
+    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.
 
-    if not has_access(request.user, course_location):
+    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()
 
-    course = get_modulestore(course_location).get_item(course_location)
+    old_location = loc_mapper().translate_locator_to_location(locator)
+    store = get_modulestore(old_location)
+    course_item = store.get_item(old_location)
+
+    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))
+
+                tabs = request.json['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 can drop some!
-
-    existing_static_tabs = [t for t in course.tabs if t['type'] == 'static_tab']
+                # 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 HttpResponseBadRequest()
+                    return JsonResponse(
+                        {"error": "number of tabs must be {}".format(len(existing_static_tabs))}, status=400
+                    )
 
                 # 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()
+                        return JsonResponse(
+                            {"error": "no tab for found location {}".format(tab)}, status=400
+                        )
 
                     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:
+                for tab in course_item.tabs:
                     if tab['type'] == 'static_tab':
-            reordered_tabs.append({'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})
+                             '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)
-
-    # check that logged in user has permissions to this item
-    if not has_access(request.user, location):
-        raise PermissionDenied()
-
-    # see tabs have been uninitialized (e.g. supporing courses created before tab support in studio)
+                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)
 
@@ -121,7 +133,7 @@ def edit_tabs(request, org, course, coursename):
 
         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_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 = [
@@ -132,15 +144,13 @@ def edit_tabs(request, org, course, coursename):
             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,
-        'locator': course_locator
+            '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))
+

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

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()

cms/static/js/models/explicit_url.js

+/**
+ * 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");
+        }
+    });
+});

cms/static/js/spec/models/explicit_url_spec.js

+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('');
+            });
+        });
+    }
+);

cms/templates/edit-tabs.html

@@ -9,12 +9,15 @@
 
 <%block name="jsextra">
   <script type='text/javascript'>
-require(["backbone", "coffee/src/views/tabs"], function(Backbone, TabsEditView) {
+require(["js/models/explicit_url", "coffee/src/views/tabs"], function(TabsModel, TabsEditView) {
+    var model = new TabsModel({
+        id: "${course_locator}",
+        explicit_url: "${course_locator.url_reverse('tabs')}"
+    });
+
     new TabsEditView({
       el: $('.main-wrapper'),
-      model: new Backbone.Model({
-        id: '${locator}'
-      }),
+      model: model,
       mast: $('.wrapper-mast')
     });
 });

cms/templates/widgets/header.html

@@ -16,13 +16,14 @@
       <% 
             ctx_loc = context_course.location
             location = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True)
-            index_url = location.url_reverse('course/')
-            checklists_url = location.url_reverse('checklists/')
-            course_team_url = location.url_reverse('course_team/')
-            assets_url = location.url_reverse('assets/')
-            import_url = location.url_reverse('import/')
-            course_info_url = location.url_reverse('course_info/')
-            export_url = location.url_reverse('export/', '')
+            index_url = location.url_reverse('course')
+            checklists_url = location.url_reverse('checklists')
+            course_team_url = location.url_reverse('course_team')
+            assets_url = location.url_reverse('assets')
+            import_url = location.url_reverse('import')
+            course_info_url = location.url_reverse('course_info')
+            export_url = location.url_reverse('export')
+            tabs_url = location.url_reverse('tabs')
       %>
       <h2 class="info-course">
         <span class="sr">${_("Current Course:")}</span>
@@ -48,7 +49,7 @@
                     <a href="${course_info_url}">${_("Updates")}</a>
                   </li>
                   <li class="nav-item nav-course-courseware-pages">
-                    <a href="${reverse('edit_tabs', kwargs=dict(org=ctx_loc.org, course=ctx_loc.course, coursename=ctx_loc.name))}">${_("Static Pages")}</a>
+                    <a href="${tabs_url}">${_("Static Pages")}</a>
                   </li>
                   <li class="nav-item nav-course-courseware-uploads">
                     <a href="${assets_url}">${_("Files &amp; Uploads")}</a>

cms/urls.py

@@ -25,7 +25,6 @@ urlpatterns = patterns('',  # nopep8
     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'^reorder_static_tabs', 'contentstore.views.reorder_static_tabs', name='reorder_static_tabs'),
 
     url(r'^preview/xblock/(?P<usage_id>.*?)/handler/(?P<handler>[^/]*)(?:/(?P<suffix>[^/]*))?$',
         'contentstore.views.preview_handler', name='preview_handler'),
@@ -48,9 +47,6 @@ urlpatterns = patterns('',  # nopep8
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/(?P<category>[^/]+)/(?P<name>[^/]+)/gradeas.*$',
         'contentstore.views.assignment_type_update', name='assignment_type_update'),
 
-    url(r'^edit_tabs/(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<coursename>[^/]+)$',
-        'contentstore.views.edit_tabs', name='edit_tabs'),
-
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/textbooks/(?P<name>[^/]+)$',
         'contentstore.views.textbook_index', name='textbook_index'),
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/textbooks/(?P<name>[^/]+)/new$',
@@ -111,6 +107,7 @@ urlpatterns += patterns(
     url(r'(?ix)^import_status/{}/(?P<filename>.+)$'.format(parsers.URL_RE_SOURCE), 'import_status_handler'),
     url(r'(?ix)^export/{}$'.format(parsers.URL_RE_SOURCE), 'export_handler'),
     url(r'(?ix)^xblock($|/){}$'.format(parsers.URL_RE_SOURCE), 'xblock_handler'),
+    url(r'(?ix)^tabs/{}$'.format(parsers.URL_RE_SOURCE), 'tabs_handler'),
 )
 
 js_info_dict = {