Merge pull request #1478 from MITx/bugfix/dave/too_many_sql_queries

Reduce SQL queries in courseware by model caching a User's groups

lms/djangoapps/courseware/access.py

 """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
@@ -363,21 +363,15 @@ def _course_org_staff_group_name(location, course_context=None):
     return 'staff_%s' % course_id.split('/')[0]
 
 
-def _course_staff_group_name(location, course_context=None):
-    """
-    Get the name of the staff group for a location in the context of a course run.
-
-    location: something that can passed to Location
-    course_context: A course_id that specifies the course run in which the location occurs.
-        Required if location doesn't have category 'course'
-
-    cdodge: We're changing the name convention of the group to better epxress different runs of courses by
-    using course_id rather than just the course number. So first check to see if the group name exists
-    """
+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 expect now as well as the legacy
+    group name we support for backwards compatibility. This should not check
+    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)
-    legacy_name = 'staff_%s' % loc.course
-    if _does_course_group_name_exist(legacy_name):
-        return legacy_name
+    legacy_group_name = '{0}_{1}'.format(role, loc.course)
 
     if loc.category == 'course':
         course_id = loc.course_id
@@ -386,22 +380,31 @@ def _course_staff_group_name(location, course_context=None):
             raise CourseContextRequired()
         course_id = course_context
 
-    return 'staff_%s' % course_id
+    group_name = '{0}_{1}'.format(role, course_id)
 
+    return [group_name, legacy_group_name]
 
-def course_beta_test_group_name(location):
+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):
     """
-    Get the name of the beta tester group for a location.  Right now, that's
-    beta_testers_COURSE.
+    Get the name of the staff group for a location in the context of a course run.
 
-    location: something that can passed to Location.
+    location: something that can passed to Location
+    course_context: A course_id that specifies the course run in which the location occurs.
+        Required if location doesn't have category 'course'
+
+    cdodge: We're changing the name convention of the group to better epxress different runs of courses by
+    using course_id rather than just the course number. So first check to see if the group name exists
     """
-    return 'beta_testers_{0}'.format(Location(location).course)
+    loc = Location(location)
+    group_name, legacy_group_name = group_names_for_staff(location, course_context)
 
-# 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_org_instructor_group_name(location, course_context=None):
     """
@@ -437,18 +440,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):
@@ -540,23 +551,22 @@ 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)
-        # org_staff_group is a group for an entire organization
-        org_staff_group = _course_org_staff_group_name(location, course_context)
-        if staff_group in user_groups or org_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) + \
+                       [_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)
+                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)
-        instructor_staff_group = _course_org_instructor_group_name(
-            location, course_context)
-        if instructor_group in user_groups or instructor_staff_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) + \
+                            [_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)
+                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)
 

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),
@@ -250,23 +251,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')
@@ -288,7 +290,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))
@@ -307,9 +309,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
@@ -317,12 +319,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_duration' in timer_context:
                     context.update(timer_context)
@@ -363,7 +365,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,