diff --git a/codecov.yml b/codecov.yml index 60f9c518..89dd6952 100644 --- a/codecov.yml +++ b/codecov.yml @@ -3,7 +3,7 @@ coverage: status: patch: default: - target: 92 + target: 91 project: default: - target: 92 + target: 91 diff --git a/enterprise_catalog/apps/api/v1/tests/test_views.py b/enterprise_catalog/apps/api/v1/tests/test_views.py index be9b0f2d..b7af5999 100644 --- a/enterprise_catalog/apps/api/v1/tests/test_views.py +++ b/enterprise_catalog/apps/api/v1/tests/test_views.py @@ -2123,6 +2123,59 @@ def test_list_with_missing_enterprise_customer(self): self.assertEqual(response.data['count'], 0) +def ddt_cross_product(data_x, data_y): + """ + Given two lists of test data dicts, produce a flat list of test data dicts + comprising every test scenario in x crossed with with every test scenario + of in y. + + Demo: + Invoking this: + + ddt_cross_product( + [ + {'date': 'past'}, + {'date': 'future'}, + ], + [ + {'size': 'big'}, + {'size': 'small'}, + ], + ) + + Returns this: + + [ + {'date': 'past', 'size': 'big'}, + {'date': 'past', 'size': 'small'}, + {'date': 'future', 'size': 'big'}, + {'date': 'future', 'size': 'small'} + ] + + Usage: + @ddt.data(*ddt_cross_product( + [ + {'date': 'past'}, + {'date': 'future'}, + ], + [ + {'size': 'big'}, + {'size': 'small'}, + ], + )) + def test_things(self, date, size): + pass + + Args: + data_x (list of dict): First ddt data args. + data_y (list of dict): Second ddt data args. + + Returns: + list of dict: The result of x cross y. Elements usable as arguments for ``@ddt.data()``. + """ + return [x | y for x in data_x for y in data_y] + + @ddt.ddt class ContentMetadataViewTests(APITestMixin): """ @@ -2131,9 +2184,23 @@ class ContentMetadataViewTests(APITestMixin): def setUp(self): super().setUp() self.set_up_staff() - self.content_metadata_object = ContentMetadataFactory( - content_type='course', - content_uuid=uuid.uuid4(), + self.content_metadata_course1 = ContentMetadataFactory( + content_type=COURSE, + ) + self.content_metadata_course1_run1 = ContentMetadataFactory( + content_type=COURSE_RUN, + parent_content_key=self.content_metadata_course1.content_key, + ) + self.content_metadata_course1_run2 = ContentMetadataFactory( + content_type=COURSE_RUN, + parent_content_key=self.content_metadata_course1.content_key, + ) + self.content_metadata_course2 = ContentMetadataFactory( + content_type=COURSE, + ) + self.content_metadata_course2_run1 = ContentMetadataFactory( + content_type=COURSE_RUN, + parent_content_key=self.content_metadata_course2.content_key, ) def test_list_success(self): @@ -2143,48 +2210,105 @@ def test_list_success(self): url = reverse('api:v1:content-metadata-list') response = self.client.get(url) response_json = response.json() - assert len(response_json.get('results')) == 1 - assert response_json.get('results')[0].get("key") == self.content_metadata_object.content_key + assert len(response_json.get('results')) == 5 + assert set(r['key'] for r in response_json.get('results')) == set([ + self.content_metadata_course1.content_key, + self.content_metadata_course1_run1.content_key, + self.content_metadata_course1_run2.content_key, + self.content_metadata_course2.content_key, + self.content_metadata_course2_run1.content_key, + ]) - def test_list_with_content_keys(self): + @ddt.data( + {'request_by_field': 'content_key'}, + {'request_by_field': 'content_uuid'}, + ) + @ddt.unpack + def test_list_with_content_identifiers(self, request_by_field): """ - Test a successful, expected api response for the metadata list endpoint with a supplied content keys query - param + Test list endpoint while passing the ``?content_identifiers=`` param to filter response. """ ContentMetadataFactory(content_type='course') - junk_identifier = urlencode({'content_identifiers': 'edx+101'}) - encoded_key = urlencode({'content_identifiers': self.content_metadata_object.content_key}) - query_param_string = f"?{encoded_key}&{junk_identifier}" - url = reverse('api:v1:content-metadata-list') + query_param_string + query_string = '?' + urlencode({ + 'content_identifiers': [ + getattr(self.content_metadata_course1, request_by_field), + getattr(self.content_metadata_course2_run1, request_by_field), + 'edx+101', # junk + ], + }, doseq=True) + url = reverse('api:v1:content-metadata-list') + query_string response = self.client.get(url) response_json = response.json() - assert len(response_json.get('results')) == 1 - assert response_json.get('results')[0].get("key") == self.content_metadata_object.content_key - assert response_json.get('results')[0].get("course_runs")[0].get('start') == '2024-02-12T11:00:00Z' + assert len(response_json.get('results')) == 2 + assert set(r['key'] for r in response_json.get('results')) == set([ + self.content_metadata_course1.content_key, + self.content_metadata_course2_run1.content_key, + ]) - def test_get_success(self): + def test_list_with_coerce_to_parent_course(self): """ - Test a successful, expected api response for the metadata fetch endpoint + Test list endpoint while passing the ``?coerce_to_parent_course=true`` param to return courses. """ - url = reverse( - 'api:v1:content-metadata-detail', - kwargs={'pk': self.content_metadata_object.id} - ) + ContentMetadataFactory(content_type='course') + query_string = '?' + urlencode({ + 'coerce_to_parent_course': True, + 'content_identifiers': [ + self.content_metadata_course1.content_key, # course should remain a course. + self.content_metadata_course2_run1.content_key, # run should be coerced to course. + 'edx+101', # junk should be ignored. + ], + }, doseq=True) + url = reverse('api:v1:content-metadata-list') + query_string response = self.client.get(url) response_json = response.json() - assert response_json.get('title') == self.content_metadata_object.json_metadata.get('title') - - def test_filter_list_by_uuid(self): + assert len(response_json.get('results')) == 2 + assert set(r['key'] for r in response_json.get('results')) == set([ + self.content_metadata_course1.content_key, # course successfully remains a course. + self.content_metadata_course2.content_key, # run successfully coerced to course. + # junk successfully ignored. + ]) + + @ddt.data(*ddt_cross_product( + [ + {'request_content_type': COURSE, 'coerce_to_parent_course': False, 'expect_content_type': COURSE}, + {'request_content_type': COURSE, 'coerce_to_parent_course': True, 'expect_content_type': COURSE}, + {'request_content_type': COURSE_RUN, 'coerce_to_parent_course': False, 'expect_content_type': COURSE_RUN}, + {'request_content_type': COURSE_RUN, 'coerce_to_parent_course': True, 'expect_content_type': COURSE}, + ], + # Repeat ever test above given different types of identifier input types. + [ + {'request_by_field': 'id'}, + {'request_by_field': 'content_key'}, + {'request_by_field': 'content_uuid'}, + ], + )) + @ddt.unpack + def test_retrieve_success( + self, + request_by_field='id', + coerce_to_parent_course=False, + request_content_type=COURSE, + expect_content_type=COURSE, + ): """ - Test that the list content_identifiers query param accepts uuids + Test a successful, expected api response for the metadata fetch endpoint given every + possible combination of inputs. """ - query_param_string = f"?content_identifiers={self.content_metadata_object.content_uuid}" - url = reverse('api:v1:content-metadata-list') + query_param_string - response = self.client.get(url) + object_to_request = ( + self.content_metadata_course1 if request_content_type == COURSE else self.content_metadata_course1_run1 + ) + query_string = '?' + urlencode({'coerce_to_parent_course': True}) if coerce_to_parent_course else '' + url = reverse( + 'api:v1:content-metadata-detail', + kwargs={'pk': getattr(object_to_request, request_by_field)} + ) + response = self.client.get(url + query_string) response_json = response.json() - assert len(response_json.get('results')) == 1 - assert response_json.get('results')[0].get("key") == self.content_metadata_object.content_key - assert response_json.get('results')[0].get("course_runs")[0].get('start') == '2024-02-12T11:00:00Z' + + expected_object_to_receive = ( + self.content_metadata_course1 if expect_content_type == COURSE else self.content_metadata_course1_run1 + ) + assert response_json.get('key') == expected_object_to_receive.content_key @ddt.ddt diff --git a/enterprise_catalog/apps/api/v1/views/content_metadata.py b/enterprise_catalog/apps/api/v1/views/content_metadata.py index 216da5e5..65b6ca94 100644 --- a/enterprise_catalog/apps/api/v1/views/content_metadata.py +++ b/enterprise_catalog/apps/api/v1/views/content_metadata.py @@ -1,5 +1,7 @@ import uuid +from django.db.models import Q +from django.shortcuts import get_object_or_404 from edx_rest_framework_extensions.auth.jwt.authentication import ( JwtAuthentication, ) @@ -12,6 +14,7 @@ PageNumberWithSizePagination, ) from enterprise_catalog.apps.api.v1.serializers import ContentMetadataSerializer +from enterprise_catalog.apps.catalog.constants import COURSE_RUN from enterprise_catalog.apps.catalog.models import ContentMetadata @@ -47,16 +50,81 @@ class ContentMetadataView(viewsets.ReadOnlyModelViewSet): queryset = ContentMetadata.objects.all() pagination_class = PageNumberWithSizePagination + @property + def pk_is_object_id(self): + return self.kwargs.get('pk', '').isdigit() + + @property + def pk_is_content_uuid(self): + return not self.pk_is_object_id and is_valid_uuid(self.kwargs.get('pk')) + + @property + def pk_is_content_key(self): + return not self.pk_is_object_id and not self.pk_is_content_uuid + + @property + def coerce_to_parent_course(self): + return self.request.query_params.get('coerce_to_parent_course', False) + def get_queryset(self, **kwargs): """ Returns all content metadata objects filtered by an optional request query param (LIST) ``content_identifiers`` + + If ``?coerce_to_parent_course=true` is passed to the list endpoint, any course runs which + would have been returned are coerced into their parent course in the response. No course + runs are returned when this setting is true. """ - content_filters = self.request.query_params.getlist('content_identifiers') queryset = self.queryset + + # Find all directly requested content + content_filters = self.request.query_params.getlist('content_identifiers') + queryset_direct = None + content_keys = [] if content_filters: content_uuids, content_keys = partition(is_valid_uuid, content_filters) - if content_keys: - queryset = queryset.filter(content_key__in=content_filters) - if content_uuids: - queryset = queryset.filter(content_uuid__in=content_filters) + queryset_direct = self.queryset.filter( + Q(content_uuid__in=content_uuids) | Q(content_key__in=content_keys) + ) + queryset = queryset_direct + + # If ``?parent_course=true`` was passed, exclude course runs. + if self.coerce_to_parent_course: + query_filters = ~Q(content_type=COURSE_RUN) + # If ``?content_identifiers=`` was passed, follow any matched course run objects back up + # to their parent courses and include those in the response. + if content_filters: + parent_content_keys = list( + record[0] for record in + queryset_direct.filter(content_type=COURSE_RUN).values_list('parent_content_key') + ) + all_content_keys_to_find = content_keys + parent_content_keys + query_filters &= ( + Q(content_uuid__in=content_uuids) | Q(content_key__in=all_content_keys_to_find) + ) + queryset = self.queryset.filter(query_filters) + return queryset + + def retrieve(self, request, *args, pk=None, **kwargs): + """ + Override to support querying by content key/uuid, and to optionally coerce runs to courses. + """ + # Support alternative pk types besisdes just the raw object IDs (which are completely opaque + # to API clients). + obj = None + if self.pk_is_content_uuid: + obj = get_object_or_404(self.queryset, content_uuid=pk) + elif self.pk_is_content_key: + obj = get_object_or_404(self.queryset, content_key=pk) + else: + obj = get_object_or_404(self.queryset, pk=pk) + + # Coerce course runs to courses if requested. + if self.coerce_to_parent_course: + if obj.content_type == COURSE_RUN: + obj = get_object_or_404(self.queryset, content_key=obj.parent_content_key) + + # Finally, call super's retrieve() which has more DRF guts that are best not duplicated in + # this codebase. + self.kwargs['pk'] = obj.id + return super().retrieve(request, *args, **kwargs)