Add the "Review" cycle analytics section.

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)

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

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
 
+    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?

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

app/views/projects/cycle_analytics/show.html.haml

@@ -19,3 +19,10 @@
       = distance_of_time_in_words code
     - else
       = "<Not enough data>"
+
+  %li.list-group-item
+    Review:
+    - if review = @cycle_analytics.review.presence
+      = distance_of_time_in_words review
+    - else
+      = "<Not enough data>"

db/migrate/20160825052008_add_table_merge_request_metrics.rb

@@ -29,6 +29,8 @@ class AddTableMergeRequestMetrics < ActiveRecord::Migration
 
       t.datetime 'wip_flag_first_removed_at'
       t.datetime 'first_assigned_to_user_other_than_author'
+      t.datetime 'merged_at'
+      t.datetime 'first_closed_at'
 
       t.timestamps null: false
     end

db/schema.rb

@@ -609,6 +609,8 @@ ActiveRecord::Schema.define(version: 20160825052008) do
     t.integer  "merge_request_id",                         null: false
     t.datetime "wip_flag_first_removed_at"
     t.datetime "first_assigned_to_user_other_than_author"
+    t.datetime "merged_at"
+    t.datetime "first_closed_at"
     t.datetime "created_at",                               null: false
     t.datetime "updated_at",                               null: false
   end