Merge branch 'epic_link_message' into 'master'

Show validation error if link creation fails

See merge request gitlab-org/gitlab!33638

ee/app/services/epic_issues/create_service.rb

@@ -16,9 +16,12 @@ module EpicIssues
 
       link.epic = issuable
       link.move_to_start
-      link.save!
 
-      yield params
+      if link.save
+        create_notes(referenced_issue, params)
+      end
+
+      link
     end
     # rubocop: enable CodeReuse/ActiveRecord
 

ee/app/services/epic_links/create_service.rb

@@ -23,13 +23,11 @@ module EpicLinks
     def create_single_link
       child_epic = referenced_issuables.first
 
-      if linkable_epic?(child_epic)
-        set_child_epic!(child_epic)
-
-        create_notes(child_epic, nil)
+      if linkable_epic?(child_epic) && set_child_epic(child_epic)
+        create_notes(child_epic)
         success
       else
-        error(child_epic.errors.messages[:parent].first, 409)
+        error(child_epic.errors.values.flatten.to_sentence, 409)
       end
     end
 
@@ -41,19 +39,21 @@ module EpicLinks
       affected_epics = [issuable]
       affected_epics << referenced_epic if referenced_epic.parent
 
-      set_child_epic!(referenced_epic)
+      if set_child_epic(referenced_epic)
+        create_notes(referenced_epic)
+      end
 
-      yield
+      referenced_epic
     end
 
-    def create_notes(referenced_epic, params)
+    def create_notes(referenced_epic)
       SystemNoteService.change_epics_relation(issuable, referenced_epic, current_user, 'relate_epic')
     end
 
-    def set_child_epic!(child_epic)
+    def set_child_epic(child_epic)
       child_epic.parent = issuable
       child_epic.move_to_start
-      child_epic.save!
+      child_epic.save
     end
 
     def linkable_issuables(epics)

ee/app/services/issuable_links/create_service.rb

@@ -20,7 +20,13 @@ module IssuableLinks
         return error(issuables_not_found_message, 404)
       end
 
+      @errors = []
       create_links
+
+      if @errors.present?
+        return error(@errors.join('. '), 422)
+      end
+
       success
     end
 
@@ -36,20 +42,28 @@ module IssuableLinks
 
     def create_links
       objects = linkable_issuables(referenced_issuables)
-
       # it is important that this is not called after relate_issuables, as it relinks epic to the issuable
       # see EpicLinks::EpicIssues#relate_issuables
       affected_epics = affected_epics(objects)
 
-      objects.each do |referenced_object|
-        relate_issuables(referenced_object) do |params|
-          create_notes(referenced_object, params)
-        end
-      end
+      link_issuables(objects)
 
       Epics::UpdateDatesService.new(affected_epics).execute unless affected_epics.blank?
     end
 
+    def link_issuables(target_issuables)
+      target_issuables.each do |referenced_object|
+        link = relate_issuables(referenced_object)
+
+        unless link.valid?
+          @errors << _("%{ref} cannot be added: %{error}") % {
+            ref: referenced_object.to_reference,
+            error: link.errors.messages.values.flatten.to_sentence
+          }
+        end
+      end
+    end
+
     def referenced_issuables
       @referenced_issuables ||= begin
         target_issuable = params[:target_issuable]

ee/app/services/issue_links/create_service.rb

@@ -2,17 +2,21 @@
 
 module IssueLinks
   class CreateService < IssuableLinks::CreateService
+    # rubocop: disable CodeReuse/ActiveRecord
     def relate_issuables(referenced_issue)
-      attrs = { source: issuable, target: referenced_issue }
+      link = IssueLink.find_or_initialize_by(source: issuable, target: referenced_issue)
 
       if params[:link_type].present?
-        attrs[:link_type] = params[:link_type]
+        link.link_type = params[:link_type]
       end
 
-      link = IssueLink.create(attrs)
+      if link.changed? && link.save
+        create_notes(referenced_issue)
+      end
 
-      yield if link.persisted?
+      link
     end
+    # rubocop: enable CodeReuse/ActiveRecord
 
     def linkable_issuables(issues)
       @linkable_issuables ||= begin
@@ -20,7 +24,7 @@ module IssueLinks
       end
     end
 
-    def create_notes(referenced_issue, params)
+    def create_notes(referenced_issue)
       SystemNoteService.relate_issue(issuable, referenced_issue, current_user)
       SystemNoteService.relate_issue(referenced_issue, issuable, current_user)
     end

ee/changelogs/unreleased/epic_link_message.yml

+---
+title: Display validation errors when issue link creation fails
+merge_request: 33638
+author:
+type: fixed

ee/spec/services/epic_issues/create_service_spec.rb

@@ -13,8 +13,6 @@ RSpec.describe EpicIssues::CreateService do
     let(:user) { create(:user) }
     let(:valid_reference) { issue.to_reference(full: true) }
 
-    let!(:existing_link) { create(:epic_issue, epic: epic, issue: issue3) }
-
     def assign_issue(references)
       params = { issuable_references: references }
 
@@ -32,6 +30,8 @@ RSpec.describe EpicIssues::CreateService do
       end
 
       it 'orders the epic issue to the first place and moves the existing ones down' do
+        existing_link = create(:epic_issue, epic: epic, issue: issue3)
+
         subject
 
         expect(created_link.relative_position).to be < existing_link.reload.relative_position
@@ -175,13 +175,15 @@ RSpec.describe EpicIssues::CreateService do
             let(:created_link2) { EpicIssue.find_by!(issue_id: issue2.id) }
 
             it 'creates new relationships' do
-              expect { subject }.to change(EpicIssue, :count).from(1).to(3)
+              expect { subject }.to change { EpicIssue.count }.by(2)
 
               expect(created_link1).to have_attributes(epic: epic)
               expect(created_link2).to have_attributes(epic: epic)
             end
 
             it 'orders the epic issues to the first place and moves the existing ones down' do
+              existing_link = create(:epic_issue, epic: epic, issue: issue3)
+
               subject
 
               expect(created_link2.relative_position).to be < created_link1.relative_position
@@ -197,6 +199,32 @@ RSpec.describe EpicIssues::CreateService do
             end
           end
         end
+
+        context 'when there are invalid references' do
+          let(:epic) { create(:epic, confidential: true, group: group) }
+          let(:valid_issue) { create(:issue, :confidential, project: project) }
+          let(:invalid_issue1) { create(:issue, project: project) }
+          let(:invalid_issue2) { create(:issue, project: project) }
+
+          subject do
+            assign_issue([invalid_issue1.to_reference(full: true),
+                          valid_issue.to_reference(full: true),
+                          invalid_issue2.to_reference(full: true)])
+          end
+
+          it 'creates links only for valid references' do
+            expect { subject }.to change { EpicIssue.count }.by(1)
+          end
+
+          it 'returns error status' do
+            expect(subject).to eq(
+              status: :error,
+              http_status: 422,
+              message: "#{invalid_issue1.to_reference} cannot be added: Cannot set confidential epic for not-confidential issue. "\
+                       "#{invalid_issue2.to_reference} cannot be added: Cannot set confidential epic for not-confidential issue"
+            )
+          end
+        end
       end
 
       context 'when user does not have permissions to link the issue' do
@@ -251,7 +279,7 @@ RSpec.describe EpicIssues::CreateService do
         end
 
         it 'does not create a new association' do
-          expect { subject }.not_to change(EpicIssue, :count).from(2)
+          expect { subject }.not_to change(EpicIssue, :count)
         end
 
         it 'updates the existing association' do
@@ -263,7 +291,7 @@ RSpec.describe EpicIssues::CreateService do
         end
 
         it 'creates 3 system notes' do
-          expect { subject }.to change { Note.count }.from(0).to(3)
+          expect { subject }.to change { Note.count }.by(3)
         end
 
         it 'updates both old and new epic milestone dates' do

ee/spec/services/epic_links/create_service_spec.rb

@@ -258,6 +258,34 @@ RSpec.describe EpicLinks::CreateService do
 
               include_examples 'returns an error'
             end
+
+            context 'when there are invalid references' do
+              let(:epic) { create(:epic, confidential: true, group: group) }
+              let(:invalid_epic1) { create(:epic, group: group) }
+              let(:valid_epic) { create(:epic, :confidential, group: group) }
+              let(:invalid_epic2) { create(:epic, group: group) }
+
+              subject do
+                add_epic([invalid_epic1.to_reference(full: true),
+                          valid_epic.to_reference(full: true),
+                          invalid_epic2.to_reference(full: true)])
+              end
+
+              it 'adds only valid references' do
+                subject
+
+                expect(epic.reload.children).to match_array([valid_epic])
+              end
+
+              it 'returns error status' do
+                expect(subject).to eq(
+                  status: :error,
+                  http_status: 422,
+                  message: "#{invalid_epic1.to_reference} cannot be added: Not-confidential epic cannot be assigned to a confidential parent epic. "\
+                           "#{invalid_epic2.to_reference} cannot be added: Not-confidential epic cannot be assigned to a confidential parent epic"
+                )
+              end
+            end
           end
         end
       end

ee/spec/services/issue_links/create_service_spec.rb

@@ -125,23 +125,60 @@ RSpec.describe IssueLinks::CreateService do
     context 'when reference of any already related issue is present' do
       let(:issue_a) { create :issue, project: project }
       let(:issue_b) { create :issue, project: project }
+      let(:issue_c) { create :issue, project: project }
 
       before do
-        create :issue_link, source: issue, target: issue_a
+        create :issue_link, source: issue, target: issue_b, link_type: IssueLink::TYPE_RELATES_TO
+        create :issue_link, source: issue, target: issue_c, link_type: IssueLink::TYPE_IS_BLOCKED_BY
       end
 
       let(:params) do
-        { issuable_references: [issue_b.to_reference, issue_a.to_reference] }
+        {
+          issuable_references: [
+            issue_a.to_reference,
+            issue_b.to_reference,
+            issue_c.to_reference
+          ],
+          link_type: IssueLink::TYPE_IS_BLOCKED_BY
+        }
+      end
+
+      it 'creates notes only for new and changed relations' do
+        expect(SystemNoteService).to receive(:relate_issue).with(issue, issue_a, anything)
+        expect(SystemNoteService).to receive(:relate_issue).with(issue_a, issue, anything)
+        expect(SystemNoteService).to receive(:relate_issue).with(issue, issue_b, anything)
+        expect(SystemNoteService).to receive(:relate_issue).with(issue_b, issue, anything)
+        expect(SystemNoteService).not_to receive(:relate_issue).with(issue, issue_c, anything)
+        expect(SystemNoteService).not_to receive(:relate_issue).with(issue_c, issue, anything)
+
+        subject
       end
 
-      it 'returns success status' do
-        is_expected.to eq(status: :success)
+      it 'sets the same type of relation for selected references' do
+        expect(subject).to eq(status: :success)
+
+        expect(IssueLink.where(target: [issue_a, issue_b, issue_c]).pluck(:link_type))
+          .to eq([IssueLink::TYPE_IS_BLOCKED_BY, IssueLink::TYPE_IS_BLOCKED_BY, IssueLink::TYPE_IS_BLOCKED_BY])
+      end
     end
 
-      it 'valid relations are created' do
-        expect { subject }.to change(IssueLink, :count).from(1).to(2)
+    context 'when there are invalid references' do
+      let(:issue_a) { create :issue, project: project }
+
+      let(:params) do
+        { issuable_references: [issue.to_reference, issue_a.to_reference] }
+      end
+
+      it 'creates links only for valid references' do
+        expect { subject }.to change { IssueLink.count }.by(1)
+      end
 
-        expect(IssueLink.find_by!(target: issue_b)).to have_attributes(source: issue)
+      it 'returns error status' do
+        expect(subject).to eq(
+          status: :error,
+          http_status: 422,
+          message: "#{issue.to_reference} cannot be added: cannot be related to itself"
+        )
       end
     end
   end

locale/gitlab.pot

@@ -520,6 +520,9 @@ msgstr ""
 msgid "%{primary} (%{secondary})"
 msgstr ""
 
+msgid "%{ref} cannot be added: %{error}"
+msgstr ""
+
 msgid "%{releases} release"
 msgid_plural "%{releases} releases"
 msgstr[0] ""