Add type field/constraint to to libraries v2. (#24861)

This commit is contained in:
Fox Piacenti
2020-09-28 09:58:27 -05:00
committed by GitHub
parent f29e418264
commit 2c15bbba2d
10 changed files with 234 additions and 22 deletions

View File

@@ -56,7 +56,7 @@ from xblock.core import XBlock
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.constants import DRAFT_NAME, COMPLEX
from openedx.core.djangoapps.content_libraries.library_bundle import LibraryBundle
from openedx.core.djangoapps.content_libraries.libraries_index import ContentLibraryIndexer, LibraryBlockIndexer
from openedx.core.djangoapps.content_libraries.models import ContentLibrary, ContentLibraryPermission
@@ -110,6 +110,10 @@ class BlockLimitReachedError(Exception):
""" Maximum number of allowed XBlocks in the library reached """
class IncompatibleTypesError(Exception):
""" Library type constraint violated """
class InvalidNameError(ValueError):
""" The specified name/identifier is not valid """
@@ -131,6 +135,7 @@ class ContentLibraryMetadata:
description = attr.ib("")
num_blocks = attr.ib(0)
version = attr.ib(0)
type = attr.ib(default=COMPLEX)
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
@@ -224,14 +229,16 @@ class AccessLevel:
NO_ACCESS = None
def get_libraries_for_user(user, org=None):
def get_libraries_for_user(user, org=None, library_type=None):
"""
Return content libraries that the user has permission to view.
"""
filter_kwargs = {}
if org:
qs = ContentLibrary.objects.filter(org__short_name=org)
else:
qs = ContentLibrary.objects.all()
filter_kwargs['org__short_name'] = org
if library_type:
filter_kwargs['type'] = library_type
qs = ContentLibrary.objects.filter(**filter_kwargs)
return permissions.perms[permissions.CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, qs)
@@ -291,6 +298,7 @@ def get_metadata_from_index(queryset, text_search=None):
key=lib.library_key,
bundle_uuid=metadata[i]['uuid'],
title=metadata[i]['title'],
type=lib.type,
description=metadata[i]['description'],
version=metadata[i]['version'],
allow_public_learning=queryset[i].allow_public_learning,
@@ -340,6 +348,7 @@ def get_library(library_key):
key=library_key,
bundle_uuid=ref.bundle_uuid,
title=bundle_metadata.title,
type=ref.type,
description=bundle_metadata.description,
num_blocks=num_blocks,
version=bundle_metadata.latest_version,
@@ -351,7 +360,9 @@ def get_library(library_key):
)
def create_library(collection_uuid, org, slug, title, description, allow_public_learning, allow_public_read):
def create_library(
collection_uuid, library_type, org, slug, title, description, allow_public_learning, allow_public_read,
):
"""
Create a new content library.
@@ -384,6 +395,7 @@ def create_library(collection_uuid, org, slug, title, description, allow_public_
ref = ContentLibrary.objects.create(
org=org,
slug=slug,
type=library_type,
bundle_uuid=bundle.uuid,
allow_public_learning=allow_public_learning,
allow_public_read=allow_public_read,
@@ -396,6 +408,7 @@ def create_library(collection_uuid, org, slug, title, description, allow_public_
key=ref.library_key,
bundle_uuid=bundle.uuid,
title=title,
type=library_type,
description=description,
num_blocks=0,
version=0,
@@ -476,6 +489,7 @@ def update_library(
description=None,
allow_public_learning=None,
allow_public_read=None,
library_type=None,
):
"""
Update a library's title or description.
@@ -491,6 +505,23 @@ def update_library(
changed = True
if allow_public_read is not None:
ref.allow_public_read = allow_public_read
changed = True
if library_type is not None:
if library_type != COMPLEX:
for block in get_library_blocks(library_key):
if block.usage_key.block_type != library_type:
raise IncompatibleTypesError(
_(
'You can only set a library to {library_type} if all existing blocks are of that type. '
'Found incompatible block {block_id} with type {block_type}.'
).format(
library_type=library_type,
block_type=block.usage_key.block_type,
block_id=block.usage_key.block_id,
),
)
ref.type = library_type
changed = True
if changed:
ref.save()
@@ -552,7 +583,7 @@ def get_library_blocks(library_key, text_search=None):
for item in LibraryBlockIndexer.get_items(filter_terms=filter_terms, text_search=text_search)
if item is not None
]
except (ElasticConnectionError) as e:
except ElasticConnectionError as e:
log.exception(e)
# If indexing is disabled, or connection to elastic failed
@@ -668,6 +699,13 @@ def create_library_block(library_key, block_type, definition_id):
"""
assert isinstance(library_key, LibraryLocatorV2)
ref = ContentLibrary.objects.get_by_key(library_key)
if ref.type != COMPLEX:
if block_type != ref.type:
raise IncompatibleTypesError(
_('Block type "{block_type}" is not compatible with library type "{library_type}".').format(
block_type=block_type, library_type=ref.type,
)
)
lib_bundle = LibraryBundle(library_key, ref.bundle_uuid, draft_name=DRAFT_NAME)
# Total number of blocks should not exceed the maximum allowed
total_blocks = len(lib_bundle.get_top_level_usages())
@@ -852,8 +890,7 @@ def delete_library_block_static_asset_file(usage_key, file_name):
def get_allowed_block_types(library_key): # pylint: disable=unused-argument
"""
Get a list of XBlock types that can be added to the specified content
library. For now, the result is the same regardless of which library is
specified, but that may change in the future.
library.
"""
# This import breaks in the LMS so keep it here. The LMS doesn't generally
# use content libraries APIs directly but some tests may want to use them to
@@ -862,6 +899,10 @@ def get_allowed_block_types(library_key): # pylint: disable=unused-argument
# TODO: return support status and template options
# See cms/djangoapps/contentstore/views/component.py
block_types = sorted(name for name, class_ in XBlock.load_classes())
lib = get_library(library_key)
if lib.type != COMPLEX:
# Problem and Video libraries only permit XBlocks of the same name.
block_types = (name for name in block_types if name == lib.type)
info = []
for block_type in block_types:
display_name = xblock_type_display_name(block_type, None)

View File

@@ -1,5 +1,16 @@
""" Constants used for the content libraries. """
from django.utils.translation import ugettext_lazy as _
# This API is only used in Studio, so we always work with this draft of any
# ./api.py and ./views.py are only used in Studio, so we always work with this draft of any
# content library bundle:
DRAFT_NAME = 'studio_draft'
VIDEO = 'video'
COMPLEX = 'complex'
PROBLEM = 'problem'
LIBRARY_TYPES = (
(VIDEO, _('Video')),
(COMPLEX, _('Complex')),
(PROBLEM, _('Problem')),
)

View File

@@ -305,7 +305,7 @@ def index_block(sender, usage_key, **kwargs): # pylint: disable=unused-argument
if LibraryBlockIndexer.indexing_is_enabled():
try:
LibraryBlockIndexer.index_items([usage_key])
except ConnectionError as e:
except ElasticConnectionError as e:
log.exception(e)
@@ -317,5 +317,5 @@ def remove_block_index(sender, usage_key, **kwargs): # pylint: disable=unused-a
if LibraryBlockIndexer.indexing_is_enabled():
try:
LibraryBlockIndexer.remove_items([usage_key])
except ConnectionError as e:
except ElasticConnectionError as e:
log.exception(e)

View File

@@ -0,0 +1,18 @@
# Generated by Django 2.2.15 on 2020-08-25 23:11
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
('content_libraries', '0002_group_permissions'),
]
operations = [
migrations.AddField(
model_name='contentlibrary',
name='type',
field=models.CharField(choices=[('video', 'Video'), ('complex', 'Complex'), ('problem', 'Problem')], default='complex', max_length=25),
),
]

View File

@@ -7,6 +7,7 @@ from django.core.exceptions import ValidationError
from django.db import models
from django.utils.translation import ugettext_lazy as _
from opaque_keys.edx.locator import LibraryLocatorV2
from openedx.core.djangoapps.content_libraries.constants import LIBRARY_TYPES, COMPLEX
from organizations.models import Organization
import six
@@ -45,6 +46,7 @@ class ContentLibrary(models.Model):
org = models.ForeignKey(Organization, on_delete=models.PROTECT, null=False)
slug = models.SlugField(allow_unicode=True)
bundle_uuid = models.UUIDField(unique=True, null=False)
type = models.CharField(max_length=25, default=COMPLEX, choices=LIBRARY_TYPES)
# How is this library going to be used?
allow_public_learning = models.BooleanField(

View File

@@ -5,6 +5,7 @@ Serializers for the content libraries REST API
from django.core.validators import validate_unicode_slug
from rest_framework import serializers
from openedx.core.djangoapps.content_libraries.constants import LIBRARY_TYPES, COMPLEX
from openedx.core.djangoapps.content_libraries.models import ContentLibraryPermission
from openedx.core.lib import blockstore_api
@@ -21,6 +22,7 @@ class ContentLibraryMetadataSerializer(serializers.Serializer):
# begins with 'lib:'. (The numeric ID of the ContentLibrary object in MySQL
# is not exposed via this API.)
id = serializers.CharField(source="key", read_only=True)
type = serializers.ChoiceField(choices=LIBRARY_TYPES, default=COMPLEX)
org = serializers.SlugField(source="key.org")
slug = serializers.CharField(source="key.slug", validators=(validate_unicode_slug, ))
bundle_uuid = serializers.UUIDField(format='hex_verbose', read_only=True)
@@ -45,6 +47,7 @@ class ContentLibraryUpdateSerializer(serializers.Serializer):
description = serializers.CharField()
allow_public_learning = serializers.BooleanField()
allow_public_read = serializers.BooleanField()
type = serializers.ChoiceField(choices=LIBRARY_TYPES)
class ContentLibraryPermissionLevelSerializer(serializers.Serializer):
@@ -75,6 +78,15 @@ class ContentLibraryPermissionSerializer(ContentLibraryPermissionLevelSerializer
group_name = serializers.CharField(source="group.name", allow_null=True, allow_blank=False, default=None)
class ContentLibraryFilterSerializer(serializers.Serializer):
"""
Serializer for filtering library listings.
"""
text_search = serializers.CharField(default=None, required=False)
org = serializers.CharField(default=None, required=False)
type = serializers.ChoiceField(choices=LIBRARY_TYPES, default=None, required=False)
class LibraryXBlockMetadataSerializer(serializers.Serializer):
"""
Serializer for LibraryXBlockMetadata

View File

@@ -16,6 +16,7 @@ from search.search_engine_base import SearchEngine
from student.tests.factories import UserFactory
from openedx.core.djangoapps.content_libraries.libraries_index import MAX_SIZE
from openedx.core.djangoapps.content_libraries.constants import COMPLEX
from openedx.core.djangolib.testing.utils import skip_unless_cms
from openedx.core.lib import blockstore_api
@@ -166,7 +167,7 @@ class ContentLibrariesRestApiTest(APITestCase):
yield
self.client = old_client # pylint: disable=attribute-defined-outside-init
def _create_library(self, slug, title, description="", org=None, expect_response=200):
def _create_library(self, slug, title, description="", org=None, library_type=COMPLEX, expect_response=200):
""" Create a library """
if org is None:
org = self.organization.short_name
@@ -175,6 +176,7 @@ class ContentLibrariesRestApiTest(APITestCase):
"slug": slug,
"title": title,
"description": description,
"type": library_type,
"collection_uuid": str(self.collection.uuid),
}, expect_response)
@@ -253,6 +255,10 @@ class ContentLibrariesRestApiTest(APITestCase):
else:
return self._api('put', url, {"access_level": access_level}, expect_response)
def _get_library_block_types(self, lib_key, expect_response=200):
""" Get the list of permitted XBlocks for this library """
return self._api('get', URL_LIB_BLOCK_TYPES.format(lib_key=lib_key), None, expect_response)
def _get_library_blocks(self, lib_key, query_params_dict=None, expect_response=200):
""" Get the list of XBlocks in the library """
if query_params_dict is None:

View File

@@ -12,6 +12,7 @@ from mock import patch
from organizations.models import Organization
from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest, elasticsearch_test
from openedx.core.djangoapps.content_libraries.constants import VIDEO, COMPLEX, PROBLEM
from student.tests.factories import UserFactory
@@ -54,6 +55,7 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest):
"title": "A Test Library",
"description": "Just Testing",
"version": 0,
"type": COMPLEX,
"has_unpublished_changes": False,
"has_unpublished_deletes": False,
}
@@ -76,6 +78,59 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest):
self._get_library(lib["id"], expect_response=404)
self._delete_library(lib["id"], expect_response=404)
@ddt.data(VIDEO, PROBLEM, COMPLEX)
def test_library_alternative_type(self, target_type):
"""
Create a library with a specific type
"""
lib = self._create_library(
slug="some-slug", title="Video Library", description="Test Video Library", library_type=target_type,
)
expected_data = {
"id": "lib:CL-TEST:some-slug",
"org": "CL-TEST",
"slug": "some-slug",
"title": "Video Library",
"type": target_type,
"description": "Test Video Library",
"version": 0,
"has_unpublished_changes": False,
"has_unpublished_deletes": False,
}
self.assertDictContainsEntries(lib, expected_data)
# Need to use a different slug each time here. Seems to be a race condition on test cleanup that will break things
# otherwise.
@ddt.data(
('to-video-fail', COMPLEX, VIDEO, (("problem", "problemA"),), 400),
('to-video-empty', COMPLEX, VIDEO, tuple(), 200),
('to-problem', COMPLEX, PROBLEM, (("problem", "problemB"),), 200),
('to-problem-fail', COMPLEX, PROBLEM, (("video", "videoA"),), 400),
('to-problem-empty', COMPLEX, PROBLEM, tuple(), 200),
('to-complex-from-video', VIDEO, COMPLEX, (("video", "videoB"),), 200),
('to-complex-from-problem', PROBLEM, COMPLEX, (("problem", "problemC"),), 200),
('to-complex-from-problem-empty', PROBLEM, COMPLEX, tuple(), 200),
('to-problem-from-video-empty', PROBLEM, VIDEO, tuple(), 200),
)
@ddt.unpack
def test_library_update_type_conversion(self, slug, start_type, target_type, xblock_specs, expect_response):
"""
Test conversion of one library type to another. Restricts options based on type/block matching.
"""
lib = self._create_library(
slug=slug, title="A Test Library", description="Just Testing", library_type=start_type,
)
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)
if expect_response == 200:
self.assertEqual(result['type'], target_type)
self.assertIn('type', result)
else:
lib = self._get_library(lib['id'])
self.assertEqual(lib['type'], start_type)
def test_library_validation(self):
"""
You can't create a library with the same slug as an existing library,
@@ -133,24 +188,30 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest):
Test the filters in the list libraries API
"""
with override_settings(FEATURES={**settings.FEATURES, 'ENABLE_CONTENT_LIBRARY_INDEX': is_indexing_enabled}):
self._create_library(slug="test-lib1", title="Foo", description="Bar")
self._create_library(slug="test-lib1", title="Foo", description="Bar", library_type=VIDEO)
self._create_library(slug="test-lib2", title="Library-Title-2", description="Bar2")
self._create_library(slug="l3", title="Library-Title-3", description="Description")
self._create_library(slug="l3", title="Library-Title-3", description="Description", library_type=VIDEO)
Organization.objects.get_or_create(
short_name="org-test",
defaults={"name": "Content Libraries Tachyon Exploration & Survey Team"},
)
self._create_library(slug="l4", title="Library-Title-4", description="Library-Description", org='org-test')
self._create_library(
slug="l4", title="Library-Title-4", description="Library-Description", org='org-test',
library_type=VIDEO,
)
self._create_library(slug="l5", title="Library-Title-5", description="Library-Description", org='org-test')
self.assertEqual(len(self._list_libraries()), 5)
self.assertEqual(len(self._list_libraries({'org': 'org-test'})), 2)
self.assertEqual(len(self._list_libraries({'text_search': 'test-lib'})), 2)
self.assertEqual(len(self._list_libraries({'text_search': 'test-lib', 'type': VIDEO})), 1)
self.assertEqual(len(self._list_libraries({'text_search': 'library-title'})), 4)
self.assertEqual(len(self._list_libraries({'text_search': 'library-title', 'type': VIDEO})), 2)
self.assertEqual(len(self._list_libraries({'text_search': 'bar'})), 2)
self.assertEqual(len(self._list_libraries({'text_search': 'org-tes'})), 2)
self.assertEqual(len(self._list_libraries({'org': 'org-test', 'text_search': 'library-title-4'})), 1)
self.assertEqual(len(self._list_libraries({'type': VIDEO})), 3)
# General Content Library XBlock tests:
@@ -293,6 +354,27 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest):
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)
@ddt.data(
('video-problem', VIDEO, 'problem', 400),
('video-video', VIDEO, 'video', 200),
('problem-problem', PROBLEM, 'problem', 200),
('problem-video', PROBLEM, 'video', 400),
('complex-video', COMPLEX, 'video', 200),
('complex-problem', COMPLEX, 'problem', 200),
)
@ddt.unpack
def test_library_blocks_type_constrained(self, slug, library_type, block_type, expect_response):
"""
Test that type-constrained libraries enforce their constraint when adding an XBlock.
"""
lib = self._create_library(
slug=slug, title="A Test Library", description="Testing XBlocks", library_type=library_type,
)
lib_id = lib["id"]
# Add a 'problem' XBlock to the library:
self._add_block_to_library(lib_id, block_type, 'test-block', expect_response=expect_response)
def test_library_blocks_with_hierarchy(self):
"""
Test library blocks with children
@@ -612,3 +694,21 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest):
self._add_block_to_library(lib_id, "problem", "problem1", expect_response=400)
# Also check that limit applies to child blocks too
self._add_block_to_library(lib_id, "html", "html1", parent_block=block_data['id'], expect_response=400)
@ddt.data(
('complex-types', COMPLEX, False),
('video-types', VIDEO, True),
('problem-types', PROBLEM, True),
)
@ddt.unpack
def test_block_types(self, slug, library_type, constrained):
"""
Test that the permitted block types listing for a library change based on type.
"""
lib = self._create_library(slug=slug, title='Test Block Types', library_type=library_type)
types = self._get_library_block_types(lib['id'])
if constrained:
self.assertEqual(len(types), 1)
self.assertEqual(types[0]['block_type'], library_type)
else:
self.assertGreater(len(types), 1)

View File

@@ -19,6 +19,7 @@ from openedx.core.djangoapps.content_libraries.tests.base import (
URL_BLOCK_METADATA_URL,
)
from openedx.core.djangoapps.content_libraries.tests.user_state_block import UserStateTestBlock
from openedx.core.djangoapps.content_libraries.constants import COMPLEX
from openedx.core.djangoapps.xblock import api as xblock_api
from openedx.core.djangolib.testing.utils import skip_unless_lms, skip_unless_cms
from openedx.core.lib import blockstore_api
@@ -46,6 +47,7 @@ class ContentLibraryContentTestMixin(object):
)
cls.library = library_api.create_library(
collection_uuid=cls.collection.uuid,
library_type=COMPLEX,
org=cls.organization,
slug=cls.__name__,
title=(cls.__name__ + " Test Lib"),
@@ -83,6 +85,7 @@ class ContentLibraryRuntimeTest(ContentLibraryContentTestMixin, TestCase):
slug="idolx",
title=("Identical OLX Test Lib 2"),
description="",
library_type=COMPLEX,
allow_public_learning=True,
allow_public_read=False,
)
@@ -177,6 +180,8 @@ class ContentLibraryXBlockUserStateTest(ContentLibraryContentTestMixin, TestCase
if the library allows direct learning.
"""
multi_db = True
@XBlock.register_temp_plugin(UserStateTestBlock, UserStateTestBlock.BLOCK_TYPE)
def test_default_values(self):
"""

View File

@@ -24,6 +24,7 @@ from openedx.core.djangoapps.content_libraries.serializers import (
ContentLibraryUpdateSerializer,
ContentLibraryPermissionLevelSerializer,
ContentLibraryPermissionSerializer,
ContentLibraryFilterSerializer,
LibraryXBlockCreationSerializer,
LibraryXBlockMetadataSerializer,
LibraryXBlockTypeSerializer,
@@ -119,11 +120,14 @@ class LibraryRootView(APIView):
"""
Return a list of all content libraries that the user has permission to view.
"""
org = request.query_params.get('org', None)
text_search = request.query_params.get('text_search', None)
serializer = ContentLibraryFilterSerializer(data=request.query_params)
serializer.is_valid(raise_exception=True)
org = serializer.validated_data['org']
library_type = serializer.validated_data['type']
text_search = serializer.validated_data['text_search']
paginator = LibraryApiPagination()
queryset = api.get_libraries_for_user(request.user, org=org)
queryset = api.get_libraries_for_user(request.user, org=org, library_type=library_type)
if text_search:
result = api.get_metadata_from_index(queryset, text_search=text_search)
result = paginator.paginate_queryset(result, request)
@@ -147,7 +151,9 @@ class LibraryRootView(APIView):
raise PermissionDenied
serializer = ContentLibraryMetadataSerializer(data=request.data)
serializer.is_valid(raise_exception=True)
data = serializer.validated_data
data = dict(serializer.validated_data)
# Converting this over because using the reserved name 'type' would shadow the built-in definition elsewhere.
data['library_type'] = data.pop('type')
# Get the organization short_name out of the "key.org" pseudo-field that the serializer added:
org_name = data["key"]["org"]
# Move "slug" out of the "key.slug" pseudo-field that the serializer added:
@@ -189,7 +195,13 @@ class LibraryDetailsView(APIView):
api.require_permission_for_library_key(key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY)
serializer = ContentLibraryUpdateSerializer(data=request.data, partial=True)
serializer.is_valid(raise_exception=True)
api.update_library(key, **serializer.validated_data)
data = dict(serializer.validated_data)
if 'type' in data:
data['library_type'] = data.pop('type')
try:
api.update_library(key, **data)
except api.IncompatibleTypesError as err:
raise ValidationError({'type': str(err)})
result = api.get_library(key)
return Response(ContentLibraryMetadataSerializer(result).data)
@@ -509,7 +521,12 @@ class LibraryBlocksView(APIView):
result = api.create_library_block_child(parent_block_usage, **serializer.validated_data)
else:
# Create a new regular top-level block:
result = api.create_library_block(library_key, **serializer.validated_data)
try:
result = api.create_library_block(library_key, **serializer.validated_data)
except api.IncompatibleTypesError as err:
raise ValidationError(
detail={'block_type': str(err)},
)
return Response(LibraryXBlockMetadataSerializer(result).data)