Use classes instead of a complicated nested hash for search results

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
+        case type_filter.to_s
         when 'topic'
-                      post_query(type_filter.to_sym, args)
+          results.add(post_query(type_filter.to_sym, args))
         when 'category'
-                      category_query(args)
+          results.add(category_query(args))
         when 'user'
-                      user_query(args)
+          results.add(user_query(args))
         end
       else
         args.merge!(limit: (Search.per_facet + 1))
-        db_result = []
+        results.add(user_query(args).to_a)
-        db_result += user_query(args).to_a
+        results.add(category_query(args).to_a)
-        db_result += category_query(args).to_a
+        results.add(post_query(:topic, args).to_a)
-        db_result += 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|
+      # Subtract how many topics we have
-          expected_topics -= 1 if row['type'] == 'topic'
+      expected_topics -= results.topic_count
-        end
-      end
 
        if expected_topics > 0
-        tmp = post_query(:post, args.merge(limit: expected_topics * 3)).to_a
+        extra_topics = post_query(:post, args.merge(limit: expected_topics * 3)).to_a
 
-        topic_ids = Set.new db_result.map{|r| r["id"]}
+        topic_ids = results.topic_ids
-
+        extra_topics.reject! do |i|
-        tmp.reject! do |i|
+          new_topic_id = i['id'].to_i
-          if topic_ids.include?(i["id"])
+          if topic_ids.include?(new_topic_id)
             true
           else
-            topic_ids << i["id"]
+            topic_ids << new_topic_id
             false
           end
         end
-
+        results.add(extra_topics[0..expected_topics-1])
-        db_result += tmp[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',
+      results = GroupedSearchResults.new(@opts[:type_filter])
+      results.add('type' => 'topic',
                   'id' => topic.id,
                   'url' => topic.relative_url,
-                        'title' => topic.title }])
+                  '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,
+                    u.email
-                    NULL AS color,
-                    NULL AS text_color
             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
+    class SearchResult
-    def group_db_result(db_result)
+      attr_accessor :type, :id
-      grouped = {}
-      db_result.each do |row|
-        type = row.delete('type')
 
-        # Add the slug for topics
+      def initialize(row)
-        if type == 'topic'
+        @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)
+          @url.gsub!('slug', new_slug)
-        elsif type == 'user'
+        when :user
-          row['avatar_template'] = User.avatar_template(row['email'])
+          @avatar_template = User.avatar_template(row['email'])
+        when :category
+          @color, @text_color = row['color'], row['text_color']
+        end
       end
 
-        # Remove attributes when we know they don't matter
+      def as_json
-        row.delete('email')
+        json = {id: @id, title: @title, url: @url}
-        unless type == 'category'
+        json[:avatar_template] = @avatar_template if @avatar_template.present?
-          row.delete('color')
+        json[:color] = @color if @color.present?
-          row.delete('text_color')
+        json[:text_color] = @text_color if @text_color.present?
+        json
+      end
     end
 
-        grouped[type] = (grouped[type] || []) << row
+    class SearchResultType
+
+      attr_accessor :more, :results
+
+      def initialize(type)
+        @type = type
+        @results = []
+        @more = false
       end
 
-      grouped.map do |type, results|
+      def size
-        more = @opts[:type_filter].blank? && (results.size > Search.per_facet)
+        @results.size
-        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
-        }
       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

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[:title].should == topic.title
-        result['url'].should == topic.relative_url
+        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[:title].should == topic.title
-        result['url'].should == topic.relative_url
+        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[:title].should == topic.title
-        result['url'].should == topic.relative_url
+        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[:title].should == category.name
-      r['url'].should == "/category/#{category.slug}"
+      r[:url].should == "/category/#{category.slug}"
 
       category.deny(:all)
       category.save