diff --git a/common/djangoapps/student/tests/factories.py b/common/djangoapps/student/tests/factories.py
index 9eee0d92b1..2221036647 100644
--- a/common/djangoapps/student/tests/factories.py
+++ b/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
diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py
index 68f5124003..0a7f8c10c7 100644
--- a/lms/djangoapps/bulk_email/tasks.py
+++ b/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
diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py
index 59464ff500..4bb7299f5b 100644
--- a/lms/djangoapps/bulk_email/tests/test_email.py
+++ b/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)]
diff --git a/lms/djangoapps/course_wiki/tests/test_access.py b/lms/djangoapps/course_wiki/tests/test_access.py
index e09f55caa5..72a0e7dcbd 100644
--- a/lms/djangoapps/course_wiki/tests/test_access.py
+++ b/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')
diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py
index 7836ab8bbc..4b50c559de 100644
--- a/lms/djangoapps/courseware/access.py
+++ b/lms/djangoapps/courseware/access.py
@@ -18,19 +18,16 @@ from external_auth.models import ExternalAuthMap
from courseware.masquerade import is_masquerading_as_student
from django.utils.timezone import UTC
from student.models import CourseEnrollment
+from courseware.roles import (
+ GlobalStaff, CourseStaffRole, CourseInstructorRole,
+ OrgStaffRole, OrgInstructorRole, CourseBetaTesterRole
+)
DEBUG_ACCESS = False
log = logging.getLogger(__name__)
-class CourseContextRequired(Exception):
- """
- Raised when a course_context is required to determine permissions
- """
- pass
-
-
def debug(*args, **kwargs):
# to avoid overly verbose output, this is off by default
if DEBUG_ACCESS:
@@ -91,22 +88,6 @@ def has_access(user, obj, action, course_context=None):
.format(type(obj)))
-def get_access_group_name(obj, action):
- '''
- Returns group name for user group which has "action" access to the given object.
-
- Used in managing access lists.
- '''
-
- if isinstance(obj, CourseDescriptor):
- return _get_access_group_name_course_desc(obj, action)
-
- # Passing an unknown object here is a coding error, so rather than
- # returning a default, complain.
- raise TypeError("Unknown object type in get_access_group_name(): '{0}'"
- .format(type(obj)))
-
-
# ================ Implementation helpers ================================
def _has_access_course_desc(user, course, action):
"""
@@ -214,20 +195,6 @@ def _has_access_course_desc(user, course, action):
return _dispatch(checkers, action, user, course)
-def _get_access_group_name_course_desc(course, action):
- '''
- Return name of group which gives staff access to course. Only understands action = 'staff' and 'instructor'
- '''
- if action == 'staff':
- return _course_staff_group_name(course.location)
- elif action == 'instructor':
- return _course_instructor_group_name(course.location)
-
- return []
-
-
-
-
def _has_access_error_desc(user, descriptor, action, course_context):
"""
Only staff should see error descriptors.
@@ -340,11 +307,11 @@ def _has_access_string(user, perm, action, course_context):
if perm != 'global':
debug("Deny: invalid permission '%s'", perm)
return False
- return _has_global_staff_access(user)
+ return GlobalStaff().has_user(user)
checkers = {
'staff': check_staff
- }
+ }
return _dispatch(checkers, action, user, perm)
@@ -370,139 +337,6 @@ def _dispatch(table, action, user, obj):
type(obj), action))
-def _does_course_group_name_exist(name):
- return len(Group.objects.filter(name=name)) > 0
-
-
-def _course_org_staff_group_name(location, course_context=None):
- """
- Get the name of the staff group for an organization which corresponds
- to the organization in the course id.
-
- 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'
-
- """
- loc = Location(location)
- 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.split('/')[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 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_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):
- """
- 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
- """
- loc = Location(location)
- group_name, legacy_group_name = group_names_for_staff(location, course_context)
-
- 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):
- """
- Get the name of the instructor group for an organization which corresponds
- to the organization in the course id.
-
- 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'
-
- """
- loc = Location(location)
- if loc.category == 'course':
- course_id = loc.course_id
- else:
- if course_context is None:
- raise CourseContextRequired()
- course_id = course_context
- return 'instructor_%s' % course_id.split('/')[0]
-
-
-def _course_instructor_group_name(location, course_context=None):
- """
- Get the name of the instructor group for a location, in the context of a course run.
- A course instructor has all staff privileges, but also can manage list of course staff (add, remove, list).
-
- 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
- """
- loc = Location(location)
- group_name, legacy_group_name = group_names_for_instructor(location, 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
-
-
-
-def _has_global_staff_access(user):
- if user.is_staff:
- debug("Allow: user.is_staff")
- return True
- else:
- debug("Deny: not user.is_staff")
- return False
-
-
def _adjust_start_date_for_beta_testers(user, descriptor):
"""
If user is in a beta test group, adjust the start date by the appropriate number of
@@ -530,11 +364,8 @@ def _adjust_start_date_for_beta_testers(user, descriptor):
# bail early if no beta testing is set up
return descriptor.start
- user_groups = [g.name for g in user.groups.all()]
-
- beta_group = course_beta_test_group_name(descriptor.location)
- if beta_group in user_groups:
- debug("Adjust start time: user in group %s", beta_group)
+ if CourseBetaTesterRole(descriptor.location).has_user(user):
+ debug("Adjust start time: user in beta role for %s", descriptor)
delta = timedelta(descriptor.days_early_for_beta)
effective = descriptor.start - delta
return effective
@@ -572,32 +403,34 @@ def _has_access_to_location(user, location, access_level, course_context):
if is_masquerading_as_student(user):
return False
- if user.is_staff:
+ if GlobalStaff().has_user(user):
debug("Allow: user.is_staff")
return True
- # If not global staff, is the user in the Auth group for this class?
- user_groups = [g.name for g in user.groups.all()]
+ if access_level not in ('staff', 'instructor'):
+ log.debug("Error in access._has_access_to_location access_level=%s unknown", access_level)
+ debug("Deny: unknown access level")
+ return False
- if access_level == 'staff':
- 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)
+ staff_access = (
+ CourseStaffRole(location, course_context).has_user(user) or
+ OrgStaffRole(location).has_user(user)
+ )
- 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)]
- 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)
+ if staff_access and access_level == 'staff':
+ debug("Allow: user has course staff access")
+ return True
+
+ instructor_access = (
+ CourseInstructorRole(location, course_context).has_user(user) or
+ OrgInstructorRole(location).has_user(user)
+ )
+
+ if instructor_access and access_level in ('staff', 'instructor'):
+ debug("Allow: user has course instructor access")
+ return True
+
+ debug("Deny: user did not have correct access")
return False
diff --git a/lms/djangoapps/courseware/roles.py b/lms/djangoapps/courseware/roles.py
new file mode 100644
index 0000000000..1643dd5051
--- /dev/null
+++ b/lms/djangoapps/courseware/roles.py
@@ -0,0 +1,192 @@
+"""
+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)
diff --git a/lms/djangoapps/courseware/tests/factories.py b/lms/djangoapps/courseware/tests/factories.py
index 7d33148dfd..91f91f2617 100644
--- a/lms/djangoapps/courseware/tests/factories.py
+++ b/lms/djangoapps/courseware/tests/factories.py
@@ -1,65 +1,57 @@
-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
+ last_name = "Staff"
-
-class GroupFactory(StudentGroupFactory):
- name = 'test_group'
-
-
-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):
diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py
index bad5f95e0c..a19b6e464d 100644
--- a/lms/djangoapps/courseware/tests/test_access.py
+++ b/lms/djangoapps/courseware/tests/test_access.py
@@ -1,68 +1,54 @@
+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))
+ 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 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 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 access if they are
+ # 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)
diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py
index 6ac2736623..0ba863f4cc 100644
--- a/lms/djangoapps/courseware/tests/test_masquerade.py
+++ b/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)
diff --git a/lms/djangoapps/courseware/tests/test_roles.py b/lms/djangoapps/courseware/tests/test_roles.py
new file mode 100644
index 0000000000..3bb064a828
--- /dev/null
+++ b/lms/djangoapps/courseware/tests/test_roles.py
@@ -0,0 +1,47 @@
+"""
+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))
+
diff --git a/lms/djangoapps/courseware/tests/test_view_authentication.py b/lms/djangoapps/courseware/tests/test_view_authentication.py
index ed27ae8efa..3639d07afa 100644
--- a/lms/djangoapps/courseware/tests/test_view_authentication.py
+++ b/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'))
diff --git a/lms/djangoapps/instructor/access.py b/lms/djangoapps/instructor/access.py
index e079ff87f6..c72bf56db8 100644
--- a/lms/djangoapps/instructor/access.py
+++ b/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))
diff --git a/lms/djangoapps/instructor/tests/test_access.py b/lms/djangoapps/instructor/tests/test_access.py
index 6a70f64fdc..6ed5159ae6 100644
--- a/lms/djangoapps/instructor/tests/test_access.py
+++ b/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)
diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py
index 7f08927ad1..35c7e22c60 100644
--- a/lms/djangoapps/instructor/tests/test_api.py
+++ b/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')
diff --git a/lms/djangoapps/instructor/tests/test_legacy_anon_csv.py b/lms/djangoapps/instructor/tests/test_legacy_anon_csv.py
index 947b72f298..b6312376ed 100644
--- a/lms/djangoapps/instructor/tests/test_legacy_anon_csv.py
+++ b/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')
-
diff --git a/lms/djangoapps/instructor/tests/test_legacy_download_csv.py b/lms/djangoapps/instructor/tests/test_legacy_download_csv.py
index c65547e408..d611218c0e 100644
--- a/lms/djangoapps/instructor/tests/test_legacy_download_csv.py
+++ b/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)
diff --git a/lms/djangoapps/instructor/tests/test_legacy_enrollment.py b/lms/djangoapps/instructor/tests/test_legacy_enrollment.py
index b5c7d9669c..a072a2768e 100644
--- a/lms/djangoapps/instructor/tests/test_legacy_enrollment.py
+++ b/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(
diff --git a/lms/djangoapps/instructor/tests/test_legacy_forum_admin.py b/lms/djangoapps/instructor/tests/test_legacy_forum_admin.py
index 29046b151f..f1b40aa44b 100644
--- a/lms/djangoapps/instructor/tests/test_legacy_forum_admin.py
+++ b/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)
diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py
index 5efd340578..04df8e181c 100644
--- a/lms/djangoapps/instructor/views/legacy.py
+++ b/lms/djangoapps/instructor/views/legacy.py
@@ -15,7 +15,7 @@ from requests.status_codes import codes
from StringIO import StringIO
from django.conf import settings
-from django.contrib.auth.models import User, Group
+from django.contrib.auth.models import User
from django.http import HttpResponse
from django_future.csrf import ensure_csrf_cookie
from django.views.decorators.cache import cache_control
@@ -32,9 +32,9 @@ from xmodule.html_module import HtmlDescriptor
from bulk_email.models import CourseEmail, CourseAuthorization
from courseware import grades
-from courseware.access import (has_access, get_access_group_name,
- course_beta_test_group_name)
+from courseware.access import has_access
from courseware.courses import get_course_with_access, get_cms_course_link
+from courseware.roles import CourseStaffRole, CourseInstructorRole, CourseBetaTesterRole
from courseware.models import StudentModule
from django_comment_common.models import (Role,
FORUM_ROLE_ADMINISTRATOR,
@@ -50,12 +50,11 @@ from instructor_task.api import (get_running_instructor_tasks,
submit_reset_problem_attempts_for_all_students,
submit_bulk_course_email)
from instructor_task.views import get_task_completion_info
-from mitxmako.shortcuts import render_to_response
+from mitxmako.shortcuts import render_to_response, render_to_string
from psychometrics import psychoanalyze
from student.models import CourseEnrollment, CourseEnrollmentAllowed, unique_id_for_user
from student.views import course_from_id
import track.views
-from mitxmako.shortcuts import render_to_string
from xblock.field_data import DictFieldData
from xblock.fields import ScopeIds
from django.utils.translation import ugettext as _u
@@ -137,35 +136,6 @@ def instructor_dashboard(request, course_id):
writer.writerow(encoded_row)
return response
- def get_staff_group(course):
- """Get or create the staff access group"""
- return get_group(course, 'staff')
-
- def get_instructor_group(course):
- """Get or create the instructor access group"""
- return get_group(course, 'instructor')
-
- def get_group(course, groupname):
- """Get or create an access group"""
- grpname = get_access_group_name(course, groupname)
- try:
- group = Group.objects.get(name=grpname)
- except Group.DoesNotExist:
- group = Group(name=grpname) # create the group
- group.save()
- return group
-
- def get_beta_group(course):
- """
- Get the group for beta testers of course.
- """
- # Not using get_group because there is no access control action called
- # 'beta', so adding it to get_access_group_name doesn't really make
- # sense.
- name = course_beta_test_group_name(course.location)
- (group, _) = Group.objects.get_or_create(name=name)
- return group
-
def get_module_url(urlname):
"""
Construct full URL for a module from its urlname.
@@ -496,58 +466,34 @@ def instructor_dashboard(request, course_id):
# Admin
elif 'List course staff' in action:
- group = get_staff_group(course)
- msg += 'Staff group = {0}'.format(group.name)
- datatable = _group_members_table(group, "List of Staff", course_id)
+ role = CourseStaffRole(course.location)
+ datatable = _role_members_table(role, "List of Staff", course_id)
track.views.server_track(request, "list-staff", {}, page="idashboard")
- elif 'List course instructors' in action and request.user.is_staff:
- group = get_instructor_group(course)
- msg += 'Instructor group = {0}'.format(group.name)
- log.debug('instructor grp={0}'.format(group.name))
- uset = group.user_set.all()
- datatable = {'header': ['Username', 'Full name']}
- datatable['data'] = [[x.username, x.profile.name] for x in uset]
- datatable['title'] = 'List of Instructors in course {0}'.format(course_id)
+ elif 'List course instructors' in action and GlobalStaff().has_user(request.user):
+ role = CourseInstructorRole(course.location)
+ datatable = _role_members_table(role, "List of Instructors", course_id)
track.views.server_track(request, "list-instructors", {}, page="idashboard")
elif action == 'Add course staff':
uname = request.POST['staffuser']
- group = get_staff_group(course)
- msg += add_user_to_group(request, uname, group, 'staff', 'staff')
+ role = CourseStaffRole(course.location)
+ msg += add_user_to_role(request, uname, role, 'staff', 'staff')
elif action == 'Add instructor' and request.user.is_staff:
uname = request.POST['instructor']
- try:
- user = User.objects.get(username=uname)
- except User.DoesNotExist:
- msg += 'Error: unknown username "{0}"'.format(uname)
- user = None
- if user is not None:
- group = get_instructor_group(course)
- msg += 'Added {0} to instructor group = {1}'.format(user, group.name)
- log.debug('staffgrp={0}'.format(group.name))
- user.groups.add(group)
- track.views.server_track(request, "add-instructor", {"instructor": unicode(user)}, page="idashboard")
+ role = CourseInstructorRole(course.location)
+ msg += add_user_to_role(request, uname, role, 'instructor', 'instructor')
elif action == 'Remove course staff':
uname = request.POST['staffuser']
- group = get_staff_group(course)
- msg += remove_user_from_group(request, uname, group, 'staff', 'staff')
+ role = CourseStaffRole(course.location)
+ msg += remove_user_from_role(request, uname, role, 'staff', 'staff')
elif action == 'Remove instructor' and request.user.is_staff:
uname = request.POST['instructor']
- try:
- user = User.objects.get(username=uname)
- except User.DoesNotExist:
- msg += 'Error: unknown username "{0}"'.format(uname)
- user = None
- if user is not None:
- group = get_instructor_group(course)
- msg += 'Removed {0} from instructor group = {1}'.format(user, group.name)
- log.debug('instructorgrp={0}'.format(group.name))
- user.groups.remove(group)
- track.views.server_track(request, "remove-instructor", {"instructor": unicode(user)}, page="idashboard")
+ role = CourseInstructorRole(course.location)
+ msg += remove_user_from_role(request, uname, role, 'instructor', 'instructor')
#----------------------------------------
# DataDump
@@ -605,25 +551,24 @@ def instructor_dashboard(request, course_id):
# Group management
elif 'List beta testers' in action:
- group = get_beta_group(course)
- msg += 'Beta test group = {0}'.format(group.name)
- datatable = _group_members_table(group, "List of beta_testers", course_id)
+ role = CourseBetaTesterRole(course.location)
+ datatable = _role_members_table(role, "List of Beta Testers", course_id)
track.views.server_track(request, "list-beta-testers", {}, page="idashboard")
elif action == 'Add beta testers':
users = request.POST['betausers']
log.debug("users: {0!r}".format(users))
- group = get_beta_group(course)
+ role = CourseBetaTesterRole(course.location)
for username_or_email in split_by_comma_and_whitespace(users):
msg += "{0}
".format(
- add_user_to_group(request, username_or_email, group, 'beta testers', 'beta-tester'))
+ add_user_to_role(request, username_or_email, role, 'beta testers', 'beta-tester'))
elif action == 'Remove beta testers':
users = request.POST['betausers']
- group = get_beta_group(course)
+ role = CourseBetaTesterRole(course.location)
for username_or_email in split_by_comma_and_whitespace(users):
msg += "{0}
".format(
- remove_user_from_group(request, username_or_email, group, 'beta testers', 'beta-tester'))
+ remove_user_from_role(request, username_or_email, role, 'beta testers', 'beta-tester'))
#----------------------------------------
# forum administration
@@ -1016,12 +961,12 @@ def _update_forum_role_membership(uname, course, rolename, add_or_remove):
return msg
-def _group_members_table(group, title, course_id):
+def _role_members_table(role, title, course_id):
"""
Return a data table of usernames and names of users in group_name.
Arguments:
- group -- a django group.
+ role -- a courseware.roles.AccessRole
title -- a descriptive title to show the user
Returns:
@@ -1030,76 +975,107 @@ def _group_members_table(group, title, course_id):
'data': [[username, name] for all users]
'title': "{title} in course {course}"
"""
- uset = group.user_set.all()
+ uset = role.users_with_role()
datatable = {'header': ['Username', 'Full name']}
datatable['data'] = [[x.username, x.profile.name] for x in uset]
datatable['title'] = '{0} in course {1}'.format(title, course_id)
return datatable
-def _add_or_remove_user_group(request, username_or_email, group, group_title, event_name, do_add):
+def _user_from_name_or_email(username_or_email):
"""
- Implementation for both add and remove functions, to get rid of shared code. do_add is bool that determines which
- to do.
+ Return the `django.contrib.auth.User` with the supplied username or email.
+
+ If `username_or_email` contains an `@` it is treated as an email, otherwise
+ it is treated as the username
"""
- user = None
username_or_email = strip_if_string(username_or_email)
- try:
- if '@' in username_or_email:
- user = User.objects.get(email=username_or_email)
- else:
- user = User.objects.get(username=username_or_email)
- except User.DoesNotExist:
- msg = u'Error: unknown username or email "{0}"'.format(username_or_email)
- user = None
- if user is not None:
- action = "Added" if do_add else "Removed"
- prep = "to" if do_add else "from"
- msg = '{action} {0} {prep} {1} group = {2}'.format(user, group_title, group.name,
- action=action, prep=prep)
- if do_add:
- user.groups.add(group)
- else:
- user.groups.remove(group)
- event = "add" if do_add else "remove"
- track.views.server_track(request, "add-or-remove-user-group", {"event_name": event_name, "user": unicode(user), "event": event}, page="idashboard")
-
- return msg
+ if '@' in username_or_email:
+ return User.objects.get(email=username_or_email)
+ else:
+ return User.objects.get(username=username_or_email)
-def add_user_to_group(request, username_or_email, group, group_title, event_name):
+def add_user_to_role(request, username_or_email, role, group_title, event_name):
"""
Look up the given user by username (if no '@') or email (otherwise), and add them to group.
Arguments:
request: django request--used for tracking log
username_or_email: who to add. Decide if it's an email by presense of an '@'
- group: django group object
+ group: A group name
group_title: what to call this group in messages to user--e.g. "beta-testers".
event_name: what to call this event when logging to tracking logs.
Returns:
html to insert in the message field
"""
- return _add_or_remove_user_group(request, username_or_email, group, group_title, event_name, True)
+ username_or_email = strip_if_string(username_or_email)
+ try:
+ user = _user_from_name_or_email(username_or_email)
+ except User.DoesNotExist:
+ return u'Error: unknown username or email "{0}"'.format(username_or_email)
+
+ role.add_users(user)
+
+ # Deal with historical event names
+ if event_name in ('staff', 'beta-tester'):
+ track.views.server_track(
+ request,
+ "add-or-remove-user-group",
+ {
+ "event_name": event_name,
+ "user": unicode(user),
+ "event": "add"
+ },
+ page="idashboard"
+ )
+ else:
+ track.views.server_track(request, "add-instructor", {"instructor": unicode(user)}, page="idashboard")
+
+ return 'Added {0} to {1}'.format(user, group_title)
-def remove_user_from_group(request, username_or_email, group, group_title, event_name):
+def remove_user_from_role(request, username_or_email, role, group_title, event_name):
"""
- Look up the given user by username (if no '@') or email (otherwise), and remove them from group.
+ Look up the given user by username (if no '@') or email (otherwise), and remove them from the supplied role.
Arguments:
request: django request--used for tracking log
username_or_email: who to remove. Decide if it's an email by presense of an '@'
- group: django group object
+ role: A courseware.roles.AccessRole
group_title: what to call this group in messages to user--e.g. "beta-testers".
event_name: what to call this event when logging to tracking logs.
Returns:
html to insert in the message field
"""
- return _add_or_remove_user_group(request, username_or_email, group, group_title, event_name, False)
+
+ username_or_email = strip_if_string(username_or_email)
+ try:
+ user = _user_from_name_or_email(username_or_email)
+ except User.DoesNotExist:
+ return u'Error: unknown username or email "{0}"'.format(username_or_email)
+
+ role.remove_users(user)
+
+ # Deal with historical event names
+ if event_name in ('staff', 'beta-tester'):
+ track.views.server_track(
+ request,
+ "add-or-remove-user-group",
+ {
+ "event_name": event_name,
+ "user": unicode(user),
+ "event": "remove"
+ },
+ page="idashboard"
+ )
+ else:
+ track.views.server_track(request, "remove-instructor", {"instructor": unicode(user)}, page="idashboard")
+
+ return 'Removed {0} from {1}'.format(user, group_title)
def get_student_grade_summary_data(request, course, course_id, get_grades=True, get_raw_scores=False, use_offline=False):
diff --git a/lms/djangoapps/open_ended_grading/tests.py b/lms/djangoapps/open_ended_grading/tests.py
index 8e309ee2de..ad6298d71a 100644
--- a/lms/djangoapps/open_ended_grading/tests.py
+++ b/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):