get courses by reversing django groups for non global users (studio dashboard)

STUD-1133

cms/djangoapps/contentstore/management/commands/map_courses_location_lower.py

@@ -0,0 +1,22 @@
+"""
+Script for traversing all courses and add/modify mapping with 'lower_id' and 'lower_course_id'
+"""
+from django.core.management.base import BaseCommand
+from xmodule.modulestore.django import modulestore, loc_mapper
+
+
+#
+# To run from command line: ./manage.py cms --settings dev map_courses_location_lower
+#
+class Command(BaseCommand):
+    """
+    Create or modify map entry for each course in 'loc_mapper' with 'lower_id' and 'lower_course_id'
+    """
+    help = "Create or modify map entry for each course in 'loc_mapper' with 'lower_id' and 'lower_course_id'"
+
+    def handle(self, *args, **options):
+        # get all courses
+        courses = modulestore('direct').get_courses()
+        for course in courses:
+            # create/modify map_entry in 'loc_mapper' with 'lower_id' and 'lower_course_id'
+            loc_mapper().create_map_entry(course.location)

cms/djangoapps/contentstore/tests/test_course_listing.py

@@ -0,0 +1,196 @@
+"""
+Unit tests for getting the list of courses for a user through iterating all courses and
+by reversing group name formats.
+"""
+import random
+from chrono import Timer
+
+from django.contrib.auth.models import Group
+from django.test import RequestFactory
+
+from contentstore.views.course import _accessible_courses_list, _accessible_courses_list_from_groups
+from contentstore.tests.utils import AjaxEnabledTestClient
+from student.tests.factories import UserFactory
+from student.roles import CourseInstructorRole, CourseStaffRole
+from xmodule.modulestore import Location
+from xmodule.modulestore.django import loc_mapper
+from xmodule.modulestore.exceptions import ItemNotFoundError
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory
+
+TOTAL_COURSES_COUNT = 1000
+USER_COURSES_COUNT = 50
+
+
+class TestCourseListing(ModuleStoreTestCase):
+    """
+    Unit tests for getting the list of courses for a logged in user
+    """
+    def setUp(self):
+        """
+        Add a user and a course
+        """
+        super(TestCourseListing, self).setUp()
+        # create and log in a staff user.
+        self.user = UserFactory(is_staff=True)  # pylint: disable=no-member
+        self.factory = RequestFactory()
+        self.client = AjaxEnabledTestClient()
+        self.client.login(username=self.user.username, password='test')
+
+    def _create_course_with_access_groups(self, course_location, group_name_format='group_name_with_dots', user=None):
+        """
+        Create dummy course with 'CourseFactory' and role (instructor/staff) groups with provided group_name_format
+        """
+        course_locator = loc_mapper().translate_location(
+            course_location.course_id, course_location, False, True
+        )
+        course = CourseFactory.create(
+            org=course_location.org,
+            number=course_location.course,
+            display_name=course_location.name
+        )
+
+        for role in [CourseInstructorRole, CourseStaffRole]:
+            # pylint: disable=protected-access
+            groupnames = role(course_locator)._group_names
+            if group_name_format == 'group_name_with_course_name_only':
+                # Create role (instructor/staff) groups with course_name only: 'instructor_run'
+                group, _ = Group.objects.get_or_create(name=groupnames[2])
+            elif group_name_format == 'group_name_with_slashes':
+                # Create role (instructor/staff) groups with format: 'instructor_edX/Course/Run'
+                # Since "Group.objects.get_or_create(name=groupnames[1])" would have made group with lowercase name
+                # so manually create group name of old type
+                if role == CourseInstructorRole:
+                    group, _ = Group.objects.get_or_create(name=u'{}_{}'.format('instructor', course_location.course_id))
+                else:
+                    group, _ = Group.objects.get_or_create(name=u'{}_{}'.format('staff', course_location.course_id))
+            else:
+                # Create role (instructor/staff) groups with format: 'instructor_edx.course.run'
+                group, _ = Group.objects.get_or_create(name=groupnames[0])
+
+            if user is not None:
+                user.groups.add(group)
+        return course
+
+    def tearDown(self):
+        """
+        Reverse the setup
+        """
+        self.client.logout()
+        ModuleStoreTestCase.tearDown(self)
+
+    def test_get_course_list(self):
+        """
+        Test getting courses with new access group format e.g. 'instructor_edx.course.run'
+        """
+        request = self.factory.get('/course')
+        request.user = self.user
+
+        course_location = Location(['i4x', 'Org1', 'Course1', 'course', 'Run1'])
+        self._create_course_with_access_groups(course_location, 'group_name_with_dots', self.user)
+
+        # get courses through iterating all courses
+        courses_list = _accessible_courses_list(request)
+        self.assertEqual(len(courses_list), 1)
+
+        # get courses by reversing group name formats
+        courses_list_by_groups = _accessible_courses_list_from_groups(request)
+        self.assertEqual(len(courses_list_by_groups), 1)
+        # check both course lists have same courses
+        self.assertEqual(courses_list, courses_list_by_groups)
+
+    def test_get_course_list_with_old_group_formats(self):
+        """
+        Test getting all courses with old course role (instructor/staff) groups
+        """
+        request = self.factory.get('/course')
+        request.user = self.user
+
+        # create a course with new groups name format e.g. 'instructor_edx.course.run'
+        course_location = Location(['i4x', 'Org_1', 'Course_1', 'course', 'Run_1'])
+        self._create_course_with_access_groups(course_location, 'group_name_with_dots', self.user)
+
+        # create a course with old groups name format e.g. 'instructor_edX/Course/Run'
+        old_course_location = Location(['i4x', 'Org_2', 'Course_2', 'course', 'Run_2'])
+        self._create_course_with_access_groups(old_course_location, 'group_name_with_slashes', self.user)
+
+        # get courses through iterating all courses
+        courses_list = _accessible_courses_list(request)
+        self.assertEqual(len(courses_list), 2)
+
+        # get courses by reversing groups name
+        courses_list_by_groups = _accessible_courses_list_from_groups(request)
+        self.assertEqual(len(courses_list_by_groups), 2)
+
+        # create a new course with older group name format (with dots in names) e.g. 'instructor_edX/Course.name/Run.1'
+        old_course_location = Location(['i4x', 'Org.Foo.Bar', 'Course.number', 'course', 'Run.name'])
+        self._create_course_with_access_groups(old_course_location, 'group_name_with_slashes', self.user)
+        # get courses through iterating all courses
+        courses_list = _accessible_courses_list(request)
+        self.assertEqual(len(courses_list), 3)
+        # get courses by reversing group name formats
+        courses_list_by_groups = _accessible_courses_list_from_groups(request)
+        self.assertEqual(len(courses_list_by_groups), 3)
+
+        # create a new course with older group name format e.g. 'instructor_Run'
+        old_course_location = Location(['i4x', 'Org_3', 'Course_3', 'course', 'Run_3'])
+        self._create_course_with_access_groups(old_course_location, 'group_name_with_course_name_only', self.user)
+
+        # get courses through iterating all courses
+        courses_list = _accessible_courses_list(request)
+        self.assertEqual(len(courses_list), 4)
+
+        # should raise an exception for getting courses with older format of access group by reversing django groups
+        with self.assertRaises(ItemNotFoundError):
+            courses_list_by_groups = _accessible_courses_list_from_groups(request)
+
+    def test_course_listing_performance(self):
+        """
+        Create large number of courses and give access of some of these courses to the user and
+        compare the time to fetch accessible courses for the user through traversing all courses and
+        reversing django groups
+        """
+        # create and log in a non-staff user
+        self.user = UserFactory()
+        request = self.factory.get('/course')
+        request.user = self.user
+        self.client.login(username=self.user.username, password='test')
+
+        # create list of random course numbers which will be accessible to the user
+        user_course_ids = random.sample(range(TOTAL_COURSES_COUNT), USER_COURSES_COUNT)
+
+        # create courses and assign those to the user which have their number in user_course_ids
+        for number in range(1, TOTAL_COURSES_COUNT):
+            org = 'Org{0}'.format(number)
+            course = 'Course{0}'.format(number)
+            run = 'Run{0}'.format(number)
+            course_location = Location(['i4x', org, course, 'course', run])
+            if number in user_course_ids:
+                self._create_course_with_access_groups(course_location, 'group_name_with_dots', self.user)
+            else:
+                self._create_course_with_access_groups(course_location, 'group_name_with_dots')
+
+        # time the get courses by iterating through all courses
+        with Timer() as iteration_over_courses_time_1:
+            courses_list = _accessible_courses_list(request)
+        self.assertEqual(len(courses_list), USER_COURSES_COUNT)
+
+        # time again the get courses by iterating through all courses
+        with Timer() as iteration_over_courses_time_2:
+            courses_list = _accessible_courses_list(request)
+        self.assertEqual(len(courses_list), USER_COURSES_COUNT)
+
+        # time the get courses by reversing django groups
+        with Timer() as iteration_over_groups_time_1:
+            courses_list = _accessible_courses_list_from_groups(request)
+        self.assertEqual(len(courses_list), USER_COURSES_COUNT)
+
+        # time again the get courses by reversing django groups
+        with Timer() as iteration_over_groups_time_2:
+            courses_list = _accessible_courses_list_from_groups(request)
+        self.assertEqual(len(courses_list), USER_COURSES_COUNT)
+
+        # test that the time taken by getting courses through reversing django groups is lower then the time
+        # taken by traversing through all courses (if accessible courses are relatively small)
+        self.assertGreaterEqual(iteration_over_courses_time_1.elapsed, iteration_over_groups_time_1.elapsed)
+        self.assertGreaterEqual(iteration_over_courses_time_2.elapsed, iteration_over_groups_time_2.elapsed)

cms/djangoapps/contentstore/views/course.py

@@ -48,7 +48,7 @@ from django_comment_common.utils import seed_permissions_roles
 from student.models import CourseEnrollment
 
 from xmodule.html_module import AboutDescriptor
-from xmodule.modulestore.locator import BlockUsageLocator
+from xmodule.modulestore.locator import BlockUsageLocator, CourseLocator
 from course_creators.views import get_course_creator_status, add_user_with_status_unrequested
 from contentstore import utils
 from student.roles import CourseInstructorRole, CourseStaffRole, CourseCreatorRole, GlobalStaff
@@ -168,76 +168,55 @@ def _accessible_courses_list(request):
         """
         Get courses to which this user has access
         """
+        if GlobalStaff().has_user(request.user):
+            return course.location.course != 'templates'
+
         return (has_course_access(request.user, course.location)
                 # pylint: disable=fixme
                 # TODO remove this condition when templates purged from db
                 and course.location.course != 'templates'
-                and course.location.org != ''
-                and course.location.course != ''
-                and course.location.name != '')
+                )
     courses = filter(course_filter, courses)
     return courses
 
 
+# pylint: disable=invalid-name
 def _accessible_courses_list_from_groups(request):
     """
     List all courses available to the logged in user by reversing access group names
     """
     courses_list = []
-    course_id_list = []
+    course_ids = set()
 
-    user_staff_group_names = request.user.groups.filter(Q(name__startswith='instructor_') | Q(name__startswith='staff_')).values_list('name', flat=True)
-    # first try to get new and unique group names (course_id's) before querying database
+    user_staff_group_names = request.user.groups.filter(
+        Q(name__startswith='instructor_') | Q(name__startswith='staff_')
+    ).values_list('name', flat=True)
+
+    # we can only get course_ids from role names with the new format (instructor_org/number/run or
+    # instructor_org.number.run but not instructor_number).
     for user_staff_group_name in user_staff_group_names:
-        # to avoid duplication try to convert all course_id's to new format e.g. "edX.course.run"
-        course_id = re.sub(r'^(instructor_|staff_)', '', user_staff_group_name).replace('/', '.')
-        if len(course_id.split('.')) == 3:
-            if course_id not in course_id_list:
-                course_id_list.append(course_id)
+        # to avoid duplication try to convert all course_id's to format with dots e.g. "edx.course.run"
+        if user_staff_group_name.startswith("instructor_"):
+            # strip starting text "instructor_"
+            course_id = user_staff_group_name[11:]
         else:
-            # old group format: course number only e.g. "course"
-            # not preferable (so return)
-            #
-            # Note: We can try to get course with only course number if there is exactly
-            #  one course with this course number
-            return (False, courses_list)
+            # strip starting text "staff_"
+            course_id = user_staff_group_name[6:]
 
-    for course_id in course_id_list:
-        # new group format: id of course e.g. "edX.course.run"
-        org, course, name = course_id.split('.')
-        course_location = Location('i4x', org, course, 'course', name)
-        try:
-            course = modulestore('direct').get_item(course_location)
-            courses_list.append(course)
-        except ItemNotFoundError:
-            course = None
+        course_ids.add(course_id.replace('/', '.').lower())
 
-        if not course:
-            # since access groups are being stored in lowercase, also do a case-insensitive search
-            # for the potential course id.
-            course_search_location = bson.son.SON({
-                '_id.tag': 'i4x',
-                # pylint: disable=no-member
-                '_id.org': re.compile(u'^{}$'.format(course_location.org), re.IGNORECASE | re.UNICODE),
-                # pylint: disable=no-member
-                '_id.course': re.compile(u'^{}$'.format(course_location.course), re.IGNORECASE | re.UNICODE),
-                '_id.category': 'course',
-                # pylint: disable=no-member
-                '_id.name': re.compile(u'^{}$'.format(course_location.name), re.IGNORECASE | re.UNICODE)
-            })
-            courses = modulestore().collection.find(course_search_location, fields=('_id'))
-            if courses.count() == 1:
-                course_id = courses[0].get('_id')
-                course_location = Location('i4x', course_id.get('org'), course_id.get('course'), 'course', course_id.get('name'))
-                try:
-                    course = modulestore('direct').get_item(course_location)
-                    courses_list.append(course)
-                except ItemNotFoundError:
-                    return (False, courses_list)
-            else:
-                return (False, courses_list)
+    for course_id in course_ids:
+        # get course_location with lowercase idget_item
+        course_location = loc_mapper().translate_locator_to_location(
+            CourseLocator(package_id=course_id), get_course=True, lower_only=True
+        )
+        if course_location is None:
+            raise ItemNotFoundError(course_id)
 
-    return (True, courses_list)
+        course = modulestore('direct').get_course(course_location.course_id)
+        courses_list.append(course)
+
+    return courses_list
 
 
 @login_required
@@ -252,14 +231,17 @@ def course_listing(request):
         # user has global access so no need to get courses from django groups
         courses = _accessible_courses_list(request)
     else:
-        success, courses_from_groups = _accessible_courses_list_from_groups(request)
-        if success:
-            courses = courses_from_groups
-        else:
+        try:
+            courses = _accessible_courses_list_from_groups(request)
+        except ItemNotFoundError:
             # user have some old groups or there was some error getting courses from django groups
             # so fallback to iterating through all courses
             courses = _accessible_courses_list(request)
 
+            # update location entry in "loc_mapper" for user courses (add keys 'lower_id' and 'lower_course_id')
+            for course in courses:
+                loc_mapper().create_map_entry(course.location)
+
     def format_course_for_view(course):
         """
         return tuple of the data which the view requires for each course

common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py

@@ -96,14 +96,27 @@ class LocMapperStore(object):
             course_location.org, course_location.course,
             course_location.name if course_location.category == 'course' else None
         )
+        # create location id with lower case
+        location_id_lower = self._construct_lower_location_son(
+            course_location.org, course_location.course,
+            course_location.name if course_location.category == 'course' else None
+        )
+
+        try:
+            self.location_map.insert({
+                '_id': location_id,
+                'lower_id': location_id_lower,
+                'course_id': package_id,
+                'lower_course_id': package_id.lower(),
+                'draft_branch': draft_branch,
+                'prod_branch': prod_branch,
+                'block_map': block_map or {},
+            })
+        except pymongo.errors.DuplicateKeyError:
+            # update old entry with 'lower_id' and 'lower_course_id'
+            location_update = {'lower_id': location_id_lower, 'lower_course_id': package_id.lower()}
+            self.location_map.update({'_id': location_id}, {'$set': location_update})
 
-        self.location_map.insert({
-            '_id': location_id,
-            'course_id': package_id,
-            'draft_branch': draft_branch,
-            'prod_branch': prod_branch,
-            'block_map': block_map or {},
-        })
         return package_id
 
     def translate_location(self, old_style_course_id, location, published=True, add_entry_if_missing=True):
@@ -191,7 +204,7 @@ class LocMapperStore(object):
         self._cache_location_map_entry(old_style_course_id, location, published_usage, draft_usage)
         return result
 
-    def translate_locator_to_location(self, locator, get_course=False):
+    def translate_locator_to_location(self, locator, get_course=False, lower_only=False):
         """
         Returns an old style Location for the given Locator if there's an appropriate entry in the
         mapping collection. Note, it requires that the course was previously mapped (a side effect of
@@ -210,7 +223,7 @@ class LocMapperStore(object):
         :param locator: a BlockUsageLocator
         """
         if get_course:
-            cached_value = self._get_course_location_from_cache(locator.package_id)
+            cached_value = self._get_course_location_from_cache(locator.package_id, lower_only)
         else:
             cached_value = self._get_location_from_cache(locator)
         if cached_value:
@@ -218,7 +231,10 @@ class LocMapperStore(object):
 
         # This does not require that the course exist in any modulestore
         # only that it has a mapping entry.
-        maps = self.location_map.find({'course_id': locator.package_id})
+        if lower_only:
+            maps = self.location_map.find({'lower_course_id': locator.package_id.lower()})
+        else:
+            maps = self.location_map.find({'course_id': locator.package_id})
         # look for one which maps to this block block_id
         if maps.count() == 0:
             return None
@@ -243,11 +259,17 @@ class LocMapperStore(object):
                         category,
                         self.decode_key_from_mongo(old_name),
                         None)
+
+                    if lower_only:
+                        candidate_key = "lower_course_id"
+                    else:
+                        candidate_key = "course_id"
+
                     published_locator = BlockUsageLocator(
-                        candidate['course_id'], branch=candidate['prod_branch'], block_id=block_id
+                        candidate[candidate_key], branch=candidate['prod_branch'], block_id=block_id
                     )
                     draft_locator = BlockUsageLocator(
-                        candidate['course_id'], branch=candidate['draft_branch'], block_id=block_id
+                        candidate[candidate_key], branch=candidate['draft_branch'], block_id=block_id
                     )
                     self._cache_location_map_entry(old_course_id, location, published_locator, draft_locator)
 
@@ -259,7 +281,7 @@ class LocMapperStore(object):
                 return result
         return None
 
-    def translate_location_to_course_locator(self, old_style_course_id, location, published=True):
+    def translate_location_to_course_locator(self, old_style_course_id, location, published=True, lower_only=False):
         """
         Used when you only need the CourseLocator and not a full BlockUsageLocator. Probably only
         useful for get_items which wildcards name or category.
@@ -270,7 +292,7 @@ class LocMapperStore(object):
         if cached:
             return cached
 
-        location_id = self._interpret_location_course_id(old_style_course_id, location)
+        location_id = self._interpret_location_course_id(old_style_course_id, location, lower_only)
 
         maps = self.location_map.find(location_id)
         maps = list(maps)
@@ -310,7 +332,7 @@ class LocMapperStore(object):
         self.location_map.update(location_id, {'$set': {'block_map': block_map}})
         return block_id
 
-    def _interpret_location_course_id(self, course_id, location):
+    def _interpret_location_course_id(self, course_id, location, lower_only=False):
         """
         Take the old style course id (org/course/run) and return a dict w/ a SON for querying the mapping table.
         If the course_id is empty, it uses location, but this may result in an inadequate id.
@@ -322,9 +344,13 @@ class LocMapperStore(object):
         if course_id:
             # re doesn't allow ?P<_id.org> and ilk
             matched = re.match(r'([^/]+)/([^/]+)/([^/]+)', course_id)
+            if lower_only:
+                return {'lower_id': self._construct_lower_location_son(*matched.groups())}
             return {'_id': self._construct_location_son(*matched.groups())}
 
         if location.category == 'course':
+            if lower_only:
+                return {'lower_id': self._construct_lower_location_son(location.org, location.course, location.name)}
             return {'_id': self._construct_location_son(location.org, location.course, location.name)}
         else:
             return bson.son.SON([('_id.org', location.org), ('_id.course', location.course)])
@@ -352,6 +378,15 @@ class LocMapperStore(object):
         else:
             return bson.son.SON([('org', org), ('course', course)])
 
+    def _construct_lower_location_son(self, org, course, name=None):
+        """
+        Construct the SON needed to represent the location with lower case
+        """
+        if name is not None:
+            name = name.lower()
+
+        return self._construct_location_son(org.lower(), course.lower(), name)
+
     def _block_id_is_guid(self, name):
         """
         Does the given name look like it's a guid?
@@ -424,12 +459,17 @@ class LocMapperStore(object):
         """
         return self.cache.get(unicode(locator))
 
-    def _get_course_location_from_cache(self, locator_package_id):
+    def _get_course_location_from_cache(self, locator_package_id, lower_only=False):
         """
         See if the package_id is in the cache. If so, return the mapped location to the
         course root.
         """
-        return self.cache.get(u'courseId+{}'.format(locator_package_id))
+        if lower_only:
+            cache_key = u'courseIdLower+{}'.format(locator_package_id.lower())
+        else:
+            cache_key = u'courseId+{}'.format(locator_package_id)
+
+        return self.cache.get(cache_key)
 
     def _cache_course_locator(self, old_course_id, published_course_locator, draft_course_locator):
         """
@@ -448,6 +488,7 @@ class LocMapperStore(object):
         setmany = {}
         if location.category == 'course':
             setmany[u'courseId+{}'.format(published_usage.package_id)] = location
+            setmany[u'courseIdLower+{}'.format(published_usage.package_id.lower())] = location
         setmany[unicode(published_usage)] = location
         setmany[unicode(draft_usage)] = location
         setmany[u'{}+{}'.format(old_course_id, location.url())] = (published_usage, draft_usage)

requirements/edx/base.txt

@@ -104,6 +104,7 @@ django_debug_toolbar==0.9.4
 django-debug-toolbar-mongo
 
 # Used for testing
+chrono==1.0.2
 coverage==3.7
 ddt==0.6.0
 django-crum==0.5