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.

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

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

merged 2 commits into from
Jul 11, 2024

Conversation

kiram15
Copy link
Contributor

@kiram15 kiram15 commented Jul 10, 2024

Adding customization to the page_size so that we can increase it to 12 (to fetch all in one page), since 12 is the limit of highlights one customer can add.

Jira link

Post-review

  • Squash commits into discrete sets of changes
  • Ensure that once the changes have been deployed to stage, prod is manually deployed

@kiram15 kiram15 requested a review from a team July 10, 2024 21:51
@@ -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',
}

@kiram15 kiram15 merged commit 41a3ecd into master Jul 11, 2024
4 checks passed
@kiram15 kiram15 deleted the kiram15/ENT-9210 branch July 11, 2024 19:42
irfanuddinahmad pushed a commit that referenced this pull request Jul 24, 2024
* fix: adding page size param to highlights

* fix: adding default pagination class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants