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.

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

Use copy_params_if_present in report, miq_policy and pxe controllers #6155

Merged

Conversation

hstastna
Copy link

@hstastna hstastna commented Sep 5, 2019

Issue: #6105

This is another PR with use of copy_params_if_present method in some controllers.
With copy_params_if_present, we improve functionality (remove "" vs nil issues and bad response of Add/Save buttons) or we simplify the code.

Similar PRs/changes:
#6152
#6139
#5815
d3c3878#diff-956e46b33fb5307990c4e1a4b5fd86ccR1267

@hstastna
Copy link
Author

hstastna commented Sep 5, 2019

@miq-bot add_label technical debt

@hstastna
Copy link
Author

hstastna commented Sep 5, 2019

@miq-bot add_reviewer @romanblanco

Roman, this should be all the remaining possible changes regarding using copy_params_if_present.

@miq-bot miq-bot requested a review from romanblanco September 5, 2019 09:41
@hstastna hstastna mentioned this pull request Sep 5, 2019
21 tasks
Copy link
Member

@romanblanco romanblanco left a comment

Choose a reason for hiding this comment

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

@hstastna changes look good!

found some more places where we can use copy_param(s)_if_present

targets = Rbac.filtered(params[:target_class]).select(:id, *columns_for_klass(params[:target_class])) if params[:target_class].present?
unless targets.nil?
@resolve[:targets] = targets.sort_by { |t| t.name.downcase }.collect { |t| [t.name, t.id.to_s] }
@resolve[:new][:target_id] = nil
end
end
@resolve[:new][:target_id] = nil if params[:target_class] == ""
@resolve[:new][:target_id] = params[:target_id] if params.key?(:target_id)
@resolve[:new][:target_id] = nil unless params[:target_class]
@resolve[:button_text] = params[:button_text] if params.key?(:button_text)
@resolve[:button_number] = params[:button_number] if params.key?(:button_number)
Copy link
Member

Choose a reason for hiding this comment

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

-    @resolve[:button_text] = params[:button_text] if params.key?(:button_text)
-    @resolve[:button_number] = params[:button_number] if params.key?(:button_number)
+   copy_params_if_present(@resolve, params, %i[button_text button_number])

ApplicationController::AE_MAX_RESOLUTION_FIELDS.times do |i|
f = ("attribute_" + (i + 1).to_s)
v = ("value_" + (i + 1).to_s)
@resolve[:new][:attrs][i][0] = params[f] if params[f.to_sym]
@resolve[:new][:attrs][i][1] = params[v] if params[v.to_sym]
end
@resolve[:new][:target_class] = params[:target_class] if params[:target_class]
# @resolve[:new][:target_attr_name] = params[:target_attr_name] if params.has_key?(:target_attr_name)
Copy link
Member

Choose a reason for hiding this comment

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

the commented code can be removed

@@ -86,18 +86,14 @@ def action_field_changed
@action = @edit[:action_id] ? MiqAction.find(@edit[:action_id]) : MiqAction.new

@edit[:new][:description] = params[:description].presence if params[:description]
Copy link
Member

Choose a reason for hiding this comment

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

- @edit[:new][:description] = params[:description].presence if params[:description]
+ copy_param_if_present(@edit[:new], params, %i[description])

@hstastna hstastna force-pushed the Report_miq_policy_copy_params_present branch from 2da9a2b to 56fa533 Compare September 5, 2019 14:16
@hstastna hstastna force-pushed the Report_miq_policy_copy_params_present branch from 56fa533 to 6735efd Compare September 5, 2019 16:13
@miq-bot
Copy link
Member

miq-bot commented Sep 5, 2019

Checked commit hstastna@6735efd with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
11 files checked, 0 offenses detected
Everything looks fine. 🏆

Copy link
Member

@romanblanco romanblanco left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mzazrivec mzazrivec self-assigned this Sep 6, 2019
@mzazrivec mzazrivec added this to the Sprint 120 Ending Sep 16, 2019 milestone Sep 6, 2019
@mzazrivec mzazrivec merged commit 6c58f52 into ManageIQ:master Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants