Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jul 26, 2023

Conversation

yocmen
Copy link
Contributor

@yocmen yocmen commented Jul 7, 2023

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
Copy link
Contributor

hkulekci commented Jul 9, 2023

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
Copy link
Contributor Author

yocmen commented Jul 10, 2023

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
Copy link
Contributor Author

yocmen commented Jul 20, 2023

hi @matchish any thoughts on this? :)

@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2023

Codecov Report

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

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

❗ 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
@@             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 Coverage Δ
src/Searchable/DefaultImportSource.php 100.00% <100.00%> (ø)

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

@matchish
Copy link
Owner

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
Copy link
Contributor Author

yocmen commented Jul 21, 2023

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
Copy link
Contributor Author

yocmen commented Jul 26, 2023

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

@matchish matchish merged commit 1f4ce15 into matchish:master Jul 26, 2023
5 of 6 checks passed
@yocmen yocmen deleted the yocmen/modify-import-query branch July 26, 2023 14:29
@joelennon
Copy link

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
Copy link
Owner

released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants