Merge pull request #13111 from edx/aj/ECOM4890-make-studio-case-insensitive-for-create-course

Make studio case-insensitive for course keys for creating courses.

cms/djangoapps/contentstore/management/commands/tests/test_create_course.py

@@ -61,3 +61,35 @@ class TestCreateCourse(ModuleStoreTestCase):
         )
         # pylint: disable=protected-access
         self.assertEqual(store, modulestore()._get_modulestore_for_courselike(new_key).get_modulestore_type())
+
+    @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
+    def test_get_course_with_different_case(self, default_store):
+        """
+        Tests that course can not be accessed with different case.
+
+        Scenario:
+            Create a course with lower case keys inside `bulk_operations` with `ignore_case=True`.
+            Verify that course is created.
+            Verify that get course from store using same course id but different case is not accessible.
+        """
+        org = 'org1'
+        number = 'course1'
+        run = 'run1'
+        with self.store.default_store(default_store):
+            lowercase_course_id = self.store.make_course_key(org, number, run)
+            with self.store.bulk_operations(lowercase_course_id, ignore_case=True):
+                # Create course with lowercase key & Verify that store returns course.
+                self.store.create_course(
+                    lowercase_course_id.org,
+                    lowercase_course_id.course,
+                    lowercase_course_id.run,
+                    self.user.id
+                )
+                course = self.store.get_course(lowercase_course_id)
+                self.assertIsNotNone(course, 'Course not found using lowercase course key.')
+                self.assertEqual(unicode(course.id), unicode(lowercase_course_id))
+
+                # Verify store does not return course with different case.
+                uppercase_course_id = self.store.make_course_key(org.upper(), number.upper(), run.upper())
+                course = self.store.get_course(uppercase_course_id)
+                self.assertIsNone(course, 'Course should not be accessed with uppercase course id.')

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -1151,6 +1151,9 @@ class ContentStoreTest(ContentStoreTestCase, XssTestMixin):
     """
     Tests for the CMS ContentStore application.
     """
+    duplicate_course_error = ("There is already a course defined with the same organization and course number. "
+                              "Please change either organization or course number to be unique.")
+
     def setUp(self):
         super(ContentStoreTest, self).setUp()
 
@@ -1203,6 +1206,22 @@ class ContentStoreTest(ContentStoreTestCase, XssTestMixin):
         self.course_data['run'] = 'run.name'
         self.assert_created_course()
 
+    @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
+    def test_course_with_different_cases(self, default_store):
+        """
+        Tests that course can not be created with different case using an AJAX request to
+        course handler.
+        """
+        course_number = '99x'
+        with self.store.default_store(default_store):
+            # Verify create a course passes with lower case.
+            self.course_data['number'] = course_number.lower()
+            self.assert_created_course()
+
+            # Verify create a course fail when same course number is provided with different case.
+            self.course_data['number'] = course_number.upper()
+            self.assert_course_creation_failed(self.duplicate_course_error)
+
     def test_create_course_check_forum_seeding(self):
         """Test new course creation and verify forum seeding """
         test_course_data = self.assert_created_course(number_suffix=uuid4().hex)
@@ -1289,7 +1308,7 @@ class ContentStoreTest(ContentStoreTestCase, XssTestMixin):
     def test_create_course_duplicate_course(self):
         """Test new course creation - error path"""
         self.client.ajax_post('/course/', self.course_data)
-        self.assert_course_creation_failed('There is already a course defined with the same organization and course number. Please change either organization or course number to be unique.')
+        self.assert_course_creation_failed(self.duplicate_course_error)
 
     def assert_course_creation_failed(self, error_message):
         """
@@ -1318,21 +1337,38 @@ class ContentStoreTest(ContentStoreTestCase, XssTestMixin):
         self.course_data['display_name'] = 'Robot Super Course Two'
         self.course_data['run'] = '2013_Summer'
 
-        self.assert_course_creation_failed('There is already a course defined with the same organization and course number. Please change either organization or course number to be unique.')
+        self.assert_course_creation_failed(self.duplicate_course_error)
 
-    def test_create_course_case_change(self):
+    @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
+    def test_create_course_case_change(self, default_store):
         """Test new course creation - error path due to case insensitive name equality"""
-        self.course_data['number'] = 'capital'
-        self.client.ajax_post('/course/', self.course_data)
-        cache_current = self.course_data['org']
-        self.course_data['org'] = self.course_data['org'].lower()
-        self.assert_course_creation_failed('There is already a course defined with the same organization and course number. Please change either organization or course number to be unique.')
-        self.course_data['org'] = cache_current
+        self.course_data['number'] = '99x'
 
-        self.client.ajax_post('/course/', self.course_data)
-        cache_current = self.course_data['number']
-        self.course_data['number'] = self.course_data['number'].upper()
-        self.assert_course_creation_failed('There is already a course defined with the same organization and course number. Please change either organization or course number to be unique.')
+        with self.store.default_store(default_store):
+
+            # Verify that the course was created properly.
+            self.assert_created_course()
+
+            # Keep the copy of original org
+            cache_current = self.course_data['org']
+
+            # Change `org` to lower case and verify that course did not get created
+            self.course_data['org'] = self.course_data['org'].lower()
+            self.assert_course_creation_failed(self.duplicate_course_error)
+
+            # Replace the org with its actual value, and keep the copy of course number.
+            self.course_data['org'] = cache_current
+            cache_current = self.course_data['number']
+
+            self.course_data['number'] = self.course_data['number'].upper()
+            self.assert_course_creation_failed(self.duplicate_course_error)
+
+            # Replace the org with its actual value, and keep the copy of course number.
+            self.course_data['number'] = cache_current
+            __ = self.course_data['run']
+
+            self.course_data['run'] = self.course_data['run'].upper()
+            self.assert_course_creation_failed(self.duplicate_course_error)
 
     def test_course_substring(self):
         """

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -178,7 +178,7 @@ class BulkOperationsMixin(object):
         self.signal_handler = None
 
     @contextmanager
-    def bulk_operations(self, course_id, emit_signals=True):
+    def bulk_operations(self, course_id, emit_signals=True, ignore_case=False):
         """
         A context manager for notifying the store of bulk operations. This affects only the current thread.
 
@@ -186,10 +186,10 @@ class BulkOperationsMixin(object):
         until the bulk operation is completed.
         """
         try:
-            self._begin_bulk_operation(course_id)
+            self._begin_bulk_operation(course_id, ignore_case)
             yield
         finally:
-            self._end_bulk_operation(course_id, emit_signals)
+            self._end_bulk_operation(course_id, emit_signals, ignore_case)
 
     # the relevant type of bulk_ops_record for the mixin (overriding classes should override
     # this variable)
@@ -206,10 +206,10 @@ class BulkOperationsMixin(object):
         if ignore_case:
             for key, record in self._active_bulk_ops.records.iteritems():
                 # Shortcut: check basic equivalence for cases where org/course/run might be None.
-                if key == course_key or (
-                    key.org.lower() == course_key.org.lower() and
-                    key.course.lower() == course_key.course.lower() and
-                    key.run.lower() == course_key.run.lower()
+                if (key == course_key) or (
+                        (key.org and key.org.lower() == course_key.org.lower()) and
+                        (key.course and key.course.lower() == course_key.course.lower()) and
+                        (key.run and key.run.lower() == course_key.run.lower())
                 ):
                     return record
 
@@ -231,7 +231,7 @@ class BulkOperationsMixin(object):
         if course_key.for_branch(None) in self._active_bulk_ops.records:
             del self._active_bulk_ops.records[course_key.for_branch(None)]
 
-    def _start_outermost_bulk_operation(self, bulk_ops_record, course_key):
+    def _start_outermost_bulk_operation(self, bulk_ops_record, course_key, ignore_case=False):
         """
         The outermost nested bulk_operation call: do the actual begin of the bulk operation.
 
@@ -239,11 +239,11 @@ class BulkOperationsMixin(object):
         """
         pass
 
-    def _begin_bulk_operation(self, course_key):
+    def _begin_bulk_operation(self, course_key, ignore_case=False):
         """
         Begin a bulk operation on course_key.
         """
-        bulk_ops_record = self._get_bulk_ops_record(course_key)
+        bulk_ops_record = self._get_bulk_ops_record(course_key, ignore_case)
 
         # Increment the number of active bulk operations (bulk operations
         # on the same course can be nested)
@@ -251,7 +251,7 @@ class BulkOperationsMixin(object):
 
         # If this is the highest level bulk operation, then initialize it
         if bulk_ops_record.is_root:
-            self._start_outermost_bulk_operation(bulk_ops_record, course_key)
+            self._start_outermost_bulk_operation(bulk_ops_record, course_key, ignore_case)
 
     def _end_outermost_bulk_operation(self, bulk_ops_record, structure_key):
         """
@@ -261,12 +261,12 @@ class BulkOperationsMixin(object):
         """
         pass
 
-    def _end_bulk_operation(self, structure_key, emit_signals=True):
+    def _end_bulk_operation(self, structure_key, emit_signals=True, ignore_case=False):
         """
         End the active bulk operation on structure_key (course or library key).
         """
         # If no bulk op is active, return
-        bulk_ops_record = self._get_bulk_ops_record(structure_key)
+        bulk_ops_record = self._get_bulk_ops_record(structure_key, ignore_case)
         if not bulk_ops_record.active:
             return
 
@@ -1017,7 +1017,7 @@ class ModuleStoreRead(ModuleStoreAssetBase):
         pass
 
     @contextmanager
-    def bulk_operations(self, course_id, emit_signals=True):    # pylint: disable=unused-argument
+    def bulk_operations(self, course_id, emit_signals=True, ignore_case=False):    # pylint: disable=unused-argument
         """
         A context manager for notifying the store of bulk operations. This affects only the current thread.
         """

common/lib/xmodule/xmodule/modulestore/mixed.py

@@ -984,13 +984,13 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
             yield
 
     @contextmanager
-    def bulk_operations(self, course_id, emit_signals=True):
+    def bulk_operations(self, course_id, emit_signals=True, ignore_case=False):
         """
         A context manager for notifying the store of bulk operations.
         If course_id is None, the default store is used.
         """
         store = self._get_modulestore_for_courselike(course_id)
-        with store.bulk_operations(course_id, emit_signals):
+        with store.bulk_operations(course_id, emit_signals, ignore_case):
             yield
 
     def ensure_indexes(self):

common/lib/xmodule/xmodule/modulestore/mongo/base.py

@@ -482,7 +482,7 @@ class MongoBulkOpsMixin(BulkOperationsMixin):
     """
     _bulk_ops_record_type = MongoBulkOpsRecord
 
-    def _start_outermost_bulk_operation(self, bulk_ops_record, course_key):
+    def _start_outermost_bulk_operation(self, bulk_ops_record, course_key, ignore_case=False):
         """
         Prevent updating the meta-data inheritance cache for the given course
         """

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -226,11 +226,11 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
                 course_key.replace(org=None, course=None, run=None, branch=None)
             ]
 
-    def _start_outermost_bulk_operation(self, bulk_write_record, course_key):
+    def _start_outermost_bulk_operation(self, bulk_write_record, course_key, ignore_case=False):
         """
         Begin a bulk write operation on course_key.
         """
-        bulk_write_record.initial_index = self.db_connection.get_course_index(course_key)
+        bulk_write_record.initial_index = self.db_connection.get_course_index(course_key, ignore_case=ignore_case)
         # Ensure that any edits to the index don't pollute the initial_index
         bulk_write_record.index = copy.deepcopy(bulk_write_record.initial_index)
         bulk_write_record.course_key = course_key
@@ -1884,7 +1884,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         """
         Internal code for creating a course or library
         """
-        index = self.get_course_index(locator)
+        index = self.get_course_index(locator, ignore_case=True)
         if index is not None:
             raise DuplicateCourseError(locator, index)
 

common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py

@@ -33,7 +33,7 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
         Returns: a CourseDescriptor
         """
         master_branch = kwargs.pop('master_branch', ModuleStoreEnum.BranchName.draft)
-        with self.bulk_operations(CourseLocator(org, course, run)):
+        with self.bulk_operations(CourseLocator(org, course, run), ignore_case=True):
             item = super(DraftVersioningModuleStore, self).create_course(
                 org, course, run, user_id, master_branch=master_branch, **kwargs
             )

common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py

@@ -357,6 +357,18 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
             with self.assertRaises(DuplicateCourseError):
                 self.store.create_course('org_x', 'course_y', 'run_z', self.user_id)
 
+    @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
+    def test_duplicate_course_error_with_different_case_ids(self, default_store):
+        """
+        Verify that course can not be created with same course_id with different case.
+        """
+        self._initialize_mixed(mappings={})
+        with self.store.default_store(default_store):
+            self.store.create_course('org_x', 'course_y', 'run_z', self.user_id)
+
+            with self.assertRaises(DuplicateCourseError):
+                self.store.create_course('ORG_X', 'COURSE_Y', 'RUN_Z', self.user_id)
+
     # Draft:
     #    problem: One lookup to locate an item that exists
     #    fake: one w/ wildcard version

common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py

@@ -704,7 +704,7 @@ class TestBulkWriteMixinOpen(TestBulkWriteMixin):
             from_index=self.conn.get_course_index.return_value,
             course_context=self.course_key,
         )
-        self.conn.get_course_index.assert_called_once_with(self.course_key)
+        self.conn.get_course_index.assert_called_once_with(self.course_key, ignore_case=False)
 
 
 class TestBulkWriteMixinOpenAfterPrevTransaction(TestBulkWriteMixinOpen, TestBulkWriteMixinPreviousTransaction):

lms/djangoapps/ccx/modulestore.py

@@ -296,8 +296,8 @@ class CCXModulestoreWrapper(object):
             yield
 
     @contextmanager
-    def bulk_operations(self, course_id, emit_signals=True):
+    def bulk_operations(self, course_id, emit_signals=True, ignore_case=False):
         """See the docs for xmodule.modulestore.mixed.MixedModuleStore"""
         course_id, _ = strip_ccx(course_id)
-        with self._modulestore.bulk_operations(course_id, emit_signals=emit_signals):
+        with self._modulestore.bulk_operations(course_id, emit_signals=emit_signals, ignore_case=ignore_case):
             yield