Merge pull request #1448 from edx/christina/checklists

Change checklist URL.

cms/djangoapps/contentstore/tests/test_checklists.py

@@ -3,7 +3,8 @@ from contentstore.utils import get_modulestore
 from contentstore.views.checklist import expand_checklist_action_url
 from xmodule.modulestore.inheritance import own_metadata
 from xmodule.modulestore.tests.factories import CourseFactory
-from django.core.urlresolvers import reverse
+from xmodule.modulestore.django import loc_mapper
+
 import json
 from .utils import CourseTestCase
 
@@ -14,6 +15,8 @@ class ChecklistTestCase(CourseTestCase):
         """ Creates the test course. """
         super(ChecklistTestCase, self).setUp()
         self.course = CourseFactory.create(org='mitX', number='333', display_name='Checklists Course')
+        self.location = loc_mapper().translate_location(self.course.location.course_id, self.course.location, False, True)
+        self.checklists_url = self.location.url_reverse('checklists/', '')
 
     def get_persisted_checklists(self):
         """ Returns the checklists as persisted in the modulestore. """
@@ -35,13 +38,8 @@ class ChecklistTestCase(CourseTestCase):
             self.assertEqual(pers['action_external'], req['action_external'])
 
     def test_get_checklists(self):
-        """ Tests the get checklists method. """
-        checklists_url = reverse("checklists", kwargs={
-            'org': self.course.location.org,
-            'course': self.course.location.course,
-            'name': self.course.location.name,
-        })
-        response = self.client.get(checklists_url)
+        """ Tests the get checklists method and URL expansion. """
+        response = self.client.get(self.checklists_url)
         self.assertContains(response, "Getting Started With Studio")
         # Verify expansion of action URL happened.
         self.assertContains(response, '/mitX/333/team/Checklists_Course')
@@ -58,17 +56,19 @@ class ChecklistTestCase(CourseTestCase):
         modulestore = get_modulestore(self.course.location)
         modulestore.update_metadata(self.course.location, own_metadata(self.course))
         self.assertEqual(self.get_persisted_checklists(), None)
-        response = self.client.get(checklists_url)
+        response = self.client.get(self.checklists_url)
         self.assertEqual(payload, response.content)
 
+    def test_get_checklists_html(self):
+        """ Tests getting the HTML template for the checklists page). """
+        response = self.client.get(self.checklists_url, HTTP_ACCEPT='text/html')
+        self.assertContains(response, "Getting Started With Studio")
+        # The HTML generated will define the handler URL (for use by the Backbone model).
+        self.assertContains(response, self.checklists_url)
+
     def test_update_checklists_no_index(self):
         """ No checklist index, should return all of them. """
-        update_url = reverse('checklists_updates', kwargs={
-            'org': self.course.location.org,
-            'course': self.course.location.course,
-            'name': self.course.location.name})
-
-        returned_checklists = json.loads(self.client.get(update_url).content)
+        returned_checklists = json.loads(self.client.get(self.checklists_url).content)
         # Verify that persisted checklists do not have expanded action URLs.
         # compare_checklists will verify that returned_checklists DO have expanded action URLs.
         pers = self.get_persisted_checklists()
@@ -78,10 +78,7 @@ class ChecklistTestCase(CourseTestCase):
 
     def test_update_checklists_index_ignored_on_get(self):
         """ Checklist index ignored on get. """
-        update_url = reverse('checklists_updates', kwargs={'org': self.course.location.org,
-                                                           'course': self.course.location.course,
-                                                           'name': self.course.location.name,
-                                                           'checklist_index': 1})
+        update_url = self.location.url_reverse('checklists/', '1')
 
         returned_checklists = json.loads(self.client.get(update_url).content)
         for pay, resp in zip(self.get_persisted_checklists(), returned_checklists):
@@ -89,27 +86,19 @@ class ChecklistTestCase(CourseTestCase):
 
     def test_update_checklists_post_no_index(self):
         """ No checklist index, will error on post. """
-        update_url = reverse('checklists_updates', kwargs={'org': self.course.location.org,
-                                                           'course': self.course.location.course,
-                                                           'name': self.course.location.name})
-        response = self.client.post(update_url)
+        response = self.client.post(self.checklists_url)
         self.assertContains(response, 'Could not save checklist', status_code=400)
 
     def test_update_checklists_index_out_of_range(self):
         """ Checklist index out of range, will error on post. """
-        update_url = reverse('checklists_updates', kwargs={'org': self.course.location.org,
-                                                           'course': self.course.location.course,
-                                                           'name': self.course.location.name,
-                                                           'checklist_index': 100})
+        update_url = self.location.url_reverse('checklists/', '100')
+
         response = self.client.post(update_url)
         self.assertContains(response, 'Could not save checklist', status_code=400)
 
     def test_update_checklists_index(self):
         """ Check that an update of a particular checklist works. """
-        update_url = reverse('checklists_updates', kwargs={'org': self.course.location.org,
-                                                           'course': self.course.location.course,
-                                                           'name': self.course.location.name,
-                                                           'checklist_index': 1})
+        update_url = self.location.url_reverse('checklists/', '1')
 
         payload = self.course.checklists[1]
         self.assertFalse(get_first_item(payload).get('is_checked'))
@@ -127,10 +116,7 @@ class ChecklistTestCase(CourseTestCase):
 
     def test_update_checklists_delete_unsupported(self):
         """ Delete operation is not supported. """
-        update_url = reverse('checklists_updates', kwargs={'org': self.course.location.org,
-                                                           'course': self.course.location.course,
-                                                           'name': self.course.location.name,
-                                                           'checklist_index': 100})
+        update_url = self.location.url_reverse('checklists/', '100')
         response = self.client.delete(update_url)
         self.assertEqual(response.status_code, 405)
 
@@ -152,7 +138,7 @@ class ChecklistTestCase(CourseTestCase):
             self.assertEqual(get_action_url(checklist, index), stored)
 
         test_expansion(self.course.checklists[0], 0, 'ManageUsers', '/mitX/333/team/Checklists_Course')
-        test_expansion(self.course.checklists[1], 1, 'CourseOutline', '/mitX/333/course/Checklists_Course')
+        test_expansion(self.course.checklists[1], 1, 'CourseOutline', '/course/mitX.333.Checklists_Course/branch/draft/block/Checklists_Course')
         test_expansion(self.course.checklists[2], 0, 'http://help.edge.edx.org/', 'http://help.edge.edx.org/')
 
 

cms/djangoapps/contentstore/views/checklist.py

@@ -8,62 +8,65 @@ from django.views.decorators.http import require_http_methods
 from django.core.urlresolvers import reverse
 from django_future.csrf import ensure_csrf_cookie
 from mitxmako.shortcuts import render_to_response
+from django.http import HttpResponseNotFound
+from django.core.exceptions import PermissionDenied
+from xmodule.modulestore.django import loc_mapper
 
 from xmodule.modulestore.inheritance import own_metadata
 
+
 from ..utils import get_modulestore
-from .access import get_location_and_verify_access
+from .access import has_access
 from xmodule.course_module import CourseDescriptor
+from xmodule.modulestore.locator import BlockUsageLocator
 
-__all__ = ['get_checklists', 'update_checklist']
-
+__all__ = ['checklists_handler']
 
-@ensure_csrf_cookie
+@require_http_methods(("GET", "POST", "PUT"))
 @login_required
-def get_checklists(request, org, course, name):
+@ensure_csrf_cookie
+def checklists_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None, checklist_index=None):
     """
-    Send models, views, and html for displaying the course checklists.
+    The restful handler for checklists.
 
-    org, course, name: Attributes of the Location for the item to edit
+    GET
+        html: return html page for all checklists
+        json: return json representing all checklists. checklist_index is not supported for GET at this time.
+    POST or PUT
+        json: updates the checked state for items within a particular checklist. checklist_index is required.
     """
-    location = get_location_and_verify_access(request, org, course, name)
+    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+    if not has_access(request.user, location):
+        raise PermissionDenied()
 
-    modulestore = get_modulestore(location)
-    course_module = modulestore.get_item(location)
+    old_location = loc_mapper().translate_locator_to_location(location)
 
-    # If course was created before checklists were introduced, copy them over
-    # from the template.
-    if not course_module.checklists:
-        course_module.checklists = CourseDescriptor.checklists.default
-        course_module.save()
-        modulestore.update_metadata(location, own_metadata(course_module))
-
-    expanded_checklists = expand_all_action_urls(course_module)
-    return render_to_response('checklists.html',
-                              {
-                                  'context_course': course_module,
-                                  'checklists': expanded_checklists
-                              })
-
-
-@require_http_methods(("GET", "POST", "PUT"))
-@ensure_csrf_cookie
-@login_required
-def update_checklist(request, org, course, name, checklist_index=None):
-    """
-    restful CRUD operations on course checklists. The payload is a json rep of
-    the modified checklist. For PUT or POST requests, the index of the
-    checklist being modified must be included; the returned payload will
-    be just that one checklist. For GET requests, the returned payload
-    is a json representation of the list of all checklists.
+    modulestore = get_modulestore(old_location)
+    course_module = modulestore.get_item(old_location)
 
-    org, course, name: Attributes of the Location for the item to edit
-    """
-    location = get_location_and_verify_access(request, org, course, name)
-    modulestore = get_modulestore(location)
-    course_module = modulestore.get_item(location)
+    json_request = 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json')
+    if request.method == 'GET':
+        # If course was created before checklists were introduced, copy them over
+        # from the template.
+        if not course_module.checklists:
+            course_module.checklists = CourseDescriptor.checklists.default
+            course_module.save()
+            modulestore.update_metadata(old_location, own_metadata(course_module))
 
-    if request.method in ("POST", "PUT"):
+        expanded_checklists = expand_all_action_urls(course_module)
+        if json_request:
+            return JsonResponse(expanded_checklists)
+        else:
+            handler_url = location.url_reverse('checklists/', '')
+            return render_to_response('checklists.html',
+                                      {
+                                          'handler_url': handler_url,
+                                          # context_course is used by analytics
+                                          'context_course': course_module,
+                                          'checklists': expanded_checklists
+                                      })
+    elif json_request:
+        # Can now assume POST or PUT because GET handled above.
         if checklist_index is not None and 0 <= int(checklist_index) < len(course_module.checklists):
             index = int(checklist_index)
             persisted_checklist = course_module.checklists[index]
@@ -77,20 +80,17 @@ def update_checklist(request, org, course, name, checklist_index=None):
             # not default
             course_module.checklists = course_module.checklists
             course_module.save()
-            modulestore.update_metadata(location, own_metadata(course_module))
+            modulestore.update_metadata(old_location, own_metadata(course_module))
             expanded_checklist = expand_checklist_action_url(course_module, persisted_checklist)
             return JsonResponse(expanded_checklist)
         else:
             return HttpResponseBadRequest(
                 ( "Could not save checklist state because the checklist index "
-                "was out of range or unspecified."),
+                  "was out of range or unspecified."),
                 content_type="text/plain"
             )
-    elif request.method == 'GET':
-        # In the JavaScript view initialize method, we do a fetch to get all
-        # the checklists.
-        expanded_checklists = expand_all_action_urls(course_module)
-        return JsonResponse(expanded_checklists)
+    else:
+        return HttpResponseNotFound()
 
 
 def expand_all_action_urls(course_module):
@@ -116,19 +116,21 @@ def expand_checklist_action_url(course_module, checklist):
     urlconf_map = {
         "ManageUsers": "manage_users",
         "SettingsDetails": "settings_details",
-        "SettingsGrading": "settings_grading",
-        "CourseOutline": "course_index",
-        "Checklists": "checklists",
+        "SettingsGrading": "settings_grading"
     }
+
     for item in expanded_checklist.get('items'):
         action_url = item.get('action_url')
-        if action_url not in urlconf_map:
-            continue
-        urlconf_name = urlconf_map[action_url]
-        item['action_url'] = reverse(urlconf_name, kwargs={
-            'org': course_module.location.org,
-            'course': course_module.location.course,
-            'name': course_module.location.name,
-        })
+        if action_url == "CourseOutline":
+            ctx_loc = course_module.location
+            location = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True)
+            item['action_url'] = location.url_reverse('course/', '')
+        elif action_url in urlconf_map:
+            urlconf_name = urlconf_map[action_url]
+            item['action_url'] = reverse(urlconf_name, kwargs={
+                'org': course_module.location.org,
+                'course': course_module.location.course,
+                'name': course_module.location.name,
+            })
 
     return expanded_checklist

cms/templates/checklists.html

@@ -20,8 +20,8 @@
 <script type="text/javascript">
 require(["domReady!", "jquery", "js/collections/checklist", "js/views/checklist"],
         function(doc, $, ChecklistCollection, ChecklistView) {
-    var checklistCollection = new ChecklistCollection()
-    checklistCollection.url = "${reverse('checklists_updates', kwargs=dict(org=context_course.location.org, course=context_course.location.course, name=context_course.location.name))}";
+    var checklistCollection = new ChecklistCollection();
+    checklistCollection.url = "${handler_url}";
 
     var editor = new ChecklistView({
         el: $('.course-checklists'),

cms/templates/widgets/header.html

@@ -15,7 +15,9 @@
       % if context_course:
       <% 
             ctx_loc = context_course.location
-            index_url = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True).url_reverse('course/', '')
+            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/', '')
       %>
       <h2 class="info-course">
         <span class="sr">${_("Current Course:")}</span>
@@ -84,7 +86,7 @@
               <div class="nav-sub">
                 <ul>
                   <li class="nav-item nav-course-tools-checklists">
-                    <a href="${reverse('checklists', kwargs=dict(org=ctx_loc.org, course=ctx_loc.course, name=ctx_loc.name))}">${_("Checklists")}</a>
+                    <a href="${checklists_url}">${_("Checklists")}</a>
                   </li>
                   <li class="nav-item nav-course-tools-import">
                     <a href="${reverse('import_course', kwargs=dict(org=ctx_loc.org, course=ctx_loc.course, name=ctx_loc.name))}">${_("Import")}</a>

cms/urls.py

@@ -108,9 +108,6 @@ urlpatterns = patterns('',  # nopep8
 # User creation and updating views
 urlpatterns += patterns(
     '',
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/checklists/(?P<name>[^/]+)$', 'contentstore.views.get_checklists', name='checklists'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/checklists/(?P<name>[^/]+)/update(/)?(?P<checklist_index>.+)?.*$',
-        'contentstore.views.update_checklist', name='checklists_updates'),
     url(r'^howitworks$', 'contentstore.views.howitworks', name='howitworks'),
     url(r'^signup$', 'contentstore.views.signup', name='signup'),
 
@@ -140,6 +137,7 @@ urlpatterns += patterns(
     url(r'^course$', 'index'),
     # (?ix) == ignore case and verbose (multiline regex)
     url(r'(?ix)^course/{}$'.format(parsers.URL_RE_SOURCE), 'course_handler'),
+    url(r'(?ix)^checklists/{}(/)?(?P<checklist_index>\d+)?$'.format(parsers.URL_RE_SOURCE), 'checklists_handler'),
 )
 
 js_info_dict = {