-
Notifications
You must be signed in to change notification settings - Fork 357
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
Add checkbox for chargeback without C & U #366
Add checkbox for chargeback without C & U #366
Conversation
@lpichler @gtanzillo is an invalid assignee, ignoring... |
89f5997
to
670049d
Compare
@miq-bot add_label ui, chargeback, enhancement |
@lpichler Cannot apply the following label because they are not recognized: chargeback |
670049d
to
19d5bcd
Compare
@@ -597,6 +597,8 @@ def gfv_chargeback | |||
@edit[:new][:cb_tag_cat] = params[:cb_tag_cat] | |||
@edit[:cb_tags] = entries_hash(params[:cb_tag_cat]) | |||
end | |||
elsif params.key?(:cb_without_metric_rollups) | |||
@edit[:new][:cb_without_metric_rollups] = params[:cb_without_metric_rollups] == "true" ? true : false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
params[:cb_without_metric_rollups] == "true" ? true : false
is just params[:cb_without_metric_rollups] == "true"
;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, sure, thanks :)
19d5bcd
to
fdf2176
Compare
LGTM 👍 (but |
Maybe it would be better to go the other way with the checkbox text. Something like "Include Capacity & Utilization Metrics" with a default of "Yes". This way, the option on the backend can be named in the positive way like |
fafd240
to
2c2e708
Compare
@himdel Can I ask for other review ? - as we discussed with @gtanzillo we changed semantic of checkbox so we have now:
thanks ! |
%label.control-label.col-md-2 | ||
= _('Include Capacity & Utilization Metrics') | ||
.col-md-8 | ||
= check_box_tag("cb_include_metrics", true, @edit[:new][:cb_include_metrics].nil? || @edit[:new][:cb_include_metrics], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you have the same condition when setting @edit[:new][:cb_include_metrics]
, here you can just use that, right? (As in, it will never be nil
here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aah.. no, sorry, the condition is in the wrong place for that..
But maybe you can just move the condition to the place where you copy options[:include_metrics]
to @edit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@himdel 👍 now we have the condition on the one place as we discussed
(I needed to add also init value for new report https://github.com/ManageIQ/manageiq-ui-classic/pull/366/files#diff-a1e3f16734c9d90e5b6e866305eab359R509 )
- only for chargeback for Vm
2c2e708
to
f45b8b7
Compare
Checked commit lpichler@f45b8b7 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@gtanzillo updated and screenshot changed, thanks 👍 |
LGTM 👍 waiting for ManageIQ/manageiq#13884 |
@himdel, ManageIQ/manageiq#13884 is now merged so you should be good to go to merge this one. |
this checkbox is allowing feature in this PR
@miq-bot assign @gtanzillo
please review @himdel
cc @isimluk