Factor out the access group generation so we can check staff/instructor status without DB lookup.

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)