Merge branch '7329-add-epic-relationships-system-notes' into 'master'

System notes for adding and removing epic relationships

Closes #7329

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

ee/app/controllers/groups/epic_links_controller.rb

@@ -12,7 +12,7 @@ class Groups::EpicLinksController < Groups::ApplicationController
   end
 
   def destroy
-    result = ::Epics::UpdateService.new(group, current_user, { parent: nil }).execute(child_epic)
+    result = ::EpicLinks::DestroyService.new(child_epic, current_user).execute
 
     render json: { issuables: issuables }, status: result[:http_status]
   end

ee/app/helpers/ee/system_note_helper.rb

@@ -16,7 +16,9 @@ module EE
       'issue_removed_from_epic' => 'epic',
       'issue_changed_epic' => 'epic',
       'epic_date_changed' => 'calendar',
-      'weight' => 'weight'
+      'weight' => 'weight',
+      'relate_epic' => 'epic',
+      'unrelate_epic' => 'epic'
     }.freeze
 
     override :system_note_icon_name

ee/app/models/ee/system_note_metadata.rb

@@ -7,7 +7,7 @@ module EE
     EE_ICON_TYPES = %w[
       weight approved unapproved relate unrelate
       epic_issue_added issue_added_to_epic epic_issue_removed issue_removed_from_epic
-      epic_issue_moved issue_changed_epic epic_date_changed
+      epic_issue_moved issue_changed_epic epic_date_changed relate_epic unrelate_epic
     ].freeze
 
     EE_TYPES_WITH_CROSS_REFERENCES = %w[

ee/app/services/ee/system_note_service.rb

@@ -172,5 +172,22 @@ module EE
 
       create_note(NoteSummary.new(noteable, nil, author, body, action: 'epic_date_changed'))
     end
+
+    def change_epics_relation(epic, child_epic, user, type)
+      note_body = if type == 'relate_epic'
+                    "added epic %{target_epic_ref} as %{direction} epic"
+                  else
+                    "removed %{direction} epic %{target_epic_ref}"
+                  end
+
+      change_epics_relation_act(epic, user, type, note_body,
+                                { direction: 'child', target_epic_ref: child_epic.to_reference(epic.group) })
+      change_epics_relation_act(child_epic, user, type, note_body,
+                                { direction: 'parent', target_epic_ref: epic.to_reference(child_epic.group) })
+    end
+
+    def change_epics_relation_act(subject_epic, user, action, text, text_params)
+      create_note(NoteSummary.new(subject_epic, nil, user, text % text_params, action: action))
+    end
   end
 end

ee/app/services/epic_links/create_service.rb

@@ -19,6 +19,12 @@ module EpicLinks
       set_child_epic!(referenced_epic)
 
       affected_epics.each(&:update_start_and_due_dates)
+
+      yield
+    end
+
+    def create_notes(referenced_epic, params)
+      SystemNoteService.change_epics_relation(issuable, referenced_epic, current_user, 'relate_epic')
     end
 
     def set_child_epic!(child_epic)

ee/app/services/epic_links/destroy_service.rb

+# frozen_string_literal: true
+
+module EpicLinks
+  class DestroyService < IssuableLinks::DestroyService
+    attr_reader :child_epic, :parent_epic
+    private :child_epic, :parent_epic
+
+    def initialize(child_epic, user)
+      @child_epic = child_epic
+      @parent_epic = child_epic&.parent
+      @current_user = user
+    end
+
+    private
+
+    def remove_relation
+      child_epic.update!({ parent_id: nil, updated_by: current_user })
+    end
+
+    def create_notes
+      return unless parent_epic
+
+      SystemNoteService.change_epics_relation(parent_epic, child_epic, current_user, 'unrelate_epic')
+    end
+
+    def permission_to_remove_relation?
+      child_epic.present? &&
+        parent_epic.present? &&
+        can?(current_user, :admin_epic, parent_epic) &&
+        can?(current_user, :admin_epic, child_epic)
+    end
+
+    def not_found_message
+      'No Epic found for given params'
+    end
+  end
+end

ee/app/services/issuable_links/destroy_service.rb

@@ -10,7 +10,7 @@ module IssuableLinks
     end
 
     def execute
-      return error('No Issue Link found', 404) unless permission_to_remove_relation?
+      return error(not_found_message, 404) unless permission_to_remove_relation?
 
       remove_relation
       create_notes
@@ -23,5 +23,9 @@ module IssuableLinks
     def remove_relation
       link.destroy!
     end
+
+    def not_found_message
+      'No Issue Link found'
+    end
   end
 end

ee/changelogs/unreleased/7329-add-epic-relationships-system-notes.yml

+---
+title: System notes for adding and removing epic relationships
+merge_request: 11416
+author:
+type: added

ee/spec/services/epic_links/create_service_spec.rb

@@ -11,6 +11,12 @@ describe EpicLinks::CreateService, :postgresql do
 
     let(:valid_reference) { epic_to_add.to_reference(full: true) }
 
+    shared_examples 'system notes created' do
+      it 'creates system notes' do
+        expect { subject }.to change { Note.system.count }.from(0).to(2)
+      end
+    end
+
     shared_examples 'returns success' do
       it 'creates a new relationship and updates epic' do
         expect { subject }.to change { epic.children.count }.by(1)
@@ -73,6 +79,7 @@ describe EpicLinks::CreateService, :postgresql do
           subject { add_epic([valid_reference]) }
 
           include_examples 'returns success'
+          include_examples 'system notes created'
         end
 
         context 'when an epic from a subgroup is given' do
@@ -85,6 +92,7 @@ describe EpicLinks::CreateService, :postgresql do
           subject { add_epic([valid_reference]) }
 
           include_examples 'returns success'
+          include_examples 'system notes created'
         end
 
         context 'when an epic from a another group is given' do
@@ -145,17 +153,16 @@ describe EpicLinks::CreateService, :postgresql do
             expect(epic.reload.children).to match_array([epic_to_add, another_epic])
           end
 
+          it 'creates system notes' do
+            expect { subject }.to change { Note.system.count }.from(0).to(4)
+          end
+
           it 'returns success status' do
             expect(subject).to eq(status: :success)
           end
 
           it 'avoids un-necessary database queries' do
-            epic1 = create(:epic, group: group)
-
-            # Establish baseline
-            add_epic([valid_reference])
-
-            control = ActiveRecord::QueryRecorder.new { add_epic([epic1.to_reference(full: true)]) }
+            control = ActiveRecord::QueryRecorder.new { add_epic([valid_reference]) }
 
             new_epics = [create(:epic, group: group), create(:epic, group: group)]
 
@@ -190,6 +197,10 @@ describe EpicLinks::CreateService, :postgresql do
             expect(epic.reload.children).to match_array([epic_to_add, another_epic])
           end
 
+          it 'creates system notes' do
+            expect { subject }.to change { Note.system.count }.from(0).to(2)
+          end
+
           it 'returns success status' do
             expect(subject).to eq(status: :success)
           end
@@ -211,6 +222,56 @@ describe EpicLinks::CreateService, :postgresql do
           end
         end
 
+        context 'when adding to an Epic that is already at maximum depth' do
+          before do
+            epic1 = create(:epic, group: group)
+            epic2 = create(:epic, group: group, parent: epic1)
+            epic3 = create(:epic, group: group, parent: epic2)
+            epic4 = create(:epic, group: group, parent: epic3)
+
+            epic.update(parent: epic4)
+          end
+
+          subject { add_epic([valid_reference]) }
+
+          it 'returns an error' do
+            expect(subject).to eq(message: 'Epic hierarchy level too deep', status: :error, http_status: 409)
+          end
+
+          it 'no relationship is created' do
+            expect { subject }.not_to change { epic.children.count }
+          end
+        end
+
+        context 'when adding an Epic that has existing children' do
+          subject { add_epic([valid_reference]) }
+
+          context 'when total depth after adding would exceed limit' do
+            before do
+              epic1 = create(:epic, group: group)
+
+              epic.update(parent: epic1) # epic is on level 2
+
+              # epic_to_add has 3 children (level 4 including epic_to_add)
+              # that would mean level 6 after relating epic_to_add on epic
+              epic2 = create(:epic, group: group, parent: epic_to_add)
+              epic3 = create(:epic, group: group, parent: epic2)
+              create(:epic, group: group, parent: epic3)
+            end
+
+            include_examples 'returns not found error'
+          end
+
+          context 'when Epic to add has more than 5 children' do
+            before do
+              create_list(:epic, 8, group: group, parent: epic_to_add)
+            end
+
+            include_examples 'returns success'
+            include_examples 'system notes created'
+          end
+        end
+
         context 'when an epic is already assigned to another epic' do
           let(:another_epic) { create(:epic, group: group) }
 
@@ -221,6 +282,7 @@ describe EpicLinks::CreateService, :postgresql do
           subject { add_epic([valid_reference]) }
 
           include_examples 'returns success'
+          include_examples 'system notes created'
         end
       end
     end

ee/spec/services/epic_links/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EpicLinks::DestroyService, :postgresql do
+  describe '#execute' do
+    let(:group) { create(:group) }
+    let(:user) { create(:user) }
+    let(:epic) { create(:epic, group: group) }
+    let!(:child_epic) { create(:epic, parent: epic, group: group) }
+
+    shared_examples 'system notes created' do
+      it 'creates system notes' do
+        expect { subject }.to change { Note.system.count }.from(0).to(2)
+      end
+    end
+
+    shared_examples 'returns success' do
+      it 'removes epic relationship' do
+        expect { subject }.to change { epic.children.count }.by(-1)
+
+        expect(epic.reload.children).not_to include(child_epic)
+      end
+
+      it 'returns success status' do
+        expect(subject).to eq(message: 'Relation was removed', status: :success)
+      end
+    end
+
+    shared_examples 'returns not found error' do
+      it 'returns an error' do
+        expect(subject).to eq(message: 'No Epic found for given params', status: :error, http_status: 404)
+      end
+
+      it 'no relationship is created' do
+        expect { subject }.not_to change { epic.children.count }
+      end
+
+      it 'does not create system notes' do
+        expect { subject }.not_to change { Note.system.count }
+      end
+    end
+
+    def remove_epic_relation(child_epic)
+      described_class.new(child_epic, user).execute
+    end
+
+    context 'when epics feature is disabled' do
+      subject { remove_epic_relation(child_epic) }
+
+      include_examples 'returns not found error'
+    end
+
+    context 'when epics feature is enabled' do
+      before do
+        stub_licensed_features(epics: true)
+      end
+
+      context 'when the user has no permissions to remove epic relation' do
+        subject { remove_epic_relation(child_epic) }
+
+        include_examples 'returns not found error'
+      end
+
+      context 'when user has permissions to remove epic relation' do
+        before do
+          group.add_developer(user)
+        end
+
+        context 'when the child epic is nil' do
+          subject { remove_epic_relation(nil) }
+
+          include_examples 'returns not found error'
+        end
+
+        context 'when a correct reference is given' do
+          subject { remove_epic_relation(child_epic) }
+
+          include_examples 'returns success'
+          include_examples 'system notes created'
+        end
+
+        context 'when epic has no parent' do
+          subject { remove_epic_relation(epic) }
+
+          include_examples 'returns not found error'
+        end
+      end
+    end
+  end
+end

ee/spec/services/system_note_service_spec.rb

@@ -257,4 +257,70 @@ describe SystemNoteService do
       end
     end
   end
+
+  describe '.relate_epic' do
+    let(:child_epic) { create(:epic, parent: epic, group: group) }
+    let(:noteable) { child_epic }
+
+    subject { described_class.change_epics_relation(epic, child_epic, author, 'relate_epic') }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'relate_epic' }
+    end
+
+    context 'when epic is added as child to a parent epic' do
+      it 'sets the note text' do
+        expect { subject }.to change { Note.system.count }.from(0).to(2)
+        expect(Note.first.note).to eq("added epic &#{child_epic.iid} as child epic")
+        expect(Note.last.note).to eq("added epic &#{epic.iid} as parent epic")
+      end
+    end
+
+    context 'when added epic is from a subgroup' do
+      let(:subgroup) {create(:group, parent: group)}
+
+      before do
+        child_epic.update!({ group: subgroup })
+      end
+
+      it 'sets the note text' do
+        expect { subject }.to change { Note.system.count }.from(0).to(2)
+        expect(Note.first.note).to eq("added epic #{group.path}/#{subgroup.path}&#{child_epic.iid} as child epic")
+        expect(Note.last.note).to eq("added epic #{group.path}&#{epic.iid} as parent epic")
+      end
+    end
+  end
+
+  describe '.unrelate_epic' do
+    let(:child_epic) { create(:epic, parent: epic, group: group) }
+    let(:noteable) { child_epic }
+
+    subject { described_class.change_epics_relation(epic, child_epic, author, 'unrelate_epic') }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'unrelate_epic' }
+    end
+
+    context 'when child epic is removed from a parent epic' do
+      it 'sets the note text' do
+        expect { subject }.to change { Note.system.count }.from(0).to(2)
+        expect(Note.first.note).to eq("removed child epic &#{child_epic.iid}")
+        expect(Note.last.note).to eq("removed parent epic &#{epic.iid}")
+      end
+    end
+
+    context 'when removed epic is from a subgroup' do
+      let(:subgroup) {create(:group, parent: group)}
+
+      before do
+        child_epic.update!({ group: subgroup })
+      end
+
+      it 'sets the note text' do
+        expect { subject }.to change { Note.system.count }.from(0).to(2)
+        expect(Note.first.note).to eq("removed child epic #{group.path}/#{subgroup.path}&#{child_epic.iid}")
+        expect(Note.last.note).to eq("removed parent epic #{group.path}&#{epic.iid}")
+      end
+    end
+  end
 end