[feature] Added OpenWispPagination class with suitable default values#587
[feature] Added OpenWispPagination class with suitable default values#587pushpitkamboj wants to merge 2 commits intoopenwisp:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new DRF pagination class openwisp_utils.api.pagination.OpenWispPagination (subclasses DRF PageNumberPagination) with an import-time check that raises ImproperlyConfigured if DRF is missing. 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 in tests/test_project/tests/test_api.py verifying inheritance, default attributes, pagination behavior across pages, custom page_size, and enforcement of max_page_size. 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=2&page_size=10
DRF_View->>OpenWispPagination: extract pagination params from request
OpenWispPagination-->>DRF_View: page_size, max_page_size, page number
DRF_View->>Database: query with limit/offset
Database-->>DRF_View: result set
DRF_View->>OpenWispPagination: paginate queryset
OpenWispPagination-->>DRF_View: paginated results + metadata (count,next,previous)
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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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?
There was a problem hiding this comment.
@pandafy this change seems to contradict the issue description which mentions overridable settings.
@pushpit-kamboj properties should be simple, eg:
@property
def page_size(self):
return app_settings.API_PAGE_SIZEThe setting being OPENWISP_API_PAGE_SIZE, these should be documented in the settings page.
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?There was a problem hiding this comment.
@pushpit-kamboj this suggestion was valid, we want settings, but done properly as @pandafy described.
Settings should be prefixed as OPENWISP_API_. I would get rid of the possibility of configuring the page size query param as it would cause incompatibility issues among adopters.
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.
There was a problem hiding this comment.
@pushpit-kamboj I think @pandafy's suggestion is good, let's make sure the test project has an API view which uses this pagination class so we can also test manually, make sure the tests use that view.
|
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
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Great work @pushpit-kamboj, there's 1 main recommendation to address, see below.
There was a problem hiding this comment.
@pushpit-kamboj I think @pandafy's suggestion is good, let's make sure the test project has an API view which uses this pagination class so we can also test manually, make sure the tests use that view.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
openwisp_utils/api/pagination.py (2)
5-9:⚠️ Potential issue | 🟡 MinorChain the caught import exception when re-raising
ImproperlyConfigured.This remains unchained; use
except ImportError as errandraise ... from err(orfrom None) for clearer tracebacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_utils/api/pagination.py` around lines 5 - 9, The except block catching ImportError should capture the exception and chain it when re-raising ImproperlyConfigured: change the clause to "except ImportError as err" and raise ImproperlyConfigured(...) from err (or from None if you want to suppress the context); ensure the raised exception still contains the same message but uses "from err" to preserve traceback chaining for ImproperlyConfigured.
15-17:⚠️ Potential issue | 🟠 MajorImplement settings-backed pagination attributes instead of hardcoded constants.
The current class locks values at
10/100/"page_size"and does not satisfy the stated requirement to allow settings-based overrides.🔧 Proposed fix (single settings dictionary)
+from django.conf import settings from django.core.exceptions import ImproperlyConfigured @@ class OpenWispPagination(PageNumberPagination): """Reusable pagination class with sensible defaults.""" - page_size = 10 - max_page_size = 100 - page_size_query_param = "page_size" + _pagination_conf = getattr(settings, "OPENWISP_PAGINATION", {}) + page_size = _pagination_conf.get("page_size", 10) + max_page_size = _pagination_conf.get("max_page_size", 100) + page_size_query_param = _pagination_conf.get("page_size_query_param", "page_size")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_utils/api/pagination.py` around lines 15 - 17, Replace the hardcoded pagination defaults with settings-backed values: import Django settings and read a single settings dict (e.g. OPENWISP_PAGINATION) and use its keys to set page_size, max_page_size, and page_size_query_param in the pagination class instead of the constants 10/100/"page_size"; fall back to the current defaults when keys are missing so the class-level attributes (page_size, max_page_size, page_size_query_param) initialize from settings.get or getattr(OPENWISP_PAGINATION, ...) with defaults.docs/developer/other-utilities.rst (1)
89-95:⚠️ Potential issue | 🟡 MinorDocument the settings entry point for configuring these defaults.
Please add a direct hyperlink/ref to the relevant section in
docs/user/settings.rst(forOPENWISP_PAGINATION) so users can discover configuration without reading source code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/developer/other-utilities.rst` around lines 89 - 95, Add a direct cross-reference from the pagination docs to the settings entry by inserting a link to the OPENWISP_PAGINATION section in docs/user/settings.rst immediately after the pagination defaults list; reference the settings section title "OPENWISP_PAGINATION" (use an RST cross-reference such as :ref:`OPENWISP_PAGINATION` or a relative link to the settings document) so readers can jump straight to configuration details without reading source code.tests/test_project/tests/test_api.py (1)
73-127:⚠️ Potential issue | 🟠 MajorAdd at least one end-to-end pagination test through a real DRF view.
Current tests are useful but still synthetic; they don’t validate pagination wiring in an actual API view flow (query params → view pagination_class → serialized response).
🧪 Suggested direction
+class ShelfListView(ModelViewSet): + queryset = Shelf.objects.all().order_by("id") + serializer_class = ShelfSerializer + pagination_class = OpenWispPagination + +# call ShelfListView.as_view({"get": "list"}) with APIRequestFactory +# assert response.data["results"] length and pagination keys🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_project/tests/test_api.py` around lines 73 - 127, Add an end-to-end test that exercises OpenWispPagination through a real DRF view: inside TestOpenWispPagination define a minimal serializer (e.g. ShelfSerializer inheriting from rest_framework.serializers.ModelSerializer) and a small ListAPIView subclass with queryset=Shelf.objects.all() and pagination_class=OpenWispPagination (or use as_view() from a generic view), then use APIClient or APIRequestFactory to perform an HTTP GET (including query params like ?page=2 or ?page_size=5) against that view, parse the returned Response and assert the JSON contains "count", "next", "previous", "results", that count==15 and that len(results) matches requested page size (and respects max_page_size) to validate full request→view→pagination→response wiring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/developer/other-utilities.rst`:
- Around line 89-95: Add a direct cross-reference from the pagination docs to
the settings entry by inserting a link to the OPENWISP_PAGINATION section in
docs/user/settings.rst immediately after the pagination defaults list; reference
the settings section title "OPENWISP_PAGINATION" (use an RST cross-reference
such as :ref:`OPENWISP_PAGINATION` or a relative link to the settings document)
so readers can jump straight to configuration details without reading source
code.
In `@openwisp_utils/api/pagination.py`:
- Around line 5-9: The except block catching ImportError should capture the
exception and chain it when re-raising ImproperlyConfigured: change the clause
to "except ImportError as err" and raise ImproperlyConfigured(...) from err (or
from None if you want to suppress the context); ensure the raised exception
still contains the same message but uses "from err" to preserve traceback
chaining for ImproperlyConfigured.
- Around line 15-17: Replace the hardcoded pagination defaults with
settings-backed values: import Django settings and read a single settings dict
(e.g. OPENWISP_PAGINATION) and use its keys to set page_size, max_page_size, and
page_size_query_param in the pagination class instead of the constants
10/100/"page_size"; fall back to the current defaults when keys are missing so
the class-level attributes (page_size, max_page_size, page_size_query_param)
initialize from settings.get or getattr(OPENWISP_PAGINATION, ...) with defaults.
In `@tests/test_project/tests/test_api.py`:
- Around line 73-127: Add an end-to-end test that exercises OpenWispPagination
through a real DRF view: inside TestOpenWispPagination define a minimal
serializer (e.g. ShelfSerializer inheriting from
rest_framework.serializers.ModelSerializer) and a small ListAPIView subclass
with queryset=Shelf.objects.all() and pagination_class=OpenWispPagination (or
use as_view() from a generic view), then use APIClient or APIRequestFactory to
perform an HTTP GET (including query params like ?page=2 or ?page_size=5)
against that view, parse the returned Response and assert the JSON contains
"count", "next", "previous", "results", that count==15 and that len(results)
matches requested page size (and respects max_page_size) to validate full
request→view→pagination→response wiring.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/developer/other-utilities.rstopenwisp_utils/api/pagination.pytests/test_project/tests/test_api.py
📜 Review details
🧰 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)
🔇 Additional comments (2)
docs/developer/other-utilities.rst (1)
113-124: Good API examples with realistic endpoints.These request examples are clear and practical for adopters.
tests/test_project/tests/test_api.py (1)
85-127: Solid baseline coverage for paginator behavior.Inheritance, default attributes, page navigation, max-page-size enforcement, and response envelope checks are all covered well.
5 participants
Checklist
Reference to Existing Issue
Closes #586
Description of Changes
OpenWispPaginationclass inheriting from DRF'sPageNumberPagination.