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

duplicate positions on scoped list #345

Closed loicginoux closed 5 years ago

loicginoux commented 5 years ago

I have a weird behaviour with duplicate positions. This is discussed and probably the same as in #333 and probably #76 and #317

Here is my setup

class Alert < ActiveRecord::Base
  belongs_to :owner, :class_name => "User"
  belongs_to :folder

  acts_as_list scope: [:owner_id, :folder_id]
  default_scope order: "position ASC"
end

folder = create(:folder, user: user) 
alert1 = create(:alert, owner: user, name: "B") 
alert2 = create(:alert, owner: user, folder_id: folder.id, position: 1, name: "D") 
alert3 = create(:alert, owner: user, folder_id: folder.id, position: 2, name: "A") 
alert4 = create(:alert, owner: user, name: "E") 
alert5 = create(:alert, owner: user, name: "C") 

p "alert ids in folder: #{folder.reload.alerts.map(&:id)}"
p "positions in folder: #{folder.reload.alerts.map(&:position)}"

# alert ids in folder: [2, 3]
# positions in folder: [1, 2]

alerts = Alert.where(id: [alert1, alert4, alert5])
alerts.update_all(folder_id: folder.id)
alerts.each do |alert|
  alert.move_to_bottom
end

p "alert ids in folder: #{folder.reload.alerts.map(&:id)}"
p "positions in folder: #{folder.reload.alerts.map(&:position)}"

# alert ids in folder: [1, 2, 3, 4, 5]
# positions in folder: [1, 1, 1, 2, 3]
# Error here !

I was here expecting the following:

# alert ids in folder: [2, 3, 1, 4, 5]

Is there a quick fix to make this work ?

brendon commented 5 years ago

Hi @loicginoux, I think this stems from your update_all call which just issues an SQL query and doesn't trigger the acts_as_list callbacks. You then have a situation with alerts with the same positions in the same scope. Calling move_to_bottom is going to have a hard time here because of the duplicates. It does some math on the positions but isn't aware of duplicates as far as I'm aware. The solution would be to just update the folder of each alert using normal methods so that the scope change logic kicks in. You have to do this one at a time in order for the positions to be maintained correctly.

Feel free to close this if that solves your problem, otherwise let me know if you need more assistance :)

loicginoux commented 5 years ago

ok thank you, it works by using another method when updating folder that triggers callbacks. maybe the documentation could make it clearer... it really seems like a bug at first sight.

brendon commented 5 years ago

Thanks @loicginoux. I guess it's one of those learnings, that update_all doesn't trigger callbacks, or validations. Callbacks are the only way we can do the magic of moving things around and scope change detection.

loicginoux commented 5 years ago

Sorry I still have issues with alerts with same position numbers with the following, I have 3 alerts existing on a folder correctly positioned, I want to insert_at(2) a 4th alert named "a2".

Here is the code:

[#<Alert id: 56428, folder_id: 518, position: 1>,
 #<Alert id: 56442, folder_id: 518, position: 2>,
 #<Alert id: 56526, folder_id: 518, position: 3>]

irb(main):032:0> a2.update_attributes(folder_id: @folder.id)
irb(main):034:0> a2.insert_at 2
   (1.1ms)  BEGIN
  Alert Load (1.4ms)  SELECT `alerts`.* FROM `alerts` WHERE `alerts`.`id` = 56515 LIMIT 1 FOR UPDATE
  Alert Load (1.5ms)  SELECT `alerts`.* FROM `alerts` WHERE `alerts`.`owner_id` = 502 AND `alerts`.`folder_id` = 518 AND (`alerts`.`position` IS NOT NULL) ORDER BY `alerts`.`position` DESC LIMIT 1
   (1.1ms)  UPDATE `alerts` SET `position` = 6, `updated_at` = '2019-03-22 15:13:18', `exclude_domain_extensions` = '--- []\n' WHERE `alerts`.`id` = 56515
   (1.2ms)  SELECT COUNT(*) FROM `alerts` WHERE `alerts`.`owner_id` = 502 AND `alerts`.`folder_id` = 518 AND (`alerts`.`position` = 6)
  SQL (1.2ms)  UPDATE `alerts` SET `position` = (`alerts`.`position` + 1), `updated_at` = '2019-03-22 15:13:18' WHERE `alerts`.`owner_id` = 502 AND `alerts`.`folder_id` = 518 AND (`alerts`.id != 56515) AND (`alerts`.`position` >= 2) AND (`alerts`.`position` < 4)
   (6.8ms)  UPDATE `alerts` SET `position` = 2, `updated_at` = '2019-03-22 15:13:18', `exclude_domain_extensions` = '--- []\n' WHERE `alerts`.`id` = 56515
   (1.1ms)  SELECT COUNT(*) FROM `alerts` WHERE `alerts`.`owner_id` = 502 AND `alerts`.`folder_id` = 518 AND (`alerts`.`position` = 2)
   (10.2ms)  COMMIT
irb(main):037:0> @folder.alerts.select(["alerts.id", :folder_id, :position])
[#<Alert id: 56428, folder_id: 518, position: 1>,
 #<Alert id: 56515, folder_id: 518, position: 2>,
 #<Alert id: 56442, folder_id: 518, position: 2>,
 #<Alert id: 56526, folder_id: 518, position: 4>]

Do you still see something wrong ?

brendon commented 5 years ago

Try a2.update_attributes(folder_id: @folder.id, position: 2) :)

loicginoux commented 5 years ago

thanks, closing it.