Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #9488 from edx/dsjen/courses-via-overview

Browse Source 
Optimize OpenID Course Claims for users.
This commit is contained in:
Dennis Jen
2015-09-17 13:35:49 -04:00
parent d0b6d18134 844edac443
commit 305f88f646
14 changed files with 246 additions and 95 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
common/djangoapps/student/tests/tests.py
View File

@@ -488,21 +488,18 @@ class DashboardTest(ModuleStoreTestCase):
self.assertContains(response, expected_url)
@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
@ddt.data((ModuleStoreEnum.Type.mongo, 1), (ModuleStoreEnum.Type.split, 3))
@ddt.unpack
def test_dashboard_metadata_caching(self, modulestore_type, expected_mongo_calls):
@ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
def test_dashboard_metadata_caching(self, modulestore_type):
"""
Check that the student dashboard makes use of course metadata caching.
After enrolling a student in a course, that course's metadata should be
cached as a CourseOverview. The student dashboard should never have to make
calls to the modulestore.
After creating a course, that course's metadata should be cached as a
CourseOverview. The student dashboard should never have to make calls to
the modulestore.
Arguments:
modulestore_type (ModuleStoreEnum.Type): Type of modulestore to create
test course in.
expected_mongo_calls (int >=0): Number of MongoDB queries expected for
a single call to the module store.
Note to future developers:
If you break this test so that the "check_mongo_calls(0)" fails,
@@ -512,11 +509,11 @@ class DashboardTest(ModuleStoreTestCase):
CourseDescriptor isn't necessary.
"""
# Create a course and log in the user.
test_course = CourseFactory.create(default_store=modulestore_type)
# Creating a new course will trigger a publish event and the course will be cached
test_course = CourseFactory.create(default_store=modulestore_type, emit_signals=True)
self.client.login(username="jack", password="test")
# Enrolling the user in the course will result in a modulestore query.
with check_mongo_calls(expected_mongo_calls):
with check_mongo_calls(0):
CourseEnrollment.enroll(self.user, test_course.id)
# Subsequent requests will only result in SQL queries to load the

2
common/lib/xmodule/xmodule/modulestore/tests/factories.py
View File

@@ -114,7 +114,7 @@ class CourseFactory(XModuleFactory):
name = kwargs.get('name', kwargs.get('run', Location.clean(kwargs.get('display_name'))))
run = kwargs.pop('run', name)
user_id = kwargs.pop('user_id', ModuleStoreEnum.UserID.test)
emit_signals = kwargs.get('emit_signals', False)
emit_signals = kwargs.pop('emit_signals', False)
# Pass the metadata just as field=value pairs
kwargs.update(kwargs.pop('metadata', {}))

11
lms/djangoapps/ccx/tests/test_tasks.py
View File

@@ -94,17 +94,14 @@ class TestSendCCXCoursePublished(ModuleStoreTestCase):
structure = CourseStructure.objects.get(course_id=course_key)
self.assertEqual(structure.structure, ccx_structure)
def test_course_overview_deleted(self):
"""Check that course overview is deleted after course published signal is sent
def test_course_overview_cached(self):
"""Check that course overview is cached after course published signal is sent
"""
course_key = CCXLocator.from_course_locator(self.course.id, self.ccx.id)
overview = CourseOverview(id=course_key)
overview.version = 1
overview.save()
overview = CourseOverview.objects.filter(id=course_key)
self.assertEqual(len(overview), 1)
self.assertEqual(len(overview), 0)
with mock_signal_receiver(SignalHandler.course_published) as receiver:
self.call_fut(self.course.id)
self.assertEqual(receiver.call_count, 3)
overview = CourseOverview.objects.filter(id=course_key)
self.assertEqual(len(overview), 0)
self.assertEqual(len(overview), 1)

17
lms/djangoapps/oauth2_handler/handlers.py
View File

@@ -2,9 +2,9 @@
from django.conf import settings
from django.core.cache import cache
from xmodule.modulestore.django import modulestore
from courseware.access import has_access
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
from openedx.core.djangoapps.user_api.models import UserPreference
from student.models import anonymous_id_for_user
from student.models import UserProfile
@@ -200,13 +200,13 @@ class CourseAccessHandler(object):
course_ids = cache.get(key)
if not course_ids:
courses = _get_all_courses()
course_keys = CourseOverview.get_all_course_keys()
# Global staff have access to all courses. Filter courses for non-global staff.
if not GlobalStaff().has_user(user):
courses = [course for course in courses if has_access(user, access_type, course)]
course_keys = [course_key for course_key in course_keys if has_access(user, access_type, course_key)]
course_ids = [unicode(course.id) for course in courses]
course_ids = [unicode(course_key) for course_key in course_keys]
cache.set(key, course_ids, self.COURSE_CACHE_TIMEOUT)
@@ -234,12 +234,3 @@ class IDTokenHandler(OpenIDHandler, ProfileHandler, CourseAccessHandler, Permiss
class UserInfoHandler(OpenIDHandler, ProfileHandler, CourseAccessHandler, PermissionsHandler):
""" Configure the UserInfo handler for the LMS. """
pass
def _get_all_courses():
""" Utility function to list all available courses. """
ms_courses = modulestore().get_courses()
courses = [course for course in ms_courses if course.scope_ids.block_type == 'course']
return courses

99
lms/djangoapps/oauth2_handler/tests.py
View File

@@ -2,16 +2,17 @@
from django.core.cache import cache
from django.test.utils import override_settings
from lang_pref import LANGUAGE_KEY
from opaque_keys.edx.locations import SlashSeparatedCourseKey
from xmodule.modulestore.tests.django_utils import TEST_DATA_MIXED_TOY_MODULESTORE
from xmodule.modulestore.tests.factories import (check_mongo_calls, CourseFactory)
from student.models import anonymous_id_for_user
from student.models import UserProfile
from student.roles import CourseStaffRole, CourseInstructorRole
from student.roles import (CourseInstructorRole, CourseStaffRole, GlobalStaff,
OrgInstructorRole, OrgStaffRole)
from student.tests.factories import UserFactory, UserProfileFactory
from openedx.core.djangoapps.user_api.preferences.api import set_user_preference
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
# Will also run default tests for IDTokens and UserInfo
from oauth2_provider.tests import IDTokenTestCase, UserInfoTestCase
@@ -19,14 +20,10 @@ from oauth2_provider.tests import IDTokenTestCase, UserInfoTestCase
class BaseTestMixin(ModuleStoreTestCase):
profile = None
MODULESTORE = TEST_DATA_MIXED_TOY_MODULESTORE
def setUp(self):
super(BaseTestMixin, self).setUp()
self.course_key = SlashSeparatedCourseKey('edX', 'toy', '2012_Fall')
self.course_key = CourseFactory.create(emit_signals=True).id
self.course_id = unicode(self.course_key)
self.user_factory = UserFactory
self.set_user(self.make_user())
@@ -77,7 +74,8 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase):
self.assertEqual(language, locale)
def test_no_special_course_access(self):
scopes, claims = self.get_id_token_values('openid course_instructor course_staff')
with check_mongo_calls(0):
scopes, claims = self.get_id_token_values('openid course_instructor course_staff')
self.assertNotIn('course_staff', scopes)
self.assertNotIn('staff_courses', claims)
@@ -86,14 +84,15 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase):
def test_course_staff_courses(self):
CourseStaffRole(self.course_key).add_users(self.user)
scopes, claims = self.get_id_token_values('openid course_staff')
with check_mongo_calls(0):
scopes, claims = self.get_id_token_values('openid course_staff')
self.assertIn('course_staff', scopes)
self.assertNotIn('staff_courses', claims) # should not return courses in id_token
def test_course_instructor_courses(self):
CourseInstructorRole(self.course_key).add_users(self.user)
with check_mongo_calls(0):
CourseInstructorRole(self.course_key).add_users(self.user)
scopes, claims = self.get_id_token_values('openid course_instructor')
@@ -104,6 +103,7 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase):
CourseStaffRole(self.course_key).add_users(self.user)
course_id = unicode(self.course_key)
nonexistent_course_id = 'some/other/course'
claims = {
@@ -113,7 +113,8 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase):
}
}
scopes, claims = self.get_id_token_values(scope='openid course_staff', claims=claims)
with check_mongo_calls(0):
scopes, claims = self.get_id_token_values(scope='openid course_staff', claims=claims)
self.assertIn('course_staff', scopes)
self.assertIn('staff_courses', claims)
@@ -133,6 +134,11 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase):
class UserInfoTest(BaseTestMixin, UserInfoTestCase):
def setUp(self):
super(UserInfoTest, self).setUp()
# create another course in the DB that only global staff have access to
CourseFactory.create(emit_signals=True)
def token_for_scope(self, scope):
full_scope = 'openid %s' % scope
self.set_access_token_scope(full_scope)
@@ -158,43 +164,64 @@ class UserInfoTest(BaseTestMixin, UserInfoTestCase):
self.assertEqual(result.status_code, 200)
return claims
def _assert_role_using_scope(self, scope, claim, assert_one_course=True):
with check_mongo_calls(0):
claims = self.get_with_scope(scope)
self.assertEqual(len(claims), 2)
courses = claims[claim]
self.assertIn(self.course_id, courses)
if assert_one_course:
self.assertEqual(len(courses), 1)
def test_request_global_staff_courses_using_scope(self):
GlobalStaff().add_users(self.user)
self._assert_role_using_scope('course_staff', 'staff_courses', assert_one_course=False)
def test_request_org_staff_courses_using_scope(self):
OrgStaffRole(self.course_key.org).add_users(self.user)
self._assert_role_using_scope('course_staff', 'staff_courses')
def test_request_org_instructor_courses_using_scope(self):
OrgInstructorRole(self.course_key.org).add_users(self.user)
self._assert_role_using_scope('course_instructor', 'instructor_courses')
def test_request_staff_courses_using_scope(self):
CourseStaffRole(self.course_key).add_users(self.user)
claims = self.get_with_scope('course_staff')
courses = claims['staff_courses']
self.assertIn(self.course_id, courses)
self.assertEqual(len(courses), 1)
self._assert_role_using_scope('course_staff', 'staff_courses')
def test_request_instructor_courses_using_scope(self):
CourseInstructorRole(self.course_key).add_users(self.user)
claims = self.get_with_scope('course_instructor')
self._assert_role_using_scope('course_instructor', 'instructor_courses')
courses = claims['instructor_courses']
def _assert_role_using_claim(self, scope, claim):
values = [self.course_id, 'some_invalid_course']
with check_mongo_calls(0):
claims = self.get_with_claim_value(scope, claim, values)
self.assertEqual(len(claims), 2)
courses = claims[claim]
self.assertIn(self.course_id, courses)
self.assertEqual(len(courses), 1)
def test_request_global_staff_courses_with_claims(self):
GlobalStaff().add_users(self.user)
self._assert_role_using_claim('course_staff', 'staff_courses')
def test_request_org_staff_courses_with_claims(self):
OrgStaffRole(self.course_key.org).add_users(self.user)
self._assert_role_using_claim('course_staff', 'staff_courses')
def test_request_org_instructor_courses_with_claims(self):
OrgInstructorRole(self.course_key.org).add_users(self.user)
self._assert_role_using_claim('course_instructor', 'instructor_courses')
def test_request_staff_courses_with_claims(self):
CourseStaffRole(self.course_key).add_users(self.user)
values = [self.course_id, 'some_invalid_course']
claims = self.get_with_claim_value('course_staff', 'staff_courses', values)
self.assertEqual(len(claims), 2)
courses = claims['staff_courses']
self.assertIn(self.course_id, courses)
self.assertEqual(len(courses), 1)
self._assert_role_using_claim('course_staff', 'staff_courses')
def test_request_instructor_courses_with_claims(self):
CourseInstructorRole(self.course_key).add_users(self.user)
values = ['edX/toy/TT_2012_Fall', self.course_id, 'invalid_course_id']
claims = self.get_with_claim_value('course_instructor', 'instructor_courses', values)
self.assertEqual(len(claims), 2)
courses = claims['instructor_courses']
self.assertIn(self.course_id, courses)
self.assertEqual(len(courses), 1)
self._assert_role_using_claim('course_instructor', 'instructor_courses')
def test_permissions_scope(self):
claims = self.get_with_scope('permissions')

2
lms/envs/test.py
View File

@@ -269,6 +269,8 @@ OPENID_PROVIDER_TRUSTED_ROOTS = ['*']
############################## OAUTH2 Provider ################################
FEATURES['ENABLE_OAUTH2_PROVIDER'] = True
# don't cache courses for testing
OIDC_COURSE_HANDLER_CACHE_TIMEOUT = 0
########################### External REST APIs #################################
FEATURES['ENABLE_MOBILE_REST_API'] = True

0
openedx/core/djangoapps/content/course_overviews/management/__init__.py Normal file
View File

0
openedx/core/djangoapps/content/course_overviews/management/commands/__init__.py Normal file
View File

60
openedx/core/djangoapps/content/course_overviews/management/commands/generate_course_overview.py Normal file
View File

@@ -0,0 +1,60 @@
"""
Command to load course overviews.
"""
import logging
from optparse import make_option
from django.core.management.base import BaseCommand, CommandError
from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import CourseKey
from xmodule.modulestore.django import modulestore
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
log = logging.getLogger(__name__)
class Command(BaseCommand):
"""
Example usage:
$ ./manage.py lms generate_course_overview --all --settings=devstack
$ ./manage.py lms generate_course_overview 'edX/DemoX/Demo_Course' --settings=devstack
"""
args = '<course_id course_id ...>'
help = 'Generates and stores course overview for one or more courses.'
option_list = BaseCommand.option_list + (
make_option('--all',
action='store_true',
default=False,
help='Generate course overview for all courses.'),
)
def handle(self, *args, **options):
course_keys = []
if options['all']:
course_keys = [course.id for course in modulestore().get_courses()]
else:
if len(args) < 1:
raise CommandError('At least one course or --all must be specified.')
try:
course_keys = [CourseKey.from_string(arg) for arg in args]
except InvalidKeyError:
log.fatal('Invalid key specified.')
if not course_keys:
log.fatal('No courses specified.')
log.info('Generating course overview for %d courses.', len(course_keys))
log.debug('Generating course overview(s) for the following courses: %s', course_keys)
for course_key in course_keys:
try:
CourseOverview.get_from_id(course_key)
except Exception as ex: # pylint: disable=broad-except
log.exception('An error occurred while generating course overview for %s: %s', unicode(
course_key), ex.message)
log.info('Finished generating course overviews.')

0
openedx/core/djangoapps/content/course_overviews/management/commands/tests/__init__.py Normal file
View File

80
openedx/core/djangoapps/content/course_overviews/management/commands/tests/test_generate_course_overview.py Normal file
View File

@@ -0,0 +1,80 @@
# pylint: disable=missing-docstring
from django.core.management.base import CommandError
from mock import patch
from openedx.core.djangoapps.content.course_overviews.management.commands import generate_course_overview
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory
class TestGenerateCourseOverview(ModuleStoreTestCase):
"""
Tests course overview management command.
"""
def setUp(self):
"""
Create courses in modulestore.
"""
super(TestGenerateCourseOverview, self).setUp()
self.course_key_1 = CourseFactory.create().id
self.course_key_2 = CourseFactory.create().id
self.command = generate_course_overview.Command()
def _assert_courses_not_in_overview(self, *courses):
"""
Assert that courses doesn't exist in the course overviews.
"""
course_keys = CourseOverview.get_all_course_keys()
for expected_course_key in courses:
self.assertNotIn(expected_course_key, course_keys)
def _assert_courses_in_overview(self, *courses):
"""
Assert courses exists in course overviews.
"""
course_keys = CourseOverview.get_all_course_keys()
for expected_course_key in courses:
self.assertIn(expected_course_key, course_keys)
def test_generate_all(self):
"""
Test that all courses in the modulestore are loaded into course overviews.
"""
# ensure that the newly created courses aren't in course overviews
self._assert_courses_not_in_overview(self.course_key_1, self.course_key_2)
self.command.handle(all=True)
# CourseOverview will be populated with all courses in the modulestore
self._assert_courses_in_overview(self.course_key_1, self.course_key_2)
def test_generate_one(self):
"""
Test that a specified course is loaded into course overviews.
"""
self._assert_courses_not_in_overview(self.course_key_1, self.course_key_2)
self.command.handle(unicode(self.course_key_1), all=False)
self._assert_courses_in_overview(self.course_key_1)
self._assert_courses_not_in_overview(self.course_key_2)
@patch('openedx.core.djangoapps.content.course_overviews.management.commands.generate_course_overview.log')
def test_invalid_key(self, mock_log):
"""
Test that invalid key errors are logged.
"""
self.command.handle('not/found', all=False)
self.assertTrue(mock_log.fatal.called)
@patch('openedx.core.djangoapps.content.course_overviews.management.commands.generate_course_overview.log')
def test_not_found_key(self, mock_log):
"""
Test keys not found are logged.
"""
self.command.handle('fake/course/id', all=False)
self.assertTrue(mock_log.exception.called)
def test_no_params(self):
"""
Test exception raised when no parameters are specified.
"""
with self.assertRaises(CommandError):
self.command.handle(all=False)

15
openedx/core/djangoapps/content/course_overviews/models.py
View File

@@ -9,6 +9,7 @@ from django.db.utils import IntegrityError
from django.utils.translation import ugettext
from model_utils.models import TimeStampedModel
from opaque_keys.edx.keys import CourseKey
from util.date_utils import strftime_localized
from xmodule import course_metadata_utils
from xmodule.course_module import CourseDescriptor
@@ -151,7 +152,7 @@ class CourseOverview(TimeStampedModel):
)
@classmethod
def _load_from_module_store(cls, course_id):
def load_from_module_store(cls, course_id):
"""
Load a CourseDescriptor, create a new CourseOverview from it, cache the
overview, and return it.
@@ -225,7 +226,7 @@ class CourseOverview(TimeStampedModel):
course_overview = None
except cls.DoesNotExist:
course_overview = None
return course_overview or cls._load_from_module_store(course_id)
return course_overview or cls.load_from_module_store(course_id)
def clean_id(self, padding_char='='):
"""
@@ -340,3 +341,13 @@ class CourseOverview(TimeStampedModel):
Returns a list of ID strings for this course's prerequisite courses.
"""
return json.loads(self._pre_requisite_courses_json)
@classmethod
def get_all_course_keys(cls):
"""
Returns all course keys from course overviews.
"""
return [
CourseKey.from_string(course_overview['id'])
for course_overview in CourseOverview.objects.values('id')
]

3
openedx/core/djangoapps/content/course_overviews/signals.py
View File

@@ -11,9 +11,10 @@ from xmodule.modulestore.django import SignalHandler
def _listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable=unused-argument
"""
Catches the signal that a course has been published in Studio and
invalidates the corresponding CourseOverview cache entry if one exists.
updates the corresponding CourseOverview cache entry.
"""
CourseOverview.objects.filter(id=course_key).delete()
CourseOverview.load_from_module_store(course_key)
@receiver(SignalHandler.course_deleted)

33
openedx/core/djangoapps/content/course_overviews/tests.py
View File

@@ -258,31 +258,22 @@ class CourseOverviewTestCase(ModuleStoreTestCase):
self.store.delete_course(course.id, ModuleStoreEnum.UserID.test)
CourseOverview.get_from_id(course.id)
@ddt.data((ModuleStoreEnum.Type.mongo, 1, 1), (ModuleStoreEnum.Type.split, 3, 4))
@ddt.unpack
def test_course_overview_caching(self, modulestore_type, min_mongo_calls, max_mongo_calls):
@ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
def test_course_overview_caching(self, modulestore_type):
"""
Tests that CourseOverview structures are actually getting cached.
Arguments:
modulestore_type (ModuleStoreEnum.Type): type of store to create the
course in.
min_mongo_calls (int): minimum number of MongoDB queries we expect
to be made.
max_mongo_calls (int): maximum number of MongoDB queries we expect
to be made.
"""
course = CourseFactory.create(default_store=modulestore_type)
# The first time we load a CourseOverview, it will be a cache miss, so
# we expect the modulestore to be queried.
with check_mongo_calls_range(max_finds=max_mongo_calls, min_finds=min_mongo_calls):
_course_overview_1 = CourseOverview.get_from_id(course.id)
# Creating a new course will trigger a publish event and the course will be cached
course = CourseFactory.create(default_store=modulestore_type, emit_signals=True)
# The second time we load a CourseOverview, it will be a cache hit, so
# we expect no modulestore queries to be made.
# The cache will be hit and mongo will not be queried
with check_mongo_calls(0):
_course_overview_2 = CourseOverview.get_from_id(course.id)
CourseOverview.get_from_id(course.id)
@ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
def test_get_non_existent_course(self, modulestore_type):
@@ -298,24 +289,18 @@ class CourseOverviewTestCase(ModuleStoreTestCase):
with self.assertRaises(CourseOverview.DoesNotExist):
CourseOverview.get_from_id(store.make_course_key('Non', 'Existent', 'Course'))
@ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
def test_get_errored_course(self, modulestore_type):
def test_get_errored_course(self):
"""
Test that getting an ErrorDescriptor back from the module store causes
get_from_id to raise an IOError.
Arguments:
modulestore_type (ModuleStoreEnum.Type): type of store to create the
course in.
load_from_module_store to raise an IOError.
"""
course = CourseFactory.create(default_store=modulestore_type)
mock_get_course = mock.Mock(return_value=ErrorDescriptor)
with mock.patch('xmodule.modulestore.mixed.MixedModuleStore.get_course', mock_get_course):
# This mock makes it so when the module store tries to load course data,
# an exception is thrown, which causes get_course to return an ErrorDescriptor,
# which causes get_from_id to raise an IOError.
with self.assertRaises(IOError):
CourseOverview.get_from_id(course.id)
CourseOverview.load_from_module_store(self.store.make_course_key('Non', 'Existent', 'Course'))
def test_malformed_grading_policy(self):
"""