Project

General

Profile

Actions

Refactor #16798

closed

Move scoped_search definitions from parent STI classes to subclasses

Added by Dominic Cleal over 7 years ago. Updated almost 6 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Search
Target version:
Difficulty:
Triaged:
Fixed in Releases:
Found in Releases:

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.

Actions #1

Updated by The Foreman Bot over 7 years ago

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

Updated by Dominic Cleal over 7 years ago

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

Updated by Dominic Cleal over 7 years ago

  • translation missing: en.field_release set to 189
Actions

Also available in: Atom PDF