FEATURE: increase search expansion to 50 results

refactor search code to deal with proper objects
use proper serializers, test the controllers
This commit is contained in:
Sam 2014-09-02 19:15:08 +10:00
parent b04a52676e
commit 4f09d552ed
13 changed files with 261 additions and 205 deletions

View File

@ -57,21 +57,50 @@ export default Em.ArrayController.extend(Discourse.Presence, {
if (results) {
self.set('noResults', results.length === 0);
var index = 0;
results = _(['topic', 'category', 'user'])
.map(function(n){
return _(results).where({type: n}).first();
})
.compact()
.each(function(list){
_.each(list.results, function(item){
item.index = index++;
urls.pushObject(item.url);
});
})
.value();
var topicMap = {};
results.topics = results.topics.map(function(topic){
topic = Discourse.Topic.create(topic);
topicMap[topic.id] = topic;
return topic;
});
self.setProperties({ resultCount: index, content: results, urls: urls });
results.posts = results.posts.map(function(post){
post = Discourse.Post.create(post);
post.set('topic', topicMap[post.topic_id]);
urls.push(post.get('url'));
return post;
});
results.users = results.users.map(function(user){
user = Discourse.User.create(user);
urls.push(user.get('path'));
return user;
});
results.categories = results.categories.map(function(category){
category = Discourse.Category.create(category);
urls.push(category.get('url'));
return category;
});
var r = results.grouped_search_result;
results.resultTypes = [];
// TODO: consider refactoring front end to take a better structure
[['topic','posts'],['user','users'],['category','categories']].forEach(function(pair){
var type = pair[0], name = pair[1];
if(results[name].length > 0) {
results.resultTypes.push({
results: results[name],
type: type,
more: r['more_' + name]
});
}
});
console.log(results)
self.setProperties({ resultCount: urls.length, content: results, urls: urls });
}
self.set('loading', false);

View File

@ -10,7 +10,7 @@
<div class='searching'></div>
{{else}}
{{#unless noResults}}
{{#each resultType in content}}
{{#each resultType in content.resultTypes}}
<ul>
<li class='heading'>
{{resultType.name}}

View File

@ -1,3 +1,3 @@
<a href='{{unbound url}}'>
<span class='badge-category' style="background-color: #{{unbound color}}; color: #{{unbound text_color}};">{{unbound title}}</span>
{{bound-category-link this}}
</a>

View File

@ -1,6 +1,6 @@
<a class='search-link' href='{{unbound url}}'>
<span class='topic'>
{{unbound title}}
{{unbound topic.title}}
</span>
{{#unless Discourse.Mobile.mobileView}}
<span class='blurb'>

View File

@ -1,4 +1,4 @@
<a href='{{unbound url}}'>
{{avatar this usernamePath="title" imageSize="small"}}
{{unbound title}}
<a href='{{unbound path}}'>
{{avatar this imageSize="small"}}
{{unbound username}}
</a>

View File

@ -36,7 +36,8 @@ class SearchController < ApplicationController
end
search = Search.new(params[:term], search_args.symbolize_keys)
render_json_dump(search.execute.as_json)
result = search.execute
render_serialized(result, GroupedSearchResultSerializer, :result => result)
end
end

View File

@ -0,0 +1,6 @@
class GroupedSearchResultSerializer < ApplicationSerializer
has_many :posts, serializer: SearchPostSerializer
has_many :users, serializer: BasicUserSerializer
has_many :categories, serializer: BasicCategorySerializer
attributes :more_posts, :more_users, :more_categories
end

View File

@ -0,0 +1,9 @@
class SearchPostSerializer < PostSerializer
has_one :topic, serializer: ListableTopicSerializer
attributes :blurb
def blurb
options[:result].blurb(object)
end
end

View File

@ -8,6 +8,10 @@ class Search
5
end
def self.per_filter
50
end
# Sometimes we want more topics than are returned due to exclusion of dupes. This is the
# factor of extra results we'll ask for.
def self.burst_factor
@ -102,8 +106,16 @@ class Search
@guardian = @opts[:guardian] || Guardian.new
@search_context = @opts[:search_context]
@include_blurbs = @opts[:include_blurbs] || false
@limit = Search.per_facet * Search.facets.size
@results = GroupedSearchResults.new(@opts[:type_filter])
@limit = Search.per_facet
if @opts[:type_filter].present?
@limit = Search.per_filter
end
@results = GroupedSearchResults.new(@opts[:type_filter], term, @search_context, @include_blurbs)
end
def self.execute(term, opts=nil)
self.new(term, opts).execute
end
# Query a term
@ -112,15 +124,20 @@ class Search
# If the term is a number or url to a topic, just include that topic
if @opts[:search_for_id] && @results.type_filter == 'topic'
return single_topic(@term.to_i).as_json if @term =~ /^\d+$/
begin
route = Rails.application.routes.recognize_path(@term)
return single_topic(route[:topic_id]).as_json if route[:topic_id].present?
rescue ActionController::RoutingError
if @term =~ /^\d+$/
single_topic(@term.to_i)
else
begin
route = Rails.application.routes.recognize_path(@term)
single_topic(route[:topic_id]) if route[:topic_id].present?
rescue ActionController::RoutingError
end
end
end
find_grouped_results.as_json
find_grouped_results unless @results.posts.present?
@results
end
private
@ -151,22 +168,24 @@ class Search
expected_topics = 0
expected_topics = Search.facets.size unless @results.type_filter.present?
expected_topics = Search.per_facet * Search.facets.size if @results.type_filter == 'topic'
expected_topics -= @results.topic_count
expected_topics -= @results.posts.length
if expected_topics > 0
extra_posts = posts_query(expected_topics * Search.burst_factor)
extra_posts = extra_posts.where("posts.topic_id NOT in (?)", @results.topic_ids) if @results.topic_ids.present?
extra_posts.each do |p|
@results.add_result(SearchResult.from_post(p, @search_context, @term, @include_blurbs))
extra_posts = extra_posts.where("posts.topic_id NOT in (?)", @results.posts.map(&:topic_id)) if @results.posts.present?
extra_posts.each do |post|
@results.add(post)
expected_topics -= 1
break if expected_topics == 0
end
end
end
# If we're searching for a single topic
def single_topic(id)
topic = Topic.find_by(id: id)
return nil unless @guardian.can_see?(topic)
post = Post.find_by(topic_id: id, post_number: 1)
return nil unless @guardian.can_see?(post)
@results.add_result(SearchResult.from_topic(topic))
@results.add(post)
@results
end
@ -189,8 +208,8 @@ class Search
.secured(@guardian)
.limit(@limit)
categories.each do |c|
@results.add_result(SearchResult.from_category(c))
categories.each do |category|
@results.add(category)
end
end
@ -202,18 +221,18 @@ class Search
.limit(@limit)
.references(:user_search_data)
users.each do |u|
@results.add_result(SearchResult.from_user(u))
users.each do |user|
@results.add(user)
end
end
def posts_query(limit, opts=nil)
opts ||= {}
posts = Post.includes(:post_search_data, {:topic => :category})
posts = Post
.joins(:post_search_data, {:topic => :category})
.where("topics.deleted_at" => nil)
.where("topics.visible")
.where("topics.archetype <> ?", Archetype.private_message)
.references(:post_search_data, {:topic => :category})
if @search_context.present? && @search_context.is_a?(Topic)
posts = posts.where("posts.raw ilike ?", "%#{@term}%")
@ -279,45 +298,31 @@ class Search
end
def aggregate_search
cols = ['topics.id', 'topics.title', 'topics.slug', 'cooked']
topics = posts_query(@limit, aggregate_search: true)
.group(*cols)
.pluck('min(posts.post_number)',*cols)
post_sql = posts_query(@limit, aggregate_search: true)
.select('topics.id', 'min(post_number) post_number, row_number() OVER() row_number')
.group('topics.id')
.to_sql
posts = Post.includes(:topic => :category)
.joins("JOIN (#{post_sql}) x ON x.id = posts.topic_id AND x.post_number = posts.post_number")
.order('row_number')
topics.each do |t|
post_number, topic_id, title, slug, cooked = t
cooked = SearchObserver::HtmlScrubber.scrub(cooked).squish
blurb = SearchResult.blurb(cooked, @term)
@results.add_result(SearchResult.new(type: :topic,
topic_id: topic_id,
id: topic_id,
title: title,
url: "/t/#{slug}/#{topic_id}/#{post_number}",
blurb: blurb))
posts.each do |post|
@results.add(post)
end
end
def topic_search
posts = if @search_context.is_a?(User)
# If we have a user filter, search all posts by default with a higher limit
posts_query(@limit * Search.burst_factor)
elsif @search_context.is_a?(Topic)
posts_query(@limit).where('posts.post_number = 1 OR posts.topic_id = ?', @search_context.id)
elsif @include_blurbs
posts_query(@limit).where('posts.post_number = 1')
end
# If no context, do an aggregate search
return aggregate_search if posts.nil?
posts.each do |p|
@results.add_result(SearchResult.from_post(p, @search_context, @term, @include_blurbs))
if @search_context.is_a?(Topic)
posts = posts_query(@limit).where('posts.topic_id = ?', @search_context.id).includes(:topic => :category)
posts.each do |post|
@results.add(post)
end
else
aggregate_search
end
end
end

View File

@ -1,36 +1,56 @@
require 'sanitize'
class Search
class GroupedSearchResults
attr_reader :topic_count, :type_filter
def initialize(type_filter)
@type_filter = type_filter
@by_type = {}
@topic_count = 0
end
include ActiveModel::Serialization
def topic_ids
topic_results = @by_type[:topic]
return Set.new if topic_results.blank?
return topic_results.results.map{|r| r.topic_id}
end
def as_json(options = nil)
@by_type.values.map do |grouped_result|
grouped_result.as_json
class TextHelper
extend ActionView::Helpers::TextHelper
def self.sanitize(text)
# we run through sanitize at the end so it does not matter
text
end
end
def add_result(result)
grouped_result = @by_type[result.type] || (@by_type[result.type] = SearchResultType.new(result.type))
attr_reader :type_filter,
:posts, :categories, :users,
:more_posts, :more_categories, :more_users,
:term, :search_context, :include_blurbs
# 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)
def initialize(type_filter, term, search_context, include_blurbs)
@type_filter = type_filter
@term = term
@search_context = search_context
@include_blurbs = include_blurbs
@posts = []
@categories = []
@users = []
end
grouped_result.add(result)
def blurb(post)
cooked = SearchObserver::HtmlScrubber.scrub(post.cooked).squish
terms = @term.split(/\s+/)
blurb = TextHelper.excerpt(cooked, terms.first, radius: 100)
# TODO highlight term
# terms.each do |term|
# blurb = TextHelper.highlight(blurb, term)
# end
blurb = TextHelper.truncate(cooked, length: 200) if blurb.blank?
Sanitize.clean(blurb)
end
def add(object)
type = object.class.to_s.downcase.pluralize
if !@type_filter.present? && send(type).length == Search.per_facet
instance_variable_set("@more_#{type}".to_sym, true)
else
grouped_result.more = true
(send type) << object
end
end

View File

@ -7,7 +7,7 @@ class Search
extend ActionView::Helpers::TextHelper
end
attr_accessor :type, :id, :topic_id, :blurb
attr_accessor :type, :id, :topic_id, :blurb, :locked, :pinned
# Category attributes
attr_accessor :color, :text_color
@ -27,7 +27,9 @@ class Search
:uploaded_avatar_id,
:color,
:text_color,
:blurb
:blurb,
:locked,
:pinned
].each do |k|
val = send(k)
json[k] = val if val
@ -73,7 +75,8 @@ class Search
end
if include_blurbs
#add a blurb from the post to the search results
custom_blurb = blurb(p.raw, term)
cooked = SearchObserver::HtmlScrubber.scrub(p.cooked).squish
custom_blurb = blurb(cooked, term)
end
if p.post_number == 1
# we want the topic link when it's the OP

View File

@ -13,20 +13,6 @@ describe Search do
ActiveRecord::Base.observers.enable :search_observer
end
def first_of_type(results, type)
return nil if results.blank?
results.each do |r|
return r[:results].first if r[:type] == type
end
nil
end
def result_ids_for_type(results, type)
results.find do |group|
group[:type] == type
end[:results].map {|r| r[:id]}
end
context 'post indexing observer' do
before do
@category = Fabricate(:category, name: 'america')
@ -56,11 +42,8 @@ describe Search do
@indexed = @user.user_search_data.search_data
end
it "should pick up on username" do
it "should pick up on data" do
@indexed.should =~ /fred/
end
it "should pick up on name" do
@indexed.should =~ /jone/
end
end
@ -77,43 +60,36 @@ describe Search do
end
it 'returns something blank on a nil search' do
ActiveRecord::Base.expects(:exec_sql).never
Search.new(nil).execute.should be_blank
end
it 'does not search when the search term is too small' do
ActiveRecord::Base.expects(:exec_sql).never
Search.new('evil', min_search_term_length: 5).execute.should be_blank
Search.execute('evil', min_search_term_length: 5)
end
it 'escapes non alphanumeric characters' do
Search.new('foo :!$);}]>@\#\"\'').execute.should be_blank # There are at least three levels of sanitation for Search.query!
Search.execute('foo :!$);}]>@\#\"\'').posts.length.should == 0 # There are at least three levels of sanitation for Search.query!
end
it "doesn't raise an error when single quotes are present" do
Search.new("'hello' world").execute.should be_blank # There are at least three levels of sanitation for Search.query!
Search.execute("'hello' world").posts.length.should == 0 # There are at least three levels of sanitation for Search.query!
end
it 'works when given two terms with spaces' do
lambda { Search.new('evil trout').execute }.should_not raise_error
lambda { Search.execute('evil trout') }.should_not raise_error
end
context 'users' do
let!(:user) { Fabricate(:user) }
let(:result) { first_of_type( Search.new('bruce', type_filter: 'user').execute, 'user') }
let(:result) { Search.execute('bruce', type_filter: 'user') }
it 'returns a result' do
result.should be_present
result[:title].should == user.username
result[:avatar_template].should_not be_nil
result[:url].should == "/users/#{user.username_lower}"
result.users.length.should == 1
result.users[0].id.should == user.id
end
end
context 'topics' do
let(:topic) { Fabricate(:topic) }
let(:post) { Fabricate(:post) }
let(:topic) { post.topic}
context 'search within topic' do
@ -139,76 +115,62 @@ describe Search do
# update posts_count
topic.reload
results = Search.new('posting', search_context: post1.topic).execute.find do |r|
r[:type] == "topic"
end[:results]
results.map{|r| r[:id]}.should == [
post1.topic_id,
"_#{post2.id}",
"_#{post3.id}",
"_#{post4.id}"]
results = Search.execute('posting', search_context: post1.topic)
results.posts.map(&:id).should == [post1.id, post2.id, post3.id, post4.id]
# stop words should work
results = Search.new('this', search_context: post1.topic).execute.find do |r|
r[:type] == "topic"
end[:results]
results.length.should == 4
results = Search.execute('this', search_context: post1.topic)
results.posts.length.should == 4
end
end
context 'searching the OP' do
let!(:post) { Fabricate(:post_with_long_raw_content, topic: topic, user: topic.user) }
let(:result) { first_of_type(Search.new('hundred', type_filter: 'topic', include_blurbs: true).execute, 'topic') }
let!(:post) { Fabricate(:post_with_long_raw_content) }
let(:result) { Search.execute('hundred', type_filter: 'topic', include_blurbs: true) }
it 'returns a result correctly' do
result.should be_present
result[:title].should == topic.title
result[:url].should == topic.relative_url
result[:blurb].should == TextHelper.excerpt(post.raw, 'hundred', radius: 100)
result.posts.length.should == 1
result.posts[0].id.should == post.id
end
end
context 'searching for a post' do
let!(:post) { Fabricate(:post, topic: topic, user: topic.user) }
let!(:reply) { Fabricate(:basic_reply, topic: topic, user: topic.user) }
let(:result) { first_of_type(Search.new('quote', type_filter: 'topic').execute, 'topic') }
let(:result) { Search.execute('quotes', type_filter: 'topic', include_blurbs: true) }
it 'returns the post' do
result.should be_present
result[:title].should == topic.title
result[:url].should == topic.relative_url + "/2"
result[:blurb].should == "this reply has no quotes"
result.posts.length.should == 1
p = result.posts[0]
p.topic.id.should == topic.id
p.id.should == reply.id
result.blurb(p).should == "this reply has no quotes"
end
end
context "search for a topic by id" do
let(:result) { first_of_type(Search.new(topic.id, type_filter: 'topic', search_for_id: true, min_search_term_length: 1).execute, 'topic') }
let(:result) { Search.execute(topic.id, type_filter: 'topic', search_for_id: true, min_search_term_length: 1) }
it 'returns the topic' do
result.should be_present
result[:title].should == topic.title
result[:url].should == topic.relative_url
result.posts.length.should == 1
result.posts.first.id.should == post.id
end
end
context "search for a topic by url" do
let(:result) { first_of_type(Search.new(topic.relative_url, search_for_id: true, type_filter: 'topic').execute, 'topic') }
let(:result) { Search.execute(topic.relative_url, search_for_id: true, type_filter: 'topic')}
it 'returns the topic' do
result.should be_present
result[:title].should == topic.title
result[:url].should == topic.relative_url
result.posts.length.should == 1
result.posts.first.id.should == post.id
end
end
context 'security' do
let!(:post) { Fabricate(:post, topic: topic, user: topic.user) }
def result(current_user)
first_of_type(Search.new('hello', guardian: current_user).execute, 'topic')
Search.execute('hello', guardian: current_user)
end
it 'secures results correctly' do
@ -220,9 +182,9 @@ describe Search do
category.set_permissions(:staff => :full)
category.save
result(nil).should_not be_present
result(Fabricate(:user)).should_not be_present
result(Fabricate(:admin)).should be_present
result(nil).posts.should_not be_present
result(Fabricate(:user)).posts.should_not be_present
result(Fabricate(:admin)).posts.should be_present
end
end
@ -236,30 +198,27 @@ describe Search do
end
}
let!(:post) {Fabricate(:post, topic: cyrillic_topic, user: cyrillic_topic.user)}
let(:result) { first_of_type(Search.new('запись').execute, 'topic') }
let(:result) { Search.execute('запись') }
it 'finds something when given cyrillic query' do
result.should be_present
result.posts.should be_present
end
end
context 'categories' do
let!(:category) { Fabricate(:category) }
def result
first_of_type(Search.new('amazing').execute, 'category')
def search
Search.execute(category.name)
end
it 'returns the correct result' do
r = result
r.should be_present
r[:title].should == category.name
r[:url].should == "/category/#{category.slug}"
search.categories.should be_present
category.set_permissions({})
category.save
result.should_not be_present
search.categories.should_not be_present
end
end
@ -272,21 +231,23 @@ describe Search do
context 'user filter' do
let(:results) { Search.new('amazing', type_filter: 'user').execute }
let(:results) { Search.execute('amazing', type_filter: 'user') }
it "returns a user result" do
results.detect {|r| r[:type] == 'user'}.should be_present
results.detect {|r| r[:type] == 'category'}.should be_blank
results.categories.length.should == 0
results.posts.length.should == 0
results.users.length.should == 1
end
end
context 'category filter' do
let(:results) { Search.new('amazing', type_filter: 'category').execute }
let(:results) { Search.execute('amazing', type_filter: 'category') }
it "returns a category result" do
results.detect {|r| r[:type] == 'user'}.should be_blank
results.detect {|r| r[:type] == 'category'}.should be_present
results.categories.length.should == 1
results.posts.length.should == 0
results.users.length.should == 0
end
end
@ -295,27 +256,30 @@ describe Search do
context 'search_context' do
context 'user as a search context' do
let(:coding_horror) { Fabricate(:coding_horror) }
it 'can find a user when using search context' do
Given!(:post) { Fabricate(:post) }
Given!(:coding_horror_post) { Fabricate(:post, user: coding_horror )}
When(:search_user) { Search.new('hello', search_context: post.user).execute }
coding_horror = Fabricate(:coding_horror)
post = Fabricate(:post)
# should find topic created by searched user first
Then { first_of_type(search_user, 'topic')[:id].should == post.topic_id }
Fabricate(:post, user: coding_horror)
result = Search.execute('hello', search_context: post.user)
result.posts.first.topic_id = post.topic_id
result.posts.length.should == 1
end
context 'category as a search context' do
let(:category) { Fabricate(:category) }
let(:topic) { Fabricate(:topic, category: category) }
let(:topic_no_cat) { Fabricate(:topic) }
it 'can use category as a search context' do
category = Fabricate(:category)
topic = Fabricate(:topic, category: category)
topic_no_cat = Fabricate(:topic)
Given!(:post) { Fabricate(:post, topic: topic, user: topic.user ) }
Given!(:another_post) { Fabricate(:post, topic: topic_no_cat, user: topic.user ) }
When(:search_cat) { Search.new('hello', search_context: category).execute }
# should find topic in searched category first
Then { first_of_type(search_cat, 'topic')[:id].should == topic.id }
post = Fabricate(:post, topic: topic, user: topic.user )
_another_post = Fabricate(:post, topic: topic_no_cat, user: topic.user )
search = Search.execute('hello', search_context: category)
search.posts.length.should == 1
search.posts.first.id.should == post.id
end
end
@ -330,10 +294,10 @@ describe Search do
it 'finds chinese topic based on title' do
SiteSetting.default_locale = 'zh_TW'
topic = Fabricate(:topic, title: 'My Title Discourse社区指南')
Fabricate(:post, topic: topic)
post = Fabricate(:post, topic: topic)
Search.new('社区指南').execute[0][:results][0][:id].should == topic.id
Search.new('指南').execute[0][:results][0][:id].should == topic.id
Search.execute('社区指南').posts.first.id.should == post.id
Search.execute('指南').posts.first.id.should == post.id
end
end

View File

@ -2,6 +2,25 @@ require 'spec_helper'
describe SearchController do
context "integration" do
before do
ActiveRecord::Base.observers.enable :search_observer
end
it "can search correctly" do
my_post = Fabricate(:post, raw: 'this is my really awesome post')
xhr :get, :query, term: 'awesome', include_blurb: true
response.should be_success
data = JSON.parse(response.body)
data['posts'][0]['id'].should == my_post.id
data['posts'][0]['blurb'].should == 'this is my really awesome post'
data['topics'][0]['id'].should == my_post.topic_id
end
end
let(:search_context) { {type: 'user', id: 'eviltrout'} }
context "basics" do