Skip to content

Commit

Permalink
Merge pull request #3892 from kleintom/3890_remove_acts_as_list
Browse files Browse the repository at this point in the history 
Remove acts as list from Lead
  • Loading branch information
@mjy
mjy authored Mar 23, 2024
2 parents 72d4cb6 + a5cfa29 commit ebf478b
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 53 deletions.
40 changes: 18 additions & 22 deletions app/models/lead.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,6 @@ class Lead < ApplicationRecord

has_closure_tree order: 'position', numeric_order: true, dont_order_roots: true , dependent: nil

# New records added at bottom (default)
# !! Note there is some oddness with update(parent: obj) vs. update(parent_id: object.id),
# if your function is not working as expected try the opposite.
acts_as_list scope: [:parent_id, :project_id]

belongs_to :parent, class_name: 'Lead'

belongs_to :otu, inverse_of: :leads
Expand Down Expand Up @@ -110,15 +105,14 @@ def self.draw(lead, indent = 0)
end

def insert_couplet
a,b,c,d = nil, nil, nil, nil
c,d = nil, nil

p = node_position

t1 = 'Inserted node'
t2 = 'Child nodes are attached to this node'

# !! This reload is key to have specs pass
if children.reload.any?
if children.any?
o = children.to_a

left_text = (p == :left || p == :root) ? t2 : t1
Expand All @@ -128,13 +122,18 @@ def insert_couplet
c = Lead.create!(text: left_text, parent: self)
d = Lead.create!(text: right_text, parent: self)

o.each do |i|
if p == :left || p == :root
i.update!(parent: c)
new_parent = (p == :left || p == :root) ? c : d
last_sibling = nil

# !! The more obvious version using add_child is actually more error
# prone than using add_sibling.
o.each_with_index do |c, i|
if i == 0
new_parent.add_child c
else
i.update!(parent: d)
last_sibling.append_sibling c
end

last_sibling = c
end
else
c = Lead.create!(text: t1, parent: self)
Expand Down Expand Up @@ -166,20 +165,17 @@ def destroy_couplet

i = has_kids.children

# Reparent the children of has_kids to self.
# !! The obvious solution using add_child is actually more error-prone
# than using add_sibling.
last_sibling = children[-1]
i.each do |z|
# reload is required for pass, it seems we need to know the sibiling state
z.reload.update!(parent_id: has_kids.parent_id)
last_sibling.append_sibling z
last_sibling = z
end

# `position` looks OK here
has_kids.destroy!
# `position` looks OK here
no_kids.destroy!
# !! `position` NOT OK here

# Give up as to why destroy fails to re-index. Force re-index of `position`
children.reload.reverse.map(&:save) # reload is required

else # Neither side has children
Lead.transaction do
a.destroy!
Expand Down
98 changes: 67 additions & 31 deletions spec/models/lead_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,9 @@
end

specify '#insert_couplet children are ordered' do
lead.insert_couplet
a = lead.children.order(:position)
expect(a.first.position).to eq(0)
expect(a.last.position).to eq(1)
ids = lead.insert_couplet
expect(Lead.find(ids[0]).position).to eq(0)
expect(Lead.find(ids[1]).position).to eq(1)
end

specify '#insert_couplet inserts between leads' do
Expand All @@ -34,7 +33,7 @@

specify '#insert_couplet between leads inserts with positions' do
FactoryBot.create(:valid_lead, parent: lead, text: 'bottom')
lead.insert_couplet
lead.reload.insert_couplet
a = lead.children.reload.order(:position)

expect(a.first.children.size).to eq(1)
Expand All @@ -44,20 +43,19 @@
FactoryBot.create(:valid_lead, parent: lead, text: 'bottom left')
FactoryBot.create(:valid_lead, parent: lead, text: 'bottom right')

lead.insert_couplet
lead.reload.insert_couplet

a = lead.reload.children.order(:position)
expect(a.first.children.size).to eq(2)
end

specify '#insert_couplet between leads with siblings maintains position' do
FactoryBot.create(:valid_lead, parent: lead, text: 'bottom left')
FactoryBot.create(:valid_lead, parent: lead, text: 'bottom right')
bl = FactoryBot.create(:valid_lead, parent: lead, text: 'bottom left')
br = FactoryBot.create(:valid_lead, parent: lead, text: 'bottom right')

lead.insert_couplet
lead.reload.insert_couplet

a = lead.reload.children.order(:position)
expect(a.first.children.order(:position).pluck(:position)).to eq([0,1])
expect(bl.reload.position).to be < br.reload.position
end

specify '#node_position of root' do
Expand Down Expand Up @@ -124,6 +122,21 @@
expect {child.update! redirect_id: root.id}.to raise_error ActiveRecord::RecordInvalid
end

# TODO: should this be a request test instead, so that we're testing
# whatever current leads_controller#update behavior is?
specify "'ui update' doesn't change order of chidren" do
# Simulate a ui 'Update' saving all three nodes of a couplet.
lead = FactoryBot.create(:valid_lead)
l = FactoryBot.create(:valid_lead, parent: lead, text: 'bottom left')
r = FactoryBot.create(:valid_lead, parent: lead, text: 'bottom right')

lead.update! text: lead.text
l.update! text: 'new text'
r.update! text: r.text

expect(l.reload.position).to be < r.reload.position
end

xspecify "keys with external referrers can't be destroyed" do
root1 = FactoryBot.create(:valid_lead)
root2 = FactoryBot.create(:valid_lead)
Expand Down Expand Up @@ -205,7 +218,6 @@
specify 'inserted lead positions are correct after #insert_couplet (left)' do
ids = l.insert_couplet
a = Lead.find(ids[0])
# a.run_callbacks(:commit)
expect(Lead.find(ids[0]).reload.position).to be < Lead.find(ids[1]).reload.position # Children should be left/right OK
end

Expand All @@ -214,33 +226,42 @@
expect(Lead.find_by(text: 'll').position).to be < Lead.find_by(text: 'lr').position # Grand children maintain position
end

specify 'positions are correct after insert_couplet (right)' do
# Test the same when inserting on a right node.
specify 'current positions are correct after #insert_couplet (right)' do
ids = lr.insert_couplet
expect(Lead.find_by(text: 'll').position).to be < Lead.find_by(text: 'lr').position # Position shouldn't change L or R
end

expect(Lead.find_by(text: 'll').position).to be < Lead.find_by(text: 'lr').position
expect(Lead.find(ids[0]).position).to be < Lead.find(ids[1]).position
expect(Lead.find_by(text: 'lrl').position).to be < Lead.find_by(text: 'lrr').position
specify 'inserted lead positions are correct after #insert_couplet (right)' do
ids = lr.insert_couplet
a = Lead.find(ids[0])
expect(Lead.find(ids[0]).reload.position).to be < Lead.find(ids[1]).reload.position # Children should be left/right OK
end

specify 're-attached child lead positions are correct after #insert_couplet (right)' do
ids = lr.insert_couplet
expect(Lead.find_by(text: 'lrl').position).to be < Lead.find_by(text: 'lrr').position # Grand children maintain position
end

specify 'insert_couplet reparents on the same side as self' do
specify 'insert_couplet reparents on the right if self was a right child' do
# lr is a right child, so reparented children should be on right.
ids = lr.insert_couplet

expect(Lead.find(ids[0]).children.size).to eq(0)
expect(Lead.find(ids[1]).all_children.size).to eq(2)

expect(Lead.find(ids[0]).text). to eq('Inserted node')
expect(Lead.find(ids[0]).text).to eq('Inserted node')
expect(Lead.find(ids[1]).text).to eq('Child nodes are attached to this node')
end

specify 'insert_couplet reparents on the right if self was a right child' do
# l is a left child, so reparented children should be on left.
ids = l.insert_couplet

expect(Lead.find(ids[0]).all_children.size).to eq(6)
expect(Lead.find(ids[0]).all_children.size).to eq(4)
expect(Lead.find(ids[1]).children.size).to eq(0)

expect(Lead.find(ids[0]).text).to eq('Child nodes are attached to this node')
expect(Lead.find(ids[1]).text). to eq('Inserted node')
expect(Lead.find(ids[1]).text).to eq('Inserted node')
end

specify 'insert_couplet creates expected parent/child relationships' do
Expand Down Expand Up @@ -308,31 +329,46 @@
expect(Lead.where('parent_id is null').first.all_children.size).to be(6)
end

specify "destroy_couplet doesn't change order of remaining nodes (left)" do
specify "destroy_couplet doesn't change order of parent node pair (left)" do
l.destroy_couplet
expect(Lead.find_by(text: 'l').position).to be < Lead.find_by(text: 'r').position


expect(Lead.find_by(text: 'lrl').position).to be < Lead.find_by(text: 'lrr').position
end

specify "destroy_couplet doesn't change order of remaining nodes (right)" do
# Test the same for destroy_couplet on a right node.
specify "destroy_couplet doesn't change order of reparented nodes (left)" do
l.destroy_couplet
expect(Lead.find_by(text: 'lrl').position).to be < Lead.find_by(text: 'lrr').position
end

r.reload
specify "destroy_couplet doesn't change order of parent node pair (right)" do
rl = r.children.create! text: 'rl'
rr = r.children.create! text: 'rr'
rll = rl.children.create! text: 'rll'
rlr = rl.children.create! text: 'rlr'

o = Lead.find_by(text: 'r')

o.destroy_couplet
r.reload.destroy_couplet
expect(Lead.find_by(text: 'l').position).to be < Lead.find_by(text: 'r').position
end

specify "destroy_couplet doesn't change order of reparented nodes (right)" do
rl = r.children.create! text: 'rl'
rr = r.children.create! text: 'rr'
rll = rl.children.create! text: 'rll'
rlr = rl.children.create! text: 'rlr'

r.reload.destroy_couplet
expect(Lead.find_by(text: 'rll').position).to be < Lead.find_by(text: 'rlr').position
end

specify "destroy_couplet doesn't change order of 3 reparented nodes (left)" do
lrm = FactoryBot.create(:valid_lead, text: 'middle child of lr')
lrl.append_sibling(lrm)

l.reload.destroy_couplet

expect(Lead.find_by(text: 'lrl').position).to be < Lead.find_by(text: 'middle child of lr').position
expect(Lead.find_by(text: 'middle child of lr').position).to be < Lead.find_by(text: 'lrr').position
end

specify '#all_children' do
lrll = lrl.children.create! text: 'lrll'
lrlr = lrl.children.create! text: 'lrlr'
Expand Down

0 comments on commit ebf478b

Please sign in to comment.