Add the "Review" cycle analytics section.

4 files changed, 38 insertions, 7 deletions

diff --git a/app/models/cycle_analytics.rb b/app/models/cycle_analytics.rb
index 68efa826b5a..16072c35b64 100644
--- a/app/models/cycle_analytics.rb
+++ b/app/models/cycle_analytics.rb
@@ -15,11 +15,18 @@ class CycleAnalytics
   def code
     issues = Issue.all.to_a
     start_time_fn = -> (merge_request) { merge_request.created_at }
-    calculate_metric(issues.map(&:closed_by_merge_requests).flatten,
+    calculate_metric(issues.map { |issue| issue.closed_by_merge_requests(nil, check_if_open: false) }.flatten,
                      start_time_fn,
                      Queries::mr_wip_flag_removed_or_assigned_to_user_other_than_author_time)
   end
 
+  def review
+    issues = Issue.all.to_a
+    calculate_metric(issues.map { |issue| issue.closed_by_merge_requests(nil, check_if_open: false) }.flatten,
+                     Queries::mr_wip_flag_removed_or_assigned_to_user_other_than_author_time,
+                     Queries::mr_first_closed_or_merged_at)
+  end
+
   private
 
   def calculate_metric(data, start_time_fn, end_time_fn)
diff --git a/app/models/cycle_analytics/queries.rb b/app/models/cycle_analytics/queries.rb
index ec0311b91b5..41081a4e08c 100644
--- a/app/models/cycle_analytics/queries.rb
+++ b/app/models/cycle_analytics/queries.rb
@@ -3,13 +3,24 @@ class CycleAnalytics
     class << self
       def issue_first_associated_with_milestone_or_first_added_to_list_label_time
         lambda do |issue|
-          issue.metrics.first_associated_with_milestone_at.presence || issue.metrics.first_added_to_board_at.presence
+          if issue.metrics.present?
+            issue.metrics.first_associated_with_milestone_at.presence ||
+              issue.metrics.first_added_to_board_at.presence
+          end
+        end
+      end
+
+      def mr_first_closed_or_merged_at
+        lambda do |merge_request|
+          if merge_request.metrics.present?
+            merge_request.metrics.merged_at.presence || merge_request.metrics.first_closed_at.presence
+          end
         end
       end
 
       def issue_closing_merge_request_opened_time
         lambda do |issue|
-          merge_requests = issue.closed_by_merge_requests
+          merge_requests = issue.closed_by_merge_requests(nil, check_if_open: false)
           merge_requests.map(&:created_at).min if merge_requests.present?
         end
       end
@@ -17,7 +28,8 @@ class CycleAnalytics
       def mr_wip_flag_removed_or_assigned_to_user_other_than_author_time
         lambda do |merge_request|
           if merge_request.metrics.present?
-            merge_request.metrics.wip_flag_first_removed_at || merge_request.metrics.first_assigned_to_user_other_than_author
+            merge_request.metrics.wip_flag_first_removed_at.presence ||
+              merge_request.metrics.first_assigned_to_user_other_than_author.presence
           end
         end
       end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 1aa0cef884e..cec5e456c94 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -198,8 +198,8 @@ class Issue < ActiveRecord::Base
 
   # From all notes on this issue, we'll select the system notes about linked
   # merge requests. Of those, the MRs closing `self` are returned.
-  def closed_by_merge_requests(current_user = nil)
-    return [] unless open?
+  def closed_by_merge_requests(current_user = nil, check_if_open: true)
+    return [] if !open? && check_if_open
 
     ext = all_references(current_user)
 
@@ -207,7 +207,11 @@ class Issue < ActiveRecord::Base
       note.all_references(current_user, extractor: ext)
     end
 
-    ext.merge_requests.select { |mr| mr.open? && mr.closes_issue?(self) }
+    if check_if_open
+      ext.merge_requests.select { |mr| mr.open? && mr.closes_issue?(self) }
+    else
+      ext.merge_requests.select { |mr| mr.closes_issue?(self) }
+    end
   end
 
   def moved?
diff --git a/app/models/merge_request/metrics.rb b/app/models/merge_request/metrics.rb
index 7f7cf95e388..a0bc41c9cc1 100644
--- a/app/models/merge_request/metrics.rb
+++ b/app/models/merge_request/metrics.rb
@@ -10,6 +10,14 @@ class MergeRequest::Metrics < ActiveRecord::Base
       self.first_assigned_to_user_other_than_author = Time.now
     end
 
+    if merge_request.merged? && self.merged_at.blank?
+      self.merged_at = Time.now
+    end
+
+    if merge_request.closed? && self.first_closed_at.blank?
+      self.first_closed_at = Time.now
+    end
+
     self.save if self.changed?
   end
 end