diff --git a/xmodule/assets/library_content/public/js/library_content_edit_helpers.js b/xmodule/assets/library_content/public/js/library_content_edit_helpers.js deleted file mode 100644 index 564cc5fb0f..0000000000 --- a/xmodule/assets/library_content/public/js/library_content_edit_helpers.js +++ /dev/null @@ -1,54 +0,0 @@ -/* JavaScript for special editing operations that can be done on LibraryContentXBlock */ -// This is a temporary UI improvements that will be removed when V2 content libraries became -// fully functional - -/** - * Toggle the "Problem Type" settings section depending on selected library type. - * As for now, the V2 libraries don't support different problem types, so they can't be - * filtered by it. We're hiding the Problem Type field for them. - */ -function checkProblemTypeShouldBeVisible(editor) { - var libraries = editor.find('.wrapper-comp-settings.metadata_edit.is-active') - .data().metadata.source_library_id.options; - var selectedIndex = $("select[name='Library']", editor)[0].selectedIndex; - var libraryKey = libraries[selectedIndex].value; - var url = URI('/xblock') - .segment(editor.find('.xblock.xblock-studio_view.xblock-studio_view-library_content.xblock-initialized') - .data('usage-id')) - .segment('handler') - .segment('is_v2_library'); - - $.ajax({ - type: 'POST', - url: url, - data: JSON.stringify({'library_key': libraryKey}), - success: function(data) { - var problemTypeSelect = editor.find("select[name='Problem Type']") - .parents("li.field.comp-setting-entry.metadata_entry"); - data.is_v2 ? problemTypeSelect.hide() : problemTypeSelect.show(); - } - }); -} - -/** - * Waits untill editor html loaded, than calls checks for Program Type field toggling. - */ -function waitForEditorLoading() { - var checkContent = setInterval(function() { - var $modal = $('.xblock-editor'); - var content = $modal.html(); - if (content) { - clearInterval(checkContent); - checkProblemTypeShouldBeVisible($modal); - } - }, 10); -} -// Initial call -waitForEditorLoading(); - -var $librarySelect = $("select[name='Library']"); -$(document).on('change', $librarySelect, waitForEditorLoading) - -var $libraryContentEditors = $('.xblock-header.xblock-header-library_content'); -var $editBtns = $libraryContentEditors.find('.action-item.action-edit'); -$(document).on('click', $editBtns, waitForEditorLoading) diff --git a/xmodule/library_content_module.py b/xmodule/library_content_module.py index 66858d4d1f..0d9d4e080f 100644 --- a/xmodule/library_content_module.py +++ b/xmodule/library_content_module.py @@ -8,6 +8,7 @@ import logging import random from copy import copy from gettext import ngettext +from rest_framework import status import bleach from django.conf import settings @@ -15,10 +16,8 @@ from django.utils.functional import classproperty from lazy import lazy from lxml import etree from lxml.etree import XMLSyntaxError -from opaque_keys import InvalidKeyError -from opaque_keys.edx.locator import LibraryLocator, LibraryLocatorV2 +from opaque_keys.edx.locator import LibraryLocator from pkg_resources import resource_string -from rest_framework import status from web_fragments.fragment import Fragment from webob import Response from xblock.completable import XBlockCompletionMode @@ -30,15 +29,16 @@ from xmodule.mako_module import MakoTemplateBlockBase from xmodule.studio_editable import StudioEditableBlock from xmodule.util.xmodule_django import add_webpack_to_fragment from xmodule.validation import StudioValidation, StudioValidationMessage +from xmodule.xml_module import XmlMixin from xmodule.x_module import ( - STUDENT_VIEW, HTMLSnippet, ResourceTemplates, + shim_xmodule_js, + STUDENT_VIEW, XModuleMixin, XModuleToXBlockMixin, - shim_xmodule_js, ) -from xmodule.xml_module import XmlMixin + # Make '_' a no-op so we can scrape strings. Using lambda instead of # `django.utils.translation.ugettext_noop` because Django cannot be imported in this file @@ -189,14 +189,9 @@ class LibraryContentBlock( @property def source_library_key(self): """ - Convenience method to get the library ID as a LibraryLocator and not just a string. - - Supports either library v1 or library v2 locators. + Convenience method to get the library ID as a LibraryLocator and not just a string """ - try: - return LibraryLocator.from_string(self.source_library_id) - except InvalidKeyError: - return LibraryLocatorV2.from_string(self.source_library_id) + return LibraryLocator.from_string(self.source_library_id) @classmethod def make_selection(cls, selected, children, max_count, mode): @@ -461,7 +456,6 @@ class LibraryContentBlock( fragment = Fragment( self.runtime.service(self, 'mako').render_template(self.mako_template, self.get_context()) ) - fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/library_content_edit_helpers.js')) add_webpack_to_fragment(fragment, 'LibraryContentBlockStudio') shim_xmodule_js(fragment, self.studio_js_module_name) return fragment @@ -523,22 +517,6 @@ class LibraryContentBlock( self.tools.update_children(self, user_perms) return Response() - @XBlock.json_handler - def is_v2_library(self, data, suffix=''): # lint-amnesty, pylint: disable=unused-argument - """ - Check the library version by library_id. - - This is a temporary handler needed for hiding the Problem Type xblock editor field for V2 libraries. - """ - lib_key = data.get('library_key') - try: - LibraryLocatorV2.from_string(lib_key) - except InvalidKeyError: - is_v2 = False - else: - is_v2 = True - return {'is_v2': is_v2} - # Copy over any overridden settings the course author may have applied to the blocks. def _copy_overrides(self, store, user_id, source, dest): """ diff --git a/xmodule/library_tools.py b/xmodule/library_tools.py index e4730813d2..fd99f41dbd 100644 --- a/xmodule/library_tools.py +++ b/xmodule/library_tools.py @@ -5,28 +5,20 @@ import hashlib from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core.exceptions import PermissionDenied -from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import UsageKey -from opaque_keys.edx.locator import ( - BlockUsageLocator, - LibraryLocator, - LibraryLocatorV2, - LibraryUsageLocator, - LibraryUsageLocatorV2, -) +from opaque_keys.edx.locator import LibraryLocator, LibraryUsageLocator, LibraryUsageLocatorV2, BlockUsageLocator from search.search_engine_base import SearchEngine from xblock.fields import Scope + +from openedx.core.djangoapps.content_libraries import api as library_api +from openedx.core.djangoapps.xblock.api import load_block +from openedx.core.lib import blockstore_api +from common.djangoapps.student.auth import has_studio_write_access from xmodule.capa_module import ProblemBlock from xmodule.library_content_module import ANY_CAPA_TYPE_VALUE from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ItemNotFoundError -from common.djangoapps.student.auth import has_studio_write_access -from openedx.core.djangoapps.content_libraries import api as library_api -from openedx.core.djangoapps.content_libraries.models import ContentLibrary -from openedx.core.djangoapps.xblock.api import load_block -from openedx.core.lib import blockstore_api - def normalize_key_for_search(library_key): """ Normalizes library key for use with search indexing """ @@ -35,63 +27,39 @@ def normalize_key_for_search(library_key): class LibraryToolsService: """ - Service for LibraryContentBlock and LibrarySourcedBlock. - - Allows to interact with libraries in the modulestore and blockstore. + Service that allows LibraryContentBlock to interact with libraries in the + modulestore. """ def __init__(self, modulestore, user_id): self.store = modulestore self.user_id = user_id - def _get_library(self, library_key, is_v2_lib): + def _get_library(self, library_key): """ - Helper method to get either V1 or V2 library. + Given a library key like "library-v1:ProblemX+PR0B", return the + 'library' XBlock with meta-information about the library. - Given a library key like "library-v1:ProblemX+PR0B" (V1) or "lib:RG:rg-1" (v2), return the 'library'. - - is_v2_lib (bool) indicates which library storage should be requested: - True - blockstore (V2 library); - False - modulestore (V1 library). + A specific version may be specified. Returns None on error. """ - if is_v2_lib: - try: - return library_api.get_library(library_key) - except ContentLibrary.DoesNotExist: - return None - else: - try: - return self.store.get_library( - library_key, remove_version=False, remove_branch=False, head_validation=False - ) - except ItemNotFoundError: - return None + if not isinstance(library_key, LibraryLocator): + library_key = LibraryLocator.from_string(library_key) + + try: + return self.store.get_library( + library_key, remove_version=False, remove_branch=False, head_validation=False + ) + except ItemNotFoundError: + return None def get_library_version(self, lib_key): """ - Get the version of the given library. - - The return value (library version) could be: - ObjectID - for V1 library; - int - for V2 library. - None - if the library does not exist. + Get the version (an ObjectID) of the given library. + Returns None if the library does not exist. """ - if not isinstance(lib_key, (LibraryLocator, LibraryLocatorV2)): - try: - lib_key = LibraryLocator.from_string(lib_key) - is_v2_lib = False - except InvalidKeyError: - lib_key = LibraryLocatorV2.from_string(lib_key) - is_v2_lib = True - else: - is_v2_lib = isinstance(lib_key, LibraryLocatorV2) - - library = self._get_library(lib_key, is_v2_lib) - + library = self._get_library(lib_key) if library: - if is_v2_lib: - return library.version # We need to know the library's version so ensure it's set in library.location.library_key.version_guid assert library.location.library_key.version_guid is not None return library.location.library_key.version_guid @@ -169,13 +137,11 @@ class LibraryToolsService: def update_children(self, dest_block, user_perms=None, version=None): """ - Update xBlock's children. - - Re-fetch all matching blocks from the libraries, and copy them as children of dest_block. - The children will be given new block_ids. - - NOTE: V1 libraies blocks definition ID should be the - exact same definition ID used in the copy block. + This method is to be used when the library that a LibraryContentBlock + references has been updated. It will re-fetch all matching blocks from + the libraries, and copy them as children of dest_block. The children + will be given new block_ids, but the definition ID used should be the + exact same definition ID used in the library. This method will update dest_block's 'source_library_version' field to store the version number of the libraries used, so we easily determine @@ -189,69 +155,49 @@ class LibraryToolsService: return source_blocks = [] - library_key = dest_block.source_library_key - is_v2_lib = isinstance(library_key, LibraryLocatorV2) - - if version and not is_v2_lib: + if version: library_key = library_key.replace(branch=ModuleStoreEnum.BranchName.library, version_guid=version) - - library = self._get_library(library_key, is_v2_lib) + library = self._get_library(library_key) if library is None: raise ValueError(f"Requested library {library_key} not found.") - + if user_perms and not user_perms.can_read(library_key): + raise PermissionDenied() filter_children = (dest_block.capa_type != ANY_CAPA_TYPE_VALUE) - - if not is_v2_lib: - if user_perms and not user_perms.can_read(library_key): - raise PermissionDenied() - if filter_children: - # Apply simple filtering based on CAPA problem types: - source_blocks.extend(self._problem_type_filter(library, dest_block.capa_type)) - else: - source_blocks.extend(library.children) - - with self.store.bulk_operations(dest_block.location.course_key): - dest_block.source_library_version = str(library.location.library_key.version_guid) - self.store.update_item(dest_block, self.user_id) - head_validation = not version - dest_block.children = self.store.copy_from_template( - source_blocks, dest_block.location, self.user_id, head_validation=head_validation - ) - # ^-- copy_from_template updates the children in the DB - # but we must also set .children here to avoid overwriting the DB again + if filter_children: + # Apply simple filtering based on CAPA problem types: + source_blocks.extend(self._problem_type_filter(library, dest_block.capa_type)) else: - # TODO: add filtering by capa_type when V2 library will support different problem types - source_blocks = library_api.get_library_blocks(library_key, block_types=None) - source_block_ids = [str(block.usage_key) for block in source_blocks] - dest_block.source_library_version = str(library.version) + source_blocks.extend(library.children) + + with self.store.bulk_operations(dest_block.location.course_key): + dest_block.source_library_version = str(library.location.library_key.version_guid) self.store.update_item(dest_block, self.user_id) - self.import_from_blockstore(dest_block, source_block_ids) + head_validation = not version + dest_block.children = self.store.copy_from_template( + source_blocks, dest_block.location, self.user_id, head_validation=head_validation + ) + # ^-- copy_from_template updates the children in the DB + # but we must also set .children here to avoid overwriting the DB again def list_available_libraries(self): """ List all known libraries. - - Collects V1 libraries along with V2. - Returns tuples of (library key, display_name). + Returns tuples of (LibraryLocator, display_name) """ - user = User.objects.get(id=self.user_id) - - v1_libs = [ + return [ (lib.location.library_key.replace(version_guid=None, branch=None), lib.display_name) for lib in self.store.get_library_summaries() ] - v2_query = library_api.get_libraries_for_user(user) - v2_libs_with_meta = library_api.get_metadata_from_index(v2_query) - v2_libs = [(lib.key, lib.title) for lib in v2_libs_with_meta] - - return v1_libs + v2_libs def import_from_blockstore(self, dest_block, blockstore_block_ids): """ Imports a block from a blockstore-based learning context (usually a content library) into modulestore, as a new child of dest_block. Any existing children of dest_block are replaced. + + This is only used by LibrarySourcedBlock. It should verify first that + the number of block IDs is reasonable. """ dest_key = dest_block.scope_ids.usage_id if not isinstance(dest_key, BlockUsageLocator): @@ -308,7 +254,7 @@ class LibraryToolsService: new_block_key = generate_block_key(source_key, dest_parent_key) try: new_block = self.store.get_item(new_block_key) - if new_block.parent.block_id != dest_parent_key.block_id: + if new_block.parent != dest_parent_key: raise ValueError( "Expected existing block {} to be a child of {} but instead it's a child of {}".format( new_block_key, dest_parent_key, new_block.parent, diff --git a/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/xmodule/modulestore/split_mongo/caching_descriptor_system.py index d5c7c357dd..6849cc2c68 100644 --- a/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -3,12 +3,12 @@ import logging import sys -from crum import get_current_user from fs.osfs import OSFS from lazy import lazy from opaque_keys.edx.locator import BlockUsageLocator, DefinitionLocator, LocalId from xblock.fields import ScopeIds from xblock.runtime import KeyValueStore, KvsFieldData + from xmodule.error_module import ErrorBlock from xmodule.errortracker import exc_info_to_str from xmodule.library_tools import LibraryToolsService @@ -74,10 +74,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): # li self.module_data = module_data self.default_class = default_class self.local_modules = {} - - user = get_current_user() - user_id = user.id if user else None - self._services['library_tools'] = LibraryToolsService(modulestore, user_id=user_id) + self._services['library_tools'] = LibraryToolsService(modulestore, user_id=None) @lazy def _parent_map(self): # lint-amnesty, pylint: disable=missing-function-docstring diff --git a/xmodule/tests/test_library_content.py b/xmodule/tests/test_library_content.py index 6d7ced7593..3e1785746b 100644 --- a/xmodule/tests/test_library_content.py +++ b/xmodule/tests/test_library_content.py @@ -4,18 +4,16 @@ Basic unit tests for LibraryContentBlock Higher-level tests are in `cms/djangoapps/contentstore/tests/test_libraries.py`. """ from unittest.mock import MagicMock, Mock, patch - import ddt + from bson.objectid import ObjectId from fs.memoryfs import MemoryFS from lxml import etree -from opaque_keys.edx.locator import LibraryLocator, LibraryLocatorV2 -from rest_framework import status from search.search_engine_base import SearchEngine from web_fragments.fragment import Fragment from xblock.runtime import Runtime as VanillaRuntime +from rest_framework import status -from xmodule.capa_module import ProblemBlock from xmodule.library_content_module import ANY_CAPA_TYPE_VALUE, LibraryContentBlock from xmodule.library_tools import LibraryToolsService from xmodule.modulestore import ModuleStoreEnum @@ -24,6 +22,7 @@ from xmodule.modulestore.tests.utils import MixedSplitTestCase from xmodule.tests import get_test_system from xmodule.validation import StudioValidationMessage from xmodule.x_module import AUTHOR_VIEW +from xmodule.capa_module import ProblemBlock from .test_course_module import DummySystem as TestImportSystem @@ -75,32 +74,6 @@ class LibraryContentTest(MixedSplitTestCase): module.xmodule_runtime = module_system -@ddt.ddt -class LibraryContentGeneralTest(LibraryContentTest): - """ - Test the base functionality of the LibraryContentBlock. - """ - - @ddt.data( - ('library-v1:ProblemX+PR0B', LibraryLocator), - ('lib:ORG:test-1', LibraryLocatorV2) - ) - @ddt.unpack - def test_source_library_key(self, library_key, expected_locator_type): - """ - Test the source_library_key property of the xblock. - - The method should correctly work either with V1 or V2 libraries. - """ - library = self.make_block( - "library_content", - self.vertical, - max_count=1, - source_library_id=library_key - ) - assert isinstance(library.source_library_key, expected_locator_type) - - class TestLibraryContentExportImport(LibraryContentTest): """ Export and import tests for LibraryContentBlock diff --git a/xmodule/tests/test_library_tools.py b/xmodule/tests/test_library_tools.py index 0e5f59aac4..950307b696 100644 --- a/xmodule/tests/test_library_tools.py +++ b/xmodule/tests/test_library_tools.py @@ -1,49 +1,35 @@ """ Tests for library tools service. """ - from unittest.mock import patch -import ddt -from bson.objectid import ObjectId from opaque_keys.edx.keys import UsageKey -from opaque_keys.edx.locator import LibraryLocator, LibraryLocatorV2 +from openedx.core.djangoapps.content_libraries import api as library_api +from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest +from openedx.core.djangoapps.xblock.api import load_block +from common.djangoapps.student.roles import CourseInstructorRole from xmodule.library_tools import LibraryToolsService from xmodule.modulestore.tests.factories import CourseFactory, LibraryFactory from xmodule.modulestore.tests.utils import MixedSplitTestCase -from common.djangoapps.student.roles import CourseInstructorRole -from common.djangoapps.student.tests.factories import UserFactory -from openedx.core.djangoapps.content_libraries import api as library_api -from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest -from openedx.core.djangoapps.xblock.api import load_block - -@ddt.ddt -class ContentLibraryToolsTest(MixedSplitTestCase, ContentLibrariesRestApiTest): +class LibraryToolsServiceTest(MixedSplitTestCase): """ - Tests for LibraryToolsService. - - Tests interaction with blockstore-based (V2) and mongo-based (V1) content libraries. + Tests for library service. """ + def setUp(self): super().setUp() - UserFactory(is_staff=True, id=self.user_id) self.tools = LibraryToolsService(self.store, self.user_id) def test_list_available_libraries(self): """ Test listing of libraries. - - Should include either V1 or V2 libraries. """ - # create V1 library _ = LibraryFactory.create(modulestore=self.store) - # create V2 library - self._create_library(slug="testlib1_preview", title="Test Library 1", description="Testing XBlocks") all_libraries = self.tools.list_available_libraries() assert all_libraries - assert len(all_libraries) == 2 + assert len(all_libraries) == 1 @patch('xmodule.modulestore.split_mongo.split.SplitMongoModuleStore.get_library_summaries') def test_list_available_libraries_fetch(self, mock_get_library_summaries): @@ -53,6 +39,15 @@ class ContentLibraryToolsTest(MixedSplitTestCase, ContentLibrariesRestApiTest): _ = self.tools.list_available_libraries() assert mock_get_library_summaries.called + +class ContentLibraryToolsTest(MixedSplitTestCase, ContentLibrariesRestApiTest): + """ + Tests for LibraryToolsService which interact with blockstore-based content libraries + """ + def setUp(self): + super().setUp() + self.tools = LibraryToolsService(self.store, self.user.id) + def test_import_from_blockstore(self): # Create a blockstore content library library = self._create_library(slug="testlib1_import", title="A Test Library", description="Testing XBlocks") @@ -99,105 +94,3 @@ class ContentLibraryToolsTest(MixedSplitTestCase, ContentLibrariesRestApiTest): imported_html_block = self.store.get_item(imported_unit_block.children[0]) assert 'Hello world' not in imported_html_block.data assert 'Foo bar' in imported_html_block.data - - def test_get_v2_library_version(self): - """ - Test get_library_version for V2 libraries. - - Covers getting results for either library key as a string or LibraryLocatorV2. - - NOTE: - We don't publish library updates so the library version will always be 0. - """ - lib = self._create_library(slug="testlib1-slug", title="Test Library 1", description="Testing Library 1") - # use library key as a string for getting the library version - result = self.tools.get_library_version(lib['id']) - assert isinstance(result, int) - assert result == 0 - # now do the same but use library key as a LibraryLocatorV2 - result2 = self.tools.get_library_version(LibraryLocatorV2.from_string(lib['id'])) - assert isinstance(result, int) - assert result2 == 0 - - def test_get_v1_library_version(self): - """ - Test get_library_version for V1 libraries. - - Covers getting results for either string library key or LibraryLocator. - """ - lib_key = LibraryFactory.create(modulestore=self.store).location.library_key - # Re-load the library from the modulestore, explicitly including version information: - lib = self.store.get_library(lib_key, remove_version=False, remove_branch=False) - # check the result using the LibraryLocator - assert isinstance(lib_key, LibraryLocator) - result = self.tools.get_library_version(lib_key) - assert result - assert isinstance(result, ObjectId) - assert result == lib.location.library_key.version_guid - # the same check for string representation of the LibraryLocator - str_key = str(lib_key) - result = self.tools.get_library_version(str_key) - assert result - assert isinstance(result, ObjectId) - assert result == lib.location.library_key.version_guid - - @ddt.data( - 'library-v1:Fake+Key', # V1 library key - 'lib:Fake:V-2', # V2 library key - LibraryLocator.from_string('library-v1:Fake+Key'), - LibraryLocatorV2.from_string('lib:Fake:V-2'), - ) - def test_get_library_version_no_library(self, lib_key): - """ - Test get_library_version result when the library does not exist. - - Provided lib_key's are valid V1 or V2 keys. - """ - assert self.tools.get_library_version(lib_key) is None - - def test_update_children_for_v2_lib(self): - """ - Test update_children with V2 library as a source. - - As for now, covers usage of update_children for the library content module only. - """ - library = self._create_library( - slug="cool-v2-lib", title="The best Library", description="Spectacular description" - ) - self._add_block_to_library(library["id"], "unit", "unit1_id")["id"] # pylint: disable=expression-not-assigned - - course = CourseFactory.create(modulestore=self.store, user_id=self.user.id) - CourseInstructorRole(course.id).add_users(self.user) - - content_block = self.make_block( - "library_content", - course, - max_count=1, - source_library_id=library['id'] - ) - - assert len(content_block.children) == 0 - self.tools.update_children(content_block) - content_block = self.store.get_item(content_block.location) - assert len(content_block.children) == 1 - - def test_update_children_for_v1_lib(self): - """ - Test update_children with V1 library as a source. - - As for now, covers usage of update_children for the library content module only. - """ - library = LibraryFactory.create(modulestore=self.store) - self.make_block("html", library, data="Hello world from the block") - course = CourseFactory.create(modulestore=self.store) - content_block = self.make_block( - "library_content", - course, - max_count=1, - source_library_id=str(library.location.library_key) - ) - - assert len(content_block.children) == 0 - self.tools.update_children(content_block) - content_block = self.store.get_item(content_block.location) - assert len(content_block.children) == 1