Commit cc17d60a3936837afdc7a2dfcefe51acbe27eb5f
1 parent
cdde5e0b
Exists in
master
and in
29 other branches
ActionItem253: fixed a bug where article categorized in more than one categories…
… in a given subtree is returned twice when searchin for the top category git-svn-id: https://svn.colivre.coop.br/svn/noosfero/trunk@1716 3f533792-8f58-4932-b0fe-aaf55b0a4547
Showing
4 changed files
with
96 additions
and
29 deletions
Show diff stats
app/controllers/public/search_controller.rb
| @@ -38,7 +38,7 @@ class SearchController < ApplicationController | @@ -38,7 +38,7 @@ class SearchController < ApplicationController | ||
| 38 | def action_category | 38 | def action_category |
| 39 | @recent_articles = @finder.recent('articles') | 39 | @recent_articles = @finder.recent('articles') |
| 40 | @recent_comments = @finder.recent('comments') | 40 | @recent_comments = @finder.recent('comments') |
| 41 | - @most_commented_articles = category.most_commented_articles | 41 | + @most_commented_articles = @finder.most_commented_articles |
| 42 | end | 42 | end |
| 43 | alias :action_region :action_category | 43 | alias :action_region :action_category |
| 44 | 44 | ||
| @@ -76,7 +76,7 @@ class SearchController < ApplicationController | @@ -76,7 +76,7 @@ class SearchController < ApplicationController | ||
| 76 | 76 | ||
| 77 | # view the summary of one category | 77 | # view the summary of one category |
| 78 | def category_index | 78 | def category_index |
| 79 | - send('action_' + @category.class.name.underscore) | 79 | + send('action_' + category.class.name.underscore) |
| 80 | end | 80 | end |
| 81 | attr_reader :category | 81 | attr_reader :category |
| 82 | 82 |
app/models/category_finder.rb
| @@ -24,33 +24,49 @@ class CategoryFinder | @@ -24,33 +24,49 @@ class CategoryFinder | ||
| 24 | end | 24 | end |
| 25 | 25 | ||
| 26 | def products(query='*', options={}) | 26 | def products(query='*', options={}) |
| 27 | - Product.find_by_contents(query, {}, {:select => 'products.*', :joins => 'inner join categories_profiles on products.enterprise_id = categories_profiles.profile_id', :conditions => ['categories_profiles.category_id in (?)', category_ids]}.merge!(options)) | 27 | + find_in_categorized(Product, query, options) |
| 28 | end | 28 | end |
| 29 | 29 | ||
| 30 | def comments(query='*', options={}) | 30 | def comments(query='*', options={}) |
| 31 | - Comment.find_by_contents(query, {}, {:select => 'comments.*', :joins => 'inner join articles_categories on articles_categories.article_id = comments.article_id', :conditions => ['articles_categories.category_id in (?)', category_ids]}.merge!(options)) | 31 | + find_in_categorized(Comment, query, options) |
| 32 | end | 32 | end |
| 33 | 33 | ||
| 34 | def recent(asset, limit = 10) | 34 | def recent(asset, limit = 10) |
| 35 | - table = case asset | ||
| 36 | - when 'people', 'communities', 'enterprises' | ||
| 37 | - 'profiles' | ||
| 38 | - else | ||
| 39 | - asset | ||
| 40 | - end | ||
| 41 | - | ||
| 42 | - with_options :limit => limit, :order => "created_at desc, #{table}.id desc" do |finder| | ||
| 43 | - finder.send(asset, '*', {}) | ||
| 44 | - end | 35 | + asset_class(asset).find(:all, options_for_find(asset_class(asset), {:limit => limit, :order => "created_at desc, #{asset_table(asset)}.id desc"})) |
| 45 | end | 36 | end |
| 46 | 37 | ||
| 47 | def count(asset) | 38 | def count(asset) |
| 48 | - send(asset).size | 39 | + asset_class(asset).count(:all, options_for_find(asset_class(asset))) |
| 40 | + end | ||
| 41 | + | ||
| 42 | + def most_commented_articles(limit=10) | ||
| 43 | + Article.find(:all, options_for_find(Article, :limit => limit, :order => 'comments_count DESC')) | ||
| 49 | end | 44 | end |
| 50 | 45 | ||
| 51 | protected | 46 | protected |
| 52 | 47 | ||
| 53 | def find_in_categorized(klass, query, options={}) | 48 | def find_in_categorized(klass, query, options={}) |
| 54 | - klass.find_by_contents(query, {}, {:include => 'categories', :conditions => ['categories.id IN (?)', category_ids]}.merge!(options)) | 49 | + klass.find_by_contents(query, {}, options_for_find(klass, options)).uniq |
| 50 | + end | ||
| 51 | + | ||
| 52 | + def options_for_find(klass, options={}) | ||
| 53 | + case klass.name | ||
| 54 | + when 'Comment' | ||
| 55 | + {:select => 'distinct comments.*', :joins => 'inner join articles_categories on articles_categories.article_id = comments.article_id', :conditions => ['articles_categories.category_id in (?)', category_ids]}.merge!(options) | ||
| 56 | + when 'Product' | ||
| 57 | + {:select => 'distinct products.*', :joins => 'inner join categories_profiles on products.enterprise_id = categories_profiles.profile_id', :conditions => ['categories_profiles.category_id in (?)', category_ids]}.merge!(options) | ||
| 58 | + when 'Article', 'Person', 'Community', 'Enterprise' | ||
| 59 | + {:include => 'categories', :conditions => ['categories.id IN (?)', category_ids]}.merge!(options) | ||
| 60 | + else | ||
| 61 | + raise "unreconized class #{klass.name}" | ||
| 62 | + end | ||
| 63 | + end | ||
| 64 | + | ||
| 65 | + def asset_class(asset) | ||
| 66 | + asset.to_s.singularize.camelize.constantize | ||
| 67 | + end | ||
| 68 | + | ||
| 69 | + def asset_table(asset) | ||
| 70 | + asset_class(asset).table_name | ||
| 55 | end | 71 | end |
| 56 | end | 72 | end |
test/functional/search_controller_test.rb
| @@ -31,9 +31,9 @@ class SearchControllerTest < Test::Unit::TestCase | @@ -31,9 +31,9 @@ class SearchControllerTest < Test::Unit::TestCase | ||
| 31 | 31 | ||
| 32 | should 'search with filtered query' do | 32 | should 'search with filtered query' do |
| 33 | @controller.expects(:locale).returns('pt_BR').at_least_once | 33 | @controller.expects(:locale).returns('pt_BR').at_least_once |
| 34 | - @controller.expects(:search).with(anything, 'carne vaca').at_least_once | ||
| 35 | - @controller.expects(:search).with(anything, 'a carne da vaca').never | ||
| 36 | get 'index', :query => 'a carne da vaca' | 34 | get 'index', :query => 'a carne da vaca' |
| 35 | + | ||
| 36 | + assert_equal 'carne vaca', assigns('filtered_query') | ||
| 37 | end | 37 | end |
| 38 | 38 | ||
| 39 | should 'search only in specified types of content' do | 39 | should 'search only in specified types of content' do |
| @@ -250,7 +250,7 @@ class SearchControllerTest < Test::Unit::TestCase | @@ -250,7 +250,7 @@ class SearchControllerTest < Test::Unit::TestCase | ||
| 250 | p2 = create_user('test2').person | 250 | p2 = create_user('test2').person |
| 251 | 251 | ||
| 252 | get :assets, :asset => 'people', :category_path => [ 'my-category' ] | 252 | get :assets, :asset => 'people', :category_path => [ 'my-category' ] |
| 253 | - assert_equal [p1], assigns(:results)[:people].instance_variable_get('@results') | 253 | + assert_equal [p1], assigns(:results)[:people] |
| 254 | end | 254 | end |
| 255 | 255 | ||
| 256 | should 'find communities' do | 256 | should 'find communities' do |
| @@ -293,7 +293,7 @@ class SearchControllerTest < Test::Unit::TestCase | @@ -293,7 +293,7 @@ class SearchControllerTest < Test::Unit::TestCase | ||
| 293 | 293 | ||
| 294 | get :assets, :asset => 'communities', :category_path => [ 'my-category' ] | 294 | get :assets, :asset => 'communities', :category_path => [ 'my-category' ] |
| 295 | 295 | ||
| 296 | - assert_equal [c3, c1], assigns(:results)[:communities].instance_variable_get('@results') | 296 | + assert_equal [c3, c1], assigns(:results)[:communities] |
| 297 | end | 297 | end |
| 298 | 298 | ||
| 299 | should 'find products' do | 299 | should 'find products' do |
| @@ -379,7 +379,7 @@ class SearchControllerTest < Test::Unit::TestCase | @@ -379,7 +379,7 @@ class SearchControllerTest < Test::Unit::TestCase | ||
| 379 | } | 379 | } |
| 380 | names.each do |thing,description| | 380 | names.each do |thing,description| |
| 381 | assert_tag :tag => 'input', :attributes => { :type => 'checkbox', :name => "find_in[]", :value => thing.to_s, :checked => 'checked' } | 381 | assert_tag :tag => 'input', :attributes => { :type => 'checkbox', :name => "find_in[]", :value => thing.to_s, :checked => 'checked' } |
| 382 | - assert_tag :tag => 'span', :content => description | 382 | + assert_tag :tag => 'label', :content => description |
| 383 | end | 383 | end |
| 384 | end | 384 | end |
| 385 | 385 | ||
| @@ -435,7 +435,10 @@ class SearchControllerTest < Test::Unit::TestCase | @@ -435,7 +435,10 @@ class SearchControllerTest < Test::Unit::TestCase | ||
| 435 | should 'list recent articles in the category' do | 435 | should 'list recent articles in the category' do |
| 436 | @controller.expects(:category).returns(@category).at_least_once | 436 | @controller.expects(:category).returns(@category).at_least_once |
| 437 | recent = [] | 437 | recent = [] |
| 438 | - @category.expects(:recent_articles).returns(recent) | 438 | + finder = CategoryFinder.new(@category) |
| 439 | + finder.expects(:recent).with('comments').returns(recent) | ||
| 440 | + finder.expects(:recent).with('articles').returns(recent) | ||
| 441 | + CategoryFinder.expects(:new).with(@category).returns(finder) | ||
| 439 | 442 | ||
| 440 | get :category_index, :category_path => [ 'my-category' ] | 443 | get :category_index, :category_path => [ 'my-category' ] |
| 441 | assert_same recent, assigns(:recent_articles) | 444 | assert_same recent, assigns(:recent_articles) |
| @@ -444,7 +447,10 @@ class SearchControllerTest < Test::Unit::TestCase | @@ -444,7 +447,10 @@ class SearchControllerTest < Test::Unit::TestCase | ||
| 444 | should 'list recent comments in the category' do | 447 | should 'list recent comments in the category' do |
| 445 | @controller.expects(:category).returns(@category).at_least_once | 448 | @controller.expects(:category).returns(@category).at_least_once |
| 446 | recent = [] | 449 | recent = [] |
| 447 | - @category.expects(:recent_comments).returns(recent) | 450 | + finder = CategoryFinder.new(@category) |
| 451 | + finder.expects(:recent).with('comments').returns(recent) | ||
| 452 | + finder.expects(:recent).with('articles').returns(recent) | ||
| 453 | + CategoryFinder.expects(:new).with(@category).returns(finder) | ||
| 448 | 454 | ||
| 449 | get :category_index, :category_path => [ 'my-category' ] | 455 | get :category_index, :category_path => [ 'my-category' ] |
| 450 | assert_same recent, assigns(:recent_comments) | 456 | assert_same recent, assigns(:recent_comments) |
| @@ -453,7 +459,9 @@ class SearchControllerTest < Test::Unit::TestCase | @@ -453,7 +459,9 @@ class SearchControllerTest < Test::Unit::TestCase | ||
| 453 | should 'list most commented articles in the category' do | 459 | should 'list most commented articles in the category' do |
| 454 | @controller.expects(:category).returns(@category).at_least_once | 460 | @controller.expects(:category).returns(@category).at_least_once |
| 455 | most_commented = [] | 461 | most_commented = [] |
| 456 | - @category.expects(:most_commented_articles).returns(most_commented) | 462 | + finder = CategoryFinder.new(@category) |
| 463 | + finder.expects(:most_commented_articles).returns(most_commented) | ||
| 464 | + CategoryFinder.expects(:new).with(@category).returns(finder) | ||
| 457 | 465 | ||
| 458 | get :category_index, :category_path => [ 'my-category' ] | 466 | get :category_index, :category_path => [ 'my-category' ] |
| 459 | assert_same most_commented, assigns(:most_commented_articles) | 467 | assert_same most_commented, assigns(:most_commented_articles) |
| @@ -509,7 +517,7 @@ class SearchControllerTest < Test::Unit::TestCase | @@ -509,7 +517,7 @@ class SearchControllerTest < Test::Unit::TestCase | ||
| 509 | child = Category.create!(:name => "Child Category", :environment => Environment.default, :parent => parent) | 517 | child = Category.create!(:name => "Child Category", :environment => Environment.default, :parent => parent) |
| 510 | 518 | ||
| 511 | get :index, :category_path => [ 'parent-category', 'child-category' ], :query => 'a sample search' | 519 | get :index, :category_path => [ 'parent-category', 'child-category' ], :query => 'a sample search' |
| 512 | - assert_tag :tag => 'h2', :content => /Search results for "a sample search" in "Child Category"/ | 520 | + assert_tag :tag => 'h1', :content => /Search results for "a sample search" in "Child Category"/ |
| 513 | end | 521 | end |
| 514 | 522 | ||
| 515 | should 'search in categoty hierachy' do | 523 | should 'search in categoty hierachy' do |
test/unit/category_finder_test.rb
| @@ -88,9 +88,9 @@ class CategoryFinderTest < ActiveSupport::TestCase | @@ -88,9 +88,9 @@ class CategoryFinderTest < ActiveSupport::TestCase | ||
| 88 | end | 88 | end |
| 89 | 89 | ||
| 90 | should 'search in category hierarchy' do | 90 | should 'search in category hierarchy' do |
| 91 | - parent = Category.create!(:name => 'child category', :environment => Environment.default) | 91 | + parent = Category.create!(:name => 'parent category', :environment => Environment.default) |
| 92 | child = Category.create!(:name => 'child category', :environment => Environment.default, :parent => parent) | 92 | child = Category.create!(:name => 'child category', :environment => Environment.default, :parent => parent) |
| 93 | - p1 = create_user('people_1').person; p1.name = 'a beautiful person'; p1.categories << parent; p1.save! | 93 | + p1 = create_user('people_1').person; p1.name = 'a beautiful person'; p1.categories << child; p1.save! |
| 94 | 94 | ||
| 95 | f = CategoryFinder.new(parent) | 95 | f = CategoryFinder.new(parent) |
| 96 | assert_includes f.people, p1 | 96 | assert_includes f.people, p1 |
| @@ -105,8 +105,8 @@ class CategoryFinderTest < ActiveSupport::TestCase | @@ -105,8 +105,8 @@ class CategoryFinderTest < ActiveSupport::TestCase | ||
| 105 | ent1 = Enterprise.create!(:name => 'teste1', :identifier => 'teste1', :categories => [@category]) | 105 | ent1 = Enterprise.create!(:name => 'teste1', :identifier => 'teste1', :categories => [@category]) |
| 106 | ent2 = Enterprise.create!(:name => 'teste2', :identifier => 'teste2', :categories => [@category]) | 106 | ent2 = Enterprise.create!(:name => 'teste2', :identifier => 'teste2', :categories => [@category]) |
| 107 | recent = @finder.recent('enterprises', 1) | 107 | recent = @finder.recent('enterprises', 1) |
| 108 | - assert_includes recent, ent1 | ||
| 109 | - assert_not_includes recent, ent2 | 108 | + assert_includes recent, ent2 |
| 109 | + assert_not_includes recent, ent1 | ||
| 110 | end | 110 | end |
| 111 | 111 | ||
| 112 | should 'count entrprises' do | 112 | should 'count entrprises' do |
| @@ -114,4 +114,47 @@ class CategoryFinderTest < ActiveSupport::TestCase | @@ -114,4 +114,47 @@ class CategoryFinderTest < ActiveSupport::TestCase | ||
| 114 | ent1 = Enterprise.create!(:name => 'teste1', :identifier => 'teste1', :categories => [@category]) | 114 | ent1 = Enterprise.create!(:name => 'teste1', :identifier => 'teste1', :categories => [@category]) |
| 115 | assert_equal count+1, @finder.count('enterprises') | 115 | assert_equal count+1, @finder.count('enterprises') |
| 116 | end | 116 | end |
| 117 | + | ||
| 118 | + should 'not list more people than limit' do | ||
| 119 | + p1 = create_user('test1').person; p1.categories << @category | ||
| 120 | + p2 = create_user('test2').person; p2.categories << @category | ||
| 121 | + recent = @finder.recent('people', 1) | ||
| 122 | + assert_includes recent, p2 | ||
| 123 | + assert_not_includes recent, p1 | ||
| 124 | + end | ||
| 125 | + | ||
| 126 | + should 'list recent articles' do | ||
| 127 | + person = create_user('teste').person | ||
| 128 | + art1 = person.articles.build(:name => 'an article to be found'); art1.categories << @category; art1.save! | ||
| 129 | + | ||
| 130 | + art2 = person.articles.build(:name => 'another article to be found'); art2.categories << @category; art2.save! | ||
| 131 | + | ||
| 132 | + result = @finder.recent('articles', 1) | ||
| 133 | + assert_includes result, art2 | ||
| 134 | + assert_not_includes result, art1 | ||
| 135 | + end | ||
| 136 | + | ||
| 137 | + should 'not return the same result twice' do | ||
| 138 | + parent = Category.create!(:name => 'parent category', :environment => Environment.default) | ||
| 139 | + child = Category.create!(:name => 'child category', :environment => Environment.default, :parent => parent) | ||
| 140 | + p1 = create_user('people_1').person; p1.name = 'a beautiful person'; p1.categories << child; p1.save! | ||
| 141 | + p1.categories << parent; p1.save! | ||
| 142 | + | ||
| 143 | + f = CategoryFinder.new(parent) | ||
| 144 | + result = f.people | ||
| 145 | + assert_equal 1, result.size | ||
| 146 | + end | ||
| 147 | + | ||
| 148 | + should 'return most commented articles' do | ||
| 149 | + Article.delete_all | ||
| 150 | + | ||
| 151 | + person = create_user('testuser').person | ||
| 152 | + articles = (1..4).map {|n| a = person.articles.build(:name => "art #{n}", :categories => [@category]); a.save!; a } | ||
| 153 | + | ||
| 154 | + 2.times { articles[0].comments.build(:title => 'test', :body => 'asdsad', :author => person).save! } | ||
| 155 | + 4.times { articles[1].comments.build(:title => 'test', :body => 'asdsad', :author => person).save! } | ||
| 156 | + | ||
| 157 | + # should respect the order (more commented comes first) | ||
| 158 | + assert_equal [articles[1], articles[0]], @finder.most_commented_articles(2) | ||
| 159 | + end | ||
| 117 | end | 160 | end |