From 4bc5ded7683f7130c649d52e249b035574d4c54c Mon Sep 17 00:00:00 2001 From: Awais Jibran Date: Wed, 27 Jul 2016 16:47:04 +0500 Subject: [PATCH] Make studio case-insensitive for course keys for creating courses. ECOM-4890 --- .../commands/tests/test_create_course.py | 32 ++++++++++ .../contentstore/tests/test_contentstore.py | 62 +++++++++++++++---- .../xmodule/xmodule/modulestore/__init__.py | 28 ++++----- .../lib/xmodule/xmodule/modulestore/mixed.py | 4 +- .../xmodule/xmodule/modulestore/mongo/base.py | 2 +- .../xmodule/modulestore/split_mongo/split.py | 6 +- .../modulestore/split_mongo/split_draft.py | 2 +- .../tests/test_mixed_modulestore.py | 12 ++++ .../test_split_modulestore_bulk_operations.py | 2 +- lms/djangoapps/ccx/modulestore.py | 4 +- 10 files changed, 117 insertions(+), 37 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_create_course.py b/cms/djangoapps/contentstore/management/commands/tests/test_create_course.py index 32fff608a4..02f37e8b16 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_create_course.py +++ b/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.') diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 1d83b52a02..921ca75507 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/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): """ diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 0607cc0c06..e3502d494f 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/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. """ diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 0f726bbaf0..9eaa483f2c 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/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): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 42531604e6..2efb6d96ef 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/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 """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 2718fc793d..b186ca82ec 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index 31ded9eba9..863bd28d53 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/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 ) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 43dbdd18ee..9e41349f18 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py index a19c9f0494..5c960fb208 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py +++ b/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): diff --git a/lms/djangoapps/ccx/modulestore.py b/lms/djangoapps/ccx/modulestore.py index 6d99f6e797..2b3b8cd627 100644 --- a/lms/djangoapps/ccx/modulestore.py +++ b/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