Update EpicIssue position value & sort correctly

app/assets/javascripts/issuable/related_issues/components/related_issues_block.vue

@@ -98,7 +98,7 @@ export default {
 
       this.$emit('saveReorder', {
         issueId: parseInt(event.item.dataset.key, 10),
-        newOrder: event.newIndex + 1,
+        newOrder: event.newIndex,
       });
     },
     addDraggingCursor() {

config/routes/group.rb

@@ -85,11 +85,7 @@ constraints(GroupUrlConstrainer.new) do
         get :realtime_changes
       end
 
-      resources :epic_issues, only: [:index, :create, :destroy], as: 'issues', path: 'issues' do
-        member do
-          put :order
-        end
-      end
+      resources :epic_issues, only: [:index, :create, :destroy, :update], as: 'issues', path: 'issues'
     end
 
     legacy_ee_group_boards_redirect = redirect do |params, request|

ee/app/controllers/groups/epic_issues_controller.rb

@@ -3,12 +3,13 @@ class Groups::EpicIssuesController < Groups::EpicsController
 
   skip_before_action :authorize_destroy_issuable!
   skip_before_action :authorize_create_epic!
+  skip_before_action :authorize_update_issuable!
 
-  before_action :authorize_admin_epic!, only: [:create, :destroy, :order]
-  before_action :authorize_issue_link_association!, only: [:destroy, :order]
+  before_action :authorize_admin_epic!, only: [:create, :destroy, :update]
+  before_action :authorize_issue_link_association!, only: [:destroy, :update]
 
-  def order
-    result = EpicIssues::OrderService.new(link, current_user, params).execute
+  def update
+    result = EpicIssues::UpdateService.new(link, current_user, params).execute
 
     render json: { message: result[:message] }, status: result[:http_status]
   end

ee/app/services/epic_issues/create_service.rb

@@ -2,10 +2,19 @@ module EpicIssues
   class CreateService < IssuableLinks::CreateService
     private
 
+    def after_create
+      issuable.epic_issues
+        .where('id NOT IN (?)', @created_links.map(&:id))
+        .update_all("position = position + #{@created_links.count}")
+    end
+
     def relate_issues(referenced_issue)
-      link = EpicIssue.find_or_initialize_by(issue: referenced_issue)
+      link = existing_links.find { |link| link.issue == referenced_issue } || EpicIssue.new(issue: referenced_issue)
       link.epic = issuable
-      link.save!
+      link.position = @created_links.count + 1
+      link.save
+
+      link
     end
 
     def create_notes(referenced_issue)
@@ -26,5 +35,9 @@ 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/destroy_service.rb

@@ -2,6 +2,10 @@ module EpicIssues
   class DestroyService < IssuableLinks::DestroyService
     private
 
+    def after_remove
+      source.epic_issues.where('position > ?', link.position).update_all("position = position - 1 ")
+    end
+
     def source
       @source ||= link.epic
     end

ee/app/services/epic_issues/list_service.rb

 module EpicIssues
   class ListService < IssuableLinks::ListService
-    def execute
-      issues.map do |referenced_issue|
-        {
-          id: referenced_issue.id,
-          title: referenced_issue.title,
-          state: referenced_issue.state,
-          reference: reference(referenced_issue),
-          path: project_issue_path(referenced_issue.project, referenced_issue.iid),
-          destroy_relation_path: destroy_relation_path(referenced_issue),
-          epic_issue_id: referenced_issue.epic_issue_id,
-          position: referenced_issue.position
-        }
-      end
-    end
-
     private
 
     def issues
@@ -23,18 +8,22 @@ module EpicIssues
       issuable.issues(current_user)
     end
 
-    def destroy_relation_path(issue)
-      if can_destroy_issue_link?(issue)
+    def relation_path(issue)
+      if can_admin_issue_link?(issue)
         group_epic_issue_path(issuable.group, issuable.iid, issue.epic_issue_id)
       end
     end
 
-    def can_destroy_issue_link?(issue)
+    def can_admin_issue_link?(issue)
       Ability.allowed?(current_user, :admin_epic_issue, issue) && Ability.allowed?(current_user, :admin_epic, issuable)
     end
 
     def reference(issue)
       issue.to_reference(full: true)
     end
+
+    def to_hash(issue)
+      super.merge(relation_path: relation_path(issue))
+    end
   end
 end

ee/app/services/epic_issues/order_service.rb → ee/app/services/epic_issues/update_service.rb

 module EpicIssues
-  class OrderService < BaseService
-    attr_reader :epic_issue, :current_user, :new_position, :old_position
+  class UpdateService < BaseService
+    attr_reader :epic_issue, :current_user, :old_position, :params
 
     def initialize(epic_issue, user, params)
       @epic_issue = epic_issue
       @current_user = user
-      @new_position = params[:position].to_i
       @old_position = epic_issue.position
+      @params = params
     end
 
     def execute
-      move_issue
+      move_issue if params[:position]
       success
     end
 
@@ -42,5 +42,12 @@ module EpicIssues
     def update_operator
       new_position > old_position ? '-' : '+'
     end
+
+    def new_position
+      @new_position ||= begin
+        replacing_issue = epic.epic_issues.order(:position).limit(1).offset(params[:position])[0]
+        replacing_issue.position
+      end
+    end
   end
 end

ee/app/services/issuable_links/create_service.rb

@@ -12,14 +12,22 @@ module IssuableLinks
       end
 
       create_issue_links
+      after_create
       success
     end
 
     private
 
     def create_issue_links
+      @created_links = []
+
       referenced_issues.each do |referenced_issue|
-        create_notes(referenced_issue) if relate_issues(referenced_issue)
+        link = relate_issues(referenced_issue)
+
+        next unless link.persisted?
+
+        @created_links << link
+        create_notes(referenced_issue)
       end
     end
 
@@ -60,5 +68,8 @@ module IssuableLinks
     def relate_issues(referenced_issue)
       raise NotImplementedError
     end
+
+    def after_create
+    end
   end
 end

ee/app/services/issuable_links/destroy_service.rb

@@ -11,6 +11,7 @@ module IssuableLinks
       return error('No Issue Link found', 404) unless permission_to_remove_relation?
 
       remove_relation
+      after_remove
       create_notes
 
       success(message: 'Relation was removed')
@@ -21,5 +22,8 @@ module IssuableLinks
     def remove_relation
       link.destroy!
     end
+
+    def after_remove
+    end
   end
 end

ee/app/services/issuable_links/list_service.rb

@@ -10,26 +10,24 @@ module IssuableLinks
 
     def execute
       issues.map do |referenced_issue|
-        {
-          id: referenced_issue.id,
-          title: referenced_issue.title,
-          state: referenced_issue.state,
-          reference: reference(referenced_issue),
-          path: project_issue_path(referenced_issue.project, referenced_issue.iid),
-          destroy_relation_path: destroy_relation_path(referenced_issue),
-          epic_issue_id: referenced_issue.epic_issue_id
-        }
+        to_hash(referenced_issue)
       end
     end
 
     private
 
-    def destroy_relation_path(issue)
-      raise NotImplementedError
-    end
-
     def reference(issue)
       issue.to_reference(issuable.project)
     end
+
+    def to_hash(issue)
+      {
+        id: issue.id,
+        title: issue.title,
+        state: issue.state,
+        reference: reference(issue),
+        path: project_issue_path(issue.project, issue.iid)
+      }
+    end
   end
 end

ee/app/services/issue_links/create_service.rb

 module IssueLinks
   class CreateService < IssuableLinks::CreateService
     def relate_issues(referenced_issue)
-      IssueLink.new(source: issuable, target: referenced_issue).save
+      IssueLink.create(source: issuable, target: referenced_issue)
     end
 
     def linkable_issues(issues)

ee/app/services/issue_links/list_service.rb

@@ -25,5 +25,9 @@ module IssueLinks
     def can_destroy_issue_link?(project)
       Ability.allowed?(current_user, :admin_issue_link, project)
     end
+
+    def to_hash(issue)
+      super.merge(destroy_relation_path: destroy_relation_path(issue))
+    end
   end
 end

spec/ee/spec/controllers/groups/epic_issues_controller_spec.rb

@@ -34,7 +34,7 @@ describe Groups::EpicIssuesController do
           'state' => issue.state,
           'reference' => "#{project.full_path}##{issue.iid}",
           'path' => "/#{project.full_path}/issues/#{issue.iid}",
-          'destroy_relation_path' => "/groups/#{group.full_path}/-/epics/#{epic.iid}/issues/#{epic_issues.id}"
+          'relation_path' => "/groups/#{group.full_path}/-/epics/#{epic.iid}/issues/#{epic_issues.id}"
          }
       ]
       expect(JSON.parse(response.body)).to eq(expected_result)
@@ -145,13 +145,13 @@ describe Groups::EpicIssuesController do
     end
   end
 
-  describe 'PUT #order' do
+  describe 'PUT #update' do
     let(:issue2) { create(:issue, project: project) }
     let!(:epic_issue1) { create(:epic_issue, epic: epic, issue: issue, position: 1) }
     let!(:epic_issue2) { create(:epic_issue, epic: epic, issue: issue2, position: 2) }
 
     subject do
-      put :order, group_id: group, epic_id: epic.to_param, id: epic_issue1.id, position: 2
+      put :update, group_id: group, epic_id: epic.to_param, id: epic_issue1.id, position: 1
     end
 
     context 'when user has permissions to admin the epic' do
@@ -184,7 +184,7 @@ describe Groups::EpicIssuesController do
 
     context 'when the epic from the association does not equal epic from the path' do
       subject do
-        put :order, group_id: group, epic_id: another_epic.to_param, id: epic_issue1.id, position: 2
+        put :update, group_id: group, epic_id: another_epic.to_param, id: epic_issue1.id, position: 2
       end
 
       let(:another_epic) { create(:epic, group: group) }

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

@@ -2,13 +2,17 @@ require 'spec_helper'
 
 describe EpicIssues::CreateService do
   describe '#execute' do
-    let(:group) { create :group }
-    let(:epic) { create :epic, group: group }
+    let(:group) { create(:group) }
+    let(:epic) { create(:epic, group: group) }
     let(:project) { create(:project, group: group) }
-    let(:issue) { create :issue, project: project }
-    let(:user) { create :user }
+    let(:issue) { create(:issue, project: project) }
+    let(:issue2) { create(:issue, project: project) }
+    let(:issue3) { create(:issue, project: project) }
+    let(:user) { create(:user) }
     let(:valid_reference) { issue.to_reference(full: true) }
 
+    let!(:existing_link) { create(:epic_issue, epic: epic, issue: issue3, position: 1) }
+
     def assign_issue(references)
       params = { issue_references: references }
 
@@ -16,10 +20,19 @@ describe EpicIssues::CreateService do
     end
 
     shared_examples 'returns success' do
-      it 'creates relationships' do
-        expect { subject }.to change(EpicIssue, :count).from(0).to(1)
+      let(:created_link) { EpicIssue.find_by!(issue_id: issue.id) }
+
+      it 'creates a new relationship' do
+        expect { subject }.to change(EpicIssue, :count).from(1).to(2)
+
+        expect(created_link).to have_attributes(epic: epic)
+      end
+
+      it 'orders the epic issue to the first place and moves the existing ones down' do
+        subject
 
-        expect(EpicIssue.find_by!(issue_id: issue.id)).to have_attributes(epic: epic)
+        expect(created_link.position).to eq(1)
+        expect(existing_link.reload.position).to eq(2)
       end
 
       it 'returns success status' do
@@ -105,6 +118,12 @@ describe EpicIssues::CreateService do
               allow(SystemNoteService).to receive(:epic_issue)
               allow(SystemNoteService).to receive(:issue_on_epic)
 
+              # Extractor makes a permission check for each issue which messes up the query count check
+              extractor = double
+              allow(Gitlab::ReferenceExtractor).to receive(:new).and_return(extractor)
+              allow(extractor).to receive(:analyze)
+              allow(extractor).to receive(:issues).and_return([issue])
+
               params = { issue_references: [valid_reference] }
               control_count = ActiveRecord::QueryRecorder.new { described_class.new(epic, user, params).execute }.count
 
@@ -115,9 +134,15 @@ describe EpicIssues::CreateService do
               epic = create(:epic, group: group)
               group.add_developer(user)
 
+              allow(extractor).to receive(:issues).and_return(issues)
               params = { issue_references: issues.map { |i| i.to_reference(full: true) } }
 
-              expect { described_class.new(epic, user, params).execute }.not_to exceed_query_limit(control_count)
+              # threshold 16 because 4 queries are generated for each insert
+              # (savepoint, exists, 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(16)
             end
           end
 
@@ -136,6 +161,36 @@ describe EpicIssues::CreateService do
 
             include_examples 'returns success'
           end
+
+          context 'when multiple valid issues are given' do
+            subject { assign_issue([issue.to_reference(full: true), issue2.to_reference(full: true)]) }
+
+            let(:created_link1) { EpicIssue.find_by!(issue_id: issue.id) }
+            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(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
+              subject
+
+              expect(created_link1.position).to eq(1)
+              expect(created_link2.position).to eq(2)
+              expect(existing_link.reload.position).to eq(3)
+            end
+
+            it 'returns success status' do
+              expect(subject).to eq(status: :success)
+            end
+
+            it 'creates 2 system notes for each assiues' do
+              expect { subject }.to change { Note.count }.from(0).to(4)
+            end
+          end
         end
       end
 
@@ -160,7 +215,7 @@ describe EpicIssues::CreateService do
         end
 
         it 'does not create a new association' do
-          expect { subject }.not_to change(EpicIssue, :count).from(1)
+          expect { subject }.not_to change(EpicIssue, :count).from(2)
         end
 
         it 'updates the existing association' do

spec/ee/spec/services/epic_issues/destroy_service_spec.rb

@@ -7,7 +7,7 @@ describe EpicIssues::DestroyService do
     let(:project) { create(:project, group: group) }
     let(:epic) { create(:epic, group: group) }
     let(:issue) { create(:issue, project: project) }
-    let!(:epic_issue) { create(:epic_issue, epic: epic, issue: issue) }
+    let!(:epic_issue) { create(:epic_issue, epic: epic, issue: issue, position: 2) }
 
     subject { described_class.new(epic_issue, user).execute }
 
@@ -43,6 +43,16 @@ describe EpicIssues::DestroyService do
           expect { subject }.to change { Note.count }.from(0).to(2)
         end
 
+        it 'updates the position value of other issues correctly' do
+          epic_issue2 = create(:epic_issue, epic: epic, issue: create(:issue, project: project), position: 1)
+          epic_issue3 = create(:epic_issue, epic: epic, issue: create(:issue, project: project), position: 3)
+
+          subject
+
+          expect(epic_issue2.reload.position).to eq(1)
+          expect(epic_issue3.reload.position).to eq(2)
+        end
+
         it 'creates a note for epic correctly' do
           subject
           note = Note.find_by(noteable_id: epic.id, noteable_type: 'Epic')

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

@@ -44,7 +44,7 @@ describe EpicIssues::ListService do
               state: issue1.state,
               reference: issue1.to_reference(full: true),
               path: "/#{project.full_path}/issues/#{issue1.iid}",
-              destroy_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}"
             },
             {
               id: issue2.id,
@@ -52,7 +52,7 @@ describe EpicIssues::ListService do
               state: issue2.state,
               reference: issue2.to_reference(full: true),
               path: "/#{project.full_path}/issues/#{issue2.iid}",
-              destroy_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}"
             },
             {
               id: issue3.id,
@@ -60,7 +60,7 @@ describe EpicIssues::ListService do
               state: issue3.state,
               reference: issue3.to_reference(full: true),
               path: "/#{other_project.full_path}/issues/#{issue3.iid}",
-              destroy_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}"
             }
           ]
           expect(subject).to match_array(expected_result)
@@ -80,7 +80,7 @@ describe EpicIssues::ListService do
               state: issue1.state,
               reference: issue1.to_reference(full: true),
               path: "/#{project.full_path}/issues/#{issue1.iid}",
-              destroy_relation_path: nil
+              relation_path: nil
             },
             {
               id: issue2.id,
@@ -88,7 +88,7 @@ describe EpicIssues::ListService do
               state: issue2.state,
               reference: issue2.to_reference(full: true),
               path: "/#{project.full_path}/issues/#{issue2.iid}",
-              destroy_relation_path: nil
+              relation_path: nil
             }
           ]
 

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

 require 'spec_helper'
 
-describe EpicIssues::OrderService do
+describe EpicIssues::UpdateService do
   describe '#execute' do
     let(:user) { create(:user) }
     let(:group) { create(:group) }
@@ -24,30 +24,39 @@ describe EpicIssues::OrderService do
     end
 
     context 'moving issue to the first position' do
-      it 'orders issues correctly' do
-        order_issue(epic_issue3, 1)
+      context 'when all positions are filled' do
+        before do
+          order_issue(epic_issue3, 0)
+        end
 
-        expect(ordered_epics).to eq([epic_issue3, epic_issue1, epic_issue2, epic_issue4])
+        it 'orders issues correctly' do
+          expect(ordered_epics).to eq([epic_issue3, epic_issue1, epic_issue2, epic_issue4])
+        end
+
+        it 'updates all affected issues positions by 1' do
+          expect(epic_issue3.reload.position).to eq(1)
+          expect(epic_issue1.reload.position).to eq(2)
+          expect(epic_issue2.reload.position).to eq(3)
+          expect(epic_issue4.reload.position).to eq(4)
+        end
       end
 
-      context 'when some order values are missing ' do
+      context 'when some position values are missing ' do
         before do
           epic_issue1.update_attribute(:position, 3)
           epic_issue2.update_attribute(:position, 6)
           epic_issue3.update_attribute(:position, 10)
           epic_issue4.update_attribute(:position, 15)
+
+          order_issue(epic_issue3, 0)
         end
 
         it 'orders issues correctly' do
-          order_issue(epic_issue3, 1)
-
           expect(ordered_epics).to eq([epic_issue3, epic_issue1, epic_issue2, epic_issue4])
         end
 
         it 'updates all affected issues positions by 1' do
-          order_issue(epic_issue3, 1)
-
-          expect(epic_issue3.reload.position).to eq(1)
+          expect(epic_issue3.reload.position).to eq(3)
           expect(epic_issue1.reload.position).to eq(4)
           expect(epic_issue2.reload.position).to eq(7)
           expect(epic_issue4.reload.position).to eq(15)
@@ -56,10 +65,21 @@ describe EpicIssues::OrderService do
     end
 
     context 'moving issue to the third position' do
-      it 'orders issues correctly' do
-        order_issue(epic_issue1, 3)
+      context 'when all positions are filled' do
+        before do
+          order_issue(epic_issue1, 2)
+        end
 
-        expect(ordered_epics).to eq([epic_issue2, epic_issue3, epic_issue1, epic_issue4])
+        it 'orders issues correctly' do
+          expect(ordered_epics).to eq([epic_issue2, epic_issue3, epic_issue1, epic_issue4])
+        end
+
+        it 'updates all affected issues positions by 1' do
+          expect(epic_issue2.reload.position).to eq(1)
+          expect(epic_issue3.reload.position).to eq(2)
+          expect(epic_issue1.reload.position).to eq(3)
+          expect(epic_issue4.reload.position).to eq(4)
+        end
       end
 
       context 'when some order values are missing ' do
@@ -68,20 +88,18 @@ describe EpicIssues::OrderService do
           epic_issue2.update_attribute(:position, 6)
           epic_issue3.update_attribute(:position, 10)
           epic_issue4.update_attribute(:position, 15)
+
+          order_issue(epic_issue1, 2)
         end
 
         it 'orders issues correctly' do
-          order_issue(epic_issue1, 10)
-
           expect(ordered_epics).to eq([epic_issue2, epic_issue3, epic_issue1, epic_issue4])
         end
 
         it 'updates all affected issues positions by 1' do
-          order_issue(epic_issue3, 1)
-
-          expect(epic_issue3.reload.position).to eq(1)
-          expect(epic_issue1.reload.position).to eq(4)
-          expect(epic_issue2.reload.position).to eq(7)
+          expect(epic_issue2.reload.position).to eq(5)
+          expect(epic_issue3.reload.position).to eq(9)
+          expect(epic_issue1.reload.position).to eq(10)
           expect(epic_issue4.reload.position).to eq(15)
         end
       end