[feature] Added OpenWispPagination class with suitable default values #587
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds a new DRF pagination class at openwisp_utils/api/pagination.py: OpenWispPagination (subclasses DRF PageNumberPagination) with an import-time check that raises ImproperlyConfigured if DRF is absent. The class sets defaults page_size = 10, max_page_size = 100, and page_size_query_param = "page_size". Adds documentation in docs/developer/other-utilities.rst describing the class and usage, and tests (tests/test_project/tests/test_api.py) verifying inheritance, defaults, pagination behavior, and max_page_size enforcement. Sequence Diagram(s)sequenceDiagram
participant Client
participant DRF_View as "DRF View"
participant OpenWispPagination as "OpenWispPagination"
participant Database
Client->>DRF_View: HTTP GET /api/resources?page=1&page_size=10
DRF_View->>OpenWispPagination: resolve pagination params from request
OpenWispPagination-->>DRF_View: page_size, max_page_size, query_param
DRF_View->>Database: query with limit/offset
Database-->>DRF_View: result set
DRF_View->>OpenWispPagination: paginate queryset
OpenWispPagination-->>DRF_View: paginated results + metadata
DRF_View-->>Client: HTTP 200 with paginated payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_utils/api/pagination.py`:
- Around line 21-25: The class-level attributes page_size, max_page_size, and
page_size_query_param are read at import time so `@override_settings` won't affect
tests; change them to properties that read from django.conf.settings at access
time and add a setter for page_size so DRF's paginate_queryset can assign to
self.page_size at runtime. Specifically, replace the class attributes in the
pagination class with: a `@property` page_size that returns getattr(settings,
"OPENWISP_PAGINATION_PAGE_SIZE", 10) and a `@page_size.setter` that stores an
instance attribute (e.g., self._page_size) when assigned; implement `@property`
max_page_size and `@property` page_size_query_param that return getattr(settings,
"...", default) so tests using override_settings observe changes. Ensure
references in methods use
self.page_size/self.max_page_size/self.page_size_query_param.
🧹 Nitpick comments (2)
openwisp_utils/api/pagination.py (2)
4-10: Useraise ... from Noneto suppress exception chaining.The
raiseinside theexcept ImportErrorblock should usefrom Noneto produce a clean traceback focused on theImproperlyConfigurederror, rather than showing the originalImportErroralongside it. This is also flagged by Ruff B904.♻️ Proposed fix
try: from rest_framework.pagination import PageNumberPagination -except ImportError: # pragma: nocover +except ImportError: # pragma: nocover raise ImproperlyConfigured( "Django REST Framework is required to use " "this feature but it is not installed" - ) + ) from None
13-19: Docstring formatting is collapsed into a single hard-to-read line.The settings list in the docstring is intended to be multi-line but renders as a single run-on sentence. Consider using proper reStructuredText or plain-text formatting.
📝 Suggested docstring formatting
class OpenWispPagination(PageNumberPagination): - """Reusable pagination class with sensible defaults. - - Configurable via Django settings: - OPENWISP_PAGINATION_PAGE_SIZE - (default: 10) - OPENWISP_PAGINATION_MAX_PAGE_SIZE (default: 100) - - OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM (default: 'page_size') - """ + """Reusable pagination class with sensible defaults. + + Configurable via Django settings: + + - OPENWISP_PAGINATION_PAGE_SIZE (default: 10) + - OPENWISP_PAGINATION_MAX_PAGE_SIZE (default: 100) + - OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM (default: 'page_size') + """
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/developer/other-utilities.rstopenwisp_utils/api/pagination.py
🧰 Additional context used
🪛 Ruff (0.14.14)
openwisp_utils/api/pagination.py
[warning] 7-10: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 7-10: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (1)
docs/developer/other-utilities.rst (1)
86-155: Documentation looks comprehensive and well-structured.The documentation clearly covers defaults, settings configuration, view-level usage, subclassing, and global configuration. The code examples are accurate and follow established patterns in the file.
One minor suggestion: consider adding a note that when using the global configuration (Line 153-155), the
PAGE_SIZEinREST_FRAMEWORKsettings is not needed becauseOpenWispPaginationalready defines its ownpage_sizevia theOPENWISP_PAGINATION_PAGE_SIZEsetting. This could prevent confusion for developers familiar with DRF's standard setup where both are typically configured together.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pushpitkamboj
force-pushed
the
feature/reusable_pagination
branch
from
97eff50 to
8b19ec1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_utils/api/pagination.py`:
- Around line 4-10: Change the bare except ImportError to capture the exception
and re-raise ImproperlyConfigured with explicit exception chaining: use "except
ImportError as err" and re-raise the ImproperlyConfigured(...) using "from err"
so the original ImportError traceback is preserved; this applies to the import
of PageNumberPagination and the subsequent raise of ImproperlyConfigured in
openwisp_utils/api/pagination.py.
🧹 Nitpick comments (2)
openwisp_utils/api/pagination.py (2)
14-19: Docstring readability: consider a proper list format.The settings are concatenated on a single wrapped line. A structured list would be easier to scan and render better in documentation tools.
Proposed fix
"""Reusable pagination class with sensible defaults. - Configurable via Django settings: - OPENWISP_PAGINATION_PAGE_SIZE - (default: 10) - OPENWISP_PAGINATION_MAX_PAGE_SIZE (default: 100) - - OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM (default: 'page_size') + Configurable via Django settings: + + - OPENWISP_PAGINATION_PAGE_SIZE (default: 10) + - OPENWISP_PAGINATION_MAX_PAGE_SIZE (default: 100) + - OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM (default: 'page_size') """
32-41: Minor: inconsistent docstrings across properties.
page_sizehas a docstring on its getter (Line 23), whilemax_page_sizehas one on its setter (Line 40), andpage_size_query_paramhas none at all. Consider either adding brief docstrings consistently to all getters (or none) to keep the class uniform.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/developer/other-utilities.rstopenwisp_utils/api/pagination.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/developer/other-utilities.rst
🧰 Additional context used
🪛 Ruff (0.14.14)
openwisp_utils/api/pagination.py
[warning] 7-10: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 7-10: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.0.0
🔇 Additional comments (3)
openwisp_utils/api/pagination.py (3)
1-2: LGTM!Clean, minimal imports.
21-30: LGTM — property-based approach correctly addresses the import-time binding issue.The getter falls back to settings at runtime, and the setter accommodates DRF's internal
self.page_size = ...assignment inpaginate_queryset. Since DRF creates a new paginator instance per request, the instance-level override won't leak across requests.
43-53: LGTM!Consistent property pattern with the other two attributes. No issues.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/developer/other-utilities.rst`:
- Around line 132-150: The example is missing the import for ModelViewSet which
prevents copy-paste usage; add the import for ModelViewSet (from
rest_framework.viewsets import ModelViewSet) at the top of the snippet alongside
the OpenWispPagination import so the classes LargePagination and ReportViewSet
(which subclasses ModelViewSet and references OpenWispPagination) work as shown.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/developer/other-utilities.rst
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pushpitkamboj
force-pushed
the
feature/reusable_pagination
branch
from
0eaa83e to
469ee65
Compare
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
Thanks for picking up this issue @pushpitkamboj. The implementation can be greatly simplified. See my comments below.
openwisp_utils/api/pagination.py
Outdated
| @property | ||
| def page_size(self): | ||
| """Return the page size from settings or default.""" | ||
| if hasattr(self, "_page_size"): | ||
| return self._page_size | ||
| return getattr(settings, "OPENWISP_PAGINATION_PAGE_SIZE", 10) | ||
|
|
||
| @page_size.setter | ||
| def page_size(self, value): | ||
| self._page_size = value | ||
|
|
||
| @property | ||
| def max_page_size(self): | ||
| if hasattr(self, "_max_page_size"): | ||
| return self._max_page_size | ||
| return getattr(settings, "OPENWISP_PAGINATION_MAX_PAGE_SIZE", 100) | ||
|
|
||
| @max_page_size.setter | ||
| def max_page_size(self, value): | ||
| """Allow setting max_page_size.""" | ||
| self._max_page_size = value | ||
|
|
||
| @property | ||
| def page_size_query_param(self): | ||
| if hasattr(self, "_page_size_query_param"): | ||
| return self._page_size_query_param | ||
| return getattr( | ||
| settings, "OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM", "page_size" | ||
| ) | ||
|
|
||
| @page_size_query_param.setter | ||
| def page_size_query_param(self, value): | ||
| self._page_size_query_param = value |
There was a problem hiding this comment.
@pushpitkamboj why do we need to use Python properties here? As far as I know, we can simple do the following:
class OpenWispPagination(PageNumberPagination):
page_size = 20
max_page_size = 100
page_size_query_param = page_size There was a problem hiding this comment.
isnt these just hard coded values?
we are not overriding the defaults from PageNumberPagination class?
docs/developer/other-utilities.rst
Outdated
| - ``OPENWISP_PAGINATION_PAGE_SIZE`` (default: ``10``): Default number of | ||
| items per page | ||
| - ``OPENWISP_PAGINATION_MAX_PAGE_SIZE`` (default: ``100``): Maximum | ||
| allowed page size | ||
| - ``OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM`` (default: | ||
| ``"page_size"``): Query parameter name for page size |
There was a problem hiding this comment.
While creating the issue, I didn't think we can benefit from having this configurable.
Since you have already implemented this, I will ask you to implement this properly:
- All settings are defined in openwisp_utils/settings.py
- The docs for the settings lives at docs/user/settings.rst. Pay attention to how other settings are defined.
- Use a hyperlink on this page to the settings.rst section to inform the users about these settings. E..g.
You can configure different properties of OpenWispPagination class by using `OPENWISP_PAGINATION_ATTRIBUTES <link to the section>`.
Instead of adding three settings, can we just add one setting which is a dictionary?
docs/developer/other-utilities.rst
Outdated
| For view-specific pagination requirements, you can subclass | ||
| ``OpenWispPagination`` and override the properties: | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| from openwisp_utils.api.pagination import OpenWispPagination | ||
| from rest_framework.viewsets import ModelViewSet | ||
|
|
||
|
|
||
| class LargePagination(OpenWispPagination): | ||
| @property | ||
| def page_size(self): | ||
| return 50 | ||
|
|
||
| @property | ||
| def max_page_size(self): | ||
| return 500 | ||
|
|
||
|
|
||
| class ReportViewSet(ModelViewSet): | ||
| queryset = Report.objects.all() | ||
| serializer_class = ReportSerializer | ||
| pagination_class = LargePagination | ||
|
|
There was a problem hiding this comment.
After implementing above changes, this example can be largely simplified.
docs/developer/other-utilities.rst
Outdated
| **Global Configuration:** | ||
|
|
||
| To use ``OpenWispPagination`` as the default pagination class for all DRF | ||
| views in your project, add it to ``REST_FRAMEWORK`` settings: | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| REST_FRAMEWORK = { | ||
| "DEFAULT_PAGINATION_CLASS": "openwisp_utils.api.pagination.OpenWispPagination", | ||
| } | ||
|
|
There was a problem hiding this comment.
Let's avoid mentioning this here. This is a DRF setting which is documented in the DRF docs.
| **API Request Examples:** | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| # Returns first 10 items (default page size) | ||
| GET /api/devices/ | ||
|
|
||
| # Returns items 11-20 (second page) | ||
| GET /api/devices/?page=2 | ||
|
|
||
| # Returns first 25 items (custom page size) | ||
| GET /api/devices/?page_size=25 | ||
|
|
||
| # Returns items 26-50 with custom page size | ||
| GET /api/devices/?page=2&page_size=25 | ||
|
|
There was a problem hiding this comment.
These examples are good, but let's use a real URL, e.g. /api/v1/controller/device/.
There was a problem hiding this comment.
Can we create a simple DRF view in the test project for any model and then test the pagination on that view? The current tests look very synthetic and do not model real world scenario.
I will suggest you to look for test in other modules, e.g. openwisp-notification / openwisp-radius on how we write tests for pagination. Here's one quick instance.
|
Hi, sorry for late response I was working on other PR which was a priority. Will work on this ASAP. |
pushpitkamboj
force-pushed
the
feature/reusable_pagination
branch
from
b04a8d9 to
00fcb9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_project/tests/test_api.py`:
- Around line 73-127: Implement settings-based overrides for OpenWispPagination
by converting its page_size, max_page_size and page_size_query_param into
`@property` getters that read OPENWISP_PAGINATION_PAGE_SIZE,
OPENWISP_PAGINATION_MAX_PAGE_SIZE and OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM
from django.conf.settings with the existing defaults as fallbacks (modify
pagination.py and the OpenWispPagination class accordingly). Then add tests in
tests/test_api.py that use `@override_settings` to set custom values for those
OPENWISP_PAGINATION_* keys and assert that OpenWispPagination.paginate_queryset
and get_paginated_response honor the overridden page size, max page size and
query param (reference OpenWispPagination and the test methods in
TestOpenWispPagination to extend with new `@override_settings` cases).
🧹 Nitpick comments (2)
tests/test_project/tests/test_api.py (2)
118-127: Passing an empty list toget_paginated_responseis misleading.
get_paginated_response([])works becausecountis derived from the paginator's queryset (set by the priorpaginate_querysetcall), but the emptyresultscontradicts the assertion context. Pass the actual paginated slice for clarity.Suggested fix
def test_get_paginated_response(self): request = self._get_request() queryset = Shelf.objects.all().order_by('id') - self.pagination.paginate_queryset(queryset, request) - response = self.pagination.get_paginated_response([]) + paginated = self.pagination.paginate_queryset(queryset, request) + response = self.pagination.get_paginated_response(paginated) self.assertIn('count', response.data) self.assertIn('next', response.data) self.assertIn('previous', response.data) self.assertIn('results', response.data) self.assertEqual(response.data['count'], 15)
111-116: This test mutates the instance rather than testing the class default.Directly setting
self.pagination.max_page_size = 10tests DRF's clamping logic, notOpenWispPagination's defaultmax_page_size. Consider requestingpage_size=200(exceeding the defaultmax_page_size=100) against the unmodified instance to verify the class's own cap is enforced.Suggested fix
def test_paginate_queryset_respects_max_page_size(self): - self.pagination.max_page_size = 10 - request = self._get_request(data={'page_size': 100}) + request = self._get_request(data={'page_size': 200}) queryset = Shelf.objects.all().order_by('id') paginated = self.pagination.paginate_queryset(queryset, request) - self.assertEqual(len(paginated), 10) + # max_page_size=100 caps the request; only 15 items exist, so we get 15 + self.assertEqual(len(paginated), 15)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/developer/other-utilities.rstopenwisp_utils/api/pagination.pytests/test_project/tests/test_api.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/developer/other-utilities.rst
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_project/tests/test_api.py (3)
openwisp_utils/api/pagination.py (1)
OpenWispPagination(12-17)tests/test_project/tests/__init__.py (1)
_create_shelf(24-30)tests/test_project/models.py (1)
Shelf(16-65)
🪛 Ruff (0.15.0)
openwisp_utils/api/pagination.py
[warning] 6-9: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 6-9: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (2)
tests/test_project/tests/test_api.py (2)
73-81: Test class setup looks good.Clean use of
setUpwithAPIRequestFactoryand theCreateMixinhelper. The 15-shelf dataset is a sensible choice for exercising page boundaries withpage_size=10.
85-91: LGTM — inheritance and default attribute tests are straightforward.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
tests/test_project/tests/test_api.py
Outdated
| class TestOpenWispPagination(CreateMixin, TestCase): | ||
| shelf_model = Shelf | ||
|
|
||
| def setUp(self): | ||
| self.factory = APIRequestFactory() | ||
| self.pagination = OpenWispPagination() | ||
| for i in range(15): | ||
| self._create_shelf(name=f'shelf{i}') | ||
|
|
||
| def _get_request(self, path='/api/shelves/', data=None): | ||
| return Request(self.factory.get(path, data)) | ||
|
|
||
| def test_inheritance(self): | ||
| self.assertIsInstance(self.pagination, PageNumberPagination) | ||
|
|
||
| def test_default_attributes(self): | ||
| self.assertEqual(self.pagination.page_size, 10) | ||
| self.assertEqual(self.pagination.max_page_size, 100) | ||
| self.assertEqual(self.pagination.page_size_query_param, 'page_size') | ||
|
|
||
| def test_paginate_queryset(self): | ||
| request = self._get_request() | ||
| queryset = Shelf.objects.all().order_by('id') | ||
| paginated = self.pagination.paginate_queryset(queryset, request) | ||
| self.assertEqual(len(paginated), 10) | ||
|
|
||
| def test_paginate_queryset_second_page(self): | ||
| request = self._get_request(data={'page': 2}) | ||
| queryset = Shelf.objects.all().order_by('id') | ||
| paginated = self.pagination.paginate_queryset(queryset, request) | ||
| self.assertEqual(len(paginated), 5) | ||
|
|
||
| def test_paginate_queryset_custom_page_size(self): | ||
| request = self._get_request(data={'page_size': 5}) | ||
| queryset = Shelf.objects.all().order_by('id') | ||
| paginated = self.pagination.paginate_queryset(queryset, request) | ||
| self.assertEqual(len(paginated), 5) | ||
|
|
||
| def test_paginate_queryset_respects_max_page_size(self): | ||
| self.pagination.max_page_size = 10 | ||
| request = self._get_request(data={'page_size': 100}) | ||
| queryset = Shelf.objects.all().order_by('id') | ||
| paginated = self.pagination.paginate_queryset(queryset, request) | ||
| self.assertEqual(len(paginated), 10) | ||
|
|
||
| def test_get_paginated_response(self): | ||
| request = self._get_request() | ||
| queryset = Shelf.objects.all().order_by('id') | ||
| self.pagination.paginate_queryset(queryset, request) | ||
| response = self.pagination.get_paginated_response([]) | ||
| self.assertIn('count', response.data) | ||
| self.assertIn('next', response.data) | ||
| self.assertIn('previous', response.data) | ||
| self.assertIn('results', response.data) | ||
| self.assertEqual(response.data['count'], 15) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add tests for settings-based overrides once the feature is implemented.
Per the linked issue #586, the pagination class should support Django settings overrides. Once the property-based approach (suggested in pagination.py) is adopted, add tests using @override_settings to verify that custom OPENWISP_PAGINATION_PAGE_SIZE, OPENWISP_PAGINATION_MAX_PAGE_SIZE, and OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM values are respected.
🤖 Prompt for AI Agents
In `@tests/test_project/tests/test_api.py` around lines 73 - 127, Implement
settings-based overrides for OpenWispPagination by converting its page_size,
max_page_size and page_size_query_param into `@property` getters that read
OPENWISP_PAGINATION_PAGE_SIZE, OPENWISP_PAGINATION_MAX_PAGE_SIZE and
OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM from django.conf.settings with the
existing defaults as fallbacks (modify pagination.py and the OpenWispPagination
class accordingly). Then add tests in tests/test_api.py that use
`@override_settings` to set custom values for those OPENWISP_PAGINATION_* keys and
assert that OpenWispPagination.paginate_queryset and get_paginated_response
honor the overridden page size, max page size and query param (reference
OpenWispPagination and the test methods in TestOpenWispPagination to extend with
new `@override_settings` cases).
pushpitkamboj
force-pushed
the
feature/reusable_pagination
branch
from
00fcb9d to
1c4e518
Compare
4 participants
Checklist
Reference to Existing Issue
Closes #586
Description of Changes
OpenWispPaginationclass inheriting from DRF'sPageNumberPagination.