diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 73c0231aa3..728432f0c1 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -364,8 +364,8 @@ def get_library(library_key): def create_library( - collection_uuid, library_type, org, slug, title, description, allow_public_learning, allow_public_read, - library_license, + collection_uuid, library_type, org, slug, title, description, allow_public_learning, allow_public_read, + library_license, ): """ Create a new content library. @@ -490,13 +490,13 @@ def set_library_group_permissions(library_key, group, access_level): def update_library( - library_key, - title=None, - description=None, - allow_public_learning=None, - allow_public_read=None, - library_type=None, - library_license=None, + library_key, + title=None, + description=None, + allow_public_learning=None, + allow_public_read=None, + library_type=None, + library_license=None, ): """ Update a library's metadata @@ -505,6 +505,7 @@ def update_library( A value of None means "don't change". """ ref = ContentLibrary.objects.get_by_key(library_key) + # Update MySQL model: changed = False if allow_public_learning is not None: @@ -514,7 +515,15 @@ def update_library( ref.allow_public_read = allow_public_read changed = True if library_type is not None: - if library_type != COMPLEX: + if library_type not in (COMPLEX, ref.type): + lib_bundle = LibraryBundle(library_key, ref.bundle_uuid, draft_name=DRAFT_NAME) + (has_unpublished_changes, has_unpublished_deletes) = lib_bundle.has_changes() + if has_unpublished_changes or has_unpublished_deletes: + raise IncompatibleTypesError( + _( + 'You may not change a library\'s type to {library_type} if it still has unpublished changes.' + ).format(library_type=library_type) + ) for block in get_library_blocks(library_key): if block.usage_key.block_type != library_type: raise IncompatibleTypesError( @@ -572,7 +581,7 @@ def delete_library(library_key): raise -def get_library_blocks(library_key, text_search=None): +def get_library_blocks(library_key, text_search=None, block_types=None): """ Get the list of top-level XBlocks in the specified library. @@ -585,6 +594,8 @@ def get_library_blocks(library_key, text_search=None): 'library_key': [str(library_key)], 'is_child': [False], } + if block_types: + filter_terms['block_type'] = block_types metadata = [ { **item, @@ -609,9 +620,11 @@ def get_library_blocks(library_key, text_search=None): # have only a single usage in the library, which is part of the definition of top level block. def_key = lib_bundle.definition_for_usage(usage_key) display_name = get_block_display_name(def_key) - if (text_search is None or - text_search.lower() in display_name.lower() or - text_search.lower() in str(usage_key).lower()): + text_match = (text_search is None or + text_search.lower() in display_name.lower() or + text_search.lower() in str(usage_key).lower()) + type_match = (block_types is None or usage_key.block_type in block_types) + if text_match and type_match: metadata.append({ "id": usage_key, "def_key": def_key, diff --git a/openedx/core/djangoapps/content_libraries/libraries_index.py b/openedx/core/djangoapps/content_libraries/libraries_index.py index 60b8f45312..78583d7252 100644 --- a/openedx/core/djangoapps/content_libraries/libraries_index.py +++ b/openedx/core/djangoapps/content_libraries/libraries_index.py @@ -75,7 +75,6 @@ class SearchIndexerBase(ABC): "schema_version": [cls.SCHEMA_VERSION], **filter_terms, } - if text_search: response = cls._perform_elastic_search(filter_terms, text_search) else: diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index dfb1960819..4200d15116 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -269,7 +269,7 @@ class ContentLibrariesRestApiTest(APITestCase): query_params_dict = {} return self._api( 'get', - URL_LIB_BLOCKS.format(lib_key=lib_key) + '?' + urlencode(query_params_dict), + URL_LIB_BLOCKS.format(lib_key=lib_key) + '?' + urlencode(query_params_dict, doseq=True), None, expect_response ) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index ca54adf21b..5dd6165a0f 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -127,7 +127,8 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): self.assertEqual(lib['type'], start_type) for block_type, block_slug in xblock_specs: self._add_block_to_library(lib['id'], block_type, block_slug) - result = self._update_library(lib["id"], type=target_type, expect_response=expect_response) + self._commit_library_changes(lib['id']) + result = self._update_library(lib['id'], type=target_type, expect_response=expect_response) if expect_response == 200: self.assertEqual(result['type'], target_type) self.assertIn('type', result) @@ -135,6 +136,32 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): lib = self._get_library(lib['id']) self.assertEqual(lib['type'], start_type) + def test_no_convert_on_unpublished(self): + """ + Verify that you can't change a library's type, even if it would normally be valid, + when there are unpublished changes. This is so that a reversion of blocks won't cause an inconsistency. + """ + lib = self._create_library( + slug='resolute', title="A complex library", description="Unconvertable", library_type=COMPLEX, + ) + self._add_block_to_library(lib['id'], "video", 'vid-block') + result = self._update_library(lib['id'], type=VIDEO, expect_response=400) + self.assertIn('type', result) + + def test_no_convert_on_pending_deletes(self): + """ + Verify that you can't change a library's type, even if it would normally be valid, + when there are unpublished changes. This is so that a reversion of blocks won't cause an inconsistency. + """ + lib = self._create_library( + slug='still-alive', title="A complex library", description="Unconvertable", library_type=COMPLEX, + ) + block = self._add_block_to_library(lib['id'], "video", 'vid-block') + self._commit_library_changes(lib['id']) + self._delete_library_block(block['id']) + result = self._update_library(lib['id'], type=VIDEO, expect_response=400) + self.assertIn('type', result) + def test_library_validation(self): """ You can't create a library with the same slug as an existing library, @@ -349,14 +376,26 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): with override_settings(FEATURES={**settings.FEATURES, 'ENABLE_CONTENT_LIBRARY_INDEX': is_indexing_enabled}): lib = self._create_library(slug="test-lib-blocks" + str(is_indexing_enabled), title="Title") block1 = self._add_block_to_library(lib["id"], "problem", "foo-bar") + self._add_block_to_library(lib["id"], "video", "vid-baz") + self._add_block_to_library(lib["id"], "html", "html-baz") self._add_block_to_library(lib["id"], "problem", "foo-baz") self._add_block_to_library(lib["id"], "problem", "bar-baz") self._set_library_block_olx(block1["id"], "") - self.assertEqual(len(self._get_library_blocks(lib["id"])), 3) + self.assertEqual(len(self._get_library_blocks(lib["id"])), 5) self.assertEqual(len(self._get_library_blocks(lib["id"], {'text_search': 'Foo'})), 2) self.assertEqual(len(self._get_library_blocks(lib["id"], {'text_search': 'Display'})), 1) + self.assertEqual(len(self._get_library_blocks(lib["id"], {'text_search': 'Video'})), 1) + self.assertEqual(len(self._get_library_blocks(lib["id"], {'text_search': 'Foo', 'block_type': 'video'})), 0) + self.assertEqual(len(self._get_library_blocks(lib["id"], {'text_search': 'Baz', 'block_type': 'video'})), 1) + self.assertEqual(len( + self._get_library_blocks(lib["id"], {'text_search': 'Baz', 'block_type': ['video', 'html']})), + 2, + ) + self.assertEqual(len(self._get_library_blocks(lib["id"], {'block_type': 'video'})), 1) + self.assertEqual(len(self._get_library_blocks(lib["id"], {'block_type': 'problem'})), 3) + self.assertEqual(len(self._get_library_blocks(lib["id"], {'block_type': 'squirrel'})), 0) @ddt.data( ('video-problem', VIDEO, 'problem', 400), @@ -478,8 +517,6 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): {"username": author.username, "group_name": None, "access_level": "author"}, reader_grant, ] - from pprint import pprint - pprint(team_response) for entry, expected in zip(team_response, expected_response): self.assertDictContainsEntries(entry, expected) diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index 9015400561..cedc102995 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -485,6 +485,12 @@ class LibraryBlocksView(APIView): str, description="The string used to filter libraries by searching in title, id, org, or description", ), + apidocs.query_parameter( + 'block_type', + str, + description="The block type to search for. If omitted or blank, searches for all types. " + "May be specified multiple times to match multiple types." + ) ], ) @convert_exceptions @@ -494,9 +500,10 @@ class LibraryBlocksView(APIView): """ key = LibraryLocatorV2.from_string(lib_key_str) text_search = request.query_params.get('text_search', None) + block_types = request.query_params.getlist('block_type') or None api.require_permission_for_library_key(key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) - result = api.get_library_blocks(key, text_search=text_search) + result = api.get_library_blocks(key, text_search=text_search, block_types=block_types) # Verify `pagination` param to maintain compatibility with older # non pagination-aware clients