Merge branch 'refactor-update-statistics-concern' into 'master'

Cleans up UpdateProjectStatistics concern

See merge request gitlab-org/gitlab-ce!28999

app/models/ci/build.rb

@@ -15,7 +15,6 @@ module Ci
     include Gitlab::Utils::StrongMemoize
     include Deployable
     include HasRef
-    include UpdateProjectStatistics
 
     BuildArchivedError = Class.new(StandardError)
 

app/models/ci/job_artifact.rb

@@ -59,7 +59,7 @@ module Ci
     validate :valid_file_format?, unless: :trace?, on: :create
     before_save :set_size, if: :file_changed?
 
-    update_project_statistics stat: :build_artifacts_size
+    update_project_statistics project_statistics_name: :build_artifacts_size
 
     after_save :update_file_store, if: :saved_change_to_file?
 

app/models/concerns/update_project_statistics.rb

@@ -5,43 +5,47 @@
 # It deals with `ProjectStatistics.increment_statistic` making sure not to update statistics on a cascade delete from the
 # project, and keeping track of value deltas on each save. It updates the DB only when a change is needed.
 #
-# How to use
-# - Invoke `update_project_statistics stat: :a_project_statistics_column, attribute: :an_attr_to_track` in a model class body.
+# Example:
 #
-# Expectation
-# - `attribute` must be an ActiveRecord attribute
+# module Ci
+#   class JobArtifact < ApplicationRecord
+#     include UpdateProjectStatistics
+#
+#     update_project_statistics project_statistics_name: :build_artifacts_size
+#   end
+# end
+#
+# Expectation:
+#
+# - `statistic_attribute` must be an ActiveRecord attribute
 # - The model must implement `project` and `project_id`. i.e. direct Project relationship or delegation
+#
 module UpdateProjectStatistics
   extend ActiveSupport::Concern
 
   class_methods do
-    attr_reader :statistic_name, :statistic_attribute
+    attr_reader :project_statistics_name, :statistic_attribute
 
-    # Configure the model to update +stat+ on ProjectStatistics when +attribute+ changes
+    # Configure the model to update `project_statistics_name` on ProjectStatistics,
+    # when `statistic_attribute` changes
+    #
+    # - project_statistics_name: A column of `ProjectStatistics` to update
+    # - statistic_attribute: An attribute of the current model, default to `size`
     #
-    # +stat+:: a column of ProjectStatistics to update
-    # +attribute+:: an attribute of the current model, default to +:size+
-    def update_project_statistics(stat:, attribute: :size)
-      @statistic_name = stat
-      @statistic_attribute = attribute
+    def update_project_statistics(project_statistics_name:, statistic_attribute: :size)
+      @project_statistics_name = project_statistics_name
+      @statistic_attribute = statistic_attribute
 
       after_save(:update_project_statistics_after_save, if: :update_project_statistics_attribute_changed?)
       after_destroy(:update_project_statistics_after_destroy, unless: :project_destroyed?)
     end
+
     private :update_project_statistics
   end
 
   included do
     private
 
-    def project_destroyed?
-      project.pending_delete?
-    end
-
-    def update_project_statistics_attribute_changed?
-      saved_change_to_attribute?(self.class.statistic_attribute)
-    end
-
     def update_project_statistics_after_save
       attr = self.class.statistic_attribute
       delta = read_attribute(attr).to_i - attribute_before_last_save(attr).to_i
@@ -49,12 +53,20 @@ module UpdateProjectStatistics
       update_project_statistics(delta)
     end
 
+    def update_project_statistics_attribute_changed?
+      saved_change_to_attribute?(self.class.statistic_attribute)
+    end
+
     def update_project_statistics_after_destroy
       update_project_statistics(-read_attribute(self.class.statistic_attribute).to_i)
     end
 
+    def project_destroyed?
+      project.pending_delete?
+    end
+
     def update_project_statistics(delta)
-      ProjectStatistics.increment_statistic(project_id, self.class.statistic_name, delta)
+      ProjectStatistics.increment_statistic(project_id, self.class.project_statistics_name, delta)
     end
   end
 end

spec/models/ci/job_artifact_spec.rb

@@ -5,10 +5,6 @@ require 'spec_helper'
 describe Ci::JobArtifact do
   let(:artifact) { create(:ci_job_artifact, :archive) }
 
-  it_behaves_like 'UpdateProjectStatistics' do
-    subject { build(:ci_job_artifact, :archive, size: 106365) }
-  end
-
   describe "Associations" do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:job) }
@@ -23,6 +19,10 @@ describe Ci::JobArtifact do
 
   it_behaves_like 'having unique enum values'
 
+  it_behaves_like 'UpdateProjectStatistics' do
+    subject { build(:ci_job_artifact, :archive, size: 106365) }
+  end
+
   describe '.with_reports' do
     let!(:artifact) { create(:ci_job_artifact, :archive) }
 

spec/support/shared_examples/models/update_project_statistics_shared_examples.rb

@@ -3,16 +3,16 @@
 require 'spec_helper'
 
 shared_examples_for 'UpdateProjectStatistics' do
-  let(:project)   { subject.project }
-  let(:stat)      { described_class.statistic_name }
-  let(:attribute) { described_class.statistic_attribute }
+  let(:project) { subject.project }
+  let(:project_statistics_name) { described_class.project_statistics_name }
+  let(:statistic_attribute) { described_class.statistic_attribute }
 
   def reload_stat
-    project.statistics.reload.send(stat).to_i
+    project.statistics.reload.send(project_statistics_name).to_i
   end
 
   def read_attribute
-    subject.read_attribute(attribute).to_i
+    subject.read_attribute(statistic_attribute).to_i
   end
 
   it { is_expected.to be_new_record }
@@ -39,7 +39,8 @@ shared_examples_for 'UpdateProjectStatistics' do
         .to receive(:increment_statistic)
         .and_call_original
 
-      subject.write_attribute(attribute, read_attribute + delta)
+      subject.write_attribute(statistic_attribute, read_attribute + delta)
+
       expect { subject.save! }
         .to change { reload_stat }
         .by(delta)