Clean-up phase for MR metrics population background migration41587-osw-mr-metrics-migration-cleanup

This finishes any old running background migration which populates MR metrics
with events data. Which solves
https://gitlab.com/gitlab-org/gitlab-ce/issues/41587.

6 files changed, 68 insertions, 87 deletions

diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb
index d0165c148eb..6aa99850e59 100644
--- a/app/serializers/merge_request_widget_entity.rb
+++ b/app/serializers/merge_request_widget_entity.rb
@@ -19,11 +19,7 @@ class MergeRequestWidgetEntity < IssuableEntity
     merge_request.project.merge_requests_ff_only_enabled
   end
 
-  expose :metrics do |merge_request|
-    metrics = build_metrics(merge_request)
-
-    MergeRequestMetricsEntity.new(metrics).as_json
-  end
+  expose :metrics, with: MergeRequestMetricsEntity
 
   expose :rebase_commit_sha
   expose :rebase_in_progress?, as: :rebase_in_progress
@@ -222,27 +218,4 @@ class MergeRequestWidgetEntity < IssuableEntity
     @presenters ||= {}
     @presenters[merge_request] ||= MergeRequestPresenter.new(merge_request, current_user: current_user)
   end
-
-  # Once SchedulePopulateMergeRequestMetricsWithEventsData fully runs,
-  # we can remove this method and just serialize MergeRequest#metrics
-  # instead. See https://gitlab.com/gitlab-org/gitlab-ce/issues/41587
-  def build_metrics(merge_request)
-    # There's no need to query and serialize metrics data for merge requests that are not
-    # merged or closed.
-    return unless merge_request.merged? || merge_request.closed?
-    return merge_request.metrics if merge_request.merged? && merge_request.metrics&.merged_by_id
-    return merge_request.metrics if merge_request.closed? && merge_request.metrics&.latest_closed_by_id
-
-    build_metrics_from_events(merge_request)
-  end
-
-  def build_metrics_from_events(merge_request)
-    closed_event = merge_request.closed_event
-    merge_event = merge_request.merge_event
-
-    MergeRequest::Metrics.new(latest_closed_at: closed_event&.updated_at,
-                              latest_closed_by: closed_event&.author,
-                              merged_at: merge_event&.updated_at,
-                              merged_by: merge_event&.author)
-  end
 end
diff --git a/changelogs/unreleased/41587-osw-mr-metrics-migration-cleanup.yml b/changelogs/unreleased/41587-osw-mr-metrics-migration-cleanup.yml
new file mode 100644
index 00000000000..f8640e46e78
--- /dev/null
+++ b/changelogs/unreleased/41587-osw-mr-metrics-migration-cleanup.yml
@@ -0,0 +1,5 @@
+---
+title: Clean-up phase for MR metrics population background migration
+merge_request:
+author:
+type: other
diff --git a/db/migrate/20180521162137_migrate_remaining_mr_metrics_populating_background_migration.rb b/db/migrate/20180521162137_migrate_remaining_mr_metrics_populating_background_migration.rb
new file mode 100644
index 00000000000..aa617d8425c
--- /dev/null
+++ b/db/migrate/20180521162137_migrate_remaining_mr_metrics_populating_background_migration.rb
@@ -0,0 +1,33 @@
+class MigrateRemainingMrMetricsPopulatingBackgroundMigration < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  BATCH_SIZE = 10_000
+
+  class MergeRequest < ActiveRecord::Base
+    self.table_name = 'merge_requests'
+
+    include ::EachBatch
+  end
+
+  def up
+    Gitlab::BackgroundMigration.steal('PopulateMergeRequestMetricsWithEventsData')
+
+    metrics_not_exists_clause =
+      <<-SQL.strip_heredoc
+        NOT EXISTS (SELECT 1 FROM merge_request_metrics
+                    WHERE merge_request_metrics.merge_request_id = merge_requests.id)
+    SQL
+
+    MergeRequest.where(metrics_not_exists_clause).each_batch(of: BATCH_SIZE) do |relation, index|
+      range = relation.pluck('MIN(id)', 'MAX(id)').first
+
+      Gitlab::BackgroundMigration::PopulateMergeRequestMetricsWithEventsData
+        .new
+        .perform(*range)
+    end
+  end
+
+  def down
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index ed29d202f91..b39389bc8b3 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180512061621) do
+ActiveRecord::Schema.define(version: 20180521162137) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
diff --git a/spec/migrations/migrate_remaining_mr_metrics_populating_background_migration_spec.rb b/spec/migrations/migrate_remaining_mr_metrics_populating_background_migration_spec.rb
new file mode 100644
index 00000000000..b948be35927
--- /dev/null
+++ b/spec/migrations/migrate_remaining_mr_metrics_populating_background_migration_spec.rb
@@ -0,0 +1,25 @@
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20180521162137_migrate_remaining_mr_metrics_populating_background_migration.rb')
+
+describe MigrateRemainingMrMetricsPopulatingBackgroundMigration, :migration, :sidekiq, :redis do
+  let(:mr_with_event) { create(:merge_request) }
+  let!(:merged_event) { create(:event, :merged, target: mr_with_event) }
+  let!(:closed_event) { create(:event, :closed, target: mr_with_event) }
+
+  before do
+    # Make sure no metrics are created and kept through after_* callbacks.
+    mr_with_event.metrics.destroy!
+  end
+
+  # This is mainly an integration test. PopulateMergeRequestMetricsWithEventsData worker
+  # already has exclusive unit tests.
+  it 'migrates remaining MRs without metrics' do
+    migrate!
+
+    mr_with_event.reload
+
+    expect(mr_with_event.metrics).to have_attributes(latest_closed_by_id: closed_event.author_id,
+                                                     merged_by_id: merged_event.author_id)
+    expect(mr_with_event.metrics.latest_closed_at.to_s).to eq(closed_event.updated_at.to_s)
+  end
+end
diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb
index d2072198d83..b591cf6dd4b 100644
--- a/spec/serializers/merge_request_widget_entity_spec.rb
+++ b/spec/serializers/merge_request_widget_entity_spec.rb
@@ -36,76 +36,21 @@ describe MergeRequestWidgetEntity do
   end
 
   describe 'metrics' do
-    context 'when metrics record exists with merged data' do
-      before do
-        resource.mark_as_merged!
-        resource.metrics.update!(merged_by: user)
-      end
-
-      it 'matches merge request metrics schema' do
-        expect(subject[:metrics].with_indifferent_access)
-          .to match_schema('entities/merge_request_metrics')
-      end
-
-      it 'returns values from metrics record' do
-        expect(subject.dig(:metrics, :merged_by, :id))
-          .to eq(resource.metrics.merged_by_id)
-      end
-    end
-
-    context 'when metrics record exists with closed data' do
-      before do
-        resource.close!
-        resource.metrics.update!(latest_closed_by: user)
-      end
-
+    context 'when metrics record exists' do
       it 'matches merge request metrics schema' do
         expect(subject[:metrics].with_indifferent_access)
           .to match_schema('entities/merge_request_metrics')
       end
-
-      it 'returns values from metrics record' do
-        expect(subject.dig(:metrics, :closed_by, :id))
-          .to eq(resource.metrics.latest_closed_by_id)
-      end
     end
 
     context 'when metrics does not exists' do
       before do
-        resource.mark_as_merged!
         resource.metrics.destroy!
         resource.reload
       end
 
-      context 'when events exists' do
-        let!(:closed_event) { create(:event, :closed, project: project, target: resource) }
-        let!(:merge_event) { create(:event, :merged, project: project, target: resource) }
-
-        it 'matches merge request metrics schema' do
-          expect(subject[:metrics].with_indifferent_access)
-            .to match_schema('entities/merge_request_metrics')
-        end
-
-        it 'returns values from events record' do
-          expect(subject.dig(:metrics, :merged_by, :id))
-            .to eq(merge_event.author_id)
-
-          expect(subject.dig(:metrics, :closed_by, :id))
-            .to eq(closed_event.author_id)
-
-          expect(subject.dig(:metrics, :merged_at).to_s)
-            .to eq(merge_event.updated_at.to_s)
-
-          expect(subject.dig(:metrics, :closed_at).to_s)
-            .to eq(closed_event.updated_at.to_s)
-        end
-      end
-
-      context 'when events does not exists' do
-        it 'matches merge request metrics schema' do
-          expect(subject[:metrics].with_indifferent_access)
-            .to match_schema('entities/merge_request_metrics')
-        end
+      it 'returns nil' do
+        expect(subject[:metrics]).to be_nil
       end
     end
   end