diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index cd924868cc..22955cc415 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -279,7 +279,7 @@ class StudioPermissionsService(object): Deprecated. To be replaced by a more general authorization service. - Only used by LibraryContentDescriptor (and library_tools.py). + Only used by LibraryContentBlock (and library_tools.py). """ def __init__(self, user): self._user = user @@ -305,7 +305,7 @@ class StudioEditModuleRuntime(object): def service(self, block, service_name): """ - This block is not bound to a user but some blocks (LibraryContentModule) may need + This block is not bound to a user but some blocks (LibraryContentBlock) may need user-specific services to check for permissions, etc. If we return None here, CombinedSystem will load services from the descriptor runtime. """ diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index 1247955ac6..2a54bac6f9 100644 --- a/common/lib/xmodule/setup.py +++ b/common/lib/xmodule/setup.py @@ -10,7 +10,6 @@ XMODULES = [ "customtag = xmodule.template_module:CustomTagDescriptor", "discuss = xmodule.backcompat_module:TranslateCustomTagDescriptor", "image = xmodule.backcompat_module:TranslateCustomTagDescriptor", - "library_content = xmodule.library_content_module:LibraryContentDescriptor", "error = xmodule.error_module:ErrorDescriptor", "nonstaff_error = xmodule.error_module:NonStaffErrorDescriptor", "poll_question = xmodule.poll_module:PollDescriptor", @@ -33,6 +32,7 @@ XBLOCKS = [ "course_info = xmodule.html_module:CourseInfoBlock", "html = xmodule.html_module:HtmlBlock", "library = xmodule.library_root_xblock:LibraryRoot", + "library_content = xmodule.library_content_module:LibraryContentBlock", "library_sourced = xmodule.library_sourced_block:LibrarySourcedBlock", "problem = xmodule.capa_module:ProblemBlock", "static_tab = xmodule.html_module:StaticTabBlock", diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index d796e870c3..14b01810f4 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -24,12 +24,21 @@ from xblock.core import XBlock from xblock.fields import Integer, List, Scope, String from capa.responsetypes import registry -from xmodule.studio_editable import StudioEditableDescriptor, StudioEditableModule +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.x_module import STUDENT_VIEW, XModule +from xmodule.xml_module import XmlMixin +from xmodule.x_module import ( + HTMLSnippet, + ResourceTemplates, + shim_xmodule_js, + STUDENT_VIEW, + XModuleMixin, + XModuleDescriptorToXBlockMixin, + XModuleToXBlockMixin, +) -from .mako_module import MakoModuleDescriptor -from .xml_module import XmlDescriptor # 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 @@ -59,17 +68,57 @@ def _get_capa_types(): ], key=lambda item: item.get('display_name')) -class LibraryContentFields(object): +@XBlock.wants('library_tools') # Only needed in studio +@XBlock.wants('studio_user_permissions') # Only available in studio +@XBlock.wants('user') +class LibraryContentBlock( + MakoTemplateBlockBase, + XmlMixin, + XModuleDescriptorToXBlockMixin, + XModuleToXBlockMixin, + HTMLSnippet, + ResourceTemplates, + XModuleMixin, + StudioEditableBlock, +): """ - Fields for the LibraryContentModule. + An XBlock whose children are chosen dynamically from a content library. + Can be used to create randomized assessments among other things. - Separated out for now because they need to be added to the module and the - descriptor. + Note: technically, all matching blocks from the content library are added + as children of this block, but only a subset of those children are shown to + any particular student. """ + # pylint: disable=abstract-method + has_children = True + has_author_view = True + + resources_dir = 'assets/library_content' + + preview_view_js = { + 'js': [], + 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + } + preview_view_css = { + 'scss': [], + } + + mako_template = 'widgets/metadata-edit.html' + studio_js_module_name = "VerticalDescriptor" + studio_view_js = { + 'js': [ + resource_string(__name__, 'js/src/vertical/edit.js'), + ], + 'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'), + } + studio_view_css = { + 'scss': [], + } + + show_in_read_only_mode = True + completion_mode = XBlockCompletionMode.AGGREGATOR - # Please note the display_name of each field below is used in - # common/test/acceptance/pages/studio/library.py:StudioLibraryContentXBlockEditModal - # to locate input elements - keep synchronized + display_name = String( display_name=_("Display Name"), help=_("The display name for this component."), @@ -117,7 +166,6 @@ class LibraryContentFields(object): default=[], scope=Scope.user_state, ) - has_children = True @property def source_library_key(self): @@ -126,19 +174,6 @@ class LibraryContentFields(object): """ return LibraryLocator.from_string(self.source_library_id) - -#pylint: disable=abstract-method -@XBlock.wants('library_tools') # Only needed in studio -class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): - """ - An XBlock whose children are chosen dynamically from a content library. - Can be used to create randomized assessments among other things. - - Note: technically, all matching blocks from the content library are added - as children of this block, but only a subset of those children are shown to - any particular student. - """ - @classmethod def make_selection(cls, selected, children, max_count, mode): """ @@ -341,12 +376,6 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): })) return fragment - def validate(self): - """ - Validates the state of this Library Content Module Instance. - """ - return self.descriptor.validate() - def author_view(self, context): """ Renders the Studio views. @@ -375,37 +404,33 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): fragment.initialize_js('LibraryContentAuthorView') return fragment + def studio_view(self, _context): + """ + Return the studio view. + """ + fragment = Fragment( + self.system.render_template(self.mako_template, self.get_context()) + ) + add_webpack_to_fragment(fragment, 'LibraryContentBlockStudio') + shim_xmodule_js(fragment, self.studio_js_module_name) + return fragment + def get_child_descriptors(self): """ Return only the subset of our children relevant to the current student. """ return list(self._get_selected_child_blocks()) - -@XBlock.wants('user') -@XBlock.wants('library_tools') # Only needed in studio -@XBlock.wants('studio_user_permissions') # Only available in studio -class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDescriptor, StudioEditableDescriptor): - """ - Descriptor class for LibraryContentModule XBlock. - """ - - resources_dir = 'assets/library_content' - - module_class = LibraryContentModule - mako_template = 'widgets/metadata-edit.html' - js = {'js': [resource_string(__name__, 'js/src/vertical/edit.js')]} - js_module_name = "VerticalDescriptor" - - show_in_read_only_mode = True - @property def non_editable_metadata_fields(self): - non_editable_fields = super(LibraryContentDescriptor, self).non_editable_metadata_fields + non_editable_fields = super().non_editable_metadata_fields # The only supported mode is currently 'random'. # Add the mode field to non_editable_metadata_fields so that it doesn't # render in the edit form. - non_editable_fields.extend([LibraryContentFields.mode, LibraryContentFields.source_library_version]) + non_editable_fields.extend([ + LibraryContentBlock.mode, + LibraryContentBlock.source_library_version, + ]) return non_editable_fields @lazy @@ -525,7 +550,7 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe is the override of the general XBlock method, and it will also ask its superclass to validate. """ - validation = super(LibraryContentDescriptor, self).validate() + validation = super().validate() if not isinstance(validation, StudioValidation): validation = StudioValidation.copy(validation) library_tools = self.runtime.service(self, "library_tools") @@ -636,7 +661,7 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe This overwrites the get_content_titles method included in x_module by default. """ titles = [] - for child in self._xmodule.get_child_descriptors(): + for child in self.get_child_descriptors(): titles.extend(child.get_content_titles()) return titles diff --git a/common/lib/xmodule/xmodule/library_tools.py b/common/lib/xmodule/xmodule/library_tools.py index ef754be232..6968eb1ccd 100644 --- a/common/lib/xmodule/xmodule/library_tools.py +++ b/common/lib/xmodule/xmodule/library_tools.py @@ -1,5 +1,5 @@ """ -XBlock runtime services for LibraryContentModule +XBlock runtime services for LibraryContentBlock """ import hashlib @@ -27,7 +27,7 @@ def normalize_key_for_search(library_key): class LibraryToolsService(object): """ - Service that allows LibraryContentModule to interact with libraries in the + Service that allows LibraryContentBlock to interact with libraries in the modulestore. """ def __init__(self, modulestore, user_id): @@ -137,7 +137,7 @@ class LibraryToolsService(object): def update_children(self, dest_block, user_perms=None, version=None): """ - This method is to be used when the library that a LibraryContentModule + 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 diff --git a/common/lib/xmodule/xmodule/static_content.py b/common/lib/xmodule/xmodule/static_content.py index b3806711af..44d8fa4033 100755 --- a/common/lib/xmodule/xmodule/static_content.py +++ b/common/lib/xmodule/xmodule/static_content.py @@ -22,6 +22,7 @@ from path import Path as path from xmodule.capa_module import ProblemBlock from xmodule.html_module import AboutBlock, CourseInfoBlock, HtmlBlock, StaticTabBlock +from xmodule.library_content_module import LibraryContentBlock from xmodule.word_cloud_module import WordCloudBlock from xmodule.x_module import XModuleDescriptor, HTMLSnippet @@ -67,6 +68,7 @@ XBLOCK_CLASSES = [ AboutBlock, CourseInfoBlock, HtmlBlock, + LibraryContentBlock, ProblemBlock, StaticTabBlock, VideoBlock, diff --git a/common/lib/xmodule/xmodule/tests/test_course_module.py b/common/lib/xmodule/xmodule/tests/test_course_module.py index 2a2120e013..bb726bb3c3 100644 --- a/common/lib/xmodule/xmodule/tests/test_course_module.py +++ b/common/lib/xmodule/xmodule/tests/test_course_module.py @@ -40,11 +40,12 @@ class CourseFieldsTestCase(unittest.TestCase): class DummySystem(ImportSystem): @patch('xmodule.modulestore.xml.OSFS', lambda dir: MemoryFS()) - def __init__(self, load_error_modules): + def __init__(self, load_error_modules, course_id=None): xmlstore = XMLModuleStore("data_dir", source_dirs=[], load_error_modules=load_error_modules) - course_id = CourseKey.from_string('/'.join([ORG, COURSE, 'test_run'])) + if course_id is None: + course_id = CourseKey.from_string('/'.join([ORG, COURSE, 'test_run'])) course_dir = "test_dir" error_tracker = Mock() diff --git a/common/lib/xmodule/xmodule/tests/test_library_content.py b/common/lib/xmodule/xmodule/tests/test_library_content.py index 888dd4cfad..8049f1a9fe 100644 --- a/common/lib/xmodule/xmodule/tests/test_library_content.py +++ b/common/lib/xmodule/xmodule/tests/test_library_content.py @@ -1,6 +1,6 @@ # -*- coding: utf-8 -*- """ -Basic unit tests for LibraryContentModule +Basic unit tests for LibraryContentBlock Higher-level tests are in `cms/djangoapps/contentstore/tests/test_libraries.py`. """ @@ -8,13 +8,15 @@ Higher-level tests are in `cms/djangoapps/contentstore/tests/test_libraries.py`. import six from bson.objectid import ObjectId +from fs.memoryfs import MemoryFS +from lxml import etree from mock import Mock, patch from search.search_engine_base import SearchEngine from six.moves import range from web_fragments.fragment import Fragment from xblock.runtime import Runtime as VanillaRuntime -from xmodule.library_content_module import ANY_CAPA_TYPE_VALUE, LibraryContentDescriptor +from xmodule.library_content_module import ANY_CAPA_TYPE_VALUE, LibraryContentBlock from xmodule.library_tools import LibraryToolsService from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.factories import CourseFactory, LibraryFactory @@ -23,12 +25,14 @@ from xmodule.tests import get_test_system from xmodule.validation import StudioValidationMessage from xmodule.x_module import AUTHOR_VIEW +from .test_course_module import DummySystem as TestImportSystem + dummy_render = lambda block, _: Fragment(block.data) # pylint: disable=invalid-name class LibraryContentTest(MixedSplitTestCase): """ - Base class for tests of LibraryContentModule (library_content_module.py) + Base class for tests of LibraryContentBlock (library_content_block.py) """ def setUp(self): super(LibraryContentTest, self).setUp() @@ -70,9 +74,73 @@ class LibraryContentTest(MixedSplitTestCase): module.xmodule_runtime = module_system -class LibraryContentModuleTestMixin(object): +class TestLibraryContentExportImport(LibraryContentTest): """ - Basic unit tests for LibraryContentModule + Export and import tests for LibraryContentBlock + """ + + maxDiff = None + + def test_xml_export_import_cycle(self): + """ + Test the export-import cycle. + """ + # Children will only set after calling this. + self.lc_block.refresh_children() + lc_block = self.store.get_item(self.lc_block.location) + + expected_olx = ( + '\n' + ' \n' + ' \n' + ' \n' + ' \n' + '\n' + ).format( + block=lc_block, + ) + + export_fs = MemoryFS() + # Set the virtual FS to export the olx to. + lc_block.runtime._descriptor_system.export_fs = export_fs # pylint: disable=protected-access + + # Export the olx. + node = etree.Element("unknown_root") + lc_block.add_xml_to_node(node) + + # Read it back + with export_fs.open('{dir}/{file_name}.xml'.format( + dir=lc_block.scope_ids.usage_id.block_type, + file_name=lc_block.scope_ids.usage_id.block_id + )) as f: + exported_olx = f.read() + + # And compare. + self.assertEqual(exported_olx, expected_olx) + + runtime = TestImportSystem(load_error_modules=True, course_id=lc_block.location.course_key) + runtime.resources_fs = export_fs + + # Now import it. + olx_element = etree.fromstring(exported_olx) + id_generator = Mock() + imported_lc_block = LibraryContentBlock.parse_xml(olx_element, runtime, None, id_generator) + + # Check the new XBlock has the same properties as the old one. + self.assertEqual(imported_lc_block.display_name, lc_block.display_name) + self.assertEqual(imported_lc_block.source_library_id, lc_block.source_library_id) + self.assertEqual(imported_lc_block.source_library_version, lc_block.source_library_version) + self.assertEqual(imported_lc_block.mode, lc_block.mode) + self.assertEqual(imported_lc_block.max_count, lc_block.max_count) + self.assertEqual(imported_lc_block.capa_type, lc_block.capa_type) + self.assertEqual(len(imported_lc_block.children), 4) + self.assertEqual(imported_lc_block.children, lc_block.children) + + +class LibraryContentBlockTestMixin(object): + """ + Basic unit tests for LibraryContentBlock """ problem_types = [ ["multiplechoiceresponse"], ["optionresponse"], ["optionresponse", "coderesponse"], @@ -241,8 +309,8 @@ class LibraryContentModuleTestMixin(object): Test the settings that are marked as "non-editable". """ non_editable_metadata_fields = self.lc_block.non_editable_metadata_fields - self.assertIn(LibraryContentDescriptor.mode, non_editable_metadata_fields) - self.assertNotIn(LibraryContentDescriptor.display_name, non_editable_metadata_fields) + self.assertIn(LibraryContentBlock.mode, non_editable_metadata_fields) + self.assertNotIn(LibraryContentBlock.display_name, non_editable_metadata_fields) def test_overlimit_blocks_chosen_randomly(self): """ @@ -272,8 +340,6 @@ class LibraryContentModuleTestMixin(object): Helper method that changes the max_count of self.lc_block, refreshes children, and asserts that the number of selected children equals the count provided. """ - # Construct the XModule for the descriptor, if not present already. - self.lc_block._xmodule # pylint: disable=pointless-statement,protected-access self.lc_block.max_count = count selected = self.lc_block.get_child_descriptors() self.assertEqual(len(selected), count) @@ -281,7 +347,7 @@ class LibraryContentModuleTestMixin(object): @patch('xmodule.library_tools.SearchEngine.get_search_engine', Mock(return_value=None, autospec=True)) -class TestLibraryContentModuleNoSearchIndex(LibraryContentModuleTestMixin, LibraryContentTest): +class TestLibraryContentBlockNoSearchIndex(LibraryContentBlockTestMixin, LibraryContentTest): """ Tests for library container when no search index is available. Tests fallback low-level CAPA problem introspection @@ -293,7 +359,7 @@ search_index_mock = Mock(spec=SearchEngine) # pylint: disable=invalid-name @patch('xmodule.library_tools.SearchEngine.get_search_engine', Mock(return_value=search_index_mock, autospec=True)) -class TestLibraryContentModuleWithSearchIndex(LibraryContentModuleTestMixin, LibraryContentTest): +class TestLibraryContentBlockWithSearchIndex(LibraryContentBlockTestMixin, LibraryContentTest): """ Tests for library container with mocked search engine response. """ @@ -312,7 +378,7 @@ class TestLibraryContentModuleWithSearchIndex(LibraryContentModuleTestMixin, Lib def setUp(self): """ Sets up search engine mock """ - super(TestLibraryContentModuleWithSearchIndex, self).setUp() + super(TestLibraryContentBlockWithSearchIndex, self).setUp() search_index_mock.search = Mock(side_effect=self._get_search_response) @@ -323,7 +389,7 @@ class TestLibraryContentModuleWithSearchIndex(LibraryContentModuleTestMixin, Lib @patch('xmodule.x_module.DescriptorSystem.applicable_aside_types', lambda self, block: []) class TestLibraryContentRender(LibraryContentTest): """ - Rendering unit tests for LibraryContentModule + Rendering unit tests for LibraryContentBlock """ def test_preview_view(self): @@ -348,7 +414,7 @@ class TestLibraryContentRender(LibraryContentTest): class TestLibraryContentAnalytics(LibraryContentTest): """ - Test analytics features of LibraryContentModule + Test analytics features of LibraryContentBlock """ def setUp(self): @@ -361,7 +427,7 @@ class TestLibraryContentAnalytics(LibraryContentTest): def _assert_event_was_published(self, event_type): """ - Check that a LibraryContentModule analytics event was published by self.lc_block. + Check that a LibraryContentBlock analytics event was published by self.lc_block. """ self.assertTrue(self.publisher.called) self.assertTrue(len(self.publisher.call_args[0]), 3) # pylint:disable=unsubscriptable-object diff --git a/lms/djangoapps/course_blocks/transformers/library_content.py b/lms/djangoapps/course_blocks/transformers/library_content.py index 59e0d5f640..61a868bcb1 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -15,7 +15,7 @@ from openedx.core.djangoapps.content.block_structure.transformer import ( FilteringTransformerMixin ) from track import contexts -from xmodule.library_content_module import LibraryContentModule +from xmodule.library_content_module import LibraryContentBlock from xmodule.modulestore.django import modulestore from ..utils import get_student_module_as_dict @@ -99,7 +99,7 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo # Update selected previous_count = len(selected) - block_keys = LibraryContentModule.make_selection(selected, library_children, max_count, mode) + block_keys = LibraryContentBlock.make_selection(selected, library_children, max_count, mode) selected = block_keys['selected'] # Save back any changes @@ -175,7 +175,7 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo with tracker.get_tracker().context(full_event_name, context): tracker.emit(full_event_name, event_data) - LibraryContentModule.publish_selected_children_events( + LibraryContentBlock.publish_selected_children_events( block_keys, format_block_keys, publish_event,