From b234e2a1920d0dd228418440027d5b978c0e0a0a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 16 Dec 2016 16:23:17 +0100
Subject: [PATCH 01/13] Refactor: Extract method: base_rollup_scope

---
 app/models/chargeback.rb                     | 20 +---------------
 app/models/chargeback/consumption_history.rb | 25 ++++++++++++++++++++
 2 files changed, 26 insertions(+), 19 deletions(-)
 create mode 100644 app/models/chargeback/consumption_history.rb

diff --git a/app/models/chargeback.rb b/app/models/chargeback.rb
index 3941a6182aa..8d6405436ca 100644
--- a/app/models/chargeback.rb
+++ b/app/models/chargeback.rb
@@ -10,30 +10,12 @@ class Chargeback < ActsAsArModel
     :fixed_compute_metric => :integer,
   )
 
-  VIRTUAL_COL_USES = {
-    "v_derived_cpu_total_cores_used" => "cpu_usage_rate_average"
-  }
-
   def self.build_results_for_report_chargeback(options)
     _log.info("Calculating chargeback costs...")
     @options = options = ReportOptions.new_from_h(options)
 
     rates = RatesCache.new
-
-    base_rollup = MetricRollup.includes(
-      :resource           => [:hardware, :tenant, :tags, :vim_performance_states, :custom_attributes, {:container_image => :custom_attributes}],
-      :parent_host        => :tags,
-      :parent_ems_cluster => :tags,
-      :parent_storage     => :tags,
-      :parent_ems         => :tags)
-                              .select(*Metric::BASE_COLS).order("resource_id, timestamp")
-    perf_cols = MetricRollup.attribute_names
-    rate_cols = ChargebackRate.where(:default => true).flat_map do |rate|
-      rate.chargeback_rate_details.map(&:metric).select { |metric| perf_cols.include?(metric.to_s) }
-    end
-
-    rate_cols.map! { |x| VIRTUAL_COL_USES.include?(x) ? VIRTUAL_COL_USES[x] : x }.flatten!
-    base_rollup = base_rollup.select(*rate_cols)
+    base_rollup = ConsumptionHistory.base_rollup_scope
 
     timerange = options.report_time_range
     data = {}
diff --git a/app/models/chargeback/consumption_history.rb b/app/models/chargeback/consumption_history.rb
new file mode 100644
index 00000000000..ac96a40441f
--- /dev/null
+++ b/app/models/chargeback/consumption_history.rb
@@ -0,0 +1,25 @@
+class Chargeback
+  class ConsumptionHistory
+    VIRTUAL_COL_USES = {
+      'v_derived_cpu_total_cores_used' => 'cpu_usage_rate_average'
+    }.freeze
+
+    def self.base_rollup_scope
+      base_rollup = MetricRollup.includes(
+        :resource           => [:hardware, :tenant, :tags, :vim_performance_states, :custom_attributes,
+                                {:container_image => :custom_attributes}],
+        :parent_host        => :tags,
+        :parent_ems_cluster => :tags,
+        :parent_storage     => :tags,
+        :parent_ems         => :tags)
+                                .select(*Metric::BASE_COLS).order('resource_id, timestamp')
+
+      perf_cols = MetricRollup.attribute_names
+      rate_cols = ChargebackRate.where(:default => true).flat_map do |rate|
+        rate.chargeback_rate_details.map(&:metric).select { |metric| perf_cols.include?(metric.to_s) }
+      end
+      rate_cols.map! { |x| VIRTUAL_COL_USES[x] || x }.flatten!
+      base_rollup.select(*rate_cols)
+    end
+  end
+end

From 5532ebaa287c1d296b5bdc6f8f6f9152d8c1b644 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 16 Dec 2016 16:36:46 +0100
Subject: [PATCH 02/13] Refactor: Extract method: for_report

---
 app/models/chargeback.rb                     | 44 +++++---------------
 app/models/chargeback/consumption_history.rb | 24 +++++++++++
 2 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/app/models/chargeback.rb b/app/models/chargeback.rb
index 8d6405436ca..ec10ce57943 100644
--- a/app/models/chargeback.rb
+++ b/app/models/chargeback.rb
@@ -14,44 +14,22 @@ def self.build_results_for_report_chargeback(options)
     _log.info("Calculating chargeback costs...")
     @options = options = ReportOptions.new_from_h(options)
 
-    rates = RatesCache.new
-    base_rollup = ConsumptionHistory.base_rollup_scope
-
-    timerange = options.report_time_range
     data = {}
+    rates = RatesCache.new
+    ConsumptionHistory.for_report(self, options) do |consumption, first_metric_rollup|
+      # we need to select ChargebackRates for groups of MetricRollups records
+      # and rates are selected by first MetricRollup record
+      rates_to_apply = rates.get(first_metric_rollup)
 
-    interval_duration = options.duration_of_report_step
-
-    timerange.step_value(interval_duration).each_cons(2) do |query_start_time, query_end_time|
-      records = base_rollup.where(:timestamp => query_start_time...query_end_time, :capture_interval_name => "hourly")
-      records = where_clause(records, options)
-      records = Metric::Helper.remove_duplicate_timestamps(records)
-      next if records.empty?
-      _log.info("Found #{records.length} records for time range #{[query_start_time, query_end_time].inspect}")
-
-      # we are building hash with grouped calculated values
-      # values are grouped by resource_id and timestamp (query_start_time...query_end_time)
-      records.group_by(&:resource_id).each do |_, metric_rollup_records|
-        metric_rollup_records = metric_rollup_records.select { |x| x.resource.present? }
-        consumption = Consumption.new(metric_rollup_records, query_start_time, query_end_time)
-        next if metric_rollup_records.empty?
-
-        # we need to select ChargebackRates for groups of MetricRollups records
-        # and rates are selected by first MetricRollup record
-        metric_rollup_record = metric_rollup_records.first
-        rates_to_apply = rates.get(metric_rollup_record)
-
-        key = report_row_key(metric_rollup_record)
-        data[key] ||= new(options, metric_rollup_record)
+      key = report_row_key(first_metric_rollup)
+      data[key] ||= new(options, first_metric_rollup)
 
-        chargeback_rates = data[key]["chargeback_rates"].split(', ') + rates_to_apply.collect(&:description)
-        data[key]["chargeback_rates"] = chargeback_rates.uniq.join(', ')
+      chargeback_rates = data[key]["chargeback_rates"].split(', ') + rates_to_apply.collect(&:description)
+      data[key]["chargeback_rates"] = chargeback_rates.uniq.join(', ')
 
-        # we are getting hash with metrics and costs for metrics defined for chargeback
-        data[key].calculate_costs(consumption, rates_to_apply)
-      end
+      # we are getting hash with metrics and costs for metrics defined for chargeback
+      data[key].calculate_costs(consumption, rates_to_apply)
     end
-
     _log.info("Calculating chargeback costs...Complete")
 
     [data.values]
diff --git a/app/models/chargeback/consumption_history.rb b/app/models/chargeback/consumption_history.rb
index ac96a40441f..2382c940fef 100644
--- a/app/models/chargeback/consumption_history.rb
+++ b/app/models/chargeback/consumption_history.rb
@@ -4,6 +4,29 @@ class ConsumptionHistory
       'v_derived_cpu_total_cores_used' => 'cpu_usage_rate_average'
     }.freeze
 
+    def self.for_report(cb_class, options)
+      base_rollup = base_rollup_scope
+      timerange = options.report_time_range
+      interval_duration = options.duration_of_report_step
+
+      timerange.step_value(interval_duration).each_cons(2) do |query_start_time, query_end_time|
+        records = base_rollup.where(:timestamp => query_start_time...query_end_time, :capture_interval_name => 'hourly')
+        records = cb_class.where_clause(records, options)
+        records = Metric::Helper.remove_duplicate_timestamps(records)
+        next if records.empty?
+        _log.info("Found #{records.length} records for time range #{[query_start_time, query_end_time].inspect}")
+
+        # we are building hash with grouped calculated values
+        # values are grouped by resource_id and timestamp (query_start_time...query_end_time)
+        records.group_by(&:resource_id).each do |_, metric_rollup_records|
+          metric_rollup_records = metric_rollup_records.select { |x| x.resource.present? }
+          consumption = Consumption.new(metric_rollup_records, query_start_time, query_end_time)
+          next if metric_rollup_records.empty?
+          yield(consumption, metric_rollup_records.first)
+        end
+      end
+    end
+
     def self.base_rollup_scope
       base_rollup = MetricRollup.includes(
         :resource           => [:hardware, :tenant, :tags, :vim_performance_states, :custom_attributes,
@@ -21,5 +44,6 @@ def self.base_rollup_scope
       rate_cols.map! { |x| VIRTUAL_COL_USES[x] || x }.flatten!
       base_rollup.select(*rate_cols)
     end
+    private_class_method :base_rollup_scope
   end
 end

From 900b9b0104e1e444ab1bacdb2ff71ef5657ce37c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 16 Dec 2016 16:55:05 +0100
Subject: [PATCH 03/13] Refactor: Extract method: key()

This is useful in world without metric_rollups.
---
 app/models/chargeback.rb             | 2 +-
 app/models/chargeback/consumption.rb | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/models/chargeback.rb b/app/models/chargeback.rb
index ec10ce57943..d9f9f174920 100644
--- a/app/models/chargeback.rb
+++ b/app/models/chargeback.rb
@@ -21,7 +21,7 @@ def self.build_results_for_report_chargeback(options)
       # and rates are selected by first MetricRollup record
       rates_to_apply = rates.get(first_metric_rollup)
 
-      key = report_row_key(first_metric_rollup)
+      key = consumption.key(self)
       data[key] ||= new(options, first_metric_rollup)
 
       chargeback_rates = data[key]["chargeback_rates"].split(', ') + rates_to_apply.collect(&:description)
diff --git a/app/models/chargeback/consumption.rb b/app/models/chargeback/consumption.rb
index f0e21d295c1..0e169c28cae 100644
--- a/app/models/chargeback/consumption.rb
+++ b/app/models/chargeback/consumption.rb
@@ -5,6 +5,10 @@ def initialize(metric_rollup_records, start_time, end_time)
       @start_time, @end_time = start_time, end_time
     end
 
+    def key(cb_class)
+      cb_class.report_row_key(@rollups.first)
+    end
+
     def max(metric)
       values(metric).max
     end

From 89542d78b5d93304ebc7b83e28862b7ef73e6447 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 16 Dec 2016 17:06:49 +0100
Subject: [PATCH 04/13] Extract method: first_metric_rollup_record

---
 app/models/chargeback/consumption.rb | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/models/chargeback/consumption.rb b/app/models/chargeback/consumption.rb
index 0e169c28cae..2cde602209d 100644
--- a/app/models/chargeback/consumption.rb
+++ b/app/models/chargeback/consumption.rb
@@ -5,8 +5,12 @@ def initialize(metric_rollup_records, start_time, end_time)
       @start_time, @end_time = start_time, end_time
     end
 
+    def first_metric_rollup_record
+      @rollups.first
+    end
+
     def key(cb_class)
-      cb_class.report_row_key(@rollups.first)
+      cb_class.report_row_key(first_metric_rollup_record)
     end
 
     def max(metric)

From 155275dc8a5ba7359e34128512af248f22e8bc83 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 16 Dec 2016 17:09:44 +0100
Subject: [PATCH 05/13] Do not pass metric_rollup to main loop of chargeback

Just pass the consumption object.

Note that we still depend on metric_rollup_record, but we are getting
close for not needing it.
---
 app/models/chargeback.rb                     | 6 +++---
 app/models/chargeback/consumption_history.rb | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/models/chargeback.rb b/app/models/chargeback.rb
index d9f9f174920..c98c7823efb 100644
--- a/app/models/chargeback.rb
+++ b/app/models/chargeback.rb
@@ -16,13 +16,13 @@ def self.build_results_for_report_chargeback(options)
 
     data = {}
     rates = RatesCache.new
-    ConsumptionHistory.for_report(self, options) do |consumption, first_metric_rollup|
+    ConsumptionHistory.for_report(self, options) do |consumption|
       # we need to select ChargebackRates for groups of MetricRollups records
       # and rates are selected by first MetricRollup record
-      rates_to_apply = rates.get(first_metric_rollup)
+      rates_to_apply = rates.get(consumption.first_metric_rollup_record)
 
       key = consumption.key(self)
-      data[key] ||= new(options, first_metric_rollup)
+      data[key] ||= new(options, consumption.first_metric_rollup_record)
 
       chargeback_rates = data[key]["chargeback_rates"].split(', ') + rates_to_apply.collect(&:description)
       data[key]["chargeback_rates"] = chargeback_rates.uniq.join(', ')
diff --git a/app/models/chargeback/consumption_history.rb b/app/models/chargeback/consumption_history.rb
index 2382c940fef..828afbf3148 100644
--- a/app/models/chargeback/consumption_history.rb
+++ b/app/models/chargeback/consumption_history.rb
@@ -22,7 +22,7 @@ def self.for_report(cb_class, options)
           metric_rollup_records = metric_rollup_records.select { |x| x.resource.present? }
           consumption = Consumption.new(metric_rollup_records, query_start_time, query_end_time)
           next if metric_rollup_records.empty?
-          yield(consumption, metric_rollup_records.first)
+          yield(consumption)
         end
       end
     end

From 520e273e8903330e8ebd3b81ee151e2285831920 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 16 Dec 2016 17:23:05 +0100
Subject: [PATCH 06/13] RatesCache should operate over Consumption not
 metric_rollup

---
 app/models/chargeback.rb             | 4 +---
 app/models/chargeback/rates_cache.rb | 5 ++++-
 spec/models/chargeback_vm_spec.rb    | 6 ++++--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/app/models/chargeback.rb b/app/models/chargeback.rb
index c98c7823efb..dd54738b4ea 100644
--- a/app/models/chargeback.rb
+++ b/app/models/chargeback.rb
@@ -17,9 +17,7 @@ def self.build_results_for_report_chargeback(options)
     data = {}
     rates = RatesCache.new
     ConsumptionHistory.for_report(self, options) do |consumption|
-      # we need to select ChargebackRates for groups of MetricRollups records
-      # and rates are selected by first MetricRollup record
-      rates_to_apply = rates.get(consumption.first_metric_rollup_record)
+      rates_to_apply = rates.get(consumption)
 
       key = consumption.key(self)
       data[key] ||= new(options, consumption.first_metric_rollup_record)
diff --git a/app/models/chargeback/rates_cache.rb b/app/models/chargeback/rates_cache.rb
index 4a23cf1f3de..085e966a3f8 100644
--- a/app/models/chargeback/rates_cache.rb
+++ b/app/models/chargeback/rates_cache.rb
@@ -1,6 +1,9 @@
 class Chargeback
   class RatesCache
-    def get(perf)
+    def get(consumption)
+      # we need to select ChargebackRates for groups of MetricRollups records
+      # and rates are selected by first MetricRollup record
+      perf = consumption.first_metric_rollup_record
       @rates ||= {}
       @rates[perf.hash_features_affecting_rate] ||= rates(perf)
     end
diff --git a/spec/models/chargeback_vm_spec.rb b/spec/models/chargeback_vm_spec.rb
index 2f002711594..68392bb2435 100644
--- a/spec/models/chargeback_vm_spec.rb
+++ b/spec/models/chargeback_vm_spec.rb
@@ -764,10 +764,11 @@ def used_average_for(metric, hours_in_interval)
                                                :parent_ems_id => ems.id, :parent_storage_id => @storage.id,
                                                :resource => @vm1)
     end
+    let(:consumption) { Chargeback::Consumption.new([metric_rollup], nil, nil) }
 
     before do
       ChargebackRate.set_assignments(:compute, [rate_assignment_options])
-      @rate = Chargeback::RatesCache.new.get(metric_rollup).first
+      @rate = Chargeback::RatesCache.new.get(consumption).first
       @assigned_rate = ChargebackRate.get_assignments("Compute").first
     end
 
@@ -848,6 +849,7 @@ def used_average_for(metric, hours_in_interval)
                          :parent_ems_id => ems.id, :parent_storage_id => @storage.id,
                          :resource => @vm1)
     end
+    let(:consumption) { Chargeback::Consumption.new([metric_rollup], nil, nil) }
 
     before do
       @storage.tag_with([classification_1.tag.name, classification_2.tag.name], :ns => '*')
@@ -857,7 +859,7 @@ def used_average_for(metric, hours_in_interval)
     it "return only one chargeback rate according to tag name of Vm" do
       [rate_assignment_options_1, rate_assignment_options_2].each do |rate_assignment|
         metric_rollup.tag_names = rate_assignment[:tag].first.tag.send(:name_path)
-        uniq_rates = chargeback_vm.send(:get, metric_rollup)
+        uniq_rates = chargeback_vm.get(consumption)
         expect([rate_assignment[:cb_rate]]).to match_array(uniq_rates)
       end
     end

From de51196295a9c0d9ce37d9b8e76904b991fd8530 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 16 Dec 2016 17:34:36 +0100
Subject: [PATCH 07/13] Initialize chargeback instances with consumption

And delegate resource and timestamp to first_metric_rollup_record
---
 app/models/chargeback.rb             |  9 ++---
 app/models/chargeback/consumption.rb |  2 ++
 spec/models/chargeback_vm_spec.rb    | 49 ++++++++++++++--------------
 3 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/app/models/chargeback.rb b/app/models/chargeback.rb
index dd54738b4ea..73ed78d5281 100644
--- a/app/models/chargeback.rb
+++ b/app/models/chargeback.rb
@@ -20,7 +20,7 @@ def self.build_results_for_report_chargeback(options)
       rates_to_apply = rates.get(consumption)
 
       key = consumption.key(self)
-      data[key] ||= new(options, consumption.first_metric_rollup_record)
+      data[key] ||= new(options, consumption)
 
       chargeback_rates = data[key]["chargeback_rates"].split(', ') + rates_to_apply.collect(&:description)
       data[key]["chargeback_rates"] = chargeback_rates.uniq.join(', ')
@@ -54,7 +54,8 @@ def self.classification_for_perf(metric_rollup_record)
     @options.tag_hash[tag]
   end
 
-  def initialize(options, metric_rollup_record)
+  def initialize(options, consumption)
+    metric_rollup_record = consumption.first_metric_rollup_record
     @options = options
     super()
     if @options[:groupby_tag].present?
@@ -63,10 +64,10 @@ def initialize(options, metric_rollup_record)
     else
       init_extra_fields(metric_rollup_record)
     end
-    self.start_date, self.end_date, self.display_range = options.report_step_range(metric_rollup_record.timestamp)
+    self.start_date, self.end_date, self.display_range = options.report_step_range(consumption.timestamp)
     self.interval_name = options.interval
     self.chargeback_rates = ''
-    self.entity = metric_rollup_record.resource
+    self.entity = consumption.resource
   end
 
   def calculate_costs(consumption, rates)
diff --git a/app/models/chargeback/consumption.rb b/app/models/chargeback/consumption.rb
index 2cde602209d..04a515e9ed8 100644
--- a/app/models/chargeback/consumption.rb
+++ b/app/models/chargeback/consumption.rb
@@ -1,5 +1,7 @@
 class Chargeback
   class Consumption
+    delegate :timestamp, :resource, :to => :first_metric_rollup_record
+
     def initialize(metric_rollup_records, start_time, end_time)
       @rollups = metric_rollup_records
       @start_time, @end_time = start_time, end_time
diff --git a/spec/models/chargeback_vm_spec.rb b/spec/models/chargeback_vm_spec.rb
index 68392bb2435..432fa16df80 100644
--- a/spec/models/chargeback_vm_spec.rb
+++ b/spec/models/chargeback_vm_spec.rb
@@ -794,40 +794,41 @@ def used_average_for(metric, hours_in_interval)
   describe '#initialize' do
     let(:report_options) { Chargeback::ReportOptions.new }
     let(:vm_owners)     { {@vm1.id => @vm1.evm_owner_name} }
-    let(:metric_rollup) do
-      FactoryGirl.build(:metric_rollup_vm_hr, :tag_names => 'environment/prod',
-                        :parent_host_id => @host1.id, :parent_ems_cluster_id => @ems_cluster.id,
-                        :parent_ems_id => ems.id, :parent_storage_id => @storage.id,
-                        :resource => @vm1, :resource_name => @vm1.name)
+    let(:consumption) { Chargeback::Consumption.new([metric_rollup], nil, nil) }
+    let(:shared_extra_fields) do
+      {'vm_name' => @vm1.name, 'owner_name' => admin.name, 'vm_uid' => 'ems_ref', 'vm_guid' => @vm1.guid,
+       'vm_id' => @vm1.id}
     end
+    subject { ChargebackVm.new(report_options, consumption).attributes }
 
     before do
       ChargebackVm.instance_variable_set(:@vm_owners, vm_owners)
     end
 
-    it 'sets extra fields' do
-      extra_fields = ChargebackVm.new(report_options, metric_rollup).attributes
-      expected_fields = {"vm_name" => @vm1.name, "owner_name" => admin.name, "provider_name" => ems.name,
-                         "provider_uid" => ems.guid, "vm_uid" => "ems_ref", "vm_guid" => @vm1.guid,
-                         "vm_id" => @vm1.id}
-
-      expect(extra_fields).to include(expected_fields)
-    end
+    context 'with parent ems' do
+      let(:metric_rollup) do
+        FactoryGirl.build(:metric_rollup_vm_hr, :tag_names => 'environment/prod',
+                          :parent_host_id => @host1.id, :parent_ems_cluster_id => @ems_cluster.id,
+                          :parent_ems_id => ems.id, :parent_storage_id => @storage.id,
+                          :resource => @vm1, :resource_name => @vm1.name)
+      end
 
-    let(:metric_rollup_without_ems) do
-      FactoryGirl.build(:metric_rollup_vm_hr, :tag_names => 'environment/prod',
-                        :parent_host_id => @host1.id, :parent_ems_cluster_id => @ems_cluster.id,
-                        :parent_storage_id => @storage.id,
-                        :resource => @vm1, :resource_name => @vm1.name)
+      it 'sets extra fields' do
+        is_expected.to include(shared_extra_fields.merge('provider_name' => ems.name, 'provider_uid' => ems.guid))
+      end
     end
 
-    it 'sets extra fields when parent ems is missing' do
-      extra_fields = ChargebackVm.new(report_options, metric_rollup_without_ems).attributes
-      expected_fields = {"vm_name" => @vm1.name, "owner_name" => admin.name, "provider_name" => nil,
-                         "provider_uid" => nil, "vm_uid" => "ems_ref", "vm_guid" => @vm1.guid,
-                         "vm_id" => @vm1.id}
+    context 'when parent ems is missing' do
+      let(:metric_rollup) do
+        FactoryGirl.build(:metric_rollup_vm_hr, :tag_names => 'environment/prod',
+                          :parent_host_id => @host1.id, :parent_ems_cluster_id => @ems_cluster.id,
+                          :parent_storage_id => @storage.id,
+                          :resource => @vm1, :resource_name => @vm1.name)
+      end
 
-      expect(extra_fields).to include(expected_fields)
+      it 'sets extra fields when parent ems is missing' do
+        is_expected.to include(shared_extra_fields.merge('provider_name' => nil, 'provider_uid' => nil))
+      end
     end
   end
 

From 77d87882f198c288c5be41c919dfe050af5b8b0a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 16 Dec 2016 18:00:57 +0100
Subject: [PATCH 08/13] init_extra_fields should depend only on consumption
 object

---
 app/models/chargeback.rb                   |  2 +-
 app/models/chargeback/consumption.rb       |  2 +-
 app/models/chargeback_container_image.rb   | 25 +++++++++++-----------
 app/models/chargeback_container_project.rb | 12 +++++------
 app/models/chargeback_vm.rb                | 20 ++++++++---------
 5 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/app/models/chargeback.rb b/app/models/chargeback.rb
index 73ed78d5281..330bf326963 100644
--- a/app/models/chargeback.rb
+++ b/app/models/chargeback.rb
@@ -62,7 +62,7 @@ def initialize(options, consumption)
       classification = self.class.classification_for_perf(metric_rollup_record)
       self.tag_name = classification.present? ? classification.description : _('<Empty>')
     else
-      init_extra_fields(metric_rollup_record)
+      init_extra_fields(consumption)
     end
     self.start_date, self.end_date, self.display_range = options.report_step_range(consumption.timestamp)
     self.interval_name = options.interval
diff --git a/app/models/chargeback/consumption.rb b/app/models/chargeback/consumption.rb
index 04a515e9ed8..a2754374947 100644
--- a/app/models/chargeback/consumption.rb
+++ b/app/models/chargeback/consumption.rb
@@ -1,6 +1,6 @@
 class Chargeback
   class Consumption
-    delegate :timestamp, :resource, :to => :first_metric_rollup_record
+    delegate :timestamp, :resource, :resource_id, :resource_name, :parent_ems, :to => :first_metric_rollup_record
 
     def initialize(metric_rollup_records, start_time, end_time)
       @rollups = metric_rollup_records
diff --git a/app/models/chargeback_container_image.rb b/app/models/chargeback_container_image.rb
index cf0007370e7..5ccccf21fc0 100644
--- a/app/models/chargeback_container_image.rb
+++ b/app/models/chargeback_container_image.rb
@@ -53,12 +53,12 @@ def self.default_key(metric_rollup_record, ts_key)
     @options[:groupby] == 'project' ? "#{project.id}_#{ts_key}" : "#{project.id}_#{image.id}_#{ts_key}"
   end
 
-  def self.image(perf)
-    @data_index.fetch_path(:container_image, :by_container_id, perf.resource_id)
+  def self.image(consumption)
+    @data_index.fetch_path(:container_image, :by_container_id, consumption.resource_id)
   end
 
-  def self.project(perf)
-    @data_index.fetch_path(:container_project, :by_container_id, perf.resource_id)
+  def self.project(consumption)
+    @data_index.fetch_path(:container_project, :by_container_id, consumption.resource_id)
   end
 
   def self.where_clause(records, _options)
@@ -87,13 +87,14 @@ def self.report_col_options
 
   private
 
-  def init_extra_fields(perf)
-    self.project_name  = self.class.project(perf).name
-    self.image_name    = self.class.image(perf).try(:full_name) || _('Deleted') # until image archiving is implemented
-    self.project_uid   = self.class.project(perf).ems_ref
-    self.provider_name = perf.parent_ems.try(:name)
-    self.provider_uid  = perf.parent_ems.try(:guid)
-    self.archived      = self.class.project(perf).archived? ? _('Yes') : _('No')
-    self.entity        = self.class.image(perf)
+  def init_extra_fields(consumption)
+    self.project_name  = self.class.project(consumption).name
+    # until image archiving is implemented
+    self.image_name    = self.class.image(consumption).try(:full_name) || _('Deleted')
+    self.project_uid   = self.class.project(consumption).ems_ref
+    self.provider_name = consumption.parent_ems.try(:name)
+    self.provider_uid  = consumption.parent_ems.try(:guid)
+    self.archived      = self.class.project(consumption).archived? ? _('Yes') : _('No')
+    self.entity        = self.class.image(consumption)
   end
 end # class ChargebackContainerImage
diff --git a/app/models/chargeback_container_project.rb b/app/models/chargeback_container_project.rb
index 959fb9bc108..ac18ffc5868 100644
--- a/app/models/chargeback_container_project.rb
+++ b/app/models/chargeback_container_project.rb
@@ -70,11 +70,11 @@ def self.report_col_options
 
   private
 
-  def init_extra_fields(perf)
-    self.project_name  = perf.resource_name
-    self.project_uid   = perf.resource.ems_ref
-    self.provider_name = perf.parent_ems.try(:name)
-    self.provider_uid  = perf.parent_ems.try(:guid)
-    self.archived      = perf.resource.archived? ? _('Yes') : _('No')
+  def init_extra_fields(consumption)
+    self.project_name  = consumption.resource_name
+    self.project_uid   = consumption.resource.ems_ref
+    self.provider_name = consumption.parent_ems.try(:name)
+    self.provider_uid  = consumption.parent_ems.try(:guid)
+    self.archived      = consumption.resource.archived? ? _('Yes') : _('No')
   end
 end # class Chargeback
diff --git a/app/models/chargeback_vm.rb b/app/models/chargeback_vm.rb
index 00106d22109..ed51824c346 100644
--- a/app/models/chargeback_vm.rb
+++ b/app/models/chargeback_vm.rb
@@ -92,9 +92,9 @@ def self.report_col_options
     }
   end
 
-  def self.vm_owner(perf)
+  def self.vm_owner(consumption)
     @vm_owners ||= vms.each_with_object({}) { |vm, res| res[vm.id] = vm.evm_owner_name }
-    @vm_owners[perf.resource_id] ||= perf.resource.evm_owner_name
+    @vm_owners[consumption.resource_id] ||= consumption.resource.evm_owner_name
   end
 
   def self.vms
@@ -134,13 +134,13 @@ def self.vms
 
   private
 
-  def init_extra_fields(perf)
-    self.vm_id         = perf.resource_id
-    self.vm_name       = perf.resource_name
-    self.vm_uid        = perf.resource.ems_ref
-    self.vm_guid       = perf.resource.try(:guid)
-    self.owner_name    = self.class.vm_owner(perf)
-    self.provider_name = perf.parent_ems.try(:name)
-    self.provider_uid  = perf.parent_ems.try(:guid)
+  def init_extra_fields(consumption)
+    self.vm_id         = consumption.resource_id
+    self.vm_name       = consumption.resource_name
+    self.vm_uid        = consumption.resource.ems_ref
+    self.vm_guid       = consumption.resource.try(:guid)
+    self.owner_name    = self.class.vm_owner(consumption)
+    self.provider_name = consumption.parent_ems.try(:name)
+    self.provider_uid  = consumption.parent_ems.try(:guid)
   end
 end # class Chargeback

From d52c11825eaa9eb92e870328f2433b499bae4127 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 16 Dec 2016 18:20:36 +0100
Subject: [PATCH 09/13] report_row_key should take just consumption class

---
 app/models/chargeback.rb             | 10 +++++-----
 app/models/chargeback/consumption.rb |  4 ----
 spec/models/chargeback_vm_spec.rb    |  3 ++-
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/app/models/chargeback.rb b/app/models/chargeback.rb
index 330bf326963..52300081f33 100644
--- a/app/models/chargeback.rb
+++ b/app/models/chargeback.rb
@@ -19,7 +19,7 @@ def self.build_results_for_report_chargeback(options)
     ConsumptionHistory.for_report(self, options) do |consumption|
       rates_to_apply = rates.get(consumption)
 
-      key = consumption.key(self)
+      key = report_row_key(consumption)
       data[key] ||= new(options, consumption)
 
       chargeback_rates = data[key]["chargeback_rates"].split(', ') + rates_to_apply.collect(&:description)
@@ -33,14 +33,14 @@ def self.build_results_for_report_chargeback(options)
     [data.values]
   end
 
-  def self.report_row_key(metric_rollup_record)
-    ts_key = @options.start_of_report_step(metric_rollup_record.timestamp)
+  def self.report_row_key(consumption)
+    ts_key = @options.start_of_report_step(consumption.timestamp)
     if @options[:groupby_tag].present?
-      classification = classification_for_perf(metric_rollup_record)
+      classification = classification_for_perf(consumption.first_metric_rollup_record)
       classification_id = classification.present? ? classification.id : 'none'
       "#{classification_id}_#{ts_key}"
     else
-      default_key(metric_rollup_record, ts_key)
+      default_key(consumption.first_metric_rollup_record, ts_key)
     end
   end
 
diff --git a/app/models/chargeback/consumption.rb b/app/models/chargeback/consumption.rb
index a2754374947..a14877626b7 100644
--- a/app/models/chargeback/consumption.rb
+++ b/app/models/chargeback/consumption.rb
@@ -11,10 +11,6 @@ def first_metric_rollup_record
       @rollups.first
     end
 
-    def key(cb_class)
-      cb_class.report_row_key(first_metric_rollup_record)
-    end
-
     def max(metric)
       values(metric).max
     end
diff --git a/spec/models/chargeback_vm_spec.rb b/spec/models/chargeback_vm_spec.rb
index 432fa16df80..1a35e34eab6 100644
--- a/spec/models/chargeback_vm_spec.rb
+++ b/spec/models/chargeback_vm_spec.rb
@@ -783,7 +783,8 @@ def used_average_for(metric, hours_in_interval)
     let(:timestamp_key) { 'Fri, 13 May 2016 10:40:00 UTC +00:00' }
     let(:beginning_of_day) { timestamp_key.in_time_zone.beginning_of_day }
     let(:metric_rollup) { FactoryGirl.build(:metric_rollup_vm_hr, :timestamp => timestamp_key, :resource => @vm1) }
-    subject { described_class.report_row_key(metric_rollup) }
+    let(:consumption) { Chargeback::Consumption.new([metric_rollup], nil, nil) }
+    subject { described_class.report_row_key(consumption) }
     before do
       described_class.instance_variable_set(:@options, report_options)
     end

From dc607067af7e8bb3ec3f4c1911a1c25ad2131c55 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 16 Dec 2016 18:30:21 +0100
Subject: [PATCH 10/13] classification_for should not depend on metric_rollup

---
 app/models/chargeback.rb             | 9 ++++-----
 app/models/chargeback/consumption.rb | 4 ++++
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/app/models/chargeback.rb b/app/models/chargeback.rb
index 52300081f33..11a49ad2e85 100644
--- a/app/models/chargeback.rb
+++ b/app/models/chargeback.rb
@@ -36,7 +36,7 @@ def self.build_results_for_report_chargeback(options)
   def self.report_row_key(consumption)
     ts_key = @options.start_of_report_step(consumption.timestamp)
     if @options[:groupby_tag].present?
-      classification = classification_for_perf(consumption.first_metric_rollup_record)
+      classification = classification_for(consumption)
       classification_id = classification.present? ? classification.id : 'none'
       "#{classification_id}_#{ts_key}"
     else
@@ -48,18 +48,17 @@ def self.default_key(metric_rollup_record, ts_key)
     "#{metric_rollup_record.resource_id}_#{ts_key}"
   end
 
-  def self.classification_for_perf(metric_rollup_record)
-    tag = metric_rollup_record.tag_names.split('|').find { |x| x.starts_with?(@options[:groupby_tag]) } # 'department/*'
+  def self.classification_for(consumption)
+    tag = consumption.tag_names.find { |x| x.starts_with?(@options[:groupby_tag]) } # 'department/*'
     tag = tag.split('/').second unless tag.blank? # 'department/finance' -> 'finance'
     @options.tag_hash[tag]
   end
 
   def initialize(options, consumption)
-    metric_rollup_record = consumption.first_metric_rollup_record
     @options = options
     super()
     if @options[:groupby_tag].present?
-      classification = self.class.classification_for_perf(metric_rollup_record)
+      classification = self.class.classification_for(consumption)
       self.tag_name = classification.present? ? classification.description : _('<Empty>')
     else
       init_extra_fields(consumption)
diff --git a/app/models/chargeback/consumption.rb b/app/models/chargeback/consumption.rb
index a14877626b7..e1740c3b211 100644
--- a/app/models/chargeback/consumption.rb
+++ b/app/models/chargeback/consumption.rb
@@ -11,6 +11,10 @@ def first_metric_rollup_record
       @rollups.first
     end
 
+    def tag_names
+      first_metric_rollup_record.tag_names.split('|')
+    end
+
     def max(metric)
       values(metric).max
     end

From 2f8fdc2960c061ffd9de614bdc1b9e4a0910db37 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 16 Dec 2016 18:32:15 +0100
Subject: [PATCH 11/13] default_key should not depend on metric_rollup

---
 app/models/chargeback.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/models/chargeback.rb b/app/models/chargeback.rb
index 11a49ad2e85..ae11b90d287 100644
--- a/app/models/chargeback.rb
+++ b/app/models/chargeback.rb
@@ -40,12 +40,12 @@ def self.report_row_key(consumption)
       classification_id = classification.present? ? classification.id : 'none'
       "#{classification_id}_#{ts_key}"
     else
-      default_key(consumption.first_metric_rollup_record, ts_key)
+      default_key(consumption, ts_key)
     end
   end
 
-  def self.default_key(metric_rollup_record, ts_key)
-    "#{metric_rollup_record.resource_id}_#{ts_key}"
+  def self.default_key(consumption, ts_key)
+    "#{consumption.resource_id}_#{ts_key}"
   end
 
   def self.classification_for(consumption)

From ff265d7b801e0e11298d82b02e39786f61254a45 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 16 Dec 2016 20:15:44 +0100
Subject: [PATCH 12/13] RatesCaches should not depend on metric_rollups

---
 app/models/chargeback/consumption.rb |  4 +++-
 app/models/chargeback/rates_cache.rb | 15 +++++++--------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/app/models/chargeback/consumption.rb b/app/models/chargeback/consumption.rb
index e1740c3b211..a471387c48e 100644
--- a/app/models/chargeback/consumption.rb
+++ b/app/models/chargeback/consumption.rb
@@ -1,6 +1,8 @@
 class Chargeback
   class Consumption
-    delegate :timestamp, :resource, :resource_id, :resource_name, :parent_ems, :to => :first_metric_rollup_record
+    delegate :timestamp, :resource, :resource_id, :resource_name, :resource_type, :parent_ems,
+             :hash_features_affecting_rate, :tag_list_with_prefix, :parents_determining_rate,
+             :to => :first_metric_rollup_record
 
     def initialize(metric_rollup_records, start_time, end_time)
       @rollups = metric_rollup_records
diff --git a/app/models/chargeback/rates_cache.rb b/app/models/chargeback/rates_cache.rb
index 085e966a3f8..ce07adfb6fd 100644
--- a/app/models/chargeback/rates_cache.rb
+++ b/app/models/chargeback/rates_cache.rb
@@ -3,23 +3,22 @@ class RatesCache
     def get(consumption)
       # we need to select ChargebackRates for groups of MetricRollups records
       # and rates are selected by first MetricRollup record
-      perf = consumption.first_metric_rollup_record
       @rates ||= {}
-      @rates[perf.hash_features_affecting_rate] ||= rates(perf)
+      @rates[consumption.hash_features_affecting_rate] ||= rates(consumption)
     end
 
     private
 
-    def rates(metric_rollup_record)
-      rates = ChargebackRate.get_assigned_for_target(metric_rollup_record.resource,
-                                                     :tag_list => metric_rollup_record.tag_list_with_prefix,
-                                                     :parents  => metric_rollup_record.parents_determining_rate)
+    def rates(consumption)
+      rates = ChargebackRate.get_assigned_for_target(consumption.resource,
+                                                     :tag_list => consumption.tag_list_with_prefix,
+                                                     :parents  => consumption.parents_determining_rate)
 
-      if metric_rollup_record.resource_type == Container.name && rates.empty?
+      if consumption.resource_type == Container.name && rates.empty?
         rates = [ChargebackRate.find_by(:description => "Default Container Image Rate", :rate_type => "Compute")]
       end
 
-      metric_rollup_record_tags = metric_rollup_record.tag_names.split("|")
+      metric_rollup_record_tags = consumption.tag_names
 
       unique_rates_by_tagged_resources(rates, metric_rollup_record_tags)
     end

From 0ac994e2e088f2eb0c2f67fd47da05e1cb0a293f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 16 Dec 2016 18:33:05 +0100
Subject: [PATCH 13/13] Make first_metric_rollup_private

---
 app/models/chargeback/consumption.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/models/chargeback/consumption.rb b/app/models/chargeback/consumption.rb
index a471387c48e..f724c34e270 100644
--- a/app/models/chargeback/consumption.rb
+++ b/app/models/chargeback/consumption.rb
@@ -9,10 +9,6 @@ def initialize(metric_rollup_records, start_time, end_time)
       @start_time, @end_time = start_time, end_time
     end
 
-    def first_metric_rollup_record
-      @rollups.first
-    end
-
     def tag_names
       first_metric_rollup_record.tag_names.split('|')
     end
@@ -44,5 +40,9 @@ def values(metric)
       @values ||= {}
       @values[metric] ||= @rollups.collect(&metric.to_sym).compact
     end
+
+    def first_metric_rollup_record
+      @fmrr ||= @rollups.first
+    end
   end
 end