Refactor to organize responsibilities

Don't let the model do all the hard work, split responsibilities.

app/models/ci/daily_report_result.rb

@@ -10,29 +10,12 @@ module Ci
     # TODO: Refactor this out when BuildReportResult is implemented.
     # They both need to share the same enum values for param.
     REPORT_PARAMS = {
-      coverage: 12
+      coverage: 0
     }.freeze
 
     enum param_type: REPORT_PARAMS
 
-    def self.store_coverage(pipeline)
-      return unless Feature.enabled?(:ci_daily_code_coverage, default_enabled: true)
-
-      base_attrs = {
-        project_id: pipeline.project_id,
-        ref_path: pipeline.source_ref_path,
-        param_type: param_types[:coverage],
-        date: pipeline.created_at.to_date,
-        last_pipeline_id: pipeline.id
-      }
-
-      data = pipeline.builds.with_coverage.map do |build|
-        base_attrs.merge(
-          title: build.group_name,
-          value: build.coverage
-        )
-      end
-
+    def self.upsert_reports(data)
       upsert_all(data, unique_by: :index_daily_report_results_unique_columns) if data.any?
     end
   end

app/services/ci/daily_report_result_service.rb

@@ -3,7 +3,28 @@
 module Ci
   class DailyReportResultService
     def execute(pipeline)
-      DailyReportResult.store_coverage(pipeline)
+      return unless Feature.enabled?(:ci_daily_code_coverage, default_enabled: true)
+
+      DailyReportResult.upsert_reports(coverage_reports(pipeline))
+    end
+
+    private
+
+    def coverage_reports(pipeline)
+      base_attrs = {
+        project_id: pipeline.project_id,
+        ref_path: pipeline.source_ref_path,
+        param_type: DailyReportResult.param_types[:coverage],
+        date: pipeline.created_at.to_date,
+        last_pipeline_id: pipeline.id
+      }
+
+      pipeline.builds.with_coverage.map do |build|
+        base_attrs.merge(
+          title: build.group_name,
+          value: build.coverage
+        )
+      end
     end
   end
 end

db/migrate/20200204131831_create_daily_report_results.rb

@@ -9,7 +9,7 @@ class CreateDailyReportResults < ActiveRecord::Migration[6.0]
       t.bigint :project_id, null: false
       t.bigint :last_pipeline_id, null: false
       t.float :value, null: false
-      t.integer :param_type, limit: 2, null: false
+      t.integer :param_type, limit: 8, null: false
       t.string :ref_path, null: false # rubocop:disable Migration/AddLimitToStringColumns
       t.string :title, null: false # rubocop:disable Migration/AddLimitToStringColumns
 

db/schema.rb

@@ -743,7 +743,7 @@ ActiveRecord::Schema.define(version: 2020_03_13_123934) do
     t.bigint "project_id", null: false
     t.bigint "last_pipeline_id", null: false
     t.float "value", null: false
-    t.integer "param_type", limit: 2, null: false
+    t.bigint "param_type", null: false
     t.string "ref_path", null: false
     t.string "title", null: false
     t.index ["last_pipeline_id"], name: "index_ci_daily_report_results_on_last_pipeline_id"

spec/db/schema_spec.rb

@@ -132,6 +132,7 @@ describe 'Database schema' do
     'Ci::Build' => %w[failure_reason],
     'Ci::BuildMetadata' => %w[timeout_source],
     'Ci::BuildTraceChunk' => %w[data_store],
+    'Ci::DailyReportResult' => %w[param_type],
     'Ci::JobArtifact' => %w[file_type],
     'Ci::Pipeline' => %w[source config_source failure_reason],
     'Ci::Processable' => %w[failure_reason],

spec/models/ci/daily_report_result_spec.rb

@@ -3,139 +3,59 @@
 require 'spec_helper'
 
 describe Ci::DailyReportResult do
-  describe '::store_coverage' do
-    let!(:pipeline) { create(:ci_pipeline, created_at: '2020-02-06 00:01:10') }
-    let!(:rspec_job) { create(:ci_build, pipeline: pipeline, name: '3/3 rspec', coverage: 80) }
-    let!(:karma_job) { create(:ci_build, pipeline: pipeline, name: '2/2 karma', coverage: 90) }
-    let!(:extra_job) { create(:ci_build, pipeline: pipeline, name: 'extra', coverage: nil) }
-
-    it 'creates daily code coverage record for each job in the pipeline that has coverage value' do
-      described_class.store_coverage(pipeline)
-
-      Ci::DailyReportResult.find_by(title: 'rspec').tap do |coverage|
-        expect(coverage).to have_attributes(
-          project_id: pipeline.project.id,
-          last_pipeline_id: pipeline.id,
-          ref_path: pipeline.source_ref_path,
-          param_type: 'coverage',
-          title: rspec_job.group_name,
-          value: rspec_job.coverage,
-          date: pipeline.created_at.to_date
-        )
-      end
-
-      Ci::DailyReportResult.find_by(title: 'karma').tap do |coverage|
-        expect(coverage).to have_attributes(
-          project_id: pipeline.project.id,
-          last_pipeline_id: pipeline.id,
-          ref_path: pipeline.source_ref_path,
-          param_type: 'coverage',
-          title: karma_job.group_name,
-          value: karma_job.coverage,
-          date: pipeline.created_at.to_date
-        )
-      end
-
-      expect(Ci::DailyReportResult.find_by(title: 'extra')).to be_nil
-    end
-
-    context 'when there is an existing daily code coverage for the matching date, project, ref_path, and group name' do
-      let!(:new_pipeline) do
+  describe '.upsert_reports' do
+    let!(:rspec_coverage) do
       create(
-          :ci_pipeline,
-          project: pipeline.project,
-          ref: pipeline.ref,
-          created_at: '2020-02-06 00:02:20'
+        :ci_daily_report_result,
+        title: 'rspec',
+        date: '2020-03-09',
+        value: 71.2
       )
     end
-      let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: '4/4 rspec', coverage: 84) }
-      let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: '3/3 karma', coverage: 92) }
+    let!(:new_pipeline) { create(:ci_pipeline) }
 
-      before do
-        # Create the existing daily code coverage records
-        described_class.store_coverage(pipeline)
-      end
-
-      it "updates the existing record's coverage value and last_pipeline_id" do
-        rspec_coverage = Ci::DailyReportResult.find_by(title: 'rspec')
-        karma_coverage = Ci::DailyReportResult.find_by(title: 'karma')
-
-        # Bump up the coverage values
-        described_class.store_coverage(new_pipeline)
-
-        rspec_coverage.reload
-        karma_coverage.reload
-
-        expect(rspec_coverage).to have_attributes(
+    it 'creates or updates matching report results' do
+      described_class.upsert_reports([
+        {
+          project_id: rspec_coverage.project_id,
+          ref_path: rspec_coverage.ref_path,
+          param_type: described_class.param_types[rspec_coverage.param_type],
           last_pipeline_id: new_pipeline.id,
-          value: new_rspec_job.coverage
-        )
-
-        expect(karma_coverage).to have_attributes(
+          date: rspec_coverage.date,
+          title: 'rspec',
+          value: 81.0
+        },
+        {
+          project_id: rspec_coverage.project_id,
+          ref_path: rspec_coverage.ref_path,
+          param_type: described_class.param_types[rspec_coverage.param_type],
           last_pipeline_id: new_pipeline.id,
-          value: new_karma_job.coverage
-        )
-      end
-    end
-
-    context 'when the ID of the pipeline is older than the last_pipeline_id' do
-      let!(:new_pipeline) do
-        create(
-          :ci_pipeline,
-          project: pipeline.project,
-          ref: pipeline.ref,
-          created_at: '2020-02-06 00:02:20'
-        )
-      end
-      let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: '4/4 rspec', coverage: 84) }
-      let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: '3/3 karma', coverage: 92) }
-
-      before do
-        # Create the existing daily code coverage records
-        # but in this case, for the newer pipeline first.
-        described_class.store_coverage(new_pipeline)
-      end
-
-      it 'updates the existing daily code coverage records' do
-        rspec_coverage = Ci::DailyReportResult.find_by(title: 'rspec')
-        karma_coverage = Ci::DailyReportResult.find_by(title: 'karma')
-
-        # Run another one but for the older pipeline.
-        # This simulates the scenario wherein the success worker
-        # of an older pipeline, for some network hiccup, was delayed
-        # and only got executed right after the newer pipeline's success worker.
-        # Ideally, we don't want to bump the coverage value with an older one
-        # but given this can be a rare edge case and can be remedied by re-running
-        # the pipeline we'll just let it be for now. In return, we are able to use
-        # Rails 6 shiny new method, upsert_all, and simplify the code a lot.
-        described_class.store_coverage(pipeline)
+          date: rspec_coverage.date,
+          title: 'karma',
+          value: 87.0
+        }
+      ])
 
       rspec_coverage.reload
-        karma_coverage.reload
 
       expect(rspec_coverage).to have_attributes(
-          last_pipeline_id: pipeline.id,
-          value: rspec_job.coverage
+        last_pipeline_id: new_pipeline.id,
+        value: 81.0
       )
 
-        expect(karma_coverage).to have_attributes(
-          last_pipeline_id: pipeline.id,
-          value: karma_job.coverage
-        )
-      end
-    end
-
-    context 'when pipeline has no builds with coverage' do
-      let!(:new_pipeline) do
-        create(
-          :ci_pipeline,
-          created_at: '2020-02-06 00:02:20'
+      expect(described_class.find_by_title('karma')).to have_attributes(
+        project_id: rspec_coverage.project_id,
+        ref_path: rspec_coverage.ref_path,
+        param_type: rspec_coverage.param_type,
+        last_pipeline_id: new_pipeline.id,
+        date: rspec_coverage.date,
+        value: 87.0
       )
     end
-      let!(:some_job) { create(:ci_build, pipeline: new_pipeline, name: 'foo') }
 
+    context 'when given data is empty' do
       it 'does nothing' do
-        expect { described_class.store_coverage(new_pipeline) }.not_to raise_error
+        expect { described_class.upsert_reports([]) }.not_to raise_error
       end
     end
   end

spec/services/ci/daily_report_result_service_spec.rb

@@ -3,11 +3,138 @@
 require 'spec_helper'
 
 describe Ci::DailyReportResultService, '#execute' do
-  let(:pipeline) { double }
+  let!(:pipeline) { create(:ci_pipeline, created_at: '2020-02-06 00:01:10') }
+  let!(:rspec_job) { create(:ci_build, pipeline: pipeline, name: '3/3 rspec', coverage: 80) }
+  let!(:karma_job) { create(:ci_build, pipeline: pipeline, name: '2/2 karma', coverage: 90) }
+  let!(:extra_job) { create(:ci_build, pipeline: pipeline, name: 'extra', coverage: nil) }
 
-  it 'stores daily code coverage' do
-    expect(Ci::DailyReportResult).to receive(:store_coverage).with(pipeline)
+  it 'creates daily code coverage record for each job in the pipeline that has coverage value' do
+    described_class.new.execute(pipeline)
+
+    Ci::DailyReportResult.find_by(title: 'rspec').tap do |coverage|
+      expect(coverage).to have_attributes(
+        project_id: pipeline.project.id,
+        last_pipeline_id: pipeline.id,
+        ref_path: pipeline.source_ref_path,
+        param_type: 'coverage',
+        title: rspec_job.group_name,
+        value: rspec_job.coverage,
+        date: pipeline.created_at.to_date
+      )
+    end
+
+    Ci::DailyReportResult.find_by(title: 'karma').tap do |coverage|
+      expect(coverage).to have_attributes(
+        project_id: pipeline.project.id,
+        last_pipeline_id: pipeline.id,
+        ref_path: pipeline.source_ref_path,
+        param_type: 'coverage',
+        title: karma_job.group_name,
+        value: karma_job.coverage,
+        date: pipeline.created_at.to_date
+      )
+    end
+
+    expect(Ci::DailyReportResult.find_by(title: 'extra')).to be_nil
+  end
+
+  context 'when there is an existing daily code coverage for the matching date, project, ref_path, and group name' do
+    let!(:new_pipeline) do
+      create(
+        :ci_pipeline,
+        project: pipeline.project,
+        ref: pipeline.ref,
+        created_at: '2020-02-06 00:02:20'
+      )
+    end
+    let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: '4/4 rspec', coverage: 84) }
+    let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: '3/3 karma', coverage: 92) }
 
+    before do
+      # Create the existing daily code coverage records
       described_class.new.execute(pipeline)
     end
+
+    it "updates the existing record's coverage value and last_pipeline_id" do
+      rspec_coverage = Ci::DailyReportResult.find_by(title: 'rspec')
+      karma_coverage = Ci::DailyReportResult.find_by(title: 'karma')
+
+      # Bump up the coverage values
+      described_class.new.execute(new_pipeline)
+
+      rspec_coverage.reload
+      karma_coverage.reload
+
+      expect(rspec_coverage).to have_attributes(
+        last_pipeline_id: new_pipeline.id,
+        value: new_rspec_job.coverage
+      )
+
+      expect(karma_coverage).to have_attributes(
+        last_pipeline_id: new_pipeline.id,
+        value: new_karma_job.coverage
+      )
+    end
+  end
+
+  context 'when the ID of the pipeline is older than the last_pipeline_id' do
+    let!(:new_pipeline) do
+      create(
+        :ci_pipeline,
+        project: pipeline.project,
+        ref: pipeline.ref,
+        created_at: '2020-02-06 00:02:20'
+      )
+    end
+    let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: '4/4 rspec', coverage: 84) }
+    let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: '3/3 karma', coverage: 92) }
+
+    before do
+      # Create the existing daily code coverage records
+      # but in this case, for the newer pipeline first.
+      described_class.new.execute(new_pipeline)
+    end
+
+    it 'updates the existing daily code coverage records' do
+      rspec_coverage = Ci::DailyReportResult.find_by(title: 'rspec')
+      karma_coverage = Ci::DailyReportResult.find_by(title: 'karma')
+
+      # Run another one but for the older pipeline.
+      # This simulates the scenario wherein the success worker
+      # of an older pipeline, for some network hiccup, was delayed
+      # and only got executed right after the newer pipeline's success worker.
+      # Ideally, we don't want to bump the coverage value with an older one
+      # but given this can be a rare edge case and can be remedied by re-running
+      # the pipeline we'll just let it be for now. In return, we are able to use
+      # Rails 6 shiny new method, upsert_all, and simplify the code a lot.
+      described_class.new.execute(pipeline)
+
+      rspec_coverage.reload
+      karma_coverage.reload
+
+      expect(rspec_coverage).to have_attributes(
+        last_pipeline_id: pipeline.id,
+        value: rspec_job.coverage
+      )
+
+      expect(karma_coverage).to have_attributes(
+        last_pipeline_id: pipeline.id,
+        value: karma_job.coverage
+      )
+    end
+  end
+
+  context 'when pipeline has no builds with coverage' do
+    let!(:new_pipeline) do
+      create(
+        :ci_pipeline,
+        created_at: '2020-02-06 00:02:20'
+      )
+    end
+    let!(:some_job) { create(:ci_build, pipeline: new_pipeline, name: 'foo') }
+
+    it 'does nothing' do
+      expect { described_class.new.execute(new_pipeline) }.not_to raise_error
+    end
+  end
 end