From c302c8a22b4ad3f8c43b906242dcad40f1a0e294 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 21 Nov 2022 15:18:30 +0530 Subject: [PATCH] feat: block metadata api - index_dictionary Adds new api to return block metadata which includes index_dictionary. Reason for new api instead of adding it to course blocks API: data like index_dictionary are too large for the cache used by course/blocks transformers API. --- docs/swagger.yaml | 45 ++++++++++ lms/djangoapps/course_api/blocks/api.py | 19 ++++ .../course_api/blocks/tests/test_views.py | 90 ++++++++++++++++++- lms/djangoapps/course_api/blocks/urls.py | 16 +++- lms/djangoapps/course_api/blocks/views.py | 65 +++++++++++++- 5 files changed, 231 insertions(+), 4 deletions(-) diff --git a/docs/swagger.yaml b/docs/swagger.yaml index aab62265b5..b697b8bb1a 100644 --- a/docs/swagger.yaml +++ b/docs/swagger.yaml @@ -1660,6 +1660,51 @@ paths: in: path required: true type: string + /courses/v1/block_metadata/{usage_key_string}: + get: + operationId: courses_v1_block_metadata_read + summary: '**Use Case**' + description: |- + Returns the block metadata. Data like index_dictionary related to a + block should be fetched using this API, because they are too large for + the cache used by the course blocks/transformers API. + + **Example requests**: + + GET /api/courses/v1/block_metadata//? + &include=index_dictionary + + **Parameters**: + + * "include": a comma-separated list of keys to include. + Valid keys are "index_dictionary". + + **Response Values** + + A dictionary containing: + * id (string): Block usage_key_str. + * type (string) Block type. + * index_dictionary: (dict) The index_dictionary JSON data + (usually this is the text content of the block, for search + indexing or other purposes) for this block. Returned only if + the "index_dictionary" is included in the "include" + parameter. + parameters: + - name: include + in: query + description: A comma-separated list of keys to include. + required: false + type: string + responses: + '200': + description: '' + tags: + - courses + parameters: + - name: usage_key_string + in: path + required: true + type: string /courses/v1/blocks/: get: operationId: courses_v1_blocks_list diff --git a/lms/djangoapps/course_api/blocks/api.py b/lms/djangoapps/course_api/blocks/api.py index af82da22de..156fa3ed6e 100644 --- a/lms/djangoapps/course_api/blocks/api.py +++ b/lms/djangoapps/course_api/blocks/api.py @@ -152,3 +152,22 @@ def get_blocks( # return serialized data return serializer.data + + +def get_block_metadata(block, includes=()): + """ + Get metadata about the specified XBlock. + + Args: + block (XModuleDescriptor): block object + includes (list|set): list or set of metadata keys to include. Valid keys are: + index_dictionary: a dictionary of data used to add this XBlock's content + to a search index. + """ + data = { + "id": str(block.scope_ids.usage_id), + "type": block.scope_ids.block_type, + } + if "index_dictionary" in includes: + data["index_dictionary"] = block.index_dictionary() + return data diff --git a/lms/djangoapps/course_api/blocks/tests/test_views.py b/lms/djangoapps/course_api/blocks/tests/test_views.py index 0a329ad426..c3a23bc9e6 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_views.py +++ b/lms/djangoapps/course_api/blocks/tests/test_views.py @@ -11,7 +11,7 @@ from urllib.parse import urlencode, urlunparse from completion.test_utils import CompletionWaffleTestMixin, submit_completions_for_testing from django.conf import settings from django.urls import reverse -from opaque_keys.edx.locator import CourseLocator +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.roles import CourseDataResearcherRole @@ -491,3 +491,91 @@ class TestBlocksInCourseView(TestBlocksView, CompletionWaffleTestMixin): # pyli }) for block_id in self.non_orphaned_block_usage_keys: assert response.data['blocks'][block_id].get('completion') + + +class TestBlockMetadataView(SharedModuleStoreTestCase): # pylint: disable=test-inherits-tests + """ + Test class for BlockMetadataView. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + + # create a toy course + cls.course = ToyCourseFactory.create( + modulestore=cls.store, + due=datetime(3013, 9, 18, 11, 30, 00), + ) + cls.course_key = cls.course.id + cls.course_usage_key = cls.store.make_course_usage_key(cls.course_key) + + cls.non_orphaned_block_usage_keys = { + str(item.location) + for item in cls.store.get_items(cls.course_key) + # remove all orphaned items in the course, except for the root 'course' block + if cls.store.get_parent_location(item.location) or item.category == 'course' + } + + def setUp(self): + super().setUp() + self.admin_user = AdminFactory.create() + self.client.login(username=self.admin_user.username, password='test') + self.usage_key = list(self.non_orphaned_block_usage_keys)[0] + self.url = reverse( + 'blocks_metadata', + kwargs={'usage_key_string': str(self.usage_key)} + ) + self.query_params = {'include': "index_dictionary"} + + def verify_response(self, expected_status_code=200, params=None, url=None): + """ + Ensure that sending a GET request to the specified URL returns the + expected status code. + + Arguments: + expected_status_code: The status_code that is expected in the + response. + params: Parameters to add to self.query_params to include in the + request. + url: The URL to send the GET request. Default is self.url. + + Returns: + response: The HttpResponse returned by the request + """ + if params: + self.query_params.update(params) + response = self.client.get(url or self.url, self.query_params) + assert response.status_code == expected_status_code, str(response.content) + return response + + def test_invalid_usage_key(self): + url = reverse( + 'blocks_metadata', + kwargs={'usage_key_string': 'invalid-usage-key'} + ) + self.verify_response(400, url=url) + + def test_non_existent_block(self): + url = reverse( + 'blocks_metadata', + kwargs={'usage_key_string': str(BlockUsageLocator(self.course_key, 'non-existent', 'block'))} + ) + self.verify_response(404, url=url) + + def test_non_existent_block_anonymous(self): + self.client.logout() + url = reverse( + 'blocks_metadata', + kwargs={'usage_key_string': str(BlockUsageLocator(self.course_key, 'non-existent', 'block'))} + ) + self.verify_response(403, url=url) + + def test_block_metadata_response(self): + response = self.verify_response() + block_data = response.data + assert block_data['id'] == str(self.usage_key) + block_key = deserialize_usage_key(block_data['id'], self.course_key) + assert block_data['type'] == block_key.block_type + assert 'index_dictionary' in block_data + assert 'content' in block_data['index_dictionary'] diff --git a/lms/djangoapps/course_api/blocks/urls.py b/lms/djangoapps/course_api/blocks/urls.py index 08dbc0ad1e..c593dfc5ce 100644 --- a/lms/djangoapps/course_api/blocks/urls.py +++ b/lms/djangoapps/course_api/blocks/urls.py @@ -6,7 +6,7 @@ Course Block API URLs from django.conf import settings from django.urls import path, re_path -from .views import BlocksInCourseView, BlocksView +from .views import BlockMetadataView, BlocksInCourseView, BlocksView urlpatterns = [ # This endpoint requires the usage_key for the starting block. @@ -24,6 +24,13 @@ urlpatterns = [ kwargs={'hide_access_denials': True}, name="blocks_in_course" ), + # This endpoint requires the usage_key + re_path( + fr'^v1/block_metadata/{settings.USAGE_KEY_PATTERN}', + BlockMetadataView.as_view(), + name="blocks_metadata" + ), + # This endpoint requires the usage_key for the starting block. re_path( fr'^v2/blocks/{settings.USAGE_KEY_PATTERN}', @@ -37,6 +44,13 @@ urlpatterns = [ BlocksInCourseView.as_view(), name="blocks_in_course" ), + + # This endpoint requires the usage_key + re_path( + fr'^v2/block_metadata/{settings.USAGE_KEY_PATTERN}', + BlockMetadataView.as_view(), + name="blocks_metadata" + ), ] if getattr(settings, 'PROVIDER_STATES_URL', None): diff --git a/lms/djangoapps/course_api/blocks/views.py b/lms/djangoapps/course_api/blocks/views.py index 437ebfc3dc..998a525ddb 100644 --- a/lms/djangoapps/course_api/blocks/views.py +++ b/lms/djangoapps/course_api/blocks/views.py @@ -8,8 +8,10 @@ from django.db import transaction from django.http import Http404 from django.utils.cache import patch_response_headers from django.utils.decorators import method_decorator +from rest_framework.exceptions import PermissionDenied +from lms.djangoapps.courseware.access import has_access from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.keys import CourseKey, UsageKey from rest_framework.generics import ListAPIView from rest_framework.response import Response @@ -18,7 +20,7 @@ from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_c from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order -from .api import get_blocks +from .api import get_block_metadata, get_blocks from .forms import BlockListGetForm @@ -337,6 +339,65 @@ class BlocksInCourseView(BlocksView): return response +@method_decorator(transaction.non_atomic_requests, name='dispatch') +@view_auth_classes(is_authenticated=False) +class BlockMetadataView(DeveloperErrorViewMixin, ListAPIView): + """ + **Use Case** + + Returns the block metadata. Data like index_dictionary related to a + block should be fetched using this API, because they are too large for + the cache used by the course blocks/transformers API. + + **Example requests**: + + GET /api/courses/v1/block_metadata//? + &include=index_dictionary + + **Parameters**: + + * "include": a comma-separated list of keys to include. + Valid keys are "index_dictionary". + + **Response Values** + + A dictionary containing: + * id (string): Block usage_key_str. + * type (string) Block type. + * index_dictionary: (dict) The index_dictionary JSON data + (usually this is the text content of the block, for search + indexing or other purposes) for this block. Returned only if + the "index_dictionary" is included in the "include" + parameter. + """ + + def list(self, request, usage_key_string): # pylint: disable=arguments-differ + """ + Retrieves the usage_key for the requested block, and then returns the + block metadata + + Arguments: + request - Django request object + """ + + try: + usage_key = UsageKey.from_string(usage_key_string) + except InvalidKeyError: + raise ValidationError(f"'{str(usage_key_string)}' is not a valid usage key.") # lint-amnesty, pylint: disable=raise-missing-from + + if not has_access(request.user, "staff", usage_key): + raise PermissionDenied(f"You do not have permission to access block '{usage_key_string}'.") + + try: + block = modulestore().get_item(usage_key) + except ItemNotFoundError as exception: + raise Http404(f"Block not found: {str(exception)}") # lint-amnesty, pylint: disable=raise-missing-from + + includes = request.GET.get("include", "").split(",") + data = get_block_metadata(block, includes) + return Response(data) + + def recurse_mark_complete(block_id, blocks): """ Helper function to walk course tree dict,