From 6a9074e1851fea87b0a5edd0b077659575c5574a Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Tue, 23 Jul 2013 10:17:16 -0400 Subject: [PATCH] Removed `get_url_reverse` function It was causing unit tests to fail, and it's a needless bit of abstraction that never should have existed in the first place. --- .../contentstore/tests/test_checklists.py | 8 +++- .../contentstore/tests/test_utils.py | 44 ------------------- cms/djangoapps/contentstore/utils.py | 32 -------------- cms/djangoapps/contentstore/views/assets.py | 7 ++- .../contentstore/views/checklist.py | 17 ++++++- cms/djangoapps/contentstore/views/user.py | 22 +++++++--- 6 files changed, 43 insertions(+), 87 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_checklists.py b/cms/djangoapps/contentstore/tests/test_checklists.py index 02999f6567..6f8f102df8 100644 --- a/cms/djangoapps/contentstore/tests/test_checklists.py +++ b/cms/djangoapps/contentstore/tests/test_checklists.py @@ -1,5 +1,5 @@ """ Unit tests for checklist methods in views.py. """ -from contentstore.utils import get_modulestore, get_url_reverse +from contentstore.utils import get_modulestore from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.tests.factories import CourseFactory from django.core.urlresolvers import reverse @@ -38,7 +38,11 @@ class ChecklistTestCase(CourseTestCase): def test_get_checklists(self): """ Tests the get checklists method. """ - checklists_url = get_url_reverse('Checklists', self.course) + 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) self.assertContains(response, "Getting Started With Studio") payload = response.content diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index fec82db1bb..26c49843b5 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/cms/djangoapps/contentstore/tests/test_utils.py @@ -72,50 +72,6 @@ class LMSLinksTestCase(TestCase): ) -class UrlReverseTestCase(ModuleStoreTestCase): - """ Tests for get_url_reverse """ - def test_course_page_names(self): - """ Test the defined course pages. """ - course = CourseFactory.create(org='mitX', number='666', display_name='URL Reverse Course') - - self.assertEquals( - '/manage_users/i4x://mitX/666/course/URL_Reverse_Course', - utils.get_url_reverse('ManageUsers', course) - ) - - self.assertEquals( - '/mitX/666/settings-details/URL_Reverse_Course', - utils.get_url_reverse('SettingsDetails', course) - ) - - self.assertEquals( - '/mitX/666/settings-grading/URL_Reverse_Course', - utils.get_url_reverse('SettingsGrading', course) - ) - - self.assertEquals( - '/mitX/666/course/URL_Reverse_Course', - utils.get_url_reverse('CourseOutline', course) - ) - - self.assertEquals( - '/mitX/666/checklists/URL_Reverse_Course', - utils.get_url_reverse('Checklists', course) - ) - - def test_unknown_passes_through(self): - """ Test that unknown values pass through. """ - course = CourseFactory.create(org='mitX', number='666', display_name='URL Reverse Course') - self.assertEquals( - 'foobar', - utils.get_url_reverse('foobar', course) - ) - self.assertEquals( - 'https://edge.edx.org/courses/edX/edX101/How_to_Create_an_edX_Course/about', - utils.get_url_reverse('https://edge.edx.org/courses/edX/edX101/How_to_Create_an_edX_Course/about', course) - ) - - class ExtraPanelTabTestCase(TestCase): """ Tests adding and removing extra course tabs. """ diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 4973bddaca..a2e927ef46 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -188,38 +188,6 @@ def update_item(location, value): get_modulestore(location).update_item(location, value) -def get_url_reverse(course_page_name, course_module): - """ - Returns the course URL link to the specified location. This value is suitable to use as an href link. - - course_page_name should correspond to an attribute in CoursePageNames (for example, 'ManageUsers' - or 'SettingsDetails'), or else it will simply be returned. This method passes back unknown values of - course_page_names so that it can also be used for absolute (known) URLs. - - course_module is used to obtain the location, org, course, and name properties for a course, if - course_page_name corresponds to an attribute in CoursePageNames. - """ - url_name = getattr(CoursePageNames, course_page_name, None) - ctx_loc = course_module.location - - if CoursePageNames.ManageUsers == url_name: - return reverse(url_name, kwargs={"location": ctx_loc}) - elif url_name in [CoursePageNames.SettingsDetails, CoursePageNames.SettingsGrading, - CoursePageNames.CourseOutline, CoursePageNames.Checklists]: - return reverse(url_name, kwargs={'org': ctx_loc.org, 'course': ctx_loc.course, 'name': ctx_loc.name}) - else: - return course_page_name - - -class CoursePageNames: - """ Constants for pages that are recognized by get_url_reverse method. """ - ManageUsers = "manage_users" - SettingsDetails = "settings_details" - SettingsGrading = "settings_grading" - CourseOutline = "course_index" - Checklists = "checklists" - - def add_extra_panel_tab(tab_type, course): """ Used to add the panel tab to a course if it does not exist. diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index 0bb9551ac9..6d371bef18 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -29,7 +29,6 @@ from xmodule.util.date_utils import get_default_time_display from xmodule.modulestore import InvalidLocationError from xmodule.exceptions import NotFoundError -from ..utils import get_url_reverse from .access import get_location_and_verify_access from util.json_request import JsonResponse @@ -320,7 +319,11 @@ def import_course(request, org, course, name): return render_to_response('import.html', { 'context_course': course_module, - 'successful_import_redirect_url': get_url_reverse('CourseOutline', course_module) + 'successful_import_redirect_url': reverse('course_index', kwargs={ + 'org': location.org, + 'course': location.course, + 'name': location.name, + }) }) diff --git a/cms/djangoapps/contentstore/views/checklist.py b/cms/djangoapps/contentstore/views/checklist.py index bcf4a1a5b9..17f0a55565 100644 --- a/cms/djangoapps/contentstore/views/checklist.py +++ b/cms/djangoapps/contentstore/views/checklist.py @@ -4,12 +4,13 @@ from util.json_request import JsonResponse from django.http import HttpResponseBadRequest from django.contrib.auth.decorators import login_required 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 xmodule.modulestore.inheritance import own_metadata -from ..utils import get_modulestore, get_url_reverse +from ..utils import get_modulestore from .access import get_location_and_verify_access from xmodule.course_module import CourseDescriptor @@ -96,10 +97,22 @@ def expand_checklist_action_urls(course_module): """ checklists = course_module.checklists modified = False + urlconf_map = { + "ManageUsers": "manage_users", + "SettingsDetails": "settings_details", + "SettingsGrading": "settings_grading", + "CourseOutline": "course_index", + "Checklists": "checklists", + } for checklist in checklists: if not checklist.get('action_urls_expanded', False): for item in checklist.get('items'): - item['action_url'] = get_url_reverse(item.get('action_url'), course_module) + urlconf_name = urlconf_map.get(item.get('action_url')) + item['action_url'] = reverse(urlconf_name, kwargs={ + 'org': course_module.location.org, + 'course': course_module.location.course, + 'name': course_module.location.name, + }) checklist['action_urls_expanded'] = True modified = True diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index ee4e4e435a..c5d642e207 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/cms/djangoapps/contentstore/views/user.py @@ -1,6 +1,7 @@ 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 @@ -11,7 +12,7 @@ from mitxmako.shortcuts import render_to_response from django.core.context_processors import csrf from xmodule.modulestore.django import modulestore -from contentstore.utils import get_url_reverse, get_lms_link_for_item +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_users_in_course_group_by_role, @@ -40,11 +41,22 @@ def index(request): and course.location.name != '') courses = filter(course_filter, courses) + def format_course_for_view(course): + return ( + course.display_name, + reverse("course_index", kwargs={ + 'org': course.location.org, + 'course': course.location.course, + 'name': course.location.name, + }), + get_lms_link_for_item( + course.location, + course_id=course.location.course_id, + ), + ) + return render_to_response('index.html', { - 'courses': [(course.display_name, - get_url_reverse('CourseOutline', course), - get_lms_link_for_item(course.location, course_id=course.location.course_id)) - for course in courses], + 'courses': [format_course_for_view(c) for c in courses], 'user': request.user, 'request_course_creator_url': reverse('request_course_creator'), 'course_creator_status': _get_course_creator_status(request.user),