Commit e752d6d1 authored by Valery Sizov's avatar Valery Sizov

[Issue sorting]Addressed review comments

parent b84723ac
...@@ -2,3 +2,4 @@ ...@@ -2,3 +2,4 @@
lib/gitlab/sanitizers/svg/whitelist.rb lib/gitlab/sanitizers/svg/whitelist.rb
lib/gitlab/diff/position_tracer.rb lib/gitlab/diff/position_tracer.rb
app/policies/project_policy.rb app/policies/project_policy.rb
app/models/concerns/relative_positioning.rb
...@@ -4,7 +4,7 @@ module RelativePositioning ...@@ -4,7 +4,7 @@ module RelativePositioning
MIN_POSITION = 0 MIN_POSITION = 0
START_POSITION = Gitlab::Database::MAX_INT_VALUE / 2 START_POSITION = Gitlab::Database::MAX_INT_VALUE / 2
MAX_POSITION = Gitlab::Database::MAX_INT_VALUE MAX_POSITION = Gitlab::Database::MAX_INT_VALUE
DISTANCE = 500 IDEAL_DISTANCE = 500
included do included do
after_save :save_positionable_neighbours after_save :save_positionable_neighbours
...@@ -44,55 +44,86 @@ def move_between(before, after) ...@@ -44,55 +44,86 @@ def move_between(before, after)
return move_after(before) unless after return move_after(before) unless after
return move_before(after) unless before 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
# 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
@positionable_neighbours = [before]
end
self.relative_position = position_between(before.relative_position, after.relative_position)
end
def move_after(before = self)
pos_before = before.relative_position pos_before = before.relative_position
pos_after = after.relative_position pos_after = before.next_relative_position
# We can't insert an issue between two other if the distance is 1 or 0 if before.shift_after?
# so we need to handle this collision properly issue_to_move = self.class.in_projects(project.id).find_by!(relative_position: pos_after)
if pos_after && (pos_after - pos_before).abs <= 1 issue_to_move.move_after
self.relative_position = pos_before @positionable_neighbours = [issue_to_move]
before.move_before(self)
after.move_after(self)
@positionable_neighbours = [before, after] pos_after = issue_to_move.relative_position
else
self.relative_position = position_between(pos_before, pos_after)
end end
end
def move_before(after) self.relative_position = position_between(pos_before, pos_after)
self.relative_position = position_between(after.prev_relative_position, after.relative_position)
end end
def move_after(before) def move_before(after = self)
self.relative_position = position_between(before.relative_position, before.next_relative_position) pos_after = after.relative_position
pos_before = after.prev_relative_position
if after.shift_before?
issue_to_move = self.class.in_projects(project.id).find_by!(relative_position: pos_before)
issue_to_move.move_before
@positionable_neighbours = [issue_to_move]
pos_before = issue_to_move.relative_position
end
self.relative_position = position_between(pos_before, pos_after)
end end
def move_to_end def move_to_end
self.relative_position = position_between(max_relative_position || START_POSITION, MAX_POSITION) self.relative_position = position_between(max_relative_position || START_POSITION, MAX_POSITION)
end end
# Indicates if there is an issue 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
def shift_before?
prev_pos = prev_relative_position
prev_pos && (relative_position - prev_pos) == 1
end
private private
# This method takes two integer values (positions) and # This method takes two integer values (positions) and
# calculates the position between them. The range is huge as # calculates the position between them. The range is huge as
# the maximum integer value is 2147483647. We are incrementing position by 1000 every time # the maximum integer value is 2147483647. We are incrementing position by IDEAL_DISTANCE * 2 every time
# when we have enough space. If distance is less then 500 we are calculating an average number # when we have enough space. If distance is less then IDEAL_DISTANCE we are calculating an average number
def position_between(pos_before, pos_after) def position_between(pos_before, pos_after)
pos_before ||= MIN_POSITION pos_before ||= MIN_POSITION
pos_after ||= MAX_POSITION pos_after ||= MAX_POSITION
pos_before, pos_after = [pos_before, pos_after].sort pos_before, pos_after = [pos_before, pos_after].sort
if pos_after - pos_before < DISTANCE * 2 halfway = (pos_after + pos_before) / 2
(pos_after + pos_before) / 2 distance_to_halfway = pos_after - halfway
if distance_to_halfway < IDEAL_DISTANCE
halfway
else else
if pos_before == MIN_POSITION if pos_before == MIN_POSITION
pos_after - DISTANCE pos_after - IDEAL_DISTANCE
elsif pos_after == MAX_POSITION elsif pos_after == MAX_POSITION
pos_before + DISTANCE pos_before + IDEAL_DISTANCE
else else
(pos_after + pos_before) / 2 halfway
end end
end end
end end
......
...@@ -66,6 +66,34 @@ ...@@ -66,6 +66,34 @@
end end
end end
describe '#shift_after?' do
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
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 describe '#move_between' do
it 'positions issue between two other' do it 'positions issue between two other' do
new_issue.move_between(issue, issue1) new_issue.move_between(issue, issue1)
...@@ -118,7 +146,7 @@ ...@@ -118,7 +146,7 @@
new_issue.move_between(nil, issue1) new_issue.move_between(nil, issue1)
expect(new_issue.relative_position).to eq(6000 - RelativePositioning::DISTANCE) expect(new_issue.relative_position).to eq(6000 - RelativePositioning::IDEAL_DISTANCE)
end end
it 'positions issue closer to the middle if we are at the very bottom' do it 'positions issue closer to the middle if we are at the very bottom' do
...@@ -127,7 +155,7 @@ ...@@ -127,7 +155,7 @@
new_issue.move_between(issue, nil) new_issue.move_between(issue, nil)
expect(new_issue.relative_position).to eq(6000 + RelativePositioning::DISTANCE) expect(new_issue.relative_position).to eq(6000 + RelativePositioning::IDEAL_DISTANCE)
end end
it 'positions issue in the middle of other two if distance is not big enough' do it 'positions issue in the middle of other two if distance is not big enough' do
...@@ -138,5 +166,39 @@ ...@@ -138,5 +166,39 @@
expect(new_issue.relative_position).to eq(250) expect(new_issue.relative_position).to eq(250)
end 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
end end
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment