From 00b3717829fcc7ed31a82bf0c7b025115c51631b Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 11 Feb 2013 10:16:57 -0500 Subject: [PATCH 1/4] Use prefetch on user to load groups and prevent extra DB lookups. --- lms/djangoapps/courseware/views.py | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index e8b36ecd2a..c2851026ea 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -86,7 +86,8 @@ def render_accordion(request, course, chapter, section): Returns the html string''' # grab the table of contents - toc = toc_for_course(request.user, request, course, chapter, section) + user = User.objects.prefetch_related("groups").get(id=request.user.id) + toc = toc_for_course(user, request, course, chapter, section) context = dict([('toc', toc), ('course_id', course.id), @@ -251,23 +252,24 @@ def index(request, course_id, chapter=None, section=None, - HTTPresponse """ - course = get_course_with_access(request.user, course_id, 'load', depth=2) - staff_access = has_access(request.user, course, 'staff') - registered = registered_for_course(course, request.user) + user = User.objects.prefetch_related("groups").get(id=request.user.id) + course = get_course_with_access(user, course_id, 'load', depth=2) + staff_access = has_access(user, course, 'staff') + registered = registered_for_course(course, user) if not registered: # TODO (vshnayder): do course instructors need to be registered to see course? - log.debug('User %s tried to view course %s but is not enrolled' % (request.user, course.location.url())) + log.debug('User %s tried to view course %s but is not enrolled' % (user, course.location.url())) return redirect(reverse('about_course', args=[course.id])) try: student_module_cache = StudentModuleCache.cache_for_descriptor_descendents( - course.id, request.user, course, depth=2) + course.id, user, course, depth=2) # Has this student been in this course before? first_time = student_module_cache.lookup(course_id, 'course', course.location.url()) is None # Load the module for the course - course_module = get_module_for_descriptor(request.user, request, course, student_module_cache, course.id) + course_module = get_module_for_descriptor(user, request, course, student_module_cache, course.id) if course_module is None: log.warning('If you see this, something went wrong: if we got this' ' far, should have gotten a course module for this user') @@ -289,7 +291,7 @@ def index(request, course_id, chapter=None, section=None, chapter_descriptor = course.get_child_by(lambda m: m.url_name == chapter) if chapter_descriptor is not None: - instance_module = get_instance_module(course_id, request.user, course_module, student_module_cache) + instance_module = get_instance_module(course_id, user, course_module, student_module_cache) save_child_position(course_module, chapter, instance_module) else: raise Http404('No chapter descriptor found with name {}'.format(chapter)) @@ -308,9 +310,9 @@ def index(request, course_id, chapter=None, section=None, # Load all descendants of the section, because we're going to display its # html, which in general will need all of its children section_module_cache = StudentModuleCache.cache_for_descriptor_descendents( - course.id, request.user, section_descriptor, depth=None) + course.id, user, section_descriptor, depth=None) - section_module = get_module(request.user, request, section_descriptor.location, + section_module = get_module(user, request, section_descriptor.location, section_module_cache, course.id, position=position, depth=None) if section_module is None: # User may be trying to be clever and access something @@ -318,12 +320,12 @@ def index(request, course_id, chapter=None, section=None, raise Http404 # Save where we are in the chapter - instance_module = get_instance_module(course_id, request.user, chapter_module, student_module_cache) + instance_module = get_instance_module(course_id, user, chapter_module, student_module_cache) save_child_position(chapter_module, section, instance_module) # check here if this section *is* a timed module. if section_module.category == 'timelimit': - timer_context = update_timelimit_module(request.user, course_id, student_module_cache, + timer_context = update_timelimit_module(user, course_id, student_module_cache, section_descriptor, section_module) if 'timer_expiration_datetime' in timer_context: context.update(timer_context) @@ -364,7 +366,7 @@ def index(request, course_id, chapter=None, section=None, log.exception("Error in index view: user={user}, course={course}," " chapter={chapter} section={section}" "position={position}".format( - user=request.user, + user=user, course=course, chapter=chapter, section=section, From 04deb30fb688eee4062332aea23b654a7996d04f Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 11 Feb 2013 10:17:57 -0500 Subject: [PATCH 2/4] Factor out the access group generation so we can check staff/instructor status without DB lookup. --- lms/djangoapps/courseware/access.py | 104 ++++++++++++++++------------ 1 file changed, 58 insertions(+), 46 deletions(-) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index b41d231011..206286f80e 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -1,10 +1,10 @@ """This file contains (or should), all access control logic for the courseware. Ideally, it will be the only place that needs to know about any special settings like DISABLE_START_DATES""" - import logging import time from datetime import datetime, timedelta +from functools import partial from django.conf import settings from django.contrib.auth.models import Group @@ -341,6 +341,29 @@ def _dispatch(table, action, user, obj): def _does_course_group_name_exist(name): return len(Group.objects.filter(name=name)) > 0 +def group_names_for(role, location, course_context=None): + """Returns the group names for a given role with this location. Plural + because it will return both the name we expectd now as well as the legacy + group name we support for backwards compatibility. This should not check + the DB for existance of a group (like some of its callers do) because that's + a DB roundtrip, and we expect this might be invoked many times as we crawl + an XModule tree.""" + loc = Location(location) + legacy_group_name = '{0}_{1}'.format(role, loc.course) + + if loc.category == 'course': + course_id = loc.course_id + else: + if course_context is None: + raise CourseContextRequired() + course_id = course_context + + group_name = '{0}_{1}'.format(role, course_id) + + return group_name, legacy_group_name + +group_names_for_staff = partial(group_names_for, 'staff') +group_names_for_instructor = partial(group_names_for, 'instructor') def _course_staff_group_name(location, course_context=None): """ @@ -354,33 +377,12 @@ def _course_staff_group_name(location, course_context=None): using course_id rather than just the course number. So first check to see if the group name exists """ loc = Location(location) - legacy_name = 'staff_%s' % loc.course - if _does_course_group_name_exist(legacy_name): - return legacy_name + group_name, legacy_group_name = group_names_for_staff(location, course_context) - if loc.category == 'course': - course_id = loc.course_id - else: - if course_context is None: - raise CourseContextRequired() - course_id = course_context - - return 'staff_%s' % course_id - - -def course_beta_test_group_name(location): - """ - Get the name of the beta tester group for a location. Right now, that's - beta_testers_COURSE. - - location: something that can passed to Location. - """ - return 'beta_testers_{0}'.format(Location(location).course) - -# nosetests thinks that anything with _test_ in the name is a test. -# Correct this (https://nose.readthedocs.org/en/latest/finding_tests.html) -course_beta_test_group_name.__test__ = False + if _does_course_group_name_exist(legacy_group_name): + return legacy_group_name + return group_name def _course_instructor_group_name(location, course_context=None): """ @@ -395,18 +397,26 @@ def _course_instructor_group_name(location, course_context=None): using course_id rather than just the course number. So first check to see if the group name exists """ loc = Location(location) - legacy_name = 'instructor_%s' % loc.course - if _does_course_group_name_exist(legacy_name): - return legacy_name + group_name, legacy_group_name = group_names_for_instructor(location, course_context) - if loc.category == 'course': - course_id = loc.course_id - else: - if course_context is None: - raise CourseContextRequired() - course_id = course_context + if _does_course_group_name_exist(legacy_group_name): + return legacy_group_name + + return group_name + +def course_beta_test_group_name(location): + """ + Get the name of the beta tester group for a location. Right now, that's + beta_testers_COURSE. + + location: something that can passed to Location. + """ + return 'beta_testers_{0}'.format(Location(location).course) + +# nosetests thinks that anything with _test_ in the name is a test. +# Correct this (https://nose.readthedocs.org/en/latest/finding_tests.html) +course_beta_test_group_name.__test__ = False - return 'instructor_%s' % course_id def _has_global_staff_access(user): @@ -498,18 +508,20 @@ def _has_access_to_location(user, location, access_level, course_context): user_groups = [g.name for g in user.groups.all()] if access_level == 'staff': - staff_group = _course_staff_group_name(location, course_context) - if staff_group in user_groups: - debug("Allow: user in group %s", staff_group) - return True - debug("Deny: user not in group %s", staff_group) + staff_groups = group_names_for_staff(location, course_context) + for staff_group in staff_groups: + if staff_group in user_groups: + debug("Allow: user in group %s", staff_group) + return True + debug("Deny: user not in groups %s", staff_groups) if access_level == 'instructor' or access_level == 'staff': # instructors get staff privileges - instructor_group = _course_instructor_group_name(location, course_context) - if instructor_group in user_groups: - debug("Allow: user in group %s", instructor_group) - return True - debug("Deny: user not in group %s", instructor_group) + instructor_groups = group_names_for_instructor(location, course_context) + for instructor_group in instructor_groups: + if instructor_group in user_groups: + debug("Allow: user in group %s", instructor_group) + return True + debug("Deny: user not in groups %s", instructor_groups) else: log.debug("Error in access._has_access_to_location access_level=%s unknown" % access_level) From 5d894fdf03d0b181a6161223e1dc98502ce947ae Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 11 Feb 2013 10:24:07 -0500 Subject: [PATCH 3/4] Typo fix --- lms/djangoapps/courseware/access.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 206286f80e..e4cc4a2b1d 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -343,9 +343,9 @@ def _does_course_group_name_exist(name): def group_names_for(role, location, course_context=None): """Returns the group names for a given role with this location. Plural - because it will return both the name we expectd now as well as the legacy + because it will return both the name we expect now as well as the legacy group name we support for backwards compatibility. This should not check - the DB for existance of a group (like some of its callers do) because that's + the DB for existence of a group (like some of its callers do) because that's a DB roundtrip, and we expect this might be invoked many times as we crawl an XModule tree.""" loc = Location(location) From 1d4defd0eae51306f9e481cc8f8387c016988f92 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 1 Mar 2013 10:54:57 -0500 Subject: [PATCH 4/4] Fix bug where we weren't combining org_staff with built-in staff for group checking --- lms/djangoapps/courseware/access.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 287e76e8d0..eaf06d79dc 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -382,7 +382,7 @@ def group_names_for(role, location, course_context=None): group_name = '{0}_{1}'.format(role, course_id) - return group_name, legacy_group_name + return [group_name, legacy_group_name] group_names_for_staff = partial(group_names_for, 'staff') group_names_for_instructor = partial(group_names_for, 'instructor') @@ -552,7 +552,7 @@ def _has_access_to_location(user, location, access_level, course_context): if access_level == 'staff': staff_groups = group_names_for_staff(location, course_context) + \ - _course_org_staff_group_name(location, course_context) + [_course_org_staff_group_name(location, course_context)] for staff_group in staff_groups: if staff_group in user_groups: debug("Allow: user in group %s", staff_group) @@ -561,7 +561,7 @@ def _has_access_to_location(user, location, access_level, course_context): if access_level == 'instructor' or access_level == 'staff': # instructors get staff privileges instructor_groups = group_names_for_instructor(location, course_context) + \ - _course_org_instructor_group_name(location, course_context) + [_course_org_instructor_group_name(location, course_context)] for instructor_group in instructor_groups: if instructor_group in user_groups: debug("Allow: user in group %s", instructor_group)