Project

General

Profile

Bug #16512

Settings-related functional test failures

Added by Dominic Cleal over 3 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
High
Assignee:
Category:
Tests
Target version:
Difficulty:
Triaged:
Bugzilla link:
Fixed in Releases:
Found in Releases:

Description

Since #16491, 18 tests have started failing:

  1) Error:
SettingsControllerTest#test_0005_settings shouldnt include ones about locations when locations are disabled:
ActionView::Template::Error: undefined method `each' for nil:NilClass
    app/views/settings/index.html.erb:4:in `_app_views_settings_index_html_erb__462412887402587618_117671740'
    app/controllers/concerns/application_shared.rb:14:in `set_timezone'
    app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
    test/functional/settings_controller_test.rb:49:in `block in <class:SettingsControllerTest>'
  • Api::V1::SettingsControllerTest.test_0004_should parse string values
  • Api::V1::SettingsControllerTest.test_0002_should show individual record
  • Api::V1::SettingsControllerTest.test_0003_should update setting
  • Api::V2::SettingsControllerTest.test_0005_settings shouldnt include ones about organizations when organizations are disabled
  • Api::V2::SettingsControllerTest.test_0003_should not update setting
  • Api::V2::SettingsControllerTest.test_0006_settings shouldnt include ones about locations when locations are disabled
  • Api::V2::SettingsControllerTest.test_0001_should get index
  • Api::V2::SettingsControllerTest.test_0004_should parse string values
  • Api::V2::SettingsControllerTest.test_0002_should show individual record
  • Api::V2::UsersControllerTest.test_0005_shows default taxonomies on show response
  • Api::V2::UsersControllerTest.test_0002_should handle taxonomy with wrong id
  • LocationsControllerTest.test_0008_should display a warning if current location has been deleted
  • OrganizationsControllerTest.test_0009_should display a warning if current organization has been deleted
  • SettingsControllerTest.test_index
  • SettingsControllerTest.test_0002_does not render an old sti type setting
  • SettingsControllerTest.test_0005_settings shouldnt include ones about locations when locations are disabled
  • SettingsControllerTest.test_0004_settings shouldnt include ones about organizations when organizations are disabled
  • SettingsControllerTest.test_0001_can render a new sti type setting

Related issues

Related to Foreman - Bug #16491: upgrade rubocop to version 0.42Closed2016-09-08

Associated revisions

Revision 2e5f252a (diff)
Added by Dominic Cleal over 3 years ago

fixes #16512 - remove Setting#respond_to_missing?

respond_to_missing? changed how AR handles group_by calls on
collections, causing group_by to be interpreted as a scope on the model.
Rather than adding a database call to determine known setting names,
it's been removed again.

The method_missing implementation has been deprecated in favour of the
hash [] style method calls which are more idiomatic and avoids making
the respond_to? behaviour any more complex.

History

#1 Updated by Dominic Cleal over 3 years ago

  • Related to Bug #16491: upgrade rubocop to version 0.42 added

#2 Updated by Dominic Cleal over 3 years ago

  • Description updated (diff)

#3 Updated by Dominic Cleal over 3 years ago

The implementation of respond_to_missing? on Setting (and possibly other models) has caused a change of behaviour with group_by on AR collections. Previously:

> Setting::General.all.group_by(&:category)
2016-09-12T11:32:39  [sql] [D]   Setting::General Load (0.2ms)  SELECT "settings".* FROM "settings" WHERE "settings"."category" IN ('Setting::General')  ORDER BY "settings"."name" ASC
=> {"Setting::General"= ....

Now:

> Setting::General.all.group_by(&:category)
2016-09-12T11:31:08 0c55c282 [sql] [D]   Setting::General Load (0.2ms)  SELECT  "settings".* FROM "settings" WHERE "settings"."category" IN ('Setting::General') AND "settings"."name" = ?  ORDER BY "settings"."name" ASC LIMIT 1  [["name", "group_by"]]
=> nil

#4 Updated by Dominic Cleal over 3 years ago

  • Status changed from New to Assigned
  • Assignee set to Dominic Cleal

#5 Updated by The Foreman Bot over 3 years ago

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

#6 Updated by Dominic Cleal over 3 years ago

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

#7 Updated by Dominic Cleal over 3 years ago

A little more detail: Setting started returning true for .respond_to?(:group_by) after respond_to_missing? was implemented, indicating that you could call Setting.group_by (which just returned nil as there's no such setting).

When calling Setting.all.group_by, Rails prefers the method on the model over its own method as it's assumed to be a scope: https://github.com/rails/rails/blob/v4.2.7.1/activerecord/lib/active_record/relation/delegation.rb#L92.

Ideally respond_to? should only respond true for known settings, but performing a DB query here would be unnecessarily expensive. My commit removed the (correct) respond_to_missing? implementation and just deprecated the method_missing behaviour so it can all be removed in time.

Also available in: Atom PDF