diff options
| -rw-r--r-- | app/models/concerns/relative_positioning.rb | 52 | ||||
| -rw-r--r-- | app/models/issue.rb | 6 | ||||
| -rw-r--r-- | spec/models/concerns/relative_positioning_spec.rb | 242 | ||||
| -rw-r--r-- | spec/models/issue_spec.rb | 8 | ||||
| -rw-r--r-- | spec/support/shared_examples/relative_positioning_shared_examples.rb | 253 |
5 files changed, 300 insertions, 261 deletions
diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb index e4fe46d722a..9cd7b8d6258 100644 --- a/app/models/concerns/relative_positioning.rb +++ b/app/models/concerns/relative_positioning.rb @@ -1,5 +1,26 @@ # frozen_string_literal: true +# This module makes it possible to handle items as a list, where the order of items can be easily altered +# Requirements: +# +# - Only works for ActiveRecord models +# - relative_position integer field must present on the model +# - This module uses GROUP BY: the model should have a parent relation, example: project -> issues, project is the parent relation (issues table has a parent_id column) +# +# Setup like this in the body of your class: +# +# include RelativePositioning +# +# # base query used for the position calculation +# def self.relative_positioning_query_base(issue) +# where(deleted: false) +# end +# +# # column that should be used in GROUP BY +# def self.relative_positioning_parent_column +# :project_id +# end +# module RelativePositioning extend ActiveSupport::Concern @@ -93,7 +114,7 @@ module RelativePositioning return move_after(before) unless after return move_before(after) unless before - # If there is no place to insert an issue we need to create one by moving the before issue closer + # If there is no place to insert an item we need to create one by moving the before item closer # to its predecessor. This process will recursively move all the predecessors until we have a place if (after.relative_position - before.relative_position) < 2 before.move_before @@ -108,11 +129,11 @@ module RelativePositioning pos_after = before.next_relative_position if before.shift_after? - issue_to_move = self.class.in_parents(parent_ids).find_by!(relative_position: pos_after) - issue_to_move.move_after - @positionable_neighbours = [issue_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables + item_to_move = self.class.relative_positioning_query_base(self).find_by!(relative_position: pos_after) + item_to_move.move_after + @positionable_neighbours = [item_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables - pos_after = issue_to_move.relative_position + pos_after = item_to_move.relative_position end self.relative_position = self.class.position_between(pos_before, pos_after) @@ -123,11 +144,11 @@ module RelativePositioning pos_before = after.prev_relative_position if after.shift_before? - issue_to_move = self.class.in_parents(parent_ids).find_by!(relative_position: pos_before) - issue_to_move.move_before - @positionable_neighbours = [issue_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables + item_to_move = self.class.relative_positioning_query_base(self).find_by!(relative_position: pos_before) + item_to_move.move_before + @positionable_neighbours = [item_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables - pos_before = issue_to_move.relative_position + pos_before = item_to_move.relative_position end self.relative_position = self.class.position_between(pos_before, pos_after) @@ -141,13 +162,13 @@ module RelativePositioning self.relative_position = self.class.position_between(min_relative_position || START_POSITION, MIN_POSITION) end - # Indicates if there is an issue that should be shifted to free the place + # Indicates if there is an item that should be shifted to free the place def shift_after? next_pos = next_relative_position next_pos && (next_pos - relative_position) == 1 end - # Indicates if there is an issue that should be shifted to free the place + # Indicates if there is an item that should be shifted to free the place def shift_before? prev_pos = prev_relative_position prev_pos && (relative_position - prev_pos) == 1 @@ -159,7 +180,7 @@ module RelativePositioning def save_positionable_neighbours return unless @positionable_neighbours - status = @positionable_neighbours.all? { |issue| issue.save(touch: false) } + status = @positionable_neighbours.all? { |item| item.save(touch: false) } @positionable_neighbours = nil status @@ -170,16 +191,15 @@ module RelativePositioning # When calculating across projects, this is much more efficient than # MAX(relative_position) without the GROUP BY, due to index usage: # https://gitlab.com/gitlab-org/gitlab-ce/issues/54276#note_119340977 - relation = self.class - .in_parents(parent_ids) + relation = self.class.relative_positioning_query_base(self) .order(Gitlab::Database.nulls_last_order('position', 'DESC')) + .group(self.class.relative_positioning_parent_column) .limit(1) - .group(self.class.parent_column) relation = yield relation if block_given? relation - .pluck(self.class.parent_column, Arel.sql("#{calculation}(relative_position) AS position")) + .pluck(self.class.relative_positioning_parent_column, Arel.sql("#{calculation}(relative_position) AS position")) .first&. last end diff --git a/app/models/issue.rb b/app/models/issue.rb index 8c5dd5e382e..164858dc432 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -91,11 +91,11 @@ class Issue < ApplicationRecord end end - class << self - alias_method :in_parents, :in_projects + def self.relative_positioning_query_base(issue) + in_projects(issue.parent_ids) end - def self.parent_column + def self.relative_positioning_parent_column :project_id end diff --git a/spec/models/concerns/relative_positioning_spec.rb b/spec/models/concerns/relative_positioning_spec.rb deleted file mode 100644 index d0ae45f7871..00000000000 --- a/spec/models/concerns/relative_positioning_spec.rb +++ /dev/null @@ -1,242 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe RelativePositioning do - let(:project) { create(:project) } - let(:issue) { create(:issue, project: project) } - let(:issue1) { create(:issue, project: project) } - let(:new_issue) { create(:issue, project: project) } - - describe '.move_to_end' do - it 'moves the object to the end' do - Issue.move_to_end([issue, issue1]) - - expect(issue1.prev_relative_position).to eq issue.relative_position - expect(issue.prev_relative_position).to eq nil - expect(issue1.next_relative_position).to eq nil - end - - it 'does not perform any moves if all issues have their relative_position set' do - issue.update!(relative_position: 1) - - expect(issue).not_to receive(:save) - - Issue.move_to_end([issue]) - end - end - - describe '#max_relative_position' do - it 'returns maximum position' do - expect(issue.max_relative_position).to eq issue1.relative_position - end - end - - describe '#prev_relative_position' do - it 'returns previous position if there is an issue above' do - expect(issue1.prev_relative_position).to eq issue.relative_position - end - - it 'returns nil if there is no issue above' do - expect(issue.prev_relative_position).to eq nil - end - end - - describe '#next_relative_position' do - it 'returns next position if there is an issue below' do - expect(issue.next_relative_position).to eq issue1.relative_position - end - - it 'returns nil if there is no issue below' do - expect(issue1.next_relative_position).to eq nil - end - end - - describe '#move_before' do - it 'moves issue before' do - [issue1, issue].each(&:move_to_end) - - issue.move_before(issue1) - - expect(issue.relative_position).to be < issue1.relative_position - end - end - - describe '#move_after' do - it 'moves issue after' do - [issue, issue1].each(&:move_to_end) - - issue.move_after(issue1) - - expect(issue.relative_position).to be > issue1.relative_position - end - end - - describe '#move_to_end' do - before do - [issue, issue1].each do |issue| - issue.move_to_end && issue.save - end - end - - it 'moves issue to the end' do - new_issue.move_to_end - - expect(new_issue.relative_position).to be > issue1.relative_position - end - end - - describe '#shift_after?' do - before do - [issue, issue1].each do |issue| - issue.move_to_end && issue.save - end - end - - it 'returns true' do - issue.update(relative_position: issue1.relative_position - 1) - - expect(issue.shift_after?).to be_truthy - end - - it 'returns false' do - issue.update(relative_position: issue1.relative_position - 2) - - expect(issue.shift_after?).to be_falsey - end - end - - describe '#shift_before?' do - before do - [issue, issue1].each do |issue| - issue.move_to_end && issue.save - end - end - - it 'returns true' do - issue.update(relative_position: issue1.relative_position + 1) - - expect(issue.shift_before?).to be_truthy - end - - it 'returns false' do - issue.update(relative_position: issue1.relative_position + 2) - - expect(issue.shift_before?).to be_falsey - end - end - - describe '#move_between' do - before do - [issue, issue1].each do |issue| - issue.move_to_end && issue.save - end - end - - it 'positions issue between two other' do - new_issue.move_between(issue, issue1) - - expect(new_issue.relative_position).to be > issue.relative_position - expect(new_issue.relative_position).to be < issue1.relative_position - end - - it 'positions issue between on top' do - new_issue.move_between(nil, issue) - - expect(new_issue.relative_position).to be < issue.relative_position - end - - it 'positions issue between to end' do - new_issue.move_between(issue1, nil) - - expect(new_issue.relative_position).to be > issue1.relative_position - end - - it 'positions issues even when after and before positions are the same' do - issue1.update relative_position: issue.relative_position - - new_issue.move_between(issue, issue1) - - 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) - - 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 - issue.update relative_position: 6000 - issue1.update relative_position: 10000 - - new_issue.move_between(issue, issue1) - - expect(new_issue.relative_position).to eq(8000) - end - - it 'positions issue closer to the middle if we are at the very top' do - issue1.update relative_position: 6000 - - 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 - issue.update relative_position: 6000 - issue1.update relative_position: nil - - new_issue.move_between(issue, nil) - - 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 - issue.update relative_position: 100 - issue1.update relative_position: 400 - - new_issue.move_between(issue, issue1) - - expect(new_issue.relative_position).to eq(250) - end - - it 'positions issue in the middle of other two is there is no place' do - issue.update relative_position: 100 - issue1.update relative_position: 101 - - new_issue.move_between(issue, issue1) - - expect(new_issue.relative_position).to be_between(issue.relative_position, issue1.relative_position) - end - - it 'uses rebalancing if there is no place' do - issue.update relative_position: 100 - issue1.update relative_position: 101 - issue2 = create(:issue, relative_position: 102, project: project) - new_issue.update relative_position: 103 - - new_issue.move_between(issue1, issue2) - new_issue.save! - - 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 none-sequential parameters' do - issue.update relative_position: 99 - issue1.update relative_position: 101 - issue2 = create(:issue, relative_position: 102, project: project) - new_issue.update relative_position: 103 - - new_issue.move_between(issue, issue2) - new_issue.save! - - expect(new_issue.relative_position).to be(100) - end - end -end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index d5b016dc8f6..2e7d78d77a8 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -871,4 +871,12 @@ describe Issue do expect(issue.labels_hook_attrs).to eq([label.hook_attrs]) end end + + context "relative positioning" do + it_behaves_like "a class that supports relative positioning" do + let(:project) { create(:project) } + let(:factory) { :issue } + let(:default_params) { { project: project } } + end + end end diff --git a/spec/support/shared_examples/relative_positioning_shared_examples.rb b/spec/support/shared_examples/relative_positioning_shared_examples.rb new file mode 100644 index 00000000000..5ee62644c54 --- /dev/null +++ b/spec/support/shared_examples/relative_positioning_shared_examples.rb @@ -0,0 +1,253 @@ +# frozen_string_literal: true + +RSpec.shared_examples "a class that supports relative positioning" do + let(:item1) { create(factory, default_params) } + let(:item2) { create(factory, default_params) } + let(:new_item) { create(factory, default_params) } + + def create_item(params) + create(factory, params.merge(default_params)) + end + + describe '.move_to_end' do + it 'moves the object to the end' do + item1.update(relative_position: 5) + item2.update(relative_position: 15) + + described_class.move_to_end([item1, item2]) + + expect(item2.prev_relative_position).to eq item1.relative_position + expect(item1.prev_relative_position).to eq nil + expect(item2.next_relative_position).to eq nil + end + + it 'does not perform any moves if all items have their relative_position set' do + item1.update!(relative_position: 1) + + expect(item1).not_to receive(:save) + + described_class.move_to_end([item1]) + end + end + + describe '#max_relative_position' do + it 'returns maximum position' do + expect(item1.max_relative_position).to eq item2.relative_position + end + end + + describe '#prev_relative_position' do + it 'returns previous position if there is an item above' do + item1.update(relative_position: 5) + item2.update(relative_position: 15) + + expect(item2.prev_relative_position).to eq item1.relative_position + end + + it 'returns nil if there is no item above' do + expect(item1.prev_relative_position).to eq nil + end + end + + describe '#next_relative_position' do + it 'returns next position if there is an item below' do + item1.update(relative_position: 5) + item2.update(relative_position: 15) + + expect(item1.next_relative_position).to eq item2.relative_position + end + + it 'returns nil if there is no item below' do + expect(item2.next_relative_position).to eq nil + end + end + + describe '#move_before' do + it 'moves item before' do + [item2, item1].each(&:move_to_end) + + item1.move_before(item2) + + expect(item1.relative_position).to be < item2.relative_position + end + end + + describe '#move_after' do + it 'moves item after' do + [item1, item2].each(&:move_to_end) + + item1.move_after(item2) + + expect(item1.relative_position).to be > item2.relative_position + end + end + + describe '#move_to_end' do + before do + [item1, item2].each do |item1| + item1.move_to_end && item1.save + end + end + + it 'moves item to the end' do + new_item.move_to_end + + expect(new_item.relative_position).to be > item2.relative_position + end + end + + describe '#shift_after?' do + before do + [item1, item2].each do |item1| + item1.move_to_end && item1.save + end + end + + it 'returns true' do + item1.update(relative_position: item2.relative_position - 1) + + expect(item1.shift_after?).to be_truthy + end + + it 'returns false' do + item1.update(relative_position: item2.relative_position - 2) + + expect(item1.shift_after?).to be_falsey + end + end + + describe '#shift_before?' do + before do + [item1, item2].each do |item1| + item1.move_to_end && item1.save + end + end + + it 'returns true' do + item1.update(relative_position: item2.relative_position + 1) + + expect(item1.shift_before?).to be_truthy + end + + it 'returns false' do + item1.update(relative_position: item2.relative_position + 2) + + expect(item1.shift_before?).to be_falsey + end + end + + describe '#move_between' do + before do + [item1, item2].each do |item1| + item1.move_to_end && item1.save + end + end + + it 'positions item between two other' do + new_item.move_between(item1, item2) + + expect(new_item.relative_position).to be > item1.relative_position + expect(new_item.relative_position).to be < item2.relative_position + end + + it 'positions item between on top' do + new_item.move_between(nil, item1) + + expect(new_item.relative_position).to be < item1.relative_position + end + + it 'positions item between to end' do + new_item.move_between(item2, nil) + + expect(new_item.relative_position).to be > item2.relative_position + end + + it 'positions items even when after and before positions are the same' do + item2.update relative_position: item1.relative_position + + new_item.move_between(item1, item2) + + expect(new_item.relative_position).to be > item1.relative_position + expect(item1.relative_position).to be < item2.relative_position + end + + it 'positions items between other two if distance is 1' do + item2.update relative_position: item1.relative_position + 1 + + new_item.move_between(item1, item2) + + expect(new_item.relative_position).to be > item1.relative_position + expect(item1.relative_position).to be < item2.relative_position + end + + it 'positions item in the middle of other two if distance is big enough' do + item1.update relative_position: 6000 + item2.update relative_position: 10000 + + new_item.move_between(item1, item2) + + expect(new_item.relative_position).to eq(8000) + end + + it 'positions item closer to the middle if we are at the very top' do + item2.update relative_position: 6000 + + new_item.move_between(nil, item2) + + expect(new_item.relative_position).to eq(6000 - RelativePositioning::IDEAL_DISTANCE) + end + + it 'positions item closer to the middle if we are at the very bottom' do + new_item.update relative_position: 1 + item1.update relative_position: 6000 + item2.destroy + + new_item.move_between(item1, nil) + + expect(new_item.relative_position).to eq(6000 + RelativePositioning::IDEAL_DISTANCE) + end + + it 'positions item in the middle of other two if distance is not big enough' do + item1.update relative_position: 100 + item2.update relative_position: 400 + + new_item.move_between(item1, item2) + + expect(new_item.relative_position).to eq(250) + end + + it 'positions item in the middle of other two is there is no place' do + item1.update relative_position: 100 + item2.update relative_position: 101 + + new_item.move_between(item1, item2) + + expect(new_item.relative_position).to be_between(item1.relative_position, item2.relative_position) + end + + it 'uses rebalancing if there is no place' do + item1.update relative_position: 100 + item2.update relative_position: 101 + item3 = create_item(relative_position: 102) + new_item.update relative_position: 103 + + new_item.move_between(item2, item3) + new_item.save! + + expect(new_item.relative_position).to be_between(item2.relative_position, item3.relative_position) + expect(item1.reload.relative_position).not_to eq(100) + end + + it 'positions item right if we pass none-sequential parameters' do + item1.update relative_position: 99 + item2.update relative_position: 101 + item3 = create_item(relative_position: 102) + new_item.update relative_position: 103 + + new_item.move_between(item1, item3) + new_item.save! + + expect(new_item.relative_position).to be(100) + end + end +end |