Fixes #360 - Fix puppetclass keyword search #364
Conversation
nadjaheitmann
force-pushed
the
360_hostgroup_search_fail
branch
from
3634ba5
to
e898842
Compare
|
@stejskalleos Search does work and the UI issue is fixed but the autocomplete still does not work and fails with |
|
I opened a follow-up issue for the auto completion #365 |
| @@ -17,6 +17,7 @@ class << self | |||
| # will need through relation to work properly | |||
| scoped_search relation: :environment, on: :name, complete_value: true, rename: :environment, only_explicit: true | |||
| scoped_search relation: :puppetclasses, on: :name, complete_value: true, rename: :class, only_explicit: true, operators: ['= ', '~ '] | |||
There was a problem hiding this comment.
Do we want to keep the class search parameter or it's time to remove it?
There was a problem hiding this comment.
Personally, I would remove it but then we should also remove it for hosts. This then partly implements fixes for #363
|
|
||
| def search_by_puppetclass(_key, operator, value) | ||
| conditions = sanitize_sql_for_conditions(["puppetclasses.name #{operator} ?", value_to_sql(operator, value)]) | ||
| hostgroup_ids = ::Hostgroup.unscoped.with_taxonomy_scope.joins(puppet: :puppetclasses).where(conditions).map(&:subtree_ids) |
There was a problem hiding this comment.
The hostgroup model is by default scoped under taxonomies, you can do just ::Hostgroup.joins(puppet: :puppetclasses).where(conditions).map(&:subtree_ids)
| hostgroup_ids = ::Hostgroup.unscoped.with_taxonomy_scope.joins(puppet: :puppetclasses).where(conditions).map(&:subtree_ids) | ||
| conds = [] | ||
| conds << "hostgroups.id IN (#{hostgroup_ids.join(',')})" if hostgroup_ids.present? | ||
| { conditions: conds.join(' OR ').presence || 'hostgroups.id < 0' } |
There was a problem hiding this comment.
Nit: Empty line between conds << and returning value
There was a problem hiding this comment.
Well, then let's do even more nitpicking and adjust also the formatting and naming scheme in lines 31ff?
There was a problem hiding this comment.
@stejskalleos If I look at this, I wonder why hostgroup search for puppetclass looks different from the one for config_groups.
| @@ -17,6 +17,7 @@ class << self | |||
| # will need through relation to work properly | |||
| scoped_search relation: :environment, on: :name, complete_value: true, rename: :environment, only_explicit: true | |||
| scoped_search relation: :puppetclasses, on: :name, complete_value: true, rename: :class, only_explicit: true, operators: ['= ', '~ '] | |||
| scoped_search relation: :puppetclasses, on: :name, complete_value: true, rename: :puppetclass, only_explicit: true, operators: ['= ', '~ '], ext_method: :search_by_puppetclass | |||
There was a problem hiding this comment.
Nit: Line is pretty long, not much readable IMHO
|
@stejskalleos Beware that the PR currently contains a fixup commit. |
nadjaheitmann
force-pushed
the
360_hostgroup_search_fail
branch
2 times, most recently
from
88f9978
to
ff72761
Compare
nadjaheitmann
force-pushed
the
360_hostgroup_search_fail
branch
from
ff72761
to
0645712
Compare
@stejskalleos Please re-review. |
|
Thanks @nadjaheitmann ! |
Actually, it implements the puppetclass keyword that should have replaced the class keyword in Foreman 3.2.
I will create a follow-up PR that fixes #363 and remove the class search completely.