diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 46c687c721..790942300d 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -36,6 +36,7 @@ for these in most other learning contexts then those methods could be promoted to the core XBlock API and made generic. """ from uuid import UUID +from datetime import datetime import logging import attr @@ -45,6 +46,7 @@ from django.core.exceptions import PermissionDenied from django.core.validators import validate_unicode_slug from django.db import IntegrityError from django.utils.translation import ugettext as _ +from elasticsearch.exceptions import ConnectionError as ElasticConnectionError from lxml import etree from opaque_keys.edx.keys import LearningContextKey from opaque_keys.edx.locator import BundleDefinitionLocator, LibraryLocatorV2, LibraryUsageLocatorV2 @@ -56,6 +58,7 @@ 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, LibraryNotIndexedException from openedx.core.djangoapps.content_libraries.models import ContentLibrary, ContentLibraryPermission from openedx.core.djangoapps.content_libraries.signals import ( CONTENT_LIBRARY_CREATED, @@ -90,6 +93,12 @@ log = logging.getLogger(__name__) ContentLibraryNotFound = ContentLibrary.DoesNotExist +class ServerError(APIException): + """ A 500 server error """ + status_code = 500 + default_detail = "Error occurred in the server. Please contact support with details of this error" + + class ContentLibraryBlockNotFound(XBlockNotFoundError): """ XBlock not found in the content library """ @@ -121,7 +130,9 @@ class ContentLibraryMetadata: bundle_uuid = attr.ib(type=UUID) title = attr.ib("") description = attr.ib("") + num_blocks = attr.ib(0) version = attr.ib(0) + last_published = attr.ib(default=None, type=datetime) has_unpublished_changes = attr.ib(False) # has_unpublished_deletes will be true when the draft version of the library's bundle # contains deletes of any XBlocks that were in the most recently published version @@ -214,16 +225,44 @@ class AccessLevel: NO_ACCESS = None -def list_libraries_for_user(user): +def get_libraries_for_user(user): """ - Lists up to 50 content libraries that the user has permission to view. - - This method makes at least one HTTP call per library so should only be used - for development until we have something more efficient. + Return content libraries that the user has permission to view. """ qs = ContentLibrary.objects.all() - filtered_qs = permissions.perms[permissions.CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, qs) - return [get_library(ref.library_key) for ref in filtered_qs[:50]] + return permissions.perms[permissions.CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, qs) + + +def get_metadata_from_index(queryset): + """ + Take a list of ContentLibrary objects and return metadata stored in + ContentLibraryIndex. + """ + if not ContentLibraryIndexer.indexing_is_enabled(): + raise NotImplementedError("Library indexing needs to be enabled for this API to work") + library_keys = [lib.library_key for lib in queryset] + try: + metadata = ContentLibraryIndexer.get_libraries(library_keys) + libraries = [ + ContentLibraryMetadata( + key=key, + bundle_uuid=metadata[i]['uuid'], + title=metadata[i]['title'], + description=metadata[i]['description'], + num_blocks=metadata[i]['num_blocks'], + version=metadata[i]['version'], + last_published=metadata[i]['last_published'], + allow_public_learning=queryset[i].allow_public_learning, + allow_public_read=queryset[i].allow_public_read, + has_unpublished_changes=metadata[i]['has_unpublished_changes'], + has_unpublished_deletes=metadata[i]['has_unpublished_deletes'], + ) + for i, key in enumerate(library_keys) + ] + return libraries + except (LibraryNotIndexedException, KeyError) as e: + log.exception(e) + raise ServerError("Libraries have missing or invalid indexes, and need to be updated.") def require_permission_for_library_key(library_key, user, permission): @@ -253,13 +292,17 @@ def get_library(library_key): ref = ContentLibrary.objects.get_by_key(library_key) bundle_metadata = get_bundle(ref.bundle_uuid) lib_bundle = LibraryBundle(library_key, ref.bundle_uuid, draft_name=DRAFT_NAME) + num_blocks = len(lib_bundle.get_top_level_usages()) + last_published = lib_bundle.get_last_published_time() (has_unpublished_changes, has_unpublished_deletes) = lib_bundle.has_changes() return ContentLibraryMetadata( key=library_key, bundle_uuid=ref.bundle_uuid, title=bundle_metadata.title, description=bundle_metadata.description, + num_blocks=num_blocks, version=bundle_metadata.latest_version, + last_published=last_published, allow_public_learning=ref.allow_public_learning, allow_public_read=ref.allow_public_read, has_unpublished_changes=has_unpublished_changes, @@ -313,7 +356,9 @@ def create_library(collection_uuid, org, slug, title, description, allow_public_ bundle_uuid=bundle.uuid, title=title, description=description, + num_blocks=0, version=0, + last_published=None, allow_public_learning=ref.allow_public_learning, allow_public_read=ref.allow_public_read, ) diff --git a/openedx/core/djangoapps/content_libraries/libraries_index.py b/openedx/core/djangoapps/content_libraries/libraries_index.py index d6e76a5fe5..5a9c7a7a55 100644 --- a/openedx/core/djangoapps/content_libraries/libraries_index.py +++ b/openedx/core/djangoapps/content_libraries/libraries_index.py @@ -51,7 +51,11 @@ class ContentLibraryIndexer: for library_key in library_keys: 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() + num_blocks = len(lib_bundle.get_top_level_usages()) + last_published = lib_bundle.get_last_published_time() + last_published_str = None + if last_published: + last_published_str = last_published.strftime('%Y-%m-%dT%H:%M:%SZ') (has_unpublished_changes, has_unpublished_deletes) = lib_bundle.has_changes() bundle_metadata = get_bundle(ref.bundle_uuid) @@ -61,8 +65,9 @@ class ContentLibraryIndexer: "uuid": str(bundle_metadata.uuid), "title": bundle_metadata.title, "description": bundle_metadata.description, + "num_blocks": num_blocks, "version": bundle_metadata.latest_version, - "num_blocks": len(usages), + "last_published": last_published_str, "has_unpublished_changes": has_unpublished_changes, "has_unpublished_deletes": has_unpublished_deletes, } diff --git a/openedx/core/djangoapps/content_libraries/library_bundle.py b/openedx/core/djangoapps/content_libraries/library_bundle.py index 27570a2ff8..8374d18503 100644 --- a/openedx/core/djangoapps/content_libraries/library_bundle.py +++ b/openedx/core/djangoapps/content_libraries/library_bundle.py @@ -2,6 +2,7 @@ Helper code for working with Blockstore bundles that contain OLX """ +import dateutil.parser import logging from django.utils.lru_cache import lru_cache @@ -349,6 +350,23 @@ class LibraryBundle(object): if f.path.startswith(path_prefix) ] + def get_last_published_time(self): + """ + Return the timestamp when the current library was last published. If the + current draft has never been published, return 0. + """ + cache_key = ("last_published_time", ) + usages_found = self.cache.get(cache_key) + if usages_found is not None: + return usages_found + version = get_bundle_version_number(self.bundle_uuid) + if version == 0: + return None + created_at_str = blockstore_api.get_bundle_version(self.bundle_uuid, version)['snapshot']['created_at'] + last_published_time = dateutil.parser.parse(created_at_str) + self.cache.set(cache_key, last_published_time) + return last_published_time + @staticmethod def olx_prefix(definition_key): """ diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index e13f57c083..f9c1fb7d18 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -8,6 +8,8 @@ from rest_framework import serializers from openedx.core.djangoapps.content_libraries.models import ContentLibraryPermission from openedx.core.lib import blockstore_api +DATETIME_FORMAT = '%Y-%m-%dT%H:%M:%SZ' + class ContentLibraryMetadataSerializer(serializers.Serializer): """ @@ -25,7 +27,9 @@ class ContentLibraryMetadataSerializer(serializers.Serializer): collection_uuid = serializers.UUIDField(format='hex_verbose', write_only=True) title = serializers.CharField() description = serializers.CharField(allow_blank=True) + num_blocks = serializers.IntegerField(read_only=True) version = serializers.IntegerField(read_only=True) + last_published = serializers.DateTimeField(format=DATETIME_FORMAT, read_only=True) allow_public_learning = serializers.BooleanField(default=False) allow_public_read = serializers.BooleanField(default=False) has_unpublished_changes = serializers.BooleanField(read_only=True) diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index 490ff4052b..4799028fda 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -4,6 +4,7 @@ Tests for Blockstore-based Content Libraries """ from contextlib import contextmanager from io import BytesIO +from urllib.parse import urlencode import unittest from django.conf import settings @@ -18,6 +19,7 @@ from openedx.core.lib import blockstore_api # backwards-incompatible changes like changed URLs. URL_PREFIX = '/api/libraries/v2/' URL_LIB_CREATE = URL_PREFIX +URL_LIB_LIST = URL_PREFIX + '?{query_params}' URL_LIB_DETAIL = URL_PREFIX + '{lib_key}/' # Get data about a library, update or delete library URL_LIB_BLOCK_TYPES = URL_LIB_DETAIL + 'block_types/' # Get the list of XBlock types that can be added to this library URL_LIB_LINKS = URL_LIB_DETAIL + 'links/' # Get the list of links in this library, or add a new one @@ -132,6 +134,12 @@ class ContentLibrariesRestApiTest(APITestCase): "collection_uuid": str(self.collection.uuid), }, expect_response) + def _list_libraries(self, query_params_dict=None, expect_response=200): + """ List libraries """ + if query_params_dict is None: + query_params_dict = {} + return self._api('get', URL_LIB_LIST.format(query_params=urlencode(query_params_dict)), None, expect_response) + def _get_library(self, lib_key, expect_response=200): """ Get a library """ return self._api('get', URL_LIB_DETAIL.format(lib_key=lib_key), 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 4cc695a1ad..3d389a6bc3 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -6,6 +6,7 @@ import unittest from uuid import UUID from django.contrib.auth.models import Group +from mock import patch from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest from openedx.core.djangoapps.content_libraries.api import BlockLimitReachedError @@ -81,6 +82,47 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): self._create_library(slug="Invalid Slug!", title="Library with Bad Slug", expect_response=400) + @patch("openedx.core.djangoapps.content_libraries.views.LibraryRootPagination.page_size", new=2) + @override_settings(SEARCH_ENGINE="search.tests.mock_search_engine.MockSearchEngine") + def test_list_library(self, is_indexing_enabled): + """ + Test the /libraries API and its pagination + """ + with override_settings(FEATURES={**settings.FEATURES, 'ENABLE_CONTENT_LIBRARY_INDEX': is_indexing_enabled}): + lib1 = self._create_library(slug="some-slug-1", title="Existing Library") + lib2 = self._create_library(slug="some-slug-2", title="Existing Library") + if not is_indexing_enabled: + lib1['num_blocks'] = lib2['num_blocks'] = None + lib1['last_published'] = lib2['last_published'] = None + lib1['has_unpublished_changes'] = lib2['has_unpublished_changes'] = None + lib1['has_unpublished_deletes'] = lib2['has_unpublished_deletes'] = None + + result = self._list_libraries() + self.assertEqual(len(result), 2) + self.assertEqual(result[0], lib1) + result = self._list_libraries({'pagination': 'true'}) + self.assertEqual(len(result['results']), 2) + self.assertEqual(result['next'], None) + + # Create another library which causes number of libraries to exceed the page size + self._create_library(slug="some-slug-3", title="Existing Library") + + # Verify that if `pagination` param isn't sent, API still honors the max page size. + # This is for maintaining compatibility with older non pagination-aware clients. + result = self._list_libraries() + self.assertEqual(len(result), 2) + + # Pagination enabled: + # Verify total elements and valid 'next' in page 1 + result = self._list_libraries({'pagination': 'true'}) + self.assertEqual(len(result['results']), 2) + self.assertIn('page=2', result['next']) + self.assertIn('pagination=true', result['next']) + # Verify total elements and null 'next' in page 2 + result = self._list_libraries({'pagination': 'true', 'page': '2'}) + self.assertEqual(len(result['results']), 1) + self.assertEqual(result['next'], None) + # General Content Library XBlock tests: def test_library_blocks(self): diff --git a/openedx/core/djangoapps/content_libraries/tests/test_libraries_index.py b/openedx/core/djangoapps/content_libraries/tests/test_libraries_index.py index 7110ccf374..2fc573d9fc 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_libraries_index.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_libraries_index.py @@ -41,8 +41,9 @@ class ContentLibraryIndexerIndexer(ContentLibrariesRestApiTest): self.assertEqual(response['title'], result['title']) self.assertEqual(response['description'], result['description']) self.assertEqual(response['uuid'], result['bundle_uuid']) - self.assertEqual(response['version'], result['version']) self.assertEqual(response['num_blocks'], 0) + self.assertEqual(response['version'], result['version']) + self.assertEqual(response['last_published'], None) self.assertEqual(response['has_unpublished_changes'], False) self.assertEqual(response['has_unpublished_deletes'], False) @@ -75,8 +76,9 @@ class ContentLibraryIndexerIndexer(ContentLibrariesRestApiTest): self.assertEqual(response['title'], "New Title") self.assertEqual(response['description'], "New Title") self.assertEqual(response['uuid'], lib['bundle_uuid']) - self.assertEqual(response['version'], lib['version']) self.assertEqual(response['num_blocks'], 0) + self.assertEqual(response['version'], lib['version']) + self.assertEqual(response['last_published'], None) self.assertEqual(response['has_unpublished_changes'], False) self.assertEqual(response['has_unpublished_deletes'], False) @@ -92,10 +94,12 @@ class ContentLibraryIndexerIndexer(ContentLibrariesRestApiTest): """ Commit library changes, and verify that there are no uncommited changes anymore """ + last_published = ContentLibraryIndexer.get_libraries([library_key])[0]['last_published'] self._commit_library_changes(str(library_key)) response = ContentLibraryIndexer.get_libraries([library_key])[0] self.assertEqual(response['has_unpublished_changes'], False) self.assertEqual(response['has_unpublished_deletes'], False) + self.assertGreaterEqual(response['last_published'], last_published) return response def verify_uncommitted_libraries(library_key, has_unpublished_changes, has_unpublished_deletes): @@ -113,6 +117,7 @@ class ContentLibraryIndexerIndexer(ContentLibrariesRestApiTest): # Verify uncommitted new blocks block = self._add_block_to_library(lib['id'], "problem", "problem1") response = verify_uncommitted_libraries(library_key, True, False) + self.assertEqual(response['last_published'], None) self.assertEqual(response['num_blocks'], 1) # Verify committed new blocks self._commit_library_changes(lib['id']) diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index 7a1a41e565..99a692c461 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -7,10 +7,12 @@ import logging from django.contrib.auth import get_user_model from django.contrib.auth.models import Group from django.shortcuts import get_object_or_404 +import edx_api_doc_tools as apidocs from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 from organizations.models import Organization from rest_framework import status from rest_framework.exceptions import NotFound, PermissionDenied, ValidationError +from rest_framework.pagination import PageNumberPagination from rest_framework.parsers import MultiPartParser from rest_framework.response import Response from rest_framework.views import APIView @@ -61,20 +63,53 @@ def convert_exceptions(fn): return wrapped_fn +class LibraryRootPagination(PageNumberPagination): + """ + Paginates over ContentLibraryMetadata objects. + """ + page_size = 50 + page_size_query_param = 'page_size' + + @view_auth_classes() class LibraryRootView(APIView): """ Views to list, search for, and create content libraries. """ + @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", + ), + ], + ) def get(self, request): """ - Return a list of all content libraries that the user has permission to - view. This is a temporary view for development and returns at most 50 - libraries. + Return a list of all content libraries that the user has permission to view. """ - result = api.list_libraries_for_user(request.user) - return Response(ContentLibraryMetadataSerializer(result, many=True).data) + paginator = LibraryRootPagination() + queryset = api.get_libraries_for_user(request.user) + paginated_qs = paginator.paginate_queryset(queryset, request) + result = api.get_metadata_from_index(paginated_qs) + serializer = ContentLibraryMetadataSerializer(result, many=True) + # Verify `pagination` param to maintain compatibility with older + # non pagination-aware clients + if request.GET.get('pagination', 'false').lower() == 'true': + return paginator.get_paginated_response(serializer.data) + return Response(serializer.data) def post(self, request): """ diff --git a/openedx/core/lib/blockstore_api/__init__.py b/openedx/core/lib/blockstore_api/__init__.py index 8b6de205c6..ae40084119 100644 --- a/openedx/core/lib/blockstore_api/__init__.py +++ b/openedx/core/lib/blockstore_api/__init__.py @@ -38,6 +38,7 @@ from .methods import ( get_bundle_files_dict, get_bundle_file_metadata, get_bundle_file_data, + get_bundle_version, get_bundle_version_files, # Links: get_bundle_links, diff --git a/openedx/core/lib/blockstore_api/methods.py b/openedx/core/lib/blockstore_api/methods.py index a9735aace2..9520f16c62 100644 --- a/openedx/core/lib/blockstore_api/methods.py +++ b/openedx/core/lib/blockstore_api/methods.py @@ -248,14 +248,23 @@ def delete_draft(draft_uuid): api_request('delete', api_url('drafts', str(draft_uuid))) +def get_bundle_version(bundle_uuid, version_number): + """ + Get the details of the specified bundle version + """ + if version_number == 0: + return None + version_url = api_url('bundle_versions', str(bundle_uuid) + ',' + str(version_number)) + return api_request('get', version_url) + + def get_bundle_version_files(bundle_uuid, version_number): """ Get a list of the files in the specified bundle version """ if version_number == 0: return [] - version_url = api_url('bundle_versions', str(bundle_uuid) + ',' + str(version_number)) - version_info = api_request('get', version_url) + version_info = get_bundle_version(bundle_uuid, version_number) return [BundleFile(path=path, **file_metadata) for path, file_metadata in version_info["snapshot"]["files"].items()] @@ -265,8 +274,7 @@ def get_bundle_version_links(bundle_uuid, version_number): """ if version_number == 0: return {} - version_url = api_url('bundle_versions', str(bundle_uuid) + ',' + str(version_number)) - version_info = api_request('get', version_url) + version_info = get_bundle_version(bundle_uuid, version_number) return { name: LinkDetails( name=name,