Merge branch '13049-infer-merge-request-in-discussions-resolve-service' into 'master'

Infer merge request in Discussions::ResolveService

See merge request gitlab-org/gitlab!32932

app/controllers/projects/discussions_controller.rb

@@ -10,7 +10,7 @@ class Projects::DiscussionsController < Projects::ApplicationController
   before_action :authorize_resolve_discussion!, only: [:resolve, :unresolve]
 
   def resolve
-    Discussions::ResolveService.new(project, current_user, merge_request: merge_request).execute(discussion)
+    Discussions::ResolveService.new(project, current_user, one_or_more_discussions: discussion).execute
 
     render_discussion
   end

app/models/concerns/resolvable_discussion.rb

@@ -23,7 +23,10 @@ module ResolvableDiscussion
       :last_note
     )
 
-    delegate :potentially_resolvable?, to: :first_note
+    delegate :potentially_resolvable?,
+             :noteable_id,
+             :noteable_type,
+             to: :first_note
 
     delegate  :resolved_at,
               :resolved_by,
@@ -79,7 +82,7 @@ module ResolvableDiscussion
     return false unless current_user
     return false unless resolvable?
 
-    current_user == self.noteable.author ||
+    current_user == self.noteable.try(:author) ||
       current_user.can?(:resolve_note, self.project)
   end
 

app/services/discussions/resolve_service.rb

@@ -2,8 +2,34 @@
 
 module Discussions
   class ResolveService < Discussions::BaseService
-    def execute(one_or_more_discussions)
-      Array(one_or_more_discussions).each { |discussion| resolve_discussion(discussion) }
+    include Gitlab::Utils::StrongMemoize
+
+    def initialize(project, user = nil, params = {})
+      @discussions = Array.wrap(params.fetch(:one_or_more_discussions))
+      @follow_up_issue = params[:follow_up_issue]
+
+      raise ArgumentError, 'Discussions must be all for the same noteable' \
+        unless noteable_is_same?
+
+      super
+    end
+
+    def execute
+      discussions.each(&method(:resolve_discussion))
+    end
+
+    private
+
+    attr_accessor :discussions, :follow_up_issue
+
+    def noteable_is_same?
+      return true unless discussions.size > 1
+
+      # Perform this check without fetching extra records
+      discussions.all? do |discussion|
+        discussion.noteable_type == first_discussion.noteable_type &&
+          discussion.noteable_id == first_discussion.noteable_id
+      end
     end
 
     def resolve_discussion(discussion)
@@ -11,16 +37,18 @@ module Discussions
 
       discussion.resolve!(current_user)
 
-      MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
+      MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request) if merge_request
       SystemNoteService.discussion_continued_in_issue(discussion, project, current_user, follow_up_issue) if follow_up_issue
     end
 
-    def merge_request
-      params[:merge_request]
+    def first_discussion
+      @first_discussion ||= discussions.first
     end
 
-    def follow_up_issue
-      params[:follow_up_issue]
+    def merge_request
+      strong_memoize(:merge_request) do
+        first_discussion.noteable if first_discussion.for_merge_request?
+      end
     end
   end
 end

app/services/issues/create_service.rb

@@ -38,9 +38,8 @@ module Issues
       return if discussions_to_resolve.empty?
 
       Discussions::ResolveService.new(project, current_user,
-                                      merge_request: merge_request_to_resolve_discussions_of,
-                                      follow_up_issue: issue)
-        .execute(discussions_to_resolve)
+                                      one_or_more_discussions: discussions_to_resolve,
+                                      follow_up_issue: issue).execute
     end
 
     private

lib/api/helpers/notes_helpers.rb

@@ -133,7 +133,7 @@ module API
 
         if resolved
           parent = noteable_parent(noteable)
-          ::Discussions::ResolveService.new(parent, current_user, merge_request: noteable).execute(discussion)
+          ::Discussions::ResolveService.new(parent, current_user, one_or_more_discussions: discussion).execute
         else
           discussion.unresolve!
         end

spec/models/concerns/resolvable_discussion_spec.rb

@@ -6,10 +6,10 @@ describe Discussion, ResolvableDiscussion do
   subject { described_class.new([first_note, second_note, third_note]) }
 
   let(:first_note) { create(:discussion_note_on_merge_request) }
-  let(:merge_request) { first_note.noteable }
+  let(:noteable) { first_note.noteable }
   let(:project) { first_note.project }
-  let(:second_note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) }
-  let(:third_note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
+  let(:second_note) { create(:discussion_note_on_merge_request, noteable: noteable, project: project, in_reply_to: first_note) }
+  let(:third_note) { create(:discussion_note_on_merge_request, noteable: noteable, project: project) }
 
   describe "#resolvable?" do
     context "when potentially resolvable" do
@@ -198,12 +198,26 @@ describe Discussion, ResolvableDiscussion do
           it "returns true" do
             expect(subject.can_resolve?(current_user)).to be true
           end
+
+          context "when the noteable has no author" do
+            it "returns true" do
+              expect(noteable).to receive(:author).and_return(nil)
+              expect(subject.can_resolve?(current_user)).to be true
+            end
+          end
         end
 
         context "when the signed in user is a random user" do
           it "returns false" do
             expect(subject.can_resolve?(current_user)).to be false
           end
+
+          context "when the noteable has no author" do
+            it "returns false" do
+              expect(noteable).to receive(:author).and_return(nil)
+              expect(subject.can_resolve?(current_user)).to be false
+            end
+          end
         end
       end
     end

spec/services/discussions/resolve_service_spec.rb

@@ -4,28 +4,24 @@ require 'spec_helper'
 
 describe Discussions::ResolveService do
   describe '#execute' do
-    let(:discussion) { create(:diff_note_on_merge_request).to_discussion }
-    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.add_maintainer(user)
-    end
+    let_it_be(:project) { create(:project, :repository) }
+    let_it_be(:user) { create(:user, developer_projects: [project]) }
+    let_it_be(:merge_request) { create(:merge_request, source_project: project) }
+    let(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion }
+    let(:service) { described_class.new(project, user, one_or_more_discussions: discussion) }
 
     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)
+      service.execute
 
-      expect(discussion.resolved?).to be(false)
+      expect(discussion).not_to be_resolved
     end
 
     it 'resolves the discussion' do
-      service.execute(discussion)
+      service.execute
 
-      expect(discussion.resolved?).to be(true)
+      expect(discussion).to be_resolved
     end
 
     it 'executes the notification service' do
@@ -33,24 +29,47 @@ describe Discussions::ResolveService do
         expect(instance).to receive(:execute).with(discussion.noteable)
       end
 
-      service.execute(discussion)
+      service.execute
     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)
+      service = described_class.new(project, user, one_or_more_discussions: discussion, follow_up_issue: issue)
+      service.execute
     end
 
     it 'can resolve multiple discussions at once' do
-      other_discussion = create(:diff_note_on_merge_request, noteable: discussion.noteable, project: discussion.noteable.source_project).to_discussion
+      other_discussion = create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion
+      service = described_class.new(project, user, one_or_more_discussions: [discussion, other_discussion])
+      service.execute
+
+      expect([discussion, other_discussion]).to all(be_resolved)
+    end
 
-      service.execute([discussion, other_discussion])
+    it 'raises an argument error if discussions do not belong to the same noteable' do
+      other_merge_request = create(:merge_request)
+      other_discussion = create(:diff_note_on_merge_request,
+                                noteable: other_merge_request,
+                                project: other_merge_request.source_project).to_discussion
+      expect do
+        described_class.new(project, user, one_or_more_discussions: [discussion, other_discussion])
+      end.to raise_error(
+        ArgumentError,
+        'Discussions must be all for the same noteable'
+      )
+    end
+
+    context 'when discussion is not for a merge request' do
+      let_it_be(:design) { create(:design, :with_file, issue: create(:issue, project: project)) }
+      let(:discussion) { create(:diff_note_on_design, noteable: design, project: project).to_discussion }
 
-      expect(discussion.resolved?).to be(true)
-      expect(other_discussion.resolved?).to be(true)
+      it 'does not execute the notification service' do
+        expect(MergeRequests::ResolvedDiscussionNotificationService).not_to receive(:new)
+
+        service.execute
+      end
     end
   end
 end