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

Setting `position: nil` on update returns `Column 'position' cannot be null` instead of putting the item at the start or the end of the list, like it does on create. #302

Closed joshuapinter closed 6 years ago

joshuapinter commented 6 years ago

When creating list items, if you set position to nil it either places the new item at the start or end of the list, depending on what you have set for add_new_at.

However, when updating an existing list item, if you use the same technique of setting position to nil, it does not move it to the start or end of the list. Instead, you get a database error because position cannot be NULL:

Column 'position' cannot be null

Is this by design, am I missing something, or is this something you would want a PR for?

Thanks!

brendon commented 6 years ago

Hi @joshuapinter, the NULL validation error is coming from your decision to enforce that at the database level. If you do want the position to be nullable then you'll need to write a migration to remove that requirement in your database.

If instead you want the item to assume the top (or bottom) of the list, you can use the move_to_top and move_to_bottom methods on the existing record.

I hope that helps. Let me know if I've missed your point :)

joshuapinter commented 6 years ago

Hi @brendon. Thanks for the quick reply!

You're definitely correct. I'm curious why this works fine when creating but doesn't work when updating?

For example, if I pass a blank value to position in a create action, it puts it at the end of the list. But when doing the same for an existing record (i.e. edit a record, set a blank value for the position) it complains about a NULL value for the position column.

Are create and update handled differently in acts_as_list?

Thanks!

brendon commented 6 years ago

You're welcome @joshuapinter :)

Create and update are definitely handled differently and here's where: https://github.com/swanandp/acts_as_list/blob/c999eff863e1426aa16f680fc9fbe52842dd788a/lib/acts_as_list/active_record/acts/callback_definer.rb#L15

Before create we will add to the list top or bottom provided the configuration option exists :) On update we don't do anything like that as it doesn't make sense. If you define a position on create we also don't do anything as we respect your wishes for the position :)

https://github.com/swanandp/acts_as_list/blob/c999eff863e1426aa16f680fc9fbe52842dd788a/lib/acts_as_list/active_record/acts/list.rb#L251

Hope that helps :D

hhunaid commented 6 years ago

@brendon It seems to me that updating positions in db would be a common scenario. Consider this: My app allows reordering pictures and on update I would want to preserve the new order. Right now I have something like

    def ensure_pictures_order
      self.pictures.each_with_index do |pic, index|
        pic.set_list_position(index) unless pic.position == index
      end
    end

in my before_update callback.

brendon commented 6 years ago

Hi @hhunaid, It's common to do that but not in the model. Often it's done when a list of id's is supplied to the controller via params that determines the order of items in the scope.

If you have a list of things already well ordered, then want to reorder them, you can either pass through a list of id's to a collection route method and run something similar to what you have above, or you can take one particular member of the list and set a new position for it. Upon saving, all the other list members will get out of its way so that only it has that position.

I've always found it a bit tricky to translate a drag and drop reordering into an ordering of items in the database but those are the two options. The first has the desirable effect of correcting any inconsistencies with the position integers that can sneak in where multiple people manipulate the list at the same time.

Hope that helps. Take a look at the acts_as_list codebase. It's not too complicated to understand. And if you can find any places for improvement we love PR's! :D And I promise I'll look at them for you quickly :)