From: Jean-Philippe Lang <jp_lang@yahoo.fr>
Date: Thu, 14 Feb 2013 20:37:17 +0000 (+0000)
Subject: Refactor: use an ordered hash to store available filters and remove :order option... 
X-Git-Tag: 2.3.0~143
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=0be82ea2c45eb2d95e5cde32365dea8a5b333123;p=redmine.git

Refactor: use an ordered hash to store available filters and remove :order option (#13154).

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

diff --git a/app/helpers/queries_helper.rb b/app/helpers/queries_helper.rb
index 9c403ab8f..6c8ba1a7f 100644
--- a/app/helpers/queries_helper.rb
+++ b/app/helpers/queries_helper.rb
@@ -24,28 +24,7 @@ module QueriesHelper
 
   def filters_options(query)
     options = [[]]
-    sorted_options = query.available_filters.sort do |a, b|
-      ord = 0
-      if !(a[1][:order] == 20 && b[1][:order] == 20) 
-        ord = a[1][:order] <=> b[1][:order]
-      else
-        cn = (CustomField::CUSTOM_FIELDS_NAMES.index(a[1][:field].class.name) <=>
-                CustomField::CUSTOM_FIELDS_NAMES.index(b[1][:field].class.name))
-        if cn != 0
-          ord = cn
-        else
-          f = (a[1][:field] <=> b[1][:field])
-          if f != 0
-            ord = f
-          else
-            # assigned_to or author 
-            ord = (a[0] <=> b[0])
-          end
-        end
-      end
-      ord
-    end
-    options += sorted_options.map do |field, field_options|
+    options += query.available_filters.map do |field, field_options|
       [field_options[:name], field]
     end
   end
diff --git a/app/models/issue_query.rb b/app/models/issue_query.rb
index 17c6962ff..1959c3ea3 100644
--- a/app/models/issue_query.rb
+++ b/app/models/issue_query.rb
@@ -58,141 +58,127 @@ class IssueQuery < Query
     (project.nil? || user.allowed_to?(:view_issues, project)) && (self.is_public? || self.user_id == user.id)
   end
 
-  def available_filters
-    return @available_filters if @available_filters
-    @available_filters = {
-      "status_id" => {
-        :type => :list_status, :order => 0,
-        :values => IssueStatus.sorted.all.collect{|s| [s.name, s.id.to_s] }
-       },
-      "tracker_id" => {
-        :type => :list, :order => 2, :values => trackers.collect{|s| [s.name, s.id.to_s] }
-       },
-      "priority_id" => {
-        :type => :list, :order => 3, :values => IssuePriority.all.collect{|s| [s.name, s.id.to_s] }
-       },
-      "subject" => { :type => :text, :order => 8 },
-      "created_on" => { :type => :date_past, :order => 9 },
-      "updated_on" => { :type => :date_past, :order => 10 },
-      "start_date" => { :type => :date, :order => 11 },
-      "due_date" => { :type => :date, :order => 12 },
-      "estimated_hours" => { :type => :float, :order => 13 },
-      "done_ratio" =>  { :type => :integer, :order => 14 }
-    }
-    IssueRelation::TYPES.each do |relation_type, options|
-      @available_filters[relation_type] = {
-        :type => :relation, :order => @available_filters.size + 100,
-        :label => options[:name]
-      }
-    end
+  def initialize_available_filters
     principals = []
+    subprojects = []
+    versions = []
+    categories = []
+    issue_custom_fields = []
+    
     if project
       principals += project.principals.sort
       unless project.leaf?
         subprojects = project.descendants.visible.all
-        if subprojects.any?
-          @available_filters["subproject_id"] = {
-            :type => :list_subprojects, :order => 13,
-            :values => subprojects.collect{|s| [s.name, s.id.to_s] }
-          }
-          principals += Principal.member_of(subprojects)
-        end
+        principals += Principal.member_of(subprojects)
       end
+      versions = project.shared_versions.all
+      categories = project.issue_categories.all
+      issue_custom_fields = project.all_issue_custom_fields
     else
       if all_projects.any?
-        # members of visible projects
         principals += Principal.member_of(all_projects)
-        # project filter
-        project_values = []
-        if User.current.logged? && User.current.memberships.any?
-          project_values << ["<< #{l(:label_my_projects).downcase} >>", "mine"]
-        end
-        project_values += all_projects_values
-        @available_filters["project_id"] = {
-          :type => :list, :order => 1, :values => project_values
-        } unless project_values.empty?
       end
+      versions = Version.visible.find_all_by_sharing('system')
+      issue_custom_fields = IssueCustomField.where(:is_filter => true, :is_for_all => true).all
     end
     principals.uniq!
     principals.sort!
     users = principals.select {|p| p.is_a?(User)}
 
-    assigned_to_values = []
-    assigned_to_values << ["<< #{l(:label_me)} >>", "me"] if User.current.logged?
-    assigned_to_values += (Setting.issue_group_assignment? ?
-                              principals : users).collect{|s| [s.name, s.id.to_s] }
-    @available_filters["assigned_to_id"] = {
-      :type => :list_optional, :order => 4, :values => assigned_to_values
-    } unless assigned_to_values.empty?
+
+    add_available_filter "status_id",
+      :type => :list_status, :values => IssueStatus.sorted.all.collect{|s| [s.name, s.id.to_s] }
+
+    if project.nil?
+      project_values = []
+      if User.current.logged? && User.current.memberships.any?
+        project_values << ["<< #{l(:label_my_projects).downcase} >>", "mine"]
+      end
+      project_values += all_projects_values
+      add_available_filter("project_id",
+        :type => :list, :values => project_values
+      ) unless project_values.empty?
+    end
+
+    add_available_filter "tracker_id",
+      :type => :list, :values => trackers.collect{|s| [s.name, s.id.to_s] }
+    add_available_filter "priority_id",
+      :type => :list, :values => IssuePriority.all.collect{|s| [s.name, s.id.to_s] }
 
     author_values = []
     author_values << ["<< #{l(:label_me)} >>", "me"] if User.current.logged?
     author_values += users.collect{|s| [s.name, s.id.to_s] }
-    @available_filters["author_id"] = {
-      :type => :list, :order => 5, :values => author_values
-    } unless author_values.empty?
+    add_available_filter("author_id",
+      :type => :list, :values => author_values
+    ) unless author_values.empty?
+
+    assigned_to_values = []
+    assigned_to_values << ["<< #{l(:label_me)} >>", "me"] if User.current.logged?
+    assigned_to_values += (Setting.issue_group_assignment? ?
+                              principals : users).collect{|s| [s.name, s.id.to_s] }
+    add_available_filter("assigned_to_id",
+      :type => :list_optional, :values => assigned_to_values
+    ) unless assigned_to_values.empty?
 
     group_values = Group.all.collect {|g| [g.name, g.id.to_s] }
-    @available_filters["member_of_group"] = {
-      :type => :list_optional, :order => 6, :values => group_values
-    } unless group_values.empty?
+    add_available_filter("member_of_group",
+      :type => :list_optional, :values => group_values
+    ) unless group_values.empty?
 
     role_values = Role.givable.collect {|r| [r.name, r.id.to_s] }
-    @available_filters["assigned_to_role"] = {
-      :type => :list_optional, :order => 7, :values => role_values
-    } unless role_values.empty?
-
-    if User.current.logged?
-      @available_filters["watcher_id"] = {
-        :type => :list, :order => 15, :values => [["<< #{l(:label_me)} >>", "me"]]
-      }
+    add_available_filter("assigned_to_role",
+      :type => :list_optional, :values => role_values
+    ) unless role_values.empty?
+
+    if versions.any?
+      add_available_filter "fixed_version_id",
+        :type => :list_optional,
+        :values => versions.sort.collect{|s| ["#{s.project.name} - #{s.name}", s.id.to_s] }
     end
 
-    if project
-      # project specific filters
-      categories = project.issue_categories.all
-      unless categories.empty?
-        @available_filters["category_id"] = {
-          :type => :list_optional, :order => 6,
-          :values => categories.collect{|s| [s.name, s.id.to_s] }
-        }
-      end
-      versions = project.shared_versions.all
-      unless versions.empty?
-        @available_filters["fixed_version_id"] = {
-          :type => :list_optional, :order => 7,
-          :values => versions.sort.collect{|s| ["#{s.project.name} - #{s.name}", s.id.to_s] }
-        }
-      end
-      add_custom_fields_filters(project.all_issue_custom_fields)
-    else
-      # global filters for cross project issue list
-      system_shared_versions = Version.visible.find_all_by_sharing('system')
-      unless system_shared_versions.empty?
-        @available_filters["fixed_version_id"] = {
-          :type => :list_optional, :order => 7,
-          :values => system_shared_versions.sort.collect{|s|
-                                       ["#{s.project.name} - #{s.name}", s.id.to_s]
-                                     }
-        }
-      end
-      add_custom_fields_filters(IssueCustomField.where(:is_filter => true, :is_for_all => true).all)
+    if categories.any?
+      add_available_filter "category_id",
+        :type => :list_optional,
+        :values => categories.collect{|s| [s.name, s.id.to_s] }
     end
-    add_associations_custom_fields_filters :project, :author, :assigned_to, :fixed_version
+
+    add_available_filter "subject", :type => :text
+    add_available_filter "created_on", :type => :date_past
+    add_available_filter "updated_on", :type => :date_past
+    add_available_filter "start_date", :type => :date
+    add_available_filter "due_date", :type => :date
+    add_available_filter "estimated_hours", :type => :float
+    add_available_filter "done_ratio", :type => :integer
+
     if User.current.allowed_to?(:set_issues_private, nil, :global => true) ||
       User.current.allowed_to?(:set_own_issues_private, nil, :global => true)
-      @available_filters["is_private"] = {
-        :type => :list, :order => 16,
+      add_available_filter "is_private",
+        :type => :list,
         :values => [[l(:general_text_yes), "1"], [l(:general_text_no), "0"]]
-      }
     end
+
+    if User.current.logged?
+      add_available_filter "watcher_id",
+        :type => :list, :values => [["<< #{l(:label_me)} >>", "me"]]
+    end
+
+    if subprojects.any?
+      add_available_filter "subproject_id",
+        :type => :list_subprojects,
+        :values => subprojects.collect{|s| [s.name, s.id.to_s] }
+    end
+
+    add_custom_fields_filters(issue_custom_fields)
+
+    add_associations_custom_fields_filters :project, :author, :assigned_to, :fixed_version
+
+    IssueRelation::TYPES.each do |relation_type, options|
+      add_available_filter relation_type, :type => :relation, :label => options[:name]
+    end
+
     Tracker.disabled_core_fields(trackers).each {|field|
-      @available_filters.delete field
+      delete_available_filter field
     }
-    @available_filters.each do |field, options|
-      options[:name] ||= l(options[:label] || "field_#{field}".gsub(/_id$/, ''))
-    end
-    @available_filters
   end
 
   def available_columns
diff --git a/app/models/query.rb b/app/models/query.rb
index 6a77508df..cd1b9c386 100644
--- a/app/models/query.rb
+++ b/app/models/query.rb
@@ -278,6 +278,37 @@ class Query < ActiveRecord::Base
     @all_projects_values = values
   end
 
+  # Adds available filters
+  def initialize_available_filters
+    # implemented by sub-classes
+  end
+  protected :initialize_available_filters
+
+  # Adds an available filter
+  def add_available_filter(field, options)
+    @available_filters ||= ActiveSupport::OrderedHash.new
+    @available_filters[field] = options
+    @available_filters
+  end
+
+  # Removes an available filter
+  def delete_available_filter(field)
+    if @available_filters
+      @available_filters.delete(field)
+    end
+  end
+
+  # Return a hash of available filters
+  def available_filters
+    unless @available_filters
+      initialize_available_filters
+      @available_filters.each do |field, options|
+        options[:name] ||= l(options[:label] || "field_#{field}".gsub(/_id$/, ''))
+      end
+    end
+    @available_filters
+  end
+
   def add_filter(field, operator, values=nil)
     # values must be an array
     return unless values.nil? || values.is_a?(Array)
@@ -692,31 +723,30 @@ class Query < ActiveRecord::Base
 
   def add_custom_fields_filters(custom_fields, assoc=nil)
     return unless custom_fields.present?
-    @available_filters ||= {}
 
-    custom_fields.select(&:is_filter?).each do |field|
+    custom_fields.select(&:is_filter?).sort.each do |field|
       case field.field_format
       when "text"
-        options = { :type => :text, :order => 20 }
+        options = { :type => :text }
       when "list"
-        options = { :type => :list_optional, :values => field.possible_values, :order => 20}
+        options = { :type => :list_optional, :values => field.possible_values }
       when "date"
-        options = { :type => :date, :order => 20 }
+        options = { :type => :date }
       when "bool"
-        options = { :type => :list, :values => [[l(:general_text_yes), "1"], [l(:general_text_no), "0"]], :order => 20 }
+        options = { :type => :list, :values => [[l(:general_text_yes), "1"], [l(:general_text_no), "0"]] }
       when "int"
-        options = { :type => :integer, :order => 20 }
+        options = { :type => :integer }
       when "float"
-        options = { :type => :float, :order => 20 }
+        options = { :type => :float }
       when "user", "version"
         next unless project
         values = field.possible_values_options(project)
         if User.current.logged? && field.field_format == 'user'
           values.unshift ["<< #{l(:label_me)} >>", "me"]
         end
-        options = { :type => :list_optional, :values => values, :order => 20}
+        options = { :type => :list_optional, :values => values }
       else
-        options = { :type => :string, :order => 20 }
+        options = { :type => :string }
       end
       filter_id = "cf_#{field.id}"
       filter_name = field.name
@@ -724,7 +754,7 @@ class Query < ActiveRecord::Base
         filter_id = "#{assoc}.#{filter_id}"
         filter_name = l("label_attribute_of_#{assoc}", :name => filter_name)
       end
-      @available_filters[filter_id] = options.merge({
+      add_available_filter filter_id, options.merge({
                :name => filter_name,
                :format => field.field_format,
                :field => field
diff --git a/app/models/time_entry_query.rb b/app/models/time_entry_query.rb
index 7283fbd2f..f0301ce32 100644
--- a/app/models/time_entry_query.rb
+++ b/app/models/time_entry_query.rb
@@ -35,13 +35,8 @@ class TimeEntryQuery < Query
     add_filter('spent_on', '*') unless filters.present?
   end
 
-  def available_filters
-    return @available_filters if @available_filters
-    @available_filters = {
-      "spent_on" => { :type => :date_past, :order => 0 },
-      "comments" => { :type => :text, :order => 5 },
-      "hours" => { :type => :float, :order => 6 }
-    }
+  def initialize_available_filters
+    add_available_filter "spent_on", :type => :date_past
 
     principals = []
     if project
@@ -49,10 +44,9 @@ class TimeEntryQuery < Query
       unless project.leaf?
         subprojects = project.descendants.visible.all
         if subprojects.any?
-          @available_filters["subproject_id"] = {
-            :type => :list_subprojects, :order => 1,
+          add_available_filter "subproject_id",
+            :type => :list_subprojects,
             :values => subprojects.collect{|s| [s.name, s.id.to_s] }
-          }
           principals += Principal.member_of(subprojects)
         end
       end
@@ -66,9 +60,9 @@ class TimeEntryQuery < Query
           project_values << ["<< #{l(:label_my_projects).downcase} >>", "mine"]
         end
         project_values += all_projects_values
-        @available_filters["project_id"] = {
-          :type => :list, :order => 1, :values => project_values
-        } unless project_values.empty?
+        add_available_filter("project_id",
+          :type => :list, :values => project_values
+        ) unless project_values.empty?
       end
     end
     principals.uniq!
@@ -78,22 +72,20 @@ class TimeEntryQuery < Query
     users_values = []
     users_values << ["<< #{l(:label_me)} >>", "me"] if User.current.logged?
     users_values += users.collect{|s| [s.name, s.id.to_s] }
-    @available_filters["user_id"] = {
-      :type => :list_optional, :order => 2, :values => users_values
-    } unless users_values.empty?
+    add_available_filter("user_id",
+      :type => :list_optional, :values => users_values
+    ) unless users_values.empty?
 
     activities = (project ? project.activities : TimeEntryActivity.shared.active)
-    @available_filters["activity_id"] = {
-      :type => :list, :order => 3, :values => activities.map {|a| [a.name, a.id.to_s]}
-    } unless activities.empty?
+    add_available_filter("activity_id",
+      :type => :list, :values => activities.map {|a| [a.name, a.id.to_s]}
+    ) unless activities.empty?
+
+    add_available_filter "comments", :type => :text
+    add_available_filter "hours", :type => :float
 
     add_custom_fields_filters(TimeEntryCustomField.where(:is_filter => true).all)
     add_associations_custom_fields_filters :project, :issue, :user
-
-    @available_filters.each do |field, options|
-      options[:name] ||= l(options[:label] || "field_#{field}".gsub(/_id$/, ''))
-    end
-    @available_filters
   end
 
   def available_columns
diff --git a/test/unit/helpers/queries_helper_test.rb b/test/unit/helpers/queries_helper_test.rb
index d32959efe..81604a38b 100644
--- a/test/unit/helpers/queries_helper_test.rb
+++ b/test/unit/helpers/queries_helper_test.rb
@@ -29,7 +29,7 @@ class QueriesHelperTest < ActionView::TestCase
            :projects_trackers,
            :custom_fields_trackers
 
-  def test_order
+  def test_filters_options_should_be_ordered
     User.current = User.find_by_login('admin')
     query = IssueQuery.new(:project => nil, :name => '_')
     assert_equal 30, query.available_filters.size
@@ -40,17 +40,16 @@ class QueriesHelperTest < ActionView::TestCase
     assert_equal "project_id", fo[2][1]
     assert_equal "tracker_id", fo[3][1]
     assert_equal "priority_id", fo[4][1]
-    assert_equal "watcher_id", fo[17][1]
-    assert_equal "is_private", fo[18][1]
+    assert_equal "is_private", fo[17][1]
+    assert_equal "watcher_id", fo[18][1]
   end
 
-  def test_order_custom_fields
+  def test_filters_options_should_be_ordered_with_custom_fields
     set_language_if_valid 'en'
-    field = UserCustomField.new(
+    field = UserCustomField.create!(
               :name => 'order test', :field_format => 'string',
               :is_for_all => true, :is_filter => true
             )
-    assert field.save
     User.current = User.find_by_login('admin')
     query = IssueQuery.new(:project => nil, :name => '_')
     assert_equal 32, query.available_filters.size
@@ -59,7 +58,7 @@ class QueriesHelperTest < ActionView::TestCase
     assert_equal "Searchable field", fo[19][0]
     assert_equal "Database", fo[20][0]
     assert_equal "Project's Development status", fo[21][0]
-    assert_equal "Assignee's order test", fo[22][0]
-    assert_equal "Author's order test", fo[23][0]
+    assert_equal "Author's order test", fo[22][0]
+    assert_equal "Assignee's order test", fo[23][0]
   end
 end
diff --git a/test/unit/query_test.rb b/test/unit/query_test.rb
index dfc291d7e..0ee76a477 100644
--- a/test/unit/query_test.rb
+++ b/test/unit/query_test.rb
@@ -28,6 +28,11 @@ class QueryTest < ActiveSupport::TestCase
            :projects_trackers,
            :custom_fields_trackers
 
+  def test_available_filters_should_be_ordered
+    query = IssueQuery.new
+    assert_equal 0, query.available_filters.keys.index('status_id')
+  end
+
   def test_custom_fields_for_all_projects_should_be_available_in_global_queries
     query = IssueQuery.new(:project => nil, :name => '_')
     assert query.available_filters.has_key?('cf_1')