Small changes to the code, add changelog

changelogs/unreleased-ee/reorder-issues-epic.yml

+---
+title: Add support for reordering issues in epics
+merge_request:
+author:
+type: added

ee/app/services/epic_issues/create_service.rb

@@ -3,10 +3,10 @@ module EpicIssues
     private
 
     def relate_issues(referenced_issue)
-      link = existing_links.find { |link| link.issue == referenced_issue } || EpicIssue.new(issue: referenced_issue)
+      link = EpicIssue.find_or_initialize_by(issue: referenced_issue)
       link.epic = issuable
       link.move_to_start
-      link.save
+      link.save!
 
       link
     end
@@ -29,9 +29,5 @@ module EpicIssues
     def issuable_group_descendants
       @descendants ||= issuable.group.self_and_descendants
     end
-
-    def existing_links
-      @existing_links ||= EpicIssue.where(issue_id: referenced_issues.map(&:id))
-    end
   end
 end

ee/app/services/epic_issues/update_service.rb

@@ -12,15 +12,21 @@ module EpicIssues
       move_issue if params[:move_after_id] || params[:move_before_id]
       epic_issue.save!
       success
+    rescue ActiveRecord::RecordNotFound
+      return error('Epic issue not found for given params', 404)
     end
 
     private
 
     def move_issue
-      before_epic_issue = EpicIssue.find_by(id: params[:move_before_id])
-      after_epic_issue = EpicIssue.find_by(id: params[:move_after_id])
+      before_epic_issue = epic.epic_issues.find(params[:move_before_id]) if params[:move_before_id]
+      after_epic_issue = epic.epic_issues.find(params[:move_after_id]) if params[:move_after_id]
 
       epic_issue.move_between(before_epic_issue, after_epic_issue)
     end
+
+    def epic
+      epic_issue.epic
+    end
   end
 end

ee/app/services/issuable_links/create_service.rb

@@ -18,14 +18,11 @@ module IssuableLinks
     private
 
     def create_issue_links
-      @created_links = []
-
       referenced_issues.each do |referenced_issue|
         link = relate_issues(referenced_issue)
 
         next unless link.persisted?
 
-        @created_links << link
         create_notes(referenced_issue)
       end
     end

ee/lib/api/epic_issues.rb

@@ -14,16 +14,12 @@ module API
         authorize!(:admin_epic, epic)
       end
 
-      def check_epic_link!
-        forbidden! if link.epic != epic
-      end
-
       def epic
         @epic ||= user_group.epics.find_by(iid: params[:epic_iid])
       end
 
       def link
-        @link ||= EpicIssue.find(params[:epic_issue_id])
+        @link ||= epic.epic_issues.find(params[:epic_issue_id])
       end
     end
 
@@ -42,7 +38,6 @@ module API
       end
       put ':id/-/epics/:epic_iid/issues/:epic_issue_id' do
         authorize_can_admin!
-        check_epic_link!
 
         update_params = {
           move_before_id: params[:move_before_id],

spec/ee/spec/requests/api/epic_issues_spec.rb

@@ -47,14 +47,14 @@ describe API::EpicIssues do
           expect(response).to have_gitlab_http_status(403)
         end
 
-        it 'returns 403 forbidden error for the link of another epic' do
+        it 'returns 404 not found error for the link of another epic' do
           group.add_developer(user)
           another_epic = create(:epic, group: group)
           url = "/groups/#{group.path}/-/epics/#{another_epic.iid}/issues/#{epic_issue1.id}?move_after_id=#{epic_issue2.id}"
 
           put api(url, user)
 
-          expect(response).to have_gitlab_http_status(403)
+          expect(response).to have_gitlab_http_status(404)
         end
       end
 

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

@@ -136,12 +136,12 @@ describe EpicIssues::CreateService do
               allow(extractor).to receive(:issues).and_return(issues)
               params = { issue_references: issues.map { |i| i.to_reference(full: true) } }
 
-              # threshold 20 because 5 queries are generated for each insert
-              # (savepoint, exists, relative_position get, insert, release savepoint)
+              # threshold 24 because 6 queries are generated for each insert
+              # (savepoint, find, exists, relative_position get, insert, release savepoint)
               # and we insert 5 issues instead of 1 which we do for control count
               expect { described_class.new(epic, user, params).execute }
                 .not_to exceed_query_limit(control_count)
-                .with_threshold(20)
+                .with_threshold(24)
             end
           end
 

spec/ee/spec/services/epic_issues/list_service_spec.rb

@@ -44,7 +44,8 @@ describe EpicIssues::ListService do
               state: issue2.state,
               reference: issue2.to_reference(full: true),
               path: "/#{project.full_path}/issues/#{issue2.iid}",
-              relation_path: "/groups/#{group.full_path}/-/epics/#{epic.iid}/issues/#{epic_issue2.id}"
+              relation_path: "/groups/#{group.full_path}/-/epics/#{epic.iid}/issues/#{epic_issue2.id}",
+              epic_issue_id: epic_issue2.id
             },
             {
               id: issue1.id,
@@ -52,7 +53,8 @@ describe EpicIssues::ListService do
               state: issue1.state,
               reference: issue1.to_reference(full: true),
               path: "/#{project.full_path}/issues/#{issue1.iid}",
-              relation_path: "/groups/#{group.full_path}/-/epics/#{epic.iid}/issues/#{epic_issue1.id}"
+              relation_path: "/groups/#{group.full_path}/-/epics/#{epic.iid}/issues/#{epic_issue1.id}",
+              epic_issue_id: epic_issue1.id
             },
             {
               id: issue3.id,
@@ -60,7 +62,8 @@ describe EpicIssues::ListService do
               state: issue3.state,
               reference: issue3.to_reference(full: true),
               path: "/#{other_project.full_path}/issues/#{issue3.iid}",
-              relation_path: "/groups/#{group.full_path}/-/epics/#{epic.iid}/issues/#{epic_issue3.id}"
+              relation_path: "/groups/#{group.full_path}/-/epics/#{epic.iid}/issues/#{epic_issue3.id}",
+              epic_issue_id: epic_issue3.id
             }
           ]
           expect(subject).to eq(expected_result)
@@ -80,7 +83,8 @@ describe EpicIssues::ListService do
               state: issue2.state,
               reference: issue2.to_reference(full: true),
               path: "/#{project.full_path}/issues/#{issue2.iid}",
-              relation_path: nil
+              relation_path: nil,
+              epic_issue_id: epic_issue2.id
             },
             {
               id: issue1.id,
@@ -88,7 +92,8 @@ describe EpicIssues::ListService do
               state: issue1.state,
               reference: issue1.to_reference(full: true),
               path: "/#{project.full_path}/issues/#{issue1.iid}",
-              relation_path: nil
+              relation_path: nil,
+              epic_issue_id: epic_issue1.id
             }
           ]
 

spec/ee/spec/services/epic_issues/update_service_spec.rb

@@ -24,6 +24,29 @@ describe EpicIssues::UpdateService do
       EpicIssue.all.order('relative_position, id')
     end
 
+    context 'when moving issues between different epics' do
+      before do
+        epic_issue3.update_attribute(:epic, create(:epic, group: group))
+      end
+
+      let(:params) { { move_before_id: epic_issue3.id, move_after_id: epic_issue4.id } }
+
+      subject { order_issue(epic_issue1, params) }
+
+      it 'returns an error' do
+        is_expected.to eq(message: 'Epic issue not found for given params', status: :error, http_status: 404)
+      end
+
+      it 'does not change the relative_position values' do
+        subject
+
+        expect(epic_issue1.relative_position).to eq(3)
+        expect(epic_issue2.relative_position).to eq(600)
+        expect(epic_issue3.relative_position).to eq(1200)
+        expect(epic_issue4.relative_position).to eq(2000)
+      end
+    end
+
     context 'moving issue to the first position' do
       let(:params) { { move_after_id: epic_issue1.id } }