Move migration to the background

- and code & specs improvements

db/post_migrate/20190926180443_move_epic_issues_after_epics.rb → db/post_migrate/20190926180443_schedule_epic_issues_after_epics_move.rb

@@ -3,35 +3,29 @@
 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
-class MoveEpicIssuesAfterEpics < ActiveRecord::Migration[5.2]
+class ScheduleEpicIssuesAfterEpicsMove < ActiveRecord::Migration[5.2]
   include Gitlab::Database::MigrationHelpers
 
   DOWNTIME = false
-  BATCH_SIZE = 1_000
+  INTERVAL = 5.minutes.to_i
+  BATCH_SIZE = 100
+  MIGRATION = 'MoveEpicIssuesAfterEpics'
 
   disable_ddl_transaction!
 
   class Epic < ActiveRecord::Base
     self.table_name = 'epics'
-  end
-
-  class EpicIssue < ActiveRecord::Base
-    self.table_name = 'epic_issues'
 
     include ::EachBatch
   end
 
   def up
-    maximum_epic_position = Epic.maximum(:relative_position)
-
-    return unless maximum_epic_position
-
-    max_position = Gitlab::Database::MAX_INT_VALUE
-
-    delta = ((maximum_epic_position - max_position) / 2.0).abs.ceil
+    return unless ::Gitlab.ee?
 
-    EpicIssue.where('relative_position < ?', maximum_epic_position).each_batch(of: BATCH_SIZE) do |batch, _|
-      batch.update_all("relative_position = relative_position + #{delta}")
+    Epic.each_batch(of: BATCH_SIZE) do |batch, index|
+      range = batch.pluck('MIN(id)', 'MAX(id)').first
+      delay = index * interval
+      BackgroundMigrationWorker.perform_in(delay, MIGRATION, *range)
     end
   end
 

ee/app/models/concerns/epic_tree_sorting.rb

@@ -21,11 +21,13 @@ module EpicTreeSorting
   included do
     def move_sequence(start_pos, end_pos, delta)
       items_to_update = scoped_items
+        .select(:id, :object_type)
         .where('relative_position BETWEEN ? AND ?', start_pos, end_pos)
+        .where.not('object_type = ? AND id = ?', self.class.table_name.singularize, self.id)
 
       items_to_update.group_by { |item| item.object_type }.each do |type, group_items|
-        items = type.camelcase.constantize.where(id: group_items.map(&:id))
-        items = items.where.not(id: self.id) if type == self.class.underscore
+        ids = group_items.map(&:id)
+        items = type.camelcase.constantize.where(id: ids).select(:id)
         items.update_all("relative_position = relative_position + #{delta}")
       end
     end

ee/app/services/epics/tree_reorder_service.rb

@@ -27,24 +27,30 @@ module Epics
     end
 
     def before_object
-      return unless params[:relative_position].to_sym == :before
+      return unless params[:relative_position] == 'before'
 
       adjacent_reference
     end
 
     def after_object
-      return unless params[:relative_position].to_sym == :after
+      return unless params[:relative_position] == 'after'
 
       adjacent_reference
     end
 
     def validate_objects
+      return 'Relative position is not valid.' unless valid_relative_position?
+
       unless supported_type?(moving_object) && supported_type?(adjacent_reference)
         return 'Only epics and epic_issues are supported.'
       end
 
       return 'You don\'t have permissions to move the objects.' unless authorized?
-      return 'Both object have to belong to same parent epic.' unless same_parent?
+      return 'Both objects have to belong to the same parent epic.' unless same_parent?
+    end
+
+    def valid_relative_position?
+      %w(before after).include?(params[:relative_position])
     end
 
     def same_parent?

ee/lib/gitlab/background_migration/move_epic_issues_after_epics.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    class MoveEpicIssuesAfterEpics
+      class EpicIssue < ActiveRecord::Base
+        self.table_name = 'epic_issues'
+      end
+
+      class Epic < ActiveRecord::Base
+        self.table_name = 'epics'
+      end
+
+      def perform(start_id, stop_id)
+        maximum_epic_position = Epic.maximum(:relative_position)
+
+        return unless maximum_epic_position
+
+        max_position = Gitlab::Database::MAX_INT_VALUE
+        delta = ((maximum_epic_position - max_position) / 2.0).abs.ceil
+
+        EpicIssue.where(epic_id: start_id..stop_id).where('relative_position < ?', max_position - delta)
+          .update_all("relative_position = relative_position + #{delta}")
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/background_migration/move_epic_issues_after_epics_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::MoveEpicIssuesAfterEpics, :migration, schema: 20190926180443 do
+  let(:users) { table(:users) }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:issues) { table(:issues) }
+  let(:epics) { table(:epics) }
+  let(:epic_issues) { table(:epic_issues) }
+
+  subject { described_class.new }
+
+  describe '#perform' do
+    let(:epic_params) do
+      {
+        title: 'Epic',
+        title_html: 'Epic',
+        group_id: group.id,
+        author_id: user.id
+      }
+    end
+    let(:issue_params) do
+      {
+        title: 'Issue',
+        title_html: 'Issue',
+        project_id: project.id,
+        author_id: user.id
+      }
+    end
+
+    let(:user) { users.create(name: 'test', email: 'test@example.com', projects_limit: 5) }
+    let(:group) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
+
+    context 'when there are epic_issues present' do
+      let(:project) { projects.create(namespace_id: group.id, name: 'foo') }
+      let(:base_epic) { epics.create(epic_params.merge(iid: 3, relative_position: 500)) }
+      let(:issue_1) { issues.create(issue_params.merge(iid: 1)) }
+      let(:issue_2) { issues.create(issue_params.merge(iid: 2)) }
+      let(:issue_3) { issues.create(issue_params.merge(iid: 3)) }
+
+      let!(:epic_1) { epics.create(epic_params.merge(iid: 1, relative_position: 100)) }
+      let!(:epic_2) { epics.create(epic_params.merge(iid: 2, relative_position: 5000)) }
+      let!(:epic_issue_1) { epic_issues.create(issue_id: issue_1.id, epic_id: base_epic.id, relative_position: 400) }
+      let!(:epic_issue_2) { epic_issues.create(issue_id: issue_2.id, epic_id: base_epic.id, relative_position: 5010) }
+      let!(:epic_issue_3) { epic_issues.create(issue_id: issue_3.id, epic_id: base_epic.id, relative_position: Gitlab::Database::MAX_INT_VALUE - 10) }
+
+      before do
+        subject.perform(epics.first.id, epics.last.id)
+      end
+
+      it 'does not change relative_position of epics' do
+        expect(base_epic.relative_position).to eq(500)
+        expect(epic_1.relative_position).to eq(100)
+        expect(epic_2.relative_position).to eq(5000)
+      end
+
+      it 'moves epic_issues after epics' do
+        expect(epic_issue_1.reload.relative_position).to be > 5000
+        expect(epic_issue_2.reload.relative_position).to be > 5000
+      end
+
+      it 'keeps epic_issues order' do
+        expect(epic_issue_1.reload.relative_position).to be < epic_issue_2.reload.relative_position
+      end
+
+      it 'does not change the relative_position of epic_issue getting to the max value' do
+        expect(epic_issue_3.reload.relative_position).to eq(Gitlab::Database::MAX_INT_VALUE - 10)
+      end
+    end
+
+    context 'when there are no epics' do
+      it 'runs correctly' do
+        expect(subject.perform(1, 10)).to be_nil
+      end
+    end
+
+    context 'when there are no epic_issues' do
+      it 'runs correctly' do
+        epics.create(epic_params.merge(iid: 3, relative_position: 500))
+
+        expect(subject.perform(1, 10)).to be_zero
+      end
+    end
+  end
+end

ee/spec/models/concerns/epic_tree_sorting_spec.rb

@@ -3,8 +3,8 @@
 require 'spec_helper'
 
 describe EpicTreeSorting do
-  let(:group) { create(:group) }
-  let(:base_epic) { create(:epic, group: group) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:base_epic) { create(:epic, group: group) }
   let!(:epic_issue1) { create(:epic_issue, epic: base_epic, relative_position: 10) }
   let!(:epic_issue2) { create(:epic_issue, epic: base_epic, relative_position: 500) }
   let!(:epic_issue3) { create(:epic_issue, epic: base_epic, relative_position: 1002) }
@@ -23,6 +23,7 @@ describe EpicTreeSorting do
       epic_issue2.move_after(epic2)
 
       expect(epic_issue2.relative_position).to be_between(epic2.reload.relative_position, epic3.reload.relative_position).exclusive
+      expect(epic_issue3.reload.relative_position).to be > epic3.reload.relative_position
     end
   end
 
@@ -53,4 +54,41 @@ describe EpicTreeSorting do
       expect(epic_issue3.relative_position).to be_between(epic1.reload.relative_position, epic_issue2.reload.relative_position).exclusive
     end
   end
+
+  context '#move_sequence' do
+    let!(:epic_issue1) { create(:epic_issue, epic: base_epic, relative_position: 1000) }
+    let!(:epic_issue2) { create(:epic_issue, epic: base_epic, relative_position: 1001) }
+    let!(:epic_issue3) { create(:epic_issue, epic: base_epic, relative_position: 1004) }
+    let!(:epic1) { create(:epic, parent: base_epic, group: group, relative_position: 1002) }
+    let!(:epic2) { create(:epic, parent: base_epic, group: group, relative_position: 1003) }
+    let!(:epic3) { create(:epic, parent: base_epic, group: group, relative_position: 1005) }
+
+    context 'when self is an epic' do
+      it 'moves all objects correctly' do
+        epic1.move_sequence(1003, 1005, 500)
+
+        expect(epic_issue1.reload.relative_position).to eq(1000)
+        expect(epic_issue2.reload.relative_position).to eq(1001)
+        expect(epic_issue3.reload.relative_position).to eq(1504)
+
+        expect(epic1.reload.relative_position).to eq(1002)
+        expect(epic2.reload.relative_position).to eq(1503)
+        expect(epic3.reload.relative_position).to eq(1505)
+      end
+    end
+
+    context 'when self is an epic_issue' do
+      it 'moves all objects correctly' do
+        epic_issue1.move_sequence(1001, 1005, 500)
+
+        expect(epic_issue1.reload.relative_position).to eq(1000)
+        expect(epic_issue2.reload.relative_position).to eq(1501)
+        expect(epic_issue3.reload.relative_position).to eq(1504)
+
+        expect(epic1.reload.relative_position).to eq(1502)
+        expect(epic2.reload.relative_position).to eq(1503)
+        expect(epic3.reload.relative_position).to eq(1505)
+      end
+    end
+  end
 end

ee/spec/requests/api/graphql/mutations/epic_tree/reorder_spec.rb

@@ -106,7 +106,7 @@ describe 'Updating an epic tree' do
           end
         end
 
-        context 'when moving an epic fails due another parent' do
+        context 'when moving an epic fails due to another parent' do
           let(:epic2) { create(:epic, relative_position: 20) }
 
           it_behaves_like 'a mutation that does not update the tree'
@@ -114,7 +114,7 @@ describe 'Updating an epic tree' do
           it 'returns the error message' do
             post_graphql_mutation(mutation, current_user: current_user)
 
-            expect(mutation_response['errors']).to eq(['Both object have to belong to same parent epic.'])
+            expect(mutation_response['errors']).to eq(['Both objects have to belong to the same parent epic.'])
           end
         end
       end
@@ -151,7 +151,7 @@ describe 'Updating an epic tree' do
         it 'returns the error message' do
           post_graphql_mutation(mutation, current_user: current_user)
 
-          expect(mutation_response['errors']).to eq(['Both object have to belong to same parent epic.'])
+          expect(mutation_response['errors']).to eq(['Both objects have to belong to the same parent epic.'])
         end
       end
     end

ee/spec/services/epics/tree_reorder_service_spec.rb

@@ -15,7 +15,7 @@ describe Epics::TreeReorderService do
     let(:epic_issue1) { create(:epic_issue, epic: epic, issue: issue1, relative_position: 30) }
     let(:epic_issue2) { create(:epic_issue, epic: epic, issue: issue2, relative_position: 40) }
 
-    let(:relative_position) { :after }
+    let(:relative_position) { 'after' }
     let!(:tree_object_1) { epic1 }
     let!(:tree_object_2) { epic2 }
     let(:adjacent_reference_id) { GitlabSchema.id_from_object(tree_object_1) }
@@ -45,7 +45,7 @@ describe Epics::TreeReorderService do
       end
     end
 
-    context 'when epics feature is enabled' do
+    context 'when epics feature is not enabled' do
       it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'
     end
 
@@ -63,6 +63,12 @@ describe Epics::TreeReorderService do
           group.add_developer(user)
         end
 
+        context 'when relative_position is not valid' do
+          let(:relative_position) { 'whatever' }
+
+          it_behaves_like 'error for the tree update', 'Relative position is not valid.'
+        end
+
         context 'when moving EpicIssue' do
           let!(:tree_object_1) { epic_issue1 }
           let!(:tree_object_2) { epic_issue2 }
@@ -93,7 +99,7 @@ describe Epics::TreeReorderService do
               epic_issue2.update(epic: other_epic)
             end
 
-            it_behaves_like 'error for the tree update', 'Both object have to belong to same parent epic.'
+            it_behaves_like 'error for the tree update', 'Both objects have to belong to the same parent epic.'
           end
 
           context 'when object being moved is not supported type' do
@@ -145,13 +151,13 @@ describe Epics::TreeReorderService do
             it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'
           end
 
-          context 'when object being moved is from of another epic' do
+          context 'when object being moved is froms another epic' do
             before do
               other_epic = create(:epic, group: group)
               epic2.update(parent: other_epic)
             end
 
-            it_behaves_like 'error for the tree update', 'Both object have to belong to same parent epic.'
+            it_behaves_like 'error for the tree update', 'Both objects have to belong to the same parent epic.'
           end
 
           context 'when moving is successful' do