search

brendon / ranked-model

An acts_as_sortable/acts_as_list replacement built for Rails 4+
https://github.com/mixonic/ranked-model
MIT License
1.09k stars 134 forks source link

Fix error when changing with_same and rebalancing #160

Closed frysch closed 5 years ago

frysch commented 5 years ago

When updating the with_same attribute and at the same time setting a new column_position which caused a rebalancing there was a NoMethodError thrown on line 215.

Here's a possible fix.

brendon commented 5 years ago

Thanks @frysch. Just checking, is it possible for the moved item to be in the current_order list even when its with_same attribute is changing? I guess that depends on the caching?

frysch commented 5 years ago

Thanks @frysch. Just checking, is it possible for the moved item to be in the current_order list even when its with_same attribute is changing? I guess that depends on the caching?

It seems to me that it's not possible, but i might miss something. Dunno if it causes any problems if it should happen thou, since it's working when not updating that with_same attribute at the same time. 🤷‍♂️

brendon commented 5 years ago

Thanks @frysch, you're right because the finder is cached but based on the new with_same values. I was thinking it'd be cleaner to detect if the with_same values had changed but that could get complicated.

frysch commented 5 years ago

I was thinking it'd be cleaner to detect if the with_same values had changed but that could get complicated.

That was actually my first approach as well. Turned out pretty messy. 😀

brendon commented 5 years ago

Haha all good. With regards to your other PR, I'll need more time to look at that one.

frysch commented 5 years ago

Haha all good. With regards to your other PR, I'll need more time to look at that one.

Take your time. And as i wrote there, do whatever you like with it. Dunno if it's too much of an edge case, or if it's something that ever would be used. Just needed to patch it a bit for our needs.

janklimo commented 4 years ago

Thank you for this @frysch 👍 I was about to start working on a PR when I saw yours got merged already 😄