✨ scout with and only trashed #186
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Warning Rate limit exceeded@GautierDele has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 45 seconds before requesting another review. ⌛ 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 (2)
WalkthroughAdds soft-delete filtering support to Scout searches by introducing applyTrashed handling in ScoutBuilder, validates a new search.text.trashed parameter, and adds unit/feature tests for 'with' and 'only' trashed cases and validation for unknown values. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Search Controller
participant SB as ScoutBuilder
participant Scout as Laravel Scout
participant Index as Search Index/DB
Client->>API: POST /.../search { search.text.value, search.text.trashed }
API->>SB: search(parameters)
SB->>SB: applyFilters(...)
SB->>SB: applyTrashed(trashed)
alt trashed == "with"
SB->>Scout: withTrashed()
else trashed == "only"
SB->>Scout: onlyTrashed()
else
SB->>Scout: (no trashed modifier)
end
SB->>SB: applySorts(...), applyInstructions(...)
Scout->>Index: Execute query
Index-->>API: Results
API-->>Client: Paginated response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests✅ Copyable Unit Test edits generated.
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Query/ScoutBuilder.php (2)
132-139: Consider handling unknown trashed values explicitly.The current implementation silently ignores unknown trashed values. While validation should prevent this, consider adding explicit handling for robustness.
public function applyTrashed(string $trashed): void { if ($trashed === 'only') { $this->queryBuilder->onlyTrashed(); } elseif ($trashed === 'with') { $this->queryBuilder->withTrashed(); + } else { + throw new InvalidArgumentException("Invalid trashed parameter: {$trashed}. Expected 'with' or 'only'."); } }
25-32: Update docstring to include trashed parameter.The method docstring should be updated to document the new trashed functionality.
* @param array $parameters An associative array of search criteria, which may include: - * - 'text': The search query string. + * - 'text': An array containing 'value' (search string) and optionally 'trashed' ('with'|'only'). * - 'filters': An array of filter conditions. * - 'sorts': An array of sorting directives. * - 'instructions': Additional query instructions. * - 'limit': Maximum number of results to return (defaults to 50 if not provided).tests/Feature/Controllers/SearchScoutOperationsTest.php (1)
430-455: Address the TODO comment and enhance test coverage.The TODO comment indicates this test doesn't verify that the 'only' functionality works correctly. Consider enhancing the test to actually verify soft-delete behavior.
public function test_getting_a_list_of_resources_only_trashed(): void { - ModelFactory::new()->count(2)->create(); + $activeModels = ModelFactory::new()->count(2)->create(); + $trashedModels = ModelFactory::new()->count(2)->create(); + $trashedModels->each->delete(); // Soft delete these models Gate::policy(Model::class, GreenPolicy::class); $response = $this->post( '/api/searchable-models/search', [ 'search' => [ 'text' => [ 'trashed' => 'only', ], ], ], ['Accept' => 'application/json'] ); - // @TODO: test only correctly applyed - $this->assertResourcePaginated( $response, - [], + $trashedModels->toArray(), // Should only return trashed models new ModelResource() ); }Do you want me to help create a more comprehensive test that actually verifies the soft-delete filtering behavior?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Query/ScoutBuilder.php(2 hunks)src/Rules/Search/SearchText.php(2 hunks)tests/Feature/Controllers/SearchScoutOperationsTest.php(1 hunks)tests/Unit/LaravelScoutTest.php(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/Feature/Controllers/SearchScoutOperationsTest.php (2)
tests/Support/Policies/GreenPolicy.php (1)
GreenPolicy(8-100)tests/Support/Traits/InteractsWithResource.php (1)
assertResourcePaginated(13-47)
src/Rules/Search/SearchText.php (2)
src/Rules/Search/SearchFilter.php (2)
buildValidationRules(12-59)SearchFilter(10-60)src/Rules/Search/Search.php (1)
buildValidationRules(11-36)
tests/Unit/LaravelScoutTest.php (1)
src/Query/ScoutBuilder.php (3)
toBase(200-203)ScoutBuilder(11-204)search(34-77)
🔇 Additional comments (6)
src/Rules/Search/SearchText.php (1)
21-23: Add 'sometimes' validation rule for optional trashed parameter.The trashed parameter should be optional since not all searches need to filter by soft-deleted records. Add 'sometimes' to the validation rules to make it optional.
- $attribute.'.trashed' => [ - Rule::in('with', 'only'), - ], + $attribute.'.trashed' => [ + 'sometimes', + Rule::in('with', 'only'), + ],Likely an incorrect or invalid review comment.
tests/Unit/LaravelScoutTest.php (2)
99-143: LGTM! Comprehensive unit test coverage for trashed functionality.The new unit tests properly cover both 'with' and 'only' trashed scenarios. The tests correctly:
- Mock ScoutBuilder dependencies
- Verify applyTrashed is called with the correct parameter
- Ensure other apply methods are not invoked when not needed
- Follow existing test patterns in the file
50-50: LGTM! Input format refactoring improves consistency.The change from
text => [['value' => 'test']]totext => ['value' => 'test']makes the input format more intuitive and consistent with the new trashed parameter structure.Also applies to: 70-70, 90-90
src/Query/ScoutBuilder.php (1)
44-46: LGTM! Proper integration of trashed functionality.The trashed parameter is correctly integrated into the search flow after filters and before sorts, which is the appropriate order for applying soft-delete filtering.
tests/Feature/Controllers/SearchScoutOperationsTest.php (2)
383-403: LGTM! Proper validation test for unknown trashed values.The test correctly verifies that unknown trashed values result in a 422 validation error with the appropriate error structure.
405-428: LGTM! Test structure is correct.The test properly verifies that the 'with' trashed parameter is accepted and returns a successful paginated response.
Contributor
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
coderabbitai bot
added a commit
that referenced
this pull request
Aug 11, 2025
Docstrings generation was requested by @GautierDele. * #186 (comment) The following files were modified: * `src/Query/ScoutBuilder.php` * `src/Rules/Search/SearchText.php`
Contributor
|
Note Generated docstrings for this pull request at #187 |
Contributor
|
Here are the copyable unit test edits: Copyable Editstests/Feature/Controllers/SearchScoutOperationsAdditionalTest.phpThis is a new file. tests/Unit/LaravelScoutTest.php@@ -141,4 +141,208 @@
($scoutQueryBuilderMock->toBase()->queryCallback)(Model::query());
}
+}
+ public function test_building_scout_with_all_parts_combined()
+ {
+ // Framework: PHPUnit with Mockery
+ \Illuminate\Support\Facades\Auth::setUser(\Mockery::mock(\Lomkit\Rest\Tests\Support\Models\User::class));
+ \Illuminate\Support\Facades\Gate::policy(\Lomkit\Rest\Tests\Support\Models\Model::class, \Lomkit\Rest\Tests\Support\Policies\GreenPolicy::class);
+
+ $scoutQueryBuilderMock = \Mockery::mock(\Lomkit\Rest\Query\ScoutBuilder::class, [new \Lomkit\Rest\Tests\Support\Rest\Resources\SearchableModelResource()])->makePartial();
+
+ $expectedFilters = [
+ ['field' => 'status', 'value' => 'active'],
+ ['field' => 'priority', 'value' => 5],
+ ];
+ $expectedSorts = [
+ ['field' => 'created_at', 'direction' => 'desc'],
+ ['field' => 'id', 'direction' => 'asc'],
+ ];
+ $expectedInstructions = [
+ ['name' => 'boost_recent'],
+ ['name' => 'exclude_flagged'],
+ ];
+ $expectedTrashed = 'with';
+
+ $scoutQueryBuilderMock->shouldReceive('applyFilters')->with($expectedFilters)->once();
+ $scoutQueryBuilderMock->shouldReceive('applySorts')->with($expectedSorts)->once();
+ $scoutQueryBuilderMock->shouldReceive('applyInstructions')->with($expectedInstructions)->once();
+ $scoutQueryBuilderMock->shouldReceive('applyTrashed')->with($expectedTrashed)->once();
+
+ $scoutQueryBuilderMock->search([
+ 'text' => ['value' => 'combine all'],
+ 'filters' => $expectedFilters,
+ 'sorts' => $expectedSorts,
+ 'instructions' => $expectedInstructions,
+ // trashed lives under text
+ 'text_extra' => 'ignored',
+ 'unknown' => 'also_ignored',
+ 'text_details' => 'also_ignored',
+ 'text_options' => ['foo' => 'bar'],
+ 'meta' => ['should' => 'not break'],
+ 'text' => [
+ 'value' => 'combine all',
+ 'trashed' => $expectedTrashed,
+ ],
+ ]);
+
+ // Verify the underlying base query text and the callback are still accessible
+ $this->assertSame('combine all', $scoutQueryBuilderMock->toBase()->query);
+ $this->assertIsCallable($scoutQueryBuilderMock->toBase()->queryCallback);
+ }
+
+ public function test_building_scout_with_multiple_filters_only()
+ {
+ // Framework: PHPUnit with Mockery
+ \Illuminate\Support\Facades\Auth::setUser(\Mockery::mock(\Lomkit\Rest\Tests\Support\Models\User::class));
+ \Illuminate\Support\Facades\Gate::policy(\Lomkit\Rest\Tests\Support\Models\Model::class, \Lomkit\Rest\Tests\Support\Policies\GreenPolicy::class);
+
+ $scoutQueryBuilderMock = \Mockery::mock(\Lomkit\Rest\Query\ScoutBuilder::class, [new \Lomkit\Rest\Tests\Support\Rest\Resources\SearchableModelResource()])->makePartial();
+
+ $expectedFilters = [
+ ['field' => 'category', 'value' => 'books'],
+ ['field' => 'stock', 'value' => true],
+ ['field' => 'rating', 'value' => 4.5],
+ ];
+
+ $scoutQueryBuilderMock->shouldReceive('applyFilters')->with($expectedFilters)->once();
+ $scoutQueryBuilderMock->shouldReceive('applySorts')->with([])->never();
+ $scoutQueryBuilderMock->shouldReceive('applyInstructions')->with([])->never();
+
+ $scoutQueryBuilderMock->search([
+ 'text' => ['value' => 'inventory'],
+ 'filters' => $expectedFilters,
+ ]);
+
+ $this->assertSame('inventory', $scoutQueryBuilderMock->toBase()->query);
+ }
+
+ public function test_building_scout_with_multiple_sorts_only()
+ {
+ // Framework: PHPUnit with Mockery
+ \Illuminate\Support\Facades\Auth::setUser(\Mockery::mock(\Lomkit\Rest\Tests\Support\Models\User::class));
+ \Illuminate\Support\Facades\Gate::policy(\Lomkit\Rest\Tests\Support\Models\Model::class, \Lomkit\Rest\Tests\Support\Policies\GreenPolicy::class);
+
+ $scoutQueryBuilderMock = \Mockery::mock(\Lomkit\Rest\Query\ScoutBuilder::class, [new \Lomkit\Rest\Tests\Support\Rest\Resources\SearchableModelResource()])->makePartial();
+
+ $expectedSorts = [
+ ['field' => 'price', 'direction' => 'desc'],
+ ['field' => 'name', 'direction' => 'asc'],
+ ];
+
+ $scoutQueryBuilderMock->shouldReceive('applyFilters')->with([])->never();
+ $scoutQueryBuilderMock->shouldReceive('applySorts')->with($expectedSorts)->once();
+ $scoutQueryBuilderMock->shouldReceive('applyInstructions')->with([])->never();
+
+ $scoutQueryBuilderMock->search([
+ 'text' => ['value' => 'gadgets'],
+ 'sorts' => $expectedSorts,
+ ]);
+
+ $this->assertSame('gadgets', $scoutQueryBuilderMock->toBase()->query);
+ }
+
+ public function test_building_scout_with_multiple_instructions_only()
+ {
+ // Framework: PHPUnit with Mockery
+ \Illuminate\Support\Facades\Auth::setUser(\Mockery::mock(\Lomkit\Rest\Tests\Support\Models\User::class));
+ \Illuminate\Support\Facades\Gate::policy(\Lomkit\Rest\Tests\Support\Models\Model::class, \Lomkit\Rest\Tests\Support\Policies\GreenPolicy::class);
+
+ $scoutQueryBuilderMock = \Mockery::mock(\Lomkit\Rest\Query\ScoutBuilder::class, [new \Lomkit\Rest\Tests\Support\Rest\Resources\SearchableModelResource()])->makePartial();
+
+ $expectedInstructions = [
+ ['name' => 'prefer_in_stock'],
+ ['name' => 'demote_backordered'],
+ ];
+
+ $scoutQueryBuilderMock->shouldReceive('applyFilters')->with([])->never();
+ $scoutQueryBuilderMock->shouldReceive('applySorts')->with([])->never();
+ $scoutQueryBuilderMock->shouldReceive('applyInstructions')->with($expectedInstructions)->once();
+
+ $scoutQueryBuilderMock->search([
+ 'text' => ['value' => 'status'],
+ 'instructions' => $expectedInstructions,
+ ]);
+
+ // Invoke the callback to mirror existing pattern
+ ($scoutQueryBuilderMock->toBase()->queryCallback)(\Lomkit\Rest\Tests\Support\Models\Model::query());
+ }
+
+ public function test_building_scout_with_invalid_trashed_value_should_not_call_applyTrashed()
+ {
+ // Framework: PHPUnit with Mockery
+ \Illuminate\Support\Facades\Auth::setUser(\Mockery::mock(\Lomkit\Rest\Tests\Support\Models\User::class));
+ \Illuminate\Support\Facades\Gate::policy(\Lomkit\Rest\Tests\Support\Models\Model::class, \Lomkit\Rest\Tests\Support\Policies\GreenPolicy::class);
+
+ $scoutQueryBuilderMock = \Mockery::mock(\Lomkit\Rest\Query\ScoutBuilder::class, [new \Lomkit\Rest\Tests\Support\Rest\Resources\SearchableModelResource()])->makePartial();
+
+ $scoutQueryBuilderMock->shouldReceive('applyFilters')->with([])->never();
+ $scoutQueryBuilderMock->shouldReceive('applySorts')->with([])->never();
+ $scoutQueryBuilderMock->shouldReceive('applyInstructions')->with([])->never();
+ // Invalid value should result in no call to applyTrashed
+ $scoutQueryBuilderMock->shouldReceive('applyTrashed')->never();
+
+ $scoutQueryBuilderMock->search([
+ 'text' => [
+ 'value' => 'invalid trashed',
+ 'trashed' => 'invalid_option',
+ ],
+ ]);
+
+ // The query should still be set to provided text
+ $this->assertSame('invalid trashed', $scoutQueryBuilderMock->toBase()->query);
+ }
+
+ public function test_building_scout_without_text_value_sets_empty_query_and_skips_mutations()
+ {
+ // Framework: PHPUnit with Mockery
+ \Illuminate\Support\Facades\Auth::setUser(\Mockery::mock(\Lomkit\Rest\Tests\Support\Models\User::class));
+ \Illuminate\Support\Facades\Gate::policy(\Lomkit\Rest\Tests\Support\Models\Model::class, \Lomkit\Rest\Tests\Support\Policies\GreenPolicy::class);
+
+ $scoutQueryBuilderMock = \Mockery::mock(\Lomkit\Rest\Query\ScoutBuilder::class, [new \Lomkit\Rest\Tests\Support\Rest\Resources\SearchableModelResource()])->makePartial();
+
+ $scoutQueryBuilderMock->shouldReceive('applyFilters')->with([])->never();
+ $scoutQueryBuilderMock->shouldReceive('applySorts')->with([])->never();
+ $scoutQueryBuilderMock->shouldReceive('applyInstructions')->with([])->never();
+ $scoutQueryBuilderMock->shouldReceive('applyTrashed')->never();
+
+ // Provide an empty 'text' array or missing 'value'
+ $scoutQueryBuilderMock->search([
+ 'text' => [],
+ ]);
+
+ // We cannot be certain of default behavior; assert it's either empty string or null.
+ $this->assertTrue(
+ $scoutQueryBuilderMock->toBase()->query === '' ||
+ $scoutQueryBuilderMock->toBase()->query === null,
+ 'Expected empty or null query when no text value provided.'
+ );
+ }
+
+ public function test_building_scout_ignores_unknown_top_level_keys()
+ {
+ // Framework: PHPUnit with Mockery
+ \Illuminate\Support\Facades\Auth::setUser(\Mockery::mock(\Lomkit\Rest\Tests\Support\Models\User::class));
+ \Illuminate\Support\Facades\Gate::policy(\Lomkit\Rest\Tests\Support\Models\Model::class, \Lomkit\Rest\Tests\Support\Policies\GreenPolicy::class);
+
+ $scoutQueryBuilderMock = \Mockery::mock(\Lomkit\Rest\Query\ScoutBuilder::class, [new \Lomkit\Rest\Tests\Support\Rest\Resources\SearchableModelResource()])->makePartial();
+
+ $scoutQueryBuilderMock->shouldReceive('applyFilters')->with([])->never();
+ $scoutQueryBuilderMock->shouldReceive('applySorts')->with([])->never();
+ $scoutQueryBuilderMock->shouldReceive('applyInstructions')->with([])->never();
+ $scoutQueryBuilderMock->shouldReceive('applyTrashed')->never();
+
+ $scoutQueryBuilderMock->search([
+ 'text' => ['value' => 'just text'],
+ 'page' => 5,
+ 'perPage' => 50,
+ 'cursor' => 'abc123',
+ 'random' => ['nested' => 'value'],
+ 'metadata' => 'ignored',
+ ]);
+
+ $this->assertSame('just text', $scoutQueryBuilderMock->toBase()->query);
+ $this->assertIsCallable($scoutQueryBuilderMock->toBase()->queryCallback);
+ }
+
}tests/Unit/ScoutBuilderTest.phpThis is a new file. tests/Unit/SearchTextTest.phpThis is a new file. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #185
Summary by CodeRabbit