Merge branch '233942-expose-more-data-for-mr-metrics-dashboard' into 'master'

Expose fields in GraphQL for MR analytics

See merge request gitlab-org/gitlab!39086

app/graphql/resolvers/concerns/resolves_merge_requests.rb

@@ -38,6 +38,9 @@ module ResolvesMergeRequests
       assignees: [:assignees],
       labels: [:labels],
       author: [:author],
+      merged_at: [:metrics],
+      commit_count: [:metrics],
+      approved_by: [:approver_users],
       milestone: [:milestone],
       head_pipeline: [:merge_request_diff, { head_pipeline: [:merge_request] }]
     }

app/graphql/types/ci/pipeline_type.rb

@@ -5,6 +5,8 @@ module Types
     class PipelineType < BaseObject
       graphql_name 'Pipeline'
 
+      connection_type_class(Types::CountableConnectionType)
+
       authorize :read_pipeline
 
       expose_permissions Types::PermissionTypes::Ci::Pipeline

app/graphql/types/issuable_connection_type.rb → app/graphql/types/countable_connection_type.rb

@@ -2,13 +2,14 @@
 
 module Types
   # rubocop: disable Graphql/AuthorizeTypes
-  class IssuableConnectionType < GraphQL::Types::Relay::BaseConnection
+  class CountableConnectionType < GraphQL::Types::Relay::BaseConnection
     field :count, Integer, null: false,
           description: 'Total count of collection'
 
     def count
       # rubocop: disable CodeReuse/ActiveRecord
       relation = object.items
+
       # sometimes relation is an Array
       relation = relation.reorder(nil) if relation.respond_to?(:reorder)
       # rubocop: enable CodeReuse/ActiveRecord

app/graphql/types/issue_type.rb

@@ -4,7 +4,7 @@ module Types
   class IssueType < BaseObject
     graphql_name 'Issue'
 
-    connection_type_class(Types::IssuableConnectionType)
+    connection_type_class(Types::CountableConnectionType)
 
     implements(Types::Notes::NoteableType)
 

app/graphql/types/merge_request_type.rb

@@ -4,7 +4,7 @@ module Types
   class MergeRequestType < BaseObject
     graphql_name 'MergeRequest'
 
-    connection_type_class(Types::IssuableConnectionType)
+    connection_type_class(Types::CountableConnectionType)
 
     implements(Types::Notes::NoteableType)
 
@@ -143,6 +143,8 @@ module Types
     end
     field :task_completion_status, Types::TaskCompletionStatus, null: false,
           description: Types::TaskCompletionStatus.description
+    field :commit_count, GraphQL::INT_TYPE, null: true,
+          description: 'Number of commits in the merge request'
 
     def diff_stats(path: nil)
       stats = Array.wrap(object.diff_stats&.to_a)
@@ -162,5 +164,14 @@ module Types
         hash.merge!(additions: status.additions, deletions: status.deletions, file_count: 1) { |_, x, y| x + y }
       end
     end
+
+    def commit_count
+      object&.metrics&.commits_count
+    end
+
+    def approvers
+      object.approver_users
+    end
   end
 end
+Types::MergeRequestType.prepend_if_ee('::EE::Types::MergeRequestType')

changelogs/unreleased/233942-expose-more-data-for-mr-metrics-dashboard.yml

+---
+title: Expose counts (pipeline, commits) and approvers for a merge request in GraphQL
+merge_request: 39086
+author:
+type: added

doc/api/graphql/reference/gitlab_schema.graphql

@@ -8255,6 +8255,31 @@ type MergeRequest implements Noteable {
   """
   allowCollaboration: Boolean
 
+  """
+  Users who approved the merge request
+  """
+  approvedBy(
+    """
+    Returns the elements in the list that come after the specified cursor.
+    """
+    after: String
+
+    """
+    Returns the elements in the list that come before the specified cursor.
+    """
+    before: String
+
+    """
+    Returns the first _n_ elements from the list.
+    """
+    first: Int
+
+    """
+    Returns the last _n_ elements from the list.
+    """
+    last: Int
+  ): UserConnection
+
   """
   Assignees of the merge request
   """
@@ -8285,6 +8310,11 @@ type MergeRequest implements Noteable {
   """
   author: User
 
+  """
+  Number of commits in the merge request
+  """
+  commitCount: Int
+
   """
   Timestamp of when the merge request was created
   """
@@ -10264,6 +10294,11 @@ type Pipeline {
 The connection type for Pipeline.
 """
 type PipelineConnection {
+  """
+  Total count of collection
+  """
+  count: Int!
+
   """
   A list of edges.
   """

doc/api/graphql/reference/gitlab_schema.json

@@ -22948,6 +22948,59 @@
               "isDeprecated": false,
               "deprecationReason": null
             },
+            {
+              "name": "approvedBy",
+              "description": "Users who approved the merge request",
+              "args": [
+                {
+                  "name": "after",
+                  "description": "Returns the elements in the list that come after the specified cursor.",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "String",
+                    "ofType": null
+                  },
+                  "defaultValue": null
+                },
+                {
+                  "name": "before",
+                  "description": "Returns the elements in the list that come before the specified cursor.",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "String",
+                    "ofType": null
+                  },
+                  "defaultValue": null
+                },
+                {
+                  "name": "first",
+                  "description": "Returns the first _n_ elements from the list.",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "Int",
+                    "ofType": null
+                  },
+                  "defaultValue": null
+                },
+                {
+                  "name": "last",
+                  "description": "Returns the last _n_ elements from the list.",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "Int",
+                    "ofType": null
+                  },
+                  "defaultValue": null
+                }
+              ],
+              "type": {
+                "kind": "OBJECT",
+                "name": "UserConnection",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "assignees",
               "description": "Assignees of the merge request",
@@ -23015,6 +23068,20 @@
               "isDeprecated": false,
               "deprecationReason": null
             },
+            {
+              "name": "commitCount",
+              "description": "Number of commits in the merge request",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "SCALAR",
+                "name": "Int",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "createdAt",
               "description": "Timestamp of when the merge request was created",
@@ -30691,6 +30758,24 @@
           "name": "PipelineConnection",
           "description": "The connection type for Pipeline.",
           "fields": [
+            {
+              "name": "count",
+              "description": "Total count of collection",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "NON_NULL",
+                "name": null,
+                "ofType": {
+                  "kind": "SCALAR",
+                  "name": "Int",
+                  "ofType": null
+                }
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "edges",
               "description": "A list of edges.",

doc/api/graphql/reference/index.md

@@ -1260,6 +1260,7 @@ Autogenerated return type of MarkAsSpamSnippet
 | ---   |  ---- | ----------  |
 | `allowCollaboration` | Boolean | Indicates if members of the target project can push to the fork |
 | `author` | User | User who created this merge request |
+| `commitCount` | Int | Number of commits in the merge request |
 | `createdAt` | Time! | Timestamp of when the merge request was created |
 | `defaultMergeCommitMessage` | String | Default merge commit message of the merge request |
 | `description` | String | Description of the merge request (Markdown rendered as HTML for caching) |

ee/app/graphql/ee/types/merge_request_type.rb

+# frozen_string_literal: true
+
+module EE
+  module Types
+    module MergeRequestType
+      extend ActiveSupport::Concern
+
+      prepended do
+        field :approved_by, ::Types::UserType.connection_type, null: true,
+              description: 'Users who approved the merge request'
+
+        def approved_by
+          object.approver_users
+        end
+      end
+    end
+  end
+end

ee/spec/requests/api/graphql/project/merge_requests_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'getting merge request listings (EE) nested in a project' do
+  include GraphqlHelpers
+
+  let_it_be(:project) { create(:project, :repository, :public) }
+  let_it_be(:current_user) { create(:user) }
+
+  let_it_be(:merge_request_a) { create(:merge_request, :unique_branches, source_project: project) }
+  let_it_be(:merge_request_b) { create(:merge_request, :closed, :unique_branches, source_project: project) }
+  let_it_be(:merge_request_c) { create(:merge_request, :closed, :unique_branches, source_project: project) }
+
+  let(:results) { graphql_data.dig('project', 'mergeRequests', 'nodes') }
+
+  let(:query) do
+    query_merge_requests(all_graphql_fields_for('MergeRequest', max_depth: 1))
+  end
+
+  def query_merge_requests(fields)
+    graphql_query_for(
+      :project,
+      { full_path: project.full_path },
+      query_graphql_field(:merge_requests, search_params, [
+        query_graphql_field(:nodes, nil, fields)
+      ])
+    )
+  end
+
+  def execute_query
+    query = query_merge_requests(requested_fields)
+    post_graphql(query, current_user: current_user)
+  end
+
+  context 'when requesting `approved_by`' do
+    let(:search_params) { { iids: [merge_request_a.iid.to_s, merge_request_b.iid.to_s] } }
+    let(:extra_iid_for_second_query) { merge_request_c.iid.to_s }
+    let(:requested_fields) { query_graphql_field(:approved_by, nil, query_graphql_field(:nodes, nil, [:username])) }
+
+    it 'exposes approver username' do
+      merge_request_a.approver_users << current_user
+
+      execute_query
+
+      user_data = { 'username' => current_user.username }
+      expect(results).to include(a_hash_including('approvedBy' => { 'nodes' => array_including(user_data) }))
+    end
+
+    include_examples 'N+1 query check'
+  end
+end

spec/graphql/types/merge_request_type_spec.rb

@@ -24,9 +24,11 @@ RSpec.describe GitlabSchema.types['MergeRequest'] do
       source_branch_exists target_branch_exists
       upvotes downvotes head_pipeline pipelines task_completion_status
       milestone assignees participants subscribed labels discussion_locked time_estimate
-      total_time_spent reference author merged_at
+      total_time_spent reference author merged_at commit_count
     ]
 
+    expected_fields << 'approved_by' if Gitlab.ee?
+
     expect(described_class).to have_graphql_fields(*expected_fields)
   end
 end

spec/requests/api/graphql/project/merge_requests_spec.rb

@@ -171,4 +171,43 @@ RSpec.describe 'getting merge request listings nested in a project' do
 
     it_behaves_like 'searching with parameters'
   end
+
+  describe 'fields' do
+    let(:requested_fields) { nil }
+    let(:extra_iid_for_second_query) { merge_request_c.iid.to_s }
+    let(:search_params) { { iids: [merge_request_a.iid.to_s, merge_request_b.iid.to_s] } }
+
+    def execute_query
+      query = query_merge_requests(requested_fields)
+      post_graphql(query, current_user: current_user)
+    end
+
+    context 'when requesting `commit_count`' do
+      let(:requested_fields) { [:commit_count] }
+
+      it 'exposes `commit_count`' do
+        merge_request_a.metrics.update!(commits_count: 5)
+
+        execute_query
+
+        expect(results).to include(a_hash_including('commitCount' => 5))
+      end
+
+      include_examples 'N+1 query check'
+    end
+
+    context 'when requesting `merged_at`' do
+      let(:requested_fields) { [:merged_at] }
+
+      before do
+        # make the MRs "merged"
+        [merge_request_a, merge_request_b, merge_request_c].each do |mr|
+          mr.update_column(:state_id, MergeRequest.available_states[:merged])
+          mr.metrics.update_column(:merged_at, Time.now)
+        end
+      end
+
+      include_examples 'N+1 query check'
+    end
+  end
 end

spec/support/shared_examples/graphql/projects/merge_request_n_plus_one_query_examples.rb

+# frozen_string_literal: true
+shared_examples 'N+1 query check' do
+  it 'prevents N+1 queries' do
+    execute_query # "warm up" to prevent undeterministic counts
+
+    control_count = ActiveRecord::QueryRecorder.new { execute_query }.count
+
+    search_params[:iids] << extra_iid_for_second_query
+    expect { execute_query }.not_to exceed_query_limit(control_count)
+  end
+end