diff options
| -rw-r--r-- | app/controllers/search_controller.rb | 56 | ||||
| -rw-r--r-- | app/models/changeset.rb | 4 | ||||
| -rw-r--r-- | app/models/document.rb | 2 | ||||
| -rw-r--r-- | app/models/issue.rb | 3 | ||||
| -rw-r--r-- | app/models/message.rb | 6 | ||||
| -rw-r--r-- | app/models/news.rb | 2 | ||||
| -rw-r--r-- | app/models/wiki_page.rb | 3 | ||||
| -rw-r--r-- | app/views/search/index.html.erb | 17 | ||||
| -rw-r--r-- | lib/plugins/acts_as_searchable/lib/acts_as_searchable.rb | 82 | ||||
| -rw-r--r-- | test/functional/search_controller_test.rb | 30 | ||||
| -rw-r--r-- | test/unit/search_test.rb | 48 |
11 files changed, 129 insertions, 124 deletions
diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index ec8b4812c..0abac94d3 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -36,9 +36,6 @@ class SearchController < ApplicationController @project end - offset = nil - begin; offset = params[:offset].to_time if params[:offset]; rescue; end - # quick jump to an issue if (m = @question.match(/^#?(\d+)$/)) && (issue = Issue.visible.find_by_id(m[1].to_i)) redirect_to issue_path(issue) @@ -66,34 +63,39 @@ class SearchController < ApplicationController # no more than 5 tokens to search for @tokens.slice! 5..-1 if @tokens.size > 5 - @results = [] - @results_by_type = Hash.new {|h,k| h[k] = 0} - limit = 10 - @scope.each do |s| - r, c = s.singularize.camelcase.constantize.search(@tokens, projects_to_search, + + @result_count = 0 + @result_count_by_type = Hash.new {|h,k| h[k] = 0} + ranks_and_ids = [] + + # get all the results ranks and ids + @scope.each do |scope| + klass = scope.singularize.camelcase.constantize + ranks_and_ids_in_scope = klass.search_result_ranks_and_ids(@tokens, User.current, projects_to_search, :all_words => @all_words, - :titles_only => @titles_only, - :limit => (limit+1), - :offset => offset, - :before => params[:previous].nil?) - @results += r - @results_by_type[s] += c + :titles_only => @titles_only + ) + @result_count_by_type[scope] += ranks_and_ids_in_scope.size + @result_count += ranks_and_ids_in_scope.size + ranks_and_ids += ranks_and_ids_in_scope.map {|r| [scope, r]} end - @results = @results.sort {|a,b| b.event_datetime <=> a.event_datetime} - if params[:previous].nil? - @pagination_previous_date = @results[0].event_datetime if offset && @results[0] - if @results.size > limit - @pagination_next_date = @results[limit-1].event_datetime - @results = @results[0, limit] - end - else - @pagination_next_date = @results[-1].event_datetime if offset && @results[-1] - if @results.size > limit - @pagination_previous_date = @results[-(limit)].event_datetime - @results = @results[-(limit), limit] - end + @result_pages = Paginator.new @result_count, limit, params['page'] + + # sort results, higher rank and id first + ranks_and_ids.sort! {|a,b| b.last <=> a.last } + ranks_and_ids = ranks_and_ids[@result_pages.offset, limit] || [] + + # load the results to display + results_by_scope = Hash.new {|h,k| h[k] = []} + ranks_and_ids.group_by(&:first).each do |scope, rs| + klass = scope.singularize.camelcase.constantize + results_by_scope[scope] += klass.search_results_from_ids(rs.map(&:last).map(&:last)) end + + @results = ranks_and_ids.map do |scope, r| + results_by_scope[scope].detect {|record| record.id == r.last} + end.compact else @question = "" end diff --git a/app/models/changeset.rb b/app/models/changeset.rb index 4eedae145..78a69422f 100644 --- a/app/models/changeset.rb +++ b/app/models/changeset.rb @@ -35,9 +35,9 @@ class Changeset < ActiveRecord::Base :url => Proc.new {|o| {:controller => 'repositories', :action => 'revision', :id => o.repository.project, :repository_id => o.repository.identifier_param, :rev => o.identifier}} acts_as_searchable :columns => 'comments', - :scope => preload(:repository => :project), + :preload => {:repository => :project}, :project_key => "#{Repository.table_name}.project_id", - :date_column => "#{Changeset.table_name}.committed_on" + :date_column => :committed_on acts_as_activity_provider :timestamp => "#{table_name}.committed_on", :author_key => :user_id, diff --git a/app/models/document.rb b/app/models/document.rb index 70e3c9b01..810b15dfa 100644 --- a/app/models/document.rb +++ b/app/models/document.rb @@ -22,7 +22,7 @@ class Document < ActiveRecord::Base acts_as_attachable :delete_permission => :delete_documents acts_as_searchable :columns => ['title', "#{table_name}.description"], - :scope => preload(:project) + :preload => :project acts_as_event :title => Proc.new {|o| "#{l(:label_document)}: #{o.title}"}, :author => Proc.new {|o| o.attachments.reorder("#{Attachment.table_name}.created_on ASC").first.try(:author) }, :url => Proc.new {|o| {:controller => 'documents', :action => 'show', :id => o.id}} diff --git a/app/models/issue.rb b/app/models/issue.rb index a23d4bd9f..84a5d7b6c 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -46,8 +46,7 @@ class Issue < ActiveRecord::Base acts_as_customizable acts_as_watchable acts_as_searchable :columns => ['subject', "#{table_name}.description", "#{Journal.table_name}.notes"], - # sort by id so that limited eager loading doesn't break with postgresql - :order_column => "#{table_name}.id", + :preload => [:project, :status, :tracker], :scope => lambda { joins(:project). joins("LEFT OUTER JOIN #{Journal.table_name} ON #{Journal.table_name}.journalized_type='Issue'" + " AND #{Journal.table_name}.journalized_id = #{Issue.table_name}.id" + diff --git a/app/models/message.rb b/app/models/message.rb index dee5a5b62..5f21e88d4 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -25,9 +25,9 @@ class Message < ActiveRecord::Base attr_protected :id acts_as_searchable :columns => ['subject', 'content'], - :scope => preload(:board => :project), - :project_key => "#{Board.table_name}.project_id", - :date_column => "#{table_name}.created_on" + :preload => {:board => :project}, + :project_key => "#{Board.table_name}.project_id" + acts_as_event :title => Proc.new {|o| "#{o.board.name}: #{o.subject}"}, :description => :content, :group => :parent, diff --git a/app/models/news.rb b/app/models/news.rb index 6ade4f4f1..046bb8137 100644 --- a/app/models/news.rb +++ b/app/models/news.rb @@ -29,7 +29,7 @@ class News < ActiveRecord::Base acts_as_attachable :edit_permission => :manage_news, :delete_permission => :manage_news acts_as_searchable :columns => ['title', 'summary', "#{table_name}.description"], - :scope => preload(:project) + :preload => :project acts_as_event :url => Proc.new {|o| {:controller => 'news', :action => 'show', :id => o.id}} acts_as_activity_provider :scope => preload(:project, :author), :author_key => :author_id diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index 61e78873d..8d81bce7e 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -33,7 +33,8 @@ class WikiPage < ActiveRecord::Base :url => Proc.new {|o| {:controller => 'wiki', :action => 'show', :project_id => o.wiki.project, :id => o.title}} acts_as_searchable :columns => ['title', "#{WikiContent.table_name}.text"], - :scope => preload(:wiki => :project).joins(:content, {:wiki => :project}), + :scope => joins(:content, {:wiki => :project}), + :preload => {:wiki => :project}, :permission => :view_wiki_pages, :project_key => "#{Wiki.table_name}.project_id" diff --git a/app/views/search/index.html.erb b/app/views/search/index.html.erb index 9c884811a..6bbe8d482 100644 --- a/app/views/search/index.html.erb +++ b/app/views/search/index.html.erb @@ -23,9 +23,9 @@ <% if @results %> <div id="search-results-counts"> - <%= render_results_by_type(@results_by_type) unless @scope.size == 1 %> + <%= render_results_by_type(@result_count_by_type) unless @scope.size == 1 %> </div> - <h3><%= l(:label_result_plural) %> (<%= @results_by_type.values.sum %>)</h3> + <h3><%= l(:label_result_plural) %> (<%= @result_count %>)</h3> <dl id="search-results"> <% @results.each do |e| %> <dt class="<%= e.event_type %>"> @@ -38,18 +38,9 @@ </dl> <% end %> -<p class="pagination"> -<% if @pagination_previous_date %> -<%= link_to_content_update("\xc2\xab " + l(:label_previous), - params.merge(:previous => 1, - :offset => @pagination_previous_date.strftime("%Y%m%d%H%M%S"))) %> +<% if @result_pages %> +<p class="pagination"><%= pagination_links_full @result_pages, @result_count, :per_page_links => false %></p> <% end %> -<% if @pagination_next_date %> -<%= link_to_content_update(l(:label_next) + " \xc2\xbb", - params.merge(:previous => nil, - :offset => @pagination_next_date.strftime("%Y%m%d%H%M%S"))) %> -<% end %> -</p> <% html_title(l(:label_search)) -%> diff --git a/lib/plugins/acts_as_searchable/lib/acts_as_searchable.rb b/lib/plugins/acts_as_searchable/lib/acts_as_searchable.rb index 8cd907b49..fa5fa202b 100644 --- a/lib/plugins/acts_as_searchable/lib/acts_as_searchable.rb +++ b/lib/plugins/acts_as_searchable/lib/acts_as_searchable.rb @@ -23,15 +23,18 @@ module Redmine end module ClassMethods + # Adds the search methods to the class. + # # Options: # * :columns - a column or an array of columns to search # * :project_key - project foreign key (default to project_id) - # * :date_column - name of the datetime column (default to created_on) - # * :sort_order - name of the column used to sort results (default to :date_column or created_on) - # * :permission - permission required to search the model (default to :view_"objects") + # * :date_column - name of the datetime column used to sort results (default to :created_on) + # * :permission - permission required to search the model + # * :scope - scope used to search results + # * :preload - associations to preload when loading results for display def acts_as_searchable(options = {}) return if self.included_modules.include?(Redmine::Acts::Searchable::InstanceMethods) - options.assert_valid_keys(:columns, :project_key, :date_column, :order_column, :search_custom_fields, :permission, :scope) + options.assert_valid_keys(:columns, :project_key, :date_column, :permission, :scope, :preload) cattr_accessor :searchable_options self.searchable_options = options @@ -43,8 +46,7 @@ module Redmine end searchable_options[:project_key] ||= "#{table_name}.project_id" - searchable_options[:date_column] ||= "#{table_name}.created_on" - searchable_options[:order_column] ||= searchable_options[:date_column] + searchable_options[:date_column] ||= :created_on # Should we search custom fields on this model ? searchable_options[:search_custom_fields] = !reflect_on_association(:custom_values).nil? @@ -59,23 +61,28 @@ module Redmine end module ClassMethods - # Searches the model for the given tokens - # projects argument can be either nil (will search all projects), a project or an array of projects - # Returns the results and the results count - def search(tokens, projects=nil, options={}) + # Searches the model for the given tokens and user visibility. + # The projects argument can be either nil (will search all projects), a project or an array of projects. + # Returns an array that contains the rank and id of all results. + # In current implementation, the rank is the record timestamp. + # + # Valid options: + # * :titles_only - searches tokens in the first searchable column only + # * :all_words - searches results that match all token + # * :limit - maximum number of results to return + # + # Example: + # Issue.search_result_ranks_and_ids("foo") + # # => [[Tue, 26 Jun 2007 22:16:00 UTC +00:00, 69], [Mon, 08 Oct 2007 14:31:00 UTC +00:00, 123]] + def search_result_ranks_and_ids(tokens, user=User.current, projects=nil, options={}) if projects.is_a?(Array) && projects.empty? # no results - return [[], 0] + return [] end - # TODO: make user an argument - user = User.current tokens = [] << tokens unless tokens.is_a?(Array) projects = [] << projects if projects.is_a?(Project) - limit_options = {} - limit_options[:limit] = options[:limit] if options[:limit] - columns = searchable_options[:columns] columns = columns[0..0] if options[:titles_only] @@ -105,38 +112,39 @@ module Redmine if scope.is_a? Proc scope = scope.call end - project_conditions = [] - if searchable_options.has_key?(:permission) - project_conditions << Project.allowed_to_condition(user, searchable_options[:permission] || :view_project) - elsif respond_to?(:visible) + + if respond_to?(:visible) && !searchable_options.has_key?(:permission) scope = scope.visible(user) else - ActiveSupport::Deprecation.warn "acts_as_searchable with implicit :permission option is deprecated. Add a visible scope to the #{self.name} model or use explicit :permission option." - project_conditions << Project.allowed_to_condition(user, "view_#{self.name.underscore.pluralize}".to_sym) + permission = searchable_options[:permission] || :view_project + scope = scope.where(Project.allowed_to_condition(user, permission)) end + # TODO: use visible scope options instead - project_conditions << "#{searchable_options[:project_key]} IN (#{projects.collect(&:id).join(',')})" unless projects.nil? - project_conditions = project_conditions.empty? ? nil : project_conditions.join(' AND ') + if projects + scope = scope.where("#{searchable_options[:project_key]} IN (?)", projects.map(&:id)) + end results = [] results_count = 0 - scope = scope. - joins(searchable_options[:include]). - order("#{searchable_options[:order_column]} " + (options[:before] ? 'DESC' : 'ASC')). - where(project_conditions). + scope. + reorder(searchable_options[:date_column] => :desc, :id => :desc). where(tokens_conditions). - uniq - - results_count = scope.count + limit(options[:limit]). + uniq. + pluck(searchable_options[:date_column], :id) + end - scope_with_limit = scope.limit(options[:limit]) - if options[:offset] - scope_with_limit = scope_with_limit.where("#{searchable_options[:date_column]} #{options[:before] ? '<' : '>'} ?", options[:offset]) - end - results = scope_with_limit.to_a + # Returns search results of given ids + def search_results_from_ids(ids) + where(:id => ids).preload(searchable_options[:preload]).to_a + end - [results, results_count] + # Returns search results with same arguments as search_result_ranks_and_ids + def search_results(*args) + ranks_and_ids = search_result_ranks_and_ids(*args) + search_results_from_ids(ranks_and_ids.map(&:last)) end end end diff --git a/test/functional/search_controller_test.rb b/test/functional/search_controller_test.rb index 4124985db..24dfa7d6f 100644 --- a/test/functional/search_controller_test.rb +++ b/test/functional/search_controller_test.rb @@ -75,8 +75,8 @@ class SearchControllerTest < ActionController::TestCase assert_select 'dt.issue a', :text => /Add ingredients categories/ assert_select 'dd', :text => /should be classified by categories/ - assert assigns(:results_by_type).is_a?(Hash) - assert_equal 5, assigns(:results_by_type)['changesets'] + assert assigns(:result_count_by_type).is_a?(Hash) + assert_equal 5, assigns(:result_count_by_type)['changesets'] assert_select 'a', :text => 'Changesets (5)' end @@ -222,20 +222,24 @@ class SearchControllerTest < ActionController::TestCase assert_equal 1, results.size end - def test_search_with_offset - get :index, :q => 'coo', :offset => '20080806073000' + def test_search_with_pagination + issue = (0..24).map {Issue.generate! :subject => 'search_with_limited_results'}.reverse + + get :index, :q => 'search_with_limited_results' assert_response :success - results = assigns(:results) - assert results.any? - assert results.map(&:event_datetime).max < '20080806T073000'.to_time - end + assert_equal issue[0..9], assigns(:results) - def test_search_previous_with_offset - get :index, :q => 'coo', :offset => '20080806073000', :previous => '1' + get :index, :q => 'search_with_limited_results', :page => 2 assert_response :success - results = assigns(:results) - assert results.any? - assert results.map(&:event_datetime).min >= '20080806T073000'.to_time + assert_equal issue[10..19], assigns(:results) + + get :index, :q => 'search_with_limited_results', :page => 3 + assert_response :success + assert_equal issue[20..24], assigns(:results) + + get :index, :q => 'search_with_limited_results', :page => 4 + assert_response :success + assert_equal [], assigns(:results) end def test_search_with_invalid_project_id diff --git a/test/unit/search_test.rb b/test/unit/search_test.rb index 5a10ec71c..c98982e8a 100644 --- a/test/unit/search_test.rb +++ b/test/unit/search_test.rb @@ -42,25 +42,25 @@ class SearchTest < ActiveSupport::TestCase def test_search_by_anonymous User.current = nil - r = Issue.search(@issue_keyword).first + r = Issue.search_results(@issue_keyword) assert r.include?(@issue) - r = Changeset.search(@changeset_keyword).first + r = Changeset.search_results(@changeset_keyword) assert r.include?(@changeset) # Removes the :view_changesets permission from Anonymous role remove_permission Role.anonymous, :view_changesets User.current = nil - r = Issue.search(@issue_keyword).first + r = Issue.search_results(@issue_keyword) assert r.include?(@issue) - r = Changeset.search(@changeset_keyword).first + r = Changeset.search_results(@changeset_keyword) assert !r.include?(@changeset) # Make the project private @project.update_attribute :is_public, false - r = Issue.search(@issue_keyword).first + r = Issue.search_results(@issue_keyword) assert !r.include?(@issue) - r = Changeset.search(@changeset_keyword).first + r = Changeset.search_results(@changeset_keyword) assert !r.include?(@changeset) end @@ -68,25 +68,25 @@ class SearchTest < ActiveSupport::TestCase User.current = User.find_by_login('rhill') assert User.current.memberships.empty? - r = Issue.search(@issue_keyword).first + r = Issue.search_results(@issue_keyword) assert r.include?(@issue) - r = Changeset.search(@changeset_keyword).first + r = Changeset.search_results(@changeset_keyword) assert r.include?(@changeset) # Removes the :view_changesets permission from Non member role remove_permission Role.non_member, :view_changesets User.current = User.find_by_login('rhill') - r = Issue.search(@issue_keyword).first + r = Issue.search_results(@issue_keyword) assert r.include?(@issue) - r = Changeset.search(@changeset_keyword).first + r = Changeset.search_results(@changeset_keyword) assert !r.include?(@changeset) # Make the project private @project.update_attribute :is_public, false - r = Issue.search(@issue_keyword).first + r = Issue.search_results(@issue_keyword) assert !r.include?(@issue) - r = Changeset.search(@changeset_keyword).first + r = Changeset.search_results(@changeset_keyword) assert !r.include?(@changeset) end @@ -94,16 +94,16 @@ class SearchTest < ActiveSupport::TestCase User.current = User.find_by_login('jsmith') assert User.current.projects.include?(@project) - r = Issue.search(@issue_keyword).first + r = Issue.search_results(@issue_keyword) assert r.include?(@issue) - r = Changeset.search(@changeset_keyword).first + r = Changeset.search_results(@changeset_keyword) assert r.include?(@changeset) # Make the project private @project.update_attribute :is_public, false - r = Issue.search(@issue_keyword).first + r = Issue.search_results(@issue_keyword) assert r.include?(@issue) - r = Changeset.search(@changeset_keyword).first + r = Changeset.search_results(@changeset_keyword) assert r.include?(@changeset) end @@ -115,26 +115,26 @@ class SearchTest < ActiveSupport::TestCase User.current = User.find_by_login('jsmith') assert User.current.projects.include?(@project) - r = Issue.search(@issue_keyword).first + r = Issue.search_results(@issue_keyword) assert r.include?(@issue) - r = Changeset.search(@changeset_keyword).first + r = Changeset.search_results(@changeset_keyword) assert !r.include?(@changeset) # Make the project private @project.update_attribute :is_public, false - r = Issue.search(@issue_keyword).first + r = Issue.search_results(@issue_keyword) assert r.include?(@issue) - r = Changeset.search(@changeset_keyword).first + r = Changeset.search_results(@changeset_keyword) assert !r.include?(@changeset) end def test_search_issue_with_multiple_hits_in_journals - i = Issue.find(1) - assert_equal 2, i.journals.where("notes LIKE '%notes%'").count + issue = Issue.find(1) + assert_equal 2, issue.journals.where("notes LIKE '%notes%'").count - r = Issue.search('%notes%').first + r = Issue.search_results('%notes%') assert_equal 1, r.size - assert_equal i, r.first + assert_equal issue, r.first end private |