Merge branch '3947-epic-system-notes' into 'master'

Add system notes for issue - epic association

Closes #3947

See merge request gitlab-org/gitlab-ee!3674

app/models/system_note_metadata.rb

@@ -16,6 +16,7 @@ class SystemNoteMetadata < ActiveRecord::Base
     opened closed merged duplicate locked unlocked
     outdated
     approved unapproved relate unrelate
+    epic_issue_added issue_added_to_epic epic_issue_removed issue_removed_from_epic
   ].freeze
 
   validates :note, presence: true

app/services/system_note_service.rb

@@ -566,6 +566,36 @@ module SystemNoteService
     create_note(NoteSummary.new(noteable, noteable.project, user, body, action: 'unrelate'))
   end
 
+  def epic_issue(epic, issue, user, type)
+    return unless validate_epic_issue_action_type(type)
+
+    action = type == :added ? 'epic_issue_added' : 'epic_issue_removed'
+
+    body = "#{type} issue #{issue.to_reference(epic.group)}"
+
+    create_note(NoteSummary.new(epic, nil, user, body, action: action))
+  end
+
+  def issue_on_epic(issue, epic, user, type)
+    return unless validate_epic_issue_action_type(type)
+
+    if type == :added
+      direction = 'to'
+      action = 'issue_added_to_epic'
+    else
+      direction = 'from'
+      action = 'issue_removed_from_epic'
+    end
+
+    body = "#{type} #{direction} epic #{epic.to_reference(issue.project)}"
+
+    create_note(NoteSummary.new(issue, issue.project, user, body, action: action))
+  end
+
+  def validate_epic_issue_action_type(type)
+    [:added, :removed].include?(type)
+  end
+
   # Called when the merge request is approved by user
   #
   # noteable - Noteable object

changelogs/unreleased-ee/3947-epic-system-notes.yml

+---
+title: Add system notes for issue - epic association
+merge_request:
+author:
+type: added

ee/app/services/epic_issues/create_service.rb

@@ -8,8 +8,9 @@ module EpicIssues
       link.save!
     end
 
-    def create_notes?
-      false
+    def create_notes(referenced_issue)
+      SystemNoteService.epic_issue(issuable, referenced_issue, current_user, :added)
+      SystemNoteService.issue_on_epic(referenced_issue, issuable, current_user, :added)
     end
 
     def extractor_context

ee/app/services/epic_issues/destroy_service.rb

@@ -2,10 +2,6 @@ module EpicIssues
   class DestroyService < IssuableLinks::DestroyService
     private
 
-    def create_notes?
-      false
-    end
-
     def source
       @source ||= link.epic
     end
@@ -17,5 +13,10 @@ module EpicIssues
     def permission_to_remove_relation?
       can?(current_user, :admin_epic_issue, target) && can?(current_user, :admin_epic, source)
     end
+
+    def create_notes
+      SystemNoteService.epic_issue(source, target, current_user, :removed)
+      SystemNoteService.issue_on_epic(target, source, current_user, :removed)
+    end
   end
 end

ee/app/services/issuable_links/create_service.rb

@@ -19,7 +19,7 @@ module IssuableLinks
 
     def create_issue_links
       referenced_issues.each do |referenced_issue|
-        create_notes(referenced_issue) if relate_issues(referenced_issue) && create_notes?
+        create_notes(referenced_issue) if relate_issues(referenced_issue)
       end
     end
 
@@ -49,19 +49,10 @@ module IssuableLinks
       extractor.issues
     end
 
-    def create_notes(referenced_issue)
-      SystemNoteService.relate_issue(issuable, referenced_issue, current_user)
-      SystemNoteService.relate_issue(referenced_issue, issuable, current_user)
-    end
-
     def extractor_context
       {}
     end
 
-    def create_notes?
-      true
-    end
-
     def linkable_issues(issues)
       raise NotImplementedError
     end

ee/app/services/issuable_links/destroy_service.rb

@@ -11,24 +11,15 @@ module IssuableLinks
       return error('No Issue Link found', 404) unless permission_to_remove_relation?
 
       remove_relation
-      create_notes if create_notes?
+      create_notes
 
       success(message: 'Relation was removed')
     end
 
     private
 
-    def create_notes
-      SystemNoteService.unrelate_issue(source, target, current_user)
-      SystemNoteService.unrelate_issue(target, source, current_user)
-    end
-
     def remove_relation
       link.destroy!
     end
-
-    def create_notes?
-      true
-    end
   end
 end

ee/app/services/issue_links/create_service.rb

@@ -7,5 +7,10 @@ module IssueLinks
     def linkable_issues(issues)
       issues.select { |issue| can?(current_user, :admin_issue_link, issue) }
     end
+
+    def create_notes(referenced_issue)
+      SystemNoteService.relate_issue(issuable, referenced_issue, current_user)
+      SystemNoteService.relate_issue(referenced_issue, issuable, current_user)
+    end
   end
 end

ee/app/services/issue_links/destroy_service.rb

@@ -13,5 +13,10 @@ module IssueLinks
     def permission_to_remove_relation?
       can?(current_user, :admin_issue_link, source) && can?(current_user, :admin_issue_link, target)
     end
+
+    def create_notes
+      SystemNoteService.unrelate_issue(source, target, current_user)
+      SystemNoteService.unrelate_issue(target, source, current_user)
+    end
   end
 end

spec/ee/spec/services/epic_issues/create_service_spec.rb

@@ -25,6 +25,32 @@ describe EpicIssues::CreateService do
       it 'returns success status' do
         expect(subject).to eq(status: :success)
       end
+
+      it 'creates 2 system notes' do
+        expect { subject }.to change { Note.count }.from(0).to(2)
+      end
+
+      it 'creates a note for epic correctly' do
+        subject
+        note = Note.find_by(noteable_id: epic.id, noteable_type: 'Epic')
+
+        expect(note.note).to eq("added issue #{issue.to_reference(epic.group)}")
+        expect(note.author).to eq(user)
+        expect(note.project).to be_nil
+        expect(note.noteable_type).to eq('Epic')
+        expect(note.system_note_metadata.action).to eq('epic_issue_added')
+      end
+
+      it 'creates a note for issue correctly' do
+        subject
+        note = Note.find_by(noteable_id: issue.id, noteable_type: 'Issue')
+
+        expect(note.note).to eq("added to epic #{epic.to_reference(issue.project)}")
+        expect(note.author).to eq(user)
+        expect(note.project).to eq(issue.project)
+        expect(note.noteable_type).to eq('Issue')
+        expect(note.system_note_metadata.action).to eq('issue_added_to_epic')
+      end
     end
 
     shared_examples 'returns an error' do
@@ -57,6 +83,10 @@ describe EpicIssues::CreateService do
           it 'returns an error' do
             expect(assign_issue([])).to eq(message: 'No Issue found for given params', status: :error, http_status: 404)
           end
+
+          it 'does not create a system note' do
+            expect { assign_issue([]) }.not_to change { Note.count }
+          end
         end
 
         context 'when there is an issue to relate' do
@@ -72,6 +102,9 @@ describe EpicIssues::CreateService do
             include_examples 'returns success'
 
             it 'does not perofrm N + 1 queries' do
+              allow(SystemNoteService).to receive(:epic_issue)
+              allow(SystemNoteService).to receive(:issue_on_epic)
+
               params = { issue_references: [valid_reference] }
               control_count = ActiveRecord::QueryRecorder.new { described_class.new(epic, user, params).execute }.count
 

spec/ee/spec/services/epic_issues/destroy_service_spec.rb

@@ -38,6 +38,32 @@ describe EpicIssues::DestroyService do
         it 'returns success message' do
           is_expected.to eq(message: 'Relation was removed', status: :success)
         end
+
+        it 'creates 2 system notes' do
+          expect { subject }.to change { Note.count }.from(0).to(2)
+        end
+
+        it 'creates a note for epic correctly' do
+          subject
+          note = Note.find_by(noteable_id: epic.id, noteable_type: 'Epic')
+
+          expect(note.note).to eq("removed issue #{issue.to_reference(epic.group)}")
+          expect(note.author).to eq(user)
+          expect(note.project).to be_nil
+          expect(note.noteable_type).to eq('Epic')
+          expect(note.system_note_metadata.action).to eq('epic_issue_removed')
+        end
+
+        it 'creates a note for issue correctly' do
+          subject
+          note = Note.find_by(noteable_id: issue.id, noteable_type: 'Issue')
+
+          expect(note.note).to eq("removed from epic #{epic.to_reference(issue.project)}")
+          expect(note.author).to eq(user)
+          expect(note.project).to eq(issue.project)
+          expect(note.noteable_type).to eq('Issue')
+          expect(note.system_note_metadata.action).to eq('issue_removed_from_epic')
+        end
       end
 
       context 'user does not have permissions to remove associations' do

spec/services/system_note_service_spec.rb

@@ -5,10 +5,11 @@ describe SystemNoteService do
   include Gitlab::Routing
 
   set(:group)    { create(:group) }
-  set(:project)  { create(:project, :repository, group: group) }
+  let(:project)  { create(:project, :repository, group: group) }
   set(:author)   { create(:user) }
   let(:noteable) { create(:issue, project: project) }
   let(:issue)    { noteable }
+  let(:epic)     { create(:epic) }
 
   shared_examples_for 'a system note' do
     let(:expected_noteable) { noteable }
@@ -1303,4 +1304,73 @@ describe SystemNoteService do
       end
     end
   end
+
+  describe '.epic_issue' do
+    let(:noteable) { epic }
+    let(:project) { nil }
+
+    context 'issue added to an epic' do
+      subject { described_class.epic_issue(epic, issue, author, :added)  }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'epic_issue_added' }
+      end
+
+      it 'creates the note text correctly' do
+        expect(subject.note).to eq("added issue #{issue.to_reference(epic.group)}")
+      end
+    end
+
+    context 'issue removed from an epic' do
+      subject { described_class.epic_issue(epic, issue, author, :removed)  }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'epic_issue_removed' }
+      end
+
+      it 'creates the note text correctly' do
+        expect(subject.note).to eq("removed issue #{issue.to_reference(epic.group)}")
+      end
+    end
+
+    context 'invalid type' do
+      it 'raises an error' do
+        expect { described_class.issue_on_epic(issue, epic, author, :invalid) }
+          .not_to change { Note.count }
+      end
+    end
+  end
+
+  describe '.issue_on_epic' do
+    context 'issue added to an epic' do
+      subject { described_class.issue_on_epic(issue, epic, author, :added)  }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'issue_added_to_epic' }
+      end
+
+      it 'creates the note text correctly' do
+        expect(subject.note).to eq("added to epic #{epic.to_reference(issue.project)}")
+      end
+    end
+
+    context 'issue removed from an epic' do
+      subject { described_class.issue_on_epic(issue, epic, author, :removed)  }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'issue_removed_from_epic' }
+      end
+
+      it 'creates the note text correctly' do
+        expect(subject.note).to eq("removed from epic #{epic.to_reference(issue.project)}")
+      end
+    end
+
+    context 'invalid type' do
+      it 'does not create a new note' do
+        expect { described_class.issue_on_epic(issue, epic, author, :invalid) }
+          .not_to change { Note.count }
+      end
+    end
+  end
 end