Addressing review comments

This commit is contained in:
Valery Sizov 2017-03-06 18:08:18 +02:00
parent 91e46dd4a2
commit 13caadea7a
3 changed files with 40 additions and 168 deletions

View file

@ -4,10 +4,6 @@ module RelativePositioning
MIN_POSITION = 0
MAX_POSITION = Gitlab::Database::MAX_INT_VALUE
included do
after_save :save_positionable_neighbours
end
def min_relative_position
self.class.in_projects(project.id).minimum(:relative_position)
end
@ -16,92 +12,24 @@ module RelativePositioning
self.class.in_projects(project.id).maximum(:relative_position)
end
def prev_relative_position
prev_pos = nil
if self.relative_position
prev_pos = self.class.
in_projects(project.id).
where('relative_position < ?', self.relative_position).
maximum(:relative_position)
end
prev_pos || MIN_POSITION
end
def next_relative_position
next_pos = nil
if self.relative_position
next_pos = self.class.
in_projects(project.id).
where('relative_position > ?', self.relative_position).
minimum(:relative_position)
end
next_pos || MAX_POSITION
end
def move_between(before, after)
return move_after(before) if before && !after
return move_before(after) if after && !before
return move_to_end unless after
return move_to_top unless before
pos_before = before.relative_position
pos_after = after.relative_position
if pos_before && pos_after
if pos_before == pos_after
self.relative_position = pos_before
before.move_before(self)
after.move_after(self)
@positionable_neighbours = [before, after]
else
self.relative_position = position_between(pos_before, pos_after)
end
elsif pos_before
self.move_after(before)
after.move_after(self)
@positionable_neighbours = [after]
elsif pos_after
self.move_before(after)
before.move_before(self)
@positionable_neighbours = [before]
if pos_after && (pos_before == pos_after)
self.relative_position = pos_before
before.decrement! :relative_position
after.increment! :relative_position
else
move_to_end
before.move_before(self)
after.move_after(self)
@positionable_neighbours = [before, after]
self.relative_position = position_between(pos_before, pos_after)
end
end
def move_before(after)
pos_after = after.relative_position
if pos_after
self.relative_position = position_between(MIN_POSITION, pos_after)
else
move_to_end
after.move_after(self)
@positionable_neighbours = [after]
end
end
def move_after(before)
pos_before = before.relative_position
if pos_before
self.relative_position = position_between(pos_before, MAX_POSITION)
else
move_to_end
before.move_before(self)
@positionable_neighbours = [before]
end
def move_to_top
self.relative_position = position_between(MIN_POSITION, min_relative_position)
end
def move_to_end
@ -129,13 +57,4 @@ module RelativePositioning
rand(pos_before.next..pos_after.pred)
end
def save_positionable_neighbours
return unless @positionable_neighbours
status = @positionable_neighbours.all?(&:save)
@positionable_neighbours = nil
status
end
end

View file

@ -12,68 +12,27 @@ describe Issue, 'RelativePositioning' do
end
describe '#min_relative_position' do
it 'returns minimum position' do
expect(issue1.min_relative_position).to eq issue.relative_position
it 'returns maximum position' do
expect(issue.min_relative_position).to eq issue.relative_position
end
end
describe '#man_relative_position' do
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
describe '#move_to_top' do
it 'moves issue to the end' do
new_issue = create :issue, project: project
it 'returns minimum position if there is no issue above' do
expect(issue.prev_relative_position).to eq RelativePositioning::MIN_POSITION
new_issue.move_to_top
expect(new_issue.relative_position).to be < issue.relative_position
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 next position if there is no issue below' do
expect(issue1.next_relative_position).to eq RelativePositioning::MAX_POSITION
end
end
describe '#move_before' do
it 'moves issue before' do
issue1.move_before(issue)
expect(issue1.relative_position).to be < issue.relative_position
end
it 'moves unpositioned issue before' do
issue.update_attribute(:relative_position, nil)
issue1.move_before(issue)
expect(issue1.relative_position).to be < issue.relative_position
end
end
describe '#move_after' do
it 'moves issue after' do
issue.move_before(issue1)
expect(issue.relative_position).to be < issue1.relative_position
end
it 'moves unpositioned issue after' do
issue1.update_attribute(:relative_position, nil)
issue.move_before(issue1)
expect(issue.relative_position).to be < issue1.relative_position
end
end
describe '#move_to_end' do
it 'moves issue to the end' do
@ -95,38 +54,30 @@ describe Issue, 'RelativePositioning' do
expect(new_issue.relative_position).to be < issue1.relative_position
end
it 'positions issue between two other if position of last one is nil' do
it 'positions issue between on top' do
new_issue = create :issue, project: project
issue1.relative_position = nil
issue1.save
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 = create :issue, project: project
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
new_issue = create :issue, project: project
issue1.update relative_position: issue.relative_position
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 two other if position of first one is nil' do
new_issue = create :issue, project: project
issue.relative_position = nil
issue.save
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 'calls move_after if after is nil' do
expect(issue).to receive(:move_after)
issue.move_between(issue1, nil)
end
it 'calls move_before if before is nil' do
expect(issue).to receive(:move_before)
issue.move_between(nil, issue1)
expect(issue.relative_position).to be < issue1.relative_position
end
end
end

View file

@ -94,13 +94,15 @@ describe Boards::Issues::MoveService, services: true do
end
it 'sorts issues' do
[issue1, issue2].each(&:move_to_end)
issue.move_between!(issue1, issue2)
params.merge! move_after_iid: issue.iid, move_before_iid: issue2.iid
described_class.new(project, user, params).execute(issue1)
expect(issue1.relative_position).to be_between(issue.relative_position, issue2.relative_position)
expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
end
end
end