From 12488db01582648d8225ebc52f9b6ed890a1c1b9 Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 23 Oct 2013 14:51:10 -0400 Subject: [PATCH] Get rid of course_index shim. --- .../contentstore/features/grading.py | 10 ++- .../contentstore/features/signup.feature | 6 +- .../contentstore/tests/test_contentstore.py | 83 +++++++++---------- cms/djangoapps/contentstore/views/course.py | 18 ++-- .../contentstore/views/import_export.py | 24 ++---- cms/static/js/index.js | 4 +- cms/templates/unit.html | 7 +- cms/urls.py | 7 -- 8 files changed, 71 insertions(+), 88 deletions(-) diff --git a/cms/djangoapps/contentstore/features/grading.py b/cms/djangoapps/contentstore/features/grading.py index 9f1aa6b269..15897a06bb 100644 --- a/cms/djangoapps/contentstore/features/grading.py +++ b/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, - world.scenario_dict['COURSE'].number, - world.scenario_dict['COURSE'].display_name.replace(' ', '_'),) + course_name = world.scenario_dict['COURSE'].display_name.replace(' ', '_') + main_page_link = '/course/{org}.{number}.{name}/branch/draft/block/{name}'.format( + 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')) diff --git a/cms/djangoapps/contentstore/features/signup.feature b/cms/djangoapps/contentstore/features/signup.feature index f1f2c13583..6b3f095530 100644 --- a/cms/djangoapps/contentstore/features/signup.feature +++ b/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 should see that the path is "/signin?next=/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=/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 diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 94bf4a8f4d..281ded2f6a 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/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) - self.assertEqual(resp.status_code, 200) - data = parse_json(resp) - self.assertEqual(data['id'], 'i4x://MITx/999/course/2013_Spring') + _create_course(self, course_data) 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 - - 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') + _create_course(self, course_data) # 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) - 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'])) + _create_course(self, test_course_data) # 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 = self._get_course_id(test_course_data) + course_id = _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) @@ -1530,13 +1515,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 = { - 'org': 'MITx', - 'course': '999', - 'name': Location.clean('Robot Super Course'), - } - - resp = self.client.get(reverse('course_index', kwargs=data)) + loc = Location(['i4x', 'MITx', '999', 'course', Location.clean('Robot Super Course'), None]) + resp = self._show_course_overview(loc) self.assertContains( resp, '
', @@ -1591,11 +1571,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', - kwargs={'org': loc.org, - 'course': loc.course, - 'name': loc.name})) - + resp = self._show_course_overview(loc) self.assertEqual(resp.status_code, 200) self.assertContains(resp, 'Chapter 2') @@ -1699,10 +1675,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) - self.assertEqual(resp.status_code, 200) - data = parse_json(resp) - self.assertEqual(data['id'], target_location.url()) + _create_course(self, course_data) import_from_xml(module_store, 'common/test/data/', ['toy'], target_location_namespace=target_location) @@ -1870,6 +1843,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): @@ -1933,3 +1913,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) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index c088e97d29..3b5c09c057 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/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 diff --git a/cms/djangoapps/contentstore/views/import_export.py b/cms/djangoapps/contentstore/views/import_export.py index 6252da0d0e..f16bc374dc 100644 --- a/cms/djangoapps/contentstore/views/import_export.py +++ b/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={ - 'org': location.org, - 'course': location.course, - 'name': location.name, - }) + 'successful_import_redirect_url': new_location.url_reverse("course/", "") }) @@ -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={ - 'org': org, - 'course': course, - 'name': name - }) + 'course_home_url': new_location.url_reverse("course/", "") }) 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={ - 'org': org, - 'course': course, - 'name': name - }) + 'course_home_url': new_location.url_reverse("course/", "") }) logging.debug('tar file being generated at {0}'.format(export_file.name)) diff --git a/cms/static/js/index.js b/cms/static/js/index.js index f2688f3dc0..46b703a09b 100644 --- a/cms/static/js/index.js +++ b/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) { - window.location = '/' + data.id.replace(/.*:\/\//, ''); + if (data.url !== undefined) { + window.location = data.url; } else if (data.ErrMsg !== undefined) { $('.wrap-error').addClass('is-shown'); $('#course_creation_error').html('

' + data.ErrMsg + '

'); diff --git a/cms/templates/unit.html b/cms/templates/unit.html index bf4d9363f6..fcef40a341 100644 --- a/cms/templates/unit.html +++ b/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")} @@ -179,7 +180,11 @@ require(["domReady!", "jquery", "coffee/src/models/module", "coffee/src/views/un
  1. - ${section.display_name_with_default} + <% + ctx_loc = context_course.location + index_url = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True).url_reverse('course/', '') + %> + ${section.display_name_with_default}
    1. diff --git a/cms/urls.py b/cms/urls.py index 1eaa939c32..9bbf1664f8 100644 --- a/cms/urls.py +++ b/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[^/]+)/(?P[^/]+)/course/(?P[^/]+)$', - 'course.old_course_index_shim', name='course_index' - ), url(r'^course$', 'index'), # (?ix) == ignore case and verbose (multiline regex)