Fix bug, position is recomputed when object saved

[chrisortman]

In models that get saved multiple times (due to callbacks in the case I ran into) the position is updated multiple times meaning that you start with a position of 2 instead of 1

I wasn't sure how best to integrate this with the existing tests. Happy to refactor that if you'd like that done differently

[houndci-bot]

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

[swanandp]

[OT] Gah, hound needs to be configured properly. The defaults aren't good enough.

[chrisortman]

I checked add_to_list_top, it is implemented differently from add_to_list_bottom so doesn't exhibit this behavior.

I agree that callbacks tend to be great breeding grounds for dragons, however refactoring that isn't a good option right now.

My first thought was that not_in_list? would have prevented this as well, but in this case, position is not changed and scope is changed so you get

false || true && true || false = true

So I guess to me this didn't feel toooo work around'ish because you really don't want to increment position if you are already at the bottom. I guess the other thing that could happen is that add_to_list_bottom should return if bottom_item == self or in the list at all because if it is in the list then move should have been called?

[chrisortman]

The CI checks that failed were

An error occurred while installing github_changelog_generator (1.11.0), and

Could I have broken that?

[chrisortman]

I'm not sure now to make this less contrived :(

I need a belongs_to and update_attributes so that I get scope_changed to be true.

[brendon]

That sure is a strange bundler error! I couldn't find anything with a quick search. That change log generator was added only recently. Perhaps there's a problem with the gem they published?

Could you explain why you need a double save in your particular application? That might help us come up with a more legitimate test case :)

[chrisortman]

I doubt that the double save is needed . It's an old rails app, and once upon a time people thought callbacks were where important logic went?

[chrisortman]

Open bug on github changeling generator
github-changelog-generator/github-changelog-generator#329

[brendon]

Lol, that GIF on the issue page!

I see what you're saying about the rails app. I can foresee perhaps someone saving an instance twice (outside of a callback) say in a controller. Would doing that cause the bug too?

[chrisortman]

No, it is specific to being saved via a callback, I can comment out the after_create call and call change_title right after the first save! then call save again and you don't recreate it.

[chrisortman]

Here's a couple of other things I noticed....

Perhaps in unless bottom_item == self I should do unless bottom_item.equals?(self)

The codebase I'm working on has overridden <=> such that ARSubclass.create({val: 'x'}) == ARSubclass.create({val: 'x'}) returns true :(

I think == feels more generally correct to me though

Also, in default_position?

If the column default is NULL this check gets broken.

default_position && default_position.to_i == send(position_column)
nil && 0 == nil
nil && false
nil

So then checks for !default_position? are flipped around because it really is the default value but falsey nil is returned

A simple fix is

if default_position.nil?
  send(position_column).nil?
else
  default_position && default_position.to_i == send(position_column)
end

There's probably a more elegant way?

[chrisortman]

How do you control which version of github_changelog_generator will get pulled in travis? I locked the version to the non-broken one in this PR but that doesn't seem to do it

[fabn]

How do you control which version of github_changelog_generator will get pulled in travis? I locked the version to the non-broken one in this PR but that doesn't seem to do it

I submitted #190 to solve that issue. Since Gemfiles are managed with appraisal they need to be regenerated as well.

As soon as the travis build finish with a green status I'll merge #190 and you then can rebase against master.

[fabn]

Ok, now we have a working version in master. Please rebase against it.

[brendon]

Hi @chrisortman :) I've had a bit of a think on this one. I see your point on default_position?. That could be handled in another pull request later. I think it's only interested to know if the position columns default is set to an integer (0 or 1 as far as I'm aware). It's a strange scenario anyway. I tried to find reasons for this part of the code existing but didn't get far despite the mention of it at the bottom of the README.

As far as the double-save thing. You mentioned the scope is changing. Thinking more about that, it's more likely that the scope change (aka, certain tracked columns being dirty) isn't reset before the callback is called, because really, on the second save, the scope hasn't changed.

How about a local variable to track wether the scope change has already been detected, similar to the @position_changed variable. Then we can clear that out or something once the list change or addition process has completed, but before callbacks are called.

The only problem is that users may have created their own scope_changed method (I have on occasion to handle a more advanced change condition), so you'd need an intermediary private method to check the public method and set the local variable appropriately.

I've not tested anything so this could all be rubbish! :D

[chrisortman]

My thinking for why I fixed it the way I did using the bottom_item check vs a new instance var is that adding the instance var increases the local state and then we have to do book keeping on it and then the book keeping could make a mistake.

But, I can't think of a scenario where if the item is already at the bottom of the list it should still have it's position changed. So the bottom_item check feels like it will always just be correct as a fact.

I have much less experience than you with this code base and won't probably have to live with the decision for as long so if you feel it should be done a different way I'll trust you on it and make the changes.

I will do a rebase here and pull the default position changes into a separate PR

[chrisortman]

Having said all that...I can't make it work using .equal? so I'd have to use == and I know at least in the app I'm in it won't work because someone overrode <=> in an active record model to not use the primary key. That seems like a really bad idea anyway so maybe I can get that changed? I don't know how common of a practice that would be, but if it is common I think the bottom_item check would break people as it is.

I could though implement a comparison like probably_same?

def probably_same?(x,y) 
  if x.is_a?(ActiveRecord::Base) && y.is_a?(x.class)
     !x.id.nil? && x.id == y.id
 else
     x == y
 end
end

Which would work around someone overriding spaceship.

[chrisortman]

Ok, I think I've come up with a better test case, and have fit it into the existing test suite better. The nice thing is that I think it might have exposed some other bugs related to having a model with these types of callbacks.

One thing it told me was that if I do the bottom_item check then the default_position? changes need to be part of the fix because once I add the bottom_item check other tests fail without changes to default_position?

Without default position changes

  1) Failure:
ListWithCallbackTest#test_before_create_callback_adds_to_given_position [/Users/cortman/code/acts_as_list/test/shared_list.rb:247]:
Expected: [5, 1, 6, 2, 3, 4]
  Actual: [5, 6, 1, 2, 3, 4]


  2) Failure:
ListTest#test_before_create_callback_adds_to_given_position [/Users/cortman/code/acts_as_list/test/shared_list.rb:247]:
Expected: [5, 1, 6, 2, 3, 4]
  Actual: [5, 6, 1, 2, 3, 4]


  3) Failure:
ListSubTestWithDefault#test_insert_at [/Users/cortman/code/acts_as_list/test/shared_list_sub.rb:73]:
Expected: 2
  Actual: 3


  4) Failure:
ListSubTest#test_insert_at [/Users/cortman/code/acts_as_list/test/shared_list_sub.rb:73]:
Expected: 2
  Actual: 3

120 runs, 696 assertions, 4 failures, 0 errors, 0 skips
rake aborted!
Command failed with status 

With

Finished in 2.979584s, 40.2741 runs/s, 239.6308 assertions/s.

  1) Failure:
ListWithCallbackTest#test_before_create_callback_adds_to_given_position [/Users/cortman/code/acts_as_list/test/shared_list.rb:247]:
Expected: [5, 1, 6, 2, 3, 4]
  Actual: [5, 6, 1, 2, 3, 4]


  2) Failure:
ListTest#test_before_create_callback_adds_to_given_position [/Users/cortman/code/acts_as_list/test/shared_list.rb:247]:
Expected: [5, 1, 6, 2, 3, 4]
  Actual: [5, 6, 1, 2, 3, 4]

120 runs, 714 assertions, 2 failures, 0 errors, 0 skips
rake aborted!
Command failed with status (1): [ruby -I"lib:lib:test" -I"/Users/cortman/.gem/ruby/2.2.4/gems/rake-1

So for now I've left them in.

I will look into why these other tests are failing

[chrisortman]

I've made some progress, but am not sure what to do next.

Once I started doing the equality checks, making sure to include STI in the models in the tests became important, but adding that is breaking other tests because the tests don't get the correct list when they are a different subtype.

Example, create ZeroBasedMixin models, but then the test creates a ListMixin model on the same parent.

Right now we construct the query using self.class.name and ListMixin.all wouldn't find the ZeroBasedMixin models so it doesn't know what position should be

What is the desired behavior when models use STI? Should the whole type hierarchy be ordered within the parent or on each individual type? whole type hierarchy seems like what you'd want, but I could see reasons for both...

[chrisortman]

Why in test/shared_zero_based.rb does test_reordering mix usages of ZeroBasedMixin and ListMixin ? Is this exercising some functionality? Or just copied & pasted??

[brendon]

@chrisortman, check out my branch: https://github.com/swanandp/acts_as_list/tree/fix-scope-change-detection and see if you still experience the bug with that.

The solution feels a bit hacky though. I can see an edge case already where you want to (for some unknown reason) change the scope in the after_save callback. None of that will be picked up. It should work fine if you're not messing with the scope of position in the after_save.

[brendon]

In regards to ZeroBasedMixin and ListMixin being mixed up. I'm not sure. @swanandp developed those tests I think.

[brendon]

An issues the refers to this problem also: #166

[chrisortman]

I cherry pick'd 309ab43e59c999ed3286466f0500d296635cf69b into your branch and ran the tests and get several errors.

https://github.com/chrisortman/acts_as_list/commits/fix-scope-change-detection

It looks like we are still getting double entry into the insert_at_bottom

  1) Failure:
ListWithCallbackTest#test_before_create_callback_adds_to_bottom [/Users/cortman/code/acts_as_list/test/shared_list.rb:221]:
Expected: 5
  Actual: 6


  2) Failure:
ListWithCallbackTest#test_before_create_callback_adds_to_given_position [/Users/cortman/code/acts_as_list/test/shared_list.rb:247]:
Expected: [5, 1, 6, 2, 3, 4]
  Actual: [5, 6, 1, 2, 3, 4]


  3) Failure:
ListWithCallbackTest#test_update_position_when_scope_changes [/Users/cortman/code/acts_as_list/test/shared_list.rb:149]:
Expected: 2
  Actual: 3


  4) Failure:
ListWithCallbackTest#test_insert [/Users/cortman/code/acts_as_list/test/shared_list.rb:54]:
Expected: 1
  Actual: 2


  5) Failure:
ListWithCallbackTest#test_insert_at [/Users/cortman/code/acts_as_list/test/shared_list.rb:76]:
Expected: 1
  Actual: 2


  6) Failure:
ListTestWithDefault#test_insert_at [/Users/cortman/code/acts_as_list/test/shared_list.rb:76]:
Expected: 1
  Actual: 2


  7) Failure:
ListTestWithDefault#test_before_create_callback_adds_to_given_position [/Users/cortman/code/acts_as_list/test/shared_list.rb:247]:
Expected: [5, 1, 6, 2, 3, 4]
  Actual: [5, 6, 1, 2, 3, 4]


  8) Failure:
ListTestWithDefault#test_insert [/Users/cortman/code/acts_as_list/test/shared_list.rb:54]:
Expected: 1
  Actual: 2


  9) Failure:
ListTestWithDefault#test_update_position_when_scope_changes [/Users/cortman/code/acts_as_list/test/shared_list.rb:149]:
Expected: 2
  Actual: 3


 10) Failure:
ListTestWithDefault#test_before_create_callback_adds_to_bottom [/Users/cortman/code/acts_as_list/test/shared_list.rb:221]:
Expected: 5
  Actual: 6


 11) Failure:
ListSubTest#test_insert_at [/Users/cortman/code/acts_as_list/test/shared_list_sub.rb:70]:
Expected: 1
  Actual: 2


 12) Failure:
ListSubTestWithDefault#test_insert_at [/Users/cortman/code/acts_as_list/test/shared_list_sub.rb:70]:
Expected: 1
  Actual: 2

120 runs, 636 assertions, 12 failures, 0 errors, 0 skips

[brendon]

Bummer. Given I didn't actually test the code against the problem. Would you be able to probe that variable at the various stages in the save cycle and see if it's behaving itself? It should be true before save, then after save be set to false. Then after the initial save is committed, the variable should be unset.

It would be super helpful is dirty tracking actually worked properly in after_saves.

[chrisortman]

It looks like the after_save callback does not get called until after the 2nd time through.

If I add @scope_changed = false to the end of add_to_list_bottom then the tests will pass

[brendon]

Interesting. Probably due to the ordering of the after_save callbacks. My after_save would need to be executed before yours.

Is your callback declaration before the acts_as_list declaration in your code? I suppose it must be in the tests for it to fail like that.

[chrisortman]

Yeah, it's in my fork of your branch
On Mon, Feb 29, 2016 at 4:11 PM Brendon Muir notifications@github.com
wrote:

Interesting. Probably due to the ordering of the after_save callbacks. My
after_save would need to be executed before yours.

Is your callback declaration before the acts_as_list declaration in your
code? I suppose it must be in the tests for it to fail like that.

—
Reply to this email directly or view it on GitHub
#188 (comment)
.

[chrisortman]

What do we think with this? We have to possible ways to fix this, are we happier with one vs the other?

[brendon]

If we can control the order of callbacks (or perhaps is there an underlying Rails hooks scheme in ActiveRecord that we can hook into?) then solving the real bug, which is thinking the scope has changed after the model has been saved, would be preferable to me.

The other way just seems to be patching a symptom of that problem.

[chrisortman]

Alright, I'm fine with fixing it by fixing the scope change detection. If you look at chrisortman@21a3bbf this is what I had to do to finish what you had already started.

I'll update this PR with these changes

[brendon]

Should we now remove this line? I agree with your approach of setting @scope_changed to false straight after adding the item to the list.

[brendon]

Thanks for your patience Chris :) Nearly there, just had a few suggestions to the code. Your solution is definitely better than the after_ callback :)

[chrisortman]

No problem. Good stuff sometimes takes time :)

I think latest commit addressed all your notes. Let me know if I missed anything else.

[brendon]

Thanks @chrisortman :) I'm happy with this. I'll merge it. I just want to get @swanandp to sign off on this too since he's the boss ;) Can you give this a look soon @swanandp?

[brendon]

@swanandp?

[swanandp]

Sorry @brendon and @chrisortman , I dropped the ball on this. Taking a look now.

[swanandp]

Looks good 👍

[brendon]

Merged! 💃