-
Notifications
You must be signed in to change notification settings - Fork 356
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
Display info about unavailable fields while adding/editing report #5418
Display info about unavailable fields while adding/editing report #5418
Conversation
d779d54
to
91d3037
Compare
@miq-bot add_label bug, hammer/yes |
2ef38df
to
18d22e0
Compare
Overall looks good 👍 thanks @hstastna |
55d06fa
to
f9afa3e
Compare
%br | ||
%p{:style => "max-width: 850px;"} | ||
%strong | ||
= _("* Caution: %{unav_fields} are not supported for %{rep_base}.") % {:unav_fields => @unavailable_fields, :rep_base => Dictionary.gettext(@edit[:new][:model], :type => :model, :notfound => :titleize, :plural => 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.
Using string arithmetics for gettext strings is generally a bad idea. It makes the work of translators more difficult, plus the resulting translated string will not always make sense.
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.
Hmm, that is interesting because I was told to do so in some other PR. So what is your suggestion instead of this? What is the best? Thank you.
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.
Well, you need to put the static parts of the string together in one place (in this case, probably the controller) and substitute %{rep_base}
here (that's the only dynamic part, correct?)
f9afa3e
to
417d035
Compare
417d035
to
bc17b0f
Compare
Checked commits hstastna/manageiq-ui-classic@01d2a9a~...bc17b0f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 **
|
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1581817
What:
When choosing Chargeback for Images/Projects/Vms under Configure Report Columns section while adding a new/editing an existing Report under Cloud Intel > Reports > Reports, some of the fields are not available. This is ok but there was a misunderstanding that those fields were missing there. This is why I am adding note about which fields are not supported to the Report editing screen.
Steps to reproduce:
=> some of the fields are not available, for example Cpu Cores Allocated Metric for Chargeback for Fms (Carefully! They are not missing!)
Before:
Choosing Chargeback for Images while adding/editing report:
Choosing Chargeback for Projects:
Choosing Chargeback for Vms:
After:
Choosing Chargeback for Images while adding/editing report:
Choosing Chargeback for Projects:
Choosing Chargeback for Vms: