From 68c182c055861ee53efc7a559f724ae547830aae Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 1 Nov 2013 14:58:27 -0400 Subject: [PATCH 1/2] Optimize and cache the query to find the list of groups a user is in --- lms/djangoapps/courseware/access.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 7836ab8bbc..7126c6f881 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -503,6 +503,14 @@ def _has_global_staff_access(user): return False +def group_names(user): + """Return the list of names of the groups that a user is in""" + if not hasattr(user, '_groups'): + user._groups = user.groups.values_list('name', flat=True) + + return user._groups + + 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,10 +538,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: + if beta_group in group_names(user): debug("Adjust start time: user in group %s", beta_group) delta = timedelta(descriptor.days_early_for_beta) effective = descriptor.start - delta @@ -577,7 +583,7 @@ def _has_access_to_location(user, location, access_level, course_context): 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()] + user_groups = group_names(user) if access_level == 'staff': staff_groups = group_names_for_staff(location, course_context) + \ From 061a46beefd2318d711b790f9a6cc9f9e832b2b1 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 1 Nov 2013 15:35:40 -0400 Subject: [PATCH 2/2] Formalize various access roles as objects This centralizes the logic for group membership, and means that we don't need to make queries to find out whether the legacy groups names exist. --- common/djangoapps/student/tests/factories.py | 35 ++- lms/djangoapps/bulk_email/tasks.py | 12 +- lms/djangoapps/bulk_email/tests/test_email.py | 4 +- .../course_wiki/tests/test_access.py | 10 +- lms/djangoapps/courseware/access.py | 235 +++--------------- lms/djangoapps/courseware/roles.py | 192 ++++++++++++++ lms/djangoapps/courseware/tests/factories.py | 66 +++-- .../courseware/tests/test_access.py | 70 +++--- .../courseware/tests/test_masquerade.py | 8 +- lms/djangoapps/courseware/tests/test_roles.py | 47 ++++ .../tests/test_view_authentication.py | 42 ++-- lms/djangoapps/instructor/access.py | 35 ++- .../instructor/tests/test_access.py | 35 +-- lms/djangoapps/instructor/tests/test_api.py | 18 +- .../instructor/tests/test_legacy_anon_csv.py | 12 +- .../tests/test_legacy_download_csv.py | 9 +- .../tests/test_legacy_enrollment.py | 6 +- .../tests/test_legacy_forum_admin.py | 8 +- lms/djangoapps/instructor/views/legacy.py | 202 +++++++-------- lms/djangoapps/open_ended_grading/tests.py | 8 +- 20 files changed, 524 insertions(+), 530 deletions(-) create mode 100644 lms/djangoapps/courseware/roles.py create mode 100644 lms/djangoapps/courseware/tests/test_roles.py 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 7126c6f881..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,147 +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 group_names(user): - """Return the list of names of the groups that a user is in""" - if not hasattr(user, '_groups'): - user._groups = user.groups.values_list('name', flat=True) - - return user._groups - - 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 @@ -538,9 +364,8 @@ def _adjust_start_date_for_beta_testers(user, descriptor): # bail early if no beta testing is set up return descriptor.start - beta_group = course_beta_test_group_name(descriptor.location) - if beta_group in group_names(user): - 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 @@ -578,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 = group_names(user) + 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):