Extract Merge Requests System Note Service

Also move specs

Move specs to MergeRequestsService

app/services/system_note_service.rb

@@ -128,82 +128,37 @@ module SystemNoteService
 
   # Called when 'merge when pipeline succeeds' is executed
   def merge_when_pipeline_succeeds(noteable, project, author, sha)
-    body = "enabled an automatic merge when the pipeline for #{sha} succeeds"
-
-    create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
+    ::SystemNotes::MergeRequestsService.new(noteable: noteable, project: project, author: author).merge_when_pipeline_succeeds(sha)
   end
 
   # Called when 'merge when pipeline succeeds' is canceled
   def cancel_merge_when_pipeline_succeeds(noteable, project, author)
-    body = 'canceled the automatic merge'
-
-    create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
+    ::SystemNotes::MergeRequestsService.new(noteable: noteable, project: project, author: author).cancel_merge_when_pipeline_succeeds
   end
 
   # Called when 'merge when pipeline succeeds' is aborted
   def abort_merge_when_pipeline_succeeds(noteable, project, author, reason)
-    body = "aborted the automatic merge because #{reason}"
-
-    ##
-    # TODO: Abort message should be sent by the system, not a particular user.
-    # See https://gitlab.com/gitlab-org/gitlab-foss/issues/63187.
-    create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
+    ::SystemNotes::MergeRequestsService.new(noteable: noteable, project: project, author: author).abort_merge_when_pipeline_succeeds(reason)
   end
 
   def handle_merge_request_wip(noteable, project, author)
-    prefix = noteable.work_in_progress? ? "marked" : "unmarked"
-
-    body = "#{prefix} as a **Work In Progress**"
-
-    create_note(NoteSummary.new(noteable, project, author, body, action: 'title'))
+    ::SystemNotes::MergeRequestsService.new(noteable: noteable, project: project, author: author).handle_merge_request_wip
   end
 
   def add_merge_request_wip_from_commit(noteable, project, author, commit)
-    body = "marked as a **Work In Progress** from #{commit.to_reference(project)}"
-
-    create_note(NoteSummary.new(noteable, project, author, body, action: 'title'))
+    ::SystemNotes::MergeRequestsService.new(noteable: noteable, project: project, author: author).add_merge_request_wip_from_commit(commit)
   end
 
   def resolve_all_discussions(merge_request, project, author)
-    body = "resolved all threads"
-
-    create_note(NoteSummary.new(merge_request, project, author, body, action: 'discussion'))
+    ::SystemNotes::MergeRequestsService.new(noteable: merge_request, project: project, author: author).resolve_all_discussions
   end
 
   def discussion_continued_in_issue(discussion, project, author, issue)
-    body = "created #{issue.to_reference} to continue this discussion"
-    note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body)
-
-    note = Note.create(note_attributes.merge(system: true, created_at: issue.system_note_timestamp))
-    note.system_note_metadata = SystemNoteMetadata.new(action: 'discussion')
-
-    note
+    ::SystemNotes::MergeRequestsService.new(project: project, author: author).discussion_continued_in_issue(discussion, issue)
   end
 
   def diff_discussion_outdated(discussion, project, author, change_position)
-    merge_request = discussion.noteable
-    diff_refs = change_position.diff_refs
-    version_index = merge_request.merge_request_diffs.viewable.count
-    position_on_text = change_position.on_text?
-    text_parts = ["changed this #{position_on_text ? 'line' : 'file'} in"]
-
-    if version_params = merge_request.version_params_for(diff_refs)
-      repository = project.repository
-      anchor = position_on_text ? change_position.line_code(repository) : change_position.file_hash
-      url = url_helpers.diffs_project_merge_request_path(project, merge_request, version_params.merge(anchor: anchor))
-
-      text_parts << "[version #{version_index} of the diff](#{url})"
-    else
-      text_parts << "version #{version_index} of the diff"
-    end
-
-    body = text_parts.join(' ')
-    note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body)
-
-    note = Note.create(note_attributes.merge(system: true))
-    note.system_note_metadata = SystemNoteMetadata.new(action: 'outdated')
-
-    note
+    ::SystemNotes::MergeRequestsService.new(project: project, author: author).diff_discussion_outdated(discussion, change_position)
   end
 
   def change_title(noteable, project, author, old_title)
@@ -233,9 +188,7 @@ module SystemNoteService
   #
   # Returns the created Note object
   def change_branch(noteable, project, author, branch_type, old_branch, new_branch)
-    body = "changed #{branch_type} branch from `#{old_branch}` to `#{new_branch}`"
-
-    create_note(NoteSummary.new(noteable, project, author, body, action: 'branch'))
+    ::SystemNotes::MergeRequestsService.new(noteable: noteable, project: project, author: author).change_branch(branch_type, old_branch, new_branch)
   end
 
   # Called when a branch in Noteable is added or deleted
@@ -253,16 +206,7 @@ module SystemNoteService
   #
   # Returns the created Note object
   def change_branch_presence(noteable, project, author, branch_type, branch, presence)
-    verb =
-      if presence == :add
-        'restored'
-      else
-        'deleted'
-      end
-
-    body = "#{verb} #{branch_type} branch `#{branch}`"
-
-    create_note(NoteSummary.new(noteable, project, author, body, action: 'branch'))
+    ::SystemNotes::MergeRequestsService.new(noteable: noteable, project: project, author: author).change_branch_presence(branch_type, branch, presence)
   end
 
   # Called when a branch is created from the 'new branch' button on a issue
@@ -270,18 +214,11 @@ module SystemNoteService
   #
   #   "created branch `201-issue-branch-button`"
   def new_issue_branch(issue, project, author, branch, branch_project: nil)
-    branch_project ||= project
-    link = url_helpers.project_compare_path(branch_project, from: branch_project.default_branch, to: branch)
-
-    body = "created branch [`#{branch}`](#{link}) to address this issue"
-
-    create_note(NoteSummary.new(issue, project, author, body, action: 'branch'))
+    ::SystemNotes::MergeRequestsService.new(noteable: issue, project: project, author: author).new_issue_branch(branch, branch_project: branch_project)
   end
 
   def new_merge_request(issue, project, author, merge_request)
-    body = "created merge request #{merge_request.to_reference(project)} to address this issue"
-
-    create_note(NoteSummary.new(issue, project, author, body, action: 'merge'))
+    ::SystemNotes::MergeRequestsService.new(noteable: issue, project: project, author: author).new_merge_request(merge_request)
   end
 
   def cross_reference(noteable, mentioner, author)

app/services/system_notes/merge_requests_service.rb

+# frozen_string_literal: true
+
+module SystemNotes
+  class MergeRequestsService < ::SystemNotes::BaseService
+    # Called when 'merge when pipeline succeeds' is executed
+    def merge_when_pipeline_succeeds(sha)
+      body = "enabled an automatic merge when the pipeline for #{sha} succeeds"
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
+    end
+
+    # Called when 'merge when pipeline succeeds' is canceled
+    def cancel_merge_when_pipeline_succeeds
+      body = 'canceled the automatic merge'
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
+    end
+
+    # Called when 'merge when pipeline succeeds' is aborted
+    def abort_merge_when_pipeline_succeeds(reason)
+      body = "aborted the automatic merge because #{reason}"
+
+      ##
+      # TODO: Abort message should be sent by the system, not a particular user.
+      # See https://gitlab.com/gitlab-org/gitlab-foss/issues/63187.
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
+    end
+
+    def handle_merge_request_wip
+      prefix = noteable.work_in_progress? ? "marked" : "unmarked"
+
+      body = "#{prefix} as a **Work In Progress**"
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'title'))
+    end
+
+    def add_merge_request_wip_from_commit(commit)
+      body = "marked as a **Work In Progress** from #{commit.to_reference(project)}"
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'title'))
+    end
+
+    def resolve_all_discussions
+      body = "resolved all threads"
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'discussion'))
+    end
+
+    def discussion_continued_in_issue(discussion, issue)
+      body = "created #{issue.to_reference} to continue this discussion"
+      note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body)
+
+      Note.create(note_attributes.merge(system: true, created_at: issue.system_note_timestamp)).tap do |note|
+        note.system_note_metadata = SystemNoteMetadata.new(action: 'discussion')
+      end
+    end
+
+    def diff_discussion_outdated(discussion, change_position)
+      merge_request = discussion.noteable
+      diff_refs = change_position.diff_refs
+      version_index = merge_request.merge_request_diffs.viewable.count
+      position_on_text = change_position.on_text?
+      text_parts = ["changed this #{position_on_text ? 'line' : 'file'} in"]
+
+      if version_params = merge_request.version_params_for(diff_refs)
+        repository = project.repository
+        anchor = position_on_text ? change_position.line_code(repository) : change_position.file_hash
+        url = url_helpers.diffs_project_merge_request_path(project, merge_request, version_params.merge(anchor: anchor))
+
+        text_parts << "[version #{version_index} of the diff](#{url})"
+      else
+        text_parts << "version #{version_index} of the diff"
+      end
+
+      body = text_parts.join(' ')
+      note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body)
+
+      Note.create(note_attributes.merge(system: true)).tap do |note|
+        note.system_note_metadata = SystemNoteMetadata.new(action: 'outdated')
+      end
+    end
+
+    # Called when a branch in Noteable is changed
+    #
+    # branch_type - 'source' or 'target'
+    # old_branch  - old branch name
+    # new_branch  - new branch name
+    #
+    # Example Note text:
+    #
+    #   "changed target branch from `Old` to `New`"
+    #
+    # Returns the created Note object
+    def change_branch(branch_type, old_branch, new_branch)
+      body = "changed #{branch_type} branch from `#{old_branch}` to `#{new_branch}`"
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'branch'))
+    end
+
+    # Called when a branch in Noteable is added or deleted
+    #
+    # branch_type - :source or :target
+    # branch      - branch name
+    # presence    - :add or :delete
+    #
+    # Example Note text:
+    #
+    #   "restored target branch `feature`"
+    #
+    # Returns the created Note object
+    def change_branch_presence(branch_type, branch, presence)
+      verb =
+        if presence == :add
+          'restored'
+        else
+          'deleted'
+        end
+
+      body = "#{verb} #{branch_type} branch `#{branch}`"
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'branch'))
+    end
+
+    # Called when a branch is created from the 'new branch' button on a issue
+    # Example note text:
+    #
+    #   "created branch `201-issue-branch-button`"
+    def new_issue_branch(branch, branch_project: nil)
+      branch_project ||= project
+      link = url_helpers.project_compare_path(branch_project, from: branch_project.default_branch, to: branch)
+
+      body = "created branch [`#{branch}`](#{link}) to address this issue"
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'branch'))
+    end
+
+    def new_merge_request(merge_request)
+      body = "created merge request #{merge_request.to_reference(project)} to address this issue"
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
+    end
+  end
+end
+
+SystemNotes::MergeRequestsService.prepend_if_ee('::EE::SystemNotes::MergeRequestsService')

ee/app/services/ee/system_note_service.rb

@@ -125,15 +125,11 @@ module EE
     #
     # Returns the created Note object
     def approve_mr(noteable, user)
-      body = "approved this merge request"
-
-      create_note(NoteSummary.new(noteable, noteable.project, user, body, action: 'approved'))
+      ::SystemNotes::MergeRequestsService.new(noteable: noteable, project: noteable.project, author: user).approve_mr
     end
 
     def unapprove_mr(noteable, user)
-      body = "unapproved this merge request"
-
-      create_note(NoteSummary.new(noteable, noteable.project, user, body, action: 'unapproved'))
+      ::SystemNotes::MergeRequestsService.new(noteable: noteable, project: noteable.project, author: user).unapprove_mr
     end
 
     # Called when the weight of a Noteable is changed

ee/app/services/ee/system_notes/merge_requests_service.rb

+# frozen_string_literal: true
+
+module EE
+  module SystemNotes
+    module MergeRequestsService
+      # Called when the merge request is approved by user
+      #
+      # Example Note text:
+      #
+      #   "approved this merge request"
+      #
+      # Returns the created Note object
+      def approve_mr
+        body = "approved this merge request"
+
+        create_note(NoteSummary.new(noteable, project, author, body, action: 'approved'))
+      end
+
+      def unapprove_mr
+        body = "unapproved this merge request"
+
+        create_note(NoteSummary.new(noteable, project, author, body, action: 'unapproved'))
+      end
+    end
+  end
+end

ee/spec/services/ee/system_notes/merge_requests_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ::SystemNotes::MergeRequestsService do
+  let(:author) { create(:user) }
+  let(:project) { create(:project) }
+  let(:noteable) { create(:merge_request, source_project: project) }
+
+  describe '#approve_mr' do
+    subject { described_class.new(noteable: noteable, project: project, author: author).approve_mr }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'approved' }
+    end
+
+    context 'when merge request approved' do
+      it 'sets the note text' do
+        expect(subject.note).to eq "approved this merge request"
+      end
+    end
+  end
+
+  describe '#unapprove_mr' do
+    subject { described_class.new(noteable: noteable, project: project, author: author).unapprove_mr }
+
+    it_behaves_like 'a system note', exclude_project: true do
+      let(:action) { 'unapproved' }
+    end
+
+    context 'when merge request approved' do
+      it 'sets the note text' do
+        expect(subject.note).to eq "unapproved this merge request"
+      end
+    end
+  end
+end

ee/spec/services/system_note_service_spec.rb

@@ -161,32 +161,22 @@ describe SystemNoteService do
   end
 
   describe '.approve_mr' do
-    let(:noteable) { create(:merge_request, source_project: project) }
-    subject { described_class.approve_mr(noteable, author) }
-
-    it_behaves_like 'a system note' do
-      let(:action) { 'approved' }
+    it 'calls MergeRequestsService' do
+      expect_next_instance_of(::SystemNotes::MergeRequestsService) do |service|
+        expect(service).to receive(:approve_mr)
       end
 
-    context 'when merge request approved' do
-      it 'sets the note text' do
-        expect(subject.note).to eq "approved this merge request"
-      end
+      described_class.approve_mr(noteable, author)
     end
   end
 
   describe '.unapprove_mr' do
-    let(:noteable) { create(:merge_request, source_project: project) }
-    subject { described_class.unapprove_mr(noteable, author) }
-
-    it_behaves_like 'a system note', exclude_project: true do
-      let(:action) { 'unapproved' }
+    it 'calls MergeRequestsService' do
+      expect_next_instance_of(::SystemNotes::MergeRequestsService) do |service|
+        expect(service).to receive(:unapprove_mr)
       end
 
-    context 'when merge request approved' do
-      it 'sets the note text' do
-        expect(subject.note).to eq "unapproved this merge request"
-      end
+      described_class.unapprove_mr(noteable, author)
     end
   end
 

spec/services/system_notes/merge_requests_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ::SystemNotes::MergeRequestsService do
+  include Gitlab::Routing
+
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, :repository, group: group) }
+  let_it_be(:author) { create(:user) }
+
+  let(:noteable) { create(:merge_request, source_project: project, target_project: project) }
+
+  let(:service) { described_class.new(noteable: noteable, project: project, author: author) }
+
+  describe '.merge_when_pipeline_succeeds' do
+    let(:pipeline) { build(:ci_pipeline_without_jobs )}
+
+    subject { service.merge_when_pipeline_succeeds(pipeline.sha) }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'merge' }
+    end
+
+    it "posts the 'merge when pipeline succeeds' system note" do
+      expect(subject.note).to match(%r{enabled an automatic merge when the pipeline for (\w+/\w+@)?\h{40} succeeds})
+    end
+  end
+
+  describe '.cancel_merge_when_pipeline_succeeds' do
+    subject { service.cancel_merge_when_pipeline_succeeds }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'merge' }
+    end
+
+    it "posts the 'merge when pipeline succeeds' system note" do
+      expect(subject.note).to eq "canceled the automatic merge"
+    end
+  end
+
+  describe '.abort_merge_when_pipeline_succeeds' do
+    subject { service.abort_merge_when_pipeline_succeeds('merge request was closed') }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'merge' }
+    end
+
+    it "posts the 'merge when pipeline succeeds' system note" do
+      expect(subject.note).to eq "aborted the automatic merge because merge request was closed"
+    end
+  end
+
+  describe '.handle_merge_request_wip' do
+    context 'adding wip note' do
+      let(:noteable) { create(:merge_request, source_project: project, title: 'WIP Lorem ipsum') }
+
+      subject { service.handle_merge_request_wip }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'title' }
+      end
+
+      it 'sets the note text' do
+        expect(subject.note).to eq 'marked as a **Work In Progress**'
+      end
+    end
+
+    context 'removing wip note' do
+      subject { service.handle_merge_request_wip }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'title' }
+      end
+
+      it 'sets the note text' do
+        expect(subject.note).to eq 'unmarked as a **Work In Progress**'
+      end
+    end
+  end
+
+  describe '.add_merge_request_wip_from_commit' do
+    subject { service.add_merge_request_wip_from_commit(noteable.diff_head_commit) }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'title' }
+    end
+
+    it "posts the 'marked as a Work In Progress from commit' system note" do
+      expect(subject.note).to match(
+        /marked as a \*\*Work In Progress\*\* from #{Commit.reference_pattern}/
+      )
+    end
+  end
+
+  describe '.resolve_all_discussions' do
+    subject { service.resolve_all_discussions }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'discussion' }
+    end
+
+    it 'sets the note text' do
+      expect(subject.note).to eq 'resolved all threads'
+    end
+  end
+
+  describe '.diff_discussion_outdated' do
+    let(:discussion) { create(:diff_note_on_merge_request, project: project).to_discussion }
+    let(:merge_request) { discussion.noteable }
+    let(:change_position) { discussion.position }
+
+    def reloaded_merge_request
+      MergeRequest.find(merge_request.id)
+    end
+
+    let(:service) { described_class.new(project: project, author: author) }
+
+    subject { service.diff_discussion_outdated(discussion, change_position) }
+
+    it_behaves_like 'a system note' do
+      let(:expected_noteable) { discussion.first_note.noteable }
+      let(:action)            { 'outdated' }
+    end
+
+    context 'when the change_position is valid for the discussion' do
+      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 { subject }.to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
+      end
+
+      it 'links to the diff in the system note' do
+        diff_id = merge_request.merge_request_diff.id
+        line_code = change_position.line_code(project.repository)
+        link = diffs_project_merge_request_path(project, merge_request, diff_id: diff_id, anchor: line_code)
+
+        expect(subject.note).to eq("changed this line in [version 1 of the diff](#{link})")
+      end
+
+      context 'discussion is on an image' do
+        let(:discussion) { create(:image_diff_note_on_merge_request, project: project).to_discussion }
+
+        it 'links to the diff in the system note' do
+          diff_id = merge_request.merge_request_diff.id
+          file_hash = change_position.file_hash
+          link = diffs_project_merge_request_path(project, merge_request, diff_id: diff_id, anchor: file_hash)
+
+          expect(subject.note).to eq("changed this file in [version 1 of the diff](#{link})")
+        end
+      end
+    end
+
+    context 'when the change_position does not point to a valid version' do
+      before do
+        allow(merge_request).to receive(:version_params_for).and_return(nil)
+      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 { subject }.to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
+      end
+
+      it 'does not create a link' do
+        expect(subject.note).to eq('changed this line in version 1 of the diff')
+      end
+    end
+  end
+
+  describe '.change_branch' do
+    subject { service.change_branch('target', old_branch, new_branch) }
+
+    let(:old_branch) { 'old_branch'}
+    let(:new_branch) { 'new_branch'}
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'branch' }
+    end
+
+    context 'when target branch name changed' do
+      it 'sets the note text' do
+        expect(subject.note).to eq "changed target branch from `#{old_branch}` to `#{new_branch}`"
+      end
+    end
+  end
+
+  describe '.change_branch_presence' do
+    subject { service.change_branch_presence(:source, 'feature', :delete) }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'branch' }
+    end
+
+    context 'when source branch deleted' do
+      it 'sets the note text' do
+        expect(subject.note).to eq "deleted source branch `feature`"
+      end
+    end
+  end
+
+  describe '.new_issue_branch' do
+    let(:branch) { '1-mepmep' }
+
+    subject { service.new_issue_branch(branch, branch_project: branch_project) }
+
+    shared_examples_for 'a system note for new issue branch' do
+      it_behaves_like 'a system note' do
+        let(:action) { 'branch' }
+      end
+
+      context 'when a branch is created from the new branch button' do
+        it 'sets the note text' do
+          expect(subject.note).to start_with("created branch [`#{branch}`]")
+        end
+      end
+    end
+
+    context 'branch_project is set' do
+      let(:branch_project) { create(:project, :repository) }
+
+      it_behaves_like 'a system note for new issue branch'
+    end
+
+    context 'branch_project is not set' do
+      let(:branch_project) { nil }
+
+      it_behaves_like 'a system note for new issue branch'
+    end
+  end
+
+  describe '.new_merge_request' do
+    subject { service.new_merge_request(merge_request) }
+
+    let!(:merge_request) { create(:merge_request, source_project: project, source_branch: generate(:branch), target_project: project) }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'merge' }
+    end
+
+    it 'sets the new merge request note text' do
+      expect(subject.note).to eq("created merge request #{merge_request.to_reference(project)} to address this issue")
+    end
+  end
+end