Replaces acts_as_list with an implementation that handles #position= (#12909).

Objects are reordered using the regular attribute writer #position= and AR callbacks.

git-svn-id: http://svn.redmine.org/redmine/trunk@15335 e93f8b46-1217-0410-a6f0-8f06a7374b81

18 files changed, 301 insertions, 48 deletions

diff --git a/app/models/board.rb b/app/models/board.rb
index d62f863c4..104c85749 100644
--- a/app/models/board.rb
+++ b/app/models/board.rb
@@ -21,7 +21,7 @@ class Board < ActiveRecord::Base
   has_many :messages, lambda {order("#{Message.table_name}.created_on DESC")}, :dependent => :destroy
   belongs_to :last_message, :class_name => 'Message'
   acts_as_tree :dependent => :nullify
-  acts_as_list :scope => '(project_id = #{project_id} AND parent_id #{parent_id ? "= #{parent_id}" : "IS NULL"})'
+  acts_as_positioned :scope => [:project_id, :parent_id]
   acts_as_watchable
 
   validates_presence_of :name, :description
diff --git a/app/models/custom_field.rb b/app/models/custom_field.rb
index f5f15a4a1..511299523 100644
--- a/app/models/custom_field.rb
+++ b/app/models/custom_field.rb
@@ -24,7 +24,7 @@ class CustomField < ActiveRecord::Base
            :dependent => :delete_all
   has_many :custom_values, :dependent => :delete_all
   has_and_belongs_to_many :roles, :join_table => "#{table_name_prefix}custom_fields_roles#{table_name_suffix}", :foreign_key => "custom_field_id"
-  acts_as_list :scope => 'type = \'#{self.class}\''
+  acts_as_positioned
   serialize :possible_values
   store :format_store
 
diff --git a/app/models/enumeration.rb b/app/models/enumeration.rb
index eac403452..fc8486174 100644
--- a/app/models/enumeration.rb
+++ b/app/models/enumeration.rb
@@ -22,7 +22,7 @@ class Enumeration < ActiveRecord::Base
 
   belongs_to :project
 
-  acts_as_list :scope => 'type = \'#{type}\' AND #{parent_id ? "parent_id = #{parent_id}" : "parent_id IS NULL"}'
+  acts_as_positioned :scope => :parent_id
   acts_as_customizable
   acts_as_tree
 
@@ -129,33 +129,38 @@ class Enumeration < ActiveRecord::Base
     return new == previous
   end
 
-  # Overrides acts_as_list reset_positions_in_list so that enumeration overrides
-  # get the same position as the overriden enumeration
-  def reset_positions_in_list
-    acts_as_list_class.where(scope_condition).reorder("#{position_column} ASC, id ASC").each_with_index do |item, i|
-      acts_as_list_class.where("id = :id OR parent_id = :id", :id => item.id).
-        update_all({position_column => (i + 1)})
-    end
-  end
+  private
 
-private
   def check_integrity
     raise "Cannot delete enumeration" if self.in_use?
   end
 
-  # Overrides acts_as_list add_to_list_bottom so that enumeration overrides
+  # Overrides Redmine::Acts::Positioned#set_default_position so that enumeration overrides
   # get the same position as the overriden enumeration
-  def add_to_list_bottom
-    if parent
-      self[position_column] = parent.position
-    else
-      super
+  def set_default_position
+    if position.nil? && parent
+      self.position = parent.position
+    end
+    super
+  end
+
+  # Overrides Redmine::Acts::Positioned#update_position so that overrides get the same
+  # position as the overriden enumeration
+  def update_position
+    super
+    if position_changed?
+      self.class.where.not(:parent_id => nil).update_all(
+        "position = coalesce((
+          select position
+          from (select id, position from enumerations) as parent
+          where parent_id = parent.id), 1)"
+      )
     end
   end
 
-  # Overrides acts_as_list remove_from_list so that enumeration overrides
+  # Overrides Redmine::Acts::Positioned#remove_position so that enumeration overrides
   # get the same position as the overriden enumeration
-  def remove_from_list
+  def remove_position
     if parent_id.blank?
       super
     end
diff --git a/app/models/issue_status.rb b/app/models/issue_status.rb
index 7855f5b92..31c0f031c 100644
--- a/app/models/issue_status.rb
+++ b/app/models/issue_status.rb
@@ -19,7 +19,7 @@ class IssueStatus < ActiveRecord::Base
   before_destroy :check_integrity
   has_many :workflows, :class_name => 'WorkflowTransition', :foreign_key => "old_status_id"
   has_many :workflow_transitions_as_new_status, :class_name => 'WorkflowTransition', :foreign_key => "new_status_id"
-  acts_as_list
+  acts_as_positioned
 
   after_update :handle_is_closed_change
   before_destroy :delete_workflow_rules
diff --git a/app/models/role.rb b/app/models/role.rb
index b7d048e2a..defbc311d 100644
--- a/app/models/role.rb
+++ b/app/models/role.rb
@@ -70,7 +70,7 @@ class Role < ActiveRecord::Base
 
   has_many :member_roles, :dependent => :destroy
   has_many :members, :through => :member_roles
-  acts_as_list
+  acts_as_positioned :scope => :builtin
 
   serialize :permissions, ::Role::PermissionsAttributeCoder
   attr_protected :builtin
@@ -223,10 +223,10 @@ private
   def self.find_or_create_system_role(builtin, name)
     role = where(:builtin => builtin).first
     if role.nil?
-      role = create(:name => name, :position => 0) do |r|
+      role = create(:name => name) do |r|
         r.builtin = builtin
       end
-      raise "Unable to create the #{name} role." if role.new_record?
+      raise "Unable to create the #{name} role (#{role.errors.full_messages.join(',')})." if role.new_record?
     end
     role
   end
diff --git a/app/models/tracker.rb b/app/models/tracker.rb
index dd802efdf..73cf569fc 100644
--- a/app/models/tracker.rb
+++ b/app/models/tracker.rb
@@ -34,7 +34,7 @@ class Tracker < ActiveRecord::Base
 
   has_and_belongs_to_many :projects
   has_and_belongs_to_many :custom_fields, :class_name => 'IssueCustomField', :join_table => "#{table_name_prefix}custom_fields_trackers#{table_name_suffix}", :association_foreign_key => 'custom_field_id'
-  acts_as_list
+  acts_as_positioned
 
   attr_protected :fields_bits
 
diff --git a/db/migrate/20160416072926_remove_position_defaults.rb b/db/migrate/20160416072926_remove_position_defaults.rb
new file mode 100644
index 000000000..ab3af1499
--- /dev/null
+++ b/db/migrate/20160416072926_remove_position_defaults.rb
@@ -0,0 +1,13 @@
+class RemovePositionDefaults < ActiveRecord::Migration
+  def up
+    [Board, CustomField, Enumeration, IssueStatus, Role, Tracker].each do |klass|
+      change_column klass.table_name, :position, :integer, :default => nil
+    end
+  end
+
+  def down
+    [Board, CustomField, Enumeration, IssueStatus, Role, Tracker].each do |klass|
+      change_column klass.table_name, :position, :integer, :default => 1
+    end
+  end
+end
diff --git a/lib/redmine.rb b/lib/redmine.rb
index 6d0d772ce..df234ebbd 100644
--- a/lib/redmine.rb
+++ b/lib/redmine.rb
@@ -28,6 +28,8 @@ rescue LoadError
   # Redcarpet is not available
 end
 
+require 'redmine/acts/positioned'
+
 require 'redmine/scm/base'
 require 'redmine/access_control'
 require 'redmine/access_keys'
diff --git a/lib/redmine/acts/positioned.rb b/lib/redmine/acts/positioned.rb
new file mode 100644
index 000000000..a322f1346
--- /dev/null
+++ b/lib/redmine/acts/positioned.rb
@@ -0,0 +1,134 @@
+# Redmine - project management software
+# Copyright (C) 2006-2016  Jean-Philippe Lang
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+
+module Redmine
+  module Acts
+    module Positioned
+      def self.included(base)
+        base.extend ClassMethods
+      end
+
+      # This extension provides the capabilities for reordering objects in a list.
+      # The class needs to have a +position+ column defined as an integer on the
+      # mapped database table.
+      module ClassMethods
+        # Configuration options are:
+        #
+        # * +scope+ - restricts what is to be considered a list. Must be a symbol
+        # or an array of symbols
+        def acts_as_positioned(options = {})
+          class_attribute :positioned_options
+          self.positioned_options = {:scope => Array(options[:scope])}
+
+          send :include, Redmine::Acts::Positioned::InstanceMethods
+
+          before_save :set_default_position
+          after_save :update_position
+          after_destroy :remove_position
+        end
+      end
+
+      module InstanceMethods
+        def self.included(base)
+          base.extend ClassMethods
+        end
+
+        # Move to the given position
+        # For compatibility with the previous way of sorting items
+        def move_to=(pos)
+          case pos.to_s
+          when 'highest'
+            self.position = 1
+          when 'higher'
+            self.position -= 1 if position > 1
+          when 'lower'
+            self.position += 1
+          when 'lowest'
+            self.position = nil
+            set_default_position
+          end
+        end
+
+        private
+
+        def position_scope
+          build_position_scope {|c| send(c)}
+        end
+
+        def position_scope_was
+          build_position_scope {|c| send("#{c}_was")}
+        end
+
+        def build_position_scope
+          condition_hash = self.class.positioned_options[:scope].inject({}) do |h, column|
+            h[column] = yield(column)
+            h
+          end
+          self.class.where(condition_hash)
+        end
+
+        def set_default_position
+          if position.nil?
+            self.position = position_scope.maximum(:position).to_i + (new_record? ? 1 : 0)
+          end
+        end
+
+        def update_position
+          if !new_record? && position_scope_changed?
+            remove_position
+            insert_position
+          elsif position_changed?
+            if position_was.nil?
+              insert_position
+            else
+              shift_positions
+            end
+          end
+        end
+
+        def insert_position
+          position_scope.where("position >= ? AND id <> ?", position, id).update_all("position = position + 1")
+        end
+
+        def remove_position
+          position_scope_was.where("position >= ? AND id <> ?", position_was, id).update_all("position = position - 1")
+        end
+
+        def position_scope_changed?
+          (changed & self.class.positioned_options[:scope].map(&:to_s)).any?
+        end
+
+        def shift_positions
+          offset = position_was <=> position
+          min, max = [position, position_was].sort
+          r = position_scope.where("id <> ? AND position BETWEEN ? AND ?", id, min, max).update_all("position = position + #{offset}")
+          if r != max - min
+            reset_positions_in_list
+          end
+        end
+
+        def reset_positions_in_list
+          position_scope.reorder(:position, :id).pluck(:id).each_with_index do |record_id, p|
+            self.class.where(:id => record_id).update_all(:position => p+1)
+          end
+        end
+      end
+    end
+  end
+end
+
+ActiveRecord::Base.send :include, Redmine::Acts::Positioned
diff --git a/test/fixtures/custom_fields.yml b/test/fixtures/custom_fields.yml
index 510e3a41c..98dccdfcb 100644
--- a/test/fixtures/custom_fields.yml
+++ b/test/fixtures/custom_fields.yml
@@ -145,3 +145,4 @@ custom_fields_011:
     SGXDqWzDp2prc2Tigqw2NTTDuQ==
   - Other value
   field_format: list
+  position: 1
diff --git a/test/fixtures/enumerations.yml b/test/fixtures/enumerations.yml
index 23ea2b3aa..199caeace 100644
--- a/test/fixtures/enumerations.yml
+++ b/test/fixtures/enumerations.yml
@@ -78,11 +78,13 @@ enumerations_012:
   type: Enumeration
   is_default: true
   active: true
+  position: 1
 enumerations_013:
   name: Another Enumeration
   id: 13
   type: Enumeration
   active: true
+  position: 2
 enumerations_014: 
   name: Inactive Activity
   id: 14
diff --git a/test/fixtures/roles.yml b/test/fixtures/roles.yml
index 2e1b7c660..0ed7aa13b 100644
--- a/test/fixtures/roles.yml
+++ b/test/fixtures/roles.yml
@@ -182,7 +182,7 @@ roles_004:
     - :browse_repository
     - :view_changesets
 
-  position: 4
+  position: 1
 roles_005: 
   name: Anonymous
   id: 5
@@ -203,5 +203,5 @@ roles_005:
     - :browse_repository
     - :view_changesets
 
-  position: 5
+  position: 1
 
diff --git a/test/functional/roles_controller_test.rb b/test/functional/roles_controller_test.rb
index 03cd91ab2..d86943966 100644
--- a/test/functional/roles_controller_test.rb
+++ b/test/functional/roles_controller_test.rb
@@ -203,6 +203,6 @@ class RolesControllerTest < ActionController::TestCase
   def test_move_lowest
     put :update, :id => 2, :role => {:move_to => 'lowest'}
     assert_redirected_to '/roles'
-    assert_equal Role.count, Role.find(2).position
+    assert_equal Role.givable.count, Role.find(2).position
   end
 end
diff --git a/test/object_helpers.rb b/test/object_helpers.rb
index 66334f63a..2b3327961 100644
--- a/test/object_helpers.rb
+++ b/test/object_helpers.rb
@@ -60,13 +60,18 @@ module ObjectHelpers
     status
   end
 
-  def Tracker.generate!(attributes={})
+  def Tracker.generate(attributes={})
     @generated_tracker_name ||= 'Tracker 0'
     @generated_tracker_name.succ!
     tracker = Tracker.new(attributes)
     tracker.name = @generated_tracker_name.dup if tracker.name.blank?
     tracker.default_status ||= IssueStatus.order('position').first || IssueStatus.generate!
     yield tracker if block_given?
+    tracker
+  end
+
+  def Tracker.generate!(attributes={}, &block)
+    tracker = Tracker.generate(attributes, &block)
     tracker.save!
     tracker
   end
diff --git a/test/unit/enumeration_test.rb b/test/unit/enumeration_test.rb
index 3695e144d..ba64440de 100644
--- a/test/unit/enumeration_test.rb
+++ b/test/unit/enumeration_test.rb
@@ -155,6 +155,7 @@ class EnumerationTest < ActiveSupport::TestCase
     b = IssuePriority.create!(:name => 'B')
     override = IssuePriority.create!(:name => 'BB', :parent_id => b.id)
     b.move_to = 'higher'
+    b.save!
 
     assert_equal [2, 1, 1], [a, b, override].map(&:reload).map(&:position)
   end
diff --git a/test/unit/issue_priority_test.rb b/test/unit/issue_priority_test.rb
index b4aa0852b..bc4202d74 100644
--- a/test/unit/issue_priority_test.rb
+++ b/test/unit/issue_priority_test.rb
@@ -55,25 +55,6 @@ class IssuePriorityTest < ActiveSupport::TestCase
     assert_equal [1, 2, 3], priorities.map(&:position)
   end
 
-  def test_reset_positions_in_list_should_set_sequential_positions
-    IssuePriority.delete_all
-
-    priorities = [1, 2, 3].map {|i| IssuePriority.create!(:name => "P#{i}")}
-    priorities[0].update_attribute :position, 4
-    priorities[1].update_attribute :position, 2
-    priorities[2].update_attribute :position, 7
-    assert_equal [4, 2, 7], priorities.map(&:reload).map(&:position)
-
-    priorities[0].reset_positions_in_list
-    assert_equal [2, 1, 3], priorities.map(&:reload).map(&:position)
-  end
-
-  def test_moving_in_list_should_reset_positions
-    priority = IssuePriority.first
-    priority.expects(:reset_positions_in_list).once
-    priority.move_to = 'higher'
-  end
-
   def test_clear_position_names_should_set_position_names_to_nil
     IssuePriority.clear_position_names
     assert IssuePriority.all.all? {|priority| priority.position_name.nil?}
@@ -102,6 +83,7 @@ class IssuePriorityTest < ActiveSupport::TestCase
   def test_moving_a_priority_should_update_position_names
     prio = IssuePriority.first
     prio.move_to = 'lowest'
+    prio.save!
     prio.reload
     assert_equal 'highest', prio.position_name
   end
diff --git a/test/unit/lib/redmine/acts/positioned_with_scope_test.rb b/test/unit/lib/redmine/acts/positioned_with_scope_test.rb
new file mode 100644
index 000000000..68db9c723
--- /dev/null
+++ b/test/unit/lib/redmine/acts/positioned_with_scope_test.rb
@@ -0,0 +1,53 @@
+# Redmine - project management software
+# Copyright (C) 2006-2016  Jean-Philippe Lang
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+
+require File.expand_path('../../../../../test_helper', __FILE__)
+
+class Redmine::Acts::PositionedWithScopeTest < ActiveSupport::TestCase
+  fixtures :projects, :boards
+
+  def test_create_should_default_to_last_position
+    b = Board.generate!(:project_id => 1)
+    assert_equal 3, b.reload.position
+
+    b = Board.generate!(:project_id => 3)
+    assert_equal 1, b.reload.position
+  end
+
+  def test_create_should_insert_at_given_position
+    b = Board.generate!(:project_id => 1, :position => 2)
+
+    assert_equal 2, b.reload.position
+    assert_equal [1, 3, 1, 2], Board.order(:id).pluck(:position)
+  end
+
+  def test_destroy_should_remove_position
+    b = Board.generate!(:project_id => 1, :position => 2)
+    b.destroy
+
+    assert_equal [1, 2, 1], Board.order(:id).pluck(:position)
+  end
+
+  def test_update_should_update_positions
+    b = Board.generate!(:project_id => 1)
+    assert_equal 3, b.position
+
+    b.position = 2
+    b.save!
+    assert_equal [1, 3, 1, 2], Board.order(:id).pluck(:position)
+  end
+end
diff --git a/test/unit/lib/redmine/acts/positioned_without_scope_test.rb b/test/unit/lib/redmine/acts/positioned_without_scope_test.rb
new file mode 100644
index 000000000..40c0147bd
--- /dev/null
+++ b/test/unit/lib/redmine/acts/positioned_without_scope_test.rb
@@ -0,0 +1,55 @@
+# Redmine - project management software
+# Copyright (C) 2006-2016  Jean-Philippe Lang
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+
+require File.expand_path('../../../../../test_helper', __FILE__)
+
+class Redmine::Acts::PositionedWithoutScopeTest < ActiveSupport::TestCase
+  fixtures :trackers, :issue_statuses
+
+  def test_create_should_default_to_last_position
+    t = Tracker.generate
+    t.save!
+
+    assert_equal 4, t.reload.position
+  end
+
+  def test_create_should_insert_at_given_position
+    t = Tracker.generate
+    t.position = 2
+    t.save!
+
+    assert_equal 2, t.reload.position
+    assert_equal [1, 3, 4, 2], Tracker.order(:id).pluck(:position)
+  end
+
+  def test_destroy_should_remove_position
+    t = Tracker.generate!
+    Tracker.generate!
+    t.destroy
+
+    assert_equal [1, 2, 3, 4], Tracker.order(:id).pluck(:position)
+  end
+
+  def test_update_should_update_positions
+    t = Tracker.generate!
+    assert_equal 4, t.position
+
+    t.position = 2
+    t.save!
+    assert_equal [1, 3, 4, 2], Tracker.order(:id).pluck(:position)
+  end
+end