Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: adding page size param to highlights #872

Merged
merged 2 commits into from
Jul 11, 2024
Merged

fix: adding page size param to highlights #872

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
440f28c
fix: adding page size param to highlights
kiram15 Jul 10, 2024
a2a6035
fix: adding default pagination class
kiram15 Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions enterprise_catalog/apps/api/v1/tests/test_curation_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def setUp(self):
# types including ones not supported by Curations/Highlighting feature, we set up a content type generator that
# deterministically provides supported content types.
supported_content_types = [COURSE, PROGRAM]
content_type_generator = (
self.content_type_generator = (
supported_content_types[idx % len(supported_content_types)]
for idx in itertools.count()
)
Expand All @@ -58,7 +58,7 @@ def setUp(self):
self.highlight_set_one = HighlightSetFactory(enterprise_curation=self.curation_config_one)
self.card_image_urls_one = [fake.image_url() + '.jpg' for idx in range(5)]
self.highlighted_content_metadata_one = [
ContentMetadataFactory(card_image_url=url, content_type=next(content_type_generator))
ContentMetadataFactory(card_image_url=url, content_type=next(self.content_type_generator))
for url in self.card_image_urls_one
]
self.highlighted_content_list_one = [
Expand All @@ -72,7 +72,7 @@ def setUp(self):
self.highlight_set_two = HighlightSetFactory(enterprise_curation=self.curation_config_two)
self.card_image_urls_two = [fake.image_url() + '.jpg' for idx in range(5)]
self.highlighted_content_metadata_two = [
ContentMetadataFactory(card_image_url=url, content_type=next(content_type_generator))
ContentMetadataFactory(card_image_url=url, content_type=next(self.content_type_generator))
for url in self.card_image_urls_two
]
self.highlighted_content_list_two = [
Expand Down Expand Up @@ -420,6 +420,41 @@ def test_list_catalog_learner_with_caching(self, include_customer_param):
assert second_response.json() == response.json()
assert second_response.status_code == status.HTTP_200_OK

def test_list_test(self):
"""
A catalog learner should be able to list the highlight sets of their own enterprise customer, but not that of
other enterprise customers.
"""
self.highlight_set_two = HighlightSetFactory(enterprise_curation=self.curation_config_one)
self.card_image_urls_two = [fake.image_url() + '.jpg' for idx in range(5)]
self.highlighted_content_metadata_two = [
ContentMetadataFactory(card_image_url=url, content_type=next(self.content_type_generator))
for url in self.card_image_urls_one
]
self.highlighted_content_list_two = [
HighlightedContentFactory(catalog_highlight_set=self.highlight_set_two, content_metadata=cm)
for cm in self.highlighted_content_metadata_two
]

url = reverse('api:v1:highlight-sets-list')

self.set_up_catalog_learner()
self.remove_role_assignments()

response = self.client.get(url)

assert response.status_code == status.HTTP_200_OK
highlight_sets_results = response.json()['results']
assert len(highlight_sets_results) == 2

limited_url = url + '?page_size=1'

response = self.client.get(limited_url)

assert response.status_code == status.HTTP_200_OK
highlight_sets_results = response.json()['results']
assert len(highlight_sets_results) == 1

def test_detail_invalid_uuid(self):
"""
An enterprise learner should get a 403 error trying to access a non-existent highlight set.
Expand Down
1 change: 1 addition & 0 deletions enterprise_catalog/apps/api/v1/views/curation/highlights.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ class HighlightSetBaseViewSet(PermissionRequiredForListingMixin, BaseViewSet):
renderer_classes = [JSONRenderer, XMLRenderer]
serializer_class = HighlightSetSerializer
lookup_field = 'uuid'
pagination_class = PageNumberWithSizePagination
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[inform/suggestion] While PageNumberWithSizePagination is the pagination_class mostly used throughout this app, there is an opportunity to rely on DefaultPagination from edx-drf-extensions (source) which is a more standard, out-of-the-box pagination_class within Open edX that includes the same page_size as PageNumberWithSizePagination but with a more consistent serialized pagination response compared to the default PageNumberPagination or the custom PageNumberWithSizePagination class.

from edx_rest_framework_extensions.paginators import DefaultPagination

pagination_class = DefaultPagination

Relying on DefaultPagination here and throughout the IDA would effectively eliminate the need for the custom PageNumberWithSizePagination paginator class altogether.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related, I also might wonder if we should start ensuring the default pagination class in the base REST_FRAMEWORK settings (source) for our Enterprise IDAs is set to DefaultPagination so all APIs use it by default without needing to explicitly opt-in to it.

# Django Rest Framework
REST_FRAMEWORK = {
    'DEFAULT_PAGINATION_CLASS': 'edx_rest_framework_extensions.paginators.DefaultPagination',
}


# Fields required for controlling access in the `list()` action
list_lookup_field = 'enterprise_curation__enterprise_uuid'
Expand Down
Loading