Make course ids and usage ids opaque to LMS and Studio [partial commit]

This commit migrates roles from named groups to a relational table.

These keys are now objects with a limited interface, and the particular
internal representation is managed by the data storage layer (the
modulestore).

For the LMS, there should be no outward-facing changes to the system.
The keys are, for now, a change to internal representation only. For
Studio, the new serialized form of the keys is used in urls, to allow
for further migration in the future.

Co-Author: Andy Armstrong <andya@edx.org>
Co-Author: Christina Roberts <christina@edx.org>
Co-Author: David Baumgold <db@edx.org>
Co-Author: Diana Huang <dkh@edx.org>
Co-Author: Don Mitchell <dmitchell@edx.org>
Co-Author: Julia Hansbrough <julia@edx.org>
Co-Author: Nimisha Asthagiri <nasthagiri@edx.org>
Co-Author: Sarina Canelake <sarina@edx.org>

[LMS-2370]

common/djangoapps/student/tests/test_roles.py

@@ -4,13 +4,12 @@ Tests of student.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 student.roles import GlobalStaff, CourseRole, CourseStaffRole
-from xmodule.modulestore.django import loc_mapper
-from xmodule.modulestore.locator import BlockUsageLocator
+from student.roles import GlobalStaff, CourseRole, CourseStaffRole, OrgStaffRole, OrgInstructorRole, \
+    CourseInstructorRole
+from xmodule.modulestore.locations import SlashSeparatedCourseKey
 
 
 class RolesTestCase(TestCase):
@@ -19,12 +18,13 @@ class RolesTestCase(TestCase):
     """
 
     def setUp(self):
-        self.course = Location('i4x://edX/toy/course/2012_Fall')
+        self.course_id = SlashSeparatedCourseKey('edX', 'toy', '2012_Fall')
+        self.course_loc = self.course_id.make_usage_key('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)
+        self.course_staff = StaffFactory(course=self.course_id)
+        self.course_instructor = InstructorFactory(course=self.course_id)
 
     def test_global_staff(self):
         self.assertFalse(GlobalStaff().has_user(self.student))
@@ -32,55 +32,124 @@ class RolesTestCase(TestCase):
         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()
+    def test_group_name_case_sensitive(self):
+        uppercase_course_id = "ORG/COURSE/NAME"
+        lowercase_course_id = uppercase_course_id.lower()
+        uppercase_course_key = SlashSeparatedCourseKey.from_deprecated_string(uppercase_course_id)
+        lowercase_course_key = SlashSeparatedCourseKey.from_deprecated_string(lowercase_course_id)
 
-        lowercase_group = "role_org/course/name"
-        uppercase_group = lowercase_group.upper()
+        role = "role"
 
-        lowercase_user = UserFactory(groups=lowercase_group)
-        uppercase_user = UserFactory(groups=uppercase_group)
+        lowercase_user = UserFactory()
+        CourseRole(role, lowercase_course_key).add_users(lowercase_user)
+        uppercase_user = UserFactory()
+        CourseRole(role, uppercase_course_key).add_users(uppercase_user)
 
-        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))
+        self.assertTrue(CourseRole(role, lowercase_course_key).has_user(lowercase_user))
+        self.assertFalse(CourseRole(role, uppercase_course_key).has_user(lowercase_user))
+        self.assertFalse(CourseRole(role, lowercase_course_key).has_user(uppercase_user))
+        self.assertTrue(CourseRole(role, uppercase_course_key).has_user(uppercase_user))
 
     def test_course_role(self):
         """
         Test that giving a user a course role enables access appropriately
         """
-        course_locator = loc_mapper().translate_location(
-            self.course.course_id, self.course, add_entry_if_missing=True
+        self.assertFalse(
+            CourseStaffRole(self.course_id).has_user(self.student),
+            "Student has premature access to {}".format(self.course_id)
+        )
+        CourseStaffRole(self.course_id).add_users(self.student)
+        self.assertTrue(
+            CourseStaffRole(self.course_id).has_user(self.student),
+            "Student doesn't have access to {}".format(unicode(self.course_id))
         )
+
+        # remove access and confirm
+        CourseStaffRole(self.course_id).remove_users(self.student)
         self.assertFalse(
-            CourseStaffRole(course_locator).has_user(self.student),
-            "Student has premature access to {}".format(unicode(course_locator))
+            CourseStaffRole(self.course_id).has_user(self.student),
+            "Student still has access to {}".format(self.course_id)
         )
+
+    def test_org_role(self):
+        """
+        Test that giving a user an org role enables access appropriately
+        """
         self.assertFalse(
-            CourseStaffRole(self.course).has_user(self.student),
-            "Student has premature access to {}".format(self.course.url())
+            OrgStaffRole(self.course_id.org).has_user(self.student),
+            "Student has premature access to {}".format(self.course_id.org)
         )
-        CourseStaffRole(course_locator).add_users(self.student)
+        OrgStaffRole(self.course_id.org).add_users(self.student)
         self.assertTrue(
-            CourseStaffRole(course_locator).has_user(self.student),
-            "Student doesn't have access to {}".format(unicode(course_locator))
+            OrgStaffRole(self.course_id.org).has_user(self.student),
+            "Student doesn't have access to {}".format(unicode(self.course_id.org))
         )
+
+        # remove access and confirm
+        OrgStaffRole(self.course_id.org).remove_users(self.student)
+        if hasattr(self.student, '_roles'):
+            del self.student._roles
+        self.assertFalse(
+            OrgStaffRole(self.course_id.org).has_user(self.student),
+            "Student still has access to {}".format(self.course_id.org)
+        )
+
+    def test_org_and_course_roles(self):
+        """
+        Test that Org and course roles don't interfere with course roles or vice versa
+        """
+        OrgInstructorRole(self.course_id.org).add_users(self.student)
+        CourseInstructorRole(self.course_id).add_users(self.student)
         self.assertTrue(
-            CourseStaffRole(self.course).has_user(self.student),
-            "Student doesn't have access to {}".format(unicode(self.course.url()))
+            OrgInstructorRole(self.course_id.org).has_user(self.student),
+            "Student doesn't have access to {}".format(unicode(self.course_id.org))
         )
-        # now try accessing something internal to the course
-        vertical_locator = BlockUsageLocator(
-            package_id=course_locator.package_id, branch='published', block_id='madeup'
+        self.assertTrue(
+            CourseInstructorRole(self.course_id).has_user(self.student),
+            "Student doesn't have access to {}".format(unicode(self.course_id))
+        )
+
+        # remove access and confirm
+        OrgInstructorRole(self.course_id.org).remove_users(self.student)
+        self.assertFalse(
+            OrgInstructorRole(self.course_id.org).has_user(self.student),
+            "Student still has access to {}".format(self.course_id.org)
         )
-        vertical_location = self.course.replace(category='vertical', name='madeuptoo')
         self.assertTrue(
-            CourseStaffRole(vertical_locator).has_user(self.student),
-            "Student doesn't have access to {}".format(unicode(vertical_locator))
+            CourseInstructorRole(self.course_id).has_user(self.student),
+            "Student doesn't have access to {}".format(unicode(self.course_id))
         )
+
+        # ok now keep org role and get rid of course one
+        OrgInstructorRole(self.course_id.org).add_users(self.student)
+        CourseInstructorRole(self.course_id).remove_users(self.student)
         self.assertTrue(
-            CourseStaffRole(vertical_location, course_context=self.course.course_id).has_user(self.student),
-            "Student doesn't have access to {}".format(unicode(vertical_location.url()))
+            OrgInstructorRole(self.course_id.org).has_user(self.student),
+            "Student lost has access to {}".format(self.course_id.org)
         )
+        self.assertFalse(
+            CourseInstructorRole(self.course_id).has_user(self.student),
+            "Student doesn't have access to {}".format(unicode(self.course_id))
+        )
+
+
+    def test_get_user_for_role(self):
+        """
+        test users_for_role
+        """
+        role = CourseStaffRole(self.course_id)
+        role.add_users(self.student)
+        self.assertGreater(len(role.users_with_role()), 0)
+
+    def test_add_users_doesnt_add_duplicate_entry(self):
+        """
+        Tests that calling add_users multiple times before a single call
+        to remove_users does not result in the user remaining in the group.
+        """
+        role = CourseStaffRole(self.course_id)
+        role.add_users(self.student)
+        self.assertTrue(role.has_user(self.student))
+        # Call add_users a second time, then remove just once.
+        role.add_users(self.student)
+        role.remove_users(self.student)
+        self.assertFalse(role.has_user(self.student))

common/djangoapps/student/tests/tests.py

@@ -20,6 +20,7 @@ from django.http import HttpResponse
 from xmodule.modulestore.tests.factories import CourseFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from courseware.tests.tests import TEST_DATA_MIXED_MODULESTORE
+from xmodule.modulestore.locations import SlashSeparatedCourseKey
 
 from mock import Mock, patch, sentinel
 
@@ -30,9 +31,6 @@ from student.tests.factories import UserFactory, CourseModeFactory
 
 import shoppingcart
 
-COURSE_1 = 'edX/toy/2012_Fall'
-COURSE_2 = 'edx/full/6.002_Spring_2012'
-
 log = logging.getLogger(__name__)
 
 
@@ -203,38 +201,33 @@ class EnrollInCourseTest(TestCase):
 
     def test_enrollment(self):
         user = User.objects.create_user("joe", "joe@joe.com", "password")
-        course_id = "edX/Test101/2013"
-        course_id_partial = "edX/Test101"
+        course_id = SlashSeparatedCourseKey("edX", "Test101", "2013")
+        course_id_partial = SlashSeparatedCourseKey("edX", "Test101", None)
 
         # Test basic enrollment
         self.assertFalse(CourseEnrollment.is_enrolled(user, course_id))
-        self.assertFalse(CourseEnrollment.is_enrolled_by_partial(user,
-            course_id_partial))
+        self.assertFalse(CourseEnrollment.is_enrolled_by_partial(user, course_id_partial))
         CourseEnrollment.enroll(user, course_id)
         self.assertTrue(CourseEnrollment.is_enrolled(user, course_id))
-        self.assertTrue(CourseEnrollment.is_enrolled_by_partial(user,
-            course_id_partial))
+        self.assertTrue(CourseEnrollment.is_enrolled_by_partial(user, course_id_partial))
         self.assert_enrollment_event_was_emitted(user, course_id)
 
         # Enrolling them again should be harmless
         CourseEnrollment.enroll(user, course_id)
         self.assertTrue(CourseEnrollment.is_enrolled(user, course_id))
-        self.assertTrue(CourseEnrollment.is_enrolled_by_partial(user,
-            course_id_partial))
+        self.assertTrue(CourseEnrollment.is_enrolled_by_partial(user, course_id_partial))
         self.assert_no_events_were_emitted()
 
         # Now unenroll the user
         CourseEnrollment.unenroll(user, course_id)
         self.assertFalse(CourseEnrollment.is_enrolled(user, course_id))
-        self.assertFalse(CourseEnrollment.is_enrolled_by_partial(user,
-            course_id_partial))
+        self.assertFalse(CourseEnrollment.is_enrolled_by_partial(user, course_id_partial))
         self.assert_unenrollment_event_was_emitted(user, course_id)
 
         # Unenrolling them again should also be harmless
         CourseEnrollment.unenroll(user, course_id)
         self.assertFalse(CourseEnrollment.is_enrolled(user, course_id))
-        self.assertFalse(CourseEnrollment.is_enrolled_by_partial(user,
-            course_id_partial))
+        self.assertFalse(CourseEnrollment.is_enrolled_by_partial(user, course_id_partial))
         self.assert_no_events_were_emitted()
 
         # The enrollment record should still exist, just be inactive
@@ -257,26 +250,26 @@ class EnrollInCourseTest(TestCase):
         self.assertFalse(self.mock_server_track.called)
         self.mock_server_track.reset_mock()
 
-    def assert_enrollment_event_was_emitted(self, user, course_id):
+    def assert_enrollment_event_was_emitted(self, user, course_key):
         """Ensures an enrollment event was emitted since the last event related assertion"""
         self.mock_server_track.assert_called_once_with(
             sentinel.request,
             'edx.course.enrollment.activated',
             {
-                'course_id': course_id,
+                'course_id': course_key.to_deprecated_string(),
                 'user_id': user.pk,
                 'mode': 'honor'
             }
         )
         self.mock_server_track.reset_mock()
 
-    def assert_unenrollment_event_was_emitted(self, user, course_id):
+    def assert_unenrollment_event_was_emitted(self, user, course_key):
         """Ensures an unenrollment event was emitted since the last event related assertion"""
         self.mock_server_track.assert_called_once_with(
             sentinel.request,
             'edx.course.enrollment.deactivated',
             {
-                'course_id': course_id,
+                'course_id': course_key.to_deprecated_string(),
                 'user_id': user.pk,
                 'mode': 'honor'
             }
@@ -286,7 +279,7 @@ class EnrollInCourseTest(TestCase):
     def test_enrollment_non_existent_user(self):
         # Testing enrollment of newly unsaved user (i.e. no database entry)
         user = User(username="rusty", email="rusty@fake.edx.org")
-        course_id = "edX/Test101/2013"
+        course_id = SlashSeparatedCourseKey("edX", "Test101", "2013")
 
         self.assertFalse(CourseEnrollment.is_enrolled(user, course_id))
 
@@ -302,7 +295,7 @@ class EnrollInCourseTest(TestCase):
 
     def test_enrollment_by_email(self):
         user = User.objects.create(username="jack", email="jack@fake.edx.org")
-        course_id = "edX/Test101/2013"
+        course_id = SlashSeparatedCourseKey("edX", "Test101", "2013")
 
         CourseEnrollment.enroll_by_email("jack@fake.edx.org", course_id)
         self.assertTrue(CourseEnrollment.is_enrolled(user, course_id))
@@ -339,8 +332,8 @@ class EnrollInCourseTest(TestCase):
 
     def test_enrollment_multiple_classes(self):
         user = User(username="rusty", email="rusty@fake.edx.org")
-        course_id1 = "edX/Test101/2013"
-        course_id2 = "MITx/6.003z/2012"
+        course_id1 = SlashSeparatedCourseKey("edX", "Test101", "2013")
+        course_id2 = SlashSeparatedCourseKey("MITx", "6.003z", "2012")
 
         CourseEnrollment.enroll(user, course_id1)
         self.assert_enrollment_event_was_emitted(user, course_id1)
@@ -361,7 +354,7 @@ class EnrollInCourseTest(TestCase):
 
     def test_activation(self):
         user = User.objects.create(username="jack", email="jack@fake.edx.org")
-        course_id = "edX/Test101/2013"
+        course_id = SlashSeparatedCourseKey("edX", "Test101", "2013")
         self.assertFalse(CourseEnrollment.is_enrolled(user, course_id))
 
         # Creating an enrollment doesn't actually enroll a student
@@ -416,7 +409,7 @@ class PaidRegistrationTest(ModuleStoreTestCase):
 
     @unittest.skipUnless(settings.FEATURES.get('ENABLE_SHOPPING_CART'), "Shopping Cart not enabled in settings")
     def test_change_enrollment_add_to_cart(self):
-        request = self.req_factory.post(reverse('change_enrollment'), {'course_id': self.course.id,
+        request = self.req_factory.post(reverse('change_enrollment'), {'course_id': self.course.id.to_deprecated_string(),
                                                                        'enrollment_action': 'add_to_cart'})
         request.user = self.user
         response = change_enrollment(request)