Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #10853 - add config_template_ids back to organization #2470

Closed
wants to merge 1 commit into from
Closed

Fixes #10853 - add config_template_ids back to organization #2470

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 16 additions & 7 deletions app/controllers/concerns/api/v2/taxonomies_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ module Api::V2::TaxonomiesController
extend ActiveSupport::Concern

included do
before_filter :rename_config_template, :only => %w{update create}
before_filter :find_optional_nested_object
before_filter :find_taxonomy, :only => %w(show update destroy settings
domain_ids subnet_ids hostgroup_ids config_template_ids compute_resource_ids
Expand All @@ -19,7 +20,8 @@ module Api::V2::TaxonomiesController
param :smart_proxy_ids, Array, N_("Smart proxy IDs"), :required => false
param :compute_resource_ids, Array, N_("Compute resource IDs"), :required => false
param :media_ids, Array, N_("Media IDs"), :required => false
param :config_template_ids, Array, N_("Provisioning template IDs"), :required => false
param :config_template_ids, Array, N_("Provisioning template IDs"), :required => false # FIXME: deprecated
param :provisioning_template_ids, Array, N_("Provisioning template IDs"), :required => false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a note - deprecated or something like that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was against custom deprecated string here, it's planned to have support for marking params as deprecated in apipie, maybe leaving a comment to easily find this later would be enough

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elobato I've added just a FIXME comment to locate the place. We should add support for deprecated options into Apipie first. Then this is information could be reused in docs and Hammer. I filed issue as a reminder http://projects.theforeman.org/issues/10869

param :domain_ids, Array, N_("Domain IDs"), :required => false
param :realm_ids, Array, N_("Realm IDs"), :required => false
param :hostgroup_ids, Array, N_("Host group IDs"), :required => false
Expand Down Expand Up @@ -53,7 +55,7 @@ def show
api :POST, '/:resource_id', N_('Create :a_resource')
param_group :resource, :as => :create
def create
@taxonomy = taxonomy_class.new(params[taxonomy_single.to_sym])
@taxonomy = taxonomy_class.new(params[taxonomy_single])
instance_variable_set("@#{taxonomy_single}", @taxonomy)
process_response @taxonomy.save
end
Expand All @@ -64,7 +66,7 @@ def update
# NOTE - if not ! and invalid, the error is undefined method `permission_failed?' for #<Location:0x7fe38c1d3ec8> (NoMethodError)
# removed process_response & added explicit render 'api/v2/taxonomies/update'. Otherwise, *_ids are not returned

process_response @taxonomy.update_attributes(params[taxonomy_single.to_sym])
process_response @taxonomy.update_attributes(params[taxonomy_single])
end

api :DELETE, '/:resource_id/:id', N_('Delete :a_resource')
Expand All @@ -76,12 +78,19 @@ def destroy

private

def rename_config_template
if params[taxonomy_single] && params[taxonomy_single][:config_template_ids].present?
params[taxonomy_single][:provisioning_template_ids] = params[taxonomy_single].delete(:config_template_ids)
::ActiveSupport::Deprecation.warn('Config templates were renamed to provisioning templates')
end
end

def params_match_database
# change params[:select_all_types] to params[:ignore_types] to match database
if params[taxonomy_single.to_sym].try(:[], :select_all_types)
params[taxonomy_single.to_sym][:ignore_types] = params[taxonomy_single.to_sym][:select_all_types]
params[taxonomy_single.to_sym] = params[taxonomy_single.to_sym].reject { |k, v| k == "select_all_types" }
return params[taxonomy_single.to_sym]
if params[taxonomy_single].try(:[], :select_all_types)
params[taxonomy_single][:ignore_types] = params[taxonomy_single][:select_all_types]
params[taxonomy_single] = params[taxonomy_single].reject { |k, v| k == "select_all_types" }
return params[taxonomy_single]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this return serves any purpose? Maybe you can do an early return if params[taxonomy_single].try(:[], :select_all_types) and avoid nesting the whole method in one if. The way it is right now it looks like you could avoid calling this method by just adding something like :if => { params[taxonomy_single].try(:[], :select_all_types) }on the filter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was originally not touched by this PR, @mbacovsky just removed .to_sym in this method

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elobato as @ares suggests I'd prefer to touch the unrelated stuff as little as possible.

end
end

Expand Down