-
Notifications
You must be signed in to change notification settings - Fork 900
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
Chargeback calculation :: Filter rates by need #12885
Conversation
@@ -225,6 +229,10 @@ def contiguous_tiers? | |||
|
|||
private | |||
|
|||
def gratis? | |||
chargeback_tiers.all?(&:gratis?) |
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.
just note: when first tier is gratis, there are no other tiers.
So chargeback_tiers.first.gratis?
should be is enough
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.
Right.
Although, that's only true for ChargebackTiers
that have been saved after the validation was introduced. The validation check for this is not here from day 1 I think.
Anyway, I prefer this way for readability reasons.
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 I agree, it is more readable.
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.
gratis?
is a nice touch
92cce5d
to
1701ad2
Compare
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
1701ad2
to
1a814c6
Compare
@miq-bot remove_label wip |
1a814c6
to
c8a62d2
Compare
results, ext = klass.send(custom_results_method, db_options[:options].merge(:userid => options[:userid], :ext_options => ext_options)) | ||
results, ext = klass.send(custom_results_method, db_options[:options].merge(:userid => options[:userid], | ||
:ext_options => ext_options, | ||
:report_cols => cols)) |
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'm ok with this.
Just a side note / thought:
at one time, :report_cols
was part of :ext_options
. Unfortunately, :ext_options
has been abused for too long and we were making it go away (albeit slowly).
Can we use :select
or something else that is part of active record's current framework? (Yes, I know this is not an active record call)
c8a62d2
to
64cbb70
Compare
Checked commits isimluk/manageiq@baef8fd~...64cbb70 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 |
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.
LGTM. Looking forward to seeing how this goes
Thanks!! |
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.
LGTM 👍
Let's use only those ChargebackRateDetails that are needed by calculated report.
In the database we always store all the rate_details. This is first step to get away of them, first we stop using them for calculation, then we can drop them.
We need report_cols in the chargeback anyway, to properly implement charging per dynamic storage types.
As a side effect we will get performance boost.
@miq-bot add_label wip, chargeback, refactoring, euwe/no
@miq-bot assign @gtanzillo