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.

7 files changed, 40 insertions, 70 deletions

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index d7eb73d7ff5..d76643ff875 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/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
diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb
index 6d0ff1798ce..98c66dfa021 100644
--- a/app/finders/notes_finder.rb
+++ b/app/finders/notes_finder.rb
@@ -27,10 +27,6 @@ class NotesFinder
     @notes
   end
 
-  def first_discussion
-    execute.discussions.first
-  end
-
   private
 
   def init_collection
diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
index 3aa842eb9a8..058c398df02 100644
--- a/app/services/issues/base_service.rb
+++ b/app/services/issues/base_service.rb
@@ -1,12 +1,11 @@
 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
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index d2cc70ed0dc..007c8354c8c 100644
--- a/app/services/issues/create_service.rb
+++ b/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
diff --git a/app/views/discussions/_new_issue_for_discussion.html.haml b/app/views/discussions/_new_issue_for_discussion.html.haml
index de28ded9820..c21ec0ecc3d 100644
--- a/app/views/discussions/_new_issue_for_discussion.html.haml
+++ b/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'
diff --git a/app/views/discussions/_notes.html.haml b/app/views/discussions/_notes.html.haml
index 4a1e93cec3a..2789391819c 100644
--- a/app/views/discussions/_notes.html.haml
+++ b/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)
diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml
index e93fe0f6cb5..a10f9caf3c1 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/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)
-        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
+        - 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.
       - 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