-
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
[WIP] Add new selector for Chargeback Rate editing screen to select the base model #4001
[WIP] Add new selector for Chargeback Rate editing screen to select the base model #4001
Conversation
6875c28
to
3170e58
Compare
acb74f4
to
662d6f9
Compare
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.
great! It looks good to me 👍
@miq-bot assign @mzazrivec |
%br | ||
= _('* Caution: Allocated CPU Count/Cores, Used CPU, Used Disk I/O and Allocated Memory are not supported for Chargeback for Projects. ') | ||
%br | ||
= _('* Caution: Allocated/Used CPU Cores are not supported for Chargeback for Vms. ') |
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.
Could we just rephrase these into a single sentence on a single line?
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.
I don't understand how. These are three different sentences how would you like to put it into one ?
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.
keyword: rephrase 😉
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.
There are three things: Chargeback for Images, Projects, Vms. We need to inform which fields are not supported for each of them. How would you put so much info into the one line to keep readability? Anyway, I see later comments and it looks like there will be completely different solution to what I was told to do 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.
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, four lines of "Caution" is also too much for me but here it is not possible to put all the info in one line and to keep readability, it's too much info to put just into one line. Anyway, I was told to do so.. Aaand.. now I was told to do it completely differently so don't worry about this, I will udpate the PR soon! ;)
- report_base = @edit[:new][:model].titleize.split(' ').last.pluralize | ||
%br | ||
%strong | ||
= _("* Caution: #{unavailable_fields} are not supported for Chargeback for #{report_base}.") |
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.
This is probably not okay from the translation side, you need to use the %
operator.
- if @edit[:new][:model] && @edit[:new][:model].starts_with?('Chargeback') | ||
- unavailable_fields = unavailable_fields_for_model | ||
- if unavailable_fields | ||
- report_base = @edit[:new][:model].titleize.split(' ').last.pluralize |
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.
Translation missing here, probably ui_lookup
but not sure.
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.
Here I don't display anything so I don't think so. I think I miss ui_lookup
later.
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.
You're displaying report_base
a few lines below.
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.
Yes, this is what i meant by "later".
app/controllers/report_controller.rb
Outdated
@@ -274,6 +274,19 @@ def title | |||
|
|||
private | |||
|
|||
# Get string with unavailable fields for displaying note about them while adding/editing report | |||
def unavailable_fields_for_model | |||
case @edit[:new][:model] |
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.
Instead of testing for the @edit[:new][:model]
I would make this method more universal by using an argument instead.
app/controllers/report_controller.rb
Outdated
'Allocated CPU Count, Used CPU, Used Disk I/O and Used Network I/O' | ||
when 'ChargebackContainerProject' | ||
'Allocated CPU Count/Cores, Used CPU, Used Disk I/O and Allocated Memory' | ||
end |
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.
Translations?
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.
I wouldn't use ui_lookup
here. Or did you mean anything else?
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.
_
I don't like the fact, that the information about fields that are not allowed for chargeback is in the UI. Practically in the views. This should come from the backend. |
after clarification with @martinpovolny we will do the different solution. It will be about "Chargeback Rates for concrete type of report" |
2c32744
to
ee1ddd7
Compare
adaac83
to
169bfd1
Compare
f2cd3db
to
a7b811f
Compare
Changes for Reports moved to another PR. |
This pull request is not mergeable. Please rebase and repush. |
This pull request has been automatically closed because it has not been updated for at least 6 months. Feel free to reopen this pull request if these changes are still valid. Thank you for all your contributions! |
This pull request is not mergeable. Please rebase and repush. |
Show also the info about no base model set (for already existing rates) as Rate based on 'Any'.
a7b811f
to
3c33d98
Compare
Also, add flash message to inform that setting the base model for the rate is required.
Edit/simplify the method by moving setting the rate details to the new method cb_rate_set_details
Save the id to @edit to be able to get the original Rate details while copying the rate and changing the base model. This is important for 'older' rates with no base model set before for not losing the original details.
Also, reset the Rate Details according to the base model if the base model was changed by user.
3c33d98
to
c317d24
Compare
Add simple condition 'if @edit[:new][:model]' and '#rate_edit_details' to rate edit table haml to be able to easily refresh/display or not display the table if report base model was not set yet.
Checked commits hstastna/manageiq-ui-classic@9a33fff~...c3623ce with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 **
|
Partially fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1581817
Needs to merge with:
ManageIQ/manageiq#17507
ManageIQ/manageiq-schema#304 (new alternative of ManageIQ/manageiq-schema#209)
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 add note/info about which fields are not supported to the Report editing screen (in another PR: #4081), and also new selector for Chargeback Rate editing screen (under Cloud Intel > Chargeback > Rates) is added to choose what the Rate is based on (Images/Projects/Vms) (this PR). We call it the base model (see ManageIQ/manageiq-schema#209).
Before:
![edit_rate_before](https://user-images.githubusercontent.com/13417815/40666135-500ed858-635f-11e8-9285-78adf59e6e15.png)
![rate_show](https://user-images.githubusercontent.com/13417815/41099016-872b10ea-6a5d-11e8-9aa5-f1c7e619a2d8.png)
Adding/editing a Chargeback Rate:
Displaying details page of a Chargeback Rate:
After:
![add_new_rate_after](https://user-images.githubusercontent.com/13417815/41103181-c8b2e5c8-6a68-11e8-87bb-fdf5fed27474.png)
![report_base_model-select](https://user-images.githubusercontent.com/13417815/41103191-cf2e49ba-6a68-11e8-9245-afe7a9fff787.png)
![new_rate_base_model](https://user-images.githubusercontent.com/13417815/41104459-ae143cdc-6a6b-11e8-8066-b4e7e4f5f622.png)
![new_rate2_show](https://user-images.githubusercontent.com/13417815/41104589-0489b808-6a6c-11e8-8f2d-3cb42cfc2061.png)
![old_rate_show](https://user-images.githubusercontent.com/13417815/41051078-65ff2abc-69b5-11e8-9819-eaf604827e9f.png)
![old_rate-edit](https://user-images.githubusercontent.com/13417815/41104808-8cd03a7a-6a6c-11e8-8504-b73d752c8a4e.png)
![old_rate_change_base](https://user-images.githubusercontent.com/13417815/41051612-e5f76f62-69b6-11e8-841d-2c3d1a8b46eb.png)
![model_required](https://user-images.githubusercontent.com/13417815/41051452-77b61904-69b6-11e8-946c-15f41c536c63.png)
Adding a new Chargeback Rate (the Rate Details table is displayed after choosing one of the "base models" by the new drop down because the rate details depend on it):
The new selector for configuring Rate columns (for choosing the base model), in editing screen:
Adding/editing an existing Chargeback Rate with base model:
(After changing the model, the Rate Details table is cleared to its default values, similarly as for Reports, and the columns of the table are updated according to the chosen base model: only appropriate columns are displayed for each base model)
Displaying details page of a new Chargeback Rate, with the base model:
Displaying details page of a Chargeback Rate, without the base model (all the existing rates):
(The rate is based on 'Any' as its details' table has all the available/any columns)
Editing the Chargeback Rate without the base model:
('Any' option is also available - user can see all the original details' values and edit them)
Changing the base model of the Chargeback Rate without the base model:
(While changing the base model from "Any" to any other, user will not lose his original details' values, the table will not be reset to its default values as usually, only the appropriate columns display - with already existing values of the rate, already saved in the DB)
Saving the Chargeback Rate without the base model:
Note:
Copying of the Rate with/without base model also works but it will not be possible to save the Rate based on 'Any'.