Merge pull request #1434 from edx/dhm/bug_has_access

Use known translation if exists to get groupname

cms/djangoapps/auth/authz.py

@@ -10,6 +10,7 @@ from django.conf import settings
 
 from xmodule.modulestore import Location
 from xmodule.modulestore.locator import CourseLocator, Locator
+from xmodule.modulestore.django import loc_mapper
 
 
 # define a couple of simple roles, we just need ADMIN and EDITOR now for our purposes
@@ -35,7 +36,9 @@ def get_course_groupname_for_role(location, role):
     if isinstance(location, Location):
         groupnames.append('{0}_{1}'.format(role, location.course))
     elif isinstance(location, CourseLocator):
-        groupnames.append('{0}_{1}'.format(role, location.as_old_location_course_id))
+        old_location = loc_mapper().translate_locator_to_location(location)
+        if old_location:
+            groupnames.append('{0}_{1}'.format(role, old_location.course_id))
 
     for groupname in groupnames:
         if Group.objects.filter(name=groupname).exists():

cms/djangoapps/contentstore/tests/test_course_index.py

 """
 Unit tests for getting the list of courses and the course outline.
 """
-from django.core.urlresolvers import reverse
+import json
 import lxml
+from django.core.urlresolvers import reverse
 
 from contentstore.tests.utils import CourseTestCase
 from xmodule.modulestore.django import loc_mapper
-from django.core.exceptions import PermissionDenied
+from xmodule.modulestore.tests.factories import CourseFactory
+from xmodule.modulestore import parsers
 
 class TestCourseIndex(CourseTestCase):
     """
     Unit tests for getting the list of courses and the course outline.
     """
-    def test_index(self):
+    def setUp(self):
+        """
+        Add a course with odd characters in the fields
+        """
+        super(TestCourseIndex, self).setUp()
+        # had a problem where index showed course but has_access failed to retrieve it for non-staff
+        self.odd_course = CourseFactory.create(
+            org='test.org_1-2',
+            number='test-2.3_course',
+            display_name='dotted.course.name-2',
+        )
+
+
+    def check_index_and_outline(self, authed_client):
         """
         Test getting the list of courses and then pulling up their outlines
         """
         index_url = reverse('contentstore.views.index')
-        index_response = self.client.get(index_url, {}, HTTP_ACCEPT='text/html')
+        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')
+        self.assertGreaterEqual(len(course_link_eles), 2)
         for link in course_link_eles:
-            self.assertRegexpMatches(link.get("href"), r'course/\w+\.\w+\.\w+.*/branch/\w+/block/.*')
+            self.assertRegexpMatches(
+                link.get("href"),
+                r'course/{0}+/branch/{0}+/block/{0}+'.format(parsers.ALLOWED_ID_CHARS)
+            )
             # now test that url
-            outline_response = self.client.get(link.get("href"), {}, HTTP_ACCEPT='text/html')
+            outline_response = authed_client.get(link.get("href"), {}, HTTP_ACCEPT='text/html')
             # ensure it has the expected 2 self referential links
             outline_parsed = lxml.html.fromstring(outline_response.content)
             outline_link = outline_parsed.find_class('course-link')[0]
@@ -31,6 +50,12 @@ class TestCourseIndex(CourseTestCase):
             course_menu_link = outline_parsed.find_class('nav-course-courseware-outline')[0]
             self.assertEqual(course_menu_link.find("a").get("href"), link.get("href"))
 
+    def test_is_staff_access(self):
+        """
+        Test that people with is_staff see the courses and can navigate into them
+        """
+        self.check_index_and_outline(self.client)
+
     def test_negative_conditions(self):
         """
         Test the error conditions for the access
@@ -38,6 +63,28 @@ class TestCourseIndex(CourseTestCase):
         locator = loc_mapper().translate_location(self.course.location.course_id, self.course.location, False, True)
         outline_url = reverse('contentstore.views.course_handler', kwargs={'course_url': unicode(locator)})
         # register a non-staff member and try to delete the course branch
-        non_staff_client = self.createNonStaffAuthedUserClient()
+        non_staff_client, _ = self.createNonStaffAuthedUserClient()
         response = non_staff_client.delete(outline_url, {}, HTTP_ACCEPT='application/json')
         self.assertEqual(response.status_code, 403)
+
+    def test_course_staff_access(self):
+        """
+        Make and register an course_staff and ensure they can access the courses
+        """
+        course_staff_client, course_staff = self.createNonStaffAuthedUserClient()
+        for course in [self.course, self.odd_course]:
+            permission_url = reverse("course_team_user", kwargs={
+                "org": course.location.org,
+                "course": course.location.course,
+                "name": course.location.name,
+                "email": course_staff.email,
+            })
+            self.client.post(
+                permission_url,
+                data=json.dumps({"role": "staff"}),
+                content_type="application/json",
+                HTTP_ACCEPT="application/json",
+            )
+
+        # test access
+        self.check_index_and_outline(course_staff_client)

cms/djangoapps/contentstore/tests/utils.py

@@ -78,4 +78,4 @@ class CourseTestCase(ModuleStoreTestCase):
 
         client = Client()
         client.login(username=uname, password=password)
-        return client
+        return client, nonstaff

cms/djangoapps/contentstore/views/course.py

@@ -131,9 +131,9 @@ def course_index(request, course_url):
     lms_link = get_lms_link_for_item(old_location)
 
     upload_asset_callback_url = reverse('upload_asset', kwargs={
-        'org': location.as_old_location_org,
-        'course': location.as_old_location_course,
-        'coursename': location.as_old_location_run
+        'org': old_location.org,
+        'course': old_location.course,
+        'coursename': old_location.name
     })
 
     course = modulestore().get_item(old_location, depth=3)

common/lib/xmodule/xmodule/modulestore/locator.py

@@ -263,56 +263,6 @@ class CourseLocator(Locator):
                              version_guid=self.version_guid,
                              branch=self.branch)
 
-    OLD_COURSE_ID_RE = re.compile(r'^(?P<org>[^.]+)\.?(?P<old_course_id>.+)?\.(?P<run>[^.]+)$')
-    @property
-    def as_old_location_course_id(self):
-        """
-        The original Location type presented its course id as org/course/run. This function
-        assumes the course_id starts w/ org, has an arbitrarily long 'course' identifier, and then
-        ends w/ run all separated by periods.
-
-        If this object does not have a course_id, this function returns None.
-        """
-        if self.course_id is None:
-            return None
-        parsed = self.OLD_COURSE_ID_RE.match(self.course_id)
-        # check whether there are 2 or > 2 'fields'
-        if parsed.group('old_course_id'):
-            return '/'.join(parsed.groups())
-        else:
-            return parsed.group('org') + '/' + parsed.group('run')
-
-    def _old_location_field_helper(self, field):
-        """
-        Parse course_id to get the old location field named field out
-        """
-        if self.course_id is None:
-            return None
-        parsed = self.OLD_COURSE_ID_RE.match(self.course_id)
-        return parsed.group(field)
-
-    @property
-    def as_old_location_org(self):
-        """
-        Presume the first part of the course_id is the org and return it.
-        """
-        return self._old_location_field_helper('org')
-
-    @property
-    def as_old_location_course(self):
-        """
-        Presume the middle part, if any, of the course_id is the old location scheme's
-        course id and return it.
-        """
-        return self._old_location_field_helper('old_course_id')
-
-    @property
-    def as_old_location_run(self):
-        """
-        Presume the last part of the course_id is the old location scheme's run and return it.
-        """
-        return self._old_location_field_helper('run')
-
     def init_from_url(self, url):
         """
         url must be a string beginning with 'edx://' and containing

common/lib/xmodule/xmodule/modulestore/tests/test_locators.py

@@ -283,21 +283,6 @@ class LocatorTest(TestCase):
             Locator.to_locator_or_location("hello.world.not.a.url")
         self.assertIsNone(Locator.parse_url("unknown://foo.bar/baz"))
 
-    def test_as_old(self):
-        """
-        Test the as_old_location_xxx accessors
-        """
-        locator = CourseLocator(course_id='org.course.id.run', branch='mybranch')
-        self.assertEqual('org', locator.as_old_location_org)
-        self.assertEqual('course.id', locator.as_old_location_course)
-        self.assertEqual('run', locator.as_old_location_run)
-        self.assertEqual('org/course.id/run', locator.as_old_location_course_id)
-        locator = CourseLocator(course_id='org.course', branch='mybranch')
-        self.assertEqual('org', locator.as_old_location_org)
-        self.assertIsNone(locator.as_old_location_course)
-        self.assertEqual('course', locator.as_old_location_run)
-        self.assertEqual('org/course', locator.as_old_location_course_id)
-
     def test_description_locator_url(self):
         object_id = '{:024x}'.format(random.randrange(16 ** 24))
         definition_locator = DefinitionLocator(object_id)