search

brendon / acts_as_list

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

Use bottom_position_in_list instead of the highest value in the table #266

Closed brendon closed 7 years ago

brendon commented 7 years ago

Fixes #264. @zharikovpro could you chime in on this one?

I can't see a good reason to have a unique constraint on the position column because unless you're using it without a scope, there will be duplicates. If you're using it without a scope, will my change in this PR still work for you? Could you test it out for me? The tests still pass, and instead of grabbing the MAXIMUM value via SQL, we're just grabbing the last item in the scoped list and adding 2 to that.

I'm not sure if we're testing for this scenario though a cursory glance at your original PR seems to indicate we are.

@yatish27 can you let me know if this works better for you?

zharikovpro commented 7 years ago

@brendon, sure.

I can't see a good reason to have a unique constraint on the position column

The reason is simple - to have consistent data with validation at the database level.

unless you're using it without a scope, there will be duplicates

It is true that with scope there will be duplicates in one column. However, unique constraint can be composed of 2 columns (say, user and position) to validate that there's no duplicates within that scope.

Could you test it out for me? The tests still pass

If tests are green, then it's just fine. Also, bottom_position_last is perfectly aligned with my initial intent. I only missed that lock issue with maximum, sorry.

P.S. To speed things up, position column could be indexed with or without scope column.

brendon commented 7 years ago

Thanks @zharikovpro, I'll merge this change then :D I appreciate you taking the time to review this.

zharikovpro commented 7 years ago

No problem, this one was easy :)

swanandp commented 7 years ago

This was easy, and simple. 👍