kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Merge branch 'make-relative-positioning-module-reusable' into 'master'

Browse source 
Make RelativePositioning reusable

See merge request gitlab-org/gitlab-ce!30993
This commit is contained in:
Bob Van Landuyt 2019-07-25 07:13:58 +00:00
commit ba997f3c42
5 changed files with 300 additions and 261 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

52
app/models/concerns/relative_positioning.rb
View file

@ -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

6
app/models/issue.rb
View file

@ -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

242
spec/models/concerns/relative_positioning_spec.rb
View file

@ -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

8
spec/models/issue_spec.rb
View file

@ -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

253
spec/support/shared_examples/relative_positioning_shared_examples.rb Normal file
View file

@ -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