From b5de5ac19ced7f7fd7b99653b3a4739bccda9af5 Mon Sep 17 00:00:00 2001 From: Till Deeke Date: Tue, 17 Jul 2018 02:44:31 +0200 Subject: [PATCH] Fix: Searching for multiple terms on assets (#5860) * Give advancedTextSearch all search terms at one The additional conditions for assets had some problems, since they were joining tables for the additional attributes. The method was called once for every search term, so the join was added multiple times if the user entered multiple search terms. * Allows search to handle multiple search terms better The search now better handles multiple search terms, adding additional orWhere clauses, instead of duplicating all queries. * Fixing typo --- app/Models/Asset.php | 37 +++++++--- app/Models/Traits/Searchable.php | 119 ++++++++++++++++--------------- app/Models/User.php | 9 ++- 3 files changed, 92 insertions(+), 73 deletions(-) diff --git a/app/Models/Asset.php b/app/Models/Asset.php index 85a4499a2e..891cfbb41a 100644 --- a/app/Models/Asset.php +++ b/app/Models/Asset.php @@ -621,10 +621,12 @@ class Asset extends Depreciable * Run additional, advanced searches. * * @param Illuminate\Database\Eloquent\Builder $query - * @param string $term The search term + * @param array $terms The search terms * @return Illuminate\Database\Eloquent\Builder */ - public function advancedTextSearch(Builder $query, string $term) { + public function advancedTextSearch(Builder $query, array $terms) { + + /** * Assigned user */ @@ -632,31 +634,44 @@ class Asset extends Depreciable $leftJoin->on("assets_users.id", "=", "assets.assigned_to") ->where("assets.assigned_type", "=", User::class); }); - $query = $query - ->orWhere('assets_users.first_name', 'LIKE', '%'.$term.'%') - ->orWhere('assets_users.last_name', 'LIKE', '%'.$term.'%') - ->orWhere('assets_users.username', 'LIKE', '%'.$term.'%') - ->orWhereRaw('CONCAT('.DB::getTablePrefix().'assets_users.first_name," ",'.DB::getTablePrefix().'assets_users.last_name) LIKE ?', ["%$term%", "%$term%"]); + + foreach($terms as $term) { + + $query = $query + ->orWhere('assets_users.first_name', 'LIKE', '%'.$term.'%') + ->orWhere('assets_users.last_name', 'LIKE', '%'.$term.'%') + ->orWhere('assets_users.username', 'LIKE', '%'.$term.'%') + ->orWhereRaw('CONCAT('.DB::getTablePrefix().'assets_users.first_name," ",'.DB::getTablePrefix().'assets_users.last_name) LIKE ?', ["%$term%", "%$term%"]); + + } /** * Assigned location - */ + */ $query = $query->leftJoin('locations as assets_locations',function ($leftJoin) { $leftJoin->on("assets_locations.id","=","assets.assigned_to") ->where("assets.assigned_type","=",Location::class); }); - $query = $query->orWhere('assets_locations.name', 'LIKE', '%'.$term.'%'); + foreach($terms as $term) { + + $query = $query->orWhere('assets_locations.name', 'LIKE', '%'.$term.'%'); + + } /** * Assigned assets - */ + */ $query = $query->leftJoin('assets as assigned_assets',function ($leftJoin) { $leftJoin->on('assigned_assets.id', '=', 'assets.assigned_to') ->where('assets.assigned_type', '=', Asset::class); }); - $query = $query->orWhere('assigned_assets.name', 'LIKE', '%'.$term.'%'); + foreach($terms as $term) { + + $query = $query->orWhere('assigned_assets.name', 'LIKE', '%'.$term.'%'); + + } return $query; } diff --git a/app/Models/Traits/Searchable.php b/app/Models/Traits/Searchable.php index 3039e291d8..b6438b824d 100644 --- a/app/Models/Traits/Searchable.php +++ b/app/Models/Traits/Searchable.php @@ -25,28 +25,25 @@ trait Searchable { { $terms = $this->prepeareSearchTerms($search); - foreach($terms as $term) { + /** + * Search the attributes of this model + */ + $query = $this->searchAttributes($query, $terms); - /** - * Search the attributes of this model - */ - $query = $this->searchAttributes($query, $term); + /** + * Search through the custom fields of the model + */ + $query = $this->searchCustomFields($query, $terms); - /** - * Search through the custom fields of the model - */ - $query = $this->searchCustomFields($query, $term); + /** + * Search through the relations of the model + */ + $query = $this->searchRelations($query, $terms); - /** - * Search through the relations of the model - */ - $query = $this->searchRelations($query, $term); - - /** - * Search for additional attributes defined by the model - */ - $query = $this->advancedTextSearch($query, $term); - } + /** + * Search for additional attributes defined by the model + */ + $query = $this->advancedTextSearch($query, $terms); return $query; } @@ -61,41 +58,43 @@ trait Searchable { } /** - * Searches the models attributes for the search term + * Searches the models attributes for the search terms * * @param Illuminate\Database\Eloquent\Builder $query - * @param string $term + * @param array $terms * @return Illuminate\Database\Eloquent\Builder */ - private function searchAttributes(Builder $query, string $term) { + private function searchAttributes(Builder $query, array $terms) { + + $table = $this->getTable(); foreach($this->getSearchableAttributes() as $column) { - /** - * Making sure to only search in date columns if the search term consists of characters that can make up a MySQL timestamp! - * - * @see https://github.com/snipe/snipe-it/issues/4590 - */ - if (!preg_match('/^[0-9 :-]++$/', $term) && in_array($column, $this->getDates())) { - continue; + foreach($terms as $term) { + /** + * Making sure to only search in date columns if the search term consists of characters that can make up a MySQL timestamp! + * + * @see https://github.com/snipe/snipe-it/issues/4590 + */ + if (!preg_match('/^[0-9 :-]++$/', $term) && in_array($column, $this->getDates())) { + continue; + } + + $query = $query->orWhere($table . '.' . $column, 'LIKE', '%'.$term.'%'); } - - $table = $this->getTable(); - - $query = $query->orWhere($table . '.' . $column, 'LIKE', '%'.$term.'%'); } return $query; } /** - * Searches the models custom fields for the search term + * Searches the models custom fields for the search terms * * @param Illuminate\Database\Eloquent\Builder $query - * @param string $term + * @param array $terms * @return Illuminate\Database\Eloquent\Builder */ - private function searchCustomFields(Builder $query, string $term) { + private function searchCustomFields(Builder $query, array $terms) { /** * If we are searching on something other that an asset, skip custom fields. @@ -104,51 +103,53 @@ trait Searchable { return $query; } - foreach (CustomField::all() as $field) { - $query->orWhere($this->getTable() . '.'. $field->db_column_name(), 'LIKE', '%'.$term.'%'); + $customFields = CustomField::all(); + + foreach ($customFields as $field) { + foreach($terms as $term) { + $query->orWhere($this->getTable() . '.'. $field->db_column_name(), 'LIKE', '%'.$term.'%'); + } } return $query; } /** - * Searches the models relations for the search term + * Searches the models relations for the search terms * * @param Illuminate\Database\Eloquent\Builder $query - * @param string $term + * @param array $terms * @return Illuminate\Database\Eloquent\Builder */ - private function searchRelations(Builder $query, string $term) { + private function searchRelations(Builder $query, array $terms) { foreach($this->getSearchableRelations() as $relation => $columns) { - /** - * Make the columns into a collection, - * for easier handling further down - * - * @var Illuminate\Support\Collection - */ - $columns = collect($columns); - - $query = $query->orWhereHas($relation, function($query) use ($relation, $columns, $term) { + $query = $query->orWhereHas($relation, function($query) use ($relation, $columns, $terms) { $table = $this->getRelationTable($relation); - + /** * We need to form the query properly, starting with a "where", * otherwise the generated nested select is wrong. * - * We can just choose the last column, since they all get "and where"d in the end. - * (And because using pop saves us from handling the removal of the first element) + * @todo This does the job, but is inelegant and fragile */ - $last = $columns->pop(); - $query->where($table . '.' . $last, 'LIKE', '%'.$term.'%'); + $firstConditionAdded = false; foreach($columns as $column) { - $query->orWhere($table . '.' . $column, 'LIKE', '%'.$term.'%'); + foreach($terms as $term) { + if (!$firstConditionAdded) { + $query->where($table . '.' . $column, 'LIKE', '%'.$term.'%'); + $firstConditionAdded = true; + continue; + } + + $query->orWhere($table . '.' . $column, 'LIKE', '%'.$term.'%'); + } } - }); + } return $query; @@ -160,12 +161,12 @@ trait Searchable { * This is a noop in this trait, but can be overridden in the implementing model, to allow more advanced searches * * @param Illuminate\Database\Eloquent\Builder $query - * @param string $term The search term + * @param array $terms The search terms * @return Illuminate\Database\Eloquent\Builder * * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ - public function advancedTextSearch(Builder $query, string $term) { + public function advancedTextSearch(Builder $query, array $terms) { return $query; } diff --git a/app/Models/User.php b/app/Models/User.php index a915a53d58..9ba915e5b1 100755 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -440,12 +440,15 @@ class User extends SnipeModel implements AuthenticatableContract, CanResetPasswo * Run additional, advanced searches. * * @param Illuminate\Database\Eloquent\Builder $query - * @param string $term The search term + * @param array $term The search terms * @return Illuminate\Database\Eloquent\Builder */ - public function advancedTextSearch(Builder $query, string $term) { - $query = $query->orWhereRaw('CONCAT('.DB::getTablePrefix().'users.first_name," ",'.DB::getTablePrefix().'users.last_name) LIKE ?', ["%$term%", "%$term%"]); + public function advancedTextSearch(Builder $query, array $terms) { + foreach($terms as $term) { + $query = $query->orWhereRaw('CONCAT('.DB::getTablePrefix().'users.first_name," ",'.DB::getTablePrefix().'users.last_name) LIKE ?', ["%$term%", "%$term%"]); + } + return $query; }