Bug #16512
closedSettings-related functional test failures
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
Updated by Dominic Cleal over 8 years ago
- Related to Bug #16491: upgrade rubocop to version 0.42 added
Updated by Dominic Cleal over 8 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
Updated by Dominic Cleal over 8 years ago
- Status changed from New to Assigned
- Assignee set to Dominic Cleal
Updated by The Foreman Bot over 8 years ago
- Status changed from Assigned to Ready For Testing
- Pull request https://github.com/theforeman/foreman/pull/3832 added
Updated by Dominic Cleal over 8 years ago
- Status changed from Ready For Testing to Closed
- % Done changed from 0 to 100
Applied in changeset 2e5f252ae12e20074f9f73815416637a8a6732ce.
Updated by Dominic Cleal over 8 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.