From f2ed54d72cabf4624f31c312a338917219ce45d9 Mon Sep 17 00:00:00 2001 From: Sid Verma Date: Thu, 6 Aug 2020 18:37:08 +0530 Subject: [PATCH] Add filtering support for list_library_blocks API --- .../core/djangoapps/content_libraries/api.py | 64 ++++++++++++++----- .../content_libraries/libraries_index.py | 3 +- .../content_libraries/serializers.py | 2 +- .../content_libraries/tests/base.py | 11 +++- .../tests/test_content_libraries.py | 55 +++++++++++++++- .../djangoapps/content_libraries/views.py | 61 ++++++++++++------ 6 files changed, 158 insertions(+), 38 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 5ad5bc9e73..0f74998081 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -58,7 +58,11 @@ from xblock.exceptions import XBlockNotFoundError from openedx.core.djangoapps.content_libraries import permissions from openedx.core.djangoapps.content_libraries.constants import DRAFT_NAME from openedx.core.djangoapps.content_libraries.library_bundle import LibraryBundle -from openedx.core.djangoapps.content_libraries.libraries_index import ContentLibraryIndexer, ItemNotIndexedException +from openedx.core.djangoapps.content_libraries.libraries_index import ( + ContentLibraryIndexer, + LibraryBlockIndexer, + ItemNotIndexedException, +) from openedx.core.djangoapps.content_libraries.models import ContentLibrary, ContentLibraryPermission from openedx.core.djangoapps.content_libraries.signals import ( CONTENT_LIBRARY_CREATED, @@ -497,28 +501,58 @@ def delete_library(library_key): raise -def get_library_blocks(library_key): +def get_library_blocks(library_key, text_search=None): """ Get the list of top-level XBlocks in the specified library. Returns a list of LibraryXBlockMetadata objects """ + metadata = None ref = ContentLibrary.objects.get_by_key(library_key) lib_bundle = LibraryBundle(library_key, ref.bundle_uuid, draft_name=DRAFT_NAME) usages = lib_bundle.get_top_level_usages() - blocks = [] - for usage_key in usages: - # For top-level definitions, we can go from definition key to usage key using the following, but this would not - # work for non-top-level blocks as they may have multiple usages. Top level blocks are guaranteed to 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) - blocks.append(LibraryXBlockMetadata( - usage_key=usage_key, - def_key=def_key, - display_name=get_block_display_name(def_key), - has_unpublished_changes=lib_bundle.does_definition_have_unpublished_changes(def_key), - )) - return blocks + + if LibraryBlockIndexer.indexing_is_enabled(): + try: + metadata = [ + { + **item, + "id": LibraryUsageLocatorV2.from_string(item['id']), + } + for item in LibraryBlockIndexer.get_items(usages, text_search=text_search) + if item is not None + ] + except (ItemNotIndexedException, KeyError, ConnectionError) as e: + log.exception(e) + + # If indexing is disabled, or connection to elastic failed + if metadata is None: + metadata = [] + for usage_key in usages: + # For top-level definitions, we can go from definition key to usage key using the following, but this would not + # work for non-top-level blocks as they may have multiple usages. Top level blocks are guaranteed to 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()): + metadata.append({ + "id": usage_key, + "def_key": def_key, + "display_name": display_name, + "has_unpublished_changes": lib_bundle.does_definition_have_unpublished_changes(def_key), + }) + + return [ + LibraryXBlockMetadata( + usage_key=item['id'], + def_key=item['def_key'], + display_name=item['display_name'], + has_unpublished_changes=item['has_unpublished_changes'], + ) + for item in metadata + ] def _lookup_usage_key(usage_key): diff --git a/openedx/core/djangoapps/content_libraries/libraries_index.py b/openedx/core/djangoapps/content_libraries/libraries_index.py index d0f0e32d2e..ec3ba6db2c 100644 --- a/openedx/core/djangoapps/content_libraries/libraries_index.py +++ b/openedx/core/djangoapps/content_libraries/libraries_index.py @@ -67,7 +67,7 @@ class SearchIndexerBase(ABC): """ searcher = SearchEngine.get_search_engine(cls.INDEX_NAME) response = searcher.search(doc_type=cls.DOCUMENT_TYPE, field_dictionary=kwargs, size=MAX_SIZE) - return response["results"] + return sorted(response["results"], key=lambda i: i['data']["id"]) @classmethod def get_items(cls, ids, text_search=None): @@ -101,6 +101,7 @@ class SearchIndexerBase(ABC): size=MAX_SIZE )) else: + # This is used only for running tests. TODO: Remove this and use elasticsearch in tests. response = searcher.search( doc_type=cls.DOCUMENT_TYPE, field_dictionary={"id": ids_str}, diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index f9c1fb7d18..14c10ee6a8 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -72,7 +72,7 @@ class LibraryXBlockMetadataSerializer(serializers.Serializer): """ id = serializers.CharField(source="usage_key", read_only=True) def_key = serializers.CharField(read_only=True) - block_type = serializers.CharField(source="def_key.block_type") + block_type = serializers.CharField(source="usage_key.block_type") display_name = serializers.CharField(read_only=True) has_unpublished_changes = serializers.BooleanField(read_only=True) # When creating a new XBlock in a library, the slug becomes the ID part of diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index 12f8bb9ea6..b5f15e8b10 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -221,9 +221,16 @@ class ContentLibrariesRestApiTest(APITestCase): else: return self._api('put', url, {"access_level": access_level}, expect_response) - def _get_library_blocks(self, lib_key, expect_response=200): + def _get_library_blocks(self, lib_key, query_params_dict=None, expect_response=200): """ Get the list of XBlocks in the library """ - return self._api('get', URL_LIB_BLOCKS.format(lib_key=lib_key), None, expect_response) + if query_params_dict is None: + query_params_dict = {} + return self._api( + 'get', + URL_LIB_BLOCKS.format(lib_key=lib_key) + '?' + urlencode(query_params_dict), + None, + expect_response + ) def _add_block_to_library(self, lib_key, block_type, slug, parent_block=None, expect_response=200): """ Add a new XBlock to the library """ 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 cc95e6eda9..b0d406575f 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -89,7 +89,7 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): self._create_library(slug="Invalid Slug!", title="Library with Bad Slug", expect_response=400) @ddt.data(True, False) - @patch("openedx.core.djangoapps.content_libraries.views.LibraryRootPagination.page_size", new=2) + @patch("openedx.core.djangoapps.content_libraries.views.LibraryApiPagination.page_size", new=2) def test_list_library(self, is_indexing_enabled): """ Test the /libraries API and its pagination @@ -247,6 +247,59 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): # fin + @ddt.data(True, False) + @patch("openedx.core.djangoapps.content_libraries.views.LibraryApiPagination.page_size", new=2) + def test_list_library_blocks(self, is_indexing_enabled): + """ + Test the /libraries/{lib_key_str}/blocks API and its pagination + """ + features = settings.FEATURES + features['ENABLE_CONTENT_LIBRARY_INDEX'] = is_indexing_enabled + with override_settings(FEATURES=features): + lib = self._create_library(slug="list_blocks-slug" + str(is_indexing_enabled), title="Library 1") + block1 = self._add_block_to_library(lib["id"], "problem", "problem1") + block2 = self._add_block_to_library(lib["id"], "unit", "unit1") + self._add_block_to_library(lib["id"], "problem", "problem2", parent_block=block2["id"]) + + result = self._get_library_blocks(lib["id"]) + self.assertEqual(len(result), 2) + self.assertIn(block1, result) + + result = self._get_library_blocks(lib["id"], {'pagination': 'true'}) + self.assertEqual(len(result['results']), 2) + self.assertEqual(result['next'], None) + + self._add_block_to_library(lib["id"], "problem", "problem3") + # Test pagination + result = self._get_library_blocks(lib["id"]) + self.assertEqual(len(result), 3) + result = self._get_library_blocks(lib["id"], {'pagination': 'true'}) + self.assertEqual(len(result['results']), 2) + self.assertIn('page=2', result['next']) + self.assertIn('pagination=true', result['next']) + result = self._get_library_blocks(lib["id"], {'pagination': 'true', 'page': '2'}) + self.assertEqual(len(result['results']), 1) + self.assertEqual(result['next'], None) + + @ddt.data(True, False) + def test_library_blocks_filters(self, is_indexing_enabled): + """ + Test the filters in the list libraries API + """ + features = settings.FEATURES + features['ENABLE_CONTENT_LIBRARY_INDEX'] = is_indexing_enabled + with override_settings(FEATURES=features): + 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") + block2 = self._add_block_to_library(lib["id"], "problem", "foo-baz") + block3 = 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"], {'text_search': 'Foo'})), 2) + self.assertEqual(len(self._get_library_blocks(lib["id"], {'text_search': 'Display'})), 1) + def test_library_blocks_with_hierarchy(self): """ Test library blocks with children diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index 18126d3098..f269a1435f 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -66,13 +66,31 @@ def convert_exceptions(fn): return wrapped_fn -class LibraryRootPagination(PageNumberPagination): +class LibraryApiPagination(PageNumberPagination): """ Paginates over ContentLibraryMetadata objects. """ page_size = 50 page_size_query_param = 'page_size' + apidoc_params = [ + apidocs.query_parameter( + 'pagination', + bool, + description="Enables paginated schema", + ), + apidocs.query_parameter( + 'page', + int, + description="Page number of result. Defaults to 1", + ), + apidocs.query_parameter( + 'page_size', + int, + description="Page size of the result. Defaults to 50", + ), + ] + @view_auth_classes() class LibraryRootView(APIView): @@ -82,21 +100,7 @@ class LibraryRootView(APIView): @apidocs.schema( parameters=[ - apidocs.query_parameter( - 'pagination', - bool, - description="Enables paginated schema", - ), - apidocs.query_parameter( - 'page', - int, - description="Page number of result. Defaults to 1", - ), - apidocs.query_parameter( - 'page_size', - int, - description="Page size of the result. Defaults to 50", - ), + *LibraryApiPagination.apidoc_params, apidocs.query_parameter( 'org', str, @@ -116,7 +120,7 @@ class LibraryRootView(APIView): org = request.query_params.get('org', None) text_search = request.query_params.get('text_search', None) - paginator = LibraryRootPagination() + paginator = LibraryApiPagination() queryset = api.get_libraries_for_user(request.user, org=org) if text_search: result = api.get_metadata_from_index(queryset, text_search=text_search) @@ -407,14 +411,35 @@ class LibraryBlocksView(APIView): """ Views to work with XBlocks in a specific content library. """ + @apidocs.schema( + parameters=[ + *LibraryApiPagination.apidoc_params, + apidocs.query_parameter( + 'text_search', + str, + description="The string used to filter libraries by searching in title, id, org, or description", + ), + ], + ) @convert_exceptions def get(self, request, lib_key_str): """ Get the list of all top-level blocks in this content library """ key = LibraryLocatorV2.from_string(lib_key_str) + text_search = request.query_params.get('text_search', None) + api.require_permission_for_library_key(key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) - result = api.get_library_blocks(key) + result = api.get_library_blocks(key, text_search=text_search) + + # Verify `pagination` param to maintain compatibility with older + # non pagination-aware clients + if request.GET.get('pagination', 'false').lower() == 'true': + paginator = LibraryApiPagination() + result = paginator.paginate_queryset(result, request) + serializer = LibraryXBlockMetadataSerializer(result, many=True) + return paginator.get_paginated_response(serializer.data) + return Response(LibraryXBlockMetadataSerializer(result, many=True).data) @convert_exceptions