Code cleanup.

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@9715 e93f8b46-1217-0410-a6f0-8f06a7374b81

3 files changed, 34 insertions, 9 deletions

diff --git a/app/models/role.rb b/app/models/role.rb
index ee59ce905..c8a1028da 100644
--- a/app/models/role.rb
+++ b/app/models/role.rb
@@ -26,11 +26,11 @@ class Role < ActiveRecord::Base
     ['own', :label_issues_visibility_own]
   ]
 
-  scope :sorted, {:order => 'builtin, position'}
-  scope :givable, { :conditions => "builtin = 0", :order => 'position' }
+  scope :sorted, order("#{table_name}.builtin ASC, #{table_name}.position ASC")
+  scope :givable, order("#{table_name}.position ASC").where(:builtin => 0)
   scope :builtin, lambda { |*args|
-    compare = 'not' if args.first == true
-    { :conditions => "#{compare} builtin = 0" }
+    compare = (args.first == true ? 'not' : '')
+    where("#{compare} builtin = 0")
   }
 
   before_destroy :check_deletable
@@ -87,7 +87,15 @@ class Role < ActiveRecord::Base
   end
 
   def <=>(role)
-    role ? position <=> role.position : -1
+    if role
+      if builtin == role.builtin
+        position <=> role.position
+      else
+        builtin <=> role.builtin
+      end
+    else
+      -1
+    end
   end
 
   def to_s
@@ -134,7 +142,7 @@ class Role < ActiveRecord::Base
 
   # Find all the roles that can be given to a project member
   def self.find_all_givable
-    find(:all, :conditions => {:builtin => 0}, :order => 'position')
+    Role.givable.all
   end
 
   # Return the builtin 'non member' role.  If the role doesn't exist,
@@ -165,7 +173,7 @@ private
   end
 
   def self.find_or_create_system_role(builtin, name)
-    role = first(:conditions => {:builtin => builtin})
+    role = where(:builtin => builtin).first
     if role.nil?
       role = create(:name => name, :position => 0) do |r|
         r.builtin = builtin
diff --git a/app/models/workflow.rb b/app/models/workflow.rb
index 5c1ef5dea..36b4c7df8 100644
--- a/app/models/workflow.rb
+++ b/app/models/workflow.rb
@@ -25,8 +25,8 @@ class Workflow < ActiveRecord::Base
   # Returns workflow transitions count by tracker and role
   def self.count_by_tracker_and_role
     counts = connection.select_all("SELECT role_id, tracker_id, count(id) AS c FROM #{Workflow.table_name} GROUP BY role_id, tracker_id")
-    roles = Role.find(:all, :order => 'builtin, position')
-    trackers = Tracker.find(:all, :order => 'position')
+    roles = Role.sorted.all
+    trackers = Tracker.sorted.all
 
     result = []
     trackers.each do |tracker|
diff --git a/test/unit/role_test.rb b/test/unit/role_test.rb
index 1c2cdcfe7..2ef20f4d5 100644
--- a/test/unit/role_test.rb
+++ b/test/unit/role_test.rb
@@ -20,6 +20,19 @@ require File.expand_path('../../test_helper', __FILE__)
 class RoleTest < ActiveSupport::TestCase
   fixtures :roles, :workflows
 
+  def test_sorted_scope
+    assert_equal Role.all.sort, Role.sorted.all
+  end
+
+  def test_givable_scope
+    assert_equal Role.all.reject(&:builtin?).sort, Role.givable.all
+  end
+
+  def test_builtin_scope
+    assert_equal Role.all.select(&:builtin?).sort, Role.builtin(true).all.sort
+    assert_equal Role.all.reject(&:builtin?).sort, Role.builtin(false).all.sort
+  end
+
   def test_copy_workflows
     source = Role.find(1)
     assert_equal 90, source.workflows.size
@@ -57,6 +70,10 @@ class RoleTest < ActiveSupport::TestCase
     assert_equal 'Non membre', Role.non_member.name
   end
 
+  def test_find_all_givable
+    assert_equal Role.all.reject(&:builtin?).sort, Role.find_all_givable
+  end
+
   context "#anonymous" do
     should "return the anonymous role" do
       role = Role.anonymous