Merge branch 'osw-introduce-merge-request-statistics' into 'master'

Improve closed/merged events queries performance on Projects::MergeRequestsController#show.json

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

22 files changed, 423 insertions, 43 deletions

diff --git a/spec/fixtures/api/schemas/entities/merge_request_metrics.json b/spec/fixtures/api/schemas/entities/merge_request_metrics.json
new file mode 100644
index 00000000000..3fa767f85df
--- /dev/null
+++ b/spec/fixtures/api/schemas/entities/merge_request_metrics.json
@@ -0,0 +1,21 @@
+{
+  "type": "object",
+  "required": ["closed_at", "merged_at", "closed_by", "merged_by"],
+  "properties" : {
+    "closed_at": { "type": ["datetime", "null"] },
+    "merged_at": { "type": ["datetime", "null"] },
+    "closed_by": {
+      "oneOf": [
+        { "type": "null" },
+        { "$ref": "user.json" }
+      ]
+    },
+    "merged_by": {
+      "oneOf": [
+        { "type": "null" },
+        { "$ref": "user.json" }
+      ]
+    }
+  },
+  "additionalProperties": false
+}
diff --git a/spec/fixtures/api/schemas/entities/merge_request_widget.json b/spec/fixtures/api/schemas/entities/merge_request_widget.json
index 342890c3dee..9de27bee751 100644
--- a/spec/fixtures/api/schemas/entities/merge_request_widget.json
+++ b/spec/fixtures/api/schemas/entities/merge_request_widget.json
@@ -31,8 +31,12 @@
     "source_project_id": { "type": "integer" },
     "target_branch": { "type": "string" },
     "target_project_id": { "type": "integer" },
-    "merge_event": { "type": ["object", "null"] },
-    "closed_event": { "type": ["object", "null"] },
+    "metrics": {
+      "oneOf": [
+        { "type": "null" },
+        { "$ref": "merge_request_metrics.json" }
+      ]
+    },
     "author": { "type": ["object", "null"] },
     "merge_user": { "type": ["object", "null"] },
     "diff_head_sha": { "type": ["string", "null"] },
diff --git a/spec/fixtures/api/schemas/entities/user.json b/spec/fixtures/api/schemas/entities/user.json
new file mode 100644
index 00000000000..6482e0eedd2
--- /dev/null
+++ b/spec/fixtures/api/schemas/entities/user.json
@@ -0,0 +1,17 @@
+{
+  "type": "object",
+  "required": [
+    "id",
+    "state",
+    "avatar_url",
+    "web_url",
+    "path"
+  ],
+  "properties": {
+    "id": { "type": "integer" },
+    "state": { "type": "string" },
+    "avatar_url": { "type": "string" },
+    "web_url": { "type": "string" },
+    "path": { "type": "string" }
+  }
+}
diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_closed_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_closed_spec.js
index d23b558f4ea..1bf97bbf093 100644
--- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_closed_spec.js
+++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_closed_spec.js
@@ -4,13 +4,16 @@ import closedComponent from '~/vue_merge_request_widget/components/states/mr_wid
 const mr = {
   targetBranch: 'good-branch',
   targetBranchPath: '/good-branch',
-  closedEvent: {
-    author: {
+  metrics: {
+    mergedBy: {},
+    mergedAt: 'mergedUpdatedAt',
+    closedBy: {
       name: 'Fatih Acet',
       username: 'fatihacet',
     },
-    updatedAt: 'closedEventUpdatedAt',
-    formattedUpdatedAt: '',
+    closedAt: 'closedEventUpdatedAt',
+    readableMergedAt: '',
+    readableClosedAt: '',
   },
   updatedAt: 'mrUpdatedAt',
   closedAt: '1 day ago',
@@ -56,7 +59,7 @@ describe('MRWidgetClosed', () => {
 
     it('should have correct elements', () => {
       expect(el.querySelector('h4').textContent).toContain('Closed by');
-      expect(el.querySelector('h4').textContent).toContain(mr.closedEvent.author.name);
+      expect(el.querySelector('h4').textContent).toContain(mr.metrics.closedBy.name);
       expect(el.textContent).toContain('The changes were not merged into');
       expect(el.querySelector('.label-branch').getAttribute('href')).toEqual(mr.targetBranchPath);
       expect(el.querySelector('.label-branch').textContent).toContain(mr.targetBranch);
diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_merged_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_merged_spec.js
index 9d3ae4b448f..2dc3b72ea40 100644
--- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_merged_spec.js
+++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_merged_spec.js
@@ -14,10 +14,13 @@ const createComponent = () => {
     canRevertInCurrentMR: true,
     canRemoveSourceBranch: true,
     sourceBranchRemoved: true,
-    mergedEvent: {
-      author: {},
-      updatedAt: 'mergedUpdatedAt',
-      formattedUpdatedAt: '',
+    metrics: {
+      mergedBy: {},
+      mergedAt: 'mergedUpdatedAt',
+      readableMergedAt: '',
+      closedBy: {},
+      closedAt: 'mergedUpdatedAt',
+      readableClosedAt: '',
     },
     updatedAt: 'mrUpdatedAt',
     targetBranch,
diff --git a/spec/javascripts/vue_mr_widget/mock_data.js b/spec/javascripts/vue_mr_widget/mock_data.js
index 1ad7c2d8efa..ca29c9fee32 100644
--- a/spec/javascripts/vue_mr_widget/mock_data.js
+++ b/spec/javascripts/vue_mr_widget/mock_data.js
@@ -33,8 +33,8 @@ export default {
   "source_project_id": 19,
   "target_branch": "master",
   "target_project_id": 19,
-  "merge_event": {
-    "author": {
+  "metrics": {
+    "merged_by": {
       "name": "Administrator",
       "username": "root",
       "id": 1,
@@ -42,9 +42,10 @@ export default {
       "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon",
       "web_url": "http://localhost:3000/root"
     },
-    "updated_at": "2017-04-07T15:39:25.696Z"
+    "merged_at": "2017-04-07T15:39:25.696Z",
+    "closed_by": null,
+    "closed_at": null
   },
-  "closed_event": null,
   "author": {
     "name": "Administrator",
     "username": "root",
diff --git a/spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb b/spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb
new file mode 100644
index 00000000000..dfe3b31f1c0
--- /dev/null
+++ b/spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb
@@ -0,0 +1,124 @@
+require 'rails_helper'
+
+describe Gitlab::BackgroundMigration::PopulateMergeRequestMetricsWithEventsData, :migration, schema: 20171128214150 do
+  describe '#perform' 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
+
+    it 'inserts metrics and updates closed and merged events' do
+      subject.perform(mr_with_event.id, mr_with_event.id)
+
+      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
+
+  describe '#insert_metrics_for_range' do
+    let!(:mrs_without_metrics) { create_list(:merge_request, 3) }
+    let!(:mrs_with_metrics) { create_list(:merge_request, 2) }
+
+    before do
+      # Make sure no metrics are created and kept through after_* callbacks.
+      mrs_without_metrics.each { |m| m.metrics.destroy! }
+    end
+
+    it 'inserts merge_request_metrics for merge_requests without one' do
+      expect { subject.insert_metrics_for_range(MergeRequest.first.id, MergeRequest.last.id) }
+        .to change(MergeRequest::Metrics, :count).from(2).to(5)
+
+      mrs_without_metrics.each do |mr_without_metrics|
+        expect(mr_without_metrics.reload.metrics).to be_present
+      end
+    end
+
+    it 'does not inserts merge_request_metrics for MRs out of given range' do
+      expect { subject.insert_metrics_for_range(mrs_with_metrics.first.id, mrs_with_metrics.last.id) }
+        .not_to change(MergeRequest::Metrics, :count).from(2)
+    end
+  end
+
+  describe '#update_metrics_with_events_data' do
+    context 'closed events data update' do
+      let(:users) { create_list(:user, 3) }
+      let(:mrs_with_event) { create_list(:merge_request, 3) }
+
+      before do
+        create_list(:event, 2, :closed, author: users.first, target: mrs_with_event.first)
+        create_list(:event, 3, :closed, author: users.second, target: mrs_with_event.second)
+        create(:event, :closed, author: users.third, target: mrs_with_event.third)
+      end
+
+      it 'migrates multiple MR metrics with closed event data' do
+        mr_without_event = create(:merge_request)
+        create(:event, :merged)
+
+        subject.update_metrics_with_events_data(mrs_with_event.first.id, mrs_with_event.last.id)
+
+        mrs_with_event.each do |mr_with_event|
+          latest_event = Event.where(action: 3, target: mr_with_event).last
+
+          mr_with_event.metrics.reload
+
+          expect(mr_with_event.metrics.latest_closed_by).to eq(latest_event.author)
+          expect(mr_with_event.metrics.latest_closed_at.to_s).to eq(latest_event.updated_at.to_s)
+        end
+
+        expect(mr_without_event.metrics.reload).to have_attributes(latest_closed_by_id: nil,
+                                                                   latest_closed_at: nil)
+      end
+
+      it 'does not updates metrics out of given range' do
+        out_of_range_mr = create(:merge_request)
+        create(:event, :closed, author: users.last, target: out_of_range_mr)
+
+        expect { subject.perform(mrs_with_event.first.id, mrs_with_event.second.id) }
+          .not_to change { out_of_range_mr.metrics.reload.merged_by }
+          .from(nil)
+      end
+    end
+
+    context 'merged events data update' do
+      let(:users) { create_list(:user, 3) }
+      let(:mrs_with_event) { create_list(:merge_request, 3) }
+
+      before do
+        create_list(:event, 2, :merged, author: users.first, target: mrs_with_event.first)
+        create_list(:event, 3, :merged, author: users.second, target: mrs_with_event.second)
+        create(:event, :merged, author: users.third, target: mrs_with_event.third)
+      end
+
+      it 'migrates multiple MR metrics with merged event data' do
+        mr_without_event = create(:merge_request)
+        create(:event, :merged)
+
+        subject.update_metrics_with_events_data(mrs_with_event.first.id, mrs_with_event.last.id)
+
+        mrs_with_event.each do |mr_with_event|
+          latest_event = Event.where(action: Event::MERGED, target: mr_with_event).last
+
+          expect(mr_with_event.metrics.reload.merged_by).to eq(latest_event.author)
+        end
+
+        expect(mr_without_event.metrics.reload).to have_attributes(merged_by_id: nil)
+      end
+
+      it 'does not updates metrics out of given range' do
+        out_of_range_mr = create(:merge_request)
+        create(:event, :merged, author: users.last, target: out_of_range_mr)
+
+        expect { subject.perform(mrs_with_event.first.id, mrs_with_event.second.id) }
+          .not_to change { out_of_range_mr.metrics.reload.merged_by }
+          .from(nil)
+      end
+    end
+  end
+end
diff --git a/spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb b/spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb
new file mode 100644
index 00000000000..97e089c5cb8
--- /dev/null
+++ b/spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb
@@ -0,0 +1,24 @@
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20171128214150_schedule_populate_merge_request_metrics_with_events_data.rb')
+
+describe SchedulePopulateMergeRequestMetricsWithEventsData, :migration, :sidekiq do
+  let!(:mrs) { create_list(:merge_request, 3) }
+
+  it 'correctly schedules background migrations' do
+    stub_const("#{described_class.name}::BATCH_SIZE", 2)
+
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(described_class::MIGRATION)
+          .to be_scheduled_migration(10.minutes, mrs.first.id, mrs.second.id)
+
+        expect(described_class::MIGRATION)
+          .to be_scheduled_migration(20.minutes, mrs.third.id, mrs.third.id)
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+      end
+    end
+  end
+end
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index c9deeb45c1a..5ced000cdb6 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -23,6 +23,32 @@ describe Issue do
     it { is_expected.to have_db_index(:deleted_at) }
   end
 
+  describe 'callbacks' do
+    describe '#ensure_metrics' do
+      it 'creates metrics after saving' do
+        issue = create(:issue)
+
+        expect(issue.metrics).to be_persisted
+        expect(Issue::Metrics.count).to eq(1)
+      end
+
+      it 'does not create duplicate metrics for an issue' do
+        issue = create(:issue)
+
+        issue.close!
+
+        expect(issue.metrics).to be_persisted
+        expect(Issue::Metrics.count).to eq(1)
+      end
+
+      it 'records current metrics' do
+        expect_any_instance_of(Issue::Metrics).to receive(:record!)
+
+        create(:issue)
+      end
+    end
+  end
+
   describe '#order_by_position_and_priority' do
     let(:project) { create :project }
     let(:p1) { create(:label, title: 'P1', project: project, priority: 1) }
diff --git a/spec/models/merge_request/metrics_spec.rb b/spec/models/merge_request/metrics_spec.rb
index 9353d5c3c8a..02ff7839739 100644
--- a/spec/models/merge_request/metrics_spec.rb
+++ b/spec/models/merge_request/metrics_spec.rb
@@ -1,16 +1,11 @@
 require 'spec_helper'
 
 describe MergeRequest::Metrics do
-  subject { create(:merge_request) }
+  subject { described_class.new }
 
-  describe "when recording the default set of metrics on merge request save" do
-    it "records the merge time" do
-      time = Time.now
-      Timecop.freeze(time) { subject.mark_as_merged }
-      metrics = subject.metrics
-
-      expect(metrics).to be_present
-      expect(metrics.merged_at).to be_like_time(time)
-    end
+  describe 'associations' do
+    it { is_expected.to belong_to(:merge_request) }
+    it { is_expected.to belong_to(:latest_closed_by).class_name('User') }
+    it { is_expected.to belong_to(:merged_by).class_name('User') }
   end
 end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index df94617a19e..d8ebd46faab 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -65,6 +65,25 @@ describe MergeRequest do
     end
   end
 
+  describe 'callbacks' do
+    describe '#ensure_merge_request_metrics' do
+      it 'creates metrics after saving' do
+        merge_request = create(:merge_request)
+
+        expect(merge_request.metrics).to be_persisted
+        expect(MergeRequest::Metrics.count).to eq(1)
+      end
+
+      it 'does not duplicate metrics for a merge request' do
+        merge_request = create(:merge_request)
+
+        merge_request.mark_as_merged!
+
+        expect(MergeRequest::Metrics.count).to eq(1)
+      end
+    end
+  end
+
   describe 'respond to' do
     it { is_expected.to respond_to(:unchecked?) }
     it { is_expected.to respond_to(:can_be_merged?) }
diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb
index be8d9c19125..981c9c27325 100644
--- a/spec/requests/api/notes_spec.rb
+++ b/spec/requests/api/notes_spec.rb
@@ -464,7 +464,7 @@ describe API::Notes do
 
   describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do
     it "creates an activity event when an issue note is created" do
-      expect(Event).to receive(:create)
+      expect(Event).to receive(:create!)
 
       post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user), body: 'hi!'
     end
diff --git a/spec/requests/api/project_milestones_spec.rb b/spec/requests/api/project_milestones_spec.rb
index 6fe8ab5a3f6..08ea7314bb3 100644
--- a/spec/requests/api/project_milestones_spec.rb
+++ b/spec/requests/api/project_milestones_spec.rb
@@ -16,7 +16,7 @@ describe API::ProjectMilestones do
 
   describe 'PUT /projects/:id/milestones/:milestone_id to test observer on close' do
     it 'creates an activity event when an milestone is closed' do
-      expect(Event).to receive(:create)
+      expect(Event).to receive(:create!)
 
       put api("/projects/#{project.id}/milestones/#{milestone.id}", user),
           state_event: 'close'
diff --git a/spec/requests/api/v3/milestones_spec.rb b/spec/requests/api/v3/milestones_spec.rb
index 9ee71ea9c5d..6021600e09c 100644
--- a/spec/requests/api/v3/milestones_spec.rb
+++ b/spec/requests/api/v3/milestones_spec.rb
@@ -161,7 +161,7 @@ describe API::V3::Milestones do
 
   describe 'PUT /projects/:id/milestones/:milestone_id to test observer on close' do
     it 'creates an activity event when an milestone is closed' do
-      expect(Event).to receive(:create)
+      expect(Event).to receive(:create!)
 
       put v3_api("/projects/#{project.id}/milestones/#{milestone.id}", user),
           state_event: 'close'
diff --git a/spec/requests/api/v3/notes_spec.rb b/spec/requests/api/v3/notes_spec.rb
index 6428d9daaba..5532795ab02 100644
--- a/spec/requests/api/v3/notes_spec.rb
+++ b/spec/requests/api/v3/notes_spec.rb
@@ -302,7 +302,7 @@ describe API::V3::Notes do
 
   describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do
     it "creates an activity event when an issue note is created" do
-      expect(Event).to receive(:create)
+      expect(Event).to receive(:create!)
 
       post v3_api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!'
     end
diff --git a/spec/serializers/event_entity_spec.rb b/spec/serializers/event_entity_spec.rb
deleted file mode 100644
index bb54597c967..00000000000
--- a/spec/serializers/event_entity_spec.rb
+++ /dev/null
@@ -1,13 +0,0 @@
-require 'spec_helper'
-
-describe EventEntity do
-  subject { described_class.represent(create(:event)).as_json }
-
-  it 'exposes author' do
-    expect(subject).to include(:author)
-  end
-
-  it 'exposes core elements of event' do
-    expect(subject).to include(:updated_at)
-  end
-end
diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb
index a5924a8589c..e25552eb0d8 100644
--- a/spec/serializers/merge_request_widget_entity_spec.rb
+++ b/spec/serializers/merge_request_widget_entity_spec.rb
@@ -35,6 +35,81 @@ describe MergeRequestWidgetEntity do
     end
   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
+
+      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
+      end
+    end
+  end
+
   it 'has email_patches_path' do
     expect(subject[:email_patches_path])
       .to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}.patch")
diff --git a/spec/services/create_deployment_service_spec.rb b/spec/services/create_deployment_service_spec.rb
index 08267d6e6a0..b9bfbb11511 100644
--- a/spec/services/create_deployment_service_spec.rb
+++ b/spec/services/create_deployment_service_spec.rb
@@ -266,7 +266,7 @@ describe CreateDeploymentService do
 
     context "while updating the 'first_deployed_to_production_at' time" do
       before do
-        merge_request.mark_as_merged
+        merge_request.metrics.update!(merged_at: Time.now)
       end
 
       context "for merge requests merged before the current deploy" do
diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb
index 2a59bc4594a..4d12de3ecce 100644
--- a/spec/services/merge_requests/close_service_spec.rb
+++ b/spec/services/merge_requests/close_service_spec.rb
@@ -52,6 +52,19 @@ describe MergeRequests::CloseService do
       end
     end
 
+    it 'updates metrics' do
+      metrics = merge_request.metrics
+      metrics_service = double(MergeRequestMetricsService)
+      allow(MergeRequestMetricsService)
+        .to receive(:new)
+        .with(metrics)
+        .and_return(metrics_service)
+
+      expect(metrics_service).to receive(:close)
+
+      described_class.new(project, user, {}).execute(merge_request)
+    end
+
     it 'refreshes the number of open merge requests for a valid MR', :use_clean_rails_memory_store_caching do
       service = described_class.new(project, user, {})
 
diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb
index 8f2c5df5907..70957431942 100644
--- a/spec/services/merge_requests/post_merge_service_spec.rb
+++ b/spec/services/merge_requests/post_merge_service_spec.rb
@@ -22,5 +22,18 @@ describe MergeRequests::PostMergeService do
       expect { service.execute(merge_request) }
         .to change { project.open_merge_requests_count }.from(1).to(0)
     end
+
+    it 'updates metrics' do
+      metrics = merge_request.metrics
+      metrics_service = double(MergeRequestMetricsService)
+      allow(MergeRequestMetricsService)
+        .to receive(:new)
+        .with(metrics)
+        .and_return(metrics_service)
+
+      expect(metrics_service).to receive(:merge)
+
+      described_class.new(project, user, {}).execute(merge_request)
+    end
   end
 end
diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb
index 94f31ff139c..a44d63e5f9f 100644
--- a/spec/services/merge_requests/reopen_service_spec.rb
+++ b/spec/services/merge_requests/reopen_service_spec.rb
@@ -47,6 +47,19 @@ describe MergeRequests::ReopenService do
       end
     end
 
+    it 'updates metrics' do
+      metrics = merge_request.metrics
+      service = double(MergeRequestMetricsService)
+      allow(MergeRequestMetricsService)
+        .to receive(:new)
+        .with(metrics)
+        .and_return(service)
+
+      expect(service).to receive(:reopen)
+
+      described_class.new(project, user, {}).execute(merge_request)
+    end
+
     it 'refreshes the number of open merge requests for a valid MR' do
       service = described_class.new(project, user, {})
 
diff --git a/spec/services/update_merge_request_metrics_service_spec.rb b/spec/services/update_merge_request_metrics_service_spec.rb
new file mode 100644
index 00000000000..b5fb999381d
--- /dev/null
+++ b/spec/services/update_merge_request_metrics_service_spec.rb
@@ -0,0 +1,42 @@
+require 'rails_helper'
+
+describe MergeRequestMetricsService do
+  let(:metrics) { create(:merge_request).metrics }
+
+  describe '#merge' do
+    it 'updates metrics' do
+      user = create(:user)
+      service = described_class.new(metrics)
+      event = double(Event, author_id: user.id, created_at: Time.now)
+
+      service.merge(event)
+
+      expect(metrics.merged_by).to eq(user)
+      expect(metrics.merged_at).to eq(event.created_at)
+    end
+  end
+
+  describe '#close' do
+    it 'updates metrics' do
+      user = create(:user)
+      service = described_class.new(metrics)
+      event = double(Event, author_id: user.id, created_at: Time.now)
+
+      service.close(event)
+
+      expect(metrics.latest_closed_by).to eq(user)
+      expect(metrics.latest_closed_at).to eq(event.created_at)
+    end
+  end
+
+  describe '#reopen' do
+    it 'updates metrics' do
+      service = described_class.new(metrics)
+
+      service.reopen
+
+      expect(metrics.latest_closed_by).to be_nil
+      expect(metrics.latest_closed_at).to be_nil
+    end
+  end
+end