From 8e8d9af2bf98359055322138880e928739183951 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Thu, 23 May 2013 11:13:23 -0400 Subject: [PATCH] Use classes instead of a complicated nested hash for search results --- lib/search.rb | 185 ++++++++++++++++++++++----------- spec/components/search_spec.rb | 22 ++-- 2 files changed, 133 insertions(+), 74 deletions(-) diff --git a/lib/search.rb b/lib/search.rb index 03de8787c..c8ba84c4c 100644 --- a/lib/search.rb +++ b/lib/search.rb @@ -64,57 +64,52 @@ class Search query: @term.split.map {|t| "#{t}:*"}.join(" & "), locale: Search.current_locale_long} + results = GroupedSearchResults.new(@opts[:type_filter]) type_filter = @opts[:type_filter] if type_filter.present? raise Discourse::InvalidAccess.new("invalid type filter") unless Search.facets.include?(type_filter) args.merge!(limit: Search.per_facet * Search.facets.size) - db_result = case type_filter.to_s - when 'topic' - post_query(type_filter.to_sym, args) - when 'category' - category_query(args) - when 'user' - user_query(args) - end + case type_filter.to_s + when 'topic' + results.add(post_query(type_filter.to_sym, args)) + when 'category' + results.add(category_query(args)) + when 'user' + results.add(user_query(args)) + end else args.merge!(limit: (Search.per_facet + 1)) - db_result = [] - db_result += user_query(args).to_a - db_result += category_query(args).to_a - db_result += post_query(:topic, args).to_a + results.add(user_query(args).to_a) + results.add(category_query(args).to_a) + results.add(post_query(:topic, args).to_a) end - db_result = db_result.to_a - expected_topics = 0 expected_topics = Search.facets.size unless type_filter.present? expected_topics = Search.per_facet * Search.facets.size if type_filter == 'topic' - if expected_topics > 0 - db_result.each do |row| - expected_topics -= 1 if row['type'] == 'topic' - end - end - if expected_topics > 0 - tmp = post_query(:post, args.merge(limit: expected_topics * 3)).to_a + # Subtract how many topics we have + expected_topics -= results.topic_count - topic_ids = Set.new db_result.map{|r| r["id"]} + if expected_topics > 0 + extra_topics = post_query(:post, args.merge(limit: expected_topics * 3)).to_a - tmp.reject! do |i| - if topic_ids.include?(i["id"]) + topic_ids = results.topic_ids + extra_topics.reject! do |i| + new_topic_id = i['id'].to_i + if topic_ids.include?(new_topic_id) true else - topic_ids << i["id"] + topic_ids << new_topic_id false end end - - db_result += tmp[0..expected_topics-1] + results.add(extra_topics[0..expected_topics-1]) end - group_db_result(db_result) + results.as_json end @@ -123,10 +118,12 @@ class Search topic = Topic.where(id: id).first return nil unless @guardian.can_see?(topic) - group_db_result([{'type' => 'topic', - 'id' => topic.id, - 'url' => topic.relative_url, - 'title' => topic.title }]) + results = GroupedSearchResults.new(@opts[:type_filter]) + results.add('type' => 'topic', + 'id' => topic.id, + 'url' => topic.relative_url, + 'title' => topic.title) + results.as_json end def add_allowed_categories(builder) @@ -167,9 +164,7 @@ SQL u.username_lower AS id, '/users/' || u.username_lower AS url, u.username AS title, - u.email, - NULL AS color, - NULL AS text_color + u.email FROM users AS u JOIN user_search_data s on s.user_id = u.id WHERE s.search_data @@ TO_TSQUERY(:locale, :query) @@ -223,41 +218,105 @@ SQL builder.exec(args) end - # Group the results by type - def group_db_result(db_result) - grouped = {} - db_result.each do |row| - type = row.delete('type') + class SearchResult + attr_accessor :type, :id - # Add the slug for topics - if type == 'topic' + def initialize(row) + @type = row['type'].to_sym + @url, @id, @title = row['url'], row['id'].to_i, row['title'] + + case @type + when :topic + # Some topics don't have slugs. In that case, use 'topic' as the slug. new_slug = Slug.for(row['title']) new_slug = "topic" if new_slug.blank? - row['url'].gsub!('slug', new_slug) - elsif type == 'user' - row['avatar_template'] = User.avatar_template(row['email']) + @url.gsub!('slug', new_slug) + when :user + @avatar_template = User.avatar_template(row['email']) + when :category + @color, @text_color = row['color'], row['text_color'] end - - # Remove attributes when we know they don't matter - row.delete('email') - unless type == 'category' - row.delete('color') - row.delete('text_color') - end - - grouped[type] = (grouped[type] || []) << row end - grouped.map do |type, results| - more = @opts[:type_filter].blank? && (results.size > Search.per_facet) - results = results[0..([results.length, Search.per_facet].min - 1)] if @opts[:type_filter].blank? - { - type: type, - name: I18n.t("search.types.#{type}"), - more: more, - results: results - } + def as_json + json = {id: @id, title: @title, url: @url} + json[:avatar_template] = @avatar_template if @avatar_template.present? + json[:color] = @color if @color.present? + json[:text_color] = @text_color if @text_color.present? + json end end + class SearchResultType + + attr_accessor :more, :results + + def initialize(type) + @type = type + @results = [] + @more = false + end + + def size + @results.size + end + + def add(result) + @results << result + end + + def as_json + { type: @type.to_s, + name: I18n.t("search.types.#{@type.to_s}"), + more: @more, + results: @results.map(&:as_json) } + end + end + + class GroupedSearchResults + + attr_reader :topic_count + + def initialize(type_filter) + @type_filter = type_filter + @by_type = {} + @topic_count = 0 + end + + def add(results) + results = [results] if results.is_a?(Hash) + + results.each do |r| + add_result(SearchResult.new(r)) + end + end + + def add_result(result) + grouped_result = @by_type[result.type] || (@by_type[result.type] = SearchResultType.new(result.type)) + + # Limit our results if there is no filter + if @type_filter.present? or (grouped_result.size < Search.per_facet) + @topic_count += 1 if (result.type == :topic) + + grouped_result.add(result) + else + grouped_result.more = true + end + end + + def topic_ids + topic_results = @by_type[:topic] + return Set.new if topic_results.blank? + + Set.new(topic_results.results.map(&:id)) + end + + def as_json + @by_type.values.map do |grouped_result| + grouped_result.as_json + end + end + + end + end diff --git a/spec/components/search_spec.rb b/spec/components/search_spec.rb index 3c3c41241..920c70332 100644 --- a/spec/components/search_spec.rb +++ b/spec/components/search_spec.rb @@ -99,15 +99,15 @@ describe Search do end it 'has the display name as the title' do - result['title'].should == user.username + result[:title].should == user.username end it 'has the avatar_template is there so it can hand it to the client' do - result['avatar_template'].should_not be_nil + result[:avatar_template].should_not be_nil end it 'has a url for the record' do - result['url'].should == "/users/#{user.username_lower}" + result[:url].should == "/users/#{user.username_lower}" end end @@ -121,8 +121,8 @@ describe Search do it 'returns a result correctly' do result.should be_present - result['title'].should == topic.title - result['url'].should == topic.relative_url + result[:title].should == topic.title + result[:url].should == topic.relative_url end end @@ -131,8 +131,8 @@ describe Search do it 'returns the topic' do result.should be_present - result['title'].should == topic.title - result['url'].should == topic.relative_url + result[:title].should == topic.title + result[:url].should == topic.relative_url end end @@ -141,8 +141,8 @@ describe Search do it 'returns the topic' do result.should be_present - result['title'].should == topic.title - result['url'].should == topic.relative_url + result[:title].should == topic.title + result[:url].should == topic.relative_url end end @@ -195,8 +195,8 @@ describe Search do it 'returns the correct result' do r = result r.should be_present - r['title'].should == category.name - r['url'].should == "/category/#{category.slug}" + r[:title].should == category.name + r[:url].should == "/category/#{category.slug}" category.deny(:all) category.save