Always require MR-iid for resolving discussions

And deduplicate the finding of MR's & discussions. Now the searching
is done in the service, istead of the controller & the API.

app/controllers/projects/issues_controller.rb

@@ -66,11 +66,14 @@ class Projects::IssuesController < Projects::ApplicationController
     )
 
     build_params = issue_params.merge(
-      merge_request_for_resolving_discussions: merge_request_for_resolving_discussions,
-      discussion_to_resolve: discussion_to_resolve
+      merge_request_for_resolving_discussions: params[:merge_request_for_resolving_discussions],
+      discussion_to_resolve: params[:discussion_to_resolve]
     )
+    service = Issues::BuildService.new(project, current_user, build_params)
+    @merge_request_for_resolving_discussions = service.merge_request_for_resolving_discussions
+    @discussion_to_resolve = service.discussions_to_resolve.first if params[:discussion_to_resolve]
 
-    @issue = @noteable = Issues::BuildService.new(project, current_user, build_params).execute
+    @issue = @noteable = service.execute
 
     respond_with(@issue)
   end
@@ -99,12 +102,11 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def create
-    create_params = issue_params.
-                      merge(
-                        merge_request_for_resolving_discussions: merge_request_for_resolving_discussions,
-                        discussion_to_resolve: discussion_to_resolve
-                      ).
-                      merge(spammable_params)
+    create_params = issue_params.merge(spammable_params).merge(
+      merge_request_for_resolving_discussions: params[:merge_request_for_resolving_discussions],
+      discussion_to_resolve: params[:discussion_to_resolve]
+    )
+
     @issue = Issues::CreateService.new(project, current_user, create_params).execute
 
     respond_to do |format|
@@ -192,21 +194,6 @@ class Projects::IssuesController < Projects::ApplicationController
   alias_method :awardable, :issue
   alias_method :spammable, :issue
 
-  def merge_request_for_resolving_discussions
-    return unless merge_request_iid = params[:merge_request_for_resolving_discussions]
-
-    @merge_request_for_resolving_discussions ||= MergeRequestsFinder.new(current_user, project_id: project.id).
-                                                   execute.
-                                                   find_by(iid: merge_request_iid)
-  end
-
-  def discussion_to_resolve
-    return unless discussion_id = params[:discussion_to_resolve]
-
-    @discussion_to_resolve ||= NotesFinder.new(project, current_user, discussion_id: discussion_id).
-                                 first_discussion
-  end
-
   def authorize_read_issue!
     return render_404 unless can?(current_user, :read_issue, @issue)
   end
@@ -253,6 +240,7 @@ class Projects::IssuesController < Projects::ApplicationController
   def issue_params
     params.require(:issue).permit(
       :title, :assignee_id, :position, :description, :confidential,
+      :discussion_to_resolve, :merge_request_for_resolving_discussions,
       :milestone_id, :due_date, :state_event, :task_num, :lock_version, label_ids: []
     )
   end

app/finders/notes_finder.rb

@@ -27,10 +27,6 @@ class NotesFinder
     @notes
   end
 
-  def first_discussion
-    execute.discussions.first
-  end
-
   private
 
   def init_collection

app/services/issues/base_service.rb

 module Issues
   class BaseService < ::IssuableBaseService
-    attr_reader :merge_request_for_resolving_discussions, :discussion_to_resolve
-
+    attr_reader :merge_request_for_resolving_discussions_iid, :discussion_to_resolve_id
     def initialize(*args)
       super
 
-      @merge_request_for_resolving_discussions ||= params.delete(:merge_request_for_resolving_discussions)
-      @discussion_to_resolve ||= params.delete(:discussion_to_resolve)
+      @merge_request_for_resolving_discussions_iid ||= params.delete(:merge_request_for_resolving_discussions)
+      @discussion_to_resolve_id ||= params.delete(:discussion_to_resolve)
     end
 
     def hook_data(issue, action)
@@ -17,25 +16,22 @@ module Issues
     end
 
     def merge_request_for_resolving_discussions
-      @merge_request_for_resolving_discussions ||= discussion_to_resolve.try(:noteable)
-    end
-
-    def for_all_discussions_in_a_merge_request?
-      discussion_to_resolve.nil? && merge_request_for_resolving_discussions
-    end
-
-    def for_single_discussion?
-      discussion_to_resolve && discussion_to_resolve.noteable == merge_request_for_resolving_discussions
+      @merge_request_for_resolving_discussions ||= MergeRequestsFinder.new(current_user, project_id: project.id).
+                                                     execute.
+                                                     find_by(iid: merge_request_for_resolving_discussions_iid)
     end
 
     def discussions_to_resolve
-      @discussions_to_resolve ||= if for_all_discussions_in_a_merge_request?
-                                    merge_request_for_resolving_discussions.resolvable_discussions
-                                  elsif for_single_discussion?
-                                    Array(discussion_to_resolve)
-                                  else
-                                    []
-                                  end
+      return [] unless merge_request_for_resolving_discussions
+
+      @discussions_to_resolve ||= NotesFinder.new(project, current_user, {
+                                                    discussion_id: discussion_to_resolve_id,
+                                                    target_type: MergeRequest.name.underscore,
+                                                    target_id: merge_request_for_resolving_discussions.id
+                                                  }).
+                                    execute.
+                                    discussions.
+                                    select(&:to_be_resolved?)
     end
 
     private

app/services/issues/create_service.rb

@@ -6,8 +6,8 @@ module Issues
       filter_spam_check_params
 
       issue_attributes = params.merge(
-        merge_request_for_resolving_discussions: merge_request_for_resolving_discussions,
-        discussion_to_resolve: discussion_to_resolve
+        merge_request_for_resolving_discussions: merge_request_for_resolving_discussions_iid,
+        discussion_to_resolve: discussion_to_resolve_id
       )
 
       @issue = BuildService.new(project, current_user, issue_attributes).execute

app/views/discussions/_new_issue_for_discussion.html.haml

@@ -5,4 +5,4 @@
       .btn.btn-default.discussion-create-issue-btn.has-tooltip{ title: "Resolve this discussion in a new issue",
                                                                "aria-label" => "Resolve this discussion in a new issue",
                                                                "data-container" => "body" }
-        = link_to custom_icon('icon_mr_issue'), new_namespace_project_issue_path(@project.namespace, @project, discussion_to_resolve: discussion.id), title: "Resolve this discussion in a new issue", class: 'new-issue-for-discussion'
+        = link_to custom_icon('icon_mr_issue'), new_namespace_project_issue_path(@project.namespace, @project, merge_request_for_resolving_discussions: merge_request.iid, discussion_to_resolve: discussion.id), title: "Resolve this discussion in a new issue", class: 'new-issue-for-discussion'

app/views/discussions/_notes.html.haml

@@ -12,7 +12,7 @@
         = render "discussions/resolve_all", discussion: discussion
         - if discussion.for_merge_request?
           .btn-group.discussion-actions
-            = render "discussions/new_issue_for_discussion", discussion: discussion
+            = render "discussions/new_issue_for_discussion", discussion: discussion, merge_request: discussion.noteable
             = render "discussions/jump_to_next", discussion: discussion
     - else
       = link_to_reply_discussion(discussion)

app/views/shared/issuable/_form.html.haml

@@ -50,30 +50,20 @@
     .col-sm-10.col-sm-offset-2
       = icon('exclamation-triangle')
       - if @merge_request_for_resolving_discussions.discussions_can_be_resolved_by?(current_user)
+        = hidden_field_tag 'merge_request_for_resolving_discussions', @merge_request_for_resolving_discussions.iid
+        - if @discussion_to_resolve
+          = hidden_field_tag 'discussion_to_resolve', @discussion_to_resolve.id
+          Creating this issue will mark the discussion at
+          = link_to @discussion_to_resolve.noteable.to_reference, Gitlab::UrlBuilder.build(@discussion_to_resolve.first_note)
+        - else
           Creating this issue will mark all discussions in
           = link_to @merge_request_for_resolving_discussions.to_reference, merge_request_path(@merge_request_for_resolving_discussions)
         as resolved.
-        = hidden_field_tag 'merge_request_for_resolving_discussions', @merge_request_for_resolving_discussions.iid
       - else
         You can't automatically mark all discussions in
         = link_to @merge_request_for_resolving_discussions.to_reference, merge_request_path(@merge_request_for_resolving_discussions)
         as resolved. Ask someone with sufficient rights to resolve the them.
 
-- if @discussion_to_resolve
-  .form-group
-    .col-sm-10.col-sm-offset-2
-      = icon('exclamation-triangle')
-      - if @discussion_to_resolve.can_resolve?(current_user)
-        Creating this issue will mark the discussion at
-        = link_to @discussion_to_resolve.noteable.to_reference, Gitlab::UrlBuilder.build(@discussion_to_resolve.first_note)
-        as resolved.
-        = hidden_field_tag 'discussion_to_resolve', @discussion_to_resolve.id
-      - else
-        You can't automatically mark the discussion at
-        = link_to @discussion_to_resolve.noteable.to_reference, Gitlab::UrlBuilder.build(@discussion_to_resolve.first_note)
-        as resolved. Ask someone with sufficient rights to resolve it.
-
-
 - is_footer = !(issuable.is_a?(MergeRequest) && issuable.new_record?)
 .row-content-block{ class: (is_footer ? "footer-block" : "middle-block") }
   .pull-right

doc/api/issues.md

@@ -341,7 +341,7 @@ POST /projects/:id/issues
 | `created_at`                              | string  | no       | Date time string, ISO 8601 formatted, e.g. `2016-03-11T03:45:40Z` (requires admin or project owner rights)                                                                                                                                           |
 | `due_date`                                | string  | no       | Date time string in the format YEAR-MONTH-DAY, e.g. `2016-03-11`                                                                                                                                                                                     |
 | `merge_request_for_resolving_discussions` | integer | no       | The IID of a merge request in which to resolve all issues. This will fill the issue with a default description and mark all discussions as resolved. When passing a description or title, these values will take precedence over the default values. |
-| `discussion_to_resolve`                   | string  | no       | The ID of a discussion to resolve. This will fill in the issue with a default description and mark the discussion as resolved.                                                                                                                                  |
+| `discussion_to_resolve`                   | string  | no       | The ID of a discussion to resolve. This will fill in the issue with a default description and mark the discussion as resolved. Use in combination with `merge_request_for_resolving_discussions`.                                                    |
 
 ```bash
 curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/4/issues?title=Issues%20with%20auth&labels=bug

lib/api/issues.rb

@@ -119,7 +119,7 @@ module API
         optional :merge_request_for_resolving_discussions, type: Integer,
                                                            desc: 'The IID of a merge request for which to resolve discussions'
         optional :discussion_to_resolve, type: String,
-                                         desc: 'The ID of a discussion to resolve'
+                                         desc: 'The ID of a discussion to resolve, also pass `merge_request_for_resolving_discussions`'
         use :issue_params
       end
       post ':id/issues' do
@@ -130,17 +130,6 @@ module API
 
         issue_params = declared_params(include_missing: false)
 
-        if merge_request_iid = params[:merge_request_for_resolving_discussions]
-          issue_params[:merge_request_for_resolving_discussions] = MergeRequestsFinder.new(current_user, project_id: user_project.id).
-            execute.
-            find_by(iid: merge_request_iid)
-        end
-
-        if discussion_id = params[:discussion_to_resolve]
-          issue_params[:discussion_to_resolve] = NotesFinder.new(user_project, current_user, discussion_id: discussion_id).
-                                                   first_discussion
-        end
-
         issue = ::Issues::CreateService.new(user_project,
                                             current_user,
                                             issue_params.merge(request: request, api: true)).execute

spec/controllers/projects/issues_controller_spec.rb

@@ -113,7 +113,7 @@ describe Projects::IssuesController do
       it 'fills in an issue for a discussion' do
         note = create(:note_on_merge_request, project: project)
 
-        get :new, namespace_id: project.namespace.path, project_id: project, discussion_to_resolve: note.discussion_id
+        get :new, namespace_id: project.namespace.path, project_id: project, merge_request_for_resolving_discussions: note.noteable.iid, discussion_to_resolve: note.discussion_id
 
         expect(assigns(:issue).title).not_to be_empty
         expect(assigns(:issue).description).not_to be_empty
@@ -474,8 +474,8 @@ describe Projects::IssuesController 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, issue: issue_params, merge_request_for_resolving_discussions: merge_request.iid
+      def post_issue(issue_params, other_params: {})
+        post :create, { namespace_id: project.namespace.to_param, project_id: project, issue: issue_params, merge_request_for_resolving_discussions: merge_request.iid }.merge(other_params)
       end
 
       it 'creates an issue for the project' do
@@ -494,6 +494,13 @@ describe Projects::IssuesController do
 
         expect(discussion.resolved?).to eq(true)
       end
+
+      it "resolves a single discussion" do
+        post_issue(other_params: { discussion_to_resolve: discussion.id })
+        discussion.first_note.reload
+
+        expect(discussion.resolved?).to eq(true)
+      end
     end
 
     context 'Akismet is enabled' do

spec/features/issues/create_issue_for_single_discussion_in_merge_request.rb

@@ -42,14 +42,14 @@ feature 'Resolve an open discussion in a merge request by creating an issue', fe
   end
 
   it 'has a link to create a new issue for a discussion' do
-    new_issue_link = new_namespace_project_issue_path(project.namespace, project, discussion_to_resolve: discussion.id)
+    new_issue_link = new_namespace_project_issue_path(project.namespace, project, discussion_to_resolve: discussion.id, merge_request_for_resolving_discussions: merge_request.iid)
 
     expect(page).to have_link 'Resolve this discussion in a new issue', href: new_issue_link
   end
 
   context 'creating the issue' do
     before do
-      click_link 'Resolve this discussion in a new issue', href: new_namespace_project_issue_path(project.namespace, project, discussion_to_resolve: discussion.id)
+      click_link 'Resolve this discussion in a new issue', href: new_namespace_project_issue_path(project.namespace, project, discussion_to_resolve: discussion.id, merge_request_for_resolving_discussions: merge_request.iid)
     end
 
     it 'has a hidden field for the discussion' do

spec/finders/notes_finder_spec.rb

@@ -157,28 +157,6 @@ describe NotesFinder do
     end
   end
 
-  describe "#first_discussion" do
-    let!(:discussion_note) { create(:note_on_merge_request, project: project) }
-
-    it 'returns a discussion' do
-      discussion = described_class.new(project, user, discussion_id: discussion_note.discussion_id).first_discussion
-
-      expect(discussion).is_a? Discussion
-    end
-
-    it 'includes the notes in the discussion' do
-      discussion = described_class.new(project, user, discussion_id: discussion_note.discussion_id).first_discussion
-
-      expect(discussion.notes).to include(discussion_note)
-    end
-
-    it 'returns nil when no notes are found' do
-      discussion = described_class.new(project, user, discussion_id: 'non-existant').first_discussion
-
-      expect(discussion).to be(nil)
-    end
-  end
-
   describe '.search' do
     let(:project) { create(:empty_project, :public) }
     let(:note) { create(:note_on_issue, note: 'WoW', project: project) }

spec/requests/api/issues_spec.rb

@@ -951,6 +951,7 @@ describe API::Issues, api: true  do
         before do
           post api("/projects/#{project.id}/issues", user),
                title: 'New Issue',
+               merge_request_for_resolving_discussions: merge_request.iid,
                discussion_to_resolve: discussion.id
         end
 

spec/services/issues/base_service_spec.rb

@@ -13,96 +13,62 @@ describe Issues::BaseService, services: true do
     let(:merge_request) { discussion.noteable }
     let(:other_merge_request) { create(:merge_request, source_project: project, source_branch: "other") }
 
-    describe "#for_single_discussion" do
-      it "is true when only a discussion is passed" do
-        service = described_class.new(project, user, discussion_to_resolve: discussion)
-
-        expect(service.for_single_discussion?).to be_truthy
-      end
-
-      it "is true when matching merge request and discussion are passed" do
-        service = described_class.new(
-          project,
-          user,
-          discussion_to_resolve: discussion,
-          merge_request_for_resolving_discussions: merge_request
-        )
-
-        expect(service.for_single_discussion?).to be_truthy
-      end
-
-      it "is false when a discussion and another merge request are passed" do
-        service = described_class.new(
-          project,
-          user,
-          discussion_to_resolve: discussion,
-          merge_request_for_resolving_discussions: other_merge_request
-        )
-
-        expect(service.for_single_discussion?).to be_falsy
-      end
-    end
-
-    describe "#for_all_discussions_in_a_merge_request" do
-      it "is true when only a merge request is passed" do
-        service = described_class.new(project, user, merge_request_for_resolving_discussions: merge_request)
-
-        expect(service.for_all_discussions_in_a_merge_request?).to be_truthy
-      end
-
-      it "is false when matching merge request and discussion are passed" do
+    describe "#discussions_to_resolve" do
+      it "contains a single discussion when matching merge request and discussion are passed" do
         service = described_class.new(
           project,
           user,
-          discussion_to_resolve: discussion,
-          merge_request_for_resolving_discussions: merge_request
+          discussion_to_resolve: discussion.id,
+          merge_request_for_resolving_discussions: merge_request.iid
         )
+        # We need to compare discussion id's because the Discussion-objects are rebuilt
+        # which causes the object-id's not to be different.
+        discussion_ids = service.discussions_to_resolve.map(&:id)
 
-        expect(service.for_all_discussions_in_a_merge_request?).to be_falsy
-      end
-    end
-
-    describe "#discussions_to_resolve" do
-      it "only contains a single discussion when only a discussion is passed" do
-        service = described_class.new(project, user, discussion_to_resolve: discussion)
-
-        expect(service.discussions_to_resolve).to contain_exactly(discussion)
+        expect(discussion_ids).to contain_exactly(discussion.id)
       end
 
-      it "is contains a single discussion when matching merge request and discussion are passed" do
+      it "contains all discussions when only a merge request is passed" do
+        second_discussion = Discussion.new([create(:diff_note_on_merge_request,
+                                                  noteable: merge_request,
+                                                  project: merge_request.target_project,
+                                                  line_number: 15)])
         service = described_class.new(
           project,
           user,
-          discussion_to_resolve: discussion,
-          merge_request_for_resolving_discussions: merge_request
+          merge_request_for_resolving_discussions: merge_request.iid
         )
+        # We need to compare discussion id's because the Discussion-objects are rebuilt
+        # which causes the object-id's not to be different.
+        discussion_ids = service.discussions_to_resolve.map(&:id)
 
-        expect(service.discussions_to_resolve).to contain_exactly(discussion)
+        expect(discussion_ids).to contain_exactly(discussion.id, second_discussion.id)
       end
 
-      it "contains all discussions when only a merge request is passed" do
-        second_discussion = Discussion.new([create(:diff_note_on_merge_request,
+      it "contains only unresolved discussions" do
+        second_discussion = Discussion.new([create(:diff_note_on_merge_request, :resolved,
                                                    noteable: merge_request,
                                                    project: merge_request.target_project,
-                                                  line_number: 15)])
+                                                   line_number: 15,
+                                                  )])
         service = described_class.new(
           project,
           user,
-          merge_request_for_resolving_discussions: merge_request
+          merge_request_for_resolving_discussions: merge_request.iid
         )
         # We need to compare discussion id's because the Discussion-objects are rebuilt
         # which causes the object-id's not to be different.
         discussion_ids = service.discussions_to_resolve.map(&:id)
 
-        expect(discussion_ids).to contain_exactly(discussion.id, second_discussion.id)
+        expect(discussion_ids).to contain_exactly(discussion.id)
       end
 
       it "is empty when a discussion and another merge request are passed" do
         service = described_class.new(
           project,
           user,
-          discussion_to_resolve: discussion,
-          merge_request_for_resolving_discussions: other_merge_request
+          discussion_to_resolve: discussion.id,
+          merge_request_for_resolving_discussions: other_merge_request.iid
         )
 
         expect(service.discussions_to_resolve).to be_empty

spec/services/issues/build_service_spec.rb

@@ -10,20 +10,17 @@ describe Issues::BuildService, services: true do
 
   context 'for a single discussion' do
     describe '#execute' do
-      it 'references the noteable title in the issue title' do
-        merge_request = create(:merge_request, title: "Hello world", source_project: project)
-        discussion = Discussion.new([create(:note_on_merge_request, project: project, noteable: merge_request)])
-        service = described_class.new(project, user, discussion_to_resolve: discussion)
+      let(:merge_request) { create(:merge_request, title: "Hello world", source_project: project) }
+      let(:discussion) { Discussion.new([create(:diff_note_on_merge_request, project: project, noteable: merge_request, note: "Almost done")]) }
+      let(:service) { described_class.new(project, user, merge_request_for_resolving_discussions: merge_request.iid, discussion_to_resolve: discussion.id) }
 
+      it 'references the noteable title in the issue title' do
         issue = service.execute
 
         expect(issue.title).to include('Hello world')
       end
 
       it 'adds the note content to the description' do
-        discussion = Discussion.new([create(:note_on_merge_request, project: project, note: "Almost done")])
-        service = described_class.new(project, user, discussion_to_resolve: discussion)
-
         issue = service.execute
 
         expect(issue.description).to include('Almost done')
@@ -33,12 +30,12 @@ describe Issues::BuildService, services: true do
 
   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 }
+    let(:issue) { described_class.new(project, user, merge_request_for_resolving_discussions: merge_request.iid).execute }
 
     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, line_number: 13)
-        service = described_class.new(project, user, merge_request_for_resolving_discussions: merge_request)
+        service = described_class.new(project, user, merge_request_for_resolving_discussions: merge_request.iid)
 
         service.execute
 
@@ -47,7 +44,7 @@ describe Issues::BuildService, services: true do
     end
 
     describe '#item_for_discussion' do
-      let(:service) { described_class.new(project, user, merge_request_for_resolving_discussions: merge_request) }
+      let(:service) { described_class.new(project, user, merge_request_for_resolving_discussions: merge_request.iid) }
 
       it 'mentions the author of the note' do
         discussion = Discussion.new([create(:diff_note_on_merge_request, author: create(:user, username: 'author'))])
@@ -125,7 +122,7 @@ describe Issues::BuildService, services: true do
 
     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
+        issue = described_class.new(project, user, merge_request_for_resolving_discussions: merge_request.iid).execute
 
         expect(issue.description).to include("Review the conversation in #{merge_request.to_reference}")
       end

spec/services/issues/create_service_spec.rb

@@ -150,7 +150,7 @@ describe Issues::CreateService, services: true do
       end
 
       describe 'for a single discussion' do
-        let(:opts) { { discussion_to_resolve: discussion } }
+        let(:opts) { { discussion_to_resolve: discussion.id, merge_request_for_resolving_discussions: merge_request.iid } }
 
         it 'resolves the discussion' do
           described_class.new(project, user, opts).execute
@@ -186,7 +186,7 @@ describe Issues::CreateService, services: true do
       end
 
       describe 'for a merge request' do
-        let(:opts) { { merge_request_for_resolving_discussions: merge_request } }
+        let(:opts) { { merge_request_for_resolving_discussions: merge_request.iid } }
 
         it 'resolves the discussion' do
           described_class.new(project, user, opts).execute

spec/support/features/resolving_discussions_in_issues_shared_examples.rb

@@ -22,4 +22,10 @@ shared_examples 'creating an issue for a discussion' do
 
     expect(discussion.resolved?).to eq(true)
   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
 end