Show validation error if link creation fails

When link creation fails due to validation error, then
an exception is raised and no reasonable error is displayed to
the user.

With this change we:
* try creating all links
* gather all errors and return them in the response

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
+
+    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 'valid relations are created' do
-        expect { subject }.to change(IssueLink, :count).from(1).to(2)
+      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] ""