I still have gap between position...

[robypez]

Hello, I'm on the latest version on Rails 5.2.

I have to save 2 times to remove the gap

@cover.items
  Cover::Item Load (0.6ms)  SELECT "cover_items".* FROM "cover_items" WHERE "cover_items"."cover_id" = $1 ORDER BY "cover_items"."position" ASC  [["cover_id", 64]]
=> [#<Cover::Item:0x007feae8c59150 id: 308, cover_id: 64, teaser_id: 200, position: 1, created_at: Wed, 08 Aug 2018 22:15:20 CEST +02:00, updated_at: Wed, 08 Aug 2018 22:15:20 CEST +02:00>,
 #<Cover::Item:0x007feae8bb1770 id: 284, cover_id: 64, teaser_id: 214, position: 3, created_at: Wed, 08 Aug 2018 16:54:47 CEST +02:00, updated_at: Wed, 08 Aug 2018 22:15:20 CEST +02:00>,
 #<Cover::Item:0x007feae8bb07f8 id: 285, cover_id: 64, teaser_id: 210, position: 4, created_at: Wed, 08 Aug 2018 16:54:47 CEST +02:00, updated_at: Wed, 08 Aug 2018 22:15:20 CEST +02:00>,
 #<Cover::Item:0x007feae91b6a58 id: 290, cover_id: 64, teaser_id: 54, position: 5, created_at: Wed, 08 Aug 2018 17:44:27 CEST +02:00, updated_at: Wed, 08 Aug 2018 22:15:20 CEST +02:00>]

Any idea?

[robypez]

Here is the log

[1mSQL (3.2ms)  UPDATE "cover_items" SET "position" = $1, "updated_at" = $2 WHERE "cover_items"."id" = $3  [["position", 2], ["updated_at", "2018-08-08 20:57:06.146173"], ["id", 284]]
   (2.3ms)  SELECT COUNT(*) FROM "cover_items" WHERE "cover_items"."cover_id" = $1 AND ("cover_items"."position" = 2)  [["cover_id", 64]]
  SQL (2.0ms)  UPDATE "cover_items" SET "position" = $1, "updated_at" = $2 WHERE "cover_items"."id" = $3  [["position", 3], ["updated_at", "2018-08-08 20:57:16.426555"], ["id", 285]]
   (2.0ms)  SELECT COUNT(*) FROM "cover_items" WHERE "cover_items"."cover_id" = $1 AND ("cover_items"."position" = 3)  [["cover_id", 64]]
  SQL (2.3ms)  UPDATE "cover_items" SET "position" = $1, "updated_at" = $2 WHERE "cover_items"."id" = $3  [["position", 4], ["updated_at", "2018-08-08 20:57:27.174361"], ["id", 290]]
   (2.4ms)  SELECT COUNT(*) FROM "cover_items" WHERE "cover_items"."cover_id" = $1 AND ("cover_items"."position" = 4)  [["cover_id", 64]]
  SQL (3.0ms)  UPDATE "cover_items" SET "position" = ("cover_items"."position" + 1), "updated_at" = '2018-08-08 20:57:46.777546' WHERE "cover_items"."cover_id" = $1 AND ("cover_items"."position" >= 1)  [["cover_id", 64]]
  SQL (2.1ms)  INSERT INTO "cover_items" ("cover_id", "teaser_id", "position", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5) RETURNING "id"  [["cover_id", 64], ["teaser_id", 217], ["position", 1], ["created_at", "2018-08-08 20:57:46.775656"], ["updated_at", "2018-08-08 20:57:46.775656"]]

[brendon]

Hi @robypez, thanks for raising an issue. We'll need more information I'm afraid. Give us what you're executing and what you expect to have happen.

[robypez]

I have a class Cover with many items

class Cover < ApplicationRecord
 has_many :items, -> { order(position: :asc) }, inverse_of: :cover, dependent: :destroy, index_errors: true
end

class Cover < ApplicationRecord
  class Item < ApplicationRecord
    acts_as_list scope: :cover, sequential_updates: false
 end
end

The cover is the cover of this site Dmove.it The cover has 4 items (link to contents) Every item has a position, I have different cover version.

When I replace an Item and I save the cover the first time I have a gap between the position. The second time I save the gap disappear.

I add a after_save callback to my cover to log the items position and they are ok, after save

[#<Cover::Item:0x007fe190819920
12:22:32 web.1       |   id: 284,
12:22:32 web.1       |   cover_id: 64,
12:22:32 web.1       |   teaser_id: 214,
12:22:32 web.1       |   position: 2,
12:22:32 web.1       |   created_at: Wed, 08 Aug 2018 16:54:47 CEST +02:00,
12:22:32 web.1       |   updated_at: Wed, 08 Aug 2018 18:21:40 CEST +02:00>,
12:22:32 web.1       |  #<Cover::Item:0x007fe190819740
12:22:32 web.1       |   id: 285,
12:22:32 web.1       |   cover_id: 64,
12:22:32 web.1       |   teaser_id: 210,
12:22:32 web.1       |   position: 3,
12:22:32 web.1       |   created_at: Wed, 08 Aug 2018 16:54:47 CEST +02:00,
12:22:32 web.1       |   updated_at: Wed, 08 Aug 2018 18:21:40 CEST +02:00>,
12:22:32 web.1       |  #<Cover::Item:0x007fe190819560
12:22:32 web.1       |   id: 290,
12:22:32 web.1       |   cover_id: 64,
12:22:32 web.1       |   teaser_id: 54,
12:22:32 web.1       |   position: 4,
12:22:32 web.1       |   created_at: Wed, 08 Aug 2018 17:44:27 CEST +02:00,
12:22:32 web.1       |   updated_at: Wed, 08 Aug 2018 18:21:40 CEST +02:00>,
12:22:32 web.1       |  #<Cover::Item:0x007fe190818548
12:22:32 web.1       |   id: 300,
12:22:32 web.1       |   cover_id: 64,
12:22:32 web.1       |   teaser_id: 207,
12:22:32 web.1       |   position: 1,
12:22:32 web.1       |   created_at: Wed, 08 Aug 2018 18:21:40 CEST +02:00,
12:22:32 web.1       |   updated_at: Wed, 08 Aug 2018 18:21:40 CEST +02:00>] 

but later the system execute this query

SQL(3.0ms)[0m UPDATE "cover_items" SET "position" = ("cover_items"."position" + 1), "updated_at" = '2018-08-08 20:57:46.777546' WHERE "cover_items"."cover_id" = $1 AND ("cover_items"."position" >= 1) [["cover_id", 64]]

and it create the gap...

[#<Cover::Item:0x007fe1a6f4f828
13:03:37 web.1       |   id: 300,
13:03:37 web.1       |   cover_id: 64,
13:03:37 web.1       |   teaser_id: 207,
13:03:37 web.1       |   position: 1,
13:03:37 web.1       |   created_at: Wed, 08 Aug 2018 18:21:40 CEST +02:00,
13:03:37 web.1       |   updated_at: Wed, 08 Aug 2018 18:21:40 CEST +02:00>,
13:03:37 web.1       |  #<Cover::Item:0x007fe1a6f4f418
13:03:37 web.1       |   id: 284,
13:03:37 web.1       |   cover_id: 64,
13:03:37 web.1       |   teaser_id: 214,
13:03:37 web.1       |   position: 3,
13:03:37 web.1       |   created_at: Wed, 08 Aug 2018 16:54:47 CEST +02:00,
13:03:37 web.1       |   updated_at: Wed, 08 Aug 2018 18:21:40 CEST +02:00>,
13:03:37 web.1       |  #<Cover::Item:0x007fe1a6f4f080
13:03:37 web.1       |   id: 285,
13:03:37 web.1       |   cover_id: 64,
13:03:37 web.1       |   teaser_id: 210,
13:03:37 web.1       |   position: 4,
13:03:37 web.1       |   created_at: Wed, 08 Aug 2018 16:54:47 CEST +02:00,
13:03:37 web.1       |   updated_at: Wed, 08 Aug 2018 18:21:40 CEST +02:00>,
13:03:37 web.1       |  #<Cover::Item:0x007fe1a6f4ebd0
13:03:37 web.1       |   id: 290,
13:03:37 web.1       |   cover_id: 64,
13:03:37 web.1       |   teaser_id: 54,
13:03:37 web.1       |   position: 5,
13:03:37 web.1       |   created_at: Wed, 08 Aug 2018 17:44:27 CEST +02:00,
13:03:37 web.1       |   updated_at: Wed, 08 Aug 2018 18:21:40 CEST +02:00>]

[brendon]

Thanks @robypez, are you able to replicate this problem in the console? It'd be good to see the console commands. There might be some double-saving going on somewhere? Acts As List isn't perfect at keeping the positions gapless but most of the time it is.

[robypez]

No, because cover items are created on frontend using a vue app. I just have the log...

Here is my post action. My cover has 4 items, so in my update I send also the item that I'm replacing.

Parameters: {"utf8"=>"✓", "authenticity_token"=>"QHdkHUAtjxxF3y+ctQvi8zgyGGb7F2HiufH4sWuUJaBVKH3fwwT1NUkZvgiEmInNmC76krN17qJJUFoFw259eA==", "cover"=>{"format"=>"hssh", "published_at"=>"2017-12-29 12:00", "items_attributes"=>{"4"=>{"id"=>"315", "_destroy"=>"1"}, "0"=>{"position"=>"1", "teaser_id"=>"217", "teaser_attributes"=>{"body"=>"<p><a href=\"/news/indiscrezioni-su-porsche-mission-e-tre-allestimenti-a-partire-da-65-000-euro\">Indiscrezioni su Porsche Mission E: <span class=\"bigtitle\">tre allestimenti a partire da 65.000 euro</span></a></p>"}, "teaser_image_horizontal_attributes"=>{"id"=>"1619", "attachment_cache"=>""}}, "1"=>{"id"=>"284", "position"=>"2", "teaser_attributes"=>{"id"=>"214", "body"=>"<p><a href=\"/news/e-via-flex-e-parte-la-rete-di-ricarica-ultraveloce-tra-italia-francia-e-spagna\"><span class=\"bigtitle\">E-VIA FLEX-E, parte la rete di ricarica ultraveloce </span>tra Italia, Francia e Spagna</a></p>"}, "teaser_image_square_attributes"=>{"id"=>"1596", "attachment_cache"=>""}}, "2"=>{"id"=>"285", "position"=>"3", "teaser_attributes"=>{"id"=>"210", "body"=>"<p><a href=\"/news/lg-electronic-e-here-insieme-per-l-auto-connessa-e-autonoma\"><span class=\"bigtitle\">LG Electronic e HERE insieme</span> per l'auto connessa e autonoma</a></p>"}, "teaser_image_square_attributes"=>{"id"=>"1570", "attachment_cache"=>""}}, "3"=>{"id"=>"290", "position"=>"4", "teaser_attributes"=>{"id"=>"54", "body"=>"<p><a href=\"/news/di-cosa-parla-dmove-in-tre-domande\"><span class=\"bigtitle\">Cos'è DMove.it in 3 domande</span></a></p>"}, "teaser_image_horizontal_attributes"=>{"id"=>"469", "attachment_cache"=>""}}}}, "commit"=>"Salva ed esci", "id"=>"64"}

This is the item that I'm replacing

"4"=>{"id"=>"315", "_destroy"=>"1"}

Here in the log the item is destroyd

[1mSQL (1.2ms) DELETE FROM "cover_items" WHERE "cover_items"."id" = $1 [["id", 315]]

At this moment my positions are ok! I destroy the item replaced and in the post attributes I have the 4 items of my cover with the right positions: 1-2-3 and 4.

But...

[1mSQL (1.6ms)  UPDATE "cover_items" SET "position" = $1, "updated_at" = $2 WHERE "cover_items"."id" = $3  [["position", 2], ["updated_at", "2018-08-08 22:24:55.391663"], ["id", 284]]
   (2.0ms)  SELECT COUNT(*) FROM "cover_items" WHERE "cover_items"."cover_id" = $1 AND ("cover_items"."position" = 2)  [["cover_id", 64]]
  SQL (1.2ms)  UPDATE "cover_items" SET "position" = $1, "updated_at" = $2 WHERE "cover_items"."id" = $3  [["position", 3], ["updated_at", "2018-08-08 22:24:55.405269"], ["id", 285]]
   (1.3ms)  SELECT COUNT(*) FROM "cover_items" WHERE "cover_items"."cover_id" = $1 AND ("cover_items"."position" = 3)  [["cover_id", 64]]
  SQL (1.2ms)  UPDATE "cover_items" SET "position" = $1, "updated_at" = $2 WHERE "cover_items"."id" = $3  [["position", 4], ["updated_at", "2018-08-08 22:24:55.418007"], ["id", 290]]
   (1.1ms)  SELECT COUNT(*) FROM "cover_items" WHERE "cover_items"."cover_id" = $1 AND ("cover_items"."position" = 4)  [["cover_id", 64]]
  SQL (2.7ms)  UPDATE "cover_items" SET "position" = ("cover_items"."position" + 1), "updated_at" = '2018-08-08 22:24:55.428907' WHERE "cover_items"."cover_id" = $1 AND ("cover_items"."position" >= 1)  [["cover_id", 64]]
  SQL (1.5ms)  INSERT INTO "cover_items" ("cover_id", "teaser_id", "position", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5) RETURNING "id"  [["cover_id", 64], ["teaser_id", 217], ["position", 1], ["created_at", "2018-08-08 22:24:55.428191"], ["updated_at", "2018-08-08 22:24:55.428191"]]

Here it creates the gap.

[brendon]

Ok. I assume you have some kind of API backend that rails is providing? What about the code from this. I really need to know what is being run in your create/update/destroy actions to be able to verify the problem. The code from the rails application is what I need.

[axos88]

I am also experiencing the issue.

Model.create(attributes.merge(position: 42))

will not take into account the length of the list and will create gaps.

Model.each { |m| m.update(position: m.position + 1) }; Model.create(attributes.merge(position: 0)

will also create a gap

[robypez]

Here are my models

class Cover < ApplicationRecord
  include Cover::Formats
  include Cover::BeforeDestroyValidations

  has_many :items, -> { order(position: :asc) }, inverse_of: :cover, dependent: :destroy, index_errors: true
  has_many :teasers, through: :items
  has_many :contents, through: :teasers

  accepts_nested_attributes_for :items, allow_destroy: true, reject_if: -> (attributes) do
    attributes[:teaser_id].blank? && attributes[:teaser_attributes].try(:[], :id).blank?
  end

  before_validation :nested_attributes_fix

  validate :contents_not_ready_or_publication_validation
  validate :contents_published_after_cover_validation

  validate :items_size_validation, if: -> { published_at.present? }

  scope :default_order, -> do
    order 'published_at DESC NULLS LAST'
  end

  scope :currents, -> do
    where.
      not(published_at: nil).
      where(arel_table[:published_at].lteq('NOW()')).
      order(published_at: :desc)
  end

  after_save :destroy_redundant_cover_items

  def self.current
    currents.first
  end

  def items_size
    items.size
  end

  def max_items_size
    FORMATS_ITEMS_SIZE[format]
  end

  protected

  def destroy_redundant_cover_items
    items.offset(max_items_size).destroy_all if max_items_size
  end

  def contents_not_ready_or_publication_validation
    ids = contents.reject { |content| content.ready_or_publication? }.map { |content| content.id }

    errors.add :contents, :not_ready_or_publication, content_ids: ids.join(', ') if ids.present?
  end

  def contents_published_after_cover_validation
    return unless published_at

    ids = contents.select do |content|
      content.publishing? and content.published_at > cover.published_at
    end.map { |content| content.id }

    errors.add :contents, :published_after_cover, content_ids: ids.join(', ') if ids.present?
  end

  def items_size_validation
    errors.add :items_size, :wrong_length, count: max_items_size if max_items_size and items_size < max_items_size
  end

  # Fixes this: https://github.com/carrierwaveuploader/carrierwave/issues/2206
  def nested_attributes_fix
    items.select { |item| item.teaser_image.try(:attachment_cache).present? }.each do |item|
      item.teaser_image.attachment_will_change!
    end
  end
end

class Cover < ApplicationRecord
  class Item < ApplicationRecord
    acts_as_list scope: :cover, sequential_updates: false
    skip_callback :destroy, :after, :decrement_positions_on_lower_items

    belongs_to :cover, inverse_of: :items, touch: true
    belongs_to :teaser, inverse_of: :cover_items, touch: true
    Teaser::IMAGE_ASSOCIATIONS.values.each do |image_association|
      has_one :"teaser_#{image_association}", through: :teaser, source: image_association
    end

    delegate *Teaser::IMAGE_ASSOCIATIONS.values, :images_with_format, :images, :body, to: :teaser, prefix: true, allow_nil: true
    delegate :format, :items_image_versions, to: :cover, prefix: true, allow_nil: true

    validates_uniqueness_of :teaser_id, scope: [:cover_id]
    validates_presence_of :position
    validates_presence_of :teaser_image

    accepts_nested_attributes_for :teaser, reject_if: -> (attributes) { attributes[:id].blank? }
    accepts_nested_attributes_for *Teaser::IMAGE_ASSOCIATIONS.values.map { |image_association| :"teaser_#{image_association}" }

    before_save :log_position

    def teaser_image_version
      return if position.blank? or cover.blank?

      cover_items_image_versions[position-1]
    end

    def teaser_image_association
      :"teaser_image_#{teaser_image_version}" if teaser_image_version
    end

    def teaser_image
      send teaser_image_association if teaser_image_association
    end

    def build_teaser_image
      return unless teaser_image_version

      build_teaser if teaser.blank?

      teaser.send :"build_image_#{teaser_image_version}"
    end

    def log_position
    end
  end
end

[brendon]

@axos88, You don't need to shuffle things around to insert or move an item to a position, just set the new position for the item and acts_as_list will move things out of the way for you. That's the primary purpose for the gem.

In regards to your first example, acts_as_list isn't going to mess with your intended position integer if you specify it. If you want to add to the end of the list there is a method for that, or simply omit the position and it will be added to the end of the list (unless you've configured otherwise).

I hope that helps :D

[brendon]

@robypez, I suspect it's this line:

skip_callback :destroy, :after, :decrement_positions_on_lower_items

decrement_positions_on_lower_items helps eliminate the gap in several scenarios. Try removing that skip callback.

[axos88]

@brendon, the frontend sends back the new order of things along with any new items (images) that were added. Processing that request results in the code shown above. It's quite realistic to insert new items and reorder the rest of them in the same request, and it causes bugs on the subsequent requests due to the gaps created

As for the other case, I'm strongly opinionated that this gem shouldn't allow gaps even in such cases. Because after having say filled position 0,1,2,3,4,42,4242 and something want to move the item 1 and 2 to the positions 5 and 6 (between 42 and 4242), he will most probably end up with the list 0,3,4,6,7,42,4242 [that will be reduced to 0,1,2,4,5,42,4242] instead of 0,3,4,42,4242,4243?, 4244?.

I wouldn't call allowing gaps to be a feature, hence the creation of this issue, but at least make it optional. Or if this is intentional than the attribute is not a position, which implies no gaps by definition, but rather a priority.

[brendon]

Hi @axos88, you haven't shown me your controller code so I can't commend on how you're doing your reordering. Give me a look if you want. There are two ways to do it. Either send back to the controller the item that was moved, and the whole collection of id's in the scope in their determined order. Then in the controller determine the position of the item in the overall list of items and move it to that position using acts_as_list... or, send back a list of id's to a collection action in the controller, then explicitly set the position of each of these using something like this:

  def sort(array_of_ids)
    array_of_ids.each.with_index(1) do |id, index|
      where(:id => id).update_all(:position => index)
    end
  end

That will skip the callbacks as far as I'm aware as it executes straight SQL.

Regarding you skipping one of the acts_as_list callbacks, if you do this, then it's hard for us to provide you support since you're changing the way acts_as_list is supposed to work.

If you think we could be doing something better, please do submit a pull request with your changes. I'd be more than happy to review it and merge it if it helps make things work better. :)

[axos88]

Hi,

I'm not bypassing any of the callbacks.

I'm not using rails, but i'll share a piece of code to reproduce the issue:

class Image < ActiveRecord::Base
    (...)
    acts_as_list scope: [:listing_id], top_of_list: 0
end

# Create a listing with 3 images
Image.create!(listing_id: 1, position: 0, file_name: 'img1.jpg') # id 42
Image.create!(listing_id: 1, position: 1, file_name: 'img2.jpg') # id 43
Image.create!(listing_id: 1, position: 2, file_name: 'img3.jpg') # id 44

# The request comes in with the following data
{ 
  listing: {
    images: [
      { file_name: 'img_new.jpg', position: 0 },
      { id: 42, position: 1 },
      { id: 43, position: 2 }
      { id: 44, position: 3 }
    ]
  }
}

# Upload position(s)
# The code below results in the following code:
# Image.find(42).update(position: 1)
# Image.find(43).update(position: 2)
# Image.find(44).update(position: 3)
params[:listing][:images].reject { |img| img[:id].nil? }.each do |img|
  Image.find(img[:id]).update(img)
end

# Create new image(s)
# This results in 
# Image.create!([{ file_name: 'img_new.jpg', position: 0 }])
new_images = params[:listing][:images].select { |img| img[:id].nil? }.each do |img|
Image.create!(img)

Now Image.all.order(:position).pluck(:position) returns: [0, 2,3,4] instead of [0,1,2,3]

So again, the code to reproduce is:

Image.create!(listing_id: 1, position: 0, file_name: 'img1.jpg') # id 42
Image.create!(listing_id: 1, position: 1, file_name: 'img2.jpg') # id 43
Image.create!(listing_id: 1, position: 2, file_name: 'img3.jpg') # id 44

Image.find(42).update(position: 1)
Image.find(43).update(position: 2)
Image.find(44).update(position: 3)
Image.create!([{ file_name: 'img_new.jpg', position: 0 }])

assert(Image.all.order(:position).pluck(:position) == [0,1,2,3]) # FAIL

[brendon]

Thanks for the extra detail @axos88, on paper it should work. Could you provide one more bit of information: What are the positions of the items after each step in the process, i.e. after each step of the code in the last block can you grab a hash of id: :position so we can see at what point things to awry?

[lsarni]

I'm also seeing so gaps on my lists. I found this recently since I added a drag and drop option for reordering the list that uses the position in the view to move the item and it breaks for all the items after the gap.

I'm using insert_at to reorder the list and this works as expected moving the item to the position I sent, but the list gets incorrectly ordered due to the presence of the gap.

All my lists seem to have this problem of missing one position at some point.

[brendon]

I'm wondering if it's because the list items themselves aren't being reloaded after each insert_at? They'll still think they have their old position, so when you subsequently change their position, the SQL will be all wrong. Can you try an experiment where you call .reload on each item before setting its position?

[democlitos]

@lsarni have you found a way to go through this issue?

[oyeanuj]

@brendon I've a question on this topic as well that I wasn't sure was answered. I have data right now that looks like something like this:

    [0] "Id: 56, Position: 2",
    [1] "Id: 55, Position: 6",           # <----------------
    [2] "Id: 57, Position: 53",
    [3] "Id: 59, Position: 55",
    [4] "Id: 60, Position: 56",

So, in this scenario, the frontend tells me that id:55 has been moved to the third item. Ideally, I'd like to move it and clean up the gaps when I update the item position. It seems that .insert_at(3) adds it to position "3" (rather than as the third element) which still keeps it as the second element. Is there a different method to arrive at the desired solution?

[brendon]

Hi @oyeanuj, acts_as_list treats the position column as a literal position value (rather than the position in the list), though those two things could be the same, and ideally they should. Positions are assumed to be sequential. Some of the positioning code is tolerant to sparse positions, but that has been patched by people over the years and so some methods (like insert_at probably aren't or can't be fixed). insert_at literally means, give this item this position integer. I don't think I ever use this method, I just set the position directly and allow the gem to take care of the rest.

If you want to position an item before another item, you need to ask the other item what its position is, then set the moved item's position to be that same position. acts_as_list will then shuffle the rest of the items out of the way.

[oyeanuj]

@brendon Interesting, so for the case above, let me know if the understanding is correct -

1. I need to find the item at that desired index,
2. fetch its position and
3. set the the position of requested item at the position found in (2)

Is that correct?

[brendon]

That's correct. As far as I know, there's no built in method to do this.

[oyeanuj]

@brendon Got it, thanks for confirming!

[dgimb89]

We recently debugged a similar problem which resulted from misusage of acts_as_list. Here's a code excerpt to illustrate our issue:

Models

class Playtest < ApplicationRecord
  has_many :playtest_review_processors, dependent: :destroy
  accepts_nested_attributes_for :playtest_review_processors, allow_destroy: true
end

class PlaytestReviewProcessor < ApplicationRecord
  belongs_to :playtest
  belongs_to :review_processor

  acts_as_list scope: :playtest
end

When updating the list of PlaytestReviewProcessor we would still reserve a slot for the to-be-deleted record:

# We allow manual reordering in the frontend, thus pass position for the association
# Note: We still reserve a position slot for the to-be-deleted record which leads to the gap
playtest.update!({"playtest_review_processors_attributes"=>[
  {"id"=>53, "playtest_id"=>316, "position"=>1, "review_processor_id"=>38},
  {"id"=>1, "playtest_id"=>316, "position"=>2, "review_processor_id"=>2},
  {"id"=>90, "playtest_id"=>316, "position"=>3, "review_processor_id" =>39, "_destroy"=>1},
  {"id"=>55, "playtest_id"=>316, "position"=>4, "review_processor_id"=>1},
  {"playtest_id"=>316, "position"=>5, "review_processor_id"=>2}
]})

# PlaytestReviewProcessor Destroy (0.7ms)  DELETE FROM "playtest_review_processors" WHERE "playtest_review_processors"."id" = 90
# PlaytestReviewProcessor Update All (1.6ms)  UPDATE "playtest_review_processors" SET "position" = ("playtest_review_processors"."position" - 1) WHERE "playtest_review_processors"."playtest_id" = 316 AND ("playtest_review_processors"."position" > 3)
# PlaytestReviewProcessor Update All (0.2ms)  UPDATE "playtest_review_processors" SET "position" = ("playtest_review_processors"."position" + 1) WHERE "playtest_review_processors"."playtest_id" = 316 AND ("playtest_review_processors"."position" >= 5)
# PlaytestReviewProcessor Create (2.6ms)  INSERT INTO "playtest_review_processors" ("playtest_id", "review_processor_id", "position") VALUES (316, 2, 5) RETURNING "id"

ap playtest.playtest_review_processors.order(:position)
[
  [0] #<PlaytestReviewProcessor:0x0000aaaaeff74190> {
                       :id => 53,
              :playtest_id => 316,
      :review_processor_id => 38,
                 :position => 1
  },
  [1] #<PlaytestReviewProcessor:0x0000ffff9b25bf70> {
                       :id => 1,
              :playtest_id => 316,
      :review_processor_id => 2,
                 :position => 2
  },
  [2] #<PlaytestReviewProcessor:0x0000ffff9b25bdb8> {
                       :id => 55,
              :playtest_id => 316,
      :review_processor_id => 1,
                 :position => 3
  },
  [3] #<PlaytestReviewProcessor:0x0000ffff9b25bc50> {
                       :id => 107,
              :playtest_id => 316,
      :review_processor_id => 2,
                 :position => 5
  }
]

As you can see, this scenario produces a gap in the list at position 4. This is prevented by not reserving positions for records that will be deleted:

playtest.update!({"playtest_review_processors_attributes"=>[
  {"id"=>53, "playtest_id"=>316, "position"=>1, "review_processor_id"=>38},
  {"id"=>1, "playtest_id"=>316, "position"=>2, "review_processor_id"=>2},
  {"id"=>122, "playtest_id"=>316, "position"=>nil, "review_processor_id"=>39, "_destroy"=>1},
  {"id"=>55, "playtest_id"=>316, "position"=>3, "review_processor_id"=>1},
  {"playtest_id"=>316, "position"=>4, "review_processor_id"=>2}
]})

# PlaytestReviewProcessor Destroy (1.5ms)  DELETE FROM "playtest_review_processors" WHERE "playtest_review_processors"."id" = 122
# PlaytestReviewProcessor Update All (0.4ms)  UPDATE "playtest_review_processors" SET "position" = ("playtest_review_processors"."position" - 1) WHERE "playtest_review_processors"."playtest_id" = 316 AND ("playtest_review_processors"."position" > 3)
# PlaytestReviewProcessor Update (0.4ms)  UPDATE "playtest_review_processors" SET "position" = 3 WHERE "playtest_review_processors"."id" = 55
#  (7.9ms)  SELECT COUNT(*) FROM "playtest_review_processors" WHERE "playtest_review_processors"."playtest_id" = 316 AND ("playtest_review_processors"."position" = 3)
# PlaytestReviewProcessor Update All (0.6ms)  UPDATE "playtest_review_processors" SET "position" = ("playtest_review_processors"."position" + 1) WHERE "playtest_review_processors"."playtest_id" = 316 AND ("playtest_review_processors"."position" >= 4)
# PlaytestReviewProcessor Create (1.3ms)  INSERT INTO "playtest_review_processors" ("playtest_id", "review_processor_id", "position") VALUES (316, 2, 4) RETURNING "id"

[
  [0] #<PlaytestReviewProcessor:0x0000aaaaef75c6d8> {
                       :id => 53,
              :playtest_id => 316,
      :review_processor_id => 38,
                 :position => 1
  },
  [1] #<PlaytestReviewProcessor:0x0000ffff9b19fe38> {
                       :id => 1,
              :playtest_id => 316,
      :review_processor_id => 2,
                 :position => 2
  },
  [2] #<PlaytestReviewProcessor:0x0000ffff9b19fcd0> {
                       :id => 55,
              :playtest_id => 316,
      :review_processor_id => 1,
                 :position => 3
  },
  [3] #<PlaytestReviewProcessor:0x0000ffff9b19fb68> {
                       :id => 124,
              :playtest_id => 316,
      :review_processor_id => 2,
                 :position => 4
  }
]

acts_as_list cannot guarantee a gapless order if position attributes are passed explicitly.

[brendon]

Thanks @dgimb89, that's true regarding explicit positions. Something that may help is the destroyed_by_association method that you can use to determine if something is being destroyed by a cascade.

[Merovex]

I'm running into this as well using insert_at(params[:position].to_i).

It looks like when I attempt to move something to the end of the list, it will take the position number after the last item. Starts with [1,2,3,4] and when I move 2 after 4, it becomes [1,3,4,5]. The problem is when I try to move 5 in front of 4, it doesn't move. The solution is to bound-check and reindex. What works for me is:

In the controller:

@child.insert_at(child_params[:position].to_i)
@parent.reindex_children if (@parent.children.count < @child.position)

In the parent

def reindex_children
  children("position ASC").each.with_index(1) do |child, position|
    child.update(position: position) unless child.position == position
  end
end

I hope this helps someone.

[crespire]

I wonder if we can have a built-in clean or fix_gaps that can help to remove gaps. Seems like acts_as_list mostly expects contiguous positions. I am working with potential NullObject items in a list, so can see this being a big problem.

[brendon]

The problem is multiple operations can step on each other and result in a list that has gaps. I'd be happy to look at a PR that adds that a function to harmonise the gaps but I think it'll be more complicated than you think given the different ways that scopes can be defined :)

[crespire]

The problem is multiple operations can step on each other and result in a list that has gaps. I'd be happy to look at a PR that adds that a function to harmonise the gaps but I think it'll be more complicated than you think given the different ways that scopes can be defined :)

Appreciate your openness on this, I can try to give it a go once I have some time. Might be a while haha.

[brendon]

Haha! No problem at all. I know what that's like :D

[mmustala]

This is what created a gap in my case. We have a list that is accessed via an API. Our UI created two records with predefined positions (1 and 2), but sent the requests in parallel. If the record with position 2 got created first, the creation of the record with position 1 pushed the position 2 record one step further, so at the end they got positions 1 and 3.

The solution for this would be not to update other records if the predefined position is available.

[brendon]

One of the main points of acts_as_list is automatic list management so it tries to help you by shuffling things around to get them out of your way. If you don't need that then you might be able to get away with not using it and just relying on your own methods of defining the position as you're currently doing? :)

[mmustala]

I was just thinking, if the gem should check if the defined position is free, and skip shuffling. I might be able to work on a pull request for that, if you think it's a good idea. Of course, when an existing record is moved to a new position, some shuffling is needed.

[Merovex]

PR submitted (my first, AFAIK). Bounds checks insert_at_position to ensure the position does not exceed the maximum position.

[brendon]

Also have a look at this: https://gist.github.com/brendon/d6cfd60cb5e70dc77a15a2476f04d279

I've been using this instead of acts_as_list on a part of my app. The main aim was to guarantee contiguous position values (which it appears to do).

One day I intend to release it as a gem... :D

[brendon]

I'm going to close this now. Please open a new issue if needed. The final advice is:

list_item.update position: 4 is the best way to move an item around in a list. This will trigger a set of callbacks behind the scenes that move other items out of the way.

When making a new item you can either leave position unset and it will be appended to the end of the list or you can specify it as an integer value and the new item will be put at that position and again, the callbacks will shuffle things out of the way for you. It should be that simple. The callbacks respect the scopes that you've defined for your lists.

list_scope.list_items.create name: 'Something', position: 4