Merge request and commit discussions API

6 files changed, 152 insertions, 49 deletions

diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb
index 7975f35ab1e..13c34e3473a 100644
--- a/lib/api/discussions.rb
+++ b/lib/api/discussions.rb
@@ -5,11 +5,12 @@ module API
 
     before { authenticate! }
 
-    NOTEABLE_TYPES = [Issue, Snippet].freeze
+    NOTEABLE_TYPES = [Issue, Snippet, MergeRequest, Commit].freeze
 
     NOTEABLE_TYPES.each do |noteable_type|
       parent_type = noteable_type.parent_class.to_s.underscore
       noteables_str = noteable_type.to_s.underscore.pluralize
+      noteables_path = noteable_type == Commit ? "repository/#{noteables_str}" : noteables_str
 
       params do
         requires :id, type: String, desc: "The ID of a #{parent_type}"
@@ -19,14 +20,12 @@ module API
           success Entities::Discussion
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           use :pagination
         end
-        get ":id/#{noteables_str}/:noteable_id/discussions" do
+        get ":id/#{noteables_path}/:noteable_id/discussions" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
-          break not_found!("Discussions") unless can?(current_user, noteable_read_ability_name(noteable), noteable)
-
           notes = noteable.notes
             .inc_relations_for_view
             .includes(:noteable)
@@ -43,13 +42,13 @@ module API
         end
         params do
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
         end
-        get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id" do
+        get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
-          if notes.empty? || !can?(current_user, noteable_read_ability_name(noteable), noteable)
+          if notes.empty?
             break not_found!("Discussion")
           end
 
@@ -62,19 +61,36 @@ module API
           success Entities::Discussion
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :body, type: String, desc: 'The content of a note'
           optional :created_at, type: String, desc: 'The creation date of the note'
+          optional :position, type: Hash do
+            requires :base_sha, type: String, desc: 'Base commit SHA in the source branch'
+            requires :start_sha, type: String, desc: 'SHA referencing commit in target branch'
+            requires :head_sha, type: String, desc: 'SHA referencing HEAD of this merge request'
+            requires :position_type, type: String, desc: 'Type of the position reference', values: %w(text image)
+            optional :new_path, type: String, desc: 'File path after change'
+            optional :new_line, type: Integer, desc: 'Line number after change'
+            optional :old_path, type: String, desc: 'File path before change'
+            optional :old_line, type: Integer, desc: 'Line number before change'
+            optional :width, type: Integer, desc: 'Width of the image'
+            optional :height, type: Integer, desc: 'Height of the image'
+            optional :x, type: Integer, desc: 'X coordinate in the image'
+            optional :y, type: Integer, desc: 'Y coordinate in the image'
+          end
         end
-        post ":id/#{noteables_str}/:noteable_id/discussions" do
+        post ":id/#{noteables_path}/:noteable_id/discussions" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
+          type = params[:position] ? 'DiffNote' : 'DiscussionNote'
+          id_key = noteable.is_a?(Commit) ? :commit_id : :noteable_id
 
           opts = {
             note: params[:body],
             created_at: params[:created_at],
-            type: 'DiscussionNote',
+            type: type,
             noteable_type: noteables_str.classify,
-            noteable_id: noteable.id
+            position: params[:position],
+            id_key => noteable.id
           }
 
           note = create_note(noteable, opts)
@@ -91,13 +107,13 @@ module API
         end
         params do
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
         end
-        get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes" do
+        get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
-          if notes.empty? || !can?(current_user, noteable_read_ability_name(noteable), noteable)
+          if notes.empty?
             break not_found!("Notes")
           end
 
@@ -108,12 +124,12 @@ module API
           success Entities::Note
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
           requires :body, type: String, desc: 'The content of a note'
           optional :created_at, type: String, desc: 'The creation date of the note'
         end
-        post ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes" do
+        post ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
@@ -139,11 +155,11 @@ module API
           success Entities::Note
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
           requires :note_id, type: Integer, desc: 'The ID of a note'
         end
-        get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
+        get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
           get_note(noteable, params[:note_id])
@@ -153,30 +169,52 @@ module API
           success Entities::Note
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
           requires :note_id, type: Integer, desc: 'The ID of a note'
-          requires :body, type: String, desc: 'The content of a note'
+          optional :body, type: String, desc: 'The content of a note'
+          optional :resolved, type: Boolean, desc: 'Mark note resolved/unresolved'
+          exactly_one_of :body, :resolved
         end
-        put ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
+        put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
-          update_note(noteable, params[:note_id])
+          if params[:resolved].nil?
+            update_note(noteable, params[:note_id])
+          else
+            resolve_note(noteable, params[:note_id], params[:resolved])
+          end
         end
 
         desc "Delete a comment in a #{noteable_type.to_s.downcase} discussion" do
           success Entities::Note
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
           requires :note_id, type: Integer, desc: 'The ID of a note'
         end
-        delete ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
+        delete ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
           delete_note(noteable, params[:note_id])
         end
+
+        if Noteable::RESOLVABLE_TYPES.include?(noteable_type.to_s)
+          desc "Resolve/unresolve an existing #{noteable_type.to_s.downcase} discussion" do
+            success Entities::Discussion
+          end
+          params do
+            requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
+            requires :discussion_id, type: String, desc: 'The ID of a discussion'
+            requires :resolved, type: Boolean, desc: 'Mark discussion resolved/unresolved'
+          end
+          put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do
+            noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
+
+            resolve_discussion(noteable, params[:discussion_id], params[:resolved])
+          end
+        end
       end
     end
 
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 8aad320e376..75d56b82424 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -286,6 +286,10 @@ module API
       end
     end
 
+    class DiffRefs < Grape::Entity
+      expose :base_sha, :head_sha, :start_sha
+    end
+
     class Commit < Grape::Entity
       expose :id, :short_id, :title, :created_at
       expose :parent_ids
@@ -601,6 +605,8 @@ module API
         merge_request.metrics&.pipeline
       end
 
+      expose :diff_refs, using: Entities::DiffRefs
+
       def build_available?(options)
         options[:project]&.feature_available?(:builds, options[:current_user])
       end
@@ -642,6 +648,11 @@ module API
       expose :id, :key, :created_at
     end
 
+    class DiffPosition < Grape::Entity
+      expose :base_sha, :start_sha, :head_sha, :old_path, :new_path,
+        :position_type
+    end
+
     class Note < Grape::Entity
       # Only Issue and MergeRequest have iid
       NOTEABLE_TYPES_WITH_IID = %w(Issue MergeRequest).freeze
@@ -655,6 +666,14 @@ module API
       expose :system?, as: :system
       expose :noteable_id, :noteable_type
 
+      expose :position, if: ->(note, options) { note.diff_note? } do |note|
+        note.position.to_h
+      end
+
+      expose :resolvable?, as: :resolvable
+      expose :resolved?, as: :resolved, if: ->(note, options) { note.resolvable? }
+      expose :resolved_by, using: Entities::UserBasic, if: ->(note, options) { note.resolvable? }
+
       # Avoid N+1 queries as much as possible
       expose(:noteable_iid) { |note| note.noteable.iid if NOTEABLE_TYPES_WITH_IID.include?(note.noteable_type) }
     end
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index b8657cd7ee4..2ed331d4fd2 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -171,6 +171,10 @@ module API
       MergeRequestsFinder.new(current_user, project_id: user_project.id).find_by!(iid: iid)
     end
 
+    def find_project_commit(id)
+      user_project.commit_by(oid: id)
+    end
+
     def find_project_snippet(id)
       finder_params = { project: user_project }
       SnippetsFinder.new(current_user, finder_params).find(id)
diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb
index b74b8149834..b4bfb677d72 100644
--- a/lib/api/helpers/notes_helpers.rb
+++ b/lib/api/helpers/notes_helpers.rb
@@ -21,6 +21,23 @@ module API
         end
       end
 
+      def resolve_note(noteable, note_id, resolved)
+        note = noteable.notes.find(note_id)
+
+        authorize! :resolve_note, note
+
+        bad_request!("Note is not resolvable") unless note.resolvable?
+
+        if resolved
+          parent = noteable_parent(noteable)
+          ::Notes::ResolveService.new(parent, current_user).execute(note)
+        else
+          note.unresolve!
+        end
+
+        present note, with: Entities::Note
+      end
+
       def delete_note(noteable, note_id)
         note = noteable.notes.find(note_id)
 
@@ -35,7 +52,7 @@ module API
 
       def get_note(noteable, note_id)
         note = noteable.notes.with_metadata.find(params[:note_id])
-        can_read_note = can?(current_user, noteable_read_ability_name(noteable), noteable) && !note.cross_reference_not_visible_for?(current_user)
+        can_read_note = !note.cross_reference_not_visible_for?(current_user)
 
         if can_read_note
           present note, with: Entities::Note
@@ -49,7 +66,20 @@ module API
       end
 
       def find_noteable(parent, noteables_str, noteable_id)
-        public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend
+        noteable = public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend
+
+        readable =
+          if noteable.is_a?(Commit)
+            # for commits there is not :read_commit policy, check if user
+            # has :read_note permission on the commit's project
+            can?(current_user, :read_note, user_project)
+          else
+            can?(current_user, noteable_read_ability_name(noteable), noteable)
+          end
+
+        return not_found!(noteables_str) unless readable
+
+        noteable
       end
 
       def noteable_parent(noteable)
@@ -57,11 +87,8 @@ module API
       end
 
       def create_note(noteable, opts)
-        noteables_str = noteable.model_name.to_s.underscore.pluralize
-
-        return not_found!(noteables_str) unless can?(current_user, noteable_read_ability_name(noteable), noteable)
-
-        authorize! :create_note, noteable
+        policy_object = noteable.is_a?(Commit) ? user_project : noteable
+        authorize!(:create_note, policy_object)
 
         parent = noteable_parent(noteable)
 
@@ -73,6 +100,21 @@ module API
         project = parent if parent.is_a?(Project)
         ::Notes::CreateService.new(project, current_user, opts).execute
       end
+
+      def resolve_discussion(noteable, discussion_id, resolved)
+        discussion = noteable.find_discussion(discussion_id)
+
+        forbidden! unless discussion.can_resolve?(current_user)
+
+        if resolved
+          parent = noteable_parent(noteable)
+          ::Discussions::ResolveService.new(parent, current_user, merge_request: noteable).execute(discussion)
+        else
+          discussion.unresolve!
+        end
+
+        present discussion, with: Entities::Discussion
+      end
     end
   end
 end
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index 69f1df6b341..39923e6d5b5 100644
--- a/lib/api/notes.rb
+++ b/lib/api/notes.rb
@@ -31,23 +31,19 @@ module API
         get ":id/#{noteables_str}/:noteable_id/notes" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
-          if can?(current_user, noteable_read_ability_name(noteable), noteable)
-            # We exclude notes that are cross-references and that cannot be viewed
-            # by the current user. By doing this exclusion at this level and not
-            # at the DB query level (which we cannot in that case), the current
-            # page can have less elements than :per_page even if
-            # there's more than one page.
-            raw_notes = noteable.notes.with_metadata.reorder(params[:order_by] => params[:sort])
-            notes =
-              # paginate() only works with a relation. This could lead to a
-              # mismatch between the pagination headers info and the actual notes
-              # array returned, but this is really a edge-case.
-              paginate(raw_notes)
-              .reject { |n| n.cross_reference_not_visible_for?(current_user) }
-            present notes, with: Entities::Note
-          else
-            not_found!("Notes")
-          end
+          # We exclude notes that are cross-references and that cannot be viewed
+          # by the current user. By doing this exclusion at this level and not
+          # at the DB query level (which we cannot in that case), the current
+          # page can have less elements than :per_page even if
+          # there's more than one page.
+          raw_notes = noteable.notes.with_metadata.reorder(params[:order_by] => params[:sort])
+          notes =
+            # paginate() only works with a relation. This could lead to a
+            # mismatch between the pagination headers info and the actual notes
+            # array returned, but this is really a edge-case.
+            paginate(raw_notes)
+            .reject { |n| n.cross_reference_not_visible_for?(current_user) }
+          present notes, with: Entities::Note
         end
 
         desc "Get a single #{noteable_type.to_s.downcase} note" do
diff --git a/lib/gitlab/diff/position.rb b/lib/gitlab/diff/position.rb
index 690b27cde81..978962ab2eb 100644
--- a/lib/gitlab/diff/position.rb
+++ b/lib/gitlab/diff/position.rb
@@ -12,6 +12,10 @@ module Gitlab
                :head_sha,
                :old_line,
                :new_line,
+               :width,
+               :height,
+               :x,
+               :y,
                :position_type, to: :formatter
 
       # A position can belong to a text line or to an image coordinate