Use MR metrics cached data whenever available (wraps up metrics migration)osw-wrap-up-metrics-migration

7 files changed, 44 insertions, 139 deletions

diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.vue
index 25a57e520ee..53a61372ff3 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.vue
@@ -27,9 +27,9 @@
     <div class="media-body">
       <mr-widget-author-time
         :action-text="s__('mrWidget|Closed by')"
-        :author="mr.metrics.closedBy"
-        :date-title="mr.metrics.closedAt"
-        :date-readable="mr.metrics.readableClosedAt"
+        :author="mr.closedBy"
+        :date-title="mr.closedAt"
+        :date-readable="mr.readableClosedAt"
       />
 
       <section class="mr-info-list">
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue
index 1a444c04a1d..ebc29c0fa06 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue
@@ -111,9 +111,9 @@
       <div class="space-children">
         <mr-widget-author-time
           :action-text="s__('mrWidget|Merged by')"
-          :author="mr.metrics.mergedBy"
-          :date-title="mr.metrics.mergedAt"
-          :date-readable="mr.metrics.readableMergedAt"
+          :author="mr.mergedBy"
+          :date-title="mr.mergedAt"
+          :date-readable="mr.readableMergedAt"
         />
         <a
           v-tooltip
diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
index 672e5280b5e..016e5b64371 100644
--- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
+++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
@@ -14,6 +14,7 @@ export default class MergeRequestStore {
   setData(data) {
     const currentUser = data.current_user;
     const pipelineStatus = data.pipeline ? data.pipeline.details.status : null;
+    const mergeUser = MergeRequestStore.formatUserObject(data.merge_user || {});
 
     this.squash = data.squash;
     this.squashBeforeMergeHelpPath =
@@ -47,8 +48,13 @@ export default class MergeRequestStore {
     }
 
     this.updatedAt = data.updated_at;
-    this.metrics = MergeRequestStore.buildMetrics(data.metrics);
-    this.setToMWPSBy = MergeRequestStore.formatUserObject(data.merge_user || {});
+    this.mergedBy = mergeUser;
+    this.mergedAt = MergeRequestStore.formatUserObject(data.merged_at);
+    this.readableMergedAt = MergeRequestStore.getReadableDate(data.merged_at);
+    this.closedBy = MergeRequestStore.formatUserObject(data.closed_by);
+    this.closedAt = MergeRequestStore.formatUserObject(data.closed_at);
+    this.readableClosedAt = MergeRequestStore.getReadableDate(data.closed_at);
+    this.setToMWPSBy = mergeUser;
     this.mergeUserId = data.merge_user_id;
     this.currentUserId = gon.current_user_id;
     this.sourceBranchPath = data.source_branch_path;
@@ -150,21 +156,6 @@ export default class MergeRequestStore {
     this.rebasePath = data.rebase_path;
   }
 
-  static buildMetrics(metrics) {
-    if (!metrics) {
-      return {};
-    }
-
-    return {
-      mergedBy: MergeRequestStore.formatUserObject(metrics.merged_by),
-      closedBy: MergeRequestStore.formatUserObject(metrics.closed_by),
-      mergedAt: formatDate(metrics.merged_at),
-      closedAt: formatDate(metrics.closed_at),
-      readableMergedAt: MergeRequestStore.getReadableDate(metrics.merged_at),
-      readableClosedAt: MergeRequestStore.getReadableDate(metrics.closed_at),
-    };
-  }
-
   static formatUserObject(user) {
     if (!user) {
       return {};
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 396647a14ae..8c5aa90cc20 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -642,14 +642,6 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  def merge_event
-    @merge_event ||= target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::MERGED).last
-  end
-
-  def closed_event
-    @closed_event ||= target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::CLOSED).last
-  end
-
   def work_in_progress?
     self.class.work_in_progress?(title)
   end
@@ -1117,6 +1109,32 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
+  def closed_at
+    strong_memoize(:closed_at) do
+      next unless closed?
+
+      metrics&.latest_closed_at || closed_event&.created_at
+    end
+  end
+
+  def closed_by
+    strong_memoize(:closed_by) do
+      next unless closed?
+
+      metrics&.latest_closed_by || closed_event&.author
+    end
+  end
+
+  def merge_event
+    @merge_event ||= target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::MERGED).last
+  end
+  private :merge_event
+
+  def closed_event
+    @closed_event ||= target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::CLOSED).last
+  end
+  private :closed_event
+
   def can_be_cherry_picked?
     merge_commit.present?
   end
diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb
index f55d448235a..e992cf20159 100644
--- a/app/serializers/merge_request_widget_entity.rb
+++ b/app/serializers/merge_request_widget_entity.rb
@@ -27,13 +27,9 @@ class MergeRequestWidgetEntity < IssuableEntity
   expose :ff_only_enabled do |merge_request|
     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 :closed_at
+  expose :closed_by
+  expose :merged_at
   expose :rebase_commit_sha
   expose :rebase_in_progress?, as: :rebase_in_progress
 
@@ -245,27 +241,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/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 48f4e53b93e..2d7fbb37f64 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1444,8 +1444,7 @@ describe MergeRequest do
       end
 
       it 'returns merged event creation date' do
-        expect(merge_request.merge_event).to be_persisted
-        expect(merge_request.merged_at).to eq(merge_request.merge_event.created_at)
+        expect(merge_request.merged_at).to be_present
       end
     end
 
@@ -1464,7 +1463,6 @@ describe MergeRequest do
 
       it 'returns merging note creation date' do
         expect(merge_request.reload.metrics).to be_nil
-        expect(merge_request.merge_event).to be_nil
         expect(merge_request.notes.count).to eq(1)
         expect(merge_request.merged_at).to eq(merge_request.notes.first.created_at)
       end
diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb
index 0ba2539a717..c9d520e0dfe 100644
--- a/spec/serializers/merge_request_widget_entity_spec.rb
+++ b/spec/serializers/merge_request_widget_entity_spec.rb
@@ -50,81 +50,6 @@ 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")