Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Add ability to filter library blocks by type. Prevent changing types with unpublished changes. (#25368)

Browse Source 
Adds the ability to filter v2 library blocks by block type. Also prevents switching the library type when there are unpublished changes/deletes, as this may cause consistency errors.
This commit is contained in:
Fox Piacenti
2020-10-23 09:50:28 -05:00
committed by GitHub
parent 3e2fc5d824
commit c181ed57b1
5 changed files with 77 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files

41
openedx/core/djangoapps/content_libraries/api.py
View File

@@ -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,

1
openedx/core/djangoapps/content_libraries/libraries_index.py
View File

@@ -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:

2
openedx/core/djangoapps/content_libraries/tests/base.py
View File

@@ -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
)

45
openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py
View File

@@ -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"], "<problem display_name=\"DisplayName\"></problem>")
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)

9
openedx/core/djangoapps/content_libraries/views.py
View File

@@ -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