diff --git a/cms/djangoapps/contentstore/views/access.py b/cms/djangoapps/contentstore/views/access.py index 5e81ce3091..6e9eab48b6 100644 --- a/cms/djangoapps/contentstore/views/access.py +++ b/cms/djangoapps/contentstore/views/access.py @@ -20,7 +20,8 @@ def has_course_access(user, course_key, role=CourseStaffRole): return True if OrgStaffRole(org=course_key.org).has_user(user): return True - return auth.has_access(user, role(course_key)) + # temporary to ensure we give universal access given a course until we impl branch specific perms + return auth.has_access(user, role(course_key.for_branch(None))) def get_user_role(user, course_id): diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 8ba35628c2..1b58c3e0a5 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -567,7 +567,11 @@ def _create_new_course(request, org, number, run, fields): fields.update(definition_data) store = modulestore() - with store.default_store(settings.FEATURES.get('DEFAULT_STORE_FOR_NEW_COURSE', 'mongo')): + store_for_new_course = ( + settings.FEATURES.get('DEFAULT_STORE_FOR_NEW_COURSE') or + store.default_modulestore.get_modulestore_type() + ) + with store.default_store(store_for_new_course): # Creating the course raises DuplicateCourseError if an existing course with this org/name is found new_course = store.create_course( org, @@ -584,7 +588,8 @@ def _create_new_course(request, org, number, run, fields): initialize_permissions(new_course.id, request.user) return JsonResponse({ - 'url': reverse_course_url('course_handler', new_course.id) + 'url': reverse_course_url('course_handler', new_course.id), + 'course_key': unicode(new_course.id), }) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index e7aa4d55cc..7d043d840b 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -154,7 +154,7 @@ def xblock_handler(request, usage_key_string): request.user, _get_xblock(usage_key, request.user), data=request.json.get('data'), - children=request.json.get('children'), + children_strings=request.json.get('children'), metadata=request.json.get('metadata'), nullout=request.json.get('nullout'), grader_type=request.json.get('graderType'), @@ -301,7 +301,23 @@ def xblock_outline_handler(request, usage_key_string): return Http404 -def _save_xblock(user, xblock, data=None, children=None, metadata=None, nullout=None, +def _update_with_callback(xblock, user, old_metadata=None, old_content=None): + """ + Updates the xblock in the modulestore. + But before doing so, it calls the xblock's editor_saved callback function. + """ + if callable(getattr(xblock, "editor_saved", None)): + if old_metadata is None: + old_metadata = own_metadata(xblock) + if old_content is None: + old_content = xblock.get_explicitly_set_fields_by_scope(Scope.content) + xblock.editor_saved(user, old_metadata, old_content) + + # Update after the callback so any changes made in the callback will get persisted. + modulestore().update_item(xblock, user.id) + + +def _save_xblock(user, xblock, data=None, children_strings=None, metadata=None, nullout=None, grader_type=None, publish=None): """ Saves xblock w/ its fields. Has special processing for grader_type, publish, and nullout and Nones in metadata. @@ -309,94 +325,127 @@ def _save_xblock(user, xblock, data=None, children=None, metadata=None, nullout= to default). """ store = modulestore() + # Perform all xblock changes within a (single-versioned) transaction + with store.bulk_operations(xblock.location.course_key): - # Don't allow updating an xblock and discarding changes in a single operation (unsupported by UI). - if publish == "discard_changes": - store.revert_to_published(xblock.location, user.id) - # Returning the same sort of result that we do for other save operations. In the future, - # we may want to return the full XBlockInfo. - return JsonResponse({'id': unicode(xblock.location)}) + # Don't allow updating an xblock and discarding changes in a single operation (unsupported by UI). + if publish == "discard_changes": + store.revert_to_published(xblock.location, user.id) + # Returning the same sort of result that we do for other save operations. In the future, + # we may want to return the full XBlockInfo. + return JsonResponse({'id': unicode(xblock.location)}) - old_metadata = own_metadata(xblock) - old_content = xblock.get_explicitly_set_fields_by_scope(Scope.content) + old_metadata = own_metadata(xblock) + old_content = xblock.get_explicitly_set_fields_by_scope(Scope.content) - if data: - # TODO Allow any scope.content fields not just "data" (exactly like the get below this) - xblock.data = data - else: - data = old_content['data'] if 'data' in old_content else None + if data: + # TODO Allow any scope.content fields not just "data" (exactly like the get below this) + xblock.data = data + else: + data = old_content['data'] if 'data' in old_content else None - if children is not None: - children_usage_keys = [] - for child in children: - child_usage_key = usage_key_with_run(child) - children_usage_keys.append(child_usage_key) - xblock.children = children_usage_keys + if children_strings is not None: + children = [] + for child_string in children_strings: + children.append(usage_key_with_run(child_string)) - # also commit any metadata which might have been passed along - if nullout is not None or metadata is not None: - # the postback is not the complete metadata, as there's system metadata which is - # not presented to the end-user for editing. So let's use the original (existing_item) and - # 'apply' the submitted metadata, so we don't end up deleting system metadata. - if nullout is not None: - for metadata_key in nullout: - setattr(xblock, metadata_key, None) - - # update existing metadata with submitted metadata (which can be partial) - # IMPORTANT NOTE: if the client passed 'null' (None) for a piece of metadata that means 'remove it'. If - # the intent is to make it None, use the nullout field - if metadata is not None: - for metadata_key, value in metadata.items(): - field = xblock.fields[metadata_key] - - if value is None: - field.delete_from(xblock) + # if new children have been added, remove them from their old parents + new_children = set(children) - set(xblock.children) + for new_child in new_children: + old_parent_location = store.get_parent_location(new_child) + if old_parent_location: + old_parent = store.get_item(old_parent_location) + old_parent.children.remove(new_child) + _update_with_callback(old_parent, user) else: - try: - value = field.from_json(value) - except ValueError: - return JsonResponse({"error": "Invalid data"}, 400) - field.write_to(xblock, value) + # the Studio UI currently doesn't present orphaned children, so assume this is an error + return JsonResponse({"error": "Invalid data, possibly caused by concurrent authors."}, 400) - if callable(getattr(xblock, "editor_saved", None)): - xblock.editor_saved(user, old_metadata, old_content) + # make sure there are no old children that became orphans + # In a single-author (no-conflict) scenario, all children in the persisted list on the server should be + # present in the updated list. If there are any children that have been dropped as part of this update, + # then that would be an error. + # + # We can be even more restrictive in a multi-author (conflict), by returning an error whenever + # len(old_children) > 0. However, that conflict can still be "merged" if the dropped child had been + # re-parented. Hence, the check for the parent in the any statement below. + # + # Note that this multi-author conflict error should not occur in modulestores (such as Split) that support + # atomic write transactions. In Split, if there was another author who moved one of the "old_children" + # into another parent, then that child would have been deleted from this parent on the server. However, + # this is error could occur in modulestores (such as Draft) that do not support atomic write-transactions + old_children = set(xblock.children) - set(children) + if any( + store.get_parent_location(old_child) == xblock.location + for old_child in old_children + ): + # since children are moved as part of a single transaction, orphans should not be created + return JsonResponse({"error": "Invalid data, possibly caused by concurrent authors."}, 400) - # commit to datastore - store.update_item(xblock, user.id) + # set the children on the xblock + xblock.children = children - # for static tabs, their containing course also records their display name - if xblock.location.category == 'static_tab': - course = store.get_course(xblock.location.course_key) - # find the course's reference to this tab and update the name. - static_tab = CourseTabList.get_tab_by_slug(course.tabs, xblock.location.name) - # only update if changed - if static_tab and static_tab['name'] != xblock.display_name: - static_tab['name'] = xblock.display_name - store.update_item(course, user.id) + # also commit any metadata which might have been passed along + if nullout is not None or metadata is not None: + # the postback is not the complete metadata, as there's system metadata which is + # not presented to the end-user for editing. So let's use the original (existing_item) and + # 'apply' the submitted metadata, so we don't end up deleting system metadata. + if nullout is not None: + for metadata_key in nullout: + setattr(xblock, metadata_key, None) - result = { - 'id': unicode(xblock.location), - 'data': data, - 'metadata': own_metadata(xblock) - } + # update existing metadata with submitted metadata (which can be partial) + # IMPORTANT NOTE: if the client passed 'null' (None) for a piece of metadata that means 'remove it'. If + # the intent is to make it None, use the nullout field + if metadata is not None: + for metadata_key, value in metadata.items(): + field = xblock.fields[metadata_key] - if grader_type is not None: - result.update(CourseGradingModel.update_section_grader_type(xblock, grader_type, user)) + if value is None: + field.delete_from(xblock) + else: + try: + value = field.from_json(value) + except ValueError: + return JsonResponse({"error": "Invalid data"}, 400) + field.write_to(xblock, value) - # If publish is set to 'republish' and this item is not in direct only categories and has previously been published, - # then this item should be republished. This is used by staff locking to ensure that changing the draft - # value of the staff lock will also update the published version, but only at the unit level. - if publish == 'republish' and xblock.category not in DIRECT_ONLY_CATEGORIES: - if modulestore().has_published_version(xblock): - publish = 'make_public' + # update the xblock and call any xblock callbacks + _update_with_callback(xblock, user, old_metadata, old_content) - # Make public after updating the xblock, in case the caller asked for both an update and a publish. - # Used by Bok Choy tests and by republishing of staff locks. - if publish == 'make_public': - modulestore().publish(xblock.location, user.id) + # for static tabs, their containing course also records their display name + if xblock.location.category == 'static_tab': + course = store.get_course(xblock.location.course_key) + # find the course's reference to this tab and update the name. + static_tab = CourseTabList.get_tab_by_slug(course.tabs, xblock.location.name) + # only update if changed + if static_tab and static_tab['name'] != xblock.display_name: + static_tab['name'] = xblock.display_name + store.update_item(course, user.id) - # Note that children aren't being returned until we have a use case. - return JsonResponse(result, encoder=EdxJSONEncoder) + result = { + 'id': unicode(xblock.location), + 'data': data, + 'metadata': own_metadata(xblock) + } + + if grader_type is not None: + result.update(CourseGradingModel.update_section_grader_type(xblock, grader_type, user)) + + # If publish is set to 'republish' and this item is not in direct only categories and has previously been published, + # then this item should be republished. This is used by staff locking to ensure that changing the draft + # value of the staff lock will also update the published version, but only at the unit level. + if publish == 'republish' and xblock.category not in DIRECT_ONLY_CATEGORIES: + if modulestore().has_published_version(xblock): + publish = 'make_public' + + # Make public after updating the xblock, in case the caller asked for both an update and a publish. + # Used by Bok Choy tests and by republishing of staff locks. + if publish == 'make_public': + modulestore().publish(xblock.location, user.id) + + # Note that children aren't being returned until we have a use case. + return JsonResponse(result, encoder=EdxJSONEncoder) @login_required diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 53bb73fe90..79ef4b16a9 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -480,10 +480,16 @@ class TestEditItem(ItemTest): display_name = 'chapter created' resp = self.create_xblock(display_name=display_name, category='chapter') chap_usage_key = self.response_usage_key(resp) + + # create 2 sequentials resp = self.create_xblock(parent_usage_key=chap_usage_key, category='sequential') self.seq_usage_key = self.response_usage_key(resp) self.seq_update_url = reverse_usage_url("xblock_handler", self.seq_usage_key) + resp = self.create_xblock(parent_usage_key=chap_usage_key, category='sequential') + self.seq2_usage_key = self.response_usage_key(resp) + self.seq2_update_url = reverse_usage_url("xblock_handler", self.seq2_usage_key) + # create problem w/ boilerplate template_id = 'multiplechoice.yaml' resp = self.create_xblock(parent_usage_key=self.seq_usage_key, category='problem', boilerplate=template_id) @@ -557,11 +563,8 @@ class TestEditItem(ItemTest): self.assertIn(chapter2_usage_key, course.children) # Remove one child from the course. - resp = self.client.ajax_post( - self.course_update_url, - data={'children': [unicode(chapter2_usage_key)]} - ) - self.assertEqual(resp.status_code, 200) + resp = self.client.delete(reverse_usage_url("xblock_handler", chapter1_usage_key)) + self.assertEqual(resp.status_code, 204) # Verify that the child is removed. course = self.get_item_from_modulestore(self.usage_key) @@ -597,6 +600,79 @@ class TestEditItem(ItemTest): self.assertEqual(unit1_usage_key, children[2]) self.assertEqual(unit2_usage_key, children[1]) + def test_move_parented_child(self): + """ + Test moving a child from one Section to another + """ + unit_1_key = self.response_usage_key( + self.create_xblock(parent_usage_key=self.seq_usage_key, category='vertical', display_name='unit 1') + ) + unit_2_key = self.response_usage_key( + self.create_xblock(parent_usage_key=self.seq2_usage_key, category='vertical', display_name='unit 2') + ) + + # move unit 1 from sequential1 to sequential2 + resp = self.client.ajax_post( + self.seq2_update_url, + data={'children': [unicode(unit_1_key), unicode(unit_2_key)]} + ) + self.assertEqual(resp.status_code, 200) + + # verify children + self.assertListEqual( + self.get_item_from_modulestore(self.seq2_usage_key).children, + [unit_1_key, unit_2_key], + ) + self.assertListEqual( + self.get_item_from_modulestore(self.seq_usage_key).children, + [self.problem_usage_key], # problem child created in setUp + ) + + def test_move_orphaned_child_error(self): + """ + Test moving an orphan returns an error + """ + unit_1_key = self.store.create_item(self.user.id, self.course_key, 'vertical', 'unit1').location + + # adding orphaned unit 1 should return an error + resp = self.client.ajax_post( + self.seq2_update_url, + data={'children': [unicode(unit_1_key)]} + ) + self.assertEqual(resp.status_code, 400) + self.assertIn("Invalid data, possibly caused by concurrent authors", resp.content) + + # verify children + self.assertListEqual( + self.get_item_from_modulestore(self.seq2_usage_key).children, + [] + ) + + def test_move_child_creates_orphan_error(self): + """ + Test creating an orphan returns an error + """ + unit_1_key = self.response_usage_key( + self.create_xblock(parent_usage_key=self.seq2_usage_key, category='vertical', display_name='unit 1') + ) + unit_2_key = self.response_usage_key( + self.create_xblock(parent_usage_key=self.seq2_usage_key, category='vertical', display_name='unit 2') + ) + + # remove unit 2 should return an error + resp = self.client.ajax_post( + self.seq2_update_url, + data={'children': [unicode(unit_1_key)]} + ) + self.assertEqual(resp.status_code, 400) + self.assertIn("Invalid data, possibly caused by concurrent authors", resp.content) + + # verify children + self.assertListEqual( + self.get_item_from_modulestore(self.seq2_usage_key).children, + [unit_1_key, unit_2_key] + ) + def _is_location_published(self, location): """ Returns whether or not the item with given location has a published version. @@ -954,44 +1030,6 @@ class TestEditSplitModule(ItemTest): self.assertEqual(2, len(split_test.children)) self.assertEqual(initial_group_id_to_child, split_test.group_id_to_child) - def test_delete_children(self): - """ - Test that deleting a child in the group_id_to_child map updates the map. - - Also test that deleting a child not in the group_id_to_child_map behaves properly. - """ - # Set to first group configuration. - self._update_partition_id(0) - split_test = self._assert_children(2) - vertical_1_usage_key = split_test.children[1] - - # Add an extra child to the split_test - resp = self.create_xblock(category='html', parent_usage_key=self.split_test_usage_key) - extra_child_usage_key = self.response_usage_key(resp) - self._assert_children(3) - - # Remove the first child (which is part of the group configuration). - resp = self.client.ajax_post( - self.split_test_update_url, - data={'children': [unicode(vertical_1_usage_key), unicode(extra_child_usage_key)]} - ) - self.assertEqual(resp.status_code, 200) - split_test = self._assert_children(2) - - # Check that group_id_to_child was updated appropriately - group_id_to_child = split_test.group_id_to_child - self.assertEqual(1, len(group_id_to_child)) - self.assertEqual(vertical_1_usage_key, group_id_to_child['1']) - - # Remove the "extra" child and make sure that group_id_to_child did not change. - resp = self.client.ajax_post( - self.split_test_update_url, - data={'children': [unicode(vertical_1_usage_key)]} - ) - self.assertEqual(resp.status_code, 200) - split_test = self._assert_children(1) - self.assertEqual(group_id_to_child, split_test.group_id_to_child) - def test_add_groups(self): """ Test the "fix up behavior" when groups are missing (after a group is added to a group configuration). diff --git a/cms/envs/acceptance.py b/cms/envs/acceptance.py index d2593d16a3..5e596783cd 100644 --- a/cms/envs/acceptance.py +++ b/cms/envs/acceptance.py @@ -50,7 +50,8 @@ update_module_store_settings( module_store_options={ 'default_class': 'xmodule.raw_module.RawDescriptor', 'fs_root': TEST_ROOT / "data", - } + }, + default_store=os.environ.get('DEFAULT_STORE', 'draft'), ) CONTENTSTORE = { diff --git a/cms/envs/bok_choy.py b/cms/envs/bok_choy.py index ff58e1d511..a7c54076e9 100644 --- a/cms/envs/bok_choy.py +++ b/cms/envs/bok_choy.py @@ -36,6 +36,7 @@ update_module_store_settings( xml_store_options={ 'data_dir': (TEST_ROOT / "data").abspath(), }, + default_store=os.environ.get('DEFAULT_STORE', 'draft'), ) # Enable django-pipeline and staticfiles diff --git a/cms/envs/common.py b/cms/envs/common.py index 250a053da5..44c787b99c 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -106,7 +106,7 @@ FEATURES = { 'ADVANCED_SECURITY': False, # Modulestore to use for new courses - 'DEFAULT_STORE_FOR_NEW_COURSE': 'mongo', + 'DEFAULT_STORE_FOR_NEW_COURSE': None, } ENABLE_JASMINE = False diff --git a/cms/static/js/spec/utils/drag_and_drop_spec.js b/cms/static/js/spec/utils/drag_and_drop_spec.js index 93e4b23340..1eea1f920a 100644 --- a/cms/static/js/spec/utils/drag_and_drop_spec.js +++ b/cms/static/js/spec/utils/drag_and_drop_spec.js @@ -323,18 +323,15 @@ define(["js/utils/drag_and_drop", "js/views/feedback_notification", "js/spec_hel }, null, { clientX: $('#unit-1').offset().left }); - expect(requests.length).toEqual(2); + expect(requests.length).toEqual(1); expect(this.savingSpies.constructor).toHaveBeenCalled(); expect(this.savingSpies.show).toHaveBeenCalled(); expect(this.savingSpies.hide).not.toHaveBeenCalled(); savingOptions = this.savingSpies.constructor.mostRecentCall.args[0]; expect(savingOptions.title).toMatch(/Saving/); expect($('#unit-1')).toHaveClass('was-dropped'); - expect(requests[0].requestBody).toEqual('{"children":["second-unit-id","third-unit-id"]}'); + expect(requests[0].requestBody).toEqual('{"children":["fourth-unit-id","first-unit-id"]}'); requests[0].respond(200); - expect(this.savingSpies.hide).not.toHaveBeenCalled(); - expect(requests[1].requestBody).toEqual('{"children":["fourth-unit-id","first-unit-id"]}'); - requests[1].respond(200); expect(this.savingSpies.hide).toHaveBeenCalled(); this.clock.tick(1001); expect($('#unit-1')).not.toHaveClass('was-dropped'); diff --git a/cms/static/js/utils/drag_and_drop.js b/cms/static/js/utils/drag_and_drop.js index 8a7ef8d3aa..23c3404559 100644 --- a/cms/static/js/utils/drag_and_drop.js +++ b/cms/static/js/utils/drag_and_drop.js @@ -288,17 +288,6 @@ define(["jquery", "jquery.ui", "underscore", "gettext", "js/views/feedback_notif if (_.isFunction(refresh)) { refresh(collapsed); } }; - // If the parent has changed, update the children of the old parent. - if (newParentLocator !== oldParentLocator) { - // Find the old parent element. - oldParentEle = $(parentSelector).filter(function () { - return $(this).data('locator') === oldParentLocator; - }); - this.saveItem(oldParentEle, childrenSelector, function () { - element.data('parent', newParentLocator); - refreshParent(oldParentEle); - }); - } saving = new NotificationView.Mini({ title: gettext('Saving…') }); @@ -310,7 +299,18 @@ define(["jquery", "jquery.ui", "underscore", "gettext", "js/views/feedback_notif }, 1000); this.saveItem(newParentEle, childrenSelector, function () { saving.hide(); + + // Refresh new parent. refreshParent(newParentEle); + + // Refresh old parent. + if (newParentLocator !== oldParentLocator) { + oldParentEle = $(parentSelector).filter(function () { + return $(this).data('locator') === oldParentLocator; + }); + refreshParent(oldParentEle); + element.data('parent', newParentLocator); + } }); }, diff --git a/common/lib/xmodule/xmodule/modulestore/modulestore_settings.py b/common/lib/xmodule/xmodule/modulestore/modulestore_settings.py index 1726c78437..ee1a7fc106 100644 --- a/common/lib/xmodule/xmodule/modulestore/modulestore_settings.py +++ b/common/lib/xmodule/xmodule/modulestore/modulestore_settings.py @@ -34,6 +34,7 @@ def convert_module_store_setting_if_needed(module_store_setting): if module_store_setting is None: return None + # Convert to Mixed, if needed if module_store_setting['default']['ENGINE'] != 'xmodule.modulestore.mixed.MixedModuleStore': warnings.warn("Direct access to a modulestore is deprecated. Please use MixedModuleStore.", DeprecationWarning) @@ -54,7 +55,8 @@ def convert_module_store_setting_if_needed(module_store_setting): ) module_store_setting = new_module_store_setting - elif isinstance(module_store_setting['default']['OPTIONS']['stores'], dict): + # Convert from dict, if needed + elif isinstance(get_mixed_stores(module_store_setting), dict): warnings.warn( "Using a dict for the Stores option in the MixedModuleStore is deprecated. Please use a list instead.", DeprecationWarning @@ -62,13 +64,13 @@ def convert_module_store_setting_if_needed(module_store_setting): # convert old-style (unordered) dict to (an ordered) list module_store_setting['default']['OPTIONS']['stores'] = convert_old_stores_into_list( - module_store_setting['default']['OPTIONS']['stores'] + get_mixed_stores(module_store_setting) ) + assert isinstance(get_mixed_stores(module_store_setting), list) - assert isinstance(module_store_setting['default']['OPTIONS']['stores'], list) - + # Add Split, if needed # If Split is not defined but the DraftMongoModuleStore is configured, add Split as a copy of Draft - mixed_stores = module_store_setting['default']['OPTIONS']['stores'] + mixed_stores = get_mixed_stores(module_store_setting) is_split_defined = any((store['ENGINE'].endswith('.DraftVersioningModuleStore')) for store in mixed_stores) if not is_split_defined: # find first setting of mongo store @@ -95,10 +97,14 @@ def update_module_store_settings( doc_store_settings=None, module_store_options=None, xml_store_options=None, + default_store=None, ): """ Updates the settings for each store defined in the given module_store_setting settings with the given doc store configuration and options, overwriting existing keys. + + If default_store is specified, the given default store is moved to the top of the + list of stores. """ for store in module_store_setting['default']['OPTIONS']['stores']: if store['NAME'] == 'xml': @@ -106,3 +112,20 @@ def update_module_store_settings( else: module_store_options and store['OPTIONS'].update(module_store_options) doc_store_settings and store['DOC_STORE_CONFIG'].update(doc_store_settings) + + if default_store: + mixed_stores = get_mixed_stores(module_store_setting) + for store in mixed_stores: + if store['NAME'] == default_store: + # move the found store to the top of the list + mixed_stores.remove(store) + mixed_stores.insert(0, store) + return + raise Exception("Could not find setting for requested default store: {}".format(default_store)) + + +def get_mixed_stores(mixed_setting): + """ + Helper for accessing stores in a configuration setting for the Mixed modulestore. + """ + return mixed_setting["default"]["OPTIONS"]["stores"] diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 45814fa91f..e26f59fc91 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -2101,7 +2101,8 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): def _serialize_fields(self, category, fields): """ - Convert any references to their serialized form. + Convert any references to their serialized form. Handle some references already being unicoded + because the client passed them that way and nothing above this layer did the necessary deserialization. Remove any fields which split or its kvs computes or adds but does not want persisted. @@ -2111,17 +2112,26 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): xblock_class = XBlock.load_class(category, self.default_class) xblock_class = self.mixologist.mix(xblock_class) + def reference_block_id(reference): + """ + Handle client possibly setting field to strings rather than keys to get the block_id + """ + # perhaps replace by fixing the views or Field Reference*.from_json to return a Key + if isinstance(reference, basestring): + reference = BlockUsageLocator.from_string(reference) + return BlockKey.from_usage_key(reference) + for field_name, value in fields.iteritems(): if value is not None: if isinstance(xblock_class.fields[field_name], Reference): - fields[field_name] = BlockKey.from_usage_key(value) + fields[field_name] = reference_block_id(value) elif isinstance(xblock_class.fields[field_name], ReferenceList): fields[field_name] = [ - BlockKey.from_usage_key(ele) for ele in value + reference_block_id(ele) for ele in value ] elif isinstance(xblock_class.fields[field_name], ReferenceValueDict): for key, subvalue in value.iteritems(): - value[key] = BlockKey.from_usage_key(subvalue) + value[key] = reference_block_id(subvalue) # should this recurse down dicts and lists just in case they contain datetime? elif not isinstance(value, datetime.datetime): # don't convert datetimes! fields[field_name] = xblock_class.fields[field_name].to_json(value) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore_settings.py b/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore_settings.py index 166bf22327..f7a3335fc2 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore_settings.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore_settings.py @@ -2,10 +2,16 @@ Tests for testing the modulestore settings migration code. """ import copy +import ddt from unittest import TestCase -from xmodule.modulestore.modulestore_settings import convert_module_store_setting_if_needed +from xmodule.modulestore.modulestore_settings import ( + convert_module_store_setting_if_needed, + update_module_store_settings, + get_mixed_stores, +) +@ddt.ddt class ModuleStoreSettingsMigration(TestCase): """ Tests for the migration code for the module store settings @@ -108,12 +114,6 @@ class ModuleStoreSettingsMigration(TestCase): } - def _get_mixed_stores(self, mixed_setting): - """ - Helper for accessing stores in a configuration setting for the Mixed modulestore. - """ - return mixed_setting["default"]["OPTIONS"]["stores"] - def assertStoreValuesEqual(self, store_setting1, store_setting2): """ Tests whether the fields in the given store_settings are equal. @@ -134,7 +134,7 @@ class ModuleStoreSettingsMigration(TestCase): self.assertEqual(new_mixed_setting["default"]["ENGINE"], "xmodule.modulestore.mixed.MixedModuleStore") # check whether the stores are in an ordered list - new_stores = self._get_mixed_stores(new_mixed_setting) + new_stores = get_mixed_stores(new_mixed_setting) self.assertIsInstance(new_stores, list) return new_mixed_setting, new_stores[0] @@ -143,7 +143,7 @@ class ModuleStoreSettingsMigration(TestCase): """ Tests whether the split module store is configured in the given setting. """ - stores = self._get_mixed_stores(mixed_setting) + stores = get_mixed_stores(mixed_setting) split_settings = [store for store in stores if store['ENGINE'].endswith('.DraftVersioningModuleStore')] if len(split_settings): # there should only be one setting for split @@ -178,8 +178,8 @@ class ModuleStoreSettingsMigration(TestCase): self.assertTrue(self.is_split_configured(new_mixed_setting)) # exclude split when comparing old and new, since split was added as part of the migration - new_stores = [store for store in self._get_mixed_stores(new_mixed_setting) if store['NAME'] != 'split'] - old_stores = self._get_mixed_stores(self.OLD_MIXED_CONFIG_WITH_DICT) + new_stores = [store for store in get_mixed_stores(new_mixed_setting) if store['NAME'] != 'split'] + old_stores = get_mixed_stores(self.OLD_MIXED_CONFIG_WITH_DICT) # compare each store configured in mixed self.assertEqual(len(new_stores), len(old_stores)) @@ -192,3 +192,14 @@ class ModuleStoreSettingsMigration(TestCase): new_mixed_setting, new_default_store_setting = self.assertMigrated(old_mixed_setting) self.assertTrue(self.is_split_configured(new_mixed_setting)) self.assertEquals(old_mixed_setting, new_mixed_setting) + + @ddt.data('draft', 'split') + def test_update_settings(self, default_store): + mixed_setting = self.ALREADY_UPDATED_MIXED_CONFIG + update_module_store_settings(mixed_setting, default_store=default_store) + self.assertTrue(get_mixed_stores(mixed_setting)[0]['NAME'] == default_store) + + def test_update_settings_error(self): + mixed_setting = self.ALREADY_UPDATED_MIXED_CONFIG + with self.assertRaises(Exception): + update_module_store_settings(mixed_setting, default_store='non-existent store') diff --git a/common/test/acceptance/fixtures/course.py b/common/test/acceptance/fixtures/course.py index b28c4474f1..69836fbee0 100644 --- a/common/test/acceptance/fixtures/course.py +++ b/common/test/acceptance/fixtures/course.py @@ -11,6 +11,7 @@ from textwrap import dedent from collections import namedtuple from path import path from lazy import lazy +from opaque_keys.edx.keys import CourseKey from . import STUDIO_BASE_URL @@ -204,6 +205,7 @@ class CourseFixture(StudioApiFixture): self.children = [] self._assets = [] self._advanced_settings = {} + self._course_key = None def __str__(self): """ @@ -263,19 +265,17 @@ class CourseFixture(StudioApiFixture): return self - @property - def _course_key(self): - """ - Return the locator string for the course. - """ - return "{org}/{number}/{run}".format(**self._course_dict) - @property def _course_location(self): """ Return the locator string for the course. """ - return "i4x://{org}/{number}/course/{run}".format(**self._course_dict) + course_key = CourseKey.from_string(self._course_key) + if getattr(course_key, 'deprecated', False): + block_id = self._course_dict['run'] + else: + block_id = 'course' + return unicode(course_key.make_usage_key('course', block_id)) @property def _assets_url(self): @@ -289,7 +289,8 @@ class CourseFixture(StudioApiFixture): """ Return the locator string for the course handouts """ - return "i4x://{org}/{number}/course_info/handouts".format(**self._course_dict) + course_key = CourseKey.from_string(self._course_key) + return unicode(course_key.make_usage_key('course_info', 'handouts')) def _create_course(self): """ @@ -315,7 +316,9 @@ class CourseFixture(StudioApiFixture): if err is not None: raise CourseFixtureError("Could not create course {0}. Error message: '{1}'".format(self, err)) - if not response.ok: + if response.ok: + self._course_key = response.json()['course_key'] + else: raise CourseFixtureError( "Could not create course {0}. Status was {1}".format( self._course_dict, response.status_code)) diff --git a/common/test/acceptance/tests/helpers.py b/common/test/acceptance/tests/helpers.py index c6f74700f1..2fe931be96 100644 --- a/common/test/acceptance/tests/helpers.py +++ b/common/test/acceptance/tests/helpers.py @@ -5,8 +5,10 @@ import json import unittest import functools import requests +import os from path import path from bok_choy.web_app_test import WebAppTest +from opaque_keys.edx.locator import CourseLocator def skip_if_browser(browser): @@ -170,8 +172,6 @@ class UniqueCourseTest(WebAppTest): Test that provides a unique course ID. """ - COURSE_ID_SEPARATOR = "/" - def __init__(self, *args, **kwargs): """ Create a unique course ID. @@ -190,11 +190,18 @@ class UniqueCourseTest(WebAppTest): @property def course_id(self): - return self.COURSE_ID_SEPARATOR.join([ + """ + Returns the serialized course_key for the test + """ + # TODO - is there a better way to make this agnostic to the underlying default module store? + default_store = os.environ.get('DEFAULT_STORE', 'draft') + course_key = CourseLocator( self.course_info['org'], self.course_info['number'], - self.course_info['run'] - ]) + self.course_info['run'], + deprecated=(default_store == 'draft') + ) + return unicode(course_key) class YouTubeConfigError(Exception): diff --git a/docs/en_us/internal/testing.md b/docs/en_us/internal/testing.md index 669eee24a0..3e2c56bbc1 100644 --- a/docs/en_us/internal/testing.md +++ b/docs/en_us/internal/testing.md @@ -274,6 +274,14 @@ To put a debugging breakpoint in a test use: from nose.tools import set_trace; set_trace() +By default, all bokchoy tests are run with the 'split' ModuleStore. +To override the modulestore that is used, use the default_store option. The currently supported stores are: + 'split' (xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore) and + 'draft' (xmodule.modulestore.mongo.DraftMongoModuleStore). +For example: + + paver test_bokchoy --default_store='draft' + ### Running Lettuce Acceptance Tests @@ -309,6 +317,14 @@ To start the debugger on failure, add the `--pdb` option to extra_args: To run tests faster by not collecting static files, you can use `paver test_acceptance -s lms --fasttest` and `paver test_acceptance -s cms --fasttest`. +By default, all acceptance tests are run with the 'draft' ModuleStore. +To override the modulestore that is used, use the default_store option. Currently, the possible stores for acceptance tests are: + 'split' (xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore) and + 'draft' (xmodule.modulestore.mongo.DraftMongoModuleStore). +For example: + paver test_acceptance --default_store='draft' +Note, however, all acceptance tests currently do not pass with 'split'. + Acceptance tests will run on a randomized port and can be run in the background of paver cms and lms or unit tests. To specify the port, change the LETTUCE_SERVER_PORT constant in cms/envs/acceptance.py and lms/envs/acceptance.py as well as the port listed in cms/djangoapps/contentstore/feature/upload.py diff --git a/lms/envs/acceptance.py b/lms/envs/acceptance.py index 8b8959c9a6..e4361ebacc 100644 --- a/lms/envs/acceptance.py +++ b/lms/envs/acceptance.py @@ -50,7 +50,8 @@ update_module_store_settings( }, module_store_options={ 'fs_root': TEST_ROOT / "data", - } + }, + default_store=os.environ.get('DEFAULT_STORE', 'draft'), ) CONTENTSTORE = { 'ENGINE': 'xmodule.contentstore.mongo.MongoContentStore', diff --git a/lms/envs/bok_choy.py b/lms/envs/bok_choy.py index 7c47b9fbea..fd797b9bc5 100644 --- a/lms/envs/bok_choy.py +++ b/lms/envs/bok_choy.py @@ -39,6 +39,7 @@ update_module_store_settings( xml_store_options={ 'data_dir': (TEST_ROOT / "data").abspath(), }, + default_store=os.environ.get('DEFAULT_STORE', 'draft'), ) # Configure the LMS to use our stub XQueue implementation diff --git a/lms/templates/courseware/course_about.html b/lms/templates/courseware/course_about.html index a259f21289..1b5ee9a32d 100644 --- a/lms/templates/courseware/course_about.html +++ b/lms/templates/courseware/course_about.html @@ -102,7 +102,7 @@ location.href = xhr.responseText; } } else if (xhr.status == 403) { - location.href = "${reverse('register_user')}?course_id=${course.id.to_deprecated_string()}&enrollment_action=enroll"; + location.href = "${reverse('register_user')}?course_id=${course.id | u}&enrollment_action=enroll"; } else { $('#register_error').html( (xhr.responseText ? xhr.responseText : 'An error occurred. Please try again later.') diff --git a/pavelib/acceptance_test.py b/pavelib/acceptance_test.py index b1a69ae552..1d9ddf2e30 100644 --- a/pavelib/acceptance_test.py +++ b/pavelib/acceptance_test.py @@ -19,6 +19,7 @@ __test__ = False # do not collect ) @cmdopts([ ("system=", "s", "System to act on"), + ("default_store=", "m", "Default modulestore to use for course creation"), ("fasttest", "a", "Run without collectstatic"), ("extra_args=", "e", "adds as extra args to the test command"), make_option("--verbose", action="store_const", const=2, dest="verbosity"), @@ -32,6 +33,7 @@ def test_acceptance(options): opts = { 'fasttest': getattr(options, 'fasttest', False), 'system': getattr(options, 'system', None), + 'default_store': getattr(options, 'default_store', None), 'verbosity': getattr(options, 'verbosity', 3), 'extra_args': getattr(options, 'extra_args', ''), } @@ -42,6 +44,12 @@ def test_acceptance(options): 'No system specified, running tests for both cms and lms.' ) print(msg) + if opts['default_store'] not in ['draft', 'split']: + msg = colorize( + 'red', + 'No modulestore specified, running tests for both draft and split.' + ) + print(msg) suite = AcceptanceTestSuite('{} acceptance'.format(opts['system']), **opts) suite.run() diff --git a/pavelib/bok_choy.py b/pavelib/bok_choy.py index 5ea785290d..c510a9f54d 100644 --- a/pavelib/bok_choy.py +++ b/pavelib/bok_choy.py @@ -22,6 +22,7 @@ __test__ = False # do not collect ('test_spec=', 't', 'Specific test to run'), ('fasttest', 'a', 'Skip some setup'), ('extra_args=', 'e', 'adds as extra args to the test command'), + ('default_store=', 's', 'Default modulestore'), make_option("--verbose", action="store_const", const=2, dest="verbosity"), make_option("-q", "--quiet", action="store_const", const=0, dest="verbosity"), make_option("-v", "--verbosity", action="count", dest="verbosity"), @@ -45,13 +46,12 @@ def test_bokchoy(options): opts = { 'test_spec': getattr(options, 'test_spec', None), 'fasttest': getattr(options, 'fasttest', False), + 'default_store': getattr(options, 'default_store', None), 'verbosity': getattr(options, 'verbosity', 2), 'extra_args': getattr(options, 'extra_args', ''), 'test_dir': 'tests', } - - test_suite = BokChoyTestSuite('bok-choy', **opts) - test_suite.run() + run_bokchoy(**opts) @task @@ -60,6 +60,7 @@ def test_bokchoy(options): ('test_spec=', 't', 'Specific test to run'), ('fasttest', 'a', 'Skip some setup'), ('imports_dir=', 'd', 'Directory containing (un-archived) courses to be imported'), + ('default_store=', 's', 'Default modulestore'), make_option("--verbose", action="store_const", const=2, dest="verbosity"), make_option("-q", "--quiet", action="store_const", const=0, dest="verbosity"), make_option("-v", "--verbosity", action="count", dest="verbosity"), @@ -72,13 +73,33 @@ def perf_report_bokchoy(options): 'test_spec': getattr(options, 'test_spec', None), 'fasttest': getattr(options, 'fasttest', False), 'imports_dir': getattr(options, 'imports_dir', None), + 'default_store': getattr(options, 'default_store', None), 'verbosity': getattr(options, 'verbosity', 2), 'test_dir': 'performance', 'ptests': True, } + run_bokchoy(**opts) - test_suite = BokChoyTestSuite('bok-choy', **opts) - test_suite.run() + +def run_bokchoy(**opts): + """ + Runs BokChoyTestSuite with the given options. + If a default store is not specified, runs the test suite for 'split' as the default store. + """ + if opts['default_store'] not in ['draft', 'split']: + msg = colorize( + 'red', + 'No modulestore specified, running tests for split.' + ) + print(msg) + stores = ['split'] + else: + stores = [opts['default_store']] + + for store in stores: + opts['default_store'] = store + test_suite = BokChoyTestSuite('bok-choy', **opts) + test_suite.run() @task diff --git a/pavelib/utils/test/bokchoy_utils.py b/pavelib/utils/test/bokchoy_utils.py index c71c6164aa..686bc515fc 100644 --- a/pavelib/utils/test/bokchoy_utils.py +++ b/pavelib/utils/test/bokchoy_utils.py @@ -18,7 +18,7 @@ except ImportError: __test__ = False # do not collect -def start_servers(): +def start_servers(default_store): """ Start the servers we will run tests on, returns PIDs for servers. """ @@ -33,9 +33,11 @@ def start_servers(): for service, info in Env.BOK_CHOY_SERVERS.iteritems(): address = "0.0.0.0:{}".format(info['port']) cmd = ( + "DEFAULT_STORE={default_store} " "coverage run --rcfile={coveragerc} -m " "manage {service} --settings bok_choy runserver " "{address} --traceback --noreload".format( + default_store=default_store, coveragerc=Env.BOK_CHOY_COVERAGERC, service=service, address=address, diff --git a/pavelib/utils/test/suites/acceptance_suite.py b/pavelib/utils/test/suites/acceptance_suite.py index d59552e75f..36bb0c083b 100644 --- a/pavelib/utils/test/suites/acceptance_suite.py +++ b/pavelib/utils/test/suites/acceptance_suite.py @@ -17,7 +17,8 @@ class AcceptanceTest(TestSuite): super(AcceptanceTest, self).__init__(*args, **kwargs) self.report_dir = Env.REPORT_DIR / 'acceptance' self.fasttest = kwargs.get('fasttest', False) - self.system = kwargs.get('system', None) + self.system = kwargs.get('system') + self.default_store = kwargs.get('default_store') self.extra_args = kwargs.get('extra_args', '') def __enter__(self): @@ -35,9 +36,10 @@ class AcceptanceTest(TestSuite): report_file = self.report_dir / "{}.xml".format(self.system) report_args = "--with-xunit --xunit-file {}".format(report_file) - cmd = ( - "./manage.py {system} --settings acceptance harvest --traceback " + cmd = ( + "DEFAULT_STORE={default_store} ./manage.py {system} --settings acceptance harvest --traceback " "--debug-mode --verbosity {verbosity} {report_args} {extra_args}".format( + default_store=self.default_store, system=self.system, verbosity=self.verbosity, report_args=report_args, @@ -65,24 +67,31 @@ class AcceptanceTestSuite(TestSuite): self.root = 'acceptance' self.db = Env.REPO_ROOT / 'test_root/db/test_edx.db' self.db_cache = Env.REPO_ROOT / 'common/test/db_cache/lettuce.db' - self.system = kwargs.get('system', None) self.fasttest = kwargs.get('fasttest', False) - if self.system: - self.subsuites = [ - AcceptanceTest('{} acceptance'.format(self.system), **kwargs), - ] + if kwargs.get('system'): + systems = [kwargs['system']] else: - kwargs['system'] = 'lms' - lms = AcceptanceTest('lms acceptance', **kwargs) - kwargs['system'] = 'cms' - cms = AcceptanceTest('cms acceptance', **kwargs) - self.subsuites = [lms, cms] + systems = ['lms', 'cms'] + + if kwargs.get('default_store'): + stores = [kwargs['default_store']] + else: + # TODO fix Acceptance tests with Split (LMS-11300) + # stores = ['split', 'draft'] + stores = ['draft'] + + self.subsuites = [] + for system in systems: + for default_store in stores: + kwargs['system'] = system + kwargs['default_store'] = default_store + self.subsuites.append(AcceptanceTest('{} acceptance using {}'.format(system, default_store), **kwargs)) def __enter__(self): super(AcceptanceTestSuite, self).__enter__() - test_utils.clean_test_files() - + test_utils.clean_test_files() + if not self.fasttest: self._setup_acceptance_db() @@ -104,7 +113,7 @@ class AcceptanceTestSuite(TestSuite): if self.db.isfile(): # Since we are using SQLLite, we can reset the database by deleting it on disk. self.db.remove() - + if self.db_cache.isfile(): # To speed up migrations, we check for a cached database file and start from that. # The cached database file should be checked into the repo diff --git a/pavelib/utils/test/suites/bokchoy_suite.py b/pavelib/utils/test/suites/bokchoy_suite.py index da98d021e6..3301d326d8 100644 --- a/pavelib/utils/test/suites/bokchoy_suite.py +++ b/pavelib/utils/test/suites/bokchoy_suite.py @@ -28,6 +28,7 @@ class BokChoyTestSuite(TestSuite): self.cache = Env.BOK_CHOY_CACHE self.fasttest = kwargs.get('fasttest', False) self.test_spec = kwargs.get('test_spec', None) + self.default_store = kwargs.get('default_store') self.verbosity = kwargs.get('verbosity', 2) self.extra_args = kwargs.get('extra_args', '') self.ptests = kwargs.get('ptests', False) @@ -64,17 +65,26 @@ class BokChoyTestSuite(TestSuite): self.cache.flush_all() sh( - "./manage.py lms --settings bok_choy loaddata --traceback" - " common/test/db_fixtures/*.json" + "DEFAULT_STORE={default_store}" + " ./manage.py lms --settings bok_choy loaddata --traceback" + " common/test/db_fixtures/*.json".format( + default_store=self.default_store, + ) ) if self.imports_dir: - sh("./manage.py cms --settings=bok_choy import {}".format(self.imports_dir)) + sh( + "DEFAULT_STORE={default_store}" + " ./manage.py cms --settings=bok_choy import {import_dir}".format( + default_store=self.default_store, + import_dir=self.imports_dir + ) + ) # Ensure the test servers are available msg = colorize('green', "Starting test servers...") print(msg) - bokchoy_utils.start_servers() + bokchoy_utils.start_servers(self.default_store) msg = colorize('green', "Waiting for servers to start...") print(msg) @@ -101,6 +111,7 @@ class BokChoyTestSuite(TestSuite): # Construct the nosetests command, specifying where to save # screenshots and XUnit XML reports cmd = [ + "DEFAULT_STORE={}".format(self.default_store), "SCREENSHOT_DIR='{}'".format(self.log_dir), "HAR_DIR='{}'".format(self.har_dir), "SELENIUM_DRIVER_LOG_DIR='{}'".format(self.log_dir),