brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.05k stars 356 forks source link

Update doesn't always reposition all items #439

Closed alexanderholder closed 2 months ago

alexanderholder commented 2 months ago

Steps to reproduce

Using update seems to have issues inserting sometimes, where as insert_at seems to work. Keen to understand if these should behave the same as we currently would just use update in our controllers.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "rails", github: "rails/rails", branch: "main"

  gem "sqlite3"

  gem "acts_as_list"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :projects, force: true do |t|
    t.string :title
    t.boolean :completed, default: false
    t.timestamps
  end

  create_table :project_items, force: true do |t|
    t.integer :project_id
    t.string :title
    t.integer :position
    t.timestamps
  end
end

class ProjectItem < ActiveRecord::Base
  acts_as_list scope: :project
  belongs_to :project
end

class Project < ActiveRecord::Base
  has_many :project_items, dependent: :destroy
end

class BugTest < Minitest::Test
  def test_association_stuff
    project = Project.create!

    5.times do
      ProjectItem.create!(project: project)
    end

    10.times do
      project.project_items.each do |project_item|
        project_item.update!(position: rand(1..5))

        assert_equal [1, 2, 3, 4, 5], ProjectItem.where(project: project).pluck(:position).sort
      end
    end
  end
end

Expected behavior

update should always work - it sometimes works (I used the 10.times to ensure it fails)

Actual behavior

update seems to error every now and then, where insert_at seems to work

System configuration

Rails version: 7.1+

Ruby version: 3.3.4

brendon commented 2 months ago

This feels like a race condition but perhaps instead of having randomly computed position updates, try to find a sequence that always fails so that it gives us something to reliably test against.

Also have a look at my other gem: https://github.com/brendon/positioning

It uses an advisory lock to ensure sequential list actions across processes.

brendon commented 2 months ago

Actually your testing problem stems from stale data. As you move through the items and update their position one at a time, ActsAsList is moving the other items out of the way but you're not reloading the future items before acting on them so ActsAsList thinks their current position is something other than what it really is. The tests always pass if you just do:

project_item.reload.update!(position: rand(1..5))

Hope that helps. I'm not sure there's a way to ensure all in-memory instances of an object are updated to be current with the database, and I'm sure that would become unwieldily eventually anyway.

alexanderholder commented 2 months ago

oh of course! thanks for the speedy reply!

brendon commented 2 months ago

No worries! :)