Merge pull request #1575 from cpennington/hotfix-memory-leaks

Improve memory profile during course grading runs

common/djangoapps/student/tests/factories.py

@@ -4,9 +4,17 @@ from student.models import (User, UserProfile, Registration,
                             PendingEmailChange, UserStanding,
                             )
 from course_modes.models import CourseMode
-from django.contrib.auth.models import Group
+from django.contrib.auth.models import Group, AnonymousUser
 from datetime import datetime
-from factory import DjangoModelFactory, SubFactory, PostGenerationMethodCall, post_generation, Sequence
+from factory import (
+    DjangoModelFactory,
+    Factory,
+    LazyAttribute,
+    post_generation,
+    PostGenerationMethodCall,
+    Sequence,
+    SubFactory,
+)
 from uuid import uuid4
 from pytz import UTC
 
@@ -16,8 +24,10 @@ from pytz import UTC
 
 class GroupFactory(DjangoModelFactory):
     FACTORY_FOR = Group
+    FACTORY_DJANGO_GET_OR_CREATE = ('name', )
+
+    name = Sequence(u'group{0}'.format)
 
-    name = u'staff_MITx/999/Robot_Super_Course'
 
 
 class UserStandingFactory(DjangoModelFactory):
@@ -30,9 +40,10 @@ class UserStandingFactory(DjangoModelFactory):
 
 class UserProfileFactory(DjangoModelFactory):
     FACTORY_FOR = UserProfile
+    FACTORY_DJANGO_GET_OR_CREATE = ('user', )
 
     user = None
-    name = u'Robot Test'
+    name = LazyAttribute(u'{0.user.first_name} {0.user.last_name}'.format)
     level_of_education = None
     gender = u'm'
     mailing_address = None
@@ -59,6 +70,7 @@ class RegistrationFactory(DjangoModelFactory):
 
 class UserFactory(DjangoModelFactory):
     FACTORY_FOR = User
+    FACTORY_DJANGO_GET_OR_CREATE = ('email', 'username')
 
     username = Sequence(u'robot{0}'.format)
     email = Sequence(u'robot+test+{0}@edx.org'.format)
@@ -82,6 +94,21 @@ class UserFactory(DjangoModelFactory):
         else:
             return None
 
+    @post_generation
+    def groups(self, create, extracted, **kwargs):
+        if extracted is None:
+            return
+
+        if isinstance(extracted, basestring):
+            extracted = [extracted]
+
+        for group_name in extracted:
+            self.groups.add(GroupFactory.simple_generate(create, name=group_name))
+
+
+class AnonymousUserFactory(Factory):
+    FACTORY_FOR = AnonymousUser
+
 
 class AdminFactory(UserFactory):
     is_staff = True

lms/djangoapps/bulk_email/tasks.py

@@ -28,7 +28,7 @@ from celery.states import SUCCESS, FAILURE, RETRY
 from celery.exceptions import RetryTaskError
 
 from django.conf import settings
-from django.contrib.auth.models import User, Group
+from django.contrib.auth.models import User
 from django.core.mail import EmailMultiAlternatives, get_connection
 from django.core.urlresolvers import reverse
 
@@ -36,8 +36,8 @@ from bulk_email.models import (
     CourseEmail, Optout, CourseEmailTemplate,
     SEND_TO_MYSELF, SEND_TO_ALL, TO_OPTIONS,
 )
-from courseware.access import _course_staff_group_name, _course_instructor_group_name
 from courseware.courses import get_course, course_image_url
+from courseware.roles import CourseStaffRole, CourseInstructorRole
 from instructor_task.models import InstructorTask
 from instructor_task.subtasks import (
     SubtaskStatus,
@@ -106,12 +106,8 @@ def _get_recipient_queryset(user_id, to_option, course_id, course_location):
     if to_option == SEND_TO_MYSELF:
         recipient_qset = User.objects.filter(id=user_id)
     else:
-        staff_grpname = _course_staff_group_name(course_location)
-        staff_group, _ = Group.objects.get_or_create(name=staff_grpname)
-        staff_qset = staff_group.user_set.all()
-        instructor_grpname = _course_instructor_group_name(course_location)
-        instructor_group, _ = Group.objects.get_or_create(name=instructor_grpname)
-        instructor_qset = instructor_group.user_set.all()
+        staff_qset = CourseStaffRole(course_location).users_with_role()
+        instructor_qset = CourseInstructorRole(course_location).users_with_role()
         recipient_qset = staff_qset | instructor_qset
         if to_option == SEND_TO_ALL:
             # We also require students to have activated their accounts to

lms/djangoapps/bulk_email/tests/test_email.py

@@ -50,10 +50,10 @@ class TestEmailSendFromDashboard(ModuleStoreTestCase):
     def setUp(self):
         self.course = CourseFactory.create()
 
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
 
         # Create staff
-        self.staff = [StaffFactory(self.course) for _ in xrange(STAFF_COUNT)]
+        self.staff = [StaffFactory(course=self.course.location) for _ in xrange(STAFF_COUNT)]
 
         # Create students
         self.students = [UserFactory() for _ in xrange(STUDENT_COUNT)]

lms/djangoapps/course_wiki/tests/test_access.py

@@ -25,7 +25,7 @@ class TestWikiAccessBase(ModuleStoreTestCase):
         self.wiki = get_or_create_root()
 
         self.course_math101 = CourseFactory.create(org='org', number='math101', display_name='Course')
-        self.course_math101_staff = [InstructorFactory(self.course_math101), StaffFactory(self.course_math101)]
+        self.course_math101_staff = [InstructorFactory(course=self.course_math101.location), StaffFactory(course=self.course_math101.location)]
 
         wiki_math101 = self.create_urlpath(self.wiki, course_wiki_slug(self.course_math101))
         wiki_math101_page = self.create_urlpath(wiki_math101, 'Child')
@@ -44,9 +44,9 @@ class TestWikiAccess(TestWikiAccessBase):
         super(TestWikiAccess, self).setUp()
 
         self.course_310b = CourseFactory.create(org='org', number='310b', display_name='Course')
-        self.course_310b_staff = [InstructorFactory(self.course_310b), StaffFactory(self.course_310b)]
+        self.course_310b_staff = [InstructorFactory(course=self.course_310b.location), StaffFactory(course=self.course_310b.location)]
         self.course_310b_ = CourseFactory.create(org='org', number='310b_', display_name='Course')
-        self.course_310b__staff = [InstructorFactory(self.course_310b_), StaffFactory(self.course_310b_)]
+        self.course_310b__staff = [InstructorFactory(course=self.course_310b_.location), StaffFactory(course=self.course_310b_.location)]
 
         self.wiki_310b = self.create_urlpath(self.wiki, course_wiki_slug(self.course_310b))
         self.wiki_310b_ = self.create_urlpath(self.wiki, course_wiki_slug(self.course_310b_))
@@ -105,7 +105,7 @@ class TestWikiAccessForNumericalCourseNumber(TestWikiAccessBase):
         super(TestWikiAccessForNumericalCourseNumber, self).setUp()
 
         self.course_200 = CourseFactory.create(org='org', number='200', display_name='Course')
-        self.course_200_staff = [InstructorFactory(self.course_200), StaffFactory(self.course_200)]
+        self.course_200_staff = [InstructorFactory(course=self.course_200.location), StaffFactory(course=self.course_200.location)]
 
         wiki_200 = self.create_urlpath(self.wiki, course_wiki_slug(self.course_200))
         wiki_200_page = self.create_urlpath(wiki_200, 'Child')
@@ -126,7 +126,7 @@ class TestWikiAccessForOldFormatCourseStaffGroups(TestWikiAccessBase):
 
         self.course_math101c = CourseFactory.create(org='org', number='math101c', display_name='Course')
         Group.objects.get_or_create(name='instructor_math101c')
-        self.course_math101c_staff = [InstructorFactory(self.course_math101c), StaffFactory(self.course_math101c)]
+        self.course_math101c_staff = [InstructorFactory(course=self.course_math101c.location), StaffFactory(course=self.course_math101c.location)]
 
         wiki_math101c = self.create_urlpath(self.wiki, course_wiki_slug(self.course_math101c))
         wiki_math101c_page = self.create_urlpath(wiki_math101c, 'Child')

lms/djangoapps/courseware/roles.py

+"""
+Classes used to model the roles used in the courseware. Each role is responsible for checking membership,
+adding users, removing users, and listing members
+"""
+
+from abc import ABCMeta, abstractmethod
+from functools import partial
+
+from django.contrib.auth.models import User, Group
+
+from xmodule.modulestore import Location
+
+
+class CourseContextRequired(Exception):
+    """
+    Raised when a course_context is required to determine permissions
+    """
+    pass
+
+
+class AccessRole(object):
+    """
+    Object representing a role with particular access to a resource
+    """
+    __metaclass__ = ABCMeta
+
+    @abstractmethod
+    def has_user(self, user):  # pylint: disable=unused-argument
+        """
+        Return whether the supplied django user has access to this role.
+        """
+        return False
+
+    @abstractmethod
+    def add_users(self, *users):
+        """
+        Add the role to the supplied django users.
+        """
+        pass
+
+    @abstractmethod
+    def remove_users(self, *users):
+        """
+        Remove the role from the supplied django users.
+        """
+        pass
+
+    @abstractmethod
+    def users_with_role(self):
+        """
+        Return a django QuerySet for all of the users with this role
+        """
+        return User.objects.none()
+
+
+class GlobalStaff(AccessRole):
+    """
+    The global staff role
+    """
+    def has_user(self, user):
+        return user.is_staff
+
+    def add_users(self, *users):
+        for user in users:
+            user.is_staff = True
+            user.save()
+
+    def remove_users(self, *users):
+        for user in users:
+            user.is_staff = False
+            user.save()
+
+    def users_with_role(self):
+        raise Exception("This operation is un-indexed, and shouldn't be used")
+
+
+class GroupBasedRole(AccessRole):
+    """
+    A role based on membership to any of a set of groups.
+    """
+    def __init__(self, group_names):
+        """
+        Create a GroupBasedRole from a list of group names
+
+        The first element of `group_names` will be the preferred group
+        to use when adding a user to this Role.
+
+        If a user is a member of any of the groups in the list, then
+        they will be consider a member of the Role
+        """
+        self._group_names = [name.lower() for name in group_names]
+
+    def has_user(self, user):
+        """
+        Return whether the supplied django user has access to this role.
+        """
+        # pylint: disable=protected-access
+        if not user.is_authenticated():
+            return False
+
+        if not hasattr(user, '_groups'):
+            user._groups = set(name.lower() for name in user.groups.values_list('name', flat=True))
+
+        return len(user._groups.intersection(self._group_names)) > 0
+
+    def add_users(self, *users):
+        """
+        Add the supplied django users to this role.
+        """
+        group, _ = Group.objects.get_or_create(name=self._group_names[0])
+        group.user_set.add(*users)
+        for user in users:
+            if hasattr(user, '_groups'):
+                del user._groups
+
+    def remove_users(self, *users):
+        """
+        Remove the supplied django users from this role.
+        """
+        group, _ = Group.objects.get_or_create(name=self._group_names[0])
+        group.user_set.remove(*users)
+        for user in users:
+            if hasattr(user, '_groups'):
+                del user._groups
+
+    def users_with_role(self):
+        """
+        Return a django QuerySet for all of the users with this role
+        """
+        return User.objects.filter(groups__name__in=self._group_names)
+
+
+class CourseRole(GroupBasedRole):
+    """
+    A named role in a particular course
+    """
+    def __init__(self, role, location, course_context=None):
+        # pylint: disable=no-member
+        loc = Location(location)
+        legacy_group_name = '{0}_{1}'.format(role, loc.course)
+
+        if loc.category.lower() == '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)
+
+        super(CourseRole, self).__init__([group_name, legacy_group_name])
+
+
+class OrgRole(GroupBasedRole):
+    """
+    A named role in a particular org
+    """
+    def __init__(self, role, location):
+        # pylint: disable=no-member
+
+        location = Location(location)
+        super(OrgRole, self).__init__(['{}_{}'.format(role, location.org)])
+
+
+class CourseStaffRole(CourseRole):
+    """A Staff member of a course"""
+    def __init__(self, *args, **kwargs):
+        super(CourseStaffRole, self).__init__('staff', *args, **kwargs)
+
+
+class CourseInstructorRole(CourseRole):
+    """A course Instructor"""
+    def __init__(self, *args, **kwargs):
+        super(CourseInstructorRole, self).__init__('instructor', *args, **kwargs)
+
+
+class CourseBetaTesterRole(CourseRole):
+    """A course Beta Tester"""
+    def __init__(self, *args, **kwargs):
+        super(CourseBetaTesterRole, self).__init__('beta_testers', *args, **kwargs)
+
+
+class OrgStaffRole(OrgRole):
+    """An organization staff member"""
+    def __init__(self, *args, **kwargs):
+        super(OrgStaffRole, self).__init__('staff', *args, **kwargs)
+
+
+class OrgInstructorRole(OrgRole):
+    """An organization staff member"""
+    def __init__(self, *args, **kwargs):
+        super(OrgInstructorRole, self).__init__('staff', *args, **kwargs)

lms/djangoapps/courseware/tests/factories.py

-from datetime import datetime
 import json
 from functools import partial
 
-from factory import DjangoModelFactory, SubFactory
-from student.tests.factories import UserFactory as StudentUserFactory
-from student.tests.factories import GroupFactory as StudentGroupFactory
+from factory import DjangoModelFactory, SubFactory, post_generation
+
+# Imported to re-export
+# pylint: disable=unused-import
+from student.tests.factories import UserFactory  # Imported to re-export
+from student.tests.factories import GroupFactory  # Imported to re-export
+from student.tests.factories import CourseEnrollmentAllowedFactory  # Imported to re-export
+from student.tests.factories import RegistrationFactory  # Imported to re-export
+# pylint: enable=unused-import
+
 from student.tests.factories import UserProfileFactory as StudentUserProfileFactory
-from student.tests.factories import CourseEnrollmentAllowedFactory as StudentCourseEnrollmentAllowedFactory
-from student.tests.factories import RegistrationFactory as StudentRegistrationFactory
 from courseware.models import StudentModule, XModuleUserStateSummaryField
 from courseware.models import XModuleStudentInfoField, XModuleStudentPrefsField
-from instructor.access import allow_access
+from courseware.roles import CourseInstructorRole, CourseStaffRole
 
 from xmodule.modulestore import Location
-from pytz import UTC
+
 
 location = partial(Location, 'i4x', 'edX', 'test_course', 'problem')
 
 
 class UserProfileFactory(StudentUserProfileFactory):
-    name = 'Robot Studio'
     courseware = 'course.xml'
 
 
-class RegistrationFactory(StudentRegistrationFactory):
-    pass
-
-
-class UserFactory(StudentUserFactory):
-    email = 'robot@edx.org'
-    last_name = 'Tester'
-    last_login = datetime.now(UTC)
-    date_joined = datetime.now(UTC)
-
-
-def InstructorFactory(course):  # pylint: disable=invalid-name
+class InstructorFactory(UserFactory):
     """
-    Given a course object, returns a User object with instructor
+    Given a course Location, returns a User object with instructor
     permissions for `course`.
     """
-    user = StudentUserFactory.create(last_name="Instructor")
-    allow_access(course, user, "instructor")
-    return user
+    last_name = "Instructor"
+
+    @post_generation
+    def course(self, create, extracted, **kwargs):
+        if extracted is None:
+            raise ValueError("Must specify a course location for a course instructor user")
+        CourseInstructorRole(extracted).add_users(self)
 
 
-def StaffFactory(course):  # pylint: disable=invalid-name
+class StaffFactory(UserFactory):
     """
-    Given a course object, returns a User object with staff
+    Given a course Location, returns a User object with staff
     permissions for `course`.
     """
-    user = StudentUserFactory.create(last_name="Staff")
-    allow_access(course, user, "staff")
-    return user
-
-
-class GroupFactory(StudentGroupFactory):
-    name = 'test_group'
-
+    last_name = "Staff"
 
-class CourseEnrollmentAllowedFactory(StudentCourseEnrollmentAllowedFactory):
-    pass
+    @post_generation
+    def course(self, create, extracted, **kwargs):
+        if extracted is None:
+            raise ValueError("Must specify a course location for a course staff user")
+        CourseStaffRole(extracted).add_users(self)
 
 
 class StudentModuleFactory(DjangoModelFactory):

lms/djangoapps/courseware/tests/test_access.py

+import courseware.access as access
+import datetime
+
 from mock import Mock
 
 from django.test import TestCase
 from django.test.utils import override_settings
 
+from courseware.tests.factories import UserFactory, CourseEnrollmentAllowedFactory, StaffFactory, InstructorFactory
+from student.tests.factories import AnonymousUserFactory
 from xmodule.modulestore import Location
-import courseware.access as access
 from courseware.tests.tests import TEST_DATA_MIXED_MODULESTORE
-from .factories import CourseEnrollmentAllowedFactory
-import datetime
 import pytz
 
 
+# pylint: disable=protected-access
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 class AccessTestCase(TestCase):
     """
     Tests for the various access controls on the student dashboard
     """
-    def test__has_global_staff_access(self):
-        u = Mock(is_staff=False)
-        self.assertFalse(access._has_global_staff_access(u))
 
-        u = Mock(is_staff=True)
-        self.assertTrue(access._has_global_staff_access(u))
+    def setUp(self):
+        self.course = Location('i4x://edX/toy/course/2012_Fall')
+        self.anonymous_user = AnonymousUserFactory()
+        self.student = UserFactory()
+        self.global_staff = UserFactory(is_staff=True)
+        self.course_staff = StaffFactory(course=self.course)
+        self.course_instructor = InstructorFactory(course=self.course)
 
     def test__has_access_to_location(self):
-        location = Location('i4x://edX/toy/course/2012_Fall')
+        self.assertFalse(access._has_access_to_location(None, self.course, 'staff', None))
+
+        self.assertFalse(access._has_access_to_location(self.anonymous_user, self.course, 'staff', None))
+        self.assertFalse(access._has_access_to_location(self.anonymous_user, self.course, 'instructor', None))
+
+        self.assertTrue(access._has_access_to_location(self.global_staff, self.course, 'staff', None))
+        self.assertTrue(access._has_access_to_location(self.global_staff, self.course, 'instructor', None))
 
-        self.assertFalse(access._has_access_to_location(None, location,
-                                                        'staff', None))
-        u = Mock()
-        u.is_authenticated.return_value = False
-        self.assertFalse(access._has_access_to_location(u, location,
-                                                        'staff', None))
-        u = Mock(is_staff=True)
-        self.assertTrue(access._has_access_to_location(u, location,
-                                                       'instructor', None))
         # A user has staff access if they are in the staff group
-        u = Mock(is_staff=False)
-        g = Mock()
-        g.name = 'staff_edX/toy/2012_Fall'
-        u.groups.all.return_value = [g]
-        self.assertTrue(access._has_access_to_location(u, location,
-                                                        'staff', None))
-        # A user has staff access if they are in the instructor group
-        g.name = 'instructor_edX/toy/2012_Fall'
-        self.assertTrue(access._has_access_to_location(u, location,
-                                                        'staff', None))
-
-        # A user has instructor access if they are in the instructor group
-        g.name = 'instructor_edX/toy/2012_Fall'
-        self.assertTrue(access._has_access_to_location(u, location,
-                                                        'instructor', None))
-
-        # A user does not have staff access if they are
+        self.assertTrue(access._has_access_to_location(self.course_staff, self.course, 'staff', None))
+        self.assertFalse(access._has_access_to_location(self.course_staff, self.course, 'instructor', None))
+
+        # A user has staff and instructor access if they are in the instructor group
+        self.assertTrue(access._has_access_to_location(self.course_instructor, self.course, 'staff', None))
+        self.assertTrue(access._has_access_to_location(self.course_instructor, self.course, 'instructor', None))
+
+        # A user does not have staff or instructor access if they are
         # not in either the staff or the the instructor group
-        g.name = 'student_only'
-        self.assertFalse(access._has_access_to_location(u, location,
-                                                        'staff', None))
-
-        # A user does not have instructor access if they are
-        # not in the instructor group
-        g.name = 'student_only'
-        self.assertFalse(access._has_access_to_location(u, location,
-                                                        'instructor', None))
+        self.assertFalse(access._has_access_to_location(self.student, self.course, 'staff', None))
+        self.assertFalse(access._has_access_to_location(self.student, self.course, 'instructor', None))
 
     def test__has_access_string(self):
         u = Mock(is_staff=True)

lms/djangoapps/courseware/tests/test_masquerade.py

@@ -12,11 +12,11 @@ import json
 
 from django.test.utils import override_settings
 from django.core.urlresolvers import reverse
-from django.contrib.auth.models import Group, User
+from django.contrib.auth.models import User
 
-from courseware.access import _course_staff_group_name
 from courseware.tests.helpers import LoginEnrollmentTestCase
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
+from courseware.roles import CourseStaffRole
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.django import modulestore, clear_existing_modulestores
 from lms.lib.xblock.runtime import quote_slashes
@@ -42,9 +42,7 @@ class TestStaffMasqueradeAsStudent(ModuleStoreTestCase, LoginEnrollmentTestCase)
         self.activate_user(self.instructor)
 
         def make_instructor(course):
-            group_name = _course_staff_group_name(course.location)
-            g = Group.objects.create(name=group_name)
-            g.user_set.add(User.objects.get(email=self.instructor))
+            CourseStaffRole(course.location).add_users(User.objects.get(email=self.instructor))
 
         make_instructor(self.graded_course)
 

lms/djangoapps/courseware/tests/test_roles.py

+"""
+Tests of courseware.roles
+"""
+
+from django.test import TestCase
+
+from xmodule.modulestore import Location
+from courseware.tests.factories import UserFactory, StaffFactory, InstructorFactory
+from student.tests.factories import AnonymousUserFactory
+
+from courseware.roles import GlobalStaff, CourseRole
+
+
+class RolesTestCase(TestCase):
+    """
+    Tests of courseware.roles
+    """
+
+    def setUp(self):
+        self.course = Location('i4x://edX/toy/course/2012_Fall')
+        self.anonymous_user = AnonymousUserFactory()
+        self.student = UserFactory()
+        self.global_staff = UserFactory(is_staff=True)
+        self.course_staff = StaffFactory(course=self.course)
+        self.course_instructor = InstructorFactory(course=self.course)
+
+    def test_global_staff(self):
+        self.assertFalse(GlobalStaff().has_user(self.student))
+        self.assertFalse(GlobalStaff().has_user(self.course_staff))
+        self.assertFalse(GlobalStaff().has_user(self.course_instructor))
+        self.assertTrue(GlobalStaff().has_user(self.global_staff))
+
+    def test_group_name_case_insensitive(self):
+        uppercase_loc = "i4x://ORG/COURSE/course/NAME"
+        lowercase_loc = uppercase_loc.lower()
+
+        lowercase_group = "role_org/course/name"
+        uppercase_group = lowercase_group.upper()
+
+        lowercase_user = UserFactory(groups=lowercase_group)
+        uppercase_user = UserFactory(groups=uppercase_group)
+
+        self.assertTrue(CourseRole("role", lowercase_loc).has_user(lowercase_user))
+        self.assertTrue(CourseRole("role", uppercase_loc).has_user(lowercase_user))
+        self.assertTrue(CourseRole("role", lowercase_loc).has_user(uppercase_user))
+        self.assertTrue(CourseRole("role", uppercase_loc).has_user(uppercase_user))
+

lms/djangoapps/courseware/tests/test_view_authentication.py

@@ -3,19 +3,19 @@ import pytz
 
 from mock import patch
 
-from django.contrib.auth.models import User, Group
+from django.contrib.auth.models import User
 from django.core.urlresolvers import reverse
 from django.test.utils import override_settings
 
 # Need access to internal func to put users in the right group
-from courseware.access import (has_access, _course_staff_group_name,
-                               course_beta_test_group_name, settings as access_settings)
+from courseware.access import has_access
+from courseware.roles import CourseBetaTesterRole, CourseInstructorRole, CourseStaffRole, GlobalStaff
 
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 
-from helpers import LoginEnrollmentTestCase, check_for_get_code
+from courseware.tests.helpers import LoginEnrollmentTestCase, check_for_get_code
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
 
 
@@ -173,9 +173,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         email, password = self.ACCOUNT_INFO[1]
 
         # Make the instructor staff in self.course
-        group_name = _course_staff_group_name(self.course.location)
-        group = Group.objects.create(name=group_name)
-        group.user_set.add(User.objects.get(email=email))
+        CourseInstructorRole(self.course.location).add_users(User.objects.get(email=email))
 
         self.login(email, password)
 
@@ -206,7 +204,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         for url in urls:
             check_for_get_code(self, 200, url)
 
-    @patch.dict(access_settings.MITX_FEATURES, {'DISABLE_START_DATES': False})
+    @patch.dict('courseware.access.settings.MITX_FEATURES', {'DISABLE_START_DATES': False})
     def test_dark_launch_enrolled_student(self):
         """
         Make sure that before course start, students can't access course
@@ -237,7 +235,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self._check_non_staff_light(self.test_course)
         self._check_non_staff_dark(self.test_course)
 
-    @patch.dict(access_settings.MITX_FEATURES, {'DISABLE_START_DATES': False})
+    @patch.dict('courseware.access.settings.MITX_FEATURES', {'DISABLE_START_DATES': False})
     def test_dark_launch_instructor(self):
         """
         Make sure that before course start instructors can access the
@@ -253,9 +251,8 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.test_course = self.update_course(self.test_course, test_course_data)
 
         # Make the instructor staff in  self.course
-        group_name = _course_staff_group_name(self.course.location)
-        group = Group.objects.create(name=group_name)
-        group.user_set.add(User.objects.get(email=instructor_email))
+        CourseStaffRole(self.course.location).add_users(User.objects.get(email=instructor_email))
+
         self.logout()
         self.login(instructor_email, instructor_password)
         # Enroll in the classes---can't see courseware otherwise.
@@ -267,7 +264,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self._check_non_staff_dark(self.test_course)
         self._check_staff(self.course)
 
-    @patch.dict(access_settings.MITX_FEATURES, {'DISABLE_START_DATES': False})
+    @patch.dict('courseware.access.settings.MITX_FEATURES', {'DISABLE_START_DATES': False})
     def test_dark_launch_staff(self):
         """
         Make sure that before course start staff can access
@@ -295,7 +292,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self._check_staff(self.course)
         self._check_staff(self.test_course)
 
-    @patch.dict(access_settings.MITX_FEATURES, {'DISABLE_START_DATES': False})
+    @patch.dict('courseware.access.settings.MITX_FEATURES', {'DISABLE_START_DATES': False})
     def test_enrollment_period(self):
         """
         Check that enrollment periods work.
@@ -323,25 +320,22 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.assertTrue(self.enroll(self.test_course))
 
         # Make the instructor staff in the self.course
-        group_name = _course_staff_group_name(self.course.location)
-        group = Group.objects.create(name=group_name)
-        group.user_set.add(User.objects.get(email=instructor_email))
+        instructor_role = CourseInstructorRole(self.course.location)
+        instructor_role.add_users(User.objects.get(email=instructor_email))
 
         self.logout()
         self.login(instructor_email, instructor_password)
         self.assertTrue(self.enroll(self.course))
 
         # now make the instructor global staff, but not in the instructor group
-        group.user_set.remove(User.objects.get(email=instructor_email))
-        instructor = User.objects.get(email=instructor_email)
-        instructor.is_staff = True
-        instructor.save()
+        instructor_role.remove_users(User.objects.get(email=instructor_email))
+        GlobalStaff().add_users(User.objects.get(email=instructor_email))
 
         # unenroll and try again
         self.unenroll(self.course)
         self.assertTrue(self.enroll(self.course))
 
-    @patch.dict(access_settings.MITX_FEATURES, {'DISABLE_START_DATES': False})
+    @patch.dict('courseware.access.settings.MITX_FEATURES', {'DISABLE_START_DATES': False})
     def test_beta_period(self):
         """
         Check that beta-test access works.
@@ -366,9 +360,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.assertFalse(has_access(student_user, self.course, 'load'))
 
         # now add the student to the beta test group
-        group_name = course_beta_test_group_name(self.course.location)
-        group = Group.objects.create(name=group_name)
-        group.user_set.add(student_user)
+        CourseBetaTesterRole(self.course.location).add_users(student_user)
 
         # now the student should see it
         self.assertTrue(has_access(student_user, self.course, 'load'))

lms/djangoapps/instructor/access.py

@@ -10,13 +10,18 @@ TO DO sync instructor and staff flags
 """
 
 import logging
-from django.contrib.auth.models import Group
-from courseware.access import (get_access_group_name,
-                               course_beta_test_group_name)
 from django_comment_common.models import Role
 
+from courseware.roles import CourseBetaTesterRole, CourseInstructorRole, CourseStaffRole
+
 log = logging.getLogger(__name__)
 
+ROLES = {
+    'beta': CourseBetaTesterRole,
+    'instructor': CourseInstructorRole,
+    'staff': CourseStaffRole,
+}
+
 
 def list_with_level(course, level):
     """
@@ -26,16 +31,7 @@ def list_with_level(course, level):
     There could be other levels specific to the course.
     If there is no Group for that course-level, returns an empty list
     """
-    if level == 'beta':
-        grpname = course_beta_test_group_name(course.location)
-    else:
-        grpname = get_access_group_name(course, level)
-
-    try:
-        return Group.objects.get(name=grpname).user_set.all()
-    except Group.DoesNotExist:
-        log.info("list_with_level called with non-existant group named {}".format(grpname))
-        return []
+    return ROLES[level](course.location).users_with_role()
 
 
 def allow_access(course, user, level):
@@ -66,18 +62,15 @@ def _change_access(course, user, level, action):
     NOTE: will create a group if it does not yet exist.
     """
 
-    if level == 'beta':
-        grpname = course_beta_test_group_name(course.location)
-    elif level in ['instructor', 'staff']:
-        grpname = get_access_group_name(course, level)
-    else:
+    try:
+        role = ROLES[level](course.location)
+    except KeyError:
         raise ValueError("unrecognized level '{}'".format(level))
-    group, _ = Group.objects.get_or_create(name=grpname)
 
     if action == 'allow':
-        user.groups.add(group)
+        role.add_users(user)
     elif action == 'revoke':
-        user.groups.remove(group)
+        role.remove_users(user)
     else:
         raise ValueError("unrecognized action '{}'".format(action))
 

lms/djangoapps/instructor/tests/test_access.py

@@ -3,15 +3,14 @@ Test instructor.access
 """
 
 from nose.tools import raises
-from django.contrib.auth.models import Group
 from student.tests.factories import UserFactory
 from xmodule.modulestore.tests.factories import CourseFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 from django.test.utils import override_settings
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
+from courseware.roles import CourseBetaTesterRole, CourseStaffRole
 
-from courseware.access import get_access_group_name
 from django_comment_common.models import (Role,
                                           FORUM_ROLE_MODERATOR)
 from instructor.access import (allow_access,
@@ -51,39 +50,29 @@ class TestInstructorAccessAllow(ModuleStoreTestCase):
     def test_allow(self):
         user = UserFactory()
         allow_access(self.course, user, 'staff')
-        group = Group.objects.get(
-            name=get_access_group_name(self.course, 'staff')
-        )
-        self.assertIn(user, group.user_set.all())
+        self.assertTrue(CourseStaffRole(self.course.location).has_user(user))
 
     def test_allow_twice(self):
         user = UserFactory()
         allow_access(self.course, user, 'staff')
         allow_access(self.course, user, 'staff')
-        group = Group.objects.get(
-            name=get_access_group_name(self.course, 'staff')
-        )
-        self.assertIn(user, group.user_set.all())
+        self.assertTrue(CourseStaffRole(self.course.location).has_user(user))
 
     def test_allow_beta(self):
         """ Test allow beta against list beta. """
         user = UserFactory()
         allow_access(self.course, user, 'beta')
-        self.assertIn(user, list_with_level(self.course, 'beta'))
+        self.assertTrue(CourseBetaTesterRole(self.course.location).has_user(user))
 
     @raises(ValueError)
     def test_allow_badlevel(self):
         user = UserFactory()
         allow_access(self.course, user, 'robot-not-a-level')
-        group = Group.objects.get(name=get_access_group_name(self.course, 'robot-not-a-level'))
-        self.assertIn(user, group.user_set.all())
 
     @raises(Exception)
     def test_allow_noneuser(self):
         user = None
         allow_access(self.course, user, 'staff')
-        group = Group.objects.get(name=get_access_group_name(self.course, 'staff'))
-        self.assertIn(user, group.user_set.all())
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
@@ -102,32 +91,22 @@ class TestInstructorAccessRevoke(ModuleStoreTestCase):
     def test_revoke(self):
         user = self.staff[0]
         revoke_access(self.course, user, 'staff')
-        group = Group.objects.get(
-            name=get_access_group_name(self.course, 'staff')
-        )
-        self.assertNotIn(user, group.user_set.all())
+        self.assertFalse(CourseStaffRole(self.course.location).has_user(user))
 
     def test_revoke_twice(self):
         user = self.staff[0]
         revoke_access(self.course, user, 'staff')
-        group = Group.objects.get(
-            name=get_access_group_name(self.course, 'staff')
-        )
-        self.assertNotIn(user, group.user_set.all())
+        self.assertFalse(CourseStaffRole(self.course.location).has_user(user))
 
     def test_revoke_beta(self):
         user = self.beta_testers[0]
         revoke_access(self.course, user, 'beta')
-        self.assertNotIn(user, list_with_level(self.course, 'beta'))
+        self.assertFalse(CourseBetaTesterRole(self.course.location).has_user(user))
 
     @raises(ValueError)
     def test_revoke_badrolename(self):
         user = UserFactory()
         revoke_access(self.course, user, 'robot-not-a-level')
-        group = Group.objects.get(
-            name=get_access_group_name(self.course, 'robot-not-a-level')
-        )
-        self.assertNotIn(user, group.user_set.all())
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)

lms/djangoapps/instructor/tests/test_api.py

@@ -183,7 +183,7 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase):
         """
         Ensure that a staff member can't access instructor endpoints.
         """
-        staff_member = StaffFactory(self.course)
+        staff_member = StaffFactory(course=self.course.location)
         CourseEnrollment.enroll(staff_member, self.course.id)
         self.client.login(username=staff_member.username, password='test')
         # Try to promote to forums admin - not working
@@ -212,7 +212,7 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase):
         """
         Ensure that an instructor member can access all endpoints.
         """
-        inst = InstructorFactory(self.course)
+        inst = InstructorFactory(course=self.course.location)
         CourseEnrollment.enroll(inst, self.course.id)
         self.client.login(username=inst.username, password='test')
 
@@ -249,7 +249,7 @@ class TestInstructorAPIEnrollment(ModuleStoreTestCase, LoginEnrollmentTestCase):
     """
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
 
         self.enrolled_student = UserFactory()
@@ -376,7 +376,7 @@ class TestInstructorAPILevelsAccess(ModuleStoreTestCase, LoginEnrollmentTestCase
     """
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
 
         self.other_instructor = UserFactory()
@@ -513,7 +513,7 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa
     """
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
 
         self.students = [UserFactory() for _ in xrange(6)]
@@ -650,7 +650,7 @@ class TestInstructorAPIRegradeTask(ModuleStoreTestCase, LoginEnrollmentTestCase)
     """
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
 
         self.student = UserFactory()
@@ -794,7 +794,7 @@ class TestInstructorSendEmail(ModuleStoreTestCase, LoginEnrollmentTestCase):
     """
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
         test_subject = u'\u1234 test subject'
         test_message = u'\u6824 test message'
@@ -916,7 +916,7 @@ class TestInstructorAPITaskLists(ModuleStoreTestCase, LoginEnrollmentTestCase):
 
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
 
         self.student = UserFactory()
@@ -1047,7 +1047,7 @@ class TestInstructorAPIAnalyticsProxy(ModuleStoreTestCase, LoginEnrollmentTestCa
 
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = InstructorFactory(self.course)
+        self.instructor = InstructorFactory(course=self.course.location)
         self.client.login(username=self.instructor.username, password='test')
 
     @patch.object(instructor.views.api.requests, 'get')

lms/djangoapps/instructor/tests/test_legacy_anon_csv.py

@@ -11,16 +11,15 @@ Notes for running by hand:
 from django.test.utils import override_settings
 
 # Need access to internal func to put users in the right group
-from django.contrib.auth.models import Group, User
+from django.contrib.auth.models import User
 
 from django.core.urlresolvers import reverse
 
-from courseware.access import _course_staff_group_name
 from courseware.tests.helpers import LoginEnrollmentTestCase
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
+from courseware.roles import CourseStaffRole
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.django import modulestore, clear_existing_modulestores
-import xmodule.modulestore.django
 
 from mock import patch
 
@@ -46,10 +45,8 @@ class TestInstructorDashboardAnonCSV(ModuleStoreTestCase, LoginEnrollmentTestCas
         self.activate_user(self.instructor)
 
         def make_instructor(course):
-            """ Create an instructor for the course. """
-            group_name = _course_staff_group_name(course.location)
-            group = Group.objects.create(name=group_name)
-            group.user_set.add(User.objects.get(email=self.instructor))
+            """ Create an instructor for the course."""
+            CourseStaffRole(course.location).add_users(User.objects.get(email=self.instructor))
 
         make_instructor(self.toy)
 
@@ -68,4 +65,3 @@ class TestInstructorDashboardAnonCSV(ModuleStoreTestCase, LoginEnrollmentTestCas
         self.assertEqual(response['Content-Type'], 'text/csv')
         body = response.content.replace('\r', '')
         self.assertEqual(body, '"User ID","Anonymized user ID"\n"2","42"\n')
-

lms/djangoapps/instructor/tests/test_legacy_download_csv.py

@@ -11,16 +11,15 @@ Notes for running by hand:
 from django.test.utils import override_settings
 
 # Need access to internal func to put users in the right group
-from django.contrib.auth.models import Group, User
+from django.contrib.auth.models import User
 
 from django.core.urlresolvers import reverse
 
-from courseware.access import _course_staff_group_name
 from courseware.tests.helpers import LoginEnrollmentTestCase
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
+from courseware.roles import CourseStaffRole
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.django import modulestore, clear_existing_modulestores
-import xmodule.modulestore.django
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
@@ -44,9 +43,7 @@ class TestInstructorDashboardGradeDownloadCSV(ModuleStoreTestCase, LoginEnrollme
 
         def make_instructor(course):
             """ Create an instructor for the course. """
-            group_name = _course_staff_group_name(course.location)
-            group = Group.objects.create(name=group_name)
-            group.user_set.add(User.objects.get(email=self.instructor))
+            CourseStaffRole(course.location).add_users(User.objects.get(email=self.instructor))
 
         make_instructor(self.toy)
 

lms/djangoapps/instructor/tests/test_legacy_enrollment.py

@@ -195,7 +195,7 @@ class TestInstructorEnrollsStudent(ModuleStoreTestCase, LoginEnrollmentTestCase)
         course = self.course
 
         # Create activated, but not enrolled, user
-        UserFactory.create(username="student3_0", email="student3_0@test.com")
+        UserFactory.create(username="student3_0", email="student3_0@test.com", first_name='Autoenrolled')
 
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
         response = self.client.post(url, {'action': 'Enroll multiple students', 'multiple_students': 'student3_0@test.com, student3_1@test.com, student3_2@test.com', 'auto_enroll': 'on', 'email_students': 'on'})
@@ -215,12 +215,12 @@ class TestInstructorEnrollsStudent(ModuleStoreTestCase, LoginEnrollmentTestCase)
         )
         self.assertEqual(
             mail.outbox[0].body,
-            "Dear Robot Test\n\nYou have been enrolled in Robot Super Course "
+            "Dear Autoenrolled Test\n\nYou have been enrolled in Robot Super Course "
             "at edx.org by a member of the course staff. "
             "The course should now appear on your edx.org dashboard.\n\n"
             "To start accessing course materials, please visit "
             "https://edx.org/courses/MITx/999/Robot_Super_Course\n\n"
-            "----\nThis email was automatically sent from edx.org to Robot Test"
+            "----\nThis email was automatically sent from edx.org to Autoenrolled Test"
         )
 
         self.assertEqual(

lms/djangoapps/instructor/tests/test_legacy_forum_admin.py

@@ -6,16 +6,16 @@ Unit tests for instructor dashboard forum administration
 from django.test.utils import override_settings
 
 # Need access to internal func to put users in the right group
-from django.contrib.auth.models import Group, User
+from django.contrib.auth.models import User
 
 from django.core.urlresolvers import reverse
 from django_comment_common.models import Role, FORUM_ROLE_ADMINISTRATOR, \
     FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_STUDENT
 from django_comment_client.utils import has_forum_access
 
-from courseware.access import _course_staff_group_name
 from courseware.tests.helpers import LoginEnrollmentTestCase
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
+from courseware.roles import CourseStaffRole
 from xmodule.modulestore.django import modulestore, clear_existing_modulestores
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
@@ -54,9 +54,7 @@ class TestInstructorDashboardForumAdmin(ModuleStoreTestCase, LoginEnrollmentTest
         self.activate_user(self.student)
         self.activate_user(self.instructor)
 
-        group_name = _course_staff_group_name(self.toy.location)
-        g = Group.objects.create(name=group_name)
-        g.user_set.add(User.objects.get(email=self.instructor))
+        CourseStaffRole(self.toy.location).add_users(User.objects.get(email=self.instructor))
 
         self.logout()
         self.login(self.instructor, self.password)

lms/djangoapps/open_ended_grading/tests.py

@@ -8,7 +8,7 @@ import json
 import logging
 
 from django.conf import settings
-from django.contrib.auth.models import Group, User
+from django.contrib.auth.models import User
 from django.core.urlresolvers import reverse
 from django.test.utils import override_settings
 from mock import MagicMock, patch, Mock
@@ -22,11 +22,11 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.open_ended_grading_classes import peer_grading_service, controller_query_service
 from xmodule.tests import test_util_open_ended
 
-from courseware.access import _course_staff_group_name
 from courseware.tests import factories
 from courseware.tests.helpers import LoginEnrollmentTestCase, check_for_get_code, check_for_post_code
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
 from lms.lib.xblock.runtime import LmsModuleSystem
+from courseware.roles import CourseStaffRole
 from mitxmako.shortcuts import render_to_string
 from student.models import unique_id_for_user
 
@@ -52,9 +52,7 @@ def make_instructor(course, user_email):
     """
     Makes a given user an instructor in a course.
     """
-    group_name = _course_staff_group_name(course.location)
-    group = Group.objects.create(name=group_name)
-    group.user_set.add(User.objects.get(email=user_email))
+    CourseStaffRole(course.location).add_users(User.objects.get(email=user_email))
 
 
 class StudentProblemListMockQuery(object):