brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.05k stars 356 forks source link

Tests break when upgrading from 0.7.2 to 0.7.4 #228

Closed jmuheim closed 7 years ago

jmuheim commented 8 years ago

I have the following model:

class SuccessCriterion < ActiveRecord::Base
  acts_as_tree order: :position, dependent: :restrict_with_error
  acts_as_list scope: [:parent_id]
end

With the following test:

it 'scopes by parent_id' do
  root = create :success_criterion
  child_1 = create :success_criterion, name: 'child 1'
  child_2 = create :success_criterion, name: 'child 2'

  root.children = [child_1, child_2]

  expect(root.position).to be 1
  expect(child_1.position).to be 1
  expect(child_2.position).to be 2
end

end

With acts_as_list v0.7.2, it passes:

josh@Macbuech:~/Documents/Work/Access4All/projects/a4aa2/src$ rspec spec/models/success_criterion_spec.rb:131
Run options: include {:focus=>true, :locations=>{"./spec/models/success_criterion_spec.rb"=>[131]}}

Randomized with seed 54471

SuccessCriterion
  acts as list
    scopes by parent_id

Top 0 slowest examples (0 seconds, 0.0% of total time):

Finished in 0.19168 seconds (files took 5.46 seconds to load)
1 example, 0 failures

Randomized with seed 54471

With v0.7.4, it doesn't:

josh@Macbuech:~/Documents/Work/Access4All/projects/a4aa2/src$ rspec spec/models/success_criterion_spec.rb:131
Run options: include {:focus=>true, :locations=>{"./spec/models/success_criterion_spec.rb"=>[131]}}

Randomized with seed 41970

SuccessCriterion
  acts as list
    scopes by parent_id (FAILED - 1)

Failures:

  1) SuccessCriterion acts as list scopes by parent_id
     Failure/Error: expect(child_1.position).to be 1

       expected #<Fixnum:3> => 1
            got #<Fixnum:5> => 2

       Compared using equal?, which compares object identity,
       but expected and actual are not the same object. Use
       `expect(actual).to eq(expected)` if you don't care about
       object identity in this example.
     # ./spec/models/success_criterion_spec.rb:131:in `block (3 levels) in <top (required)>'

Top 0 slowest examples (0 seconds, 0.0% of total time):

Finished in 0.20221 seconds (files took 4.54 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/models/success_criterion_spec.rb:123 # SuccessCriterion acts as list scopes by parent_id

Randomized with seed 41970

Any idea what's going on here?

brendon commented 8 years ago

Hi @jmuheim, can you upgrade to the latest 0.8.x version and see if you're experiencing the same failure?

jmuheim commented 8 years ago

Yes I do.

brendon commented 8 years ago

Ok, thanks for that. Are you able to isolate this failure so that you don't rely on acts_as_tree? That gem might be calling another callback that modify's the position again. Take a look at the SQL executed and that might also give us a clue as to what's going on.

brendon commented 7 years ago

@jmuheim, can you let us know where you're at, or close the issue if you've managed to sort it out :)

brendon commented 7 years ago

@jmuheim, I'm closing this for now. Let me know if you want to reopen it. Also, try master and see if that fixes the problem.

jmuheim commented 7 years ago

I'm sorry for not responding. The problem has disappeared with the newest version.

Thank you.

brendon commented 7 years ago

Great to hear! :)

calleo commented 5 years ago

Just in case anyone else ends up here.

I was upgrading from 0.6 and tests started to fail when going from 0.7.2 to 0.7.4. All list items had position "1".

We are using Factory Girl 4.5 and Rails 4.2. This is how the code looks that was creating the list items list.items << create(:item)

Once I changed it to create(:item, list: list) things started to work again.

Note that we are using acts_as_list with a scope.

Looks like the position is first calculated when the list item is created (with scope "list_id = nil") which gives position "1". Appending the item to a list (using "<<") does not re-compute the position. So, supplying a list when the item is created fixes the problem.

Could be an issue with FactoryGirl that has been corrected long time ago.

brendon commented 5 years ago

That's a strange one. Changing the list and re-saving the item should change the position to the next available position in the list. But you're right, supplying the list on create will give you correct positions :)

brendon commented 5 years ago

@calleo, looking at this further, I'm wondering if << runs any callbacks on the item being appended to the list? It needs to run the callbacks to pick up the change in scope, otherwise its position will remain as it is.

If you wanted to look into this further, feel free :) I'm happy to provide some guidance if necessary :)