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

Reorder positions #233

Closed MarcReniu closed 7 years ago

MarcReniu commented 8 years ago

Imagine you have the next elements with missing higher positions:

If I want to move position of ElementA to top of list, I can use move_to_top method, but in case I execute next move_higher over ElementB, it becomes position 4, and ElementA, position 5.

The final goal is to reorder all elements, considering top_of_list config. So, there is some method to perform it, or I have to do it manually?

brendon commented 7 years ago

Hi @MarcReniu, there isn't currently a method to clean up the list orderings. move_higher simply swaps the position values of the two concerned records as far as I'm aware.

We're always keen to improve the library. If you have any ideas (and PR's) that can help, we'll do our best to see them merged. One feature I'd like is an auto-repair process that fixes up duplicate position values in a list.

MarcReniu commented 7 years ago

I finally made a module to check and perform the reorder. It is ugly as f***, but it only makes the minimum possible queries, and no executes validations or callbacks.

You could use it as an starting point, or throw it to the garbage, as you wish.

module Ordenable
  extend ActiveSupport::Concern

  def valid_ordering? relation
    records = self.send(relation)
    records_class = records.class_name.constantize

    top_of_list_config = records_class.acts_as_list_top
    position_column = records_class.quoted_position_column.delete("`")
    current_order = records.collect(&:"#{position_column}")
    correct_order = (top_of_list_config...(records.size + top_of_list_config)).to_a

    return current_order == correct_order
  end

  def correct_ordering relation
    return if self.valid_ordering?(relation)

    records = self.send(relation)
    records_class = records.class_name.constantize

    top_of_list_config = records_class.acts_as_list_top
    position_column = records_class.quoted_position_column.delete("`")
    new_positions = (top_of_list_config...(records.size + top_of_list_config)).to_a

    records.each_with_index do |record, index|
      record.update_column(position_column, new_positions[index])
    end
  end
end
brendon commented 7 years ago

Thanks @MarcReniu, the important part is this:

records.each_with_index do |record, index|
  record.update_column(position_column, new_positions[index])
end

which is what a lot of people do when processing a reorder request from the browser anyway (many drag and drop plugins send through just an array of id's. I prefer to set the position explicitly these days. That, coupled with our scope change detection allows for some pretty powerful reordering (i.e. automatic removal from the old scope, insertion at the specified position in the new scope.)

We recently merged a PR that dealt with items having the same position integer.

The fact that you have a gap between the top_of_list value and the first position is of no great consequence because what you really care about is the relative positioning. The user don't get to see the position value normally.

I recently had to refactor some code where I had the id of the item being moved, and an array of ids that included the id of the item being moved:

  # Expects an array of sibling_ids (strings), and a parent component_instance
  # This method relies on scope_changed? in acts_as_list to do the magic of positioning
  def move(parent, sibling_ids = [], scope = parent.children)
    self.parent = parent

    if sibling_ids.present?
      raise ArgumentError.new('self.id must exist in sibling_ids') unless sibling_ids.include?(id.to_s)

      if sibling_ids.size == 1
        self.position = 1
      elsif sibling_ids.size > 1
        position_among_siblings = sibling_ids.index(id.to_s)
        reference = scope.offset(position_among_siblings).first

        self.position = reference.position if reference
      end
    end

    save
  end

Sorry, that's a bit rambling but it might give you some ideas :)

I'll close this for now but do get in touch if you have any more issues :)