Skip to content

Commit

Permalink
Fixes theforeman#7230, #12021 - Upgrade to Rails 4.1.5
Browse files Browse the repository at this point in the history
This commits upgrades Rails to Rails 4.1.5. See a description of the
changes included here, and go to the pull request in GitHub to see more
detailed explanations:

* Update gems to a Rails 4 compatible version, including dependencies
* Fix counter cache columns
* Remove conditions, order, limit from has_many relations
* Remove test runner
    On minitest 5, the runner API was deprecated, so our custom test
    runner is no longer working.

* Remove useless add_index on lookup_values match
    An index is added previously on lookup_values :priority, and on Rails 4
    rename_column changes 'priority' to 'match' already changes the name of
    the index.

* Alias assert_include to assert_includes.
* Expire topbar cache
* Subclass ApplicationMailer in test to avoid 'missing template' error
* Fixes #12021 - Use .to_param to find parent object in LookupKeys controller
    On Rails 4, .find will not default to .friendly.find so find_hostgroup,
    find_environment and find_host will fail for non numeric IDs. However, we can
    use from_param to find these objects. This is something we can do both on
    Rails 3 and Rails 4, whereas using .friendly isn't an option until friendly_id
     5.0, which depends on Rails 4.

* Use explicit friendly ID search.
* find_common finder uses from_param -> friendly -> find
    The finder needed to be refactored because with the new Friendly ID we
    have to use .friendly explicitely. It currently follows the strategy of
    searching like this:
      - from_param -> .friendly -> regular find

* Remove RecordNotFound errors from parameterizable in rails 4
* Protect attributes using attr_accessible and protected_attributes
* url_for doesn't append port when protocol is specified
    Some of our tests are checking if port 80 or 443 is included in the URL
    generated by lib/foreman/renderer.rb. However url_for has changed in
    Rails 4 and now it does not append the port even if explicitly added.

    If the protocol is specified, as it is in this case, then the url
    generated will just have the protocol (http or https) but not the port
    (80 or 443)

* Validate object instead of _id column in join tables
    On Rails 4 such validations will fail when we try to create objects
    without explicitely assigning the id.
    Puppetclass.new(:config_group => config_group) would fail even for a
    valid config group, :config_group_id => config_group.id should be used
    instead.

    To avoid that, we validate the object, not the ID column

* Rails 4 non backwards compatible syntax changes
    Changes that have to deal with how some of the internal Rails objects
    need a new syntax, like generating routes, exceptions, form builders.
    None of these are Rails 3 compatible.

* Fixes #12199 - rails 4 migration errors
    There are actually two parts to the issue of not being able to migrate
    with this branch. The first is
    https://gist.github.com/eLobato/0f5db50b5c93cc6c277c and can be
    temporarily fixed with rails/sass-rails#136 (comment) I am not sure of a
    long term fix for that atm.

    Once that is patched, this error
    https://gist.github.com/johnpmitsch/96e5ba3629890931193a happens on a
    migration. This will fix that error by changing the Migrator class
    initializer arguments which have changed from rails 3 to rails 4

* Refactor fact value test to not modify user_roles directly
* Return external usergroupsas an array
* Use eager_load to preload associations to be used in where

Fixes theforeman#7230, #12021 - Upgrade to Rails 4.1.5

This commits upgrades Rails to Rails 4.1.5. See a description of the
changes included here, and go to the pull request in GitHub to see more
detailed explanations:

* Update gems to a Rails 4 compatible version, including dependencies
* Fix counter cache columns
* Remove conditions, order, limit from has_many relations
* Remove test runner
    On minitest 5, the runner API was deprecated, so our custom test
    runner is no longer working.

* Remove useless add_index on lookup_values match
    An index is added previously on lookup_values :priority, and on Rails 4
    rename_column changes 'priority' to 'match' already changes the name of
    the index.

* Alias assert_include to assert_includes.
* Expire topbar cache
* Subclass ApplicationMailer in test to avoid 'missing template' error
* Fixes #12021 - Use .to_param to find parent object in LookupKeys controller
    On Rails 4, .find will not default to .friendly.find so find_hostgroup,
    find_environment and find_host will fail for non numeric IDs. However, we can
    use from_param to find these objects. This is something we can do both on
    Rails 3 and Rails 4, whereas using .friendly isn't an option until friendly_id
     5.0, which depends on Rails 4.

* Use explicit friendly ID search.
* find_common finder uses from_param -> friendly -> find
    The finder needed to be refactored because with the new Friendly ID we
    have to use .friendly explicitely. It currently follows the strategy of
    searching like this:
      - from_param -> .friendly -> regular find

* Remove RecordNotFound errors from parameterizable in rails 4
* Protect attributes using attr_accessible and protected_attributes
* url_for doesn't append port when protocol is specified
    Some of our tests are checking if port 80 or 443 is included in the URL
    generated by lib/foreman/renderer.rb. However url_for has changed in
    Rails 4 and now it does not append the port even if explicitly added.

    If the protocol is specified, as it is in this case, then the url
    generated will just have the protocol (http or https) but not the port
    (80 or 443)

* Validate object instead of _id column in join tables
    On Rails 4 such validations will fail when we try to create objects
    without explicitely assigning the id.
    Puppetclass.new(:config_group => config_group) would fail even for a
    valid config group, :config_group_id => config_group.id should be used
    instead.

    To avoid that, we validate the object, not the ID column

* Rails 4 non backwards compatible syntax changes
    Changes that have to deal with how some of the internal Rails objects
    need a new syntax, like generating routes, exceptions, form builders.
    None of these are Rails 3 compatible.

* Fixes #12199 - rails 4 migration errors
    There are actually two parts to the issue of not being able to migrate
    with this branch. The first is
    https://gist.github.com/eLobato/0f5db50b5c93cc6c277c and can be
    temporarily fixed with rails/sass-rails#136 (comment) I am not sure of a
    long term fix for that atm.

    Once that is patched, this error
    https://gist.github.com/johnpmitsch/96e5ba3629890931193a happens on a
    migration. This will fix that error by changing the Migrator class
    initializer arguments which have changed from rails 3 to rails 4

* Refactor fact value test to not modify user_roles directly
* Return external usergroupsas an array
* Use eager_load to preload associations to be used in where
* Auto detect jenkins rake task in application.rb and set test RAILS_ENV
  in that case
* Reference previously undigested assets in CSS and js as digested using
  asset-url or image-url
* Adapted sprockets manifest.json instead of manifest.yml (ehelms)
  • Loading branch information
dLobatog authored and alongoldboim committed Dec 20, 2015
1 parent d0d5316 commit 13769db
Show file tree
Hide file tree
Showing 78 changed files with 409 additions and 371 deletions.
16 changes: 9 additions & 7 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,28 @@ require File.expand_path('../lib/regexp_extensions', FOREMAN_GEMFILE)

source 'https://rubygems.org'

gem 'rails', '3.2.22'
gem 'rack-cache', '< 1.3.0'
gem 'rails', '4.1.5'
gem 'json', '~> 1.5'
gem 'rest-client', '~> 1.6.0', :require => 'rest_client'
gem 'audited-activerecord', '3.0.0'
gem 'audited-activerecord', '~> 4.0'
gem 'will_paginate', '~> 3.0'
gem 'ancestry', '~> 2.0'
gem 'scoped_search', '~> 3.0'
gem 'scoped_search', '>= 3.2.2', '< 4'
gem 'ldap_fluff', '>= 0.3.5', '< 1.0'
gem 'net-ldap', '>= 0.8.0'
gem 'apipie-rails', '~> 0.2.5'
gem 'apipie-rails', '~> 0.3.4'
gem 'rabl', '~> 0.11'
gem 'oauth', '~> 0.4'
gem 'deep_cloneable', '~> 2.0'
gem 'foreigner', '~> 1.4'
gem 'validates_lengths_from_database', '~> 0.2'
gem 'friendly_id', '~> 4.0'
gem 'friendly_id', '~> 5.0'
gem 'secure_headers', '~> 1.3'
gem 'safemode', '~> 1.2'
gem 'fast_gettext', '~> 0.8'
gem 'gettext_i18n_rails', '~> 1.0'
gem 'i18n', '~> 0.6.4'
gem 'rails-i18n', '~> 3.0.0'
gem 'rails-i18n', '~> 4.0.0'
gem 'turbolinks', '~> 2.5'
gem 'logging', '>= 1.8.0', '< 3.0.0'
gem 'fog-core', '1.34.0'
Expand All @@ -36,6 +35,9 @@ if RUBY_VERSION.start_with? '1.9.'
else
gem 'net-ssh'
end
gem 'activerecord-session_store', '~> 0.1.1'
gem 'rails-observers', '~> 0.1'
gem 'protected_attributes', '~> 1.1.1'

Dir["#{File.dirname(FOREMAN_GEMFILE)}/bundler.d/*.rb"].each do |bundle|
self.instance_eval(Bundler.read_file(bundle))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ function build_modal(element, url) {
b.removeClass("disabled").attr("disabled", false);
});
return false;

}

function build_redirect(url) {
Expand Down
2 changes: 1 addition & 1 deletion app/assets/javascripts/topbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ $(document).on('mouseleave','.loc-submenu', function(){
function mark_active_menu() {
var menus = $('.menu_tab_dropdown'),
path = window.location.pathname + window.location.search,
link = $("[href='%s'".replace('%s', path));
link = $("[href='%s']".replace('%s', path));

menus.removeClass('active');
$("[class^='menu_tab_']").removeClass('active');
Expand Down
20 changes: 12 additions & 8 deletions app/assets/stylesheets/application.scss
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,11 @@ a.config_group_name {
}

.editable {
background: url("edit.png") no-repeat scroll 98% 6px transparent;
background: image-url("edit.png") no-repeat scroll 98% 6px transparent;
cursor: pointer;
padding: 4px 26px 4px 0;
&:hover {
background: url("edit-hover.png") no-repeat scroll 98% 6px #F2F2F2;
background: image-url("edit-hover.png") no-repeat scroll 98% 6px #F2F2F2;
}
}

Expand Down Expand Up @@ -291,23 +291,23 @@ glyphicon
}

.sorting {
background: url('sort_both.png') no-repeat center right;
background: image-url('sort_both.png') no-repeat center right;
}

.sorting_asc {
background: url('sort_asc.png') no-repeat center right;
background: image-url('sort_asc.png') no-repeat center right;
}

.sorting_desc {
background: url('sort_desc.png') no-repeat center right;
background: image-url('sort_desc.png') no-repeat center right;
}

.sorting_asc_disabled {
background: url('sort_asc_disabled.png') no-repeat center right;
background: image-url('sort_asc_disabled.png') no-repeat center right;
}

.sorting_desc_disabled {
background: url('sort_desc_disabled.png') no-repeat center right;
background: image-url('sort_desc_disabled.png') no-repeat center right;
}
}

Expand Down Expand Up @@ -349,7 +349,7 @@ glyphicon
.spinner-placeholder {
width: 16px;
height: 16px;
background: url('spinner.gif');
background: image-url('spinner.gif');
text-indent: 20px;
white-space: nowrap;
margin: 20% 30% 0 40%;
Expand Down Expand Up @@ -549,3 +549,7 @@ span.glyphicon.host-status {
html.turbolinks-progress-bar::before {
height: 2px !important;
}

.ms-container {
background: transparent image-url('switch.png') no-repeat 270px 80px;
}
4 changes: 2 additions & 2 deletions app/assets/stylesheets/charts.scss
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,9 @@
}

.statistics-pie.small .overlay {
background: url(/assets/pie_overlay.png) no-repeat;
background: image-url('/assets/pie_overlay.png') no-repeat;
background-size: cover;
-ms-behavior: url(/assets/background-size.htc);
-ms-behavior: asset-url('/assets/background-size.htc');
z-index: 1;
}

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/v1/reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def destroy
param :id, :identifier, :required => true

def last
conditions = { :host_id => Host.authorized(:view_hosts).find(params[:host_id]).try(:id) } if params[:host_id].present?
conditions = { :host_id => Host.authorized(:view_hosts).friendly.find(params[:host_id]).try(:id) } if params[:host_id].present?
max_id = resource_scope.where(conditions).maximum(:id)
@report = resource_scope.includes(:logs => [:message, :source]).find(max_id)
render :show
Expand Down
4 changes: 3 additions & 1 deletion app/controllers/api/v2/config_reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ def destroy
param :id, :identifier, :required => true

def last
conditions = { :host_id => Host.authorized(:view_hosts).find(params[:host_id]).try(:id) } if params[:host_id].present?
if params[:host_id].present?
conditions = { :host_id => resource_finder(Host.authorized(:view_hosts), params[:host_id]).try(:id) }
end
max_id = resource_scope.where(conditions).maximum(:id)
@config_report = resource_scope.includes(:logs => [:message, :source]).find(max_id)
render :show
Expand Down
6 changes: 4 additions & 2 deletions app/controllers/api/v2/host_classes_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ def destroy
# overwrite resource_name so it's host and and not host_class, since we want to return @host
def find_host
not_found and return false if params[:host_id].blank?
@host = Host.find(params[:host_id]) if Host::Managed.respond_to?(:authorized) &&
Host::Managed.authorized("view_host", Host::Managed)
if Host::Managed.respond_to?(:authorized) &&
Host::Managed.authorized("view_host", Host::Managed)
@host = resource_finder(Host.authorized(:view_hosts), params[:host_id])
end
end
end
end
Expand Down
6 changes: 6 additions & 0 deletions app/controllers/api/v2/models_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ def update
def destroy
process_response @model.destroy
end

private

def find_resource
@model = Model.friendly.find(params[:id]) || super
end
end
end
end
5 changes: 3 additions & 2 deletions app/controllers/api/v2/parameters_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,9 @@ def allowed_nested_id
def find_parameter
# nested_obj is required, so no need to check here
@parameters = nested_obj.send(parameters_method)
@parameter = @parameters.find(params[:id])
return @parameter if @parameter
@parameter = @parameters.from_param(params[:id])
@parameter ||= @parameters.friendly.find(params[:id])
return @parameter if @parameter.present?
not_found
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/v2/reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def destroy
param :id, :identifier, :required => true

def last
conditions = { :host_id => Host.authorized(:view_hosts).find(params[:host_id]).try(:id) } if params[:host_id].present?
conditions = { :host_id => resource_finder(Host.authorized(:view_hosts), params[:host_id]).try(:id) } if params[:host_id].present?
max_id = resource_scope.where(conditions).maximum(:id)
@report = resource_scope.includes(:logs => [:message, :source]).find(max_id)
render :show
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ def remote_user_provided?
end

def display_layout?
return nil if two_pane?
return false if two_pane?
"application"
end

Expand Down
59 changes: 23 additions & 36 deletions app/controllers/concerns/api/v2/lookup_keys_common_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,6 @@ module Api::V2::LookupKeysCommonController
before_filter :return_if_smart_mismatch, :only => [:show, :update, :destroy]
end

def puppetclass_id?
params.keys.include?('puppetclass_id')
end

def environment_id?
params.keys.include?('environment_id')
end

def host_id?
params.keys.include?('host_id')
end

def hostgroup_id?
params.keys.include?('hostgroup_id')
end

def smart_variable_id?
params.keys.include?('smart_variable_id') || controller_name.match(/smart_variables/)
end
Expand All @@ -43,28 +27,22 @@ def smart_class_parameter_id?
params.keys.include?('smart_class_parameter_id') || controller_name.match(/smart_class_parameters/)
end

def find_puppetclass
@puppetclass = Puppetclass.authorized(:view_puppetclasses).find(params['puppetclass_id'])
rescue ActiveRecord::RecordNotFound
not_found({ :error => { :message => (_("Puppet class with id '%{id}' was not found") % { :id => params['puppetclass_id'] }) } })
end
[Puppetclass, Environment, Host::Base, Hostgroup].each do |model|
model_string = model.to_s.split('::').first.downcase

def find_environment
@environment = Environment.authorized(:view_environments).find(params['environment_id'])
rescue ActiveRecord::RecordNotFound
not_found({ :error => { :message => (_("Environment with id '%{id}' was not found") % { :id => params['environment_id'] }) } })
end

def find_host
@host = Host::Base.authorized(:view_hosts).find(params['host_id'])
rescue ActiveRecord::RecordNotFound
not_found({ :error => { :message => (_("Host with id '%{id}' was not found") % { :id => params['host_id'] }) } })
end
define_method("#{model_string}_id?") do
params.keys.include?("#{model_string}_id")
end

def find_hostgroup
@hostgroup = Hostgroup.authorized(:view_hostgroups).find(params['hostgroup_id'])
rescue ActiveRecord::RecordNotFound
not_found({ :error => { :message => (_("Hostgroup with id '%{id}' was not found") % { :id => params['hostgroup_id'] }) } })
define_method("find_#{model_string}") do
scope = model.authorized(:"view_#{model_string.pluralize}")
begin
instance_variable_set("@#{model_string}",
resource_finder(scope, params["#{model_string}_id"]))
rescue ActiveRecord::RecordNotFound
model_not_found(model_string)
end
end
end

def find_smart_variable
Expand Down Expand Up @@ -144,4 +122,13 @@ def return_if_smart_mismatch
not_found "#{obj} not found by id '#{id}'"
end
end

private

def model_not_found(model)
error_message = (
_("%{model} with id '%{id}' was not found") %
{ :id => params["#{model}_id"], :model => model.capitalize } )
not_found(:error => { :message => error_message } )
end
end
16 changes: 11 additions & 5 deletions app/controllers/concerns/find_common.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
# this mixin is used by both ApplicationController and Api::BaseController
# searches for an object based on its id, name, label, etc and assign it to an instance variable
# This mixin is used by both ApplicationController and Api::BaseController
# Searches for an object based on its id, name, label, etc and assign it to an instance variable
# friendly_id performs the logic if params[:id] is 'id' or 'id-name' or 'name'

module FindCommon
# example: @host = Host.find(params[:id])
def find_resource
not_found and return if params[:id].blank?
instance_variable_set("@#{resource_name}", resource_scope.find(params[:id]))
instance_variable_set("@#{resource_name}",
resource_finder(resource_scope, params[:id]))
end

def resource_finder(scope, id)
raise ActiveRecord::RecordNotFound if scope.empty?
result = scope.from_param(id) if scope.respond_to?(:from_param)
result ||= scope.friendly.find(id) if scope.respond_to?(:friendly)
result || scope.find(id)
end

def resource_name(resource = controller_name)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/config_reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ def index
def show
# are we searching for the last report?
if params[:id] == "last"
conditions = { :host_id => Host.authorized(:view_hosts).find(params[:host_id]).try(:id) } if params[:host_id].present?
conditions = { :host_id => resource_finder(Host.authorized(:view_hosts), params[:host_id]).try(:id) } if params[:host_id].present?
params[:id] = resource_base.where(conditions).maximum(:id)
end

Expand Down
6 changes: 3 additions & 3 deletions app/controllers/hosts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def puppetclass_parameters
def externalNodes
certname = params[:name]
@host ||= resource_base.find_by_certname certname
@host ||= resource_base.find certname
@host ||= resource_base.friendly.find certname
not_found and return unless @host

begin
Expand Down Expand Up @@ -352,7 +352,7 @@ def update_multiple_parameters
skipped = []
params[:name].each do |name, value|
next if value.empty?
if (host_param = host.host_parameters.find(name))
if (host_param = host.host_parameters.friendly.find(name))
counter += 1 if host_param.update_attribute(:value, value)
else
skipped << name
Expand Down Expand Up @@ -698,7 +698,7 @@ def taxonomy_scope
# overwrite application_controller
def find_resource
not_found and return false if (id = params[:id]).blank?
@host = resource_base.find(id)
@host = resource_base.friendly.find(id)
@host ||= resource_base.find_by_mac params[:host][:mac] if params[:host] && params[:host][:mac]

not_found and return(false) unless @host
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/settings_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def index
end

def update
@setting = Setting.find(params[:id])
@setting = Setting.friendly.find(params[:id])
if @setting.parse_string_value(params[:setting][:value]) && @setting.save
render :json => @setting
else
Expand Down
6 changes: 3 additions & 3 deletions app/controllers/unattended_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ def built
def template
return head(:not_found) unless (params.has_key?("id") and params.has_key?(:hostgroup))

template = ProvisioningTemplate.find(params['id'])
@host = Hostgroup.find(params['hostgroup'])
template = ProvisioningTemplate.find_by_name(params['id'])
@host = Hostgroup.find_by_name(params['hostgroup'])

return head(:not_found) unless template and @host

Expand Down Expand Up @@ -107,7 +107,7 @@ def get_host_details

def find_host_by_spoof
host = Nic::Base.primary.find_by_ip(params.delete('spoof')).try(:host) if params['spoof'].present?
host ||= Host.find(params.delete('hostname')) if params['hostname'].present?
host ||= Host.find_by_name(params.delete('hostname')) if params['hostname'].present?
@spoof = host.present?
host
end
Expand Down
8 changes: 8 additions & 0 deletions app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,14 @@ def link_to(*args, &block)
end
end

def link_to_function(name, function, html_options = {})
onclick_tag = "#{html_options[:onclick]}; " if html_options[:onclick]
onclick = "#{onclick_tag}#{function}; return false;"
href = html_options[:href] || '#'

content_tag(:a, name, html_options.merge(:href => href, :onclick => onclick))
end

protected

def contract(model)
Expand Down
Loading

0 comments on commit 13769db

Please sign in to comment.