Use RelativePositioning for ordering issues in epic

app/models/concerns/relative_positioning.rb

@@ -14,8 +14,12 @@ module RelativePositioning
     [project.id]
   end
 
+  def min_relative_position
+    self.class.in_parent(parent_id).minimum(:relative_position)
+  end
+
   def max_relative_position
-    self.class.in_projects(project_ids).maximum(:relative_position)
+    self.class.in_parent(parent_id).maximum(:relative_position)
   end
 
   def prev_relative_position
@@ -23,7 +27,7 @@ module RelativePositioning
 
     if self.relative_position
       prev_pos = self.class
-        .in_projects(project_ids)
+        .in_parent([parent_id])
         .where('relative_position < ?', self.relative_position)
         .maximum(:relative_position)
     end
@@ -36,7 +40,7 @@ module RelativePositioning
 
     if self.relative_position
       next_pos = self.class
-        .in_projects(project_ids)
+        .in_parent([parent_id])
         .where('relative_position > ?', self.relative_position)
         .minimum(:relative_position)
     end
@@ -63,7 +67,7 @@ module RelativePositioning
     pos_after = before.next_relative_position
 
     if before.shift_after?
-      issue_to_move = self.class.in_projects(project_ids).find_by!(relative_position: pos_after)
+      issue_to_move = self.class.in_parent([parent_id]).find_by!(relative_position: pos_after)
       issue_to_move.move_after
       @positionable_neighbours = [issue_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables
 
@@ -78,7 +82,7 @@ module RelativePositioning
     pos_before = after.prev_relative_position
 
     if after.shift_before?
-      issue_to_move = self.class.in_projects(project_ids).find_by!(relative_position: pos_before)
+      issue_to_move = self.class.in_parent([parent_id]).find_by!(relative_position: pos_before)
       issue_to_move.move_before
       @positionable_neighbours = [issue_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables
 
@@ -92,6 +96,10 @@ module RelativePositioning
     self.relative_position = position_between(max_relative_position || START_POSITION, MAX_POSITION)
   end
 
+  def move_to_start
+    self.relative_position = position_between(min_relative_position || START_POSITION, MIN_POSITION)
+  end
+
   # Indicates if there is an issue that should be shifted to free the place
   def shift_after?
     next_pos = next_relative_position

app/models/issue.rb

@@ -47,6 +47,8 @@ class Issue < ActiveRecord::Base
 
   validates :project, presence: true
 
+  alias_attribute :parent_id, :project_id
+
   scope :in_projects, ->(project_ids) { where(project_id: project_ids) }
 
   scope :assigned, -> { where('EXISTS (SELECT TRUE FROM issue_assignees WHERE issue_id = issues.id)') }
@@ -93,6 +95,11 @@ class Issue < ActiveRecord::Base
 
   acts_as_paranoid
 
+  class << self
+    alias_method :in_parent, :in_projects
+  end
+
+
   def self.reference_prefix
     '#'
   end

db/migrate/20171214115254_add_position_to_epic_issues.rb

@@ -6,13 +6,11 @@ class AddPositionToEpicIssues < ActiveRecord::Migration
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(:epic_issues, :position, :integer, default: 1, allow_null: false)
-    add_timestamps_with_timezone :epic_issues, null: true
+    default_position = Gitlab::Database::MAX_INT_VALUE / 2
+    add_column_with_default(:epic_issues, :relative_position, :integer, default: default_position, allow_null: false)
   end
 
   def down
-    remove_column(:epic_issues, :position)
-    remove_column(:epic_issues, :created_at)
-    remove_column(:epic_issues, :updated_at)
+    remove_column(:epic_issues, :relative_position)
   end
 end

db/post_migrate/20171221154744_schedule_epic_issue_positions_migration.rb

-class ScheduleEpicIssuePositionsMigration < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  MIGRATION = 'SetEpicIssuesPositionValues'.freeze
-  BATCH_SIZE = 100
-
-  class Epic < ActiveRecord::Base
-    self.table_name = 'epics'
-
-    include EachBatch
-  end
-
-  def up
-    Epic.select(:id).each_batch(of: BATCH_SIZE) do |batch, index|
-      start_id, end_id = batch.pluck('MIN(id), MAX(id)').first
-
-      BackgroundMigrationWorker.perform_in(index * 5.minutes, MIGRATION, [start_id, end_id])
-    end
-  end
-end

db/schema.rb

@@ -765,9 +765,7 @@ ActiveRecord::Schema.define(version: 20171221154744) do
   create_table "epic_issues", force: :cascade do |t|
     t.integer "epic_id", null: false
     t.integer "issue_id", null: false
-    t.integer "position", default: 1, null: false
-    t.datetime_with_timezone "created_at"
-    t.datetime_with_timezone "updated_at"
+    t.integer "relative_position", default: 1073741823, null: false
   end
 
   add_index "epic_issues", ["epic_id"], name: "index_epic_issues_on_epic_id", using: :btree

ee/app/models/ee/epic.rb

@@ -85,10 +85,10 @@ module EE
     end
 
     def issues(current_user)
-      related_issues = ::Issue.select('issues.*, epic_issues.id as epic_issue_id, epic_issues.position')
+      related_issues = ::Issue.select('issues.*, epic_issues.id as epic_issue_id, epic_issues.relative_position')
         .joins(:epic_issue)
         .where("epic_issues.epic_id = #{id}")
-        .order('epic_issues.position')
+        .order('epic_issues.relative_position, epic_issues.id')
 
       Ability.issues_readable_by_user(related_issues, current_user)
     end

ee/app/models/epic_issue.rb

 class EpicIssue < ActiveRecord::Base
+  include RelativePositioning
+
   validates :epic, :issue, presence: true
   validates :issue, uniqueness: true
 
   belongs_to :epic
   belongs_to :issue
+
+  alias_attribute :parent_id, :epic_id
+
+  scope :in_epic, ->(epic_id) { where(epic_id: epic_id) }
+
+  class << self
+    alias_method :in_parent, :in_epic
+  end
 end

ee/app/services/epic_issues/create_service.rb

@@ -2,16 +2,10 @@ 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 = existing_links.find { |link| link.issue == referenced_issue } || EpicIssue.new(issue: referenced_issue)
       link.epic = issuable
-      link.position = @created_links.count + 1
+      link.move_to_start
       link.save
 
       link

ee/app/services/epic_issues/destroy_service.rb

@@ -2,10 +2,6 @@ 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

@@ -21,5 +21,9 @@ module EpicIssues
     def reference(issue)
       issue.to_reference(full: true)
     end
+
+    def to_hash(issue)
+      super.merge(epic_issue_id: issue.epic_issue_id)
+    end
   end
 end

ee/app/services/epic_issues/update_service.rb

 module EpicIssues
   class UpdateService < BaseService
-    attr_reader :epic_issue, :current_user, :old_position, :params
+    attr_reader :epic_issue, :current_user, :params
 
     def initialize(epic_issue, user, params)
       @epic_issue = epic_issue
       @current_user = user
-      @old_position = epic_issue.position
       @params = params
     end
 
     def execute
-      move_issue if params[:position]
+      move_issue if params[:move_after_id] || params[:move_before_id]
+      epic_issue.save!
       success
     end
 
     private
 
     def move_issue
-      epic_issue.update_attribute(:position, new_position)
-      issues_to_move.update_all("position = position #{update_operator} 1")
-    end
-
-    def epic
-      @epic ||= epic_issue.epic
-    end
-
-    def issues_to_move
-      @issues_to_move ||= epic.epic_issues
-        .where('position >= ? AND position <= ? AND id != ?', from, to, epic_issue.id)
-    end
-
-    def from
-      [new_position, old_position].min
-    end
-
-    def to
-      [new_position, old_position].max
-    end
-
-    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])
-
-        return replacing_issue[0].position if replacing_issue.present?
+      before_epic_issue = EpicIssue.find_by(id: params[:move_before_id])
+      after_epic_issue = EpicIssue.find_by(id: params[:move_after_id])
 
-        epic.epic_issues.order(:position).last.position
-      end
+      epic_issue.move_between(before_epic_issue, after_epic_issue)
     end
   end
 end

ee/app/services/issuable_links/create_service.rb

@@ -12,7 +12,6 @@ module IssuableLinks
       end
 
       create_issue_links
-      after_create
       success
     end
 
@@ -68,8 +67,5 @@ 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,7 +11,6 @@ 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')
@@ -22,8 +21,5 @@ module IssuableLinks
     def remove_relation
       link.destroy!
     end
-
-    def after_remove
-    end
   end
 end

ee/lib/api/epic_issues.rb

@@ -36,14 +36,20 @@ module API
       end
       params do
         requires :epic_iid, type: Integer, desc: 'The iid of the epic'
-        requires :epic_issue_id, type: Integer, desc: 'The id of the epic issue association'
-        requires :position, type: Integer, desc: 'The new position of the issue in the epic (index starting with 0)'
+        requires :epic_issue_id, type: Integer, desc: 'The id of the epic issue association to update'
+        optional :move_before_id, type: Integer, desc: 'The id of the epic issue association that should be positioned before the actual issue'
+        optional :move_after_id, type: Integer, desc: 'The id of the epic issue association that should be positioned after the actual issue'
       end
       put ':id/-/epics/:epic_iid/issues/:epic_issue_id' do
         authorize_can_admin!
         check_epic_link!
 
-        result = ::EpicIssues::UpdateService.new(link, current_user, { position: params[:position].to_i }).execute
+        update_params = {
+          move_before_id: params[:move_before_id],
+          move_after_id: params[:move_after_id],
+        }
+
+        result = ::EpicIssues::UpdateService.new(link, current_user, update_params).execute
 
         # For now we return empty body
         # The issues list in the correct order in body will be returned as part of #4250

lib/gitlab/background_migration/set_epic_issues_position_values.rb

-module Gitlab
-  module BackgroundMigration
-    class SetEpicIssuesPositionValues
-      class Epic < ActiveRecord::Base
-        self.table_name = 'epics'
-      end
-
-      class EpicIssue < ActiveRecord::Base
-        self.table_name = 'epic_issues'
-      end
-
-      def perform(start_id, end_id)
-        epic_issues =  EpicIssue.where(epic_id: start_id..end_id).order('epic_id, id').group_by { |e| e['epic_id'] }
-        return if epic_issues.empty?
-        update = []
-
-        epic_issues.each do |epic_id, issues|
-          issues.each_with_index do |epic_issue, index|
-            update << "WHEN id = #{epic_issue['id']} THEN #{index + 1}"
-          end
-        end
-
-        EpicIssue.where(epic_id: start_id..end_id).update_all("position = CASE #{update.join(' ')} END")
-      end
-    end
-  end
-end

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

@@ -172,11 +172,11 @@ describe Groups::EpicIssuesController 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) }
+    let!(:epic_issue1) { create(:epic_issue, epic: epic, issue: issue, relative_position: 1) }
+    let!(:epic_issue2) { create(:epic_issue, epic: epic, issue: issue2, relative_position: 2) }
 
     subject do
-      put :update, group_id: group, epic_id: epic.to_param, id: epic_issue1.id, epic: { position: 1 }
+      put :update, group_id: group, epic_id: epic.to_param, id: epic_issue1.id, epic: { move_before_id: epic_issue2.id }
     end
 
     it_behaves_like 'unlicensed epics action'
@@ -194,7 +194,7 @@ describe Groups::EpicIssuesController do
         end
 
         it 'updates the issue position value' do
-          expect { subject }.to change { epic_issue1.reload.position }.from(1).to(2)
+          expect { subject }.to change { epic_issue1.reload.relative_position }
         end
       end
 
@@ -204,15 +204,11 @@ describe Groups::EpicIssuesController do
 
           expect(response.status).to eq(403)
         end
-
-        it 'does not change the position value' do
-          expect { subject }.not_to change { epic_issue1.reload.position }.from(1)
-        end
       end
 
       context 'when the epic from the association does not equal epic from the path' do
         subject do
-          put :update, group_id: group, epic_id: another_epic.to_param, id: epic_issue1.id, epic: { position: 2 }
+          put :update, group_id: group, epic_id: another_epic.to_param, id: epic_issue1.id, epic: { after_move_id: epic_issue1.id }
         end
 
         let(:another_epic) { create(:epic, group: group) }
@@ -226,10 +222,6 @@ describe Groups::EpicIssuesController do
 
           expect(response.status).to eq(404)
         end
-
-        it 'does not change the position value' do
-          expect { subject }.not_to change { epic_issue1.position }.from(1)
-        end
       end
 
       context 'when the epic_issue record does not exists' do

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

@@ -6,10 +6,11 @@ describe API::EpicIssues do
   let(:project) { create(:project, :public, group: group) }
   let(:epic) { create(:epic, group: group) }
   let(:issues) { create_list(:issue, 2, project: project) }
-  let!(:epic_issue1) { create(:epic_issue, epic: epic, issue: issues[0], position: 1) }
+  let!(:epic_issue1) { create(:epic_issue, epic: epic, issue: issues[0], relative_position: 1) }
+  let!(:epic_issue2) { create(:epic_issue, epic: epic, issue: issues[1], relative_position: 2) }
 
   describe 'PUT /groups/:id/-/epics/:epic_iid/issues/:epic_issue_id' do
-    let(:url) { "/groups/#{group.path}/-/epics/#{epic.iid}/issues/#{epic_issue1.id}?position=1" }
+    let(:url) { "/groups/#{group.path}/-/epics/#{epic.iid}/issues/#{epic_issue1.id}?move_after_id=#{epic_issue2.id}" }
 
     context 'when epics feature is disabled' do
       it 'returns 403 forbidden error' do
@@ -49,7 +50,7 @@ describe API::EpicIssues do
         it 'returns 403 forbidden 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}?position=1"
+          url = "/groups/#{group.path}/-/epics/#{another_epic.iid}/issues/#{epic_issue1.id}?move_after_id=#{epic_issue2.id}"
 
           put api(url, user)
 
@@ -58,8 +59,6 @@ describe API::EpicIssues do
       end
 
       context 'when the request is correct' do
-        let!(:epic_issue2) { create(:epic_issue, epic: epic, issue: issues[1], position: 2) }
-
         before do
           group.add_developer(user)
           put api(url, user)
@@ -70,8 +69,7 @@ describe API::EpicIssues do
         end
 
         it 'updates the positions values' do
-          expect(epic_issue1.reload.position).to eq(2)
-          expect(epic_issue2.reload.position).to eq(1)
+          expect(epic_issue1.reload.relative_position).to be < epic_issue2.relative_position
         end
       end
     end

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

@@ -11,7 +11,7 @@ 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, position: 1) }
+    let!(:existing_link) { create(:epic_issue, epic: epic, issue: issue3) }
 
     def assign_issue(references)
       params = { issue_references: references }
@@ -31,8 +31,7 @@ describe EpicIssues::CreateService do
       it 'orders the epic issue to the first place and moves the existing ones down' do
         subject
 
-        expect(created_link.position).to eq(1)
-        expect(existing_link.reload.position).to eq(2)
+        expect(created_link.relative_position).to be < existing_link.reload.relative_position
       end
 
       it 'returns success status' do
@@ -137,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 16 because 4 queries are generated for each insert
-              # (savepoint, exists, insert, release savepoint)
+              # threshold 20 because 5 queries are generated for each insert
+              # (savepoint, 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(16)
+                .with_threshold(20)
             end
           end
 
@@ -178,9 +177,8 @@ describe EpicIssues::CreateService do
             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)
+              expect(created_link2.relative_position).to be < created_link1.relative_position
+              expect(created_link1.relative_position).to be < existing_link.reload.relative_position
             end
 
             it 'returns success status' 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, position: 2) }
+    let!(:epic_issue) { create(:epic_issue, epic: epic, issue: issue) }
 
     subject { described_class.new(epic_issue, user).execute }
 
@@ -43,16 +43,6 @@ 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

@@ -11,9 +11,9 @@ describe EpicIssues::ListService do
   let(:issue2) { create :issue, project: project }
   let(:issue3) { create :issue, project: other_project }
 
-  let!(:epic_issue1) { create(:epic_issue, issue: issue1, epic: epic) }
-  let!(:epic_issue2) { create(:epic_issue, issue: issue2, epic: epic) }
-  let!(:epic_issue3) { create(:epic_issue, issue: issue3, epic: epic) }
+  let!(:epic_issue1) { create(:epic_issue, issue: issue1, epic: epic, relative_position: 2) }
+  let!(:epic_issue2) { create(:epic_issue, issue: issue2, epic: epic, relative_position: 1) }
+  let!(:epic_issue3) { create(:epic_issue, issue: issue3, epic: epic, relative_position: 3) }
 
   describe '#execute' do
     subject { described_class.new(epic, user).execute }
@@ -38,14 +38,6 @@ describe EpicIssues::ListService do
 
         it 'returns related issues JSON' do
           expected_result = [
-            {
-              id: issue1.id,
-              title: issue1.title,
-              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}"
-            },
             {
               id: issue2.id,
               title: issue2.title,
@@ -54,6 +46,14 @@ describe EpicIssues::ListService do
               path: "/#{project.full_path}/issues/#{issue2.iid}",
               relation_path: "/groups/#{group.full_path}/-/epics/#{epic.iid}/issues/#{epic_issue2.id}"
             },
+            {
+              id: issue1.id,
+              title: issue1.title,
+              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}"
+            },
             {
               id: issue3.id,
               title: issue3.title,
@@ -63,7 +63,7 @@ describe EpicIssues::ListService do
               relation_path: "/groups/#{group.full_path}/-/epics/#{epic.iid}/issues/#{epic_issue3.id}"
             }
           ]
-          expect(subject).to match_array(expected_result)
+          expect(subject).to eq(expected_result)
         end
       end
 
@@ -74,14 +74,6 @@ describe EpicIssues::ListService do
 
         it 'returns related issues JSON' do
           expected_result = [
-            {
-              id: issue1.id,
-              title: issue1.title,
-              state: issue1.state,
-              reference: issue1.to_reference(full: true),
-              path: "/#{project.full_path}/issues/#{issue1.iid}",
-              relation_path: nil
-            },
             {
               id: issue2.id,
               title: issue2.title,
@@ -89,10 +81,18 @@ describe EpicIssues::ListService do
               reference: issue2.to_reference(full: true),
               path: "/#{project.full_path}/issues/#{issue2.iid}",
               relation_path: nil
+            },
+            {
+              id: issue1.id,
+              title: issue1.title,
+              state: issue1.state,
+              reference: issue1.to_reference(full: true),
+              path: "/#{project.full_path}/issues/#{issue1.iid}",
+              relation_path: nil
             }
           ]
 
-          expect(subject).to match_array(expected_result)
+          expect(subject).to eq(expected_result)
         end
       end
     end

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

@@ -6,119 +6,97 @@ describe EpicIssues::UpdateService do
     let(:group) { create(:group) }
     let(:epic) { create(:epic, group: group) }
     let(:issues) { create_list(:issue, 4) }
-    let!(:epic_issue1) { create(:epic_issue, epic: epic, issue: issues[0], position: 1) }
-    let!(:epic_issue2) { create(:epic_issue, epic: epic, issue: issues[1], position: 2) }
-    let!(:epic_issue3) { create(:epic_issue, epic: epic, issue: issues[2], position: 3) }
-    let!(:epic_issue4) { create(:epic_issue, epic: epic, issue: issues[3], position: 4) }
+    let!(:epic_issue1) { create(:epic_issue, epic: epic, issue: issues[0], relative_position: 3) }
+    let!(:epic_issue2) { create(:epic_issue, epic: epic, issue: issues[1], relative_position: 600) }
+    let!(:epic_issue3) { create(:epic_issue, epic: epic, issue: issues[2], relative_position: 1200) }
+    let!(:epic_issue4) { create(:epic_issue, epic: epic, issue: issues[3], relative_position: 2000) }
+    let(:default_position_value) { Gitlab::Database::MAX_INT_VALUE / 2 }
 
     before do
       group.add_developer(user)
     end
 
-    def order_issue(issue, new_order)
-      described_class.new(issue, user, { position: new_order } ).execute
+    def order_issue(issue, params)
+      described_class.new(issue, user, params ).execute
     end
 
     def ordered_epics
-      EpicIssue.all.order(:position)
+      EpicIssue.all.order('relative_position, id')
     end
 
     context 'moving issue to the first position' do
-      context 'when all positions are filled' do
+      let(:params) { { move_after_id: epic_issue1.id } }
+
+      context 'when some positions are close to each other' do
         before do
-          order_issue(epic_issue3, 0)
+          epic_issue2.update_attribute(:relative_position, 4)
+
+          order_issue(epic_issue3, params)
         end
 
         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 position values are missing ' do
+      context 'when there is enough place between positions' 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)
+          order_issue(epic_issue3, params)
         end
 
         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(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)
-        end
       end
     end
 
     context 'moving issue to the third position' do
-      context 'when all positions are filled' do
+      let(:params) { { move_before_id: epic_issue3.id, move_after_id: epic_issue4.id } }
+
+      context 'when some positions are close to each other' do
         before do
-          order_issue(epic_issue1, 2)
+          epic_issue2.update_attribute(:relative_position, 1998)
+          epic_issue3.update_attribute(:relative_position, 1999)
+
+          order_issue(epic_issue1, params)
         end
 
         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
+      context 'when all positions are same' 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)
+          epic_issue1.update_attribute(:relative_position, 10)
+          epic_issue2.update_attribute(:relative_position, 10)
+          epic_issue3.update_attribute(:relative_position, 10)
+          epic_issue4.update_attribute(:relative_position, 10)
 
-          order_issue(epic_issue1, 2)
+          order_issue(epic_issue1, params)
         end
 
-        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(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)
+        it 'orders affected 2 issues correctly' do
+          expect(epic_issue1.reload.relative_position)
+            .to be_between(epic_issue3.reload.relative_position, epic_issue4.reload.relative_position)
         end
       end
-    end
 
-    context 'moving issues to the last position' do
-      context 'when index of the last possition is correct' do
+      context 'when there is enough place between positions' do
         before do
-          order_issue(epic_issue1, 3)
+          order_issue(epic_issue1, params)
         end
 
         it 'orders issues correctly' do
-          expect(ordered_epics).to eq([epic_issue2, epic_issue3, epic_issue4, epic_issue1])
+          expect(ordered_epics).to eq([epic_issue2, epic_issue3, epic_issue1, epic_issue4])
         end
       end
+    end
 
-      context 'when index of the last possition is too high' do
+    context 'moving issues to the last position' do
+      context 'when index of the last possition is correct' do
         before do
-          order_issue(epic_issue1, 100)
+          order_issue(epic_issue1, move_before_id: epic_issue4.id)
         end
 
         it 'orders issues correctly' do

spec/factories/epic_issues.rb

@@ -2,6 +2,6 @@ FactoryBot.define do
   factory :epic_issue do
     epic
     issue
-    position 1
+    relative_position Gitlab::Database::MAX_INT_VALUE / 2
   end
 end

spec/lib/gitlab/background_migration/set_epic_issue_position_values_spec.rb

-require 'spec_helper'
-
-describe Gitlab::BackgroundMigration::SetEpicIssuesPositionValues, :migration, schema: 20171221154744 do
-  let(:groups) { table(:namespaces) }
-  let(:projects) { table(:projects) }
-  let(:users) { table(:users) }
-  let(:issues) { table(:issues) }
-  let(:epics) { table(:epics) }
-  let(:epic_issues) { table(:epic_issues) }
-
-  let(:group) { groups.create(name: 'group', path: 'group') }
-  let(:project) { projects.create(name: 'group', namespace_id: group.id) }
-  let(:user) { users.create(username: 'User') }
-
-  describe '#perform' do
-    context 'when there are some epics in the db' do
-      let(:epic1) { epics.create(id: 1, title: 'Epic 1', title_html: 'Epic 1', group_id: group.id, author_id: user.id, iid: 1) }
-      let(:epic2) { epics.create(id: 2, title: 'Epic 2', title_html: 'Epic 2', group_id: group.id, author_id: user.id, iid: 2) }
-      let!(:epic3) { epics.create(id: 3, title: 'Epic 3', title_html: 'Epic 3', group_id: group.id, author_id: user.id, iid: 3) }
-
-      let(:issue1) { issues.create(title: 'Issue 1', title_html: 'Issue 1', project_id: project.id, author_id: user.id) }
-      let(:issue2) { issues.create(title: 'Issue 2', title_html: 'Issue 2', project_id: project.id, author_id: user.id) }
-      let(:issue3) { issues.create(title: 'Issue 3', title_html: 'Issue 3', project_id: project.id, author_id: user.id) }
-      let(:issue4) { issues.create(title: 'Issue 4', title_html: 'Issue 4', project_id: project.id, author_id: user.id) }
-      let(:issue5) { issues.create(title: 'Issue 5', title_html: 'Issue 5', project_id: project.id, author_id: user.id) }
-
-      let!(:epic_issue1) { epic_issues.create!(epic_id: epic1.id, issue_id: issue1.id) }
-      let!(:epic_issue2) { epic_issues.create!(epic_id: epic1.id, issue_id: issue2.id) }
-      let!(:epic_issue3) { epic_issues.create!(epic_id: epic2.id, issue_id: issue3.id) }
-      let!(:epic_issue4) { epic_issues.create!(epic_id: epic2.id, issue_id: issue4.id) }
-      let!(:epic_issue5) { epic_issues.create!(epic_id: epic2.id, issue_id: issue5.id) }
-
-      it 'sets the position value correctly' do
-        subject.perform(1, 3)
-
-        expect(epic_issue1.reload.position).to eq(1)
-        expect(epic_issue2.reload.position).to eq(2)
-        expect(epic_issue3.reload.position).to eq(1)
-        expect(epic_issue4.reload.position).to eq(2)
-        expect(epic_issue5.reload.position).to eq(3)
-      end
-    end
-
-    context 'when there are no epics in the db' do
-      it 'runs the migration without errors' do
-        expect(subject.perform(1, 2)).to be_nil
-      end
-    end
-  end
-end

spec/migrations/schedule_epic_issue_positions_migration_spec.rb

-require 'spec_helper'
-require Rails.root.join('db', 'post_migrate', '20171221154744_schedule_epic_issue_positions_migration.rb')
-
-describe ScheduleEpicIssuePositionsMigration, :migration, :sidekiq do
-  let(:groups) { table(:namespaces) }
-  let(:users) { table(:users) }
-  let(:epics) { table(:epics) }
-
-  before do
-    stub_const("#{described_class.name}::BATCH_SIZE", 2)
-
-    group = groups.create(name: 'group', path: 'group')
-    user = users.create(username: 'User')
-    epics.create(id: 1, title: 'Epic 1', title_html: 'Epic 1', group_id: group.id, author_id: user.id, iid: 1)
-    epics.create(id: 2, title: 'Epic 2', title_html: 'Epic 2', group_id: group.id, author_id: user.id, iid: 2)
-    epics.create(id: 3, title: 'Epic 3', title_html: 'Epic 3', group_id: group.id, author_id: user.id, iid: 3)
-  end
-
-  it 'correctly schedules background migrations' do
-    Sidekiq::Testing.fake! do
-      Timecop.freeze do
-        migrate!
-
-        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(5.minutes, 1, 2)
-        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(10.minutes, 3, 3)
-        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
-      end
-    end
-  end
-end