Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make insert_at respect position when creating a new record #287

Closed
goalaleo opened this issue Oct 16, 2017 · 7 comments
Closed

Make insert_at respect position when creating a new record #287

goalaleo opened this issue Oct 16, 2017 · 7 comments

Comments

@goalaleo
Copy link

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
Copy link
Owner

brendon commented Oct 16, 2017

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.

@brendon brendon closed this as completed Oct 16, 2017
@goalaleo
Copy link
Author

@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
Copy link
Owner

brendon commented Oct 17, 2017

@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
Copy link

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

@brendon
Copy link
Owner

brendon commented Dec 13, 2018

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

@DanielHeath
Copy link

Makes sense to me, will do.

@DanielHeath
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants