diff --git a/cms/djangoapps/contentstore/features/video.py b/cms/djangoapps/contentstore/features/video.py index 16ca144afe..fd7a352162 100644 --- a/cms/djangoapps/contentstore/features/video.py +++ b/cms/djangoapps/contentstore/features/video.py @@ -154,7 +154,8 @@ def xml_only_video(step): world.ItemFactory.create( parent_location=parent_location, category='video', - data='' % youtube_id + data='' % youtube_id, + modulestore=store, ) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 3d95fd1c8e..2eb8967c1b 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -741,22 +741,21 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): def test_illegal_draft_crud_ops(self): draft_store = modulestore('draft') - direct_store = modulestore('direct') course = CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course') location = course.id.make_usage_key('chapter', 'neuvo') - # Ensure draft mongo store does not allow us to create chapters either directly or via convert to draft - with self.assertRaises(InvalidVersionError): - draft_store.create_and_save_xmodule(location) - direct_store.create_and_save_xmodule(location) + # Ensure draft mongo store does not create drafts for things that shouldn't be draft + newobject = draft_store.create_and_save_xmodule(location) + self.assertFalse(getattr(newobject, 'is_draft', False)) with self.assertRaises(InvalidVersionError): draft_store.convert_to_draft(location) chapter = draft_store.get_item(location) chapter.data = 'chapter data' - with self.assertRaises(InvalidVersionError): - draft_store.update_item(chapter, self.user.id) + draft_store.update_item(chapter, self.user.id) + newobject = draft_store.get_item(chapter.location) + self.assertFalse(getattr(newobject, 'is_draft', False)) with self.assertRaises(InvalidVersionError): draft_store.unpublish(location) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index ed9bb06b76..9ceb68d8d5 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -14,7 +14,6 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.locations import SlashSeparatedCourseKey, Location from xmodule.modulestore.store_utilities import delete_course -from xmodule.modulestore.draft import DIRECT_ONLY_CATEGORIES from student.roles import CourseInstructorRole, CourseStaffRole @@ -52,15 +51,10 @@ def delete_course_and_groups(course_id, commit=False): def get_modulestore(category_or_location): """ - Returns the correct modulestore to use for modifying the specified location + This function no longer does anything more than just calling `modulestore()`. It used + to select 'direct' v 'draft' based on the category. """ - if isinstance(category_or_location, Location): - category_or_location = category_or_location.category - - if category_or_location in DIRECT_ONLY_CATEGORIES: - return modulestore('direct') - else: - return modulestore() + return modulestore() def get_lms_link_for_item(location, preview=False): diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 1e13a96a59..ef582c1373 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -2,7 +2,6 @@ from __future__ import absolute_import import json import logging -from collections import defaultdict from django.http import HttpResponseBadRequest, Http404 from django.contrib.auth.decorators import login_required diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index ddf605eb96..4f7667db72 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -53,7 +53,6 @@ from django_comment_common.utils import seed_permissions_roles from student.models import CourseEnrollment from student.roles import CourseRole, UserBasedRole -from xmodule.html_module import AboutDescriptor from xmodule.modulestore.keys import CourseKey from course_creators.views import get_course_creator_status, add_user_with_status_unrequested from contentstore import utils diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 6eba2559db..d5d40fd277 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -381,8 +381,7 @@ def _save_item(request, usage_key, data=None, children=None, metadata=None, null # interface for publishing. However, as of now, only the DraftMongoModulestore # does, so we have to check for the attribute explicitly. store = get_modulestore(block.location) - if hasattr(store, 'publish'): - store.publish(block.location, request.user.id) + store.publish(block.location, request.user.id) _xmodule_recurse( existing_item, diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index bfdbd72455..8c2888669c 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -75,25 +75,27 @@ class DraftModuleStore(MongoModuleStore): in the request. The depth is counted in the number of calls to get_children() to cache. None indicates to cache all descendents """ - - try: - return wrap_draft(super(DraftModuleStore, self).get_item(as_draft(usage_key), depth=depth)) - except ItemNotFoundError: - return wrap_draft(super(DraftModuleStore, self).get_item(usage_key, depth=depth)) + if usage_key.category not in DIRECT_ONLY_CATEGORIES: + try: + return wrap_draft(super(DraftModuleStore, self).get_item(as_draft(usage_key), depth=depth)) + except ItemNotFoundError: + return wrap_draft(super(DraftModuleStore, self).get_item(usage_key, depth=depth)) + else: + return super(DraftModuleStore, self).get_item(usage_key, depth=depth) def create_xmodule(self, location, definition_data=None, metadata=None, system=None, fields={}): """ - Create the new xmodule but don't save it. Returns the new module with a draft locator + Create the new xmodule but don't save it. Returns the new module with a draft locator if + the category allows drafts. If the category does not allow drafts, just creates a published module. :param location: a Location--must have a category :param definition_data: can be empty. The initial definition_data for the kvs :param metadata: can be empty, the initial metadata for the kvs :param system: if you already have an xmodule from the course, the xmodule.system value """ - if location.category in DIRECT_ONLY_CATEGORIES: - raise InvalidVersionError(location) - draft_loc = as_draft(location) - return super(DraftModuleStore, self).create_xmodule(draft_loc, definition_data, metadata, system, fields) + if location.category not in DIRECT_ONLY_CATEGORIES: + location = as_draft(location) + return super(DraftModuleStore, self).create_xmodule(location, definition_data, metadata, system, fields) def get_items(self, course_key, settings=None, content=None, revision=None, **kwargs): """ @@ -139,12 +141,12 @@ class DraftModuleStore(MongoModuleStore): :param source: the location of the source (its revision must be None) """ - original = self.collection.find_one({'_id': source_location.to_deprecated_son()}) - draft_location = as_draft(source_location) - if draft_location.category in DIRECT_ONLY_CATEGORIES: + if source_location.category in DIRECT_ONLY_CATEGORIES: raise InvalidVersionError(source_location) + original = self.collection.find_one({'_id': source_location.to_deprecated_son()}) if not original: raise ItemNotFoundError(source_location) + draft_location = as_draft(source_location) original['_id'] = draft_location.to_deprecated_son() try: self.collection.insert(original) @@ -161,6 +163,9 @@ class DraftModuleStore(MongoModuleStore): In addition to the superclass's behavior, this method converts the unit to draft if it's not already draft. """ + if xblock.location.category in DIRECT_ONLY_CATEGORIES: + return super(DraftModuleStore, self).update_item(xblock, user_id, allow_not_found) + draft_loc = as_draft(xblock.location) try: if not self.has_item(draft_loc): @@ -180,6 +185,9 @@ class DraftModuleStore(MongoModuleStore): location: Something that can be passed to Location """ + if location.category in DIRECT_ONLY_CATEGORIES: + return super(DraftModuleStore, self).delete_item(as_published(location)) + super(DraftModuleStore, self).delete_item(as_draft(location)) if delete_all_versions: super(DraftModuleStore, self).delete_item(as_published(location)) @@ -190,6 +198,10 @@ class DraftModuleStore(MongoModuleStore): """ Save a current draft to the underlying modulestore """ + if location.category in DIRECT_ONLY_CATEGORIES: + # ignore noop attempt to publish something that can't be draft. + # ignoring v raising exception b/c bok choy tests always pass make_public which calls publish + return try: original_published = super(DraftModuleStore, self).get_item(location) except ItemNotFoundError: