Merge pull request #4353 from edx/flowerhack/lms-deprecated-urls

Enables LMS to handle deprecated URLs

cms/djangoapps/contentstore/views/tests/test_course_index.py

@@ -4,11 +4,11 @@ Unit tests for getting the list of courses and the course outline.
 import json
 import lxml
 
-from cms.urls import COURSE_KEY_PATTERN
 from contentstore.tests.utils import CourseTestCase
 from contentstore.utils import reverse_course_url
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from opaque_keys.edx.locator import Locator
+from django.conf import settings
 
 
 class TestCourseIndex(CourseTestCase):
@@ -39,7 +39,7 @@ class TestCourseIndex(CourseTestCase):
         for link in course_link_eles:
             self.assertRegexpMatches(
                 link.get("href"),
-                'course/{}'.format(COURSE_KEY_PATTERN)
+                'course/{}'.format(settings.COURSE_KEY_PATTERN)
             )
             # now test that url
             outline_response = authed_client.get(link.get("href"), {}, HTTP_ACCEPT='text/html')

cms/envs/common.py

@@ -168,6 +168,12 @@ AUTHENTICATION_BACKENDS = (
 
 LMS_BASE = None
 
+# These are standard regexes for pulling out info like course_ids, usage_ids, etc.
+# They are used so that URLs with deprecated-format strings still work.
+from lms.envs.common import (
+    COURSE_KEY_PATTERN, COURSE_ID_PATTERN, USAGE_KEY_PATTERN, ASSET_KEY_PATTERN
+)
+
 #################### CAPA External Code Evaluation #############################
 XQUEUE_INTERFACE = {
     'url': 'http://localhost:8888',

cms/urls.py

@@ -5,10 +5,6 @@ from django.conf.urls import patterns, include, url
 from ratelimitbackend import admin
 admin.autodiscover()
 
-COURSE_KEY_PATTERN = r'(?P<course_key_string>(?:[^/]+/[^/]+/[^/]+)|(?:[^/]+))'
-USAGE_KEY_PATTERN = r'(?P<usage_key_string>(?:i4x://?[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|(?:[^/]+))'
-ASSET_KEY_PATTERN = r'(?P<asset_key_string>(?:/?c4x(:/)?/[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|(?:[^/]+))'
-
 urlpatterns = patterns('',  # nopep8
 
     url(r'^transcripts/upload$', 'contentstore.views.upload_transcripts', name='upload_transcripts'),
@@ -70,30 +66,30 @@ urlpatterns += patterns(
     url(r'^signin$', 'login_page', name='login'),
     url(r'^request_course_creator$', 'request_course_creator'),
 
-    url(r'^course_team/{}/(?P<email>.+)?$'.format(COURSE_KEY_PATTERN), 'course_team_handler'),
-    url(r'^course_info/{}$'.format(COURSE_KEY_PATTERN), 'course_info_handler'),
+    url(r'^course_team/{}/(?P<email>.+)?$'.format(settings.COURSE_KEY_PATTERN), 'course_team_handler'),
+    url(r'^course_info/{}$'.format(settings.COURSE_KEY_PATTERN), 'course_info_handler'),
     url(
-        r'^course_info_update/{}/(?P<provided_id>\d+)?$'.format(COURSE_KEY_PATTERN),
+        r'^course_info_update/{}/(?P<provided_id>\d+)?$'.format(settings.COURSE_KEY_PATTERN),
         'course_info_update_handler'
     ),
-    url(r'^course/{}?$'.format(COURSE_KEY_PATTERN), 'course_handler', name='course_handler'),
-    url(r'^subsection/{}$'.format(USAGE_KEY_PATTERN), 'subsection_handler'),
-    url(r'^unit/{}$'.format(USAGE_KEY_PATTERN), 'unit_handler'),
-    url(r'^container/{}$'.format(USAGE_KEY_PATTERN), 'container_handler'),
-    url(r'^checklists/{}/(?P<checklist_index>\d+)?$'.format(COURSE_KEY_PATTERN), 'checklists_handler'),
-    url(r'^orphan/{}$'.format(COURSE_KEY_PATTERN), 'orphan_handler'),
-    url(r'^assets/{}/{}?$'.format(COURSE_KEY_PATTERN, ASSET_KEY_PATTERN), 'assets_handler'),
-    url(r'^import/{}$'.format(COURSE_KEY_PATTERN), 'import_handler'),
-    url(r'^import_status/{}/(?P<filename>.+)$'.format(COURSE_KEY_PATTERN), 'import_status_handler'),
-    url(r'^export/{}$'.format(COURSE_KEY_PATTERN), 'export_handler'),
-    url(r'^xblock/{}/(?P<view_name>[^/]+)$'.format(USAGE_KEY_PATTERN), 'xblock_view_handler'),
-    url(r'^xblock/{}?$'.format(USAGE_KEY_PATTERN), 'xblock_handler'),
-    url(r'^tabs/{}$'.format(COURSE_KEY_PATTERN), 'tabs_handler'),
-    url(r'^settings/details/{}$'.format(COURSE_KEY_PATTERN), 'settings_handler'),
-    url(r'^settings/grading/{}(/)?(?P<grader_index>\d+)?$'.format(COURSE_KEY_PATTERN), 'grading_handler'),
-    url(r'^settings/advanced/{}$'.format(COURSE_KEY_PATTERN), 'advanced_settings_handler'),
-    url(r'^textbooks/{}$'.format(COURSE_KEY_PATTERN), 'textbooks_list_handler'),
-    url(r'^textbooks/{}/(?P<textbook_id>\d[^/]*)$'.format(COURSE_KEY_PATTERN), 'textbooks_detail_handler'),
+    url(r'^course/{}?$'.format(settings.COURSE_KEY_PATTERN), 'course_handler', name='course_handler'),
+    url(r'^subsection/{}$'.format(settings.USAGE_KEY_PATTERN), 'subsection_handler'),
+    url(r'^unit/{}$'.format(settings.USAGE_KEY_PATTERN), 'unit_handler'),
+    url(r'^container/{}$'.format(settings.USAGE_KEY_PATTERN), 'container_handler'),
+    url(r'^checklists/{}/(?P<checklist_index>\d+)?$'.format(settings.COURSE_KEY_PATTERN), 'checklists_handler'),
+    url(r'^orphan/{}$'.format(settings.COURSE_KEY_PATTERN), 'orphan_handler'),
+    url(r'^assets/{}/{}?$'.format(settings.COURSE_KEY_PATTERN, settings.ASSET_KEY_PATTERN), 'assets_handler'),
+    url(r'^import/{}$'.format(settings.COURSE_KEY_PATTERN), 'import_handler'),
+    url(r'^import_status/{}/(?P<filename>.+)$'.format(settings.COURSE_KEY_PATTERN), 'import_status_handler'),
+    url(r'^export/{}$'.format(settings.COURSE_KEY_PATTERN), 'export_handler'),
+    url(r'^xblock/{}/(?P<view_name>[^/]+)$'.format(settings.USAGE_KEY_PATTERN), 'xblock_view_handler'),
+    url(r'^xblock/{}?$'.format(settings.USAGE_KEY_PATTERN), 'xblock_handler'),
+    url(r'^tabs/{}$'.format(settings.COURSE_KEY_PATTERN), 'tabs_handler'),
+    url(r'^settings/details/{}$'.format(settings.COURSE_KEY_PATTERN), 'settings_handler'),
+    url(r'^settings/grading/{}(/)?(?P<grader_index>\d+)?$'.format(settings.COURSE_KEY_PATTERN), 'grading_handler'),
+    url(r'^settings/advanced/{}$'.format(settings.COURSE_KEY_PATTERN), 'advanced_settings_handler'),
+    url(r'^textbooks/{}$'.format(settings.COURSE_KEY_PATTERN), 'textbooks_list_handler'),
+    url(r'^textbooks/{}/(?P<textbook_id>\d[^/]*)$'.format(settings.COURSE_KEY_PATTERN), 'textbooks_detail_handler'),
 )
 
 if settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS'):
@@ -113,7 +109,7 @@ urlpatterns += patterns('',
 
 
 if settings.FEATURES.get('ENABLE_EXPORT_GIT'):
-    urlpatterns += (url(r'^export_git/{}$'.format(COURSE_KEY_PATTERN),
+    urlpatterns += (url(r'^export_git/{}$'.format(settings.COURSE_KEY_PATTERN),
                         'contentstore.views.export_git', name='export_git'),)
 
 if settings.FEATURES.get('ENABLE_SERVICE_STATUS'):

common/djangoapps/course_modes/urls.py

 from django.conf.urls import include, patterns, url
+from django.conf import settings
+
 from django.views.generic import TemplateView
 
 from course_modes import views
 
 urlpatterns = patterns(
     '',
-    url(r'^choose/(?P<course_id>[^/]+/[^/]+/[^/]+)/$', views.ChooseModeView.as_view(), name="course_modes_choose"),
+    # pylint seems to dislike as_view() calls because it's a `classonlymethod` instead of `classmethod`, so we disable the warning
+    url(r'^choose/{}/$'.format(settings.COURSE_ID_PATTERN), views.ChooseModeView.as_view(), name="course_modes_choose"),  # pylint: disable=no-value-for-parameter
 )

common/djangoapps/student/views.py

@@ -87,6 +87,7 @@ from util.password_policy_validators import (
 from third_party_auth import pipeline, provider
 from xmodule.error_module import ErrorDescriptor
 
+
 log = logging.getLogger("edx.student")
 AUDIT_LOG = logging.getLogger("audit")
 
@@ -657,13 +658,16 @@ def change_enrollment(request):
         return HttpResponseBadRequest(_("Enrollment action is invalid"))
 
 
+# TODO: This function is kind of gnarly/hackish/etc and is only used in one location.
+# It'd be awesome if we could get rid of it; manually parsing course_id strings form larger strings
+# seems Probably Incorrect
 def _parse_course_id_from_string(input_str):
     """
     Helper function to determine if input_str (typically the queryparam 'next') contains a course_id.
     @param input_str:
     @return: the course_id if found, None if not
     """
-    m_obj = re.match(r'^/courses/(?P<course_id>[^/]+/[^/]+/[^/]+)', input_str)
+    m_obj = re.match(r'^/courses/{}'.format(settings.COURSE_ID_PATTERN), input_str)
     if m_obj:
         return SlashSeparatedCourseKey.from_deprecated_string(m_obj.group('course_id'))
     return None

common/djangoapps/util/request.py

@@ -5,7 +5,8 @@ from django.conf import settings
 from microsite_configuration import microsite
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 
-COURSE_REGEX = re.compile(r'^.*?/courses/(?P<course_id>[^/]+/[^/]+/[^/]+)')
+
+COURSE_REGEX = re.compile(r'^.*?/courses/{}'.format(settings.COURSE_ID_PATTERN))
 
 
 def safe_get_host(request):

lms/djangoapps/class_dashboard/urls.py

@@ -3,17 +3,19 @@ Class Dashboard API endpoint urls.
 """
 
 from django.conf.urls import patterns, url
+from django.conf import settings
+COURSE_ID_PATTERN = settings.COURSE_ID_PATTERN
 
 urlpatterns = patterns('',  # nopep8
     # Json request data for metrics for entire course
-    url(r'^(?P<course_id>[^/]+/[^/]+/[^/]+)/all_sequential_open_distrib$',
+    url(r'^{}/all_sequential_open_distrib$'.format(settings.COURSE_ID_PATTERN),
         'class_dashboard.views.all_sequential_open_distrib', name="all_sequential_open_distrib"),
 
-    url(r'^(?P<course_id>[^/]+/[^/]+/[^/]+)/all_problem_grade_distribution$',
+    url(r'^{}/all_problem_grade_distribution$'.format(settings.COURSE_ID_PATTERN),
         'class_dashboard.views.all_problem_grade_distribution', name="all_problem_grade_distribution"),
 
     # Json request data for metrics for particular section
-    url(r'^(?P<course_id>[^/]+/[^/]+/[^/]+)/problem_grade_distribution/(?P<section>\d+)$',
+    url(r'^{}/problem_grade_distribution/(?P<section>\d+)$'.format(settings.COURSE_ID_PATTERN),
         'class_dashboard.views.section_problem_grade_distrib', name="section_problem_grade_distrib"),
 
     # For listing students that opened a sub-section

lms/djangoapps/instructor/views/legacy.py

@@ -1033,7 +1033,7 @@ def instructor_dashboard(request, course_id):
         'course_errors': modulestore().get_course_errors(course.id),
         'instructor_tasks': instructor_tasks,
         'offline_grade_log': offline_grades_available(course_key),
-        'cohorts_ajax_url': reverse('cohorts', kwargs={'course_key': course_key.to_deprecated_string()}),
+        'cohorts_ajax_url': reverse('cohorts', kwargs={'course_key_string': course_key.to_deprecated_string()}),
 
         'analytics_results': analytics_results,
         'disable_buttons': disable_buttons,

lms/djangoapps/shoppingcart/urls.py

@@ -13,7 +13,7 @@ if settings.FEATURES['ENABLE_SHOPPING_CART']:
         url(r'^$', 'show_cart'),
         url(r'^clear/$', 'clear_cart'),
         url(r'^remove_item/$', 'remove_item'),
-        url(r'^add/course/(?P<course_id>[^/]+/[^/]+/[^/]+)/$', 'add_course_to_cart', name='add_course_to_cart'),
+        url(r'^add/course/{}/$'.format(settings.COURSE_ID_PATTERN), 'add_course_to_cart', name='add_course_to_cart'),
     )
 
 if settings.FEATURES.get('ENABLE_PAYMENT_FAKE'):

lms/djangoapps/verify_student/urls.py

@@ -2,22 +2,28 @@ from django.conf.urls import patterns, url
 
 from verify_student import views
 
+from django.conf import settings
+
+
 urlpatterns = patterns(
     '',
     url(
-        r'^show_requirements/(?P<course_id>[^/]+/[^/]+/[^/]+)/$',
+        r'^show_requirements/{}/$'.format(settings.COURSE_ID_PATTERN),
         views.show_requirements,
         name="verify_student_show_requirements"
     ),
 
+    # pylint sometimes seems to dislike the as_view() function because as_view() is
+    # decorated with `classonlymethod` instead of `classmethod`.  It's inconsistent
+    # about *which* as_view() calls it grumbles about, but we disable those warnings
     url(
-        r'^verify/(?P<course_id>[^/]+/[^/]+/[^/]+)/$',
-        views.VerifyView.as_view(),  # pylint: disable=E1120
+        r'^verify/{}/$'.format(settings.COURSE_ID_PATTERN),
+        views.VerifyView.as_view(),  # pylint: disable=no-value-for-parameter
         name="verify_student_verify"
     ),
 
     url(
-        r'^verified/(?P<course_id>[^/]+/[^/]+/[^/]+)/$',
+        r'^verified/{}/$'.format(settings.COURSE_ID_PATTERN),
         views.VerifiedView.as_view(),
         name="verify_student_verified"
     ),
@@ -41,8 +47,8 @@ urlpatterns = patterns(
     ),
 
     url(
-        r'^midcourse_reverify/(?P<course_id>[^/]+/[^/]+/[^/]+)/$',
-        views.MidCourseReverifyView.as_view(),  # pylint: disable=E1120
+        r'^midcourse_reverify/{}/$'.format(settings.COURSE_ID_PATTERN),
+        views.MidCourseReverifyView.as_view(),  # pylint: disable=no-value-for-parameter
         name="verify_student_midcourse_reverify"
     ),
 

lms/envs/common.py

@@ -399,6 +399,14 @@ COURSE_SETTINGS = {
 # TODO (vshnayder): Will probably need to change as we get real access control in.
 LMS_MIGRATION_ALLOWED_IPS = []
 
+# These are standard regexes for pulling out info like course_ids, usage_ids, etc.
+# They are used so that URLs with deprecated-format strings still work.
+COURSE_ID_PATTERN = r'(?P<course_id>(?:[^/]+/[^/]+/[^/]+)|(?:[^/]+))'
+COURSE_KEY_PATTERN = r'(?P<course_key_string>(?:[^/]+/[^/]+/[^/]+)|(?:[^/]+))'
+USAGE_KEY_PATTERN = r'(?P<usage_key_string>(?:i4x://?[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|(?:[^/]+))'
+ASSET_KEY_PATTERN = r'(?P<asset_key_string>(?:/?c4x(:/)?/[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|(?:[^/]+))'
+USAGE_ID_PATTERN = r'(?P<usage_id>(?:i4x://?[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|(?:[^/]+))'
+
 
 ############################## EVENT TRACKING #################################