matchish / laravel-scout-elasticsearch

Search among multiple models with ElasticSearch and Laravel Scout
MIT License
702 stars 113 forks source link

Feature: Add the makeAllSearchableUsing API to the default import source #253

Closed yocmen closed 1 year ago

yocmen commented 1 year ago

In this PR I'm adding the handle of the method makeAllSearchableUsing in the DefaultImportSourceto allow/match this to the scout docs.

https://laravel.com/docs/10.x/scout#modifying-the-import-query

I did this because I found a case where I needed to use the makeAllSearchableUsing method in a Model and found this was the easiest way to do it.

hkulekci commented 1 year ago

this means that for each query this method will work, right? What do you think, adding it with a condition instead of applying for each query?

yocmen commented 1 year ago

Hi @hkulekci, the Searchable Trait already have the makeAllSearchableUsing method just returning the query. The implementation is identical to the scout one (using when(true..) so any searchable model will have that method by default and ready to overwrite it in the model with new logic when you need it, I can add a "exists method" check if you want it, I just did it like the scout implementation and is working really good for me.

Thanks, and let me know anything :)

yocmen commented 1 year ago

hi @matchish any thoughts on this? :)

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 :tada:

Comparison is base (0c1b798) 96.06% compared to head (1b3754a) 96.08%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #253 +/- ## ============================================ + Coverage 96.06% 96.08% +0.01% Complexity 192 192 ============================================ Files 36 36 Lines 636 639 +3 ============================================ + Hits 611 614 +3 Misses 25 25 ``` | [Impacted Files](https://app.codecov.io/gh/matchish/laravel-scout-elasticsearch/pull/253?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sergey+Shlyakhov) | Coverage Δ | | |---|---|---| | [src/Searchable/DefaultImportSource.php](https://app.codecov.io/gh/matchish/laravel-scout-elasticsearch/pull/253?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sergey+Shlyakhov#diff-c3JjL1NlYXJjaGFibGUvRGVmYXVsdEltcG9ydFNvdXJjZS5waHA=) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

matchish commented 1 year ago

Hey) thanks for your effort to create the PR. Small improvement that should improve readability

        $query = $this->className::makeAllSearchableUsing($this->model()->newQuery());
        $softDelete = $this->className::usesSoftDelete() && config('scout.soft_delete', false);
        $query
            ->when($softDelete, function ($query) {
                return $query->withTrashed();
            })
            ->orderBy($this->model()->getQualifiedKeyName());
        $scopes = $this->scopes;

And could you add a test to cover the PR?

yocmen commented 1 year ago

Hi @matchish, I added a new Test, dont know if that was the right place, let me know.

In order to add the Test I added a new Post model, including migration and factory, on this model I added the makeAllSearchableUsing method modifing the query to be used when the data is imported.

This was the best way I found to make a test, I can't figure how to mock any existing model to assert if the method was called, so instead I added like this.

Please let me know anything, all help is welcome.

yocmen commented 1 year ago

Hi @matchish @hkulekci what do you think on my last changes?

joelennon commented 1 year ago

Thanks for this @yocmen

@matchish can you tag a new release please so we can use this change without having to lock to the latest commit? Really appreciate it and keep up the great work with this package!

matchish commented 1 year ago

released