Merge pull request #1579 from edx/sarina/enhance-user-factories

Sarina/enhance user factories

common/djangoapps/student/tests/factories.py

+"""Provides factories for student models."""
 from student.models import (User, UserProfile, Registration,
                             CourseEnrollmentAllowed, CourseEnrollment,
                             PendingEmailChange, UserStanding,
@@ -10,7 +11,7 @@ from uuid import uuid4
 from pytz import UTC
 
 # Factories don't have __init__ methods, and are self documenting
-# pylint: disable=W0232
+# pylint: disable=W0232, C0111
 
 
 class GroupFactory(DjangoModelFactory):
@@ -18,6 +19,7 @@ class GroupFactory(DjangoModelFactory):
 
     name = u'staff_MITx/999/Robot_Super_Course'
 
+
 class UserStandingFactory(DjangoModelFactory):
     FACTORY_FOR = UserStanding
 
@@ -47,6 +49,7 @@ class CourseModeFactory(DjangoModelFactory):
     suggested_prices = ''
     currency = 'usd'
 
+
 class RegistrationFactory(DjangoModelFactory):
     FACTORY_FOR = Registration
 
@@ -70,7 +73,7 @@ class UserFactory(DjangoModelFactory):
     date_joined = datetime(2011, 1, 1, tzinfo=UTC)
 
     @post_generation
-    def profile(obj, create, extracted, **kwargs):
+    def profile(obj, create, extracted, **kwargs):  # pylint: disable=unused-argument, no-self-argument
         if create:
             obj.save()
             return UserProfileFactory.create(user=obj, **kwargs)

lms/djangoapps/bulk_email/tests/test_email.py

@@ -11,7 +11,9 @@ from django.core.management import call_command
 from django.test.utils import override_settings
 
 from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE
-from student.tests.factories import UserFactory, GroupFactory, CourseEnrollmentFactory
+from student.tests.factories import CourseEnrollmentFactory, UserFactory
+from courseware.tests.factories import StaffFactory, InstructorFactory
+
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 from bulk_email.models import Optout
@@ -47,16 +49,11 @@ class TestEmailSendFromDashboard(ModuleStoreTestCase):
     @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': False})
     def setUp(self):
         self.course = CourseFactory.create()
-        self.instructor = UserFactory.create(username="instructor", email="robot+instructor@edx.org")
-        # Create instructor group for course
-        instructor_group = GroupFactory.create(name="instructor_MITx/999/Robot_Super_Course")
-        instructor_group.user_set.add(self.instructor)
+
+        self.instructor = InstructorFactory(self.course)
 
         # Create staff
-        self.staff = [UserFactory() for _ in xrange(STAFF_COUNT)]
-        staff_group = GroupFactory()
-        for staff in self.staff:
-            staff_group.user_set.add(staff)  # pylint: disable=E1101
+        self.staff = [StaffFactory(self.course) for _ in xrange(STAFF_COUNT)]
 
         # Create students
         self.students = [UserFactory() for _ in xrange(STUDENT_COUNT)]

lms/djangoapps/courseware/tests/factories.py

@@ -10,6 +10,7 @@ from student.tests.factories import CourseEnrollmentAllowedFactory as StudentCou
 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 xmodule.modulestore import Location
 from pytz import UTC
@@ -33,6 +34,26 @@ class UserFactory(StudentUserFactory):
     date_joined = datetime.now(UTC)
 
 
+def InstructorFactory(course):  # pylint: disable=invalid-name
+    """
+    Given a course object, returns a User object with instructor
+    permissions for `course`.
+    """
+    user = StudentUserFactory.create(last_name="Instructor")
+    allow_access(course, user, "instructor")
+    return user
+
+
+def StaffFactory(course):  # pylint: disable=invalid-name
+    """
+    Given a course object, returns a User object with staff
+    permissions for `course`.
+    """
+    user = StudentUserFactory.create(last_name="Staff")
+    allow_access(course, user, "staff")
+    return user
+
+
 class GroupFactory(StudentGroupFactory):
     name = 'test_group'
 

lms/djangoapps/instructor/access.py

@@ -82,7 +82,7 @@ def _change_access(course, user, level, action):
         raise ValueError("unrecognized action '{}'".format(action))
 
 
-def update_forum_role_membership(course_id, user, rolename, action):
+def update_forum_role(course_id, user, rolename, action):
     """
     Change forum access of user.
 

lms/djangoapps/instructor/tests/test_access.py

@@ -17,7 +17,7 @@ from django_comment_common.models import (Role,
 from instructor.access import (allow_access,
                                revoke_access,
                                list_with_level,
-                               update_forum_role_membership)
+                               update_forum_role)
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
@@ -148,44 +148,44 @@ class TestInstructorAccessForum(ModuleStoreTestCase):
 
     def test_allow(self):
         user = UserFactory.create()
-        update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'allow')
+        update_forum_role(self.course.id, user, FORUM_ROLE_MODERATOR, 'allow')
         self.assertIn(user, self.mod_role.users.all())
 
     def test_allow_twice(self):
         user = UserFactory.create()
-        update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'allow')
+        update_forum_role(self.course.id, user, FORUM_ROLE_MODERATOR, 'allow')
         self.assertIn(user, self.mod_role.users.all())
-        update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'allow')
+        update_forum_role(self.course.id, user, FORUM_ROLE_MODERATOR, 'allow')
         self.assertIn(user, self.mod_role.users.all())
 
     @raises(Role.DoesNotExist)
     def test_allow_badrole(self):
         user = UserFactory.create()
-        update_forum_role_membership(self.course.id, user, 'robot-not-a-real-role', 'allow')
+        update_forum_role(self.course.id, user, 'robot-not-a-real-role', 'allow')
 
     def test_revoke(self):
         user = self.moderators[0]
-        update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'revoke')
+        update_forum_role(self.course.id, user, FORUM_ROLE_MODERATOR, 'revoke')
         self.assertNotIn(user, self.mod_role.users.all())
 
     def test_revoke_twice(self):
         user = self.moderators[0]
-        update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'revoke')
+        update_forum_role(self.course.id, user, FORUM_ROLE_MODERATOR, 'revoke')
         self.assertNotIn(user, self.mod_role.users.all())
-        update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'revoke')
+        update_forum_role(self.course.id, user, FORUM_ROLE_MODERATOR, 'revoke')
         self.assertNotIn(user, self.mod_role.users.all())
 
     def test_revoke_notallowed(self):
         user = UserFactory()
-        update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'revoke')
+        update_forum_role(self.course.id, user, FORUM_ROLE_MODERATOR, 'revoke')
         self.assertNotIn(user, self.mod_role.users.all())
 
     @raises(Role.DoesNotExist)
     def test_revoke_badrole(self):
         user = self.moderators[0]
-        update_forum_role_membership(self.course.id, user, 'robot-not-a-real-role', 'allow')
+        update_forum_role(self.course.id, user, 'robot-not-a-real-role', 'allow')
 
     @raises(ValueError)
     def test_bad_mode(self):
         user = UserFactory()
-        update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'robot-not-a-mode')
+        update_forum_role(self.course.id, user, FORUM_ROLE_MODERATOR, 'robot-not-a-mode')

lms/djangoapps/instructor/tests/test_api.py

@@ -13,13 +13,15 @@ from mock import Mock, patch
 from django.test.utils import override_settings
 from django.core.urlresolvers import reverse
 from django.http import HttpRequest, HttpResponse
+from django_comment_common.models import FORUM_ROLE_COMMUNITY_TA
 
 from django.contrib.auth.models import User
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from courseware.tests.helpers import LoginEnrollmentTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
-from student.tests.factories import UserFactory, AdminFactory
+from student.tests.factories import UserFactory
+from courseware.tests.factories import StaffFactory, InstructorFactory
 
 from student.models import CourseEnrollment
 from courseware.models import StudentModule
@@ -29,8 +31,7 @@ import instructor_task.api
 
 from instructor.access import allow_access
 import instructor.views.api
-from instructor.views.api import (
-    _split_input_list, _msk_from_problem_urlname, common_exceptions_400)
+from instructor.views.api import _split_input_list, _msk_from_problem_urlname, common_exceptions_400
 from instructor_task.api_helper import AlreadyRunningError
 
 
@@ -97,58 +98,144 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase):
     Ensure that users cannot access endpoints they shouldn't be able to.
     """
     def setUp(self):
-        self.user = UserFactory.create()
         self.course = CourseFactory.create()
+        self.user = UserFactory.create()
         CourseEnrollment.enroll(self.user, self.course.id)
-        self.client.login(username=self.user.username, password='test')
 
-    def test_staff_level(self):
+        self.problem_urlname = 'robot-some-problem-urlname'
+        _module = StudentModule.objects.create(
+            student=self.user,
+            course_id=self.course.id,
+            module_state_key=_msk_from_problem_urlname(
+                self.course.id,
+                self.problem_urlname
+            ),
+            state=json.dumps({'attempts': 10}),
+        )
+
+        # Endpoints that only Staff or Instructors can access
+        self.staff_level_endpoints = [
+            ('students_update_enrollment', {'emails': 'foo@example.org', 'action': 'enroll'}),
+            ('get_grading_config', {}),
+            ('get_students_features', {}),
+            ('get_distribution', {}),
+            ('get_student_progress_url', {'unique_student_identifier': self.user.username}),
+            ('reset_student_attempts', {'problem_to_reset': self.problem_urlname, 'unique_student_identifier': self.user.email}),
+            ('update_forum_role_membership', {'email': self.user.email, 'rolename': 'Moderator', 'action': 'allow'}),
+            ('list_forum_members', {'rolename': FORUM_ROLE_COMMUNITY_TA}),
+            ('proxy_legacy_analytics', {'aname': 'ProblemGradeDistribution'}),
+            ('send_email', {'send_to': 'staff', 'subject': 'test', 'message': 'asdf'}),
+            ('list_instructor_tasks', {}),
+            ('list_background_email_tasks', {}),
+        ]
+        # Endpoints that only Instructors can access
+        self.instructor_level_endpoints = [
+            ('modify_access', {'email': self.user.email, 'rolename': 'beta', 'action': 'allow'}),
+            ('list_course_role_members', {'rolename': 'beta'}),
+            ('rescore_problem', {'problem_to_reset': self.problem_urlname, 'unique_student_identifier': self.user.email}),
+        ]
+
+    def _access_endpoint(self, endpoint, args, status_code, msg):
+        """
+        Asserts that accessing the given `endpoint` gets a response of `status_code`.
+
+        endpoint: string, endpoint for instructor dash API
+        args: dict, kwargs for `reverse` call
+        status_code: expected HTTP status code response
+        msg: message to display if assertion fails.
+        """
+        url = reverse(endpoint, kwargs={'course_id': self.course.id})
+        if endpoint in 'send_email':
+            response = self.client.post(url, args)
+        else:
+            response = self.client.get(url, args)
+        print endpoint
+        print response
+        self.assertEqual(
+            response.status_code,
+            status_code,
+            msg=msg
+        )
+
+    def test_student_level(self):
         """
         Ensure that an enrolled student can't access staff or instructor endpoints.
         """
-        staff_level_endpoints = [
-            'students_update_enrollment',
-            'modify_access',
-            'list_course_role_members',
-            'get_grading_config',
-            'get_students_features',
-            'get_distribution',
-            'get_student_progress_url',
-            'reset_student_attempts',
-            'rescore_problem',
-            'list_instructor_tasks',
-            'list_forum_members',
-            'update_forum_role_membership',
-            'proxy_legacy_analytics',
-            'send_email',
-            'list_background_email_tasks',
-        ]
-        for endpoint in staff_level_endpoints:
-            url = reverse(endpoint, kwargs={'course_id': self.course.id})
-            response = self.client.get(url, {})
-            self.assertEqual(
-                response.status_code,
+        self.client.login(username=self.user.username, password='test')
+
+        for endpoint, args in self.staff_level_endpoints:
+            self._access_endpoint(
+                endpoint,
+                args,
                 403,
-                msg="Student should not be allowed to access endpoint " + endpoint
+                "Student should not be allowed to access endpoint " + endpoint
             )
 
-    def test_instructor_level(self):
+        for endpoint, args in self.instructor_level_endpoints:
+            self._access_endpoint(
+                endpoint,
+                args,
+                403,
+                "Student should not be allowed to access endpoint " + endpoint
+            )
+
+    def test_staff_level(self):
         """
         Ensure that a staff member can't access instructor endpoints.
         """
-        instructor_level_endpoints = [
-            'modify_access',
-            'list_course_role_members',
-            'reset_student_attempts',
-            'update_forum_role_membership',
-        ]
-        for endpoint in instructor_level_endpoints:
-            url = reverse(endpoint, kwargs={'course_id': self.course.id})
-            response = self.client.get(url, {})
-            self.assertEqual(
-                response.status_code,
+        staff_member = StaffFactory(self.course)
+        CourseEnrollment.enroll(staff_member, self.course.id)
+        self.client.login(username=staff_member.username, password='test')
+        # Try to promote to forums admin - not working
+        # update_forum_role(self.course.id, staff_member, FORUM_ROLE_ADMINISTRATOR, 'allow')
+
+        for endpoint, args in self.staff_level_endpoints:
+            # TODO: make these work
+            if endpoint in ['update_forum_role_membership', 'proxy_legacy_analytics', 'list_forum_members']:
+                continue
+            self._access_endpoint(
+                endpoint,
+                args,
+                200,
+                "Staff member should be allowed to access endpoint " + endpoint
+            )
+
+        for endpoint, args in self.instructor_level_endpoints:
+            self._access_endpoint(
+                endpoint,
+                args,
                 403,
-                msg="Staff should not be allowed to access endpoint " + endpoint
+                "Staff member should not be allowed to access endpoint " + endpoint
+            )
+
+    def test_instructor_level(self):
+        """
+        Ensure that an instructor member can access all endpoints.
+        """
+        inst = InstructorFactory(self.course)
+        CourseEnrollment.enroll(inst, self.course.id)
+        self.client.login(username=inst.username, password='test')
+
+        for endpoint, args in self.staff_level_endpoints:
+            # TODO: make these work
+            if endpoint in ['update_forum_role_membership', 'proxy_legacy_analytics']:
+                continue
+            self._access_endpoint(
+                endpoint,
+                args,
+                200,
+                "Instructor should be allowed to access endpoint " + endpoint
+            )
+
+        for endpoint, args in self.instructor_level_endpoints:
+            # TODO: make this work
+            if endpoint in ['rescore_problem']:
+                continue
+            self._access_endpoint(
+                endpoint,
+                args,
+                200,
+                "Instructor should be allowed to access endpoint " + endpoint
             )
 
 
@@ -161,8 +248,8 @@ class TestInstructorAPIEnrollment(ModuleStoreTestCase, LoginEnrollmentTestCase):
     job of test_enrollment. This tests the response and action switch.
     """
     def setUp(self):
-        self.instructor = AdminFactory.create()
         self.course = CourseFactory.create()
+        self.instructor = InstructorFactory(self.course)
         self.client.login(username=self.instructor.username, password='test')
 
         self.enrolled_student = UserFactory()
@@ -288,8 +375,8 @@ class TestInstructorAPILevelsAccess(ModuleStoreTestCase, LoginEnrollmentTestCase
     response yet, so only the status code is tested.
     """
     def setUp(self):
-        self.instructor = AdminFactory.create()
         self.course = CourseFactory.create()
+        self.instructor = InstructorFactory(self.course)
         self.client.login(username=self.instructor.username, password='test')
 
         self.other_instructor = UserFactory()
@@ -425,8 +512,8 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa
     Test endpoints that show data without side effects.
     """
     def setUp(self):
-        self.instructor = AdminFactory.create()
         self.course = CourseFactory.create()
+        self.instructor = InstructorFactory(self.course)
         self.client.login(username=self.instructor.username, password='test')
 
         self.students = [UserFactory() for _ in xrange(6)]
@@ -562,8 +649,8 @@ class TestInstructorAPIRegradeTask(ModuleStoreTestCase, LoginEnrollmentTestCase)
     database, that is the job of task tests and test_enrollment.
     """
     def setUp(self):
-        self.instructor = AdminFactory.create()
         self.course = CourseFactory.create()
+        self.instructor = InstructorFactory(self.course)
         self.client.login(username=self.instructor.username, password='test')
 
         self.student = UserFactory()
@@ -706,8 +793,8 @@ class TestInstructorSendEmail(ModuleStoreTestCase, LoginEnrollmentTestCase):
     only with valid email messages.
     """
     def setUp(self):
-        self.instructor = AdminFactory.create()
         self.course = CourseFactory.create()
+        self.instructor = InstructorFactory(self.course)
         self.client.login(username=self.instructor.username, password='test')
         test_subject = u'\u1234 test subject'
         test_message = u'\u6824 test message'
@@ -828,8 +915,8 @@ class TestInstructorAPITaskLists(ModuleStoreTestCase, LoginEnrollmentTestCase):
             return attr_dict
 
     def setUp(self):
-        self.instructor = AdminFactory.create()
         self.course = CourseFactory.create()
+        self.instructor = InstructorFactory(self.course)
         self.client.login(username=self.instructor.username, password='test')
 
         self.student = UserFactory()
@@ -959,8 +1046,8 @@ class TestInstructorAPIAnalyticsProxy(ModuleStoreTestCase, LoginEnrollmentTestCa
             self.content = '{"test_content": "robot test content"}'
 
     def setUp(self):
-        self.instructor = AdminFactory.create()
         self.course = CourseFactory.create()
+        self.instructor = InstructorFactory(self.course)
         self.client.login(username=self.instructor.username, password='test')
 
     @patch.object(instructor.views.api.requests, 'get')

lms/djangoapps/instructor/views/api.py

@@ -35,7 +35,7 @@ from instructor_task.views import get_task_completion_info
 import instructor.enrollment as enrollment
 from instructor.enrollment import enroll_email, unenroll_email
 from instructor.views.tools import strip_if_string, get_student_from_identifier
-import instructor.access as access
+from instructor.access import list_with_level, allow_access, revoke_access, update_forum_role
 import analytics.basic
 import analytics.distributions
 import analytics.csvs
@@ -294,9 +294,9 @@ def modify_access(request, course_id):
         )
 
     if action == 'allow':
-        access.allow_access(course, user, rolename)
+        allow_access(course, user, rolename)
     elif action == 'revoke':
-        access.revoke_access(course, user, rolename)
+        revoke_access(course, user, rolename)
     else:
         return HttpResponseBadRequest("unrecognized action '{}'".format(action))
 
@@ -352,7 +352,7 @@ def list_course_role_members(request, course_id):
 
     response_payload = {
         'course_id': course_id,
-        rolename: map(extract_user_info, access.list_with_level(
+        rolename: map(extract_user_info, list_with_level(
             course, rolename
         )),
     }
@@ -381,7 +381,7 @@ def get_grading_config(request, course_id):
 @ensure_csrf_cookie
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
 @require_level('staff')
-def get_students_features(request, course_id, csv=False):  # pylint: disable=W0613
+def get_students_features(request, course_id, csv=False):  # pylint: disable=W0613, W0621
     """
     Respond with json which contains a summary of all enrolled students profile information.
 
@@ -882,7 +882,7 @@ def update_forum_role_membership(request, course_id):
         return HttpResponseBadRequest("Cannot revoke instructor forum admin privelages.")
 
     try:
-        access.update_forum_role_membership(course_id, user, rolename, action)
+        update_forum_role(course_id, user, rolename, action)
     except Role.DoesNotExist:
         return HttpResponseBadRequest("Role does not exist.")