Commit 89920136 authored by Heinrich Lee Yu's avatar Heinrich Lee Yu

Fix bug when moving batches of items to the end

Starts from START_POSITION when there are no existing
positions.

Also improves the test to actually test the behavior
parent 2e7f4bbb
...@@ -26,7 +26,7 @@ module Boards ...@@ -26,7 +26,7 @@ module Boards
list_service = Boards::Issues::ListService.new(board_parent, current_user, filter_params) list_service = Boards::Issues::ListService.new(board_parent, current_user, filter_params)
issues = list_service.execute issues = list_service.execute
issues = issues.page(params[:page]).per(params[:per] || 20).without_count issues = issues.page(params[:page]).per(params[:per] || 20).without_count
Issue.move_to_end(issues) if Gitlab::Database.read_write? Issue.move_nulls_to_end(issues) if Gitlab::Database.read_write?
issues = issues.preload(:milestone, issues = issues.preload(:milestone,
:assignees, :assignees,
project: [ project: [
......
...@@ -34,7 +34,7 @@ module RelativePositioning ...@@ -34,7 +34,7 @@ module RelativePositioning
end end
class_methods do class_methods do
def move_to_end(objects) def move_nulls_to_end(objects)
objects = objects.reject(&:relative_position) objects = objects.reject(&:relative_position)
return if objects.empty? return if objects.empty?
...@@ -43,7 +43,7 @@ module RelativePositioning ...@@ -43,7 +43,7 @@ module RelativePositioning
self.transaction do self.transaction do
objects.each do |object| objects.each do |object|
relative_position = position_between(max_relative_position, MAX_POSITION) relative_position = position_between(max_relative_position || START_POSITION, MAX_POSITION)
object.relative_position = relative_position object.relative_position = relative_position
max_relative_position = relative_position max_relative_position = relative_position
object.save(touch: false) object.save(touch: false)
......
...@@ -9,24 +9,27 @@ RSpec.shared_examples "a class that supports relative positioning" do ...@@ -9,24 +9,27 @@ RSpec.shared_examples "a class that supports relative positioning" do
create(factory, params.merge(default_params)) create(factory, params.merge(default_params))
end end
describe '.move_to_end' do describe '.move_nulls_to_end' do
it 'moves the object to the end' do it 'moves items with null relative_position to the end' do
item1.update(relative_position: 5) described_class.move_nulls_to_end([item1, item2])
item2.update(relative_position: 15)
described_class.move_to_end([item1, item2])
expect(item2.prev_relative_position).to eq item1.relative_position expect(item2.prev_relative_position).to eq item1.relative_position
expect(item1.prev_relative_position).to eq nil expect(item1.prev_relative_position).to eq nil
expect(item2.next_relative_position).to eq nil expect(item2.next_relative_position).to eq nil
end end
it 'moves the item near the start position when there are no existing positions' do
described_class.move_nulls_to_end([item1])
expect(item1.relative_position).to eq(described_class::START_POSITION + described_class::IDEAL_DISTANCE)
end
it 'does not perform any moves if all items have their relative_position set' do it 'does not perform any moves if all items have their relative_position set' do
item1.update!(relative_position: 1) item1.update!(relative_position: 1)
expect(item1).not_to receive(:save) expect(item1).not_to receive(:save)
described_class.move_to_end([item1]) described_class.move_nulls_to_end([item1])
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