Refactor #16798

Move scoped_search definitions from parent STI classes to subclasses

Added by Dominic Cleal about 4 years ago. Updated over 2 years ago.

Target version:
Bugzilla link:
Fixed in Releases:
Found in Releases:


Under Rails 5, the CommonParameter indexes ("Global parameters") show all types of parameters and not only CommonParameters, as CommonParameter.search_for (with no string) returns Parameter.all.

Tests actually fail as HostParameters are in fixtures without corresponding hosts:

ActionView::Template::Error: undefined method `id' for nil:NilClass
app/models/parameters/host_parameter.rb:8:in `to_s'
app/helpers/application_helper.rb:31:in `link_to'
app/helpers/application_helper.rb:116:in `link_to_if_authorized'
app/views/common_parameters/index.html.erb:18:in `block in app_views_common_parameters_index_html_erb__2292157232256250179_107176060'
app/views/common_parameters/index.html.erb:16:in `_app_views_common_parameters_index_html_erb___2292157232256250179_107176060'
app/controllers/concerns/application_shared.rb:14:in `set_timezone'
app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
test/controllers/common_parameters_controller_test.rb:5:in `test_index'

(The common parameters controller should never be rendering a HostParameter.)

When calling CommonParameter.search_for, scoped_search's scope will call Parameter.all as the search definition is registered on Parameter. Under Rails 4, it would apply the current scope (where type='CommonParameter') to all related STI classes, so even explicitly calling scopes on Parameter are filtered by type='CommonParameter' while within the scoped_definition scope called through CommonParameter.

This was reverted in Rails ticket in 5.0.0. Now when calling CommonParameter.search_for, scoped_search calls Parameter.all again, and it returns every Parameter subclass.

scoped_search doesn't support STI - registering a definition on a parent class and expecting it to work with subclasses - and there are known bugs with it already ( + related). Moving scoped search definitions to the subclasses and not relying on inheritance would be more reliable until that changes. LookupKey already does this through defining self.inherited to work around some of these bugs.

For this issue, moving the definition to CommonParameter would cause scoped_search to call CommonParameter.all instead, so it doesn't rely on the current scope.

Associated revisions

Revision 08763fc2 (diff)
Added by Dominic Cleal about 4 years ago

fixes #16798 - move scoped_search definitions to STI subclasses

scoped_search doesn't support class inheritance with STI, so registering
definitions on the subclass fixes various issues. This fixes an issue
where scoped_search on CommonParameter calls Parameter.all and is
returned a list of CommonParameters under Rails 4.2, as it relies on a
bug (#18806) for applying the type='CommonParameter' condition when
calling Parameter.all.

On Rails 5.0, this bug was fixed and calling Parameter.all within a
scope on CommonParameter now returns all types of parameters.
Registering all scoped_search definitions on the subclasses ensures that
scoped_search calls CommonParameter.all instead.

The taxonomies API changed as it called Taxonomix objects with scopes
such as `.locations.search_for`, which no longer existed when the
scoped_search definitions were removed from Taxonomy (.locations is a
Taxonomy association with a where clause). This now explicitly searches
via the appropriate Taxonomy subclass.


#1 Updated by The Foreman Bot about 4 years ago

  • Status changed from Assigned to Ready For Testing
  • Pull request added

#2 Updated by Dominic Cleal about 4 years ago

  • Status changed from Ready For Testing to Closed
  • % Done changed from 0 to 100

#3 Updated by Dominic Cleal about 4 years ago

  • Legacy Backlogs Release (now unused) set to 189

Also available in: Atom PDF