Merge branch '194161-code-review-analytics-api' into 'master'

Resolve "Code Review Analytics MVC - API Implementation"

Closes #194161

See merge request gitlab-org/gitlab!22801

ee/app/models/ee/merge_request.rb

@@ -52,6 +52,16 @@ module EE
           true
         end
       end
+
+      scope :order_review_time_desc, -> do
+        joins(:metrics).reorder(::Gitlab::Database.nulls_last_order('merge_request_metrics.first_comment_at'))
+      end
+
+      scope :with_code_review_api_entity_associations, -> do
+        preload(
+          :author, :approved_by_users, :metrics,
+          latest_merge_request_diff: :merge_request_diff_files, target_project: :namespace, milestone: :project)
+      end
     end
 
     class_methods do
@@ -63,6 +73,24 @@ module EE
       def with_api_entity_associations
         super.preload(:blocking_merge_requests)
       end
+
+      def sort_by_attribute(method, *args)
+        if method.to_s == 'review_time_desc'
+          order_review_time_desc
+        else
+          super
+        end
+      end
+
+      # Includes table keys in group by clause when sorting
+      # preventing errors in postgres
+      #
+      # Returns an array of arel columns
+      def grouping_columns(sort)
+        grouping_columns = super
+        grouping_columns << ::MergeRequest::Metrics.arel_table[:first_comment_at] if sort.to_s == 'review_time_desc'
+        grouping_columns
+      end
     end
 
     override :mergeable?

ee/lib/api/analytics/code_review_analytics.rb

+# frozen_string_literal: true
+
+module API
+  module Analytics
+    class CodeReviewAnalytics < Grape::API
+      include PaginationParams
+
+      helpers ::Gitlab::IssuableMetadata
+
+      helpers do
+        def project
+          @project ||= find_project!(params[:project_id])
+        end
+
+        def finder
+          @finder ||= begin
+            finder_options = {
+              state: 'opened',
+              project_id: project.id,
+              sort: 'review_time_desc',
+              attempt_project_search_optimizations: true
+            }
+
+            finder_options = params.slice(*MergeRequestsFinder.valid_params).merge(finder_options)
+
+            MergeRequestsFinder.new(current_user, finder_options)
+          end
+        end
+      end
+
+      before do
+        not_found! unless Feature.enabled?(:code_review_analytics)
+      end
+
+      resource :analytics do
+        desc 'List code review information about project' do
+        end
+        params do
+          requires :project_id, type: Integer, desc: 'Project ID'
+          optional :label_name, type: Array, desc: 'Array of label names to filter by'
+          optional :milestone_title, type: String, desc: 'Milestone title to filter by'
+          use :pagination
+        end
+        get 'code_review' do
+          authorize! :read_code_review_analytics, project
+
+          merge_requests = paginate(finder.execute.with_code_review_api_entity_associations)
+
+          present merge_requests,
+                  with: EE::API::Entities::Analytics::CodeReview::MergeRequest,
+                  current_user: current_user,
+                  issuable_metadata: issuable_meta_data(merge_requests, 'MergeRequest', current_user)
+        end
+      end
+    end
+  end
+end

ee/lib/ee/api/api.rb

@@ -49,6 +49,7 @@ module EE
         mount ::API::ProjectAliases
         mount ::API::Dependencies
         mount ::API::VisualReviewDiscussions
+        mount ::API::Analytics::CodeReviewAnalytics
 
         version 'v3', using: :path do
           # Although the following endpoints are kept behind V3 namespace,

ee/lib/ee/api/entities.rb

@@ -92,7 +92,7 @@ module EE
         prepended do
           expose :group_saml_identity,
                  using: ::API::Entities::Identity,
-                 if:  -> (member, options) { Ability.allowed?(options[:current_user], :read_group_saml_identity, member.source) }
+                 if: -> (member, options) { Ability.allowed?(options[:current_user], :read_group_saml_identity, member.source) }
         end
       end
 
@@ -576,14 +576,14 @@ module EE
 
       class GitlabLicense < Grape::Entity
         expose :id,
-          :plan,
-          :created_at,
-          :starts_at,
-          :expires_at,
-          :historical_max,
-          :maximum_user_count,
-          :licensee,
-          :add_ons
+               :plan,
+               :created_at,
+               :starts_at,
+               :expires_at,
+               :historical_max,
+               :maximum_user_count,
+               :licensee,
+               :add_ons
 
         expose :expired?, as: :expired
 
@@ -1008,6 +1008,46 @@ module EE
         expose :issue, using: ::API::Entities::IssueBasic
         expose :link_type
       end
+
+      module Analytics
+        module CodeReview
+          class MergeRequest < ::API::Entities::MergeRequestSimple
+            expose :milestone, using: ::API::Entities::Milestone
+            expose :author, using: ::API::Entities::UserBasic
+            expose :approved_by_users, as: :approved_by, using: ::API::Entities::UserBasic
+            expose :notes_count do |mr|
+              if options[:issuable_metadata]
+                # Avoids an N+1 query when metadata is included
+                options[:issuable_metadata][mr.id].user_notes_count
+              else
+                mr.notes.user.count
+              end
+            end
+            expose :review_time do |mr|
+              next unless mr.metrics.first_comment_at
+
+              review_time = (mr.metrics.merged_at || Time.now) - mr.metrics.first_comment_at
+
+              (review_time / ActiveSupport::Duration::SECONDS_PER_HOUR).floor
+            end
+            expose :diff_stats
+
+            private
+
+            # rubocop: disable CodeReuse/ActiveRecord
+            def diff_stats
+              result = {
+                additions: object.diffs.diff_files.sum(&:added_lines),
+                deletions: object.diffs.diff_files.sum(&:removed_lines),
+                commits_count: object.commits_count
+              }
+              result[:total] = result[:additions] + result[:deletions]
+              result
+            end
+            # rubocop: enable CodeReuse/ActiveRecord
+          end
+        end
+      end
     end
   end
 end

ee/spec/lib/ee/api/entities/analytics/code_review/merge_request_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::API::Entities::Analytics::CodeReview::MergeRequest do
+  subject(:entity_representation) { described_class.new(merge_request).as_json }
+
+  let(:merge_request) do
+    create(:merge_request, :with_diffs, :with_productivity_metrics,
+           milestone: milestone,
+           source_project: project,
+           metrics_data: { first_comment_at: 1.day.ago, merged_at: 1.hour.ago }
+    )
+  end
+  let(:project) { create :project, :repository }
+  let(:milestone) { create(:milestone, project: project) }
+  let!(:note) { create(:note_on_merge_request, project: project, noteable: merge_request) }
+
+  it 'exposes mr attributes' do
+    expect(entity_representation).to include(
+      {
+        id: merge_request.id,
+        iid: merge_request.iid,
+        title: merge_request.title,
+        created_at: merge_request.created_at,
+        notes_count: 1,
+        review_time: 23,
+        diff_stats: {
+          additions: 118,
+          deletions: 9,
+          total: 127,
+          commits_count: 29
+        }
+      }
+    )
+    expect(entity_representation[:milestone][:title]).to eq milestone.title
+    expect(entity_representation[:author][:id]).to eq merge_request.author.id
+  end
+end

ee/spec/models/merge_request_spec.rb

@@ -789,4 +789,15 @@ describe MergeRequest do
       end
     end
   end
+
+  describe 'review time sorting' do
+    it 'orders by first_comment_at' do
+      merge_request_1 = create(:merge_request, :with_productivity_metrics, metrics_data: { first_comment_at: 1.day.ago })
+      merge_request_2 = create(:merge_request, :with_productivity_metrics, metrics_data: { first_comment_at: 3.days.ago })
+      merge_request_3 = create(:merge_request, :with_productivity_metrics, metrics_data: { first_comment_at: nil })
+
+      expect(described_class.order_review_time_desc).to match([merge_request_2, merge_request_1, merge_request_3])
+      expect(described_class.sort_by_attribute('review_time_desc')).to match([merge_request_2, merge_request_1, merge_request_3])
+    end
+  end
 end

ee/spec/requests/api/analytics/code_review_analytics_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Analytics::CodeReviewAnalytics do
+  let_it_be(:group) { create(:group, :private) }
+  let_it_be(:project) { create(:project, namespace: group) }
+  let(:current_user) { reporter }
+
+  let_it_be(:reporter) do
+    create(:user).tap { |u| project.add_reporter(u) }
+  end
+  let_it_be(:guest) do
+    create(:user).tap { |u| project.add_guest(u) }
+  end
+
+  describe 'GET code_review' do
+    subject(:api_call) do
+      get api("/analytics/code_review?#{query_params.to_query}", current_user)
+    end
+
+    let(:query_params) { { project_id: project.id } }
+
+    it 'is successful' do
+      api_call
+
+      expect(response).to have_gitlab_http_status(:ok)
+    end
+
+    context 'with merge requests present' do
+      let_it_be(:label) { create :label, project: project }
+      let_it_be(:milestone) { create :milestone, project: project }
+      let!(:merge_request_1) { create(:merge_request, :opened, source_project: project, target_branch: 'mr1') }
+      let!(:merge_request_2) { create(:labeled_merge_request, :opened, source_project: project, labels: [label], target_branch: 'mr2') }
+      let!(:merge_request_3) { create(:labeled_merge_request, :opened, source_project: project, labels: [label], milestone: milestone, target_branch: 'mr3') }
+      let!(:closed_merge_request) { create(:merge_request, :closed, source_project: project, target_branch: 'mr4') }
+      let!(:merged_merge_request) { create(:merge_request, :merged, source_project: project, target_branch: 'mr5') }
+
+      it 'returns list of open MRs with pagination headers' do
+        api_call
+
+        expect(json_response.map { |mr| mr['id']}).to match_array([merge_request_1.id, merge_request_2.id, merge_request_3.id])
+        expect(json_response.first.keys)
+          .to include(*%w[id iid web_url created_at milestone review_time author approved_by notes_count diff_stats])
+        expect(response.headers).to include(*%w[X-Per-Page X-Page X-Next-Page X-Prev-Page X-Total X-Total-Pages])
+      end
+
+      context 'with label & milestone filters' do
+        let(:query_params) { super().merge(label_name: [label.title], milestone_title: milestone.title) }
+
+        it 'applies filter' do
+          api_call
+
+          expect(json_response.map { |mr| mr['id']}).to match_array([merge_request_3.id])
+        end
+      end
+    end
+
+    context 'when feature is disabled' do
+      before do
+        stub_feature_flags(code_review_analytics: false)
+      end
+
+      it 'is not found' do
+        api_call
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'when user has no authorization' do
+      let(:current_user) { guest }
+
+      it 'is not authorized' do
+        api_call
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
+
+    context 'when feature is not available in plan' do
+      before do
+        stub_licensed_features(code_review_analytics: false)
+      end
+
+      it 'is not_authorized' do
+        api_call
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
+
+    context 'when project_id is not specified' do
+      subject(:api_call) { get api("/analytics/code_review", current_user) }
+
+      it 'is not found' do
+        api_call
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+      end
+    end
+  end
+end