GraphQL support for Notes created in discussions62826-graphql-note-mutations

A new `discussion_id` argument on the `createNote` mutation allows
people to create a note within that discussion.

The ability to lazy-load Discussions has been added, so
GraphQL.object_from_id can treat Discussions the same as AR objects and
batch load them.

https://gitlab.com/gitlab-org/gitlab-ce/issues/62826
https://gitlab.com/gitlab-org/gitlab-ee/issues/9489

11 files changed, 160 insertions, 2 deletions

diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb
index 152ebb930e2..7edd14e48f7 100644
--- a/app/graphql/gitlab_schema.rb
+++ b/app/graphql/gitlab_schema.rb
@@ -66,6 +66,8 @@ class GitlabSchema < GraphQL::Schema
 
       if gid.model_class < ApplicationRecord
         Gitlab::Graphql::Loaders::BatchModelLoader.new(gid.model_class, gid.model_id).find
+      elsif gid.model_class.respond_to?(:lazy_find)
+        gid.model_class.lazy_find(gid.model_id)
       else
         gid.find
       end
diff --git a/app/graphql/mutations/notes/create/note.rb b/app/graphql/mutations/notes/create/note.rb
index 3c571a4538f..5236e48026e 100644
--- a/app/graphql/mutations/notes/create/note.rb
+++ b/app/graphql/mutations/notes/create/note.rb
@@ -5,6 +5,35 @@ module Mutations
     module Create
       class Note < Base
         graphql_name 'CreateNote'
+
+        argument :discussion_id,
+                  GraphQL::ID_TYPE,
+                  required: false,
+                  description: 'The global id of the discussion this note is in reply to'
+
+        private
+
+        def create_note_params(noteable, args)
+          discussion_id = nil
+
+          if args[:discussion_id]
+            discussion = GitlabSchema.object_from_id(args[:discussion_id])
+            authorize_discussion!(discussion)
+
+            discussion_id = discussion.id
+          end
+
+          super(noteable, args).merge({
+            in_reply_to_discussion_id: discussion_id
+          })
+        end
+
+        def authorize_discussion!(discussion)
+          unless Ability.allowed?(current_user, :read_note, discussion, scope: :user)
+            raise Gitlab::Graphql::Errors::ResourceNotAvailable,
+              "The discussion does not exist or you don't have permission to perform this action"
+          end
+        end
       end
     end
   end
diff --git a/app/graphql/types/notes/discussion_type.rb b/app/graphql/types/notes/discussion_type.rb
index c4691942f2d..a3fb28298f6 100644
--- a/app/graphql/types/notes/discussion_type.rb
+++ b/app/graphql/types/notes/discussion_type.rb
@@ -8,8 +8,16 @@ module Types
       authorize :read_note
 
       field :id, GraphQL::ID_TYPE, null: false
+      field :reply_id, GraphQL::ID_TYPE, null: false, description: 'The ID used to reply to this discussion'
       field :created_at, Types::TimeType, null: false
       field :notes, Types::Notes::NoteType.connection_type, null: false, description: "All notes in the discussion"
+
+      # The gem we use to generate Global IDs is hard-coded to work with
+      # `id` properties. To generate a GID for the `reply_id` property,
+      # we must use the ::Gitlab::GlobalId module.
+      def reply_id
+        ::Gitlab::GlobalId.build(object, id: object.reply_id)
+      end
     end
   end
 end
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index ae13cdfd85f..dd896f77084 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -38,6 +38,17 @@ class Discussion
     grouped_notes.values.map { |notes| build(notes, context_noteable) }
   end
 
+  def self.lazy_find(discussion_id)
+    BatchLoader.for(discussion_id).batch do |discussion_ids, loader|
+      results = Note.where(discussion_id: discussion_ids).fresh.to_a.group_by(&:discussion_id)
+      results.each do |discussion_id, notes|
+        next if notes.empty?
+
+        loader.call(discussion_id, Discussion.build(notes))
+      end
+    end
+  end
+
   # Returns an alphanumeric discussion ID based on `build_discussion_id`
   def self.discussion_id(note)
     Digest::SHA1.hexdigest(build_discussion_id(note).join("-"))
diff --git a/app/models/note.rb b/app/models/note.rb
index 4e9ea146485..5c31cff9816 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -158,6 +158,8 @@ class Note < ApplicationRecord
       Discussion.build_collection(all.includes(:noteable).fresh, context_noteable)
     end
 
+    # Note: Where possible consider using Discussion#lazy_find to return
+    # Discussions in order to benefit from having records batch loaded.
     def find_discussion(discussion_id)
       notes = where(discussion_id: discussion_id).fresh.to_a
 
diff --git a/lib/gitlab/global_id.rb b/lib/gitlab/global_id.rb
new file mode 100644
index 00000000000..cc82b6c5897
--- /dev/null
+++ b/lib/gitlab/global_id.rb
@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module GlobalId
+    def self.build(object = nil, model_name: nil, id: nil, params: nil)
+      if object
+        model_name ||= object.class.name
+        id ||= object.id
+      end
+
+      ::URI::GID.build(app: GlobalID.app, model_name: model_name, model_id: id, params: params)
+    end
+  end
+end
diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb
index 93b86b9b812..dec6b23d72a 100644
--- a/spec/graphql/gitlab_schema_spec.rb
+++ b/spec/graphql/gitlab_schema_spec.rb
@@ -143,6 +143,26 @@ describe GitlabSchema do
       end
     end
 
+    context 'for classes that are not ActiveRecord subclasses and have implemented .lazy_find' do
+      it 'returns the correct record' do
+        note = create(:discussion_note_on_merge_request)
+
+        result = described_class.object_from_id(note.to_global_id)
+
+        expect(result.__sync).to eq(note)
+      end
+
+      it 'batchloads the queries' do
+        note1 = create(:discussion_note_on_merge_request)
+        note2 = create(:discussion_note_on_merge_request)
+
+        expect do
+          [described_class.object_from_id(note1.to_global_id),
+           described_class.object_from_id(note2.to_global_id)].map(&:__sync)
+        end.not_to exceed_query_limit(1)
+      end
+    end
+
     context 'for other classes' do
       # We cannot use an anonymous class here as `GlobalID` expects `.name` not
       # to return `nil`
diff --git a/spec/graphql/types/notes/discussion_type_spec.rb b/spec/graphql/types/notes/discussion_type_spec.rb
index 2a1eb0efd35..ba7fc961212 100644
--- a/spec/graphql/types/notes/discussion_type_spec.rb
+++ b/spec/graphql/types/notes/discussion_type_spec.rb
@@ -2,7 +2,7 @@
 require 'spec_helper'
 
 describe GitlabSchema.types['Discussion'] do
-  it { is_expected.to have_graphql_fields(:id, :created_at, :notes) }
+  it { is_expected.to have_graphql_fields(:id, :created_at, :notes, :reply_id) }
 
   it { is_expected.to require_graphql_authorizations(:read_note) }
 end
diff --git a/spec/lib/gitlab/global_id_spec.rb b/spec/lib/gitlab/global_id_spec.rb
new file mode 100644
index 00000000000..d35b5da0b75
--- /dev/null
+++ b/spec/lib/gitlab/global_id_spec.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::GlobalId do
+  describe '.build' do
+    set(:object) { create(:issue) }
+
+    it 'returns a standard GlobalId if only object is passed' do
+      expect(described_class.build(object).to_s).to eq(object.to_global_id.to_s)
+    end
+
+    it 'returns a GlobalId from params' do
+      expect(described_class.build(model_name: 'MyModel', id: 'myid').to_s).to eq(
+        'gid://gitlab/MyModel/myid'
+      )
+    end
+
+    it 'returns a GlobalId from object and `id` param' do
+      expect(described_class.build(object, id: 'myid').to_s).to eq(
+        'gid://gitlab/Issue/myid'
+      )
+    end
+
+    it 'returns a GlobalId from object and `model_name` param' do
+      expect(described_class.build(object, model_name: 'MyModel').to_s).to eq(
+        "gid://gitlab/MyModel/#{object.id}"
+      )
+    end
+
+    it 'returns an error if model_name and id are not able to be determined' do
+      expect { described_class.build(id: 'myid') }.to raise_error(URI::InvalidComponentError)
+      expect { described_class.build(model_name: 'MyModel') }.to raise_error(URI::InvalidComponentError)
+      expect { described_class.build }.to raise_error(URI::InvalidComponentError)
+    end
+  end
+end
diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb
index 22d4dab0617..950bdec4d00 100644
--- a/spec/models/discussion_spec.rb
+++ b/spec/models/discussion_spec.rb
@@ -10,6 +10,20 @@ describe Discussion do
   let(:second_note) { create(:diff_note_on_merge_request, in_reply_to: first_note) }
   let(:third_note) { create(:diff_note_on_merge_request) }
 
+  describe '.lazy_find' do
+    let!(:note1) { create(:discussion_note_on_merge_request).to_discussion }
+    let!(:note2) { create(:discussion_note_on_merge_request, in_reply_to: note1).to_discussion }
+
+    subject { [note1, note2].map { |note| described_class.lazy_find(note.discussion_id) } }
+
+    it 'batches requests' do
+      expect do
+        [described_class.lazy_find(note1.id),
+         described_class.lazy_find(note2.id)].map(&:__sync)
+      end.not_to exceed_query_limit(1)
+    end
+  end
+
   describe '.build' do
     it 'returns a discussion of the right type' do
       discussion = described_class.build([first_note, second_note], merge_request)
diff --git a/spec/requests/api/graphql/mutations/notes/create/note_spec.rb b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb
index 17aed69b650..14aaa430ac9 100644
--- a/spec/requests/api/graphql/mutations/notes/create/note_spec.rb
+++ b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb
@@ -7,10 +7,12 @@ describe 'Adding a Note' do
 
   set(:current_user) { create(:user) }
   let(:noteable) { create(:merge_request, source_project: project, target_project: project) }
-  let(:project) { create(:project, :repository) }
+  let(:project) { create(:project) }
+  let(:discussion) { nil }
   let(:mutation) do
     variables = {
       noteable_id: GitlabSchema.id_from_object(noteable).to_s,
+      discussion_id: (GitlabSchema.id_from_object(discussion).to_s if discussion),
       body: 'Body text'
     }
 
@@ -39,5 +41,24 @@ describe 'Adding a Note' do
 
       expect(mutation_response['note']['body']).to eq('Body text')
     end
+
+    describe 'creating Notes in reply to a discussion' do
+      context 'when the user does not have permission to create notes on the discussion' do
+        let(:discussion) { create(:discussion_note).to_discussion }
+
+        it_behaves_like 'a mutation that returns top-level errors',
+                  errors: ["The discussion does not exist or you don't have permission to perform this action"]
+      end
+
+      context 'when the user has permission to create notes on the discussion' do
+        let(:discussion) { create(:discussion_note, project: project).to_discussion }
+
+        it 'creates a Note in a discussion' do
+          post_graphql_mutation(mutation, current_user: current_user)
+
+          expect(mutation_response['note']['discussion']['id']).to eq(discussion.to_global_id.to_s)
+        end
+      end
+    end
   end
 end