Commit 54090a57 authored by Alex Kalderimis's avatar Alex Kalderimis

Fix relative siblings for epic tree nodes

Epic tree notes are polymorphic objects, and simply filtering on `id` is
not suitable for defining the set of siblings. This addresses this and
includes a reproducible test case.
parent aadcc4ac
...@@ -19,13 +19,16 @@ module EpicTreeSorting ...@@ -19,13 +19,16 @@ module EpicTreeSorting
end end
included do included do
extend ::Gitlab::Utils::Override
override :move_sequence
def move_sequence(start_pos, end_pos, delta, include_self = false) def move_sequence(start_pos, end_pos, delta, include_self = false)
items_to_update = scoped_items items_to_update = scoped_items
.select(:id, :object_type) .select(:id, :object_type)
.where('relative_position BETWEEN ? AND ?', start_pos, end_pos) .where('relative_position BETWEEN ? AND ?', start_pos, end_pos)
unless include_self unless include_self
items_to_update = items_to_update.where.not('object_type = ? AND id = ?', self.class.table_name.singularize, self.id) items_to_update = relative_siblings(items_to_update)
end end
items_to_update.group_by { |item| item.object_type }.each do |type, group_items| items_to_update.group_by { |item| item.object_type }.each do |type, group_items|
...@@ -34,5 +37,12 @@ module EpicTreeSorting ...@@ -34,5 +37,12 @@ module EpicTreeSorting
items.update_all("relative_position = relative_position + #{delta}") items.update_all("relative_position = relative_position + #{delta}")
end end
end end
private
override :relative_siblings
def relative_siblings(relation = scoped_items)
relation.where.not('object_type = ? AND id = ?', self.class.table_name.singularize, self.id)
end
end end
end end
---
title: Correct sibling query for epic tree nodes
merge_request: 41986
author:
type: fixed
...@@ -12,6 +12,49 @@ RSpec.describe EpicTreeSorting do ...@@ -12,6 +12,49 @@ RSpec.describe EpicTreeSorting do
let!(:epic2) { create(:epic, parent: base_epic, group: group, relative_position: 1000) } let!(:epic2) { create(:epic, parent: base_epic, group: group, relative_position: 1000) }
let!(:epic3) { create(:epic, parent: base_epic, group: group, relative_position: 1001) } let!(:epic3) { create(:epic, parent: base_epic, group: group, relative_position: 1001) }
describe '#relative_siblings' do
def siblings(obj)
obj.send(:relative_siblings).pluck(:id, :object_type)
end
def polymorphic_ident(obj)
case obj
when Epic
[obj.id, 'epic']
when EpicIssue
[obj.id, 'epic_issue']
end
end
it 'includes both epics and epic issues for an epic issue' do
idents = [epic_issue2, epic_issue3, epic1, epic2, epic3].map { |obj| polymorphic_ident(obj) }
expect(siblings(epic_issue1)).to match_array(idents)
end
it 'includes both epics and epic issues for an epic' do
idents = [epic_issue1, epic_issue2, epic_issue3, epic2, epic3].map { |obj| polymorphic_ident(obj) }
expect(siblings(epic1)).to match_array(idents)
end
context 'there is an ID collision' do
let(:max_epic_issue_id) { EpicIssue.maximum(:id) }
let(:max_epic_id) { Epic.maximum(:id) }
let(:collision_id) { [max_epic_id, max_epic_issue_id].max.succ }
it 'includes the collision from either collision member' do
colliding_epic = create(:epic, id: collision_id, parent: base_epic, group: group)
colliding_epic_issue = create(:epic_issue, id: collision_id, epic: base_epic)
expect(siblings(colliding_epic)).to include(polymorphic_ident(colliding_epic_issue))
expect(siblings(colliding_epic_issue)).to include(polymorphic_ident(colliding_epic))
end
end
end
describe '#move_after' do describe '#move_after' do
it 'moves an epic' do it 'moves an epic' do
epic1.move_after(epic_issue2) epic1.move_after(epic_issue2)
......
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