From 5bf94226fcbb778a28d63bb11ae10c43061f2a8b Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Thu, 29 Aug 2024 08:49:27 +0300 Subject: [PATCH] refactor: Use convert_exceptions + update tests --- .../rest_api/v1/tests/test_views.py | 44 +++++++++++++++---- .../collections/rest_api/v1/views.py | 30 ++++++------- .../djangoapps/content_libraries/utils.py | 44 +++++++++++++++++++ .../djangoapps/content_libraries/views.py | 30 +------------ 4 files changed, 94 insertions(+), 54 deletions(-) create mode 100644 openedx/core/djangoapps/content_libraries/utils.py diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py index dbaa5f4c1cf9..1b4ce1a1b2fa 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py @@ -5,6 +5,7 @@ from __future__ import annotations from openedx_learning.api.authoring_models import Collection +from openedx_learning.api.authoring import create_collection from opaque_keys.edx.locator import LibraryLocatorV2 from openedx.core.djangolib.testing.utils import skip_unless_cms @@ -33,23 +34,24 @@ def setUp(self): self.lib2 = ContentLibrary.objects.get(slug="test-lib-col-2") # Create Content Library Collections - self.col1 = Collection.objects.create( + self.col1 = create_collection( learning_package_id=self.lib1.learning_package.id, title="Collection 1", + created_by=self.user.id, description="Description for Collection 1", - created_by=self.user, ) - self.col2 = Collection.objects.create( + + self.col2 = create_collection( learning_package_id=self.lib1.learning_package.id, title="Collection 2", + created_by=self.user.id, description="Description for Collection 2", - created_by=self.user, ) - self.col3 = Collection.objects.create( + self.col3 = create_collection( learning_package_id=self.lib2.learning_package.id, title="Collection 3", + created_by=self.user.id, description="Description for Collection 3", - created_by=self.user, ) def test_get_library_collection(self): @@ -94,6 +96,12 @@ def test_get_invalid_library_collection(self): assert resp.status_code == 404 + # Fetch collection with invalid library_key provided, it should fail + resp = self.client.get( + URL_LIB_COLLECTION.format(lib_key=123, collection_id=123) + ) + assert resp.status_code == 404 + def test_list_library_collections(self): """ Test listing Content Library Collections @@ -120,11 +128,15 @@ def test_list_invalid_library_collections(self): """ Test listing invalid Content Library Collections """ - invalid_key = LibraryLocatorV2.from_string("lib:DoesNotExist:NE1") - resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=invalid_key)) + non_existing_key = LibraryLocatorV2.from_string("lib:DoesNotExist:NE1") + resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=non_existing_key)) assert resp.status_code == 404 + # List collections with invalid library_key provided, it should fail + resp = resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=123)) + assert resp.status_code == 404 + def test_create_library_collection(self): """ Test creating a Content Library Collection @@ -181,6 +193,14 @@ def test_create_invalid_library_collection(self): assert resp.status_code == 400 + # Create collection with invalid library_key provided, it should fail + resp = self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=123), + {**post_data_missing_title, **post_data_missing_desc}, + format="json" + ) + assert resp.status_code == 404 + def test_update_library_collection(self): """ Test updating a Content Library Collection @@ -243,6 +263,14 @@ def test_update_invalid_library_collection(self): assert resp.status_code == 404 + # Update collection with invalid library_key provided, it should fail + resp = self.client.patch( + URL_LIB_COLLECTION.format(lib_key=123, collection_id=self.col3.id), + patch_data, + format="json" + ) + assert resp.status_code == 404 + def test_delete_library_collection(self): """ Test deleting a Content Library Collection diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py index 0d89c4b8a2c7..7484b0893cff 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -19,6 +19,7 @@ ) from openedx.core.djangoapps.content_libraries import api, permissions +from openedx.core.djangoapps.content_libraries.utils import convert_exceptions from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryCollectionSerializer, ContentLibraryCollectionCreateOrUpdateSerializer, @@ -39,11 +40,7 @@ def _verify_and_fetch_library_collection(self, library_key, collection_id, user, """ Verify that the collection belongs to the library and the user has the correct permissions """ - try: - library_obj = api.require_permission_for_library_key(library_key, user, permission) - except api.ContentLibraryNotFound as exc: - raise Http404 from exc - + library_obj = api.require_permission_for_library_key(library_key, user, permission) collection = None if library_obj.learning_package_id: collection = authoring_api.get_learning_package_collections( @@ -51,6 +48,7 @@ def _verify_and_fetch_library_collection(self, library_key, collection_id, user, ).filter(id=collection_id).first() return collection + @convert_exceptions def retrieve(self, request, *args, **kwargs): """ Retrieve the Content Library Collection @@ -74,6 +72,7 @@ def retrieve(self, request, *args, **kwargs): serializer = self.get_serializer(collection) return Response(serializer.data) + @convert_exceptions def list(self, request, *args, **kwargs): """ List Collections that belong to Content Library @@ -85,17 +84,15 @@ def list(self, request, *args, **kwargs): # Check if user has permissions to view collections by checking if user # has permission to view the Content Library they belong to library_key = LibraryLocatorV2.from_string(lib_key_str) - try: - content_library = api.require_permission_for_library_key( - library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY - ) - except api.ContentLibraryNotFound as exc: - raise Http404 from exc + content_library = api.require_permission_for_library_key( + library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY + ) collections = authoring_api.get_learning_package_collections(content_library.learning_package.id) serializer = self.get_serializer(collections, many=True) return Response(serializer.data) + @convert_exceptions def create(self, request, *args, **kwargs): """ Create a Collection that belongs to a Content Library @@ -107,12 +104,9 @@ def create(self, request, *args, **kwargs): # Check if user has permissions to create a collection in the Content Library # by checking if user has permission to edit the Content Library library_key = LibraryLocatorV2.from_string(lib_key_str) - try: - content_library = api.require_permission_for_library_key( - library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY - ) - except api.ContentLibraryNotFound as exc: - raise Http404 from exc + content_library = api.require_permission_for_library_key( + library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + ) create_serializer = ContentLibraryCollectionCreateOrUpdateSerializer(data=request.data) create_serializer.is_valid(raise_exception=True) @@ -134,6 +128,7 @@ def create(self, request, *args, **kwargs): return Response(serializer.data) + @convert_exceptions def partial_update(self, request, *args, **kwargs): """ Update a Collection that belongs to a Content Library @@ -170,6 +165,7 @@ def partial_update(self, request, *args, **kwargs): return Response(serializer.data) + @convert_exceptions def destroy(self, request, *args, **kwargs): """ Deletes a Collection that belongs to a Content Library diff --git a/openedx/core/djangoapps/content_libraries/utils.py b/openedx/core/djangoapps/content_libraries/utils.py new file mode 100644 index 000000000000..bdfb1337d7bf --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/utils.py @@ -0,0 +1,44 @@ +""" Utils used for the content libraries. """ + +from functools import wraps +import logging + +from rest_framework.exceptions import NotFound, ValidationError + +from opaque_keys import InvalidKeyError + +from openedx.core.djangoapps.content_libraries import api + + +log = logging.getLogger(__name__) + + +def convert_exceptions(fn): + """ + Catch any Content Library API exceptions that occur and convert them to + DRF exceptions so DRF will return an appropriate HTTP response + """ + + @wraps(fn) + def wrapped_fn(*args, **kwargs): + try: + return fn(*args, **kwargs) + except InvalidKeyError as exc: + log.exception(str(exc)) + raise NotFound # lint-amnesty, pylint: disable=raise-missing-from + except api.ContentLibraryNotFound: + log.exception("Content library not found") + raise NotFound # lint-amnesty, pylint: disable=raise-missing-from + except api.ContentLibraryBlockNotFound: + log.exception("XBlock not found in content library") + raise NotFound # lint-amnesty, pylint: disable=raise-missing-from + except api.LibraryBlockAlreadyExists as exc: + log.exception(str(exc)) + raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from + except api.InvalidNameError as exc: + log.exception(str(exc)) + raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from + except api.BlockLimitReachedError as exc: + log.exception(str(exc)) + raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from + return wrapped_fn diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index bde8142d3fcc..11e5c9d23b2e 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -63,7 +63,6 @@ https://github.com/openedx/edx-platform/pull/30456 """ -from functools import wraps import itertools import json import logging @@ -120,40 +119,13 @@ from openedx.core.djangoapps.xblock import api as xblock_api from .models import ContentLibrary, LtiGradedResource, LtiProfile +from .utils import convert_exceptions User = get_user_model() log = logging.getLogger(__name__) -def convert_exceptions(fn): - """ - Catch any Content Library API exceptions that occur and convert them to - DRF exceptions so DRF will return an appropriate HTTP response - """ - - @wraps(fn) - def wrapped_fn(*args, **kwargs): - try: - return fn(*args, **kwargs) - except api.ContentLibraryNotFound: - log.exception("Content library not found") - raise NotFound # lint-amnesty, pylint: disable=raise-missing-from - except api.ContentLibraryBlockNotFound: - log.exception("XBlock not found in content library") - raise NotFound # lint-amnesty, pylint: disable=raise-missing-from - except api.LibraryBlockAlreadyExists as exc: - log.exception(str(exc)) - raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from - except api.InvalidNameError as exc: - log.exception(str(exc)) - raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from - except api.BlockLimitReachedError as exc: - log.exception(str(exc)) - raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from - return wrapped_fn - - class LibraryApiPaginationDocs: """ API docs for query params related to paginating ContentLibraryMetadata objects.