Skip to content

E
edx-platform
Commit 603f3695 by zubair-arbi
Options

Merge pull request #2423 from edx/zub/story/std1133-studioedgelogin 

try to get courses by django groups for non global users
parents 2057f518 6892bc73
Showing with 350 additions and 26 deletions
cms/djangoapps/contentstore/management/commands/map_courses_location_lower.py 0 → 100644
"""
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 → 100644
"""
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
......@@ -7,6 +7,7 @@ import string # pylint: disable=W0402
import re
import bson
from django.db.models import Q
from django.utils.translation import ugettext as _
from django.contrib.auth.decorators import login_required
from django_future.csrf import ensure_csrf_cookie
......@@ -47,10 +48,10 @@ 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
from student.roles import CourseInstructorRole, CourseStaffRole, CourseCreatorRole, GlobalStaff
from student import auth
from microsite_configuration.middleware import MicrositeConfiguration
......@@ -156,11 +157,9 @@ def _xmodule_json(xmodule, course_id):
return result
@login_required
@ensure_csrf_cookie
def course_listing(request):
def _accessible_courses_list(request):
"""
List all courses available to the logged in user
List all courses available to the logged in user by iterating through all the courses
"""
courses = modulestore('direct').get_courses()
......@@ -169,14 +168,79 @@ def course_listing(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_ids = set()
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 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:
# strip starting text "staff_"
course_id = user_staff_group_name[6:]
course_ids.add(course_id.replace('/', '.').lower())
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)
course = modulestore('direct').get_course(course_location.course_id)
courses_list.append(course)
return courses_list
@login_required
@ensure_csrf_cookie
def course_listing(request):
"""
List all courses available to the logged in user
Try to get all courses by first reversing django groups and fallback to old method if it fails
Note: overhead of pymongo reads will increase if getting courses from django groups fails
"""
if GlobalStaff().has_user(request.user):
# user has global access so no need to get courses from django groups
courses = _accessible_courses_list(request)
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):
"""
......
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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment