Settings for CB calculation&report are worth to be represented by separate class #12049
Conversation
isimluk
changed the title
isimluk
force-pushed
the
refactor-extract-chargeback-options
branch
2 times, most recently
from
c54b327
to
1327bff
Compare
| @@ -8,7 +8,7 @@ class Chargeback < ActsAsArModel | |||
|
|
|||
| def self.build_results_for_report_chargeback(options) | |||
| _log.info("Calculating chargeback costs...") | |||
| @options = options | |||
| @options = options = ReportOptions.new_from_h(options) | |||
There was a problem hiding this comment.
@isimluk why are not you using only @options ? even options changed his type maybe it can be confusing
| @@ -0,0 +1,102 @@ | |||
| class Chargeback | |||
There was a problem hiding this comment.
@isimluk how this works ? it is something like extension of class Chargeback ?
Should it have different name ? (or same as file) or isn't better to have in module ?
There was a problem hiding this comment.
This just adds one identifier (ReportOptions) to the namespace of Chargeback class.
I think there are other places in MiQ. using the same feature.
There was a problem hiding this comment.
I was wondering how this works too. It doesn't seem obvious that you can call ReportOptions.new_from_h with this code. Although, it's pretty cool that you can.
There was a problem hiding this comment.
You can re-open the class any time. Having the class Chargeback here is the same as putting the ReportOptions block into the app/models/chargeback.rb.
The ReportOptions is basically new Datatype. That is what Struct.new does, the new datatype.
Having a Struct here has great advantage. Struct is basically hash on steroids. A hash can be replaces with a Struct (struct defines the same methods) and the upside is that we can define methods on the given struct.
I have used this patter multiple times in the ui. It helped to lighten up some of the controllers. These controllers were previously manipulating complex hashes. Now, they just issue a method on a struct to do the right thing.
|
@isimluk I had some comments but otherwise looks good 👍 more readable now (also tried to run report and it is ok ) |
|
Thanks! |
isimluk
force-pushed
the
refactor-extract-chargeback-options
branch
from
1327bff
to
cbbfe12
Compare
| @@ -0,0 +1,102 @@ | |||
| class Chargeback | |||
There was a problem hiding this comment.
I was wondering how this works too. It doesn't seem obvious that you can call ReportOptions.new_from_h with this code. Although, it's pretty cool that you can.
isimluk
force-pushed
the
refactor-extract-chargeback-options
branch
from
cbbfe12
to
f068bd8
Compare
There are three possible callers of build_results_for_report_chargeback each of them sets @options to the same value that is later passed in as an argument. Let's have a single place to set it. In future I want to get a rid of class variable. But it will take few moments before we get there.
This was used for Ad-hoc chargeback schedule. It is unused since 2010 (a59b87cf535fd464d56d145bac12873cff65ea7d).
This was never actual option. Only a design idea that never come to be implemented. Let's drop this from documentation now, so we can consolidate the options more.
This will allow us to encapsulate code manipulating it here.
Timezone depends solely on Chargeback/ReportOptions.
The chargeback's timerange depends only on Chargeback report options.
It is only dependent on ReportOptions and shall be part of it.
|
Checked commits isimluk/manageiq@52a00de~...f068bd8 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 app/models/chargeback/report_options.rb
|
|
Backported to Euwe via #13419 |
There are three possible callers of
build_results_for_report_chargebackeach of them sets@optionsto the same value that is later passed in as an argument.Having the options and
@optionsin a single method brings some ambiguity. Let's have a single place to set it.In future, I want to get a rid of this class variable. But it will take few moments before we get there. We are on track.
@miq-bot add_label chargeback, refactoring
@miq-bot assign @gtanzillo