Refactor #16798

Move scoped_search definitions from parent STI classes to subclasses

Added by Dominic Cleal 12 months ago. Updated 10 months ago.

Status:Closed
Priority:Normal
Assigned To:Dominic Cleal
Category:Search
Target version:-
Difficulty: Bugzilla link:
Found in release: Pull request:https://github.com/theforeman/foreman/pull/3917
Story points-
Velocity based estimate-
Release1.14.0Release relationshipAuto

Description

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:

CommonParametersControllerTest#test_index:
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 https://github.com/rails/rails/issues/18806 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 (https://github.com/wvanbergen/scoped_search/issues/112 + 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
Added by Dominic Cleal 11 months 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.

History

#1 Updated by The Foreman Bot 12 months ago

  • Status changed from Assigned to Ready For Testing
  • Pull request https://github.com/theforeman/foreman/pull/3917 added

#2 Updated by Dominic Cleal 10 months ago

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

#3 Updated by Dominic Cleal 10 months ago

  • Release set to 1.14.0

Also available in: Atom PDF