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

Removing guard conditions in Chargeback #17463

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented May 22, 2018

We have same(on first look apparently) condition on two places.

This is commit based story how to remove condition from one place in ChargebackRateDetail#charge method.
Thanks to it method ChargebackRateDetail#charge is more clean.

follow up after:
#17387
#17414

cc @gtanzillo

@miq-bot assign @jrafanie

@miq-bot miq-bot added the wip label May 22, 2018
@lpichler lpichler force-pushed the remove_column_guard_conditions_in_chargeback branch from 9a6faf4 to 0a5c61a Compare May 22, 2018 15:00
Goal: Make this condition as is in affects_report_fields
and then remove the condition
Calculation with zero rates is usefull and it is not affecting anything.
this filtering is already done in rate_details_relevant_to in ChargebackRateDetail
@lpichler lpichler force-pushed the remove_column_guard_conditions_in_chargeback branch from 0a5c61a to f5bc378 Compare May 22, 2018 15:01
@lpichler lpichler changed the title [WIP] Removing guard conditions in Chargebaback Removing guard conditions in Chargebaback May 22, 2018
@@ -94,7 +94,7 @@ def rate_values(consumption, options)

def charge(relevant_fields, consumption, options)
result = {}
if (relevant_fields & [metric_key]).present? || (relevant_fields & cost_keys).present?
if (relevant_fields & [metric_key]).present? || ((cost_keys.to_set & report_cols).present? && !gratis?)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is from ChargebackRateDetail#affects_report_fields

this filtering is covered by ChargebackRate#rate_details_relevant_to
@miq-bot
Copy link
Member

miq-bot commented May 22, 2018

Checked commits lpichler/manageiq@433c2cc~...7478035 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

This looks good to me. Walking through the commits one by one I was able to follow the process. Looking at the net of those changes, though, it's surprising that it's only those few lines. 👍

@jrafanie what do you think?

@gtanzillo gtanzillo added this to the Sprint 87 Ending Jun 4, 2018 milestone May 30, 2018
@gtanzillo gtanzillo merged commit 5a763e1 into ManageIQ:master May 30, 2018
@jrafanie jrafanie changed the title Removing guard conditions in Chargebaback Removing guard conditions in Chargeback May 30, 2018
@lpichler lpichler deleted the remove_column_guard_conditions_in_chargeback branch May 31, 2018 13:51
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