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

Make insert_at respect position when creating a new record #287

Closed goalaleo closed 6 years ago

goalaleo commented 6 years ago

Request

Currently the position of a new record uses the add_new_at value. Is it possible to make new records respect the parameter given to insert_at.

Example:

class Person < ActiveRecord::Base
    has_many :dogs
end

class Dog < ActiveRecord::Base
    belongs_to :person
    acts_as_list scope: :person
end

# irb
person = Person.new
person.dogs << Dog.new(name: "First Dog")
new_dog = Dog.new(person: person, name: "Second Dog")

# imo this should set new_dog's position to 1, and update existing ones as needed
new_dog.insert_at(1)

new_dog.position

# expected
=> 1
# got
=> 2
brendon commented 6 years ago

Hi @goalaleo, you can actually just pass the position you want into the .new method:

new_dog = Dog.new(person: person, name: "Second Dog", position: 1)

Acts As List will shuffle things around so that your new object has that position and the others don't. insert_at is for existing records either in this scope or any other scope.

I hope that helps. I'll close this for now but let me know if you think I've missed the point.

goalaleo commented 6 years ago

@brendon ok, thanks 👍 ! I still feel like the naming of insert_at is a bit misleading, as it does work on new instances, does a literal insert DB operation, but doesn't actually insert the record to the given position :/ Either way, great gem!

brendon commented 6 years ago

@goalaleo, yes you're right. It should probably be named reposition_at or something like that though that would break the api and I think this method has been around since the beginning. @swanandp, did you have any thoughts on this one?

DanielHeath commented 5 years ago

I would expect an ArgumentError or something for new records rather than silently failing.

brendon commented 5 years ago

Thanks @DanielHeath :) Would you be keen to put together a small PR for that? I'd be happy to merge that for you.

DanielHeath commented 5 years ago

Makes sense to me, will do.

DanielHeath commented 5 years ago

Upon closer inspection, there's clearly code intended to insert new records at the right position.

I've fixed it so this case now works.