Merge pull request #1542 from edx/dhm/restful_course

RESTful refactoring of /course access continued

cms/djangoapps/contentstore/features/signup.feature

@@ -26,4 +26,4 @@ Feature: CMS.Sign in
     And I visit the url "/signin?next=http://www.google.com/"
     When I fill in and submit the signin form
     And I wait for "2" seconds
-    Then I should see that the path is "/"
+    Then I should see that the path is "/course"

cms/djangoapps/contentstore/tests/test_course_index.py

@@ -31,7 +31,7 @@ class TestCourseIndex(CourseTestCase):
         """
         Test getting the list of courses and then pulling up their outlines
         """
-        index_url = reverse('contentstore.views.index')
+        index_url = '/course'
         index_response = authed_client.get(index_url, {}, HTTP_ACCEPT='text/html')
         parsed_html = lxml.html.fromstring(index_response.content)
         course_link_eles = parsed_html.find_class('course-link')

cms/djangoapps/contentstore/tests/test_i18n.py

 from unittest import skip
 
-from django.core.urlresolvers import reverse
 from django.contrib.auth.models import User
-from django.test.client import Client
 from django.test.utils import override_settings
 
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from contentstore.tests.modulestore_config import TEST_MODULESTORE
+from contentstore.tests.utils import AjaxEnabledTestClient
 
 
 @override_settings(MODULESTORE=TEST_MODULESTORE)
@@ -45,10 +44,10 @@ class InternationalizationTest(ModuleStoreTestCase):
 
     def test_course_plain_english(self):
         """Test viewing the index page with no courses"""
-        self.client = Client()
+        self.client = AjaxEnabledTestClient()
         self.client.login(username=self.uname, password=self.password)
 
-        resp = self.client.get(reverse('index'))
+        resp = self.client.get_html('/course')
         self.assertContains(resp,
                             '<h1 class="page-header">My Courses</h1>',
                             status_code=200,
@@ -56,10 +55,10 @@ class InternationalizationTest(ModuleStoreTestCase):
 
     def test_course_explicit_english(self):
         """Test viewing the index page with no courses"""
-        self.client = Client()
+        self.client = AjaxEnabledTestClient()
         self.client.login(username=self.uname, password=self.password)
 
-        resp = self.client.get(reverse('index'),
+        resp = self.client.get_html('/course',
                                {},
                                HTTP_ACCEPT_LANGUAGE='en'
                                )
@@ -80,10 +79,10 @@ class InternationalizationTest(ModuleStoreTestCase):
     @skip
     def test_course_with_accents(self):
         """Test viewing the index page with no courses"""
-        self.client = Client()
+        self.client = AjaxEnabledTestClient()
         self.client.login(username=self.uname, password=self.password)
 
-        resp = self.client.get(reverse('index'),
+        resp = self.client.get_html('/course',
                                {},
                                HTTP_ACCEPT_LANGUAGE='fr'
                                )

cms/djangoapps/contentstore/tests/tests.py

-from django.test.client import Client
 from django.test.utils import override_settings
 from django.core.cache import cache
 from django.core.urlresolvers import reverse
 
-from contentstore.tests.utils import parse_json, user, registration
+from contentstore.tests.utils import parse_json, user, registration, AjaxEnabledTestClient
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from contentstore.tests.test_course_settings import CourseTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
@@ -82,12 +81,12 @@ class AuthTestCase(ContentStoreTestCase):
         self.email = 'a@b.com'
         self.pw = 'xyz'
         self.username = 'testuser'
-        self.client = Client()
+        self.client = AjaxEnabledTestClient()
         # clear the cache so ratelimiting won't affect these tests
         cache.clear()
 
     def check_page_get(self, url, expected):
-        resp = self.client.get(url)
+        resp = self.client.get_html(url)
         self.assertEqual(resp.status_code, expected)
         return resp
 
@@ -152,20 +151,20 @@ class AuthTestCase(ContentStoreTestCase):
     def test_private_pages_auth(self):
         """Make sure pages that do require login work."""
         auth_pages = (
-            reverse('index'),
+            '/course',
         )
 
         # These are pages that should just load when the user is logged in
         # (no data needed)
         simple_auth_pages = (
-            reverse('index'),
+            '/course',
         )
 
         # need an activated user
         self.test_create_account()
 
         # Create a new session
-        self.client = Client()
+        self.client = AjaxEnabledTestClient()
 
         # Not logged in.  Should redirect to login.
         print('Not logged in')
@@ -184,7 +183,7 @@ class AuthTestCase(ContentStoreTestCase):
     def test_index_auth(self):
 
         # not logged in.  Should return a redirect.
-        resp = self.client.get(reverse('index'))
+        resp = self.client.get_html('/course')
         self.assertEqual(resp.status_code, 302)
 
         # Logged in should work.

cms/djangoapps/contentstore/tests/utils.py

@@ -30,12 +30,24 @@ def registration(email):
 
 
 class AjaxEnabledTestClient(Client):
+    """
+    Convenience class to make testing easier.
+    """
     def ajax_post(self, path, data=None, content_type="application/json", **kwargs):
+        """
+        Convenience method for client post which serializes the data into json and sets the accept type
+        to json
+        """
         if not isinstance(data, basestring):
             data = json.dumps(data or {})
         kwargs.setdefault("HTTP_X_REQUESTED_WITH", "XMLHttpRequest")
         return self.post(path=path, data=data, content_type=content_type, **kwargs)
 
+    def get_html(self, path, data=None, follow=False, **extra):
+        """
+        Convenience method for client.get which sets the accept type to html
+        """
+        return self.get(path, data or {}, follow, HTTP_ACCEPT="text/html", **extra)
 
 @override_settings(MODULESTORE=TEST_MODULESTORE)
 class CourseTestCase(ModuleStoreTestCase):

cms/djangoapps/contentstore/views/course.py

@@ -3,9 +3,11 @@ Views related to operations on course objects
 """
 import json
 import random
-from django.utils.translation import ugettext as _
 import string  # pylint: disable=W0402
+import re
+import bson
 
+from django.utils.translation import ugettext as _
 from django.contrib.auth.decorators import login_required
 from django_future.csrf import ensure_csrf_cookie
 from django.conf import settings
@@ -16,6 +18,7 @@ from django.http import HttpResponseBadRequest, HttpResponseNotFound
 from util.json_request import JsonResponse
 from mitxmako.shortcuts import render_to_response
 
+from xmodule.error_module import ErrorDescriptor
 from xmodule.modulestore.django import modulestore, loc_mapper
 from xmodule.modulestore.inheritance import own_metadata
 from xmodule.contentstore.content import StaticContent
@@ -49,9 +52,9 @@ from student.models import CourseEnrollment
 
 from xmodule.html_module import AboutDescriptor
 from xmodule.modulestore.locator import BlockUsageLocator
-import re
-import bson
-__all__ = ['create_new_course', 'course_info', 'course_handler',
+from course_creators.views import get_course_creator_status, add_user_with_status_unrequested
+
+__all__ = ['course_info', 'course_handler',
            'course_info_updates', 'get_course_settings',
            'course_config_graders_page',
            'course_config_advanced_page',
@@ -69,11 +72,12 @@ def course_handler(request, tag=None, course_id=None, branch=None, version_guid=
     will typically be a 'course' object but may not be especially as we support modules.
 
     GET
-        html: return html page overview for the given course
+        html: return course listing page if not given a course id
+        html: return html page overview for the given course if given a course id
         json: return json representing the course branch's index entry as well as dag w/ all of the children
         replaced w/ json docs where each doc has {'_id': , 'display_name': , 'children': }
     POST
-        json: create (or update?) this course or branch in this course for this user, return resulting json
+        json: create a course, return resulting json
         descriptor (same as in GET course/...). Leaving off /branch/draft would imply create the course w/ default
         branches. Cannot change the structure contents ('_id', 'display_name', 'children') but can change the
         index entry.
@@ -86,13 +90,13 @@ def course_handler(request, tag=None, course_id=None, branch=None, version_guid=
     if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'):
         if request.method == 'GET':
             raise NotImplementedError('coming soon')
+        elif request.method == 'POST':  # not sure if this is only post. If one will have ids, it goes after access
+            return create_new_course(request)
         elif not has_access(
             request.user,
             BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
         ):
             raise PermissionDenied()
-        elif request.method == 'POST':
-            raise NotImplementedError()
         elif request.method == 'PUT':
             raise NotImplementedError()
         elif request.method == 'DELETE':
@@ -100,13 +104,66 @@ def course_handler(request, tag=None, course_id=None, branch=None, version_guid=
         else:
             return HttpResponseBadRequest()
     elif request.method == 'GET':  # assume html
-        return course_index(request, course_id, branch, version_guid, block)
+        if course_id is None:
+            return course_listing(request)
+        else:
+            return course_index(request, course_id, branch, version_guid, block)
     else:
         return HttpResponseNotFound()
 
 
 @login_required
 @ensure_csrf_cookie
+def course_listing(request):
+    """
+    List all courses available to the logged in user
+    """
+    courses = modulestore('direct').get_items(['i4x', None, None, 'course', None])
+
+    # filter out courses that we don't have access too
+    def course_filter(course):
+        """
+        Get courses to which this user has access
+        """
+        return (has_access(request.user, course.location)
+                # pylint: disable=fixme
+                # TODO remove this condition when templates purged from db
+                and course.location.course != 'templates'
+                and course.location.org != ''
+                and course.location.course != ''
+                and course.location.name != '')
+    courses = filter(course_filter, courses)
+
+    def format_course_for_view(course):
+        """
+        return tuple of the data which the view requires for each course
+        """
+        # published = false b/c studio manipulates draft versions not b/c the course isn't pub'd
+        course_loc = loc_mapper().translate_location(
+            course.location.course_id, course.location, published=False, add_entry_if_missing=True
+        )
+        return (
+            course.display_name,
+            # note, couldn't get django reverse to work; so, wrote workaround
+            course_loc.url_reverse('course/', ''),
+            get_lms_link_for_item(
+                course.location
+            ),
+            course.display_org_with_default,
+            course.display_number_with_default,
+            course.location.name
+        )
+
+    return render_to_response('index.html', {
+        'courses': [format_course_for_view(c) for c in courses if not isinstance(c, ErrorDescriptor)],
+        'user': request.user,
+        'request_course_creator_url': reverse('contentstore.views.request_course_creator'),
+        'course_creator_status': _get_course_creator_status(request.user),
+    })
+
+
+@login_required
+@ensure_csrf_cookie
 def course_index(request, course_id, branch, version_guid, block):
     """
     Display an editable course overview.
@@ -142,7 +199,6 @@ def course_index(request, course_id, branch, version_guid, block):
     })
 
 
-@login_required
 @expect_json
 def create_new_course(request):
     """
@@ -756,3 +812,28 @@ def textbook_by_id(request, org, course, name, tid):
             own_metadata(course_module)
         )
         return JsonResponse()
+
+
+def _get_course_creator_status(user):
+    """
+    Helper method for returning the course creator status for a particular user,
+    taking into account the values of DISABLE_COURSE_CREATION and ENABLE_CREATOR_GROUP.
+
+    If the user passed in has not previously visited the index page, it will be
+    added with status 'unrequested' if the course creator group is in use.
+    """
+    if user.is_staff:
+        course_creator_status = 'granted'
+    elif settings.MITX_FEATURES.get('DISABLE_COURSE_CREATION', False):
+        course_creator_status = 'disallowed_for_this_site'
+    elif settings.MITX_FEATURES.get('ENABLE_CREATOR_GROUP', False):
+        course_creator_status = get_course_creator_status(user)
+        if course_creator_status is None:
+            # User not grandfathered in as an existing user, has not previously visited the dashboard page.
+            # Add the user to the course creator admin table with status 'unrequested'.
+            add_user_with_status_unrequested(user)
+            course_creator_status = get_course_creator_status(user)
+    else:
+        course_creator_status = 'granted'
+
+    return course_creator_status

cms/djangoapps/contentstore/views/public.py

@@ -9,7 +9,6 @@ from django.conf import settings
 from mitxmako.shortcuts import render_to_response
 
 from external_auth.views import ssl_login_shortcut
-from .user import index
 
 __all__ = ['signup', 'old_login_redirect', 'login_page', 'howitworks']
 
@@ -46,6 +45,6 @@ def login_page(request):
 def howitworks(request):
     "Proxy view"
     if request.user.is_authenticated():
-        return index(request)
+        return redirect('/course')
     else:
         return render_to_response('howitworks.html', {})

cms/djangoapps/contentstore/views/user.py

 import json
 from django.conf import settings
 from django.core.exceptions import PermissionDenied
-from django.core.urlresolvers import reverse
 from django.contrib.auth.models import User, Group
 from django.contrib.auth.decorators import login_required
 from django.views.decorators.http import require_http_methods
@@ -9,17 +8,12 @@ from django.utils.translation import ugettext as _
 from django.views.decorators.http import require_POST
 from django_future.csrf import ensure_csrf_cookie
 from mitxmako.shortcuts import render_to_response
-from django.core.context_processors import csrf
 
 from xmodule.modulestore.django import modulestore, loc_mapper
-from xmodule.error_module import ErrorDescriptor
-from contentstore.utils import get_lms_link_for_item
 from util.json_request import JsonResponse
 from auth.authz import (
     STAFF_ROLE_NAME, INSTRUCTOR_ROLE_NAME, get_course_groupname_for_role)
-from course_creators.views import (
-    get_course_creator_status, add_user_with_status_unrequested,
-    user_requested_access)
+from course_creators.views import user_requested_access
 
 from .access import has_access
 
@@ -28,51 +22,7 @@ from xmodule.modulestore.locator import BlockUsageLocator
 from django.http import HttpResponseNotFound
 
 
-__all__ = ['index', 'request_course_creator', 'course_team_handler']
-
-
-@login_required
-@ensure_csrf_cookie
-def index(request):
-    """
-    List all courses available to the logged in user
-    """
-    courses = modulestore('direct').get_items(['i4x', None, None, 'course', None])
-
-    # filter out courses that we don't have access too
-    def course_filter(course):
-        return (has_access(request.user, course.location)
-                # TODO remove this condition when templates purged from db
-                and course.location.course != 'templates'
-                and course.location.org != ''
-                and course.location.course != ''
-                and course.location.name != '')
-    courses = filter(course_filter, courses)
-
-    def format_course_for_view(course):
-        # published = false b/c studio manipulates draft versions not b/c the course isn't pub'd
-        course_loc = loc_mapper().translate_location(
-            course.location.course_id, course.location, published=False, add_entry_if_missing=True
-        )
-        return (
-            course.display_name,
-            # note, couldn't get django reverse to work; so, wrote workaround
-            course_loc.url_reverse('course/', ''),
-            get_lms_link_for_item(
-                course.location
-            ),
-            course.display_org_with_default,
-            course.display_number_with_default,
-            course.location.name
-        )
-
-    return render_to_response('index.html', {
-        'courses': [format_course_for_view(c) for c in courses if not isinstance(c, ErrorDescriptor)],
-        'user': request.user,
-        'request_course_creator_url': reverse('contentstore.views.request_course_creator'),
-        'course_creator_status': _get_course_creator_status(request.user),
-        'csrf': csrf(request)['csrf_token']
-    })
+__all__ = ['request_course_creator', 'course_team_handler']
 
 
 @require_POST
@@ -254,27 +204,3 @@ def _course_team_user(request, location, email):
 
     return JsonResponse()
 
-
-def _get_course_creator_status(user):
-    """
-    Helper method for returning the course creator status for a particular user,
-    taking into account the values of DISABLE_COURSE_CREATION and ENABLE_CREATOR_GROUP.
-
-    If the user passed in has not previously visited the index page, it will be
-    added with status 'unrequested' if the course creator group is in use.
-    """
-    if user.is_staff:
-        course_creator_status = 'granted'
-    elif settings.MITX_FEATURES.get('DISABLE_COURSE_CREATION', False):
-        course_creator_status = 'disallowed_for_this_site'
-    elif settings.MITX_FEATURES.get('ENABLE_CREATOR_GROUP', False):
-        course_creator_status = get_course_creator_status(user)
-        if course_creator_status is None:
-            # User not grandfathered in as an existing user, has not previously visited the dashboard page.
-            # Add the user to the course creator admin table with status 'unrequested'.
-            add_user_with_status_unrequested(user)
-            course_creator_status = get_course_creator_status(user)
-    else:
-        course_creator_status = 'granted'
-
-    return course_creator_status

cms/static/js/index.js

@@ -32,7 +32,7 @@ require(["domReady", "jquery", "underscore", "js/utils/cancel_on_escape"],
                 'run': run
             });
 
-            $.postJSON('/create_new_course', {
+            $.postJSON('/course', {
                     'org': org,
                     'number': number,
                     'display_name': display_name,

cms/templates/index.html

@@ -212,7 +212,6 @@ require(["domReady!", "jquery", "jquery.form", "js/index"], function(doc, $) {
             <h4 class="title">${_('Your Course Creator Request Status:')}</h4>
 
             <form id="request-coursecreator" action="${request_course_creator_url}" method="post" enctype="multipart/form-data">
-              <input type="hidden" name="csrfmiddlewaretoken" value="${csrf}">
               <div class="form-actions">
                 <button type="submit" id="request-coursecreator-submit" name="request-coursecreator-submit" class="action-primary action-request"><i class="icon-cog icon-inline icon-spin"></i> <span class="label">${_('Request the Ability to Create Courses')}</span></button>
               </div>

cms/templates/signup.html

@@ -124,7 +124,7 @@ require(["jquery", "jquery.cookie"], function($) {
       submit_data,
       function(json) {
         if(json.success) {
-          location.href = "${reverse('index')}";
+          location.href = "${'/course'}";
         } else {
           $('#register_error').html(json.value).stop().addClass('is-shown');
         }

cms/urls.py

@@ -30,7 +30,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'^create_new_course', 'contentstore.views.create_new_course', name='create_new_course'),
     url(r'^reorder_static_tabs', 'contentstore.views.reorder_static_tabs', name='reorder_static_tabs'),
 
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/export/(?P<name>[^/]+)$',
@@ -116,12 +115,11 @@ urlpatterns += patterns(
 urlpatterns += patterns(
     'contentstore.views',
 
-    url(r'^course$', 'index', name='index'),
     url(r'^request_course_creator$', 'request_course_creator'),
     # (?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'),
     url(r'(?ix)^course_team/{}(/)?(?P<email>.+)?$'.format(parsers.URL_RE_SOURCE), 'course_team_handler'),
+    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'),
     url(r'(?ix)^orphan/{}$'.format(parsers.URL_RE_SOURCE), 'orphan'),
     url(r'(?ix)^assets/{}(/)?(?P<asset_id>.+)?$'.format(parsers.URL_RE_SOURCE), 'assets_handler'),
     url(r'(?ix)^import/{}$'.format(parsers.URL_RE_SOURCE), 'import_handler'),

common/djangoapps/student/tests/test_userstanding.py

@@ -53,7 +53,7 @@ class UserStandingTest(TestCase):
         try:
             self.some_url = reverse('dashboard')
         except NoReverseMatch:
-            self.some_url = reverse('index')
+            self.some_url = '/course'
 
         # since it's only possible to disable accounts from lms, we're going
         # to skip tests for cms