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

15 files changed, 132 insertions, 55 deletions

diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.js
index b25cc3443ef..dd8b2665b1d 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.js
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.js
@@ -16,9 +16,9 @@ export default {
       <div class="media-body">
         <mr-widget-author-and-time
           actionText="Closed by"
-          :author="mr.closedEvent.author"
-          :dateTitle="mr.closedEvent.updatedAt"
-          :dateReadable="mr.closedEvent.formattedUpdatedAt"
+          :author="mr.metrics.closedBy"
+          :dateTitle="mr.metrics.closedAt"
+          :dateReadable="mr.metrics.readableClosedAt"
         />
         <section class="mr-info-list">
           <p>
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js
index 7c73ebf667d..ba9681680ef 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js
@@ -68,9 +68,9 @@ export default {
         <div class="space-children">
           <mr-widget-author-and-time
             actionText="Merged by"
-            :author="mr.mergedEvent.author"
-            :date-title="mr.mergedEvent.updatedAt"
-            :date-readable="mr.mergedEvent.formattedUpdatedAt" />
+            :author="mr.metrics.mergedBy"
+            :date-title="mr.metrics.mergedAt"
+            :date-readable="mr.metrics.readableMergedAt" />
           <a
             v-if="mr.canRevertInCurrentMR"
             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 93d31a2a684..474c17ec133 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
@@ -39,9 +39,8 @@ export default class MergeRequestStore {
     }
 
     this.updatedAt = data.updated_at;
-    this.mergedEvent = MergeRequestStore.getEventObject(data.merge_event);
-    this.closedEvent = MergeRequestStore.getEventObject(data.closed_event);
-    this.setToMWPSBy = MergeRequestStore.getAuthorObject({ author: data.merge_user || {} });
+    this.metrics = MergeRequestStore.buildMetrics(data.metrics);
+    this.setToMWPSBy = MergeRequestStore.formatUserObject(data.merge_user || {});
     this.mergeUserId = data.merge_user_id;
     this.currentUserId = gon.current_user_id;
     this.sourceBranchPath = data.source_branch_path;
@@ -125,43 +124,42 @@ export default class MergeRequestStore {
     return this.state === stateKey.nothingToMerge;
   }
 
-  static getEventObject(event) {
+  static buildMetrics(metrics) {
+    if (!metrics) {
+      return {};
+    }
+
     return {
-      author: MergeRequestStore.getAuthorObject(event),
-      updatedAt: formatDate(MergeRequestStore.getEventUpdatedAtDate(event)),
-      formattedUpdatedAt: MergeRequestStore.getEventDate(event),
+      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 getAuthorObject(event) {
-    if (!event) {
+  static formatUserObject(user) {
+    if (!user) {
       return {};
     }
 
     return {
-      name: event.author.name || '',
-      username: event.author.username || '',
-      webUrl: event.author.web_url || '',
-      avatarUrl: event.author.avatar_url || '',
+      name: user.name || '',
+      username: user.username || '',
+      webUrl: user.web_url || '',
+      avatarUrl: user.avatar_url || '',
     };
   }
 
-  static getEventUpdatedAtDate(event) {
-    if (!event) {
+  static getReadableDate(date) {
+    if (!date) {
       return '';
     }
 
-    return event.updated_at;
-  }
-
-  static getEventDate(event) {
     const timeagoInstance = new Timeago();
 
-    if (!event) {
-      return '';
-    }
-
-    return timeagoInstance.format(MergeRequestStore.getEventUpdatedAtDate(event));
+    return timeagoInstance.format(date);
   }
 
 }
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 5ca4a7086cb..4251561a0a0 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -96,7 +96,7 @@ module Issuable
 
     strip_attributes :title
 
-    after_save :record_metrics, unless: :imported?
+    after_save :ensure_metrics, unless: :imported?
 
     # We want to use optimistic lock for cases when only title or description are involved
     # http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html
@@ -335,11 +335,6 @@ module Issuable
     false
   end
 
-  def record_metrics
-    metrics = self.metrics || create_metrics
-    metrics.record!
-  end
-
   ##
   # Override in issuable specialization
   #
@@ -347,6 +342,10 @@ module Issuable
     false
   end
 
+  def ensure_metrics
+    self.metrics || create_metrics
+  end
+
   ##
   # Overriden in MergeRequest
   #
diff --git a/app/models/issue.rb b/app/models/issue.rb
index dc64888b6fc..4eafc1316d6 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -276,6 +276,11 @@ class Issue < ActiveRecord::Base
 
   private
 
+  def ensure_metrics
+    super
+    metrics.record!
+  end
+
   # Returns `true` if the given User can read the current Issue.
   #
   # This method duplicates the same check of issue_policy.rb
diff --git a/app/models/merge_request/metrics.rb b/app/models/merge_request/metrics.rb
index cdc408738be..9e660eccd86 100644
--- a/app/models/merge_request/metrics.rb
+++ b/app/models/merge_request/metrics.rb
@@ -1,12 +1,6 @@
 class MergeRequest::Metrics < ActiveRecord::Base
   belongs_to :merge_request
   belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :pipeline_id
-
-  def record!
-    if merge_request.merged? && self.merged_at.blank?
-      self.merged_at = Time.now
-    end
-
-    self.save
-  end
+  belongs_to :latest_closed_by, class_name: 'User'
+  belongs_to :merged_by, class_name: 'User'
 end
diff --git a/app/serializers/event_entity.rb b/app/serializers/event_entity.rb
deleted file mode 100644
index 935d67a4f37..00000000000
--- a/app/serializers/event_entity.rb
+++ /dev/null
@@ -1,4 +0,0 @@
-class EventEntity < Grape::Entity
-  expose :author, using: UserEntity
-  expose :updated_at
-end
diff --git a/app/serializers/merge_request_metrics_entity.rb b/app/serializers/merge_request_metrics_entity.rb
new file mode 100644
index 00000000000..3548107ac16
--- /dev/null
+++ b/app/serializers/merge_request_metrics_entity.rb
@@ -0,0 +1,6 @@
+class MergeRequestMetricsEntity < Grape::Entity
+  expose :latest_closed_at, as: :closed_at
+  expose :merged_at
+  expose :latest_closed_by, as: :closed_by, using: UserEntity
+  expose :merged_by, using: UserEntity
+end
diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb
index f8e59b2ffd7..e905e6876c2 100644
--- a/app/serializers/merge_request_widget_entity.rb
+++ b/app/serializers/merge_request_widget_entity.rb
@@ -17,9 +17,11 @@ class MergeRequestWidgetEntity < IssuableEntity
     merge_request.project.merge_requests_ff_only_enabled
   end
 
-  # Events
-  expose :merge_event, using: EventEntity
-  expose :closed_event, using: EventEntity
+  expose :metrics do |merge_request|
+    metrics = build_metrics(merge_request)
+
+    MergeRequestMetricsEntity.new(metrics).as_json
+  end
 
   # User entities
   expose :merge_user, using: UserEntity
@@ -178,4 +180,27 @@ 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/app/services/event_create_service.rb b/app/services/event_create_service.rb
index 6328d567a07..44dc90b3462 100644
--- a/app/services/event_create_service.rb
+++ b/app/services/event_create_service.rb
@@ -103,6 +103,6 @@ class EventCreateService
       author_id: current_user.id
     )
 
-    Event.create(attributes)
+    Event.create!(attributes)
   end
 end
diff --git a/app/services/merge_request_metrics_service.rb b/app/services/merge_request_metrics_service.rb
new file mode 100644
index 00000000000..9248de14a53
--- /dev/null
+++ b/app/services/merge_request_metrics_service.rb
@@ -0,0 +1,19 @@
+class MergeRequestMetricsService
+  delegate :update!, to: :@merge_request_metrics
+
+  def initialize(merge_request_metrics)
+    @merge_request_metrics = merge_request_metrics
+  end
+
+  def merge(event)
+    update!(merged_by_id: event.author_id, merged_at: event.created_at)
+  end
+
+  def close(event)
+    update!(latest_closed_by_id: event.author_id, latest_closed_at: event.created_at)
+  end
+
+  def reopen
+    update!(latest_closed_by_id: nil, latest_closed_at: nil)
+  end
+end
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index 6b32d65a74b..20a2b50d3de 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -24,6 +24,10 @@ module MergeRequests
 
     private
 
+    def merge_request_metrics_service(merge_request)
+      MergeRequestMetricsService.new(merge_request.metrics)
+    end
+
     def create_assignee_note(merge_request)
       SystemNoteService.change_assignee(
         merge_request, merge_request.project, current_user, merge_request.assignee)
diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb
index 40213c99014..f727ec002e7 100644
--- a/app/services/merge_requests/close_service.rb
+++ b/app/services/merge_requests/close_service.rb
@@ -8,7 +8,7 @@ module MergeRequests
       merge_request.allow_broken = true
 
       if merge_request.close
-        event_service.close_mr(merge_request, current_user)
+        create_event(merge_request)
         create_note(merge_request)
         notification_service.close_mr(merge_request, current_user)
         todo_service.close_merge_request(merge_request, current_user)
@@ -19,5 +19,16 @@ module MergeRequests
 
       merge_request
     end
+
+    private
+
+    def create_event(merge_request)
+      # Making sure MergeRequest::Metrics updates are in sync with
+      # Event creation.
+      Event.transaction do
+        close_event = event_service.close_mr(merge_request, current_user)
+        merge_request_metrics_service(merge_request).close(close_event)
+      end
+    end
   end
 end
diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb
index b1d6bac4d4a..c78e78afcd1 100644
--- a/app/services/merge_requests/post_merge_service.rb
+++ b/app/services/merge_requests/post_merge_service.rb
@@ -9,7 +9,7 @@ module MergeRequests
       close_issues(merge_request)
       todo_service.merge_merge_request(merge_request, current_user)
       merge_request.mark_as_merged
-      create_merge_event(merge_request, current_user)
+      create_event(merge_request)
       create_note(merge_request)
       notification_service.merge_mr(merge_request, current_user)
       execute_hooks(merge_request, 'merge')
@@ -34,5 +34,14 @@ module MergeRequests
     def create_merge_event(merge_request, current_user)
       EventCreateService.new.merge_mr(merge_request, current_user)
     end
+
+    def create_event(merge_request)
+      # Making sure MergeRequest::Metrics updates are in sync with
+      # Event creation.
+      Event.transaction do
+        merge_event = create_merge_event(merge_request, current_user)
+        merge_request_metrics_service(merge_request).merge(merge_event)
+      end
+    end
   end
 end
diff --git a/app/services/merge_requests/reopen_service.rb b/app/services/merge_requests/reopen_service.rb
index c599a90f9fe..120677a7149 100644
--- a/app/services/merge_requests/reopen_service.rb
+++ b/app/services/merge_requests/reopen_service.rb
@@ -4,7 +4,7 @@ module MergeRequests
       return merge_request unless can?(current_user, :update_merge_request, merge_request)
 
       if merge_request.reopen
-        event_service.reopen_mr(merge_request, current_user)
+        create_event(merge_request)
         create_note(merge_request, 'reopened')
         notification_service.reopen_mr(merge_request, current_user)
         execute_hooks(merge_request, 'reopen')
@@ -16,5 +16,16 @@ module MergeRequests
 
       merge_request
     end
+
+    private
+
+    def create_event(merge_request)
+      # Making sure MergeRequest::Metrics updates are in sync with
+      # Event creation.
+      Event.transaction do
+        event_service.reopen_mr(merge_request, current_user)
+        merge_request_metrics_service(merge_request).reopen
+      end
+    end
   end
 end