Define default limit #159
Define default limit #159
Conversation
WalkthroughThis pull request refactors pagination by introducing a dynamic default limit. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Resource
participant QueryBuilder
participant PaginableTrait
Client->>Controller: Send pagination request
Controller->>Resource: Retrieve associated resource
Resource->>QueryBuilder: Provide $defaultLimit value
QueryBuilder->>PaginableTrait: Call paginate() with dynamic limit
PaginableTrait-->>QueryBuilder: Return paginated data
QueryBuilder-->>Resource: Forward paginated data
Resource-->>Controller: Deliver response data
Controller-->>Client: Return JSON response with pagination info
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thanks for the proposal, this needs unit tests / doc update please Be careful about ScoutBuilder line 52, there is still a 50 limit going on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Feature/Controllers/SearchPaginateOperationsTest.php (1)
86-102: Good test coverage for the new default limit feature.This test appropriately validates the default pagination behavior by ensuring:
- The correct default limit (32) from ModelResource is applied when no limit is specified
- Pagination metadata is properly calculated (100 items ÷ 32 per page = 4 pages)
- The correct number of items is returned in the response
Consider adding another test case that explicitly sends an empty search object (
{'search': {}}) to ensure both scenarios work correctly:public function test_to_get_a_list_of_paginated_resources_with_empty_search_object(): void { ModelFactory::new()->count(100)->create()->fresh(); Gate::policy(Model::class, GreenPolicy::class); $response = $this->post( '/api/models/search', [ 'search' => [], ], ['Accept' => 'application/json'] ); $response->assertStatus(200); $response->assertJsonPath('per_page', 32); $response->assertJsonPath('last_page', 4); $response->assertJsonCount(32, 'data'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/Feature/Controllers/SearchPaginateOperationsTest.php(3 hunks)tests/Support/Rest/Resources/ModelResource.php(1 hunks)
🔇 Additional comments (2)
tests/Support/Rest/Resources/ModelResource.php (1)
35-35: Appropriate implementation of the default limit feature.This addition aligns with the PR objective of introducing a customizable default pagination limit for specific models. Setting it to 32 (different from the global default of 50) provides a good test case for model-specific limits.
tests/Feature/Controllers/SearchPaginateOperationsTest.php (1)
46-46: LGTM - Whitespace improvements.These formatting changes improve code consistency without affecting functionality.
Also applies to: 72-72
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Query/ScoutBuilder.php (1)
21-21: Update docblock comment to reflect new default limit behavior.The docblock comment in line 21 still refers to "defaulting to 50 if no limit is specified" but the implementation now uses a dynamic default limit from the resource. Update the documentation to match the implementation.
- * defaulting to 50 if no limit is specified. Any extra parameters, after excluding + * defaulting to the resource's defaultLimit (or 50 if not set). Any extra parameters, after excluding
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Console/stubs/resource.stub(1 hunks)src/Console/stubs/user-resource.stub(1 hunks)src/Http/Resource.php(1 hunks)src/Query/ScoutBuilder.php(1 hunks)tests/Support/Rest/Resources/ModelResource.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Console/stubs/resource.stub
- src/Http/Resource.php
- tests/Support/Rest/Resources/ModelResource.php
- src/Console/stubs/user-resource.stub
🧰 Additional context used
🧬 Code Definitions (1)
src/Query/ScoutBuilder.php (1)
src/Http/Response.php (1)
resource(26-31)
🔇 Additional comments (1)
src/Query/ScoutBuilder.php (1)
52-53: LGTM! Great implementation of dynamic default limit.The implementation correctly retrieves the default limit from the resource model, falling back to 50 when not specified. This change aligns perfectly with the PR objective to allow customizable pagination limits per model.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Support/Http/Controllers/ModelWithDefaultLimitController.php (1)
7-7: Remove unused importThe
ModelWithResourceimport doesn't appear to be used in this controller.-use Lomkit\Rest\Tests\Support\Rest\Resources\ModelWithResource;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/Feature/Controllers/SearchPaginateOperationsTest.php(1 hunks)tests/Support/Http/Controllers/ModelWithDefaultLimitController.php(1 hunks)tests/Support/Rest/Resources/ModelResource.php(1 hunks)tests/Support/Rest/Resources/ModelWithDefaultLimitResource.php(1 hunks)tests/Support/Routes/api.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/Support/Rest/Resources/ModelResource.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Feature/Controllers/SearchPaginateOperationsTest.php
🧰 Additional context used
🧬 Code Definitions (1)
tests/Support/Rest/Resources/ModelWithDefaultLimitResource.php (4)
src/Http/Resource.php (1)
Resource(21-185)src/Http/Requests/RestRequest.php (1)
RestRequest(9-23)tests/Support/Models/Model.php (1)
Model(10-112)tests/Support/Rest/Resources/ModelResource.php (1)
ModelResource(29-160)
🔇 Additional comments (3)
tests/Support/Routes/api.php (1)
10-10: LGTM - Proper route registration for the new resourceThe route registration follows the established pattern and correctly defines the new resource route for 'model-with-default-limit', associating it with the appropriate controller.
tests/Support/Rest/Resources/ModelWithDefaultLimitResource.php (1)
29-32: LGTM - Properly implemented custom default limitThe
ModelWithDefaultLimitResourceclass correctly extendsModelResourceand implements a custom default limit of 32, overriding the baseResourceclass default of 50. This provides a good test case for the new pagination limit feature.tests/Support/Http/Controllers/ModelWithDefaultLimitController.php (1)
9-12: LGTM - Controller properly connected to resourceThe controller correctly extends the base Controller class and properly associates with the
ModelWithDefaultLimitResourceclass through the static $resource property.
|
Thanks, merging ! |
closes #157
This PR adds the possibility of defining a default pagnination limit for a specific model.
If not defined, the value remains 50.
I took the opportunity to update the
Resourcemodels with this new feature.It will be necessary to update the doc according to.
Summary by CodeRabbit
New Features
Tests