Merge branch 'bvl-comments-graphql' into 'master'

Expose comments on Noteables in GraphQL

Closes #62825

See merge request gitlab-org/gitlab-ce!29212

app/graphql/gitlab_schema.rb

@@ -7,8 +7,8 @@ class GitlabSchema < GraphQL::Schema
   AUTHENTICATED_COMPLEXITY = 250
   ADMIN_COMPLEXITY         = 300
 
-  DEFAULT_MAX_DEPTH = 10
-  AUTHENTICATED_MAX_DEPTH = 15
+  DEFAULT_MAX_DEPTH = 15
+  AUTHENTICATED_MAX_DEPTH = 20
 
   use BatchLoader::GraphQL
   use Gitlab::Graphql::Authorize

app/graphql/types/issue_type.rb

@@ -4,6 +4,8 @@ module Types
   class IssueType < BaseObject
     graphql_name 'Issue'
 
+    implements(Types::Notes::NoteableType)
+
     authorize :read_issue
 
     expose_permissions Types::PermissionTypes::Issue

app/graphql/types/merge_request_type.rb

@@ -4,6 +4,8 @@ module Types
   class MergeRequestType < BaseObject
     graphql_name 'MergeRequest'
 
+    implements(Types::Notes::NoteableType)
+
     authorize :read_merge_request
 
     expose_permissions Types::PermissionTypes::MergeRequest

app/graphql/types/notes/diff_position_type.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+module Types
+  module Notes
+    class DiffPositionType < BaseObject
+      graphql_name 'DiffPosition'
+
+      field :head_sha, GraphQL::STRING_TYPE, null: false,
+            description: "The sha of the head at the time the comment was made"
+      field :base_sha,  GraphQL::STRING_TYPE, null: true,
+            description: "The merge base of the branch the comment was made on"
+      field :start_sha, GraphQL::STRING_TYPE, null: false,
+            description: "The sha of the branch being compared against"
+
+      field :file_path, GraphQL::STRING_TYPE, null: false,
+            description: "The path of the file that was changed"
+      field :old_path, GraphQL::STRING_TYPE, null: true,
+            description: "The path of the file on the start sha."
+      field :new_path, GraphQL::STRING_TYPE, null: true,
+            description: "The path of the file on the head sha."
+      field :position_type, Types::Notes::PositionTypeEnum, null: false
+
+      # Fields for text positions
+      field :old_line, GraphQL::INT_TYPE, null: true,
+            description: "The line on start sha that was changed",
+            resolve: -> (position, _args, _ctx) { position.old_line if position.on_text? }
+      field :new_line, GraphQL::INT_TYPE, null: true,
+            description: "The line on head sha that was changed",
+            resolve: -> (position, _args, _ctx) { position.new_line if position.on_text? }
+
+      # Fields for image positions
+      field :x, GraphQL::INT_TYPE, null: true,
+            description: "The X postion on which the comment was made",
+            resolve: -> (position, _args, _ctx) { position.x if position.on_image? }
+      field :y, GraphQL::INT_TYPE, null: true,
+            description: "The Y position on which the comment was made",
+            resolve: -> (position, _args, _ctx) { position.y if position.on_image? }
+      field :width, GraphQL::INT_TYPE, null: true,
+            description: "The total width of the image",
+            resolve: -> (position, _args, _ctx) { position.width if position.on_image? }
+      field :height, GraphQL::INT_TYPE, null: true,
+            description: "The total height of the image",
+            resolve: -> (position, _args, _ctx) { position.height if position.on_image? }
+    end
+  end
+end

app/graphql/types/notes/discussion_type.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+module Types
+  module Notes
+    class DiscussionType < BaseObject
+      graphql_name 'Discussion'
+
+      authorize :read_note
+
+      field :id, GraphQL::ID_TYPE, null: false
+      field :created_at, Types::TimeType, null: false
+      field :notes, Types::Notes::NoteType.connection_type, null: false, description: "All notes in the discussion"
+    end
+  end
+end

app/graphql/types/notes/note_type.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+module Types
+  module Notes
+    class NoteType < BaseObject
+      graphql_name 'Note'
+
+      authorize :read_note
+
+      expose_permissions Types::PermissionTypes::Note
+
+      field :id, GraphQL::ID_TYPE, null: false
+
+      field :project, Types::ProjectType,
+            null: true,
+            description: "The project this note is associated to",
+            resolve: -> (note, args, context) { Gitlab::Graphql::Loaders::BatchModelLoader.new(Project, note.project_id).find }
+
+      field :author, Types::UserType,
+            null: false,
+            description: "The user who wrote this note",
+            resolve: -> (note, args, context) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, note.author_id).find }
+
+      field :resolved_by, Types::UserType,
+            null: true,
+            description: "The user that resolved the discussion",
+            resolve: -> (note, _args, _context) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, note.resolved_by_id).find }
+
+      field :system, GraphQL::BOOLEAN_TYPE,
+            null: false,
+            description: "Whether or not this note was created by the system or by a user"
+
+      field :body, GraphQL::STRING_TYPE,
+            null: false,
+            method: :note,
+            description: "The content note itself"
+
+      field :created_at, Types::TimeType, null: false
+      field :updated_at, Types::TimeType, null: false
+      field :discussion, Types::Notes::DiscussionType, null: true, description: "The discussion this note is a part of"
+      field :resolvable, GraphQL::BOOLEAN_TYPE, null: false, method: :resolvable?
+      field :resolved_at, Types::TimeType, null: true, description: "The time the discussion was resolved"
+      field :position, Types::Notes::DiffPositionType, null: true, description: "The position of this note on a diff"
+    end
+  end
+end

app/graphql/types/notes/noteable_type.rb

@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+module Types
+  module Notes
+    module NoteableType
+      include Types::BaseInterface
+
+      field :notes, Types::Notes::NoteType.connection_type, null: false, description: "All notes on this noteable"
+      field :discussions, Types::Notes::DiscussionType.connection_type, null: false, description: "All discussions on this noteable"
+
+      definition_methods do
+        def resolve_type(object, context)
+          case object
+          when Issue
+            Types::IssueType
+          when MergeRequest
+            Types::MergeRequestType
+          else
+            raise "Unknown GraphQL type for #{object}"
+          end
+        end
+      end
+    end
+  end
+end

app/graphql/types/notes/position_type_enum.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+module Types
+  module Notes
+    class PositionTypeEnum < BaseEnum
+      graphql_name 'DiffPositionType'
+      description 'Type of file the position refers to'
+
+      value 'text'
+      value 'image'
+    end
+  end
+end

app/graphql/types/permission_types/note.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module Types
+  module PermissionTypes
+    class Note < BasePermissionType
+      graphql_name 'NotePermissions'
+
+      abilities :read_note, :create_note, :admin_note, :resolve_note, :award_emoji
+    end
+  end
+end

app/models/concerns/diff_positionable_note.rb

@@ -3,6 +3,7 @@ module DiffPositionableNote
   extend ActiveSupport::Concern
 
   included do
+    delegate :on_text?, :on_image?, to: :position, allow_nil: true
     before_validation :set_original_position, on: :create
     before_validation :update_position, on: :create, if: :on_text?
 
@@ -28,14 +29,6 @@ module DiffPositionableNote
     end
   end
 
-  def on_text?
-    position&.position_type == "text"
-  end
-
-  def on_image?
-    position&.position_type == "image"
-  end
-
   def supported?
     for_commit? || self.noteable.has_complete_diff_refs?
   end

app/models/discussion.rb

@@ -4,6 +4,7 @@
 #
 # A discussion of this type can be resolvable.
 class Discussion
+  include GlobalID::Identification
   include ResolvableDiscussion
 
   attr_reader :notes, :context_noteable
@@ -11,14 +12,19 @@ class Discussion
   delegate  :created_at,
             :project,
             :author,
-
             :noteable,
             :commit_id,
             :for_commit?,
             :for_merge_request?,
+            :to_ability_name,
+            :editable?,
 
             to: :first_note
 
+  def declarative_policy_delegate
+    first_note
+  end
+
   def project_id
     project&.id
   end

app/models/note.rb

@@ -342,7 +342,7 @@ class Note < ApplicationRecord
   end
 
   def to_ability_name
-    for_snippet? ? noteable.class.name.underscore : noteable_type.underscore
+    for_snippet? ? noteable.class.name.underscore : noteable_type.demodulize.underscore
   end
 
   def can_be_discussion_note?

changelogs/unreleased/bvl-comments-graphql.yml

@@ -0,0 +1,5 @@
+---
+title: Expose notes and discussions in GraphQL
+merge_request: 29212
+author:
+type: added

lib/gitlab/diff/position.rb

@@ -134,6 +134,14 @@ module Gitlab
         @line_code ||= diff_file(repository)&.line_code_for_position(self)
       end
 
+      def on_image?
+        position_type == 'image'
+      end
+
+      def on_text?
+        position_type == 'text'
+      end
+
       private
 
       def find_diff_file(repository)

spec/graphql/types/issue_type_spec.rb

@@ -7,6 +7,8 @@ describe GitlabSchema.types['Issue'] do
 
   it { expect(described_class).to require_graphql_authorizations(:read_issue) }
 
+  it { expect(described_class.interfaces).to include(Types::Notes::NoteableType.to_graphql) }
+
   it 'has specific fields' do
     %i[relative_position web_path web_url reference].each do |field_name|
       expect(described_class).to have_graphql_field(field_name)

spec/graphql/types/merge_request_type_spec.rb

@@ -5,6 +5,8 @@ describe GitlabSchema.types['MergeRequest'] do
 
   it { expect(described_class).to require_graphql_authorizations(:read_merge_request) }
 
+  it { expect(described_class.interfaces).to include(Types::Notes::NoteableType.to_graphql) }
+
   describe 'nested head pipeline' do
     it { expect(described_class).to have_graphql_field(:head_pipeline) }
   end

spec/graphql/types/notes/diff_position_type_spec.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+require 'spec_helper'
+
+describe GitlabSchema.types['DiffPosition'] do
+  it 'exposes the expected fields' do
+    expected_fields = [:head_sha, :base_sha, :start_sha, :file_path, :old_path,
+                       :new_path, :position_type, :old_line, :new_line, :x, :y,
+                       :width, :height]
+
+    is_expected.to have_graphql_field(*expected_fields)
+  end
+end

spec/graphql/types/notes/discussion_type_spec.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+require 'spec_helper'
+
+describe GitlabSchema.types['Discussion'] do
+  it { is_expected.to have_graphql_fields(:id, :created_at, :notes) }
+
+  it { is_expected.to require_graphql_authorizations(:read_note) }
+end

spec/graphql/types/notes/note_type_spec.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+require 'spec_helper'
+
+describe GitlabSchema.types['Note'] do
+  it 'exposes the expected fields' do
+    expected_fields = [:id, :project, :author, :body, :created_at,
+                       :updated_at, :discussion, :resolvable, :position, :user_permissions,
+                       :resolved_by, :resolved_at, :system]
+
+    is_expected.to have_graphql_fields(*expected_fields)
+  end
+
+  it { is_expected.to expose_permissions_using(Types::PermissionTypes::Note) }
+  it { is_expected.to require_graphql_authorizations(:read_note) }
+end

spec/graphql/types/notes/noteable_type_spec.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Types::Notes::NoteableType do
+  it { is_expected.to have_graphql_fields(:notes, :discussions) }
+
+  describe ".resolve_type" do
+    it 'knows the correct type for objects' do
+      expect(described_class.resolve_type(build(:issue), {})).to eq(Types::IssueType)
+      expect(described_class.resolve_type(build(:merge_request), {})).to eq(Types::MergeRequestType)
+    end
+  end
+end

spec/graphql/types/permission_types/note_spec.rb

@@ -0,0 +1,11 @@
+require 'spec_helper'
+
+describe GitlabSchema.types['NotePermissions'] do
+  it 'has the expected fields' do
+    expected_permissions = [
+      :read_note, :create_note, :admin_note, :resolve_note, :award_emoji
+    ]
+
+    is_expected.to have_graphql_fields(expected_permissions)
+  end
+end

spec/lib/gitlab/diff/position_spec.rb

@@ -46,6 +46,9 @@ describe Gitlab::Diff::Position do
       )
     end
 
+    it { is_expected.to be_on_text }
+    it { is_expected.not_to be_on_image }
+
     describe "#diff_file" do
       it "returns the correct diff file" do
         diff_file = subject.diff_file(project.repository)
@@ -91,6 +94,9 @@ describe Gitlab::Diff::Position do
       )
     end
 
+    it { is_expected.not_to be_on_text }
+    it { is_expected.to be_on_image }
+
     it "returns the correct diff file" do
       diff_file = subject.diff_file(project.repository)
 

spec/models/discussion_spec.rb

@@ -29,4 +29,12 @@ describe Discussion do
       ])
     end
   end
+
+  describe 'authorization' do
+    it 'delegates to the first note' do
+      policy = DeclarativePolicy.policy_for(instance_double(User, id: 1), subject)
+
+      expect(policy).to be_a(NotePolicy)
+    end
+  end
 end

spec/policies/note_policy_spec.rb

@@ -133,6 +133,25 @@ describe NotePolicy do
           end
         end
       end
+
+      context 'for discussions' do
+        let(:policy) { described_class.new(user, note.discussion) }
+
+        it 'allows the author to manage the discussion' do
+          expect(policy).to be_allowed(:admin_note)
+          expect(policy).to be_allowed(:resolve_note)
+          expect(policy).to be_allowed(:read_note)
+          expect(policy).to be_allowed(:award_emoji)
+        end
+
+        context 'when the user does not have access to the noteable' do
+          before do
+            noteable.update_attribute(:confidential, true)
+          end
+
+          it_behaves_like 'a discussion with a private noteable'
+        end
+      end
     end
   end
 end

spec/requests/api/graphql/project/issue/notes_spec.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'getting notes for an issue' do
+  include GraphqlHelpers
+
+  let(:noteable) { create(:issue) }
+  let(:noteable_data) { graphql_data['project']['issue'] }
+
+  def noteable_query(noteable_fields)
+    <<~QRY
+      {
+        project(fullPath: "#{noteable.project.full_path}") {
+          issue(iid: "#{noteable.iid}") {
+            #{noteable_fields}
+          }
+        }
+      }
+    QRY
+  end
+
+  it_behaves_like 'exposing regular notes on a noteable in GraphQL'
+end

spec/requests/api/graphql/project/merge_request/diff_notes_spec.rb

@@ -0,0 +1,90 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'getting notes for a merge request' do
+  include GraphqlHelpers
+
+  let(:noteable) { create(:merge_request) }
+
+  def noteable_query(noteable_fields)
+    <<~QRY
+      {
+        project(fullPath: "#{noteable.project.full_path}") {
+          id
+          mergeRequest(iid: "#{noteable.iid}") {
+            #{noteable_fields}
+          }
+        }
+      }
+    QRY
+  end
+  let(:noteable_data) { graphql_data['project']['mergeRequest'] }
+
+  it_behaves_like "exposing regular notes on a noteable in GraphQL"
+
+  context 'diff notes on a merge request' do
+    let(:project) { noteable.project }
+    let!(:note) { create(:diff_note_on_merge_request, noteable: noteable, project: project) }
+    let(:user) { note.author }
+
+    let(:query) do
+      noteable_query(
+        <<~NOTES
+        notes {
+          edges {
+            node {
+              #{all_graphql_fields_for('Note')}
+            }
+          }
+        }
+      NOTES
+      )
+    end
+
+    it_behaves_like 'a working graphql query' do
+      before do
+        post_graphql(query, current_user: user)
+      end
+    end
+
+    it 'includes the note' do
+      post_graphql(query, current_user: user)
+
+      expect(graphql_data['project']['mergeRequest']['notes']['edges'].last['node']['body'])
+        .to eq(note.note)
+    end
+
+    context 'the position of the diffnote' do
+      it 'includes a correct position' do
+        post_graphql(query, current_user: user)
+
+        note_data = noteable_data['notes']['edges'].last['node']
+
+        expect(note_data['position']['positionType']).to eq('text')
+        expect(note_data['position']['newLine']).to be_present
+        expect(note_data['position']['x']).not_to be_present
+        expect(note_data['position']['y']).not_to be_present
+        expect(note_data['position']['width']).not_to be_present
+        expect(note_data['position']['height']).not_to be_present
+      end
+
+      context 'with a note on an image' do
+        let(:note) { create(:image_diff_note_on_merge_request, noteable: noteable, project: project) }
+
+        it 'includes a correct position' do
+          post_graphql(query, current_user: user)
+
+          note_data = noteable_data['notes']['edges'].last['node']
+
+          expect(note_data['position']['positionType']).to eq('image')
+          expect(note_data['position']['x']).to be_present
+          expect(note_data['position']['y']).to be_present
+          expect(note_data['position']['width']).to be_present
+          expect(note_data['position']['height']).to be_present
+          expect(note_data['position']['newLine']).not_to be_present
+        end
+      end
+    end
+  end
+end

spec/support/shared_examples/graphql/notes_on_noteables_shared_examples.rb

@@ -0,0 +1,75 @@
+# frozen_string_literal: true
+require 'spec_helper'
+
+shared_context 'exposing regular notes on a noteable in GraphQL' do
+  include GraphqlHelpers
+
+  let(:note) do
+    create(:note,
+           noteable: noteable,
+           project: (noteable.project if noteable.respond_to?(:project)))
+  end
+  let(:user) { note.author }
+
+  context 'for regular notes' do
+    let(:query) do
+      note_fields = <<~NOTES
+      notes {
+        edges {
+          node {
+            #{all_graphql_fields_for('Note')}
+          }
+        }
+      }
+      NOTES
+
+      noteable_query(note_fields)
+    end
+
+    it_behaves_like 'a working graphql query' do
+      before do
+        post_graphql(query, current_user: user)
+      end
+    end
+
+    it 'includes the note' do
+      post_graphql(query, current_user: user)
+
+      expect(noteable_data['notes']['edges'].first['node']['body'])
+        .to eq(note.note)
+    end
+  end
+
+  context "for discussions" do
+    let(:query) do
+      discussion_fields = <<~DISCUSSIONS
+      discussions {
+        edges {
+          node {
+            #{all_graphql_fields_for('Discussion')}
+          }
+        }
+      }
+      DISCUSSIONS
+
+      noteable_query(discussion_fields)
+    end
+
+    let!(:reply) { create(:note, noteable: noteable, in_reply_to: note, discussion_id: note.discussion_id) }
+
+    it_behaves_like 'a working graphql query' do
+      before do
+        post_graphql(query, current_user: user)
+      end
+    end
+
+    it 'includes all discussion notes' do
+      post_graphql(query, current_user: user)
+
+      discussion = noteable_data['discussions']['edges'].first['node']
+      ids = discussion['notes']['edges'].map { |note_edge| note_edge['node']['id'] }
+
+      expect(ids).to eq([note.to_global_id.to_s, reply.to_global_id.to_s])
+    end
+  end
+end