Merge branch 'revert-eb11ab45' into 'master'

Revert "Merge branch 'ajk-relative-positioning-improvements' into 'master'"

See merge request gitlab-org/gitlab!39787

.rubocop_todo.yml

@@ -1483,6 +1483,7 @@ Rails/SaveBang:
     - 'spec/support/shared_examples/models/members_notifications_shared_example.rb'
     - 'spec/support/shared_examples/models/mentionable_shared_examples.rb'
     - 'spec/support/shared_examples/models/project_latest_successful_build_for_shared_examples.rb'
+    - 'spec/support/shared_examples/models/relative_positioning_shared_examples.rb'
     - 'spec/support/shared_examples/models/slack_mattermost_notifications_shared_examples.rb'
     - 'spec/support/shared_examples/models/update_project_statistics_shared_examples.rb'
     - 'spec/support/shared_examples/models/with_uploads_shared_examples.rb'

changelogs/unreleased/ajk-relative-positioning-improvements.yml

----
-title: Performance and robustness improvements for relative positioning
-merge_request: 37724
-author:
-type: performance

ee/app/models/concerns/epic_tree_sorting.rb

@@ -8,8 +8,8 @@ module EpicTreeSorting
   class_methods do
     def relative_positioning_query_base(object)
       from_union([
-        EpicIssue.select("id, relative_position, epic_id as parent_id, epic_id, 'epic_issue' as object_type").in_epic(object.parent_ids),
-        Epic.select("id, relative_position, parent_id, parent_id as epic_id, 'epic' as object_type").where(parent_id: object.parent_ids)
+        EpicIssue.select("id, relative_position, epic_id, 'epic_issue' as object_type").in_epic(object.parent_ids),
+        Epic.select("id, relative_position, parent_id as epic_id, 'epic' as object_type").where(parent_id: object.parent_ids)
       ])
     end
 
@@ -19,14 +19,11 @@ module EpicTreeSorting
   end
 
   included do
-    def move_sequence(start_pos, end_pos, delta, include_self = false)
+    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)
-
-      unless include_self
-        items_to_update = items_to_update.where.not('object_type = ? AND id = ?', self.class.table_name.singularize, self.id)
-      end
+        .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|
         ids = group_items.map(&:id)

ee/spec/models/epic_issue_spec.rb

@@ -28,49 +28,9 @@ RSpec.describe EpicIssue do
 
   context "relative positioning" do
     it_behaves_like "a class that supports relative positioning" do
-      let_it_be(:epic) { create(:epic) }
+      let(:epic) { create(:epic) }
       let(:factory) { :epic_issue }
       let(:default_params) { { epic: epic } }
     end
-
-    context 'with a mixed tree level' do
-      let_it_be(:epic) { create(:epic) }
-      let_it_be_with_reload(:left) { create(:epic_issue, epic: epic, relative_position: 100) }
-      let_it_be_with_reload(:middle) { create(:epic, group: epic.group, parent: epic, relative_position: 101) }
-      let_it_be_with_reload(:right) { create(:epic_issue, epic: epic, relative_position: 102) }
-
-      it 'can create space by using move_sequence_after' do
-        left.move_sequence_after
-        [left, middle, right].each(&:reset)
-
-        expect(middle.relative_position - left.relative_position).to be > 1
-        expect(left.relative_position).to be < middle.relative_position
-        expect(middle.relative_position).to be < right.relative_position
-      end
-
-      it 'can create space by using move_sequence_before' do
-        right.move_sequence_before
-        [left, middle, right].each(&:reset)
-
-        expect(right.relative_position - middle.relative_position).to be > 1
-        expect(left.relative_position).to be < middle.relative_position
-        expect(middle.relative_position).to be < right.relative_position
-      end
-
-      it 'moves nulls to the end' do
-        leaves = create_list(:epic_issue, 2, epic: epic, relative_position: nil)
-        nested = create(:epic, group: epic.group, parent: epic, relative_position: nil)
-        moved = [*leaves, nested]
-        level = [nested, *leaves, right]
-
-        expect do
-          EpicIssue.move_nulls_to_end(level)
-        end.not_to change { right.reset.relative_position }
-
-        moved.each(&:reset)
-
-        expect(moved.map(&:relative_position)).to all(be > right.relative_position)
-      end
-    end
   end
 end

ee/spec/models/epic_spec.rb

@@ -616,19 +616,9 @@ RSpec.describe Epic do
   end
 
   context "relative positioning" do
-    context 'there is no parent' do
-      it_behaves_like "a class that supports relative positioning" do
-        let(:factory) { :epic }
-        let(:default_params) { {} }
-      end
-    end
-
-    context 'there is a parent' do
-      it_behaves_like "a class that supports relative positioning" do
-        let_it_be(:parent) { create(:epic) }
-        let(:factory) { :epic }
-        let(:default_params) { { parent: parent, group: parent.group } }
-      end
+    it_behaves_like "a class that supports relative positioning" do
+      let(:factory) { :epic }
+      let(:default_params) { {} }
     end
   end
 

ee/spec/models/issue_spec.rb

@@ -454,14 +454,20 @@ RSpec.describe Issue do
   end
 
   describe 'relative positioning with group boards' do
-    let_it_be(:group) { create(:group) }
-    let_it_be(:board) { create(:board, group: group) }
-    let_it_be(:project) { create(:project, namespace: group) }
-    let_it_be(:project1) { create(:project, namespace: group) }
-    let_it_be_with_reload(:issue) { create(:issue, project: project) }
-    let_it_be_with_reload(:issue1) { create(:issue, project: project1, relative_position: issue.relative_position + RelativePositioning::IDEAL_DISTANCE) }
+    let(:group) { create(:group) }
+    let!(:board) { create(:board, group: group) }
+    let(:project) { create(:project, namespace: group) }
+    let(:project1) { create(:project, namespace: group) }
+    let(:issue) { build(:issue, project: project) }
+    let(:issue1) { build(:issue, project: project1) }
     let(:new_issue) { build(:issue, project: project1, relative_position: nil) }
 
+    before do
+      [issue, issue1].each do |issue|
+        issue.move_to_end && issue.save
+      end
+    end
+
     describe '#max_relative_position' do
       it 'returns maximum position' do
         expect(issue.max_relative_position).to eq issue1.relative_position
@@ -540,20 +546,18 @@ RSpec.describe Issue do
         issue1.update relative_position: issue.relative_position
 
         new_issue.move_between(issue, issue1)
-        [issue, issue1].each(&:reset)
 
-        expect(new_issue.relative_position)
-          .to be_between(issue.relative_position, issue1.relative_position).exclusive
+        expect(new_issue.relative_position).to be > issue.relative_position
+        expect(issue.relative_position).to be < issue1.relative_position
       end
 
       it 'positions issues between other two if distance is 1' do
         issue1.update relative_position: issue.relative_position + 1
 
         new_issue.move_between(issue, issue1)
-        [issue, issue1].each(&:reset)
 
-        expect(new_issue.relative_position)
-          .to be_between(issue.relative_position, issue1.relative_position).exclusive
+        expect(new_issue.relative_position).to be > issue.relative_position
+        expect(issue.relative_position).to be < issue1.relative_position
       end
 
       it 'positions issue in the middle of other two if distance is big enough' do
@@ -562,20 +566,24 @@ RSpec.describe Issue do
 
         new_issue.move_between(issue, issue1)
 
-        expect(new_issue.relative_position)
-          .to be_between(issue.relative_position, issue1.relative_position).exclusive
+        expect(new_issue.relative_position).to eq(8000)
       end
 
       it 'positions issue closer to the middle if we are at the very top' do
-        new_issue.move_between(nil, issue)
+        issue1.update relative_position: 6000
 
-        expect(new_issue.relative_position).to eq(issue.relative_position - RelativePositioning::IDEAL_DISTANCE)
+        new_issue.move_between(nil, issue1)
+
+        expect(new_issue.relative_position).to eq(6000 - RelativePositioning::IDEAL_DISTANCE)
       end
 
       it 'positions issue closer to the middle if we are at the very bottom' do
-        new_issue.move_between(issue1, nil)
+        issue.update relative_position: 6000
+        issue1.update relative_position: nil
+
+        new_issue.move_between(issue, nil)
 
-        expect(new_issue.relative_position).to eq(issue1.relative_position + RelativePositioning::IDEAL_DISTANCE)
+        expect(new_issue.relative_position).to eq(6000 + RelativePositioning::IDEAL_DISTANCE)
       end
 
       it 'positions issue in the middle of other two if distance is not big enough' do
@@ -592,10 +600,8 @@ RSpec.describe Issue do
         issue1.update relative_position: 101
 
         new_issue.move_between(issue, issue1)
-        [issue, issue1].each(&:reset)
 
-        expect(new_issue.relative_position)
-          .to be_between(issue.relative_position, issue1.relative_position).exclusive
+        expect(new_issue.relative_position).to be_between(issue.relative_position, issue1.relative_position)
       end
 
       it 'uses rebalancing if there is no place' do
@@ -606,15 +612,12 @@ RSpec.describe Issue do
 
         new_issue.move_between(issue1, issue2)
         new_issue.save!
-        [issue, issue1, issue2].each(&:reset)
-
-        expect(new_issue.relative_position)
-          .to be_between(issue1.relative_position, issue2.relative_position).exclusive
 
-        expect([issue, issue1, issue2, new_issue].map(&:relative_position).uniq).to have_attributes(size: 4)
+        expect(new_issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
+        expect(issue.reload.relative_position).not_to eq(100)
       end
 
-      it 'positions issue right if we pass non-sequential parameters' do
+      it 'positions issue right if we pass none-sequential parameters' do
         issue.update relative_position: 99
         issue1.update relative_position: 101
         issue2 = create(:issue, relative_position: 102, project: project)

ee/spec/services/epic_issues/create_service_spec.rb

@@ -169,9 +169,7 @@ RSpec.describe EpicIssues::CreateService do
           end
 
           context 'when multiple valid issues are given' do
-            let(:references) { [issue, issue2].map { |i| i.to_reference(full: true) } }
-
-            subject { assign_issue(references) }
+            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) }
@@ -183,18 +181,13 @@ RSpec.describe EpicIssues::CreateService do
               expect(created_link2).to have_attributes(epic: epic)
             end
 
-            it 'places each issue at the start' do
-              subject
-
-              expect(created_link2.relative_position).to be < created_link1.relative_position
-            end
             it 'orders the epic issues to the first place and moves the existing ones down' do
               existing_link = create(:epic_issue, epic: epic, issue: issue3)
 
               subject
 
-              expect([created_link1, created_link2].map(&:relative_position))
-                .to all(be < existing_link.reset.relative_position)
+              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

lib/gitlab/database.rb

@@ -22,7 +22,6 @@ module Gitlab
 
     # https://www.postgresql.org/docs/9.2/static/datatype-numeric.html
     MAX_INT_VALUE = 2147483647
-    MIN_INT_VALUE = -2147483648
 
     # The max value between MySQL's TIMESTAMP and PostgreSQL's timestampz:
     # https://www.postgresql.org/docs/9.1/static/datatype-datetime.html

spec/features/boards/issue_ordering_spec.rb

@@ -21,7 +21,7 @@ RSpec.describe 'Issue Boards', :js do
   end
 
   context 'un-ordered issues' do
-    let!(:issue4) { create(:labeled_issue, project: project, labels: [label], relative_position: nil) }
+    let!(:issue4) { create(:labeled_issue, project: project, labels: [label]) }
 
     before do
       visit project_board_path(project, board)

spec/models/analytics/cycle_analytics/project_stage_spec.rb

@@ -23,7 +23,7 @@ RSpec.describe Analytics::CycleAnalytics::ProjectStage do
 
   context 'relative positioning' do
     it_behaves_like 'a class that supports relative positioning' do
-      let_it_be(:project) { create(:project) }
+      let(:project) { build(:project) }
       let(:factory) { :cycle_analytics_project_stage }
       let(:default_params) { { project: project } }
     end

spec/models/issue_spec.rb

@@ -6,7 +6,6 @@ RSpec.describe Issue do
   include ExternalAuthorizationServiceHelpers
 
   let_it_be(:user) { create(:user) }
-  let_it_be(:reusable_project) { create(:project) }
 
   describe "Associations" do
     it { is_expected.to belong_to(:milestone) }
@@ -146,13 +145,13 @@ RSpec.describe Issue do
   end
 
   describe '#order_by_position_and_priority' do
-    let(:project) { reusable_project }
+    let(:project) { create :project }
     let(:p1) { create(:label, title: 'P1', project: project, priority: 1) }
     let(:p2) { create(:label, title: 'P2', project: project, priority: 2) }
     let!(:issue1) { create(:labeled_issue, project: project, labels: [p1]) }
     let!(:issue2) { create(:labeled_issue, project: project, labels: [p2]) }
-    let!(:issue3) { create(:issue, project: project, relative_position: -200) }
-    let!(:issue4) { create(:issue, project: project, relative_position: -100) }
+    let!(:issue3) { create(:issue, project: project, relative_position: 100) }
+    let!(:issue4) { create(:issue, project: project, relative_position: 200) }
 
     it 'returns ordered list' do
       expect(project.issues.order_by_position_and_priority)
@@ -161,10 +160,10 @@ RSpec.describe Issue do
   end
 
   describe '#sort' do
-    let(:project) { reusable_project }
+    let(:project) { create(:project) }
 
     context "by relative_position" do
-      let!(:issue)  { create(:issue, project: project, relative_position: nil) }
+      let!(:issue)  { create(:issue, project: project) }
       let!(:issue2) { create(:issue, project: project, relative_position: 2) }
       let!(:issue3) { create(:issue, project: project, relative_position: 1) }
 
@@ -1028,7 +1027,7 @@ RSpec.describe Issue do
 
   context "relative positioning" do
     it_behaves_like "a class that supports relative positioning" do
-      let_it_be(:project) { create(:project) }
+      let(:project) { create(:project) }
       let(:factory) { :issue }
       let(:default_params) { { project: project } }
     end