Merge pull request #1470 from edx/christina/cleanup-course-index

Get rid of course_index shim.

cms/djangoapps/contentstore/features/grading.py

@@ -67,9 +67,13 @@ def change_assignment_name(step, old_name, new_name):
 
 @step(u'I go back to the main course page')
 def main_course_page(step):
-    main_page_link = '/{}/{}/course/{}'.format(world.scenario_dict['COURSE'].org,
+    course_name = world.scenario_dict['COURSE'].display_name.replace(' ', '_')
-                                               world.scenario_dict['COURSE'].number,
+    main_page_link = '/course/{org}.{number}.{name}/branch/draft/block/{name}'.format(
-                                               world.scenario_dict['COURSE'].display_name.replace(' ', '_'),)
+        org=world.scenario_dict['COURSE'].org,
+        number=world.scenario_dict['COURSE'].number,
+        name=course_name
+    )
+
     world.visit(main_page_link)
     assert_in('Course Outline', world.css_text('h1.page-header'))
 

cms/djangoapps/contentstore/features/signup.feature

@@ -14,11 +14,11 @@ Feature: CMS.Sign in
   Scenario: Login with a valid redirect
     Given I have opened a new course in Studio
     And I am not logged in
-    And I visit the url "/MITx/999/course/Robot_Super_Course"
+    And I visit the url "/course/MITx.999.Robot_Super_Course/branch/draft/block/Robot_Super_Course"
-    And I should see that the path is "/signin?next=/MITx/999/course/Robot_Super_Course"
+    And I should see that the path is "/signin?next=/course/MITx.999.Robot_Super_Course/branch/draft/block/Robot_Super_Course"
     When I fill in and submit the signin form
     And I wait for "2" seconds
-    Then I should see that the path is "/MITx/999/course/Robot_Super_Course"
+    Then I should see that the path is "/course/MITx.999.Robot_Super_Course/branch/draft/block/Robot_Super_Course"
 
   Scenario: Login with an invalid redirect
     Given I have opened a new course in Studio

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -56,6 +56,7 @@ from pymongo import MongoClient
 from student.models import CourseEnrollment
 
 from contentstore.utils import delete_course_and_groups
+from xmodule.modulestore.django import loc_mapper
 
 TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE)
 TEST_DATA_CONTENTSTORE['DOC_STORE_CONFIG']['db'] = 'test_xcontent_%s' % uuid4().hex
@@ -710,10 +711,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         items = module_store.get_items(Location([source_location.tag, source_location.org, source_location.course, None, None, 'draft']))
         self.assertGreater(len(items), 0)
 
-        resp = self.client.post(reverse('create_new_course'), course_data)
+        _create_course(self, course_data)
-        self.assertEqual(resp.status_code, 200)
-        data = parse_json(resp)
-        self.assertEqual(data['id'], 'i4x://MITx/999/course/2013_Spring')
 
         content_store = contentstore()
 
@@ -776,7 +774,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         }
 
         module_store = modulestore('direct')
-        draft_store = modulestore('draft')
         content_store = contentstore()
 
         import_from_xml(module_store, 'common/test/data/', ['toy'])
@@ -801,11 +798,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         self.assertEqual(new_data, html_module.data)
 
         # create the destination course
-
+        _create_course(self, course_data)
-        resp = self.client.post(reverse('create_new_course'), course_data)
-        self.assertEqual(resp.status_code, 200)
-        data = parse_json(resp)
-        self.assertEqual(data['id'], 'i4x://MITx/999/course/2013_Spring')
 
         # do the actual cloning
         clone_course(module_store, content_store, source_location, dest_location)
@@ -1369,25 +1362,21 @@ class ContentStoreTest(ModuleStoreTestCase):
         test_course_data.update(self.course_data)
         if number_suffix:
             test_course_data['number'] = '{0}_{1}'.format(test_course_data['number'], number_suffix)
-        resp = self.client.post(reverse('create_new_course'), test_course_data)
+        _create_course(self, test_course_data)
-        self.assertEqual(resp.status_code, 200)
-        data = parse_json(resp)
-        self.assertNotIn('ErrMsg', data)
-        self.assertEqual(data['id'], 'i4x://MITx/{0}/course/2013_Spring'.format(test_course_data['number']))
         # Verify that the creator is now registered in the course.
-        self.assertTrue(CourseEnrollment.is_enrolled(self.user, self._get_course_id(test_course_data)))
+        self.assertTrue(CourseEnrollment.is_enrolled(self.user, _get_course_id(test_course_data)))
         return test_course_data
 
     def test_create_course_check_forum_seeding(self):
         """Test new course creation and verify forum seeding """
         test_course_data = self.assert_created_course(number_suffix=uuid4().hex)
-        self.assertTrue(are_permissions_roles_seeded(self._get_course_id(test_course_data)))
+        self.assertTrue(are_permissions_roles_seeded(_get_course_id(test_course_data)))
 
     def test_forum_unseeding_on_delete(self):
         """Test new course creation and verify forum unseeding """
         test_course_data = self.assert_created_course(number_suffix=uuid4().hex)
-        self.assertTrue(are_permissions_roles_seeded(self._get_course_id(test_course_data)))
+        course_id = _get_course_id(test_course_data)
-        course_id = self._get_course_id(test_course_data)
+        self.assertTrue(are_permissions_roles_seeded(course_id))
         delete_course_and_groups(course_id, commit=True)
         self.assertFalse(are_permissions_roles_seeded(course_id))
 
@@ -1397,18 +1386,14 @@ class ContentStoreTest(ModuleStoreTestCase):
         second_course_data = self.assert_created_course(number_suffix=uuid4().hex)
 
         # unseed the forums for the first course
-        course_id = self._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))
 
-        second_course_id = self._get_course_id(second_course_data)
+        second_course_id = _get_course_id(second_course_data)
         # permissions should still be there for the other course
         self.assertTrue(are_permissions_roles_seeded(second_course_id))
 
-    def _get_course_id(self, test_course_data):
-        """Returns the course ID (org/number/run)."""
-        return "{org}/{number}/{run}".format(**test_course_data)
-
     def test_create_course_duplicate_course(self):
         """Test new course creation - error path"""
         self.client.post(reverse('create_new_course'), self.course_data)
@@ -1418,7 +1403,7 @@ class ContentStoreTest(ModuleStoreTestCase):
         """
         Checks that the course did not get created
         """
-        course_id = self._get_course_id(self.course_data)
+        course_id = _get_course_id(self.course_data)
         initially_enrolled = CourseEnrollment.is_enrolled(self.user, course_id)
         resp = self.client.post(reverse('create_new_course'), self.course_data)
         self.assertEqual(resp.status_code, 200)
@@ -1544,13 +1529,8 @@ class ContentStoreTest(ModuleStoreTestCase):
         """Test viewing the course overview page with an existing course"""
         CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course')
 
-        data = {
+        loc = Location(['i4x', 'MITx', '999', 'course', Location.clean('Robot Super Course'), None])
-            'org': 'MITx',
+        resp = self._show_course_overview(loc)
-            'course': '999',
-            'name': Location.clean('Robot Super Course'),
-        }
-
-        resp = self.client.get(reverse('course_index', kwargs=data))
         self.assertContains(
             resp,
             '<article class="courseware-overview" data-id="i4x://MITx/999/course/Robot_Super_Course">',
@@ -1605,11 +1585,7 @@ class ContentStoreTest(ModuleStoreTestCase):
         """
         import_from_xml(modulestore('direct'), 'common/test/data/', ['simple'])
         loc = Location(['i4x', 'edX', 'simple', 'course', '2012_Fall', None])
-        resp = self.client.get(reverse('course_index',
+        resp = self._show_course_overview(loc)
-                                       kwargs={'org': loc.org,
-                                               'course': loc.course,
-                                               'name': loc.name}))
-
         self.assertEqual(resp.status_code, 200)
         self.assertContains(resp, 'Chapter 2')
 
@@ -1713,10 +1689,7 @@ class ContentStoreTest(ModuleStoreTestCase):
 
         target_course_id = '{0}/{1}/{2}'.format(target_location.org, target_location.course, target_location.name)
 
-        resp = self.client.post(reverse('create_new_course'), course_data)
+        _create_course(self, course_data)
-        self.assertEqual(resp.status_code, 200)
-        data = parse_json(resp)
-        self.assertEqual(data['id'], target_location.url())
 
         import_from_xml(module_store, 'common/test/data/', ['toy'], target_location_namespace=target_location)
 
@@ -1884,6 +1857,13 @@ class ContentStoreTest(ModuleStoreTestCase):
         location = course.location._replace(tag='c4x', category='asset', name=course.course_image)
         content_store.find(location)
 
+    def _show_course_overview(self, location):
+        """
+        Show the course overview page.
+        """
+        new_location = loc_mapper().translate_location(location.course_id, location, False, True)
+        return self.client.get(new_location.url_reverse('course/', ''), HTTP_ACCEPT='text/html')
+
 
 @override_settings(MODULESTORE=TEST_MODULESTORE)
 class MetadataSaveTestCase(ModuleStoreTestCase):
@@ -1947,3 +1927,22 @@ class MetadataSaveTestCase(ModuleStoreTestCase):
         # TODO: create the same test as `test_metadata_not_persistence`,
         # but check persistence for some other module.
         pass
+
+
+def _create_course(test, course_data):
+    """
+    Creates a course 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)
+
+    response = test.client.post(reverse('create_new_course'), 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/", ""))
+
+
+def _get_course_id(test_course_data):
+    """Returns the course ID (org/number/run)."""
+    return "{org}/{number}/{run}".format(**test_course_data)

cms/djangoapps/contentstore/views/course.py

@@ -105,17 +105,6 @@ def course_handler(request, tag=None, course_id=None, branch=None, version_guid=
         return HttpResponseNotFound()
 
 
-@login_required
-@ensure_csrf_cookie
-def old_course_index_shim(request, org, course, name):
-    """
-    A shim for any unconverted uses of course_index
-    """
-    old_location = Location(['i4x', org, course, 'course', name])
-    locator = loc_mapper().translate_location(old_location.course_id, old_location, False, True)
-    return course_index(request, locator.course_id, locator.branch, locator.version_guid, locator.usage_id)
-
-
 @login_required
 @ensure_csrf_cookie
 def course_index(request, course_id, branch, version_guid, block):
@@ -164,7 +153,9 @@ def course_index(request, course_id, branch, version_guid, block):
 @expect_json
 def create_new_course(request):
     """
-    Create a new course
+    Create a new course.
+
+    Returns the URL for the course overview page.
     """
     if not is_user_in_creator_group(request.user):
         raise PermissionDenied()
@@ -255,7 +246,8 @@ def create_new_course(request):
     # work.
     CourseEnrollment.enroll(request.user, new_course.location.course_id)
 
-    return JsonResponse({'id': new_course.location.url()})
+    new_location = loc_mapper().translate_location(new_course.location.course_id, new_course.location, False, True)
+    return JsonResponse({'url': new_location.url_reverse("course/", "")})
 
 
 @login_required

cms/djangoapps/contentstore/views/import_export.py

@@ -27,7 +27,7 @@ from auth.authz import create_all_course_groups
 from xmodule.modulestore.xml_importer import import_from_xml
 from xmodule.contentstore.django import contentstore
 from xmodule.modulestore.xml_exporter import export_to_xml
-from xmodule.modulestore.django import modulestore
+from xmodule.modulestore.django import modulestore, loc_mapper
 from xmodule.modulestore import Location
 from xmodule.exceptions import SerializationError
 
@@ -240,14 +240,10 @@ def import_course(request, org, course, name):
             return JsonResponse({'Status': 'OK'})
     else:
         course_module = modulestore().get_item(location)
-
+        new_location = loc_mapper().translate_location(course_module.location.course_id, course_module.location, False, True)
         return render_to_response('import.html', {
             'context_course': course_module,
-            'successful_import_redirect_url': reverse('course_index', kwargs={
+            'successful_import_redirect_url': new_location.url_reverse("course/", "")
-                'org': location.org,
-                'course': location.course,
-                'name': location.name,
-            })
         })
 
 
@@ -286,6 +282,8 @@ def generate_export_course(request, org, course, name):
     loc = Location(location)
     export_file = NamedTemporaryFile(prefix=name + '.', suffix=".tar.gz")
 
+    new_location = loc_mapper().translate_location(course_module.location.course_id, course_module.location, False, True)
+
     root_dir = path(mkdtemp())
 
     try:
@@ -317,11 +315,7 @@ def generate_export_course(request, org, course, name):
             'edit_unit_url': reverse('edit_unit', kwargs={
                 'location': parent.location
             }) if parent else '',
-            'course_home_url': reverse('course_index', kwargs={
+            'course_home_url': new_location.url_reverse("course/", "")
-                'org': org,
-                'course': course,
-                'name': name
-            })
         })
     except Exception, e:
         logging.exception('There was an error exporting course {0}. {1}'.format(course_module.location, unicode(e)))
@@ -331,11 +325,7 @@ def generate_export_course(request, org, course, name):
             'in_err': True,
             'unit': None,
             'raw_err_msg': str(e),
-            'course_home_url': reverse('course_index', kwargs={
+            'course_home_url': new_location.url_reverse("course/", "")
-                'org': org,
-                'course': course,
-                'name': name
-            })
         })
 
     logging.debug('tar file being generated at {0}'.format(export_file.name))

cms/static/js/index.js

@@ -39,8 +39,8 @@ require(["domReady", "jquery", "underscore", "js/utils/cancel_on_escape"],
                     'run': run
                 },
                 function (data) {
-                    if (data.id !== undefined) {
+                    if (data.url !== undefined) {
-                        window.location = '/' + data.id.replace(/.*:\/\//, '');
+                        window.location = data.url;
                     } else if (data.ErrMsg !== undefined) {
                         $('.wrap-error').addClass('is-shown');
                         $('#course_creation_error').html('<p>' + data.ErrMsg + '</p>');

cms/templates/unit.html

@@ -2,6 +2,7 @@
 <%!
 from django.core.urlresolvers import reverse
 from django.utils.translation import ugettext as _
+from xmodule.modulestore.django import loc_mapper
 %>
 <%namespace name="units" file="widgets/units.html" />
 <%block name="title">${_("Individual Unit")}</%block>
@@ -179,7 +180,11 @@ require(["domReady!", "jquery", "coffee/src/models/module", "coffee/src/views/un
             </div>
             <ol>
               <li>
-                <a href="${reverse('course_index', kwargs=dict(org=context_course.location.org, course=context_course.location.course, name=context_course.location.name))}" class="section-item">${section.display_name_with_default}</a>
+                <%
+                  ctx_loc = context_course.location
+                  index_url = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True).url_reverse('course/', '')
+                %>
+                <a href="${index_url}" class="section-item">${section.display_name_with_default}</a>
                 <ol>
                   <li>
                     <a href="${reverse('edit_subsection', args=[subsection.location])}" class="section-item">

cms/urls.py

@@ -135,13 +135,6 @@ urlpatterns += patterns(
 # restful api
 urlpatterns += patterns(
     'contentstore.views',
-    # index page, course outline page, and course structure json access
-    # replaces url(r'^listing', 'contentstore.views.index', name='index'),
-    # ? url(r'^create_new_course', 'contentstore.views.create_new_course', name='create_new_course')
-    # TODO remove shim and this pattern once import_export and test_contentstore no longer use
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<name>[^/]+)$',
-        'course.old_course_index_shim', name='course_index'
-    ),
 
     url(r'^course$', 'index'),
     # (?ix) == ignore case and verbose (multiline regex)