Merge branch 'issue_1984' into 'master'

Batch comments on merge requests

See merge request gitlab-org/gitlab-ee!4213

config/routes/project.rb

@@ -159,6 +159,17 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         resources :approver_groups, only: :destroy
         ## EE-specific
 
+        ## EE-specific
+        scope module: :merge_requests do
+          resources :drafts, only: [:index, :update, :create, :destroy] do
+            collection do
+              post :publish
+              delete :discard
+            end
+          end
+        end
+        ## EE-specific
+
         resources :discussions, only: [:show], constraints: { id: /\h{40}/ } do
           member do
             post :resolve

db/schema.rb

@@ -906,6 +906,21 @@ ActiveRecord::Schema.define(version: 20181001172651) do
   add_index "deployments", ["environment_id", "iid", "project_id"], name: "index_deployments_on_environment_id_and_iid_and_project_id", using: :btree
   add_index "deployments", ["project_id", "iid"], name: "index_deployments_on_project_id_and_iid", unique: true, using: :btree
 
+  create_table "draft_notes", id: :bigserial, force: :cascade do |t|
+    t.integer "merge_request_id", null: false
+    t.integer "author_id", null: false
+    t.boolean "resolve_discussion", default: false, null: false
+    t.string "discussion_id"
+    t.text "note", null: false
+    t.text "position"
+    t.text "original_position"
+    t.text "change_position"
+  end
+
+  add_index "draft_notes", ["author_id"], name: "index_draft_notes_on_author_id", using: :btree
+  add_index "draft_notes", ["discussion_id"], name: "index_draft_notes_on_discussion_id", using: :btree
+  add_index "draft_notes", ["merge_request_id"], name: "index_draft_notes_on_merge_request_id", using: :btree
+
   create_table "emails", force: :cascade do |t|
     t.integer "user_id", null: false
     t.string "email", null: false
@@ -3160,6 +3175,8 @@ ActiveRecord::Schema.define(version: 20181001172651) do
   add_foreign_key "container_repositories", "projects"
   add_foreign_key "deploy_keys_projects", "projects", name: "fk_58a901ca7e", on_delete: :cascade
   add_foreign_key "deployments", "projects", name: "fk_b9a3851b82", on_delete: :cascade
+  add_foreign_key "draft_notes", "merge_requests", on_delete: :cascade
+  add_foreign_key "draft_notes", "users", column: "author_id", on_delete: :cascade
   add_foreign_key "environments", "projects", name: "fk_d1c8c1da6a", on_delete: :cascade
   add_foreign_key "epic_issues", "epics", on_delete: :cascade
   add_foreign_key "epic_issues", "issues", on_delete: :cascade

ee/app/controllers/projects/merge_requests/drafts_controller.rb

+# frozen_string_literal: true
+
+class Projects::MergeRequests::DraftsController < Projects::MergeRequests::ApplicationController
+  include Gitlab::Utils::StrongMemoize
+
+  respond_to :json
+
+  before_action :check_draft_notes_available!, except: [:index]
+  before_action :authorize_create_draft!, only: [:create]
+  before_action :authorize_admin_draft!, only: [:update, :destroy]
+
+  def index
+    drafts = prepare_notes_for_rendering(draft_notes)
+    render json: DraftNoteSerializer.new(current_user: current_user).represent(drafts)
+  end
+
+  def create
+    create_params = draft_note_params.merge(in_reply_to_discussion_id: params[:in_reply_to_discussion_id])
+    create_service = DraftNotes::CreateService.new(merge_request, current_user, create_params)
+
+    draft_note = create_service.execute
+
+    prepare_notes_for_rendering(draft_note)
+
+    render json: DraftNoteSerializer.new(current_user: current_user).represent(draft_note)
+  end
+
+  def update
+    draft_note.update!(draft_note_params)
+
+    prepare_notes_for_rendering(draft_note)
+
+    render json: DraftNoteSerializer.new(current_user: current_user).represent(draft_note)
+  end
+
+  def destroy
+    draft_note.destroy!
+
+    head :ok
+  end
+
+  def publish
+    DraftNotes::PublishService.new(merge_request, current_user).execute(params[:id])
+
+    head :ok
+  end
+
+  def discard
+    draft_notes.delete_all
+
+    head :ok
+  end
+
+  private
+
+  def draft_note
+    strong_memoize(:draft_note) do
+      draft_notes.try(:find, params[:id])
+    end
+  end
+
+  def draft_notes
+    return unless current_user && draft_notes_available?
+
+    strong_memoize(:draft_notes) do
+      merge_request.draft_notes.authored_by(current_user)
+    end
+  end
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def merge_request
+    @merge_request ||= MergeRequestsFinder.new(current_user, project_id: @project.id).find_by!(iid: params[:merge_request_id])
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+
+  def draft_note_params
+    params.require(:draft_note).permit(
+      :note,
+      :position,
+      :resolve_discussion
+    )
+  end
+
+  def prepare_notes_for_rendering(notes)
+    return [] unless notes
+
+    notes = Array.wrap(notes)
+
+    # Preload author and access-level information
+    DraftNote.preload_author(notes)
+    user_ids = notes.map(&:author_id)
+    project.team.max_member_access_for_user_ids(user_ids)
+
+    notes.map(&method(:render_draft_note))
+  end
+
+  def render_draft_note(note)
+    params = { quick_actions_target_id: merge_request.id, quick_actions_target_type: 'MergeRequest', text: note.note }
+    result = PreviewMarkdownService.new(@project, current_user, params).execute
+    markdown_params = { markdown_engine: result[:markdown_engine], issuable_state_filter_enabled: true }
+
+    note.rendered_note = view_context.markdown(result[:text], markdown_params)
+    note.users_referenced = result[:users]
+    note.commands_changes = view_context.markdown(result[:commands])
+
+    note
+  end
+
+  def draft_notes_available?
+    @project.feature_available?(:batch_comments) &&
+      ::Feature.enabled?(:batch_comments, current_user, default_enabled: false)
+  end
+
+  def authorize_admin_draft!
+    access_denied! unless can?(current_user, :admin_note, draft_note)
+  end
+
+  def authorize_create_draft!
+    access_denied! unless can?(current_user, :create_note, merge_request)
+  end
+
+  def check_draft_notes_available!
+    return if draft_notes_available?
+
+    access_denied!('Draft Notes are not available with your current License')
+  end
+end

ee/app/models/draft_note.rb

+# frozen_string_literal: true
+class DraftNote < ActiveRecord::Base
+  include DiffPositionableNote
+  include Gitlab::Utils::StrongMemoize
+
+  PUBLISH_ATTRS = %i(noteable_id noteable_type type note).freeze
+  DIFF_ATTRS = %i(position original_position change_position).freeze
+
+  # Attribute used to store quick actions changes and users referenced.
+  attr_accessor :commands_changes
+  attr_accessor :users_referenced
+
+  # Text with quick actions filtered out
+  attr_accessor :rendered_note
+
+  belongs_to :author, class_name: 'User'
+  belongs_to :merge_request
+
+  validates :merge_request_id, presence: true
+  validates :author_id, presence: true, uniqueness: { scope: [:merge_request_id, :discussion_id] }, if: :discussion_id?
+  validates :discussion_id, allow_nil: true, format: { with: /\A\h{40}\z/ }
+
+  scope :authored_by, ->(u) { where(author_id: u.id) }
+
+  def project
+    merge_request.target_project
+  end
+
+  # noteable_id and noteable_type methods
+  # are used to generate discussion_id on Discussion.discussion_id
+  def noteable_id
+    merge_request_id
+  end
+
+  def noteable
+    merge_request
+  end
+
+  def noteable_type
+    "MergeRequest"
+  end
+
+  def for_commit?
+    false
+  end
+
+  def resolvable?
+    false
+  end
+
+  def emoji_awardable?
+    false
+  end
+
+  def on_diff?
+    position&.complete?
+  end
+
+  def type
+    return 'DiffNote' if  on_diff?
+    return 'DiscussionNote' if discussion_id.present?
+
+    'Note'
+  end
+
+  def references
+    {
+      users: users_referenced,
+      commands: commands_changes
+    }
+  end
+
+  def line_code
+    @line_code ||= original_position&.line_code(project.repository)
+  end
+
+  def file_hash
+    return unless diff_file
+
+    Digest::SHA1.hexdigest(diff_file.file_path)
+  end
+
+  def publish_params
+    attrs = PUBLISH_ATTRS.dup
+    attrs.concat(DIFF_ATTRS) if on_diff?
+    params = slice(*attrs)
+    params[:in_reply_to_discussion_id] = discussion_id if discussion_id.present?
+
+    params
+  end
+
+  def self.preload_author(draft_notes)
+    ActiveRecord::Associations::Preloader.new.preload(draft_notes, { author: :status })
+  end
+
+  private
+
+  def diff_file
+    strong_memoize(:diff_file) do
+      original_position&.diff_file(project.repository)
+    end
+  end
+end

ee/app/models/ee/merge_request.rb

@@ -9,6 +9,7 @@ module EE
       has_many :approved_by_users, through: :approvals, source: :user
       has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
+      has_many :draft_notes
 
       # codeclimate_artifact is deprecated and replaced with code_quality_artifact (#5779)
       delegate :codeclimate_artifact, to: :head_pipeline, prefix: :head, allow_nil: true

ee/app/models/license.rb

@@ -68,6 +68,7 @@ class License < ActiveRecord::Base
     packages
     code_owner_as_approver_suggestion
     feature_flags
+    batch_comments
   ].freeze
 
   EEU_FEATURES = EEP_FEATURES + %i[

ee/app/policies/draft_note_policy.rb

+# frozen_string_literal: true
+
+class DraftNotePolicy < BasePolicy
+  delegate { @subject.merge_request }
+
+  condition(:is_author) { @user && @subject.author == @user }
+
+  rule { is_author }.policy do
+    enable :read_note
+    enable :admin_note
+    enable :resolve_note
+  end
+end

ee/app/serializers/draft_note_entity.rb

+# frozen_string_literal: true
+class DraftNoteEntity < Grape::Entity
+  include RequestAwareEntity
+
+  expose :id
+  expose :author, using: NoteUserEntity
+  expose :merge_request_id
+  expose :position, if: -> (note, _) { note.on_diff? }
+  expose :line_code
+  expose :file_hash
+  expose :note
+  expose :rendered_note, as: :note_html
+  expose :references
+  expose :discussion_id
+  expose :resolve_discussion
+  expose :noteable_type
+
+  expose :current_user do
+    expose :can_edit do |note|
+      can?(current_user, :admin_note, note)
+    end
+
+    expose :can_award_emoji do |note|
+      note.emoji_awardable?
+    end
+
+    expose :can_resolve do |note|
+      note.resolvable? && can?(current_user, :resolve_note, note)
+    end
+  end
+
+  private
+
+  def current_user
+    request.current_user
+  end
+end

ee/app/serializers/draft_note_serializer.rb

+# frozen_string_literal: true
+class DraftNoteSerializer < BaseSerializer
+  entity DraftNoteEntity
+end

ee/app/services/draft_notes/base_service.rb

+# frozen_string_literal: true
+
+module DraftNotes
+  class BaseService
+    attr_accessor :merge_request, :current_user, :params
+
+    def initialize(merge_request, current_user, params = nil)
+      @merge_request, @current_user, @params = merge_request, current_user, params.dup
+    end
+
+    private
+
+    def project
+      merge_request.target_project
+    end
+  end
+end

ee/app/services/draft_notes/create_service.rb

+# frozen_string_literal: true
+
+module DraftNotes
+  class CreateService < DraftNotes::BaseService
+    attr_accessor :in_draft_mode, :in_reply_to_discussion_id
+
+    def initialize(merge_request, current_user, params = nil)
+      @in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id)
+      super
+    end
+
+    def execute
+      if in_reply_to_discussion_id.present?
+        unless discussion
+          return base_error('Discussion to reply to cannot be found')
+        end
+
+        params[:discussion_id] = discussion.reply_id
+      end
+
+      if params[:resolve_discussion] && !can_resolve_discussion?
+        return base_error('User is not allowed to resolve discussion')
+      end
+
+      draft_note = DraftNote.new(params)
+      draft_note.merge_request = merge_request
+      draft_note.author = current_user
+      draft_note.save
+
+      draft_note
+    end
+
+    private
+
+    def base_error(text)
+      DraftNote.new.tap do |draft|
+        draft.errors.add(:base, text)
+      end
+    end
+
+    def discussion
+      @discussion ||= merge_request.notes.find_discussion(in_reply_to_discussion_id)
+    end
+
+    def can_resolve_discussion?
+      note = discussion&.notes&.first
+      return false unless note
+
+      current_user && Ability.allowed?(current_user, :resolve_note, note)
+    end
+  end
+end

ee/app/services/draft_notes/publish_service.rb

+# frozen_string_literal: true
+
+module DraftNotes
+  class PublishService < DraftNotes::BaseService
+    def execute(draft_id = nil)
+      if draft_id
+        publish_draft_note(draft_id)
+      else
+        publish_draft_notes
+      end
+    end
+
+    private
+
+    def publish_draft_note(draft_id)
+      draft = DraftNote.find(draft_id)
+
+      create_note_from_draft(draft)
+      draft.delete
+
+      MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
+    end
+
+    def publish_draft_notes
+      draft_notes.each(&method(:create_note_from_draft))
+      draft_notes.delete_all
+
+      MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
+    end
+
+    def create_note_from_draft(draft)
+      note = Notes::CreateService.new(draft.project, draft.author, draft.publish_params).execute
+      set_discussion_resolve_status(note, draft)
+    end
+
+    def set_discussion_resolve_status(note, draft_note)
+      return unless draft_note.discussion_id.present?
+
+      discussion = note.discussion
+
+      if draft_note.resolve_discussion && discussion.can_resolve?(current_user)
+        discussion.resolve!(current_user)
+      else
+        discussion.unresolve!
+      end
+    end
+
+    def draft_notes
+      @draft_notes ||= merge_request.draft_notes.authored_by(current_user)
+    end
+  end
+end

ee/changelogs/unreleased/issue_1984.yml

+---
+title: Batch comments on merge requests
+merge_request: 4213
+author:
+type: added

ee/db/migrate/20180917145556_create_draft_notes.rb

+# frozen_string_literal: true
+class CreateDraftNotes < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table :draft_notes, id: :bigserial do |t|
+      t.references :merge_request, null: false, index: true, foreign_key: { on_delete: :cascade }
+      t.foreign_key :users, null: false, column: :author_id, on_delete: :cascade
+      t.integer :author_id, null: false, index: true
+      t.boolean :resolve_discussion, default: false, null: false
+      t.string :discussion_id
+      t.text :note, null: false
+      t.text :position
+      t.text :original_position
+      t.text :change_position
+    end
+
+    add_index :draft_notes, :discussion_id
+  end
+end

ee/spec/factories/draft_note.rb

+# frozen_string_literal: true
+FactoryBot.define do
+  factory :draft_note do
+    note { generate(:title) }
+    association :author, factory: :user
+    association :merge_request, factory: :merge_request
+
+    factory :draft_note_on_text_diff do
+      transient do
+        line_number 14
+        diff_refs { merge_request.try(:diff_refs) }
+      end
+
+      position do
+        Gitlab::Diff::Position.new(
+          old_path: "files/ruby/popen.rb",
+          new_path: "files/ruby/popen.rb",
+          old_line: nil,
+          new_line: line_number,
+          diff_refs: diff_refs
+        )
+      end
+    end
+
+    factory :draft_note_on_discussion, traits: [:on_discussion]
+
+    trait :on_discussion do
+      discussion_id { create(:discussion_note_on_merge_request, noteable: merge_request, project: project).discussion_id }
+    end
+  end
+end

ee/spec/lib/gitlab/import_export/all_models.yml

@@ -9,6 +9,7 @@ merge_requests:
 - approvers
 - approver_groups
 - approved_by_users
+- draft_notes
 pipelines:
 - source_pipeline
 - sourced_pipelines

ee/spec/services/draft_notes/create_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe DraftNotes::CreateService do
+  let(:merge_request) { create(:merge_request) }
+  let(:project) { merge_request.target_project }
+  let(:user) { merge_request.author }
+
+  def create_draft(params)
+    described_class.new(merge_request, user, params).execute
+  end
+
+  it 'creates a simple draft note' do
+    draft = create_draft(note: 'This is a test')
+
+    expect(draft).to be_an_instance_of(DraftNote)
+    expect(draft.note).to eq('This is a test')
+    expect(draft.author).to eq(user)
+    expect(draft.project).to eq(merge_request.target_project)
+    expect(draft.discussion_id).to be_nil
+  end
+
+  it 'cannot resolve when there is nothing to resolve' do
+    draft = create_draft(note: 'Not a reply!', resolve_discussion: true)
+
+    expect(draft.errors[:base]).to include('User is not allowed to resolve discussion')
+    expect(draft).not_to be_persisted
+  end
+
+  context 'in a discussion' do
+    it 'creates a draft note with discussion_id' do
+      discussion = create(:discussion_note_on_merge_request, noteable: merge_request, project: project).discussion
+
+      draft = create_draft(note: 'A reply!', in_reply_to_discussion_id: discussion.reply_id)
+
+      expect(draft.note).to eq('A reply!')
+      expect(draft.discussion_id).to eq(discussion.reply_id)
+      expect(draft.resolve_discussion).to be_falsey
+    end
+
+    it 'creates a draft that resolves the discussion' do
+      discussion = create(:discussion_note_on_merge_request, noteable: merge_request, project: project).discussion
+
+      draft = create_draft(note: 'A reply!', in_reply_to_discussion_id: discussion.reply_id, resolve_discussion: true)
+
+      expect(draft.note).to eq('A reply!')
+      expect(draft.discussion_id).to eq(discussion.reply_id)
+      expect(draft.resolve_discussion).to be true
+    end
+  end
+
+  it 'creates a draft note with a position in a diff' do
+    diff_refs = project.commit(RepoHelpers.sample_commit.id).try(:diff_refs)
+
+    position = Gitlab::Diff::Position.new(
+      old_path: "files/ruby/popen.rb",
+      new_path: "files/ruby/popen.rb",
+      old_line: nil,
+      new_line: 14,
+      diff_refs: diff_refs
+    )
+
+    draft = create_draft(note: 'Comment on diff', position: position.to_json)
+
+    expect(draft.note).to eq('Comment on diff')
+    expect(draft.original_position.to_json).to eq(position.to_json)
+  end
+end

ee/spec/services/draft_notes/publish_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe DraftNotes::PublishService do
+  let(:merge_request) { create(:merge_request) }
+  let(:project) { merge_request.target_project }
+  let(:user) { merge_request.author }
+
+  def publish(id: nil)
+    DraftNotes::PublishService.new(merge_request, user).execute(id)
+  end
+
+  it 'publishes a single draft note' do
+    drafts = create_list(:draft_note, 2, merge_request: merge_request, author: user)
+
+    expect { publish(id: drafts.first.id) }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1)
+    expect(DraftNote.count).to eq(1)
+  end
+
+  it 'publishes all draft notes for a user in a merge request' do
+    create_list(:draft_note, 2, merge_request: merge_request, author: user)
+
+    expect { publish }.to change { DraftNote.count }.by(-2).and change { Note.count }.by(2)
+    expect(DraftNote.count).to eq(0)
+  end
+
+  it 'only publishes the draft notes belonging to the current user' do
+    other_user = create(:user)
+    project.add_maintainer(other_user)
+
+    create_list(:draft_note, 2, merge_request: merge_request, author: user)
+    create_list(:draft_note, 2, merge_request: merge_request, author: other_user)
+
+    expect { publish }.to change { DraftNote.count }.by(-2).and change { Note.count }.by(2)
+    expect(DraftNote.count).to eq(2)
+  end
+
+  context 'with quick actions' do
+    it 'performs quick actions' do
+      create(:draft_note, merge_request: merge_request, author: user, note: "thanks\n/assign #{user.to_reference}")
+
+      expect { publish }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(2)
+      expect(merge_request.reload.assignee).to eq(user)
+      expect(merge_request.notes.last).to be_system
+    end
+
+    it 'does not create a note if it only contains quick actions' do
+      create(:draft_note, merge_request: merge_request, author: user, note: "/assign #{user.to_reference}")
+
+      expect { publish }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1)
+      expect(merge_request.reload.assignee).to eq(user)
+      expect(merge_request.notes.last).to be_system
+    end
+  end
+
+  context 'with drafts that resolve discussions' do
+    let(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
+    let(:draft_note) { create(:draft_note, merge_request: merge_request, author: user, resolve_discussion: true, discussion_id: note.discussion.reply_id) }
+
+    it 'resolves the discussion' do
+      publish(id: draft_note.id)
+
+      expect(note.discussion.resolved?).to be true
+    end
+
+    it 'sends notifications if all discussions are resolved' do
+      expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(merge_request)
+
+      publish
+    end
+  end
+end