Add latest changes from gitlab-org/gitlab@master

7 files changed, 522 insertions, 4 deletions

diff --git a/spec/services/draft_notes/create_service_spec.rb b/spec/services/draft_notes/create_service_spec.rb
new file mode 100644
index 00000000000..8f244ed386b
--- /dev/null
+++ b/spec/services/draft_notes/create_service_spec.rb
@@ -0,0 +1,94 @@
+# 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 thread')
+    expect(draft).not_to be_persisted
+  end
+
+  context 'in a thread' 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 thread' 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
+
+  context 'diff highlight cache clearing' do
+    context 'when diff file is unfolded and it is not a reply' do
+      it 'clears diff highlighting cache' do
+        expect_next_instance_of(DraftNote) do |draft|
+          allow(draft).to receive_message_chain(:diff_file, :unfolded?) { true }
+        end
+
+        expect(merge_request).to receive_message_chain(:diffs, :clear_cache)
+
+        create_draft(note: 'This is a test')
+      end
+    end
+
+    context 'when diff file is not unfolded and it is not a reply' do
+      it 'clears diff highlighting cache' do
+        expect_next_instance_of(DraftNote) do |draft|
+          allow(draft).to receive_message_chain(:diff_file, :unfolded?) { false }
+        end
+
+        expect(merge_request).not_to receive(:diffs)
+
+        create_draft(note: 'This is a test')
+      end
+    end
+  end
+end
diff --git a/spec/services/draft_notes/destroy_service_spec.rb b/spec/services/draft_notes/destroy_service_spec.rb
new file mode 100644
index 00000000000..d0bf88dcdbe
--- /dev/null
+++ b/spec/services/draft_notes/destroy_service_spec.rb
@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+require 'spec_helper'
+
+describe DraftNotes::DestroyService do
+  let(:merge_request) { create(:merge_request) }
+  let(:project) { merge_request.target_project }
+  let(:user) { merge_request.author }
+
+  def destroy(draft_note = nil)
+    DraftNotes::DestroyService.new(merge_request, user).execute(draft_note)
+  end
+
+  it 'destroys a single draft note' do
+    drafts = create_list(:draft_note, 2, merge_request: merge_request, author: user)
+
+    expect { destroy(drafts.first) }
+      .to change { DraftNote.count }.by(-1)
+
+    expect(DraftNote.count).to eq(1)
+  end
+
+  it 'destroys all draft notes for a user in a merge request' do
+    create_list(:draft_note, 2, merge_request: merge_request, author: user)
+
+    expect { destroy }.to change { DraftNote.count }.by(-2)
+    expect(DraftNote.count).to eq(0)
+  end
+
+  context 'diff highlight cache clearing' do
+    context 'when destroying all draft notes of a user' do
+      it 'clears highlighting cache if unfold required for any' do
+        drafts = create_list(:draft_note, 2, merge_request: merge_request, author: user)
+
+        allow_any_instance_of(DraftNote).to receive_message_chain(:diff_file, :unfolded?) { true }
+        expect(merge_request).to receive_message_chain(:diffs, :clear_cache)
+
+        destroy(drafts.first)
+      end
+    end
+
+    context 'when destroying one draft note' do
+      it 'clears highlighting cache if unfold required' do
+        create_list(:draft_note, 2, merge_request: merge_request, author: user)
+
+        allow_any_instance_of(DraftNote).to receive_message_chain(:diff_file, :unfolded?) { true }
+        expect(merge_request).to receive_message_chain(:diffs, :clear_cache)
+
+        destroy
+      end
+    end
+  end
+end
diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb
new file mode 100644
index 00000000000..4ebae2f9aa2
--- /dev/null
+++ b/spec/services/draft_notes/publish_service_spec.rb
@@ -0,0 +1,261 @@
+# frozen_string_literal: true
+require 'spec_helper'
+
+describe DraftNotes::PublishService do
+  include RepoHelpers
+
+  let(:merge_request) { create(:merge_request) }
+  let(:project) { merge_request.target_project }
+  let(:user) { merge_request.author }
+  let(:commit) { project.commit(sample_commit.id) }
+
+  let(:position) do
+    Gitlab::Diff::Position.new(
+      old_path: "files/ruby/popen.rb",
+      new_path: "files/ruby/popen.rb",
+      old_line: nil,
+      new_line: 14,
+      diff_refs: commit.diff_refs
+    )
+  end
+
+  def publish(draft: nil)
+    DraftNotes::PublishService.new(merge_request, user).execute(draft)
+  end
+
+  context 'single draft note' do
+    let(:commit_id) { nil }
+    let!(:drafts) { create_list(:draft_note, 2, merge_request: merge_request, author: user, commit_id: commit_id, position: position) }
+
+    it 'publishes' do
+      expect { publish(draft: drafts.first) }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1)
+      expect(DraftNote.count).to eq(1)
+    end
+
+    it 'does not skip notification', :sidekiq_might_not_need_inline do
+      expect(Notes::CreateService).to receive(:new).with(project, user, drafts.first.publish_params).and_call_original
+      expect_next_instance_of(NotificationService) do |notification_service|
+        expect(notification_service).to receive(:new_note)
+      end
+
+      result = publish(draft: drafts.first)
+
+      expect(result[:status]).to eq(:success)
+    end
+
+    context 'commit_id is set' do
+      let(:commit_id) { commit.id }
+
+      it 'creates note from draft with commit_id' do
+        result = publish(draft: drafts.first)
+
+        expect(result[:status]).to eq(:success)
+        expect(merge_request.notes.first.commit_id).to eq(commit_id)
+      end
+    end
+  end
+
+  context 'multiple draft notes' do
+    let(:commit_id) { nil }
+
+    before do
+      create(:draft_note, merge_request: merge_request, author: user, note: 'first note', commit_id: commit_id, position: position)
+      create(:draft_note, merge_request: merge_request, author: user, note: 'second note', commit_id: commit_id, position: position)
+    end
+
+    context 'when review fails to create' do
+      before do
+        expect_next_instance_of(Review) do |review|
+          allow(review).to receive(:save!).and_raise(ActiveRecord::RecordInvalid.new(review))
+        end
+      end
+
+      it 'does not publish any draft note' do
+        expect { publish }.not_to change { DraftNote.count }
+      end
+
+      it 'returns an error' do
+        result = publish
+
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to match(/Unable to save Review/)
+      end
+    end
+
+    it 'returns success' do
+      result = publish
+
+      expect(result[:status]).to eq(:success)
+    end
+
+    it 'publishes all draft notes for a user in a merge request' do
+      expect { publish }.to change { DraftNote.count }.by(-2).and change { Note.count }.by(2).and change { Review.count }.by(1)
+      expect(DraftNote.count).to eq(0)
+
+      notes = merge_request.notes.order(id: :asc)
+      expect(notes.first.note).to eq('first note')
+      expect(notes.last.note).to eq('second note')
+    end
+
+    it 'sends batch notification' do
+      expect_next_instance_of(NotificationService) do |notification_service|
+        expect(notification_service).to receive_message_chain(:async, :new_review).with(kind_of(Review))
+      end
+
+      publish
+    end
+
+    context 'commit_id is set' do
+      let(:commit_id) { commit.id }
+
+      it 'creates note from draft with commit_id' do
+        result = publish
+
+        expect(result[:status]).to eq(:success)
+
+        merge_request.notes.each do |note|
+          expect(note.commit_id).to eq(commit_id)
+        end
+      end
+    end
+  end
+
+  context 'draft notes with suggestions' do
+    let(:project) { create(:project, :repository) }
+    let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+    let(:suggestion_note) do
+      <<-MARKDOWN.strip_heredoc
+        ```suggestion
+          foo
+        ```
+      MARKDOWN
+    end
+
+    let!(:draft) { create(:draft_note_on_text_diff, note: suggestion_note, merge_request: merge_request, author: user) }
+
+    it 'creates a suggestion with correct content' do
+      expect { publish(draft: draft) }.to change { Suggestion.count }.by(1)
+        .and change { DiffNote.count }.from(0).to(1)
+
+      suggestion = Suggestion.last
+
+      expect(suggestion.from_line).to eq(14)
+      expect(suggestion.to_line).to eq(14)
+      expect(suggestion.from_content).to eq("    vars = {\n")
+      expect(suggestion.to_content).to eq("  foo\n")
+    end
+
+    context 'when the diff is changed' do
+      let(:file_path) { 'files/ruby/popen.rb' }
+      let(:branch_name) { project.default_branch }
+      let(:commit) { project.repository.commit }
+
+      def update_file(file_path, new_content)
+        params = {
+          file_path: file_path,
+          commit_message: "Update File",
+          file_content: new_content,
+          start_project: project,
+          start_branch: project.default_branch,
+          branch_name: branch_name
+        }
+
+        Files::UpdateService.new(project, user, params).execute
+      end
+
+      before do
+        project.add_developer(user)
+      end
+
+      it 'creates a suggestion based on the latest diff content and positions' do
+        diff_file = merge_request.diffs(paths: [file_path]).diff_files.first
+        raw_data = diff_file.new_blob.data
+
+        # Add a line break to the beginning of the file
+        result = update_file(file_path, raw_data.prepend("\n"))
+        oldrev = merge_request.diff_head_sha
+        newrev = result[:result]
+
+        expect(newrev).to be_present
+
+        # Generates new MR revision at DB level
+        refresh = MergeRequests::RefreshService.new(project, user)
+        refresh.execute(oldrev, newrev, merge_request.source_branch_ref)
+
+        expect { publish(draft: draft) }.to change { Suggestion.count }.by(1)
+          .and change { DiffNote.count }.from(0).to(1)
+
+        suggestion = Suggestion.last
+
+        expect(suggestion.from_line).to eq(15)
+        expect(suggestion.to_line).to eq(15)
+        expect(suggestion.from_content).to eq("    vars = {\n")
+        expect(suggestion.to_content).to eq("  foo\n")
+      end
+    end
+  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
+      other_user = create(:user)
+      project.add_developer(other_user)
+
+      create(:draft_note, merge_request: merge_request,
+                          author: user,
+                          note: "thanks\n/assign #{other_user.to_reference}")
+
+      expect { publish }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(2)
+      expect(merge_request.reload.assignees).to match_array([other_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.assignees).to eq([user])
+      expect(merge_request.notes.last).to be_system
+    end
+  end
+
+  context 'with drafts that resolve threads' 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 thread' do
+      publish(draft: draft_note)
+
+      expect(note.discussion.resolved?).to be true
+    end
+
+    it 'sends notifications if all threads are resolved' do
+      expect_next_instance_of(MergeRequests::ResolvedDiscussionNotificationService) do |instance|
+        expect(instance).to receive(:execute).with(merge_request)
+      end
+
+      publish
+    end
+  end
+
+  context 'user cannot create notes' do
+    before do
+      allow(Ability).to receive(:allowed?).with(user, :create_note, merge_request).and_return(false)
+    end
+
+    it 'returns an error' do
+      expect(publish[:status]).to eq(:error)
+    end
+  end
+end
diff --git a/spec/services/notification_recipients/build_service_spec.rb b/spec/services/notification_recipients/build_service_spec.rb
index 2e848c2f04d..e203093623d 100644
--- a/spec/services/notification_recipients/build_service_spec.rb
+++ b/spec/services/notification_recipients/build_service_spec.rb
@@ -58,4 +58,56 @@ describe NotificationRecipients::BuildService do
       end
     end
   end
+
+  describe '#build_new_review_recipients' do
+    let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+    let(:review) { create(:review, merge_request: merge_request, project: project, author: merge_request.author) }
+    let(:notes) { create_list(:note_on_merge_request, 3, review: review, noteable: review.merge_request, project: review.project) }
+
+    shared_examples 'no N+1 queries' do
+      it 'avoids N+1 queries', :request_store do
+        create_user
+
+        service.build_new_review_recipients(review)
+
+        control_count = ActiveRecord::QueryRecorder.new do
+          service.build_new_review_recipients(review)
+        end
+
+        create_user
+
+        expect { service.build_new_review_recipients(review) }.not_to exceed_query_limit(control_count)
+      end
+    end
+
+    context 'when there are multiple watchers' do
+      def create_user
+        watcher = create(:user)
+        create(:notification_setting, source: project, user: watcher, level: :watch)
+
+        other_projects.each do |other_project|
+          create(:notification_setting, source: other_project, user: watcher, level: :watch)
+        end
+      end
+
+      include_examples 'no N+1 queries'
+    end
+
+    context 'when there are multiple subscribers' do
+      def create_user
+        subscriber = create(:user)
+        merge_request.subscriptions.create(user: subscriber, project: project, subscribed: true)
+      end
+
+      include_examples 'no N+1 queries'
+
+      context 'when the project is private' do
+        before do
+          project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+        end
+
+        include_examples 'no N+1 queries'
+      end
+    end
+  end
 end
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 2a7166e3895..d3376ef0a04 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -2863,6 +2863,57 @@ describe NotificationService, :mailer do
     end
   end
 
+  describe '#new_review' do
+    let(:project) { create(:project, :repository) }
+    let(:user) { create(:user) }
+    let(:user2) { create(:user) }
+    let(:reviewer) { create(:user) }
+    let(:merge_request) { create(:merge_request, source_project: project, assignees: [user, user2], author: create(:user)) }
+    let(:review) { create(:review, merge_request: merge_request, project: project, author: reviewer) }
+    let(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, author: reviewer, review: review) }
+
+    before do
+      build_team(review.project)
+      add_users(review.project)
+      add_user_subscriptions(merge_request)
+      project.add_maintainer(merge_request.author)
+      project.add_maintainer(reviewer)
+      merge_request.assignees.each { |assignee| project.add_maintainer(assignee) }
+
+      create(:diff_note_on_merge_request,
+             project: project,
+             noteable: merge_request,
+             author: reviewer,
+             review: review,
+             note: "cc @mention")
+    end
+
+    it 'sends emails' do
+      expect(Notify).not_to receive(:new_review_email).with(review.author.id, review.id)
+      expect(Notify).not_to receive(:new_review_email).with(@unsubscriber.id, review.id)
+      merge_request.assignee_ids.each do |assignee_id|
+        expect(Notify).to receive(:new_review_email).with(assignee_id, review.id).and_call_original
+      end
+      expect(Notify).to receive(:new_review_email).with(merge_request.author.id, review.id).and_call_original
+      expect(Notify).to receive(:new_review_email).with(@u_watcher.id, review.id).and_call_original
+      expect(Notify).to receive(:new_review_email).with(@u_mentioned.id, review.id).and_call_original
+      expect(Notify).to receive(:new_review_email).with(@subscriber.id, review.id).and_call_original
+      expect(Notify).to receive(:new_review_email).with(@watcher_and_subscriber.id, review.id).and_call_original
+      expect(Notify).to receive(:new_review_email).with(@subscribed_participant.id, review.id).and_call_original
+
+      subject.new_review(review)
+    end
+
+    it_behaves_like 'project emails are disabled' do
+      let(:notification_target)  { review }
+      let(:notification_trigger) { subject.new_review(review) }
+
+      around do |example|
+        perform_enqueued_jobs { example.run }
+      end
+    end
+  end
+
   def build_team(project)
     @u_watcher               = create_global_setting_for(create(:user), :watch)
     @u_participating         = create_global_setting_for(create(:user), :participating)
diff --git a/spec/services/prometheus/proxy_variable_substitution_service_spec.rb b/spec/services/prometheus/proxy_variable_substitution_service_spec.rb
index 82ea356d599..5982dcbc404 100644
--- a/spec/services/prometheus/proxy_variable_substitution_service_spec.rb
+++ b/spec/services/prometheus/proxy_variable_substitution_service_spec.rb
@@ -64,7 +64,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
       let(:params_keys) do
         {
           query: 'up{pod_name="{{pod_name}}"}',
-          variables: ['pod_name', pod_name]
+          variables: { 'pod_name' => pod_name }
         }
       end
 
@@ -76,7 +76,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
         let(:params_keys) do
           {
             query: 'up{pod_name="{{pod_name}}",env="{{ci_environment_slug}}"}',
-            variables: ['pod_name', pod_name, 'ci_environment_slug', 'custom_value']
+            variables: { 'pod_name' => pod_name, 'ci_environment_slug' => 'custom_value' }
           }
         end
 
@@ -95,8 +95,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
           }
         end
 
-        it_behaves_like 'error', 'Optional parameter "variables" must be an ' \
-          'array of keys and values. Ex: [key1, value1, key2, value2]'
+        it_behaves_like 'error', 'Optional parameter "variables" must be a Hash. Ex: variables[key1]=value1'
       end
 
       context 'with nil variables' do
diff --git a/spec/services/users/migrate_to_ghost_user_service_spec.rb b/spec/services/users/migrate_to_ghost_user_service_spec.rb
index a7d7c16a66f..c2a793b2368 100644
--- a/spec/services/users/migrate_to_ghost_user_service_spec.rb
+++ b/spec/services/users/migrate_to_ghost_user_service_spec.rb
@@ -84,6 +84,15 @@ describe Users::MigrateToGhostUserService do
       end
     end
 
+    context 'reviews' do
+      let!(:user)   { create(:user) }
+      let(:service) { described_class.new(user) }
+
+      include_examples "migrating a deleted user's associated records to the ghost user", Review, [:author] do
+        let(:created_record) { create(:review, author: user) }
+      end
+    end
+
     context "when record migration fails with a rollback exception" do
       before do
         expect_any_instance_of(ActiveRecord::Associations::CollectionProxy)