Refactor: makes issue id a regular QueryColumn.

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

9 files changed, 48 insertions, 27 deletions

diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb
index 73ea79cb7..a0c48e20d 100644
--- a/app/helpers/issues_helper.rb
+++ b/app/helpers/issues_helper.rb
@@ -382,10 +382,10 @@ module IssuesHelper
 
     export = FCSV.generate(:col_sep => l(:general_csv_separator)) do |csv|
       # csv header fields
-      csv << [ "#" ] + columns.collect {|c| Redmine::CodesetUtil.from_utf8(c.caption.to_s, encoding) }
+      csv << columns.collect {|c| Redmine::CodesetUtil.from_utf8(c.caption.to_s, encoding) }
       # csv lines
       issues.each do |issue|
-        csv << [ issue.id.to_s ] + columns.collect {|c| Redmine::CodesetUtil.from_utf8(csv_content(c, issue), encoding) }
+        csv << columns.collect {|c| Redmine::CodesetUtil.from_utf8(csv_content(c, issue), encoding) }
       end
     end
     export
diff --git a/app/helpers/queries_helper.rb b/app/helpers/queries_helper.rb
index d140ab3a3..80bceaf10 100644
--- a/app/helpers/queries_helper.rb
+++ b/app/helpers/queries_helper.rb
@@ -67,7 +67,9 @@ module QueriesHelper
     when 'Date'
       format_date(value)
     when 'Fixnum'
-      if column.name == :done_ratio
+      if column.name == :id
+        link_to value, issue_path(issue)
+      elsif column.name == :done_ratio
         progress_bar(value, :width => '80px')
       else
         value.to_s
diff --git a/app/models/issue_query.rb b/app/models/issue_query.rb
index 3ca31db9a..5b818cc28 100644
--- a/app/models/issue_query.rb
+++ b/app/models/issue_query.rb
@@ -20,6 +20,7 @@ class IssueQuery < Query
   self.queried_class = Issue
 
   self.available_columns = [
+    QueryColumn.new(:id, :sortable => "#{Issue.table_name}.id", :default_order => 'desc', :caption => '#', :frozen => true),
     QueryColumn.new(:project, :sortable => "#{Project.table_name}.name", :groupable => true),
     QueryColumn.new(:tracker, :sortable => "#{Tracker.table_name}.position", :groupable => true),
     QueryColumn.new(:parent, :sortable => ["#{Issue.table_name}.root_id", "#{Issue.table_name}.lft ASC"], :default_order => 'desc', :caption => :field_parent_issue),
@@ -216,10 +217,6 @@ class IssueQuery < Query
     @available_columns
   end
 
-  def sortable_columns
-    {'id' => "#{Issue.table_name}.id"}.merge(super)
-  end
-
   def default_columns_names
     @default_columns_names ||= begin
       default_columns = Setting.issue_list_default_columns.map(&:to_sym)
diff --git a/app/models/query.rb b/app/models/query.rb
index cd1b9c386..db6ca07a5 100644
--- a/app/models/query.rb
+++ b/app/models/query.rb
@@ -28,11 +28,12 @@ class QueryColumn
     end
     self.default_order = options[:default_order]
     @inline = options.key?(:inline) ? options[:inline] : true
-    @caption_key = options[:caption] || "field_#{name}"
+    @caption_key = options[:caption] || "field_#{name}".to_sym
+    @frozen = options[:frozen]
   end
 
   def caption
-    l(@caption_key)
+    @caption_key.is_a?(Symbol) ? l(@caption_key) : @caption_key
   end
 
   # Returns true if the column is sortable, otherwise false
@@ -48,6 +49,10 @@ class QueryColumn
     @inline
   end
 
+  def frozen?
+    @frozen
+  end
+
   def value(object)
     object.send name
   end
@@ -382,9 +387,10 @@ class Query < ActiveRecord::Base
 
   def columns
     # preserve the column_names order
-    (has_default_columns? ? default_columns_names : column_names).collect do |name|
+    cols = (has_default_columns? ? default_columns_names : column_names).collect do |name|
        available_columns.find { |col| col.name == name }
     end.compact
+    available_columns.select(&:frozen?) | cols
   end
 
   def inline_columns
diff --git a/app/views/issues/_list.html.erb b/app/views/issues/_list.html.erb
index f3e8cae2c..17bf99623 100644
--- a/app/views/issues/_list.html.erb
+++ b/app/views/issues/_list.html.erb
@@ -9,7 +9,6 @@
                               :onclick => 'toggleIssuesSelection(this); return false;',
                               :title => "#{l(:button_check_all)}/#{l(:button_uncheck_all)}" %>
       </th>
-      <%= sort_header_tag('id', :caption => '#', :default_order => 'desc') %>
       <% query.inline_columns.each do |column| %>
         <%= column_header(column) %>
       <% end %>
@@ -32,13 +31,12 @@
   <% end %>
   <tr id="issue-<%= issue.id %>" class="hascontextmenu <%= cycle('odd', 'even') %> <%= issue.css_classes %> <%= level > 0 ? "idnt idnt-#{level}" : nil %>">
     <td class="checkbox hide-when-print"><%= check_box_tag("ids[]", issue.id, false, :id => nil) %></td>
-    <td class="id"><%= link_to issue.id, issue_path(issue) %></td>
     <%= raw query.inline_columns.map {|column| "<td class=\"#{column.css_classes}\">#{column_content(column, issue)}</td>"}.join %>
   </tr>
   <% @query.block_columns.each do |column|
        if (text = column_content(column, issue)) && text.present? -%>
   <tr class="<%= current_cycle %>">
-    <td colspan="<%= @query.inline_columns.size + 2 %>" class="<%= column.css_classes %>"><%= text %></td>
+    <td colspan="<%= @query.inline_columns.size + 1 %>" class="<%= column.css_classes %>"><%= text %></td>
   </tr>
   <% end -%>
   <% end -%>
diff --git a/app/views/queries/_columns.html.erb b/app/views/queries/_columns.html.erb
index 78d3c5703..329ffd67c 100644
--- a/app/views/queries/_columns.html.erb
+++ b/app/views/queries/_columns.html.erb
@@ -4,7 +4,7 @@
       <%= label_tag "available_columns", l(:description_available_columns) %>
       <br />
       <%= select_tag 'available_columns',
-              options_for_select((query.available_inline_columns - query.columns).collect {|column| [column.caption, column.name]}),
+              options_for_select((query.available_inline_columns - query.columns).reject(&:frozen?).collect {|column| [column.caption, column.name]}),
               :multiple => true, :size => 10, :style => "width:150px",
               :ondblclick => "moveOptions(this.form.available_columns, this.form.selected_columns);" %>
     </td>
@@ -18,7 +18,7 @@
       <%= label_tag "selected_columns", l(:description_selected_columns) %>
       <br />
       <%= select_tag((defined?(tag_name) ? tag_name : 'c[]'),
-              options_for_select(query.inline_columns.collect {|column| [column.caption, column.name]}),
+              options_for_select((query.inline_columns & query.available_inline_columns).reject(&:frozen?).collect {|column| [column.caption, column.name]}),
               :id => 'selected_columns', :multiple => true, :size => 10, :style => "width:150px",
               :ondblclick => "moveOptions(this.form.selected_columns, this.form.available_columns);") %>
     </td>
diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb
index 65e28b570..df3c2e330 100644
--- a/test/functional/issues_controller_test.rb
+++ b/test/functional/issues_controller_test.rb
@@ -380,7 +380,7 @@ class IssuesControllerTest < ActionController::TestCase
     assert_equal 'text/csv; header=present', @response.content_type
     assert @response.body.starts_with?("#,")
     lines = @response.body.chomp.split("\n")
-    assert_equal assigns(:query).columns.size + 1, lines[0].split(',').size
+    assert_equal assigns(:query).columns.size, lines[0].split(',').size
   end
 
   def test_index_csv_with_project
@@ -397,7 +397,7 @@ class IssuesControllerTest < ActionController::TestCase
     assert_equal 'text/csv; header=present', @response.content_type
     assert @response.body.starts_with?("#,")
     lines = @response.body.chomp.split("\n")
-    assert_equal assigns(:query).columns.size + 2, lines[0].split(',').size
+    assert_equal assigns(:query).columns.size + 1, lines[0].split(',').size
   end
 
   def test_index_csv_with_spent_time_column
@@ -416,9 +416,9 @@ class IssuesControllerTest < ActionController::TestCase
     assert_response :success
     assert_not_nil assigns(:issues)
     assert_equal 'text/csv; header=present', @response.content_type
-    assert @response.body.starts_with?("#,")
-    lines = @response.body.chomp.split("\n")
-    assert_equal assigns(:query).available_inline_columns.size + 1, lines[0].split(',').size
+    assert_match /\A#,/, response.body
+    lines = response.body.chomp.split("\n")
+    assert_equal assigns(:query).available_inline_columns.size, lines[0].split(',').size
   end
 
   def test_index_csv_with_multi_column_field
@@ -708,12 +708,12 @@ class IssuesControllerTest < ActionController::TestCase
     # query should use specified columns
     query = assigns(:query)
     assert_kind_of IssueQuery, query
-    assert_equal [:project, :tracker, :subject, :assigned_to], query.columns.map(&:name)
+    assert_equal [:id, :project, :tracker, :subject, :assigned_to], query.columns.map(&:name)
   end
 
   def test_index_without_project_and_explicit_default_columns_should_not_add_project_column
     Setting.issue_list_default_columns = ['tracker', 'subject', 'assigned_to']
-    columns = ['tracker', 'subject', 'assigned_to']
+    columns = ['id', 'tracker', 'subject', 'assigned_to']
     get :index, :set_filter => 1, :c => columns
 
     # query should use specified columns
diff --git a/test/functional/queries_controller_test.rb b/test/functional/queries_controller_test.rb
index 8a4e49182..a232fec2e 100644
--- a/test/functional/queries_controller_test.rb
+++ b/test/functional/queries_controller_test.rb
@@ -114,7 +114,7 @@ class QueriesControllerTest < ActionController::TestCase
     assert_redirected_to :controller => 'issues', :action => 'index', :project_id => nil, :query_id => q
     assert !q.is_public?
     assert !q.has_default_columns?
-    assert_equal [:tracker, :subject, :priority, :category], q.columns.collect {|c| c.name}
+    assert_equal [:id, :tracker, :subject, :priority, :category], q.columns.collect {|c| c.name}
     assert q.valid?
   end
 
diff --git a/test/unit/query_test.rb b/test/unit/query_test.rb
index 0ee76a477..bf194155e 100644
--- a/test/unit/query_test.rb
+++ b/test/unit/query_test.rb
@@ -750,16 +750,34 @@ class QueryTest < ActiveSupport::TestCase
   def test_set_column_names
     q = IssueQuery.new
     q.column_names = ['tracker', :subject, '', 'unknonw_column']
-    assert_equal [:tracker, :subject], q.columns.collect {|c| c.name}
-    c = q.columns.first
-    assert q.has_column?(c)
+    assert_equal [:id, :tracker, :subject], q.columns.collect {|c| c.name}
+  end
+
+  def test_has_column_should_accept_a_column_name
+    q = IssueQuery.new
+    q.column_names = ['tracker', :subject]
+    assert q.has_column?(:tracker)
+    assert !q.has_column?(:category)
+  end
+
+  def test_has_column_should_accept_a_column
+    q = IssueQuery.new
+    q.column_names = ['tracker', :subject]
+
+    tracker_column = q.available_columns.detect {|c| c.name==:tracker}
+    assert_kind_of QueryColumn, tracker_column
+    category_column = q.available_columns.detect {|c| c.name==:category}
+    assert_kind_of QueryColumn, category_column
+
+    assert q.has_column?(tracker_column)
+    assert !q.has_column?(category_column)
   end
 
   def test_inline_and_block_columns
     q = IssueQuery.new
     q.column_names = ['subject', 'description', 'tracker']
 
-    assert_equal [:subject, :tracker], q.inline_columns.map(&:name)
+    assert_equal [:id, :subject, :tracker], q.inline_columns.map(&:name)
     assert_equal [:description], q.block_columns.map(&:name)
   end