Infer merge request in Discussions::ResolveService

This change allows Discussions::ResolveService to infer the related
Merge Request without the params[:merge_request] being present.

Previously the param was expected to be passed and the service would
error if it were not present.

This change is being made for two reasons:

1) Discussions can be associated with Designs, which have no
relationship with Merge Requests. In order to be able to use this
service to resolve Discussions on Designs, we need to make this service
able to treat the param as optional.

2) This service is to be called from within the GraphQL API. It
shouldn't be the API's responsibility to find the related Merge Request
and pass it to the service, but instead the service should be
responsible for making sure the right services are called after
resolving Discussions on Merge Requests.

The params[:merge_request] has been kept as an optimisation for existing
code that has fetched the Merge Request already before calling the
service (this won't be the case for the GraphQL API).

https://gitlab.com/gitlab-org/gitlab/-/issues/13049

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
 
-      expect(discussion.resolved?).to be(true)
-      expect(other_discussion.resolved?).to be(true)
+    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 }
+
+      it 'does not execute the notification service' do
+        expect(MergeRequests::ResolvedDiscussionNotificationService).not_to receive(:new)
+
+        service.execute
+      end
     end
   end
 end