Merge branch 'issue_6944' into 'master'

Resolve: When you add an issue to an epic that is already in the epic, you get erroneous system notes

Closes #6944

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

ee/app/services/epic_issues/create_service.rb

@@ -42,9 +42,17 @@ module EpicIssues
     end
 
     def linkable_issues(issues)
-      return [] unless can?(current_user, :admin_epic, issuable.group)
+      @linkable_issues ||= begin
+        return [] unless can?(current_user, :admin_epic, issuable.group)
 
-      issues.select { |issue| issuable_group_descendants.include?(issue.project.group) }
+        issues.select do |issue|
+          issuable_group_descendants.include?(issue.project.group) && !previous_related_issues.include?(issue)
+        end
+      end
+    end
+
+    def previous_related_issues
+      @related_issues ||= issuable.issues.to_a
     end
 
     def issuable_group_descendants

ee/app/services/issuable_links/create_service.rb

@@ -7,7 +7,14 @@ module IssuableLinks
     end
 
     def execute
-      if referenced_issues.blank?
+      # If ALL referenced issues are already assigned to the given epic it renders a conflict status,
+      # otherwise create issue links for the issues which
+      # are still not assigned and return success message.
+      if render_conflict_error?
+        return error('Issue(s) already assigned', 409)
+      end
+
+      if render_not_found_error?
         return error('No Issue found for given params', 404)
       end
 
@@ -17,8 +24,18 @@ module IssuableLinks
 
     private
 
+    def render_conflict_error?
+      referenced_issues.present? && (referenced_issues - previous_related_issues).empty?
+    end
+
+    def render_not_found_error?
+      linkable_issues(referenced_issues).empty?
+    end
+
     def create_issue_links
-      referenced_issues.each do |referenced_issue|
+      issues = linkable_issues(referenced_issues)
+
+      issues.each do |referenced_issue|
         relate_issues(referenced_issue) do |params|
           create_notes(referenced_issue, params)
         end
@@ -29,15 +46,13 @@ module IssuableLinks
       @referenced_issues ||= begin
         target_issue = params[:target_issue]
 
-        issues = if params[:issue_references].present?
-                   extract_issues_from_references
-                 elsif target_issue
-                   [target_issue]
-                 else
-                   []
-                 end
-
-        linkable_issues(issues)
+        if params[:issue_references].present?
+          extract_issues_from_references
+        elsif target_issue
+          [target_issue]
+        else
+          []
+        end
       end
     end
 
@@ -59,6 +74,10 @@ module IssuableLinks
       raise NotImplementedError
     end
 
+    def previous_related_issues
+      raise NotImplementedError
+    end
+
     def relate_issues(referenced_issue)
       raise NotImplementedError
     end

ee/app/services/issue_links/create_service.rb

@@ -7,12 +7,18 @@ module IssueLinks
     end
 
     def linkable_issues(issues)
-      issues.select { |issue| can?(current_user, :admin_issue_link, issue) }
+      @linkable_issues ||= begin
+        issues.select { |issue| can?(current_user, :admin_issue_link, issue) }
+      end
     end
 
     def create_notes(referenced_issue, params)
       SystemNoteService.relate_issue(issuable, referenced_issue, current_user)
       SystemNoteService.relate_issue(referenced_issue, issuable, current_user)
     end
+
+    def previous_related_issues
+      @related_issues ||= issuable.related_issues(current_user).to_a
+    end
   end
 end

ee/changelogs/unreleased/issue_6944.yml

+---
+title: Do not allow to assign an issue to an epic twice
+merge_request: 8004
+author:
+type: fixed

ee/spec/services/epic_issues/create_service_spec.rb

@@ -24,7 +24,7 @@ describe EpicIssues::CreateService do
 
       it 'creates a new relationship and updates epic' do
         expect(epic).to receive(:update_start_and_due_dates)
-        expect { subject }.to change(EpicIssue, :count).from(1).to(2)
+        expect { subject }.to change(EpicIssue, :count).by(1)
 
         expect(created_link).to have_attributes(epic: epic)
       end
@@ -39,13 +39,13 @@ describe EpicIssues::CreateService do
         expect(subject).to eq(status: :success)
       end
 
-      it 'creates 2 system notes' do
-        expect { subject }.to change { Note.count }.from(0).to(2)
+      it 'creates 1 system note for epic and 1 system note for issue' do
+        expect { subject }.to change { Note.count }.by(2)
       end
 
       it 'creates a note for epic correctly' do
         subject
-        note = Note.find_by(noteable_id: epic.id, noteable_type: 'Epic')
+        note = Note.where(noteable_id: epic.id, noteable_type: 'Epic').last
 
         expect(note.note).to eq("added issue #{issue.to_reference(epic.group)}")
         expect(note.author).to eq(user)
@@ -199,6 +199,36 @@ describe EpicIssues::CreateService do
         include_examples 'returns an error'
       end
 
+      context 'when assigning issue(s) to the same epic' do
+        before do
+          group.add_developer(user)
+          assign_issue([valid_reference])
+          epic.reload
+        end
+
+        subject { assign_issue([valid_reference]) }
+
+        it 'no relationship is created' do
+          expect { subject }.not_to change { EpicIssue.count }
+        end
+
+        it 'does not create notes' do
+          expect { subject }.not_to change { Note.count }
+        end
+
+        it 'returns an error' do
+          expect(subject).to eq(message: 'Issue(s) already assigned', status: :error, http_status: 409)
+        end
+
+        context 'when at least one of the issues is still not assigned to the epic' do
+          let(:valid_reference) { issue2.to_reference(full: true) }
+
+          subject { assign_issue([valid_reference, issue.to_reference(full: true)]) }
+
+          include_examples 'returns success'
+        end
+      end
+
       context 'when an issue is already assigned to another epic' do
         before do
           group.add_developer(user)