jro_apps/redmine Commit - r13357:2fe806a4a49c

Rewrites search engine to properly paginate results (#18631)....

Jean-Philippe Lang -

r13357:2fe806a4a49c

parent child

Context file:

r13357:2fe806a4a49c

Collapse all files

app/controllers/search_controller.rb +29 -27

@@ -36,9 +36,6 class SearchController < ApplicationController
36	@project	36	@project
37	end	37	end
38		38
39	offset = nil
40	begin; offset = params[:offset].to_time if params[:offset]; rescue; end
41
42	# quick jump to an issue	39	# quick jump to an issue
43	if (m = @question.match(/^#?(\d+)$/)) && (issue = Issue.visible.find_by_id(m[1].to_i))	40	if (m = @question.match(/^#?(\d+)$/)) && (issue = Issue.visible.find_by_id(m[1].to_i))
44	redirect_to issue_path(issue)	41	redirect_to issue_path(issue)
@@ -66,34 +63,39 class SearchController < ApplicationController
66	# no more than 5 tokens to search for	63	# no more than 5 tokens to search for
67	@tokens.slice! 5..-1 if @tokens.size > 5	64	@tokens.slice! 5..-1 if @tokens.size > 5
68		65
69	@results = []
70	@results_by_type = Hash.new {\|h,k\| h[k] = 0}
71
72	limit = 10	66	limit = 10
73	@scope.each do \|s\|	67
74	r, c = s.singularize.camelcase.constantize.search(@tokens, projects_to_search,	68	@result_count = 0
		69	@result_count_by_type = Hash.new {\|h,k\| h[k] = 0}
		70	ranks_and_ids = []
		71
		72	# get all the results ranks and ids
		73	@scope.each do \|scope\|
		74	klass = scope.singularize.camelcase.constantize
		75	ranks_and_ids_in_scope = klass.search_result_ranks_and_ids(@tokens, User.current, projects_to_search,
75	:all_words => @all_words,	76	:all_words => @all_words,
76	:titles_only => @titles_only,	77	:titles_only => @titles_only
77	:limit => (limit+1),	78	)
78	:offset => offset,	79	@result_count_by_type[scope] += ranks_and_ids_in_scope.size
79	:before => params[:previous].nil?)	80	@result_count += ranks_and_ids_in_scope.size
80	@results += r	81	ranks_and_ids += ranks_and_ids_in_scope.map {\|r\| [scope, r]}
81	@results_by_type[s] += c
82	end	82	end
83	@results = @results.sort {\|a,b\| b.event_datetime <=> a.event_datetime}	83	@result_pages = Paginator.new @result_count, limit, params['page']
84	if params[:previous].nil?	84
85	@pagination_previous_date = @results[0].event_datetime if offset && @results[0]	85	# sort results, higher rank and id first
86	if @results.size > limit	86	ranks_and_ids.sort! {\|a,b\| b.last <=> a.last }
87	@pagination_next_date = @results[limit-1].event_datetime	87	ranks_and_ids = ranks_and_ids[@result_pages.offset, limit] \|\| []
88	@results = @results[0, limit]	88
89	end	89	# load the results to display
90	else	90	results_by_scope = Hash.new {\|h,k\| h[k] = []}
91	@pagination_next_date = @results[-1].event_datetime if offset && @results[-1]	91	ranks_and_ids.group_by(&:first).each do \|scope, rs\|
92	if @results.size > limit	92	klass = scope.singularize.camelcase.constantize
93	@pagination_previous_date = @results[-(limit)].event_datetime	93	results_by_scope[scope] += klass.search_results_from_ids(rs.map(&:last).map(&:last))
94	@results = @results[-(limit), limit]
95	end
96	end	94	end
		95
		96	@results = ranks_and_ids.map do \|scope, r\|
		97	results_by_scope[scope].detect {\|record\| record.id == r.last}
		98	end.compact
97	else	99	else
98	@question = ""	100	@question = ""
99	end	101	end

app/models/changeset.rb +2 -2

                             :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,

app/models/document.rb +1 -1

               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}}

app/models/issue.rb +1 -2

               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
+                                 :preload => [:project, :status, :tracker],
-                                 :order_column => "#{table_name}.id",
                                  :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" +

app/models/message.rb +3 -3

               attr_protected :id
               acts_as_searchable :columns => ['subject', 'content'],
-                                 :scope => preload(:board => :project),
+                                 :preload => {:board => :project},
-                                 :project_key => "#{Board.table_name}.project_id",
+                                 :project_key => "#{Board.table_name}.project_id"
-                                 :date_column => "#{table_name}.created_on"
               acts_as_event :title => Proc.new {|o| "#{o.board.name}: #{o.subject}"},
                             :description => :content,
                             :group => :parent,

app/models/news.rb +1 -1

               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

app/models/wiki_page.rb +2 -1

                             :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"

app/views/search/index.html.erb +4 -13

             <% 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 %>">
                 </dl>
             <% end %>
-            <p class="pagination">
+            <% if @result_pages %>
-            <% if @pagination_previous_date %>
+            <p class="pagination"><%= pagination_links_full @result_pages, @result_count, :per_page_links => false %></p>
-            <%= link_to_content_update("\xc2\xab " + l(:label_previous),
-                  params.merge(:previous => 1,
-                               :offset => @pagination_previous_date.strftime("%Y%m%d%H%M%S"))) %>&nbsp;
             <% 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)) -%>

lib/plugins/acts_as_searchable/lib/acts_as_searchable.rb +45 -37

                   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)
+                    # * :date_column - name of the datetime column used to sort results (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
-                    # * :permission - permission required to search the model (default to :view_"objects")
+                    # * :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
                       end
                       searchable_options[:project_key] ||= "#{table_name}.project_id"
-                      searchable_options[:date_column] ||= "#{table_name}.created_on"
+                      searchable_options[:date_column] ||= :created_on
-                      searchable_options[:order_column] ||= searchable_options[:date_column]
                       # Should we search custom fields on this model ?
                       searchable_options[:search_custom_fields] = !reflect_on_association(:custom_values).nil?
                     end
                     module ClassMethods
-                      # Searches the model for the given tokens
+                      # Searches the model for the given tokens and user visibility.
-                      # projects argument can be either nil (will search all projects), a project or an array of projects
+                      # The projects argument can be either nil (will search all projects), a project or an array of projects.
-                      # Returns the results and the results count
+                      # Returns an array that contains the rank and id of all results.
-                      def search(tokens, projects=nil, options={})
+                      # 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]
                         if scope.is_a? Proc
                           scope = scope.call
                         end
-                        project_conditions = []
-                        if searchable_options.has_key?(:permission)
+                        if respond_to?(:visible) && !searchable_options.has_key?(:permission)
-                          project_conditions << Project.allowed_to_condition(user, searchable_options[:permission] || :view_project)
-                        elsif respond_to?(:visible)
                           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."
+                          permission = searchable_options[:permission] || :view_project
-                          project_conditions << Project.allowed_to_condition(user, "view_#{self.name.underscore.pluralize}".to_sym)
+                          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?
+                        if projects
-                        project_conditions = project_conditions.empty? ? nil : project_conditions.join(' AND ')
+                          scope = scope.where("#{searchable_options[:project_key]} IN (?)", projects.map(&:id))
+                        end
                         results = []
                         results_count = 0
-                        scope = scope.
+                        scope.
-                          joins(searchable_options[:include]).
+                          reorder(searchable_options[:date_column] => :desc, :id => :desc).
-                          order("#{searchable_options[:order_column]} " + (options[:before] ? 'DESC' : 'ASC')).
-                          where(project_conditions).
                           where(tokens_conditions).
-                          uniq
+                          limit(options[:limit]).
+                          uniq.
-                        results_count = scope.count
+                          pluck(searchable_options[:date_column], :id)
+                      end
-                        scope_with_limit = scope.limit(options[:limit])
+                      # Returns search results of given ids
-                        if options[:offset]
+                      def search_results_from_ids(ids)
-                          scope_with_limit = scope_with_limit.where("#{searchable_options[:date_column]} #{options[:before] ? '<' : '>'} ?", options[:offset])
+                        where(:id => ids).preload(searchable_options[:preload]).to_a
-                        end
+                      end
-                        results = scope_with_limit.to_a
-                        [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

test/functional/search_controller_test.rb +17 -13

                 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 assigns(:result_count_by_type).is_a?(Hash)
-                assert_equal 5, assigns(:results_by_type)['changesets']
+                assert_equal 5, assigns(:result_count_by_type)['changesets']
                 assert_select 'a', :text => 'Changesets (5)'
               end
                 assert_equal 1, results.size
               end
-              def test_search_with_offset
+              def test_search_with_pagination
-                get :index, :q => 'coo', :offset => '20080806073000'
+                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_equal issue[0..9], assigns(:results)
-                assert results.any?
-                assert results.map(&:event_datetime).max < '20080806T073000'.to_time
-              end
-              def test_search_previous_with_offset
+                get :index, :q => 'search_with_limited_results', :page => 2
-                get :index, :q => 'coo', :offset => '20080806073000', :previous => '1'
                 assert_response :success
-                results = assigns(:results)
+                assert_equal issue[10..19], assigns(:results)
-                assert results.any?
-                assert results.map(&:event_datetime).min >= '20080806T073000'.to_time
+                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

test/unit/search_test.rb +24 -24

               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
                 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
                 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
                 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)
+                issue = Issue.find(1)
-                assert_equal 2, i.journals.where("notes LIKE '%notes%'").count
+                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

General Comments 0

Write
Preview

You need to be logged in to leave comments. Login now

No TODOs yet

	Site-wide shortcuts
/	Use quick search box
g h	Goto home page
g g	Goto my private gists page
g G	Goto my public gists page
g 0-9	Goto bookmarked items from 0-9
n r	New repository page
n g	New gist page

	Repositories
g s	Goto summary page
g c	Goto changelog page
g f	Goto files page
g F	Goto files page with file search activated
g p	Goto pull requests page
g o	Goto repository settings
g O	Goto repository access permissions settings
t s	Toggle sidebar on some pages