Feature: delegate all open discussions to Issue

When a merge request can only be merged when all discussions are
resolved. This feature allows to easily delegate those discussions to a
new issue, while marking them as resolved in the merge request.

The user is presented with a new issue, prepared with mentions of all
unresolved discussions, including the first unresolved note of the
discussion, time and link to the note.

When the issue is created, the discussions in the merge request will get
a system note directing the user to the newly created issue.

9 files changed, 464 insertions, 0 deletions

diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 90419368f22..dbe5ddccbcf 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -55,6 +55,30 @@ describe Projects::IssuesController do
   end
 
   describe 'GET #new' do
+    context 'internal issue tracker' do
+      before do
+        sign_in(user)
+        project.team << [user, :developer]
+      end
+
+      it 'builds a new issue' do
+        get :new, namespace_id: project.namespace.path, project_id: project
+
+        expect(assigns(:issue)).to be_a_new(Issue)
+      end
+
+      it 'fills in an issue for a merge request' do
+        project_with_repository = create(:project)
+        project_with_repository.team << [user, :developer]
+        mr = create(:merge_request_with_diff_notes, source_project: project_with_repository)
+
+        get :new, namespace_id: project_with_repository.namespace.path, project_id: project_with_repository, merge_request_for_resolving_discussions: mr.iid
+
+        expect(assigns(:issue).title).not_to be_empty
+        expect(assigns(:issue).description).not_to be_empty
+      end
+    end
+
     context 'external issue tracker' do
       it 'redirects to the external issue tracker' do
         external = double(new_issue_path: 'https://example.com/issues/new')
@@ -272,6 +296,42 @@ describe Projects::IssuesController do
   end
 
   describe 'POST #create' do
+    context 'resolving discussions in MergeRequest' do
+      let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+      let(:merge_request) { discussion.noteable }
+      let(:project) { merge_request.source_project }
+
+      before do
+        project.team << [user, :master]
+        sign_in user
+      end
+
+      let(:merge_request_params) do
+        { merge_request_for_resolving_discussions: merge_request.iid }
+      end
+
+      def post_issue(issue_params)
+        post :create, namespace_id: project.namespace.to_param, project_id: project.to_param, issue: issue_params, merge_request_for_resolving_discussions: merge_request.iid
+      end
+
+      it 'creates an issue for the project' do
+        expect { post_issue({ title: 'Hello' }) }.to change { project.issues.reload.size }.by(1)
+      end
+
+      it "doesn't overwrite given params" do
+        post_issue(description: 'Manually entered description')
+
+        expect(assigns(:issue).description).to eq('Manually entered description')
+      end
+
+      it 'resolves the discussion in the merge_request' do
+        post_issue(title: 'Hello')
+        discussion.first_note.reload
+
+        expect(discussion.resolved?).to eq(true)
+      end
+    end
+
     context 'Akismet is enabled' do
       before do
         allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true)
diff --git a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb
new file mode 100644
index 00000000000..762cab0c0e1
--- /dev/null
+++ b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb
@@ -0,0 +1,76 @@
+require 'rails_helper'
+
+feature 'Resolving all open discussions in a merge request from an issue', feature: true do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, only_allow_merge_if_all_discussions_are_resolved: true) }
+  let(:merge_request) { create(:merge_request, source_project: project) }
+  let!(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request, noteable: merge_request, project: project)]).first }
+
+  before do
+    project.team << [user, :master]
+    login_as user
+  end
+
+  context 'with the internal tracker disabled' do
+    before do
+      project.project_feature.update_attribute(:issues_access_level, ProjectFeature::DISABLED)
+      visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+    end
+
+    it 'does not show a link to create a new issue' do
+      expect(page).not_to have_link 'open an issue to resolve them later'
+    end
+  end
+
+  context 'merge request has discussions that need to be resolved' do
+    before do
+      visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+    end
+
+    it 'shows a warning that the merge request contains unresolved discussions' do
+      expect(page).to have_content 'This merge request has unresolved discussions'
+    end
+
+    it 'has a link to resolve all discussions by creating an issue' do
+      page.within '.mr-widget-body' do
+        expect(page).to have_link 'open an issue to resolve them later', href: new_namespace_project_issue_path(project.namespace, project, merge_request_for_resolving_discussions: merge_request.iid)
+      end
+    end
+
+    context 'creating an issue for discussions' do
+      before do
+        page.click_link 'open an issue to resolve them later', href: new_namespace_project_issue_path(project.namespace, project, merge_request_for_resolving_discussions: merge_request.iid)
+      end
+
+      it 'shows an issue with the title filled in' do
+        title_field = page.find_field('issue[title]')
+
+        expect(title_field.value).to include(merge_request.title)
+      end
+
+      it 'has a mention of the discussion in the description'  do
+        description_field = page.find_field('issue[description]')
+
+        expect(description_field.value).to include(discussion.first_note.note)
+      end
+
+      it 'has a hidden field for the merge request' do
+        merge_request_field = find('#merge_request_for_resolving_discussions', visible: false)
+
+        expect(merge_request_field.value).to eq(merge_request.iid.to_s)
+      end
+
+      it 'can create a new issue for the project' do
+        expect { click_button 'Submit issue' }.to change { project.issues.reload.size }.by(1)
+      end
+
+      it 'resolves the discussion in the merge request' do
+        click_button 'Submit issue'
+
+        discussion.first_note.reload
+
+        expect(discussion.resolved?).to eq(true)
+      end
+    end
+  end
+end
diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb
index 2a67c60b978..bc32fadd391 100644
--- a/spec/models/discussion_spec.rb
+++ b/spec/models/discussion_spec.rb
@@ -521,6 +521,15 @@ describe Discussion, model: true do
     end
   end
 
+  describe "#first_note_to_resolve" do
+    it "returns the first not that still needs to be resolved" do
+      allow(first_note).to receive(:to_be_resolved?).and_return(false)
+      allow(second_note).to receive(:to_be_resolved?).and_return(true)
+
+      expect(subject.first_note_to_resolve).to eq(second_note)
+    end
+  end
+
   describe "#collapsed?" do
     context "when a diff discussion" do
       before do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 2cc818af6c7..925232169f1 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1124,6 +1124,46 @@ describe MergeRequest, models: true do
       allow(subject).to receive(:diff_discussions).and_return([first_discussion, second_discussion, third_discussion])
     end
 
+    describe '#resolvable_discussions' do
+      before do
+        allow(first_discussion).to receive(:to_be_resolved?).and_return(true)
+        allow(second_discussion).to receive(:to_be_resolved?).and_return(false)
+        allow(third_discussion).to receive(:to_be_resolved?).and_return(false)
+      end
+
+      it 'includes only discussions that need to be resolved' do
+        expect(subject.resolvable_discussions).to eq([first_discussion])
+      end
+    end
+
+    describe '#discussions_can_be_resolved_by? user' do
+      let(:user) { build(:user) }
+
+      context 'all discussions can be resolved by the user' do
+        before do
+          allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
+          allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
+          allow(third_discussion).to receive(:can_resolve?).with(user).and_return(true)
+        end
+
+        it 'allows a user to resolve the discussions' do
+          expect(subject.discussions_can_be_resolved_by?(user)).to be(true)
+        end
+      end
+
+      context 'one discussion cannot be resolved by the user' do
+        before do
+          allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
+          allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
+          allow(third_discussion).to receive(:can_resolve?).with(user).and_return(false)
+        end
+
+        it 'allows a user to resolve the discussions' do
+          expect(subject.discussions_can_be_resolved_by?(user)).to be(false)
+        end
+      end
+    end
+
     describe "#discussions_resolvable?" do
       context "when all discussions are unresolvable" do
         before do
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index 5700f800c2e..553983575c4 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -692,6 +692,32 @@ describe API::Issues, api: true  do
       ])
     end
 
+    context 'resolving issues in a merge request' do
+      let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+      let(:merge_request) { discussion.noteable }
+      let(:project) { merge_request.source_project }
+      before do
+        project.team << [user, :master]
+        post api("/projects/#{project.id}/issues", user),
+             title: 'New Issue',
+             merge_request_for_resolving_discussions: merge_request.iid
+      end
+
+      it 'creates a new project issue' do
+        expect(response).to have_http_status(:created)
+      end
+
+      it 'resolves the discussions in a merge request' do
+        discussion.first_note.reload
+
+        expect(discussion.resolved?).to be(true)
+      end
+
+      it 'assigns a description to the issue mentioning the merge request' do
+        expect(json_response['description']).to include(merge_request.to_reference)
+      end
+    end
+
     context 'with due date' do
       it 'creates a new project issue' do
         due_date = 2.weeks.from_now.strftime('%Y-%m-%d')
diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb
new file mode 100644
index 00000000000..12c3cdf28c6
--- /dev/null
+++ b/spec/services/discussions/resolve_service_spec.rb
@@ -0,0 +1,52 @@
+require 'spec_helper'
+
+describe Discussions::ResolveService do
+  describe '#execute' do
+    let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+    let(:project) { merge_request.project }
+    let(:merge_request) { discussion.noteable }
+    let(:user) { create(:user) }
+    let(:service) { described_class.new(discussion.noteable.project, user, merge_request: merge_request) }
+
+    before do
+      project.team << [user, :master]
+    end
+
+    it "doesn't resolve discussions the user can't resolve" do
+      expect(discussion).to receive(:can_resolve?).with(user).and_return(false)
+
+      service.execute(discussion)
+
+      expect(discussion.resolved?).to be(false)
+    end
+
+    it 'resolves the discussion' do
+      service.execute(discussion)
+
+      expect(discussion.resolved?).to be(true)
+    end
+
+    it 'executes the notification service' do
+      expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(discussion.noteable)
+
+      service.execute(discussion)
+    end
+
+    it 'adds a system note to the discussion' do
+      issue = create(:issue, project: project)
+
+      expect(SystemNoteService).to receive(:discussion_continued_in_issue).with(discussion, project, user, issue)
+      service = described_class.new(project, user, merge_request: merge_request, follow_up_issue: issue)
+      service.execute(discussion)
+    end
+
+    it 'can resolve multiple discussions at once' do
+      other_discussion = Discussion.for_diff_notes([create(:diff_note_on_merge_request, noteable: discussion.noteable, project: discussion.noteable.source_project)]).first
+
+      service.execute([discussion, other_discussion])
+
+      expect(discussion.resolved?).to be(true)
+      expect(other_discussion.resolved?).to be(true)
+    end
+  end
+end
diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb
new file mode 100644
index 00000000000..4cfba35c830
--- /dev/null
+++ b/spec/services/issues/build_service_spec.rb
@@ -0,0 +1,130 @@
+require 'spec_helper.rb'
+
+describe Issues::BuildService, services: true do
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+
+  before do
+    project.team << [user, :developer]
+  end
+
+  context 'for discussions in a merge request' do
+    let(:merge_request) { create(:merge_request_with_diff_notes, source_project: project) }
+    let(:issue) { described_class.new(project, user, merge_request_for_resolving_discussions: merge_request).execute }
+
+    def position_on_line(line_number)
+      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: merge_request.diff_refs
+      )
+    end
+
+    describe '#items_for_discussions' do
+      it 'has an item for each discussion' do
+        create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.source_project, position: position_on_line(13))
+        service = described_class.new(project, user, merge_request_for_resolving_discussions: merge_request)
+
+        service.execute
+
+        expect(service.items_for_discussions.size).to eq(2)
+      end
+    end
+
+    describe '#item_for_discussion' do
+      let(:service) { described_class.new(project, user, merge_request_for_resolving_discussions: merge_request) }
+
+      it 'mentions the author of the note' do
+        discussion = Discussion.new([create(:diff_note_on_merge_request, author: create(:user, username: 'author'))])
+        expect(service.item_for_discussion(discussion)).to include('@author')
+      end
+
+      it 'wraps the note in a blockquote' do
+        note_text = "This is a string\n"\
+                    ">>>\n"\
+                    "with a blockquote\n"\
+                    "> That has a quote\n"\
+                    ">>>\n"
+        note_result = "This is a string\n"\
+                    "> with a blockquote\n"\
+                    "> > That has a quote\n"
+        discussion = Discussion.new([create(:diff_note_on_merge_request, note: note_text)])
+        expect(service.item_for_discussion(discussion)).to include(">>>\n#{note_result}\n>>>")
+      end
+    end
+
+    describe '#execute' do
+      it 'has the merge request reference in the title' do
+        expect(issue.title).to include(merge_request.title)
+      end
+
+      it 'has the reference of the merge request in the description' do
+        expect(issue.description).to include(merge_request.to_reference)
+      end
+
+      it 'does not assign title when a title was given' do
+        issue = described_class.new(project, user,
+                                    merge_request_for_resolving_discussions: merge_request,
+                                    title: 'What an issue').execute
+
+        expect(issue.title).to eq('What an issue')
+      end
+
+      it 'does not assign description when a description was given' do
+        issue = described_class.new(project, user,
+                                    merge_request_for_resolving_discussions: merge_request,
+                                    description: 'Fix at your earliest conveignance').execute
+
+        expect(issue.description).to eq('Fix at your earliest conveignance')
+      end
+
+      describe 'with multiple discussions' do
+        before do
+          create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.target_project, position: position_on_line(15))
+        end
+
+        it 'mentions all the authors in the description' do
+          authors = merge_request.diff_discussions.map(&:author)
+
+          expect(issue.description).to include(*authors.map(&:to_reference))
+        end
+
+        it 'has a link for each unresolved discussion in the description' do
+          notes = merge_request.diff_discussions.map(&:first_note)
+          links = notes.map { |note| Gitlab::UrlBuilder.build(note) }
+
+          expect(issue.description).to include(*links)
+        end
+
+        it 'mentions additional notes' do
+          create_list(:diff_note_on_merge_request, 2, noteable: merge_request, project: merge_request.target_project, position: position_on_line(15))
+
+          expect(issue.description).to include('(+2 comments)')
+        end
+      end
+    end
+  end
+
+  context 'For a merge request without discussions' do
+    let(:merge_request) { create(:merge_request, source_project: project) }
+
+    describe '#execute' do
+      it 'mentions the merge request in the description' do
+        issue = described_class.new(project, user, merge_request_for_resolving_discussions: merge_request).execute
+
+        expect(issue.description).to include("Review the conversation in #{merge_request.to_reference}")
+      end
+    end
+  end
+
+  describe '#execute' do
+    it 'builds a new issues with given params' do
+      issue = described_class.new(project, user, title: 'Issue #1', description: 'Issue description').execute
+
+      expect(issue.title).to eq('Issue #1')
+      expect(issue.description).to eq('Issue description')
+    end
+  end
+end
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index 5c0331ebe66..8bde61ee336 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -136,5 +136,48 @@ describe Issues::CreateService, services: true do
     end
 
     it_behaves_like 'new issuable record that supports slash commands'
+
+    context 'for a merge request' do
+      let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+      let(:merge_request) { discussion.noteable }
+      let(:project) { merge_request.source_project }
+      let(:opts) { { merge_request_for_resolving_discussions: merge_request } }
+
+      before do
+        project.team << [user, :master]
+      end
+
+      it 'resolves the discussion for the merge request' do
+        described_class.new(project, user, opts).execute
+        discussion.first_note.reload
+
+        expect(discussion.resolved?).to be(true)
+      end
+
+      it 'added a system note to the discussion' do
+        described_class.new(project, user, opts).execute
+
+        reloaded_discussion = MergeRequest.find(merge_request.id).discussions.first
+
+        expect(reloaded_discussion.last_note.system).to eq(true)
+      end
+
+      it 'assigns the title and description for the issue' do
+        issue = described_class.new(project, user, opts).execute
+
+        expect(issue.title).not_to be_nil
+        expect(issue.description).not_to be_nil
+      end
+
+      it 'can set nil explicityly to the title and description' do
+        issue = described_class.new(project, user,
+                                    merge_request_for_resolving_discussions: merge_request,
+                                    description: nil,
+                                    title: nil).execute
+
+        expect(issue.description).to be_nil
+        expect(issue.title).to be_nil
+      end
+    end
   end
 end
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 435cfb07292..07a9d8e1997 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -712,4 +712,32 @@ describe SystemNoteService, services: true do
       end
     end
   end
+
+  describe '.discussion_continued_in_issue' do
+    let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+    let(:merge_request) { discussion.noteable }
+    let(:project) { merge_request.source_project }
+    let(:issue) { create(:issue, project: project) }
+    let(:user) { create(:user) }
+
+    def reloaded_merge_request
+      MergeRequest.find(merge_request.id)
+    end
+
+    before do
+      project.team << [user, :developer]
+    end
+
+    it 'creates a new note in the discussion' do
+      # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded.
+      expect { SystemNoteService.discussion_continued_in_issue(discussion, project, user, issue) }.
+        to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
+    end
+
+    it 'mentions the created issue in the system note' do
+      note = SystemNoteService.discussion_continued_in_issue(discussion, project, user, issue)
+
+      expect(note.note).to include(issue.to_reference)
+    end
+  end
 end