Commit d07e3bd16b6055a3ba50703b0b02103d7ea5ffab
1 parent
3e5043a1
Exists in
master
and in
28 other branches
ActionItem509: implemented pagination infrastructure only left put views helpers
git-svn-id: https://svn.colivre.coop.br/svn/noosfero/trunk@2137 3f533792-8f58-4932-b0fe-aaf55b0a4547
Showing
8 changed files
with
87 additions
and
29 deletions
Show diff stats
app/controllers/public/search_controller.rb
| @@ -89,20 +89,20 @@ class SearchController < ApplicationController | @@ -89,20 +89,20 @@ class SearchController < ApplicationController | ||
| 89 | def load_product_categories_menu(asset) | 89 | def load_product_categories_menu(asset) |
| 90 | @results[asset].uniq! | 90 | @results[asset].uniq! |
| 91 | @categories_menu = ProductCategory.menu_categories(@product_category, environment).map do |cat| | 91 | @categories_menu = ProductCategory.menu_categories(@product_category, environment).map do |cat| |
| 92 | - hits = @finder.count(:products, @filtered_query, calculate_find_options(asset, nil, cat, @region, params[:radius])) | 92 | + hits = @finder.count(:products, @filtered_query, calculate_find_options(asset, nil, nil, cat, @region, params[:radius])) |
| 93 | childs = [] | 93 | childs = [] |
| 94 | # REFACTOR DUPLICATED CODE inner loop doing the same thing that outter loop | 94 | # REFACTOR DUPLICATED CODE inner loop doing the same thing that outter loop |
| 95 | childs = cat.children.map do |child| | 95 | childs = cat.children.map do |child| |
| 96 | - child_hits = @finder.count(:products, @filtered_query, calculate_find_options(asset, nil, child, @region, params[:radius])) | 96 | + child_hits = @finder.count(:products, @filtered_query, calculate_find_options(asset, nil, nil, child, @region, params[:radius])) |
| 97 | [child, child_hits] | 97 | [child, child_hits] |
| 98 | end.select{|child, child_hits| child_hits > 0 } | 98 | end.select{|child, child_hits| child_hits > 0 } |
| 99 | [cat, hits, childs] | 99 | [cat, hits, childs] |
| 100 | end.select{|cat, hits| hits > 0 } | 100 | end.select{|cat, hits| hits > 0 } |
| 101 | end | 101 | end |
| 102 | 102 | ||
| 103 | - def calculate_find_options(asset, limit, product_category, region, radius) | 103 | + def calculate_find_options(asset, limit, page, product_category, region, radius) |
| 104 | 104 | ||
| 105 | - result = { :limit => limit, :product_category => product_category} | 105 | + result = { :product_category => product_category, :per_page => limit, :page => page } |
| 106 | if [:enterprises, :people].include?(asset) && region | 106 | if [:enterprises, :people].include?(asset) && region |
| 107 | result.merge!(:within => radius, :region => region.id) | 107 | result.merge!(:within => radius, :region => region.id) |
| 108 | end | 108 | end |
| @@ -110,6 +110,12 @@ class SearchController < ApplicationController | @@ -110,6 +110,12 @@ class SearchController < ApplicationController | ||
| 110 | result | 110 | result |
| 111 | end | 111 | end |
| 112 | 112 | ||
| 113 | + # limit the number of results per page | ||
| 114 | + # TODO: dont hardcore like this | ||
| 115 | + def limit | ||
| 116 | + 10 | ||
| 117 | + end | ||
| 118 | + | ||
| 113 | public | 119 | public |
| 114 | 120 | ||
| 115 | include SearchHelper | 121 | include SearchHelper |
| @@ -125,9 +131,6 @@ class SearchController < ApplicationController | @@ -125,9 +131,6 @@ class SearchController < ApplicationController | ||
| 125 | [ :events, N_('Events') ] | 131 | [ :events, N_('Events') ] |
| 126 | ] | 132 | ] |
| 127 | 133 | ||
| 128 | - # TODO don't hardcode like this >:-( | ||
| 129 | - LIST_LIMIT = 10 | ||
| 130 | - | ||
| 131 | def complete_region | 134 | def complete_region |
| 132 | # FIXME this logic should be in the model | 135 | # FIXME this logic should be in the model |
| 133 | @regions = Region.find(:all, :conditions => [ '(name like ? or name like ?) and lat is not null and lng is not null', '%' + params[:region][:name] + '%', '%' + params[:region][:name].capitalize + '%' ]) | 136 | @regions = Region.find(:all, :conditions => [ '(name like ? or name like ?) and lat is not null and lng is not null', '%' + params[:region][:name] + '%', '%' + params[:region][:name].capitalize + '%' ]) |
| @@ -145,16 +148,11 @@ class SearchController < ApplicationController | @@ -145,16 +148,11 @@ class SearchController < ApplicationController | ||
| 145 | # how many assets we are searching for? | 148 | # how many assets we are searching for? |
| 146 | number_of_result_assets = @searching.values.select{|v| v}.size | 149 | number_of_result_assets = @searching.values.select{|v| v}.size |
| 147 | 150 | ||
| 148 | - # apply limit when searching for only one type of asset | ||
| 149 | - limit = (number_of_result_assets == 1) ? nil: LIST_LIMIT | ||
| 150 | - # apply limit to all searches | ||
| 151 | -# limit = nil | ||
| 152 | - | ||
| 153 | @results = {} | 151 | @results = {} |
| 154 | @names = {} | 152 | @names = {} |
| 155 | 153 | ||
| 156 | SEARCH_IN.select { |key,description| @searching[key] }.each do |key, description| | 154 | SEARCH_IN.select { |key,description| @searching[key] }.each do |key, description| |
| 157 | - @results[key] = @finder.find(key, @filtered_query, calculate_find_options(key, limit, @product_category, @region, params[:radius])) | 155 | + @results[key] = @finder.find(key, @filtered_query, calculate_find_options(key, limit, params[:page], @product_category, @region, params[:radius])) |
| 158 | @names[key] = gettext(description) | 156 | @names[key] = gettext(description) |
| 159 | end | 157 | end |
| 160 | 158 |
app/models/category_finder.rb
| @@ -7,8 +7,6 @@ class CategoryFinder | @@ -7,8 +7,6 @@ class CategoryFinder | ||
| 7 | 7 | ||
| 8 | attr_reader :category_id | 8 | attr_reader :category_id |
| 9 | 9 | ||
| 10 | - | ||
| 11 | - | ||
| 12 | def find(asset, query='', options={}) | 10 | def find(asset, query='', options={}) |
| 13 | @region = Region.find_by_id(options.delete(:region)) if options.has_key?(:region) | 11 | @region = Region.find_by_id(options.delete(:region)) if options.has_key?(:region) |
| 14 | if @region && options[:within] | 12 | if @region && options[:within] |
| @@ -17,10 +15,12 @@ class CategoryFinder | @@ -17,10 +15,12 @@ class CategoryFinder | ||
| 17 | options.delete(:within) | 15 | options.delete(:within) |
| 18 | end | 16 | end |
| 19 | 17 | ||
| 18 | + options = {:page => 1, :per_page => options.delete(:limit)}.merge(options) | ||
| 20 | if query.blank? | 19 | if query.blank? |
| 21 | - asset_class(asset).find(:all, options_for_find(asset_class(asset), {:order => "created_at desc, #{asset_table(asset)}.id desc"}.merge(options))) | 20 | + asset_class(asset).paginate(:all, options_for_find(asset_class(asset), {:order => "created_at desc, #{asset_table(asset)}.id desc"}.merge(options))) |
| 22 | else | 21 | else |
| 23 | - asset_class(asset).find_by_contents(query, {}, options_for_find(asset_class(asset), options)).uniq | 22 | + ferret_options = {:page => options.delete(:page), :per_page => options.delete(:per_page)} |
| 23 | + asset_class(asset).find_by_contents(query, ferret_options, options_for_find(asset_class(asset), options)).uniq | ||
| 24 | end | 24 | end |
| 25 | end | 25 | end |
| 26 | 26 | ||
| @@ -33,8 +33,10 @@ class CategoryFinder | @@ -33,8 +33,10 @@ class CategoryFinder | ||
| 33 | end | 33 | end |
| 34 | 34 | ||
| 35 | def count(asset, query='', options={}) | 35 | def count(asset, query='', options={}) |
| 36 | + # because will_paginate needs a page | ||
| 37 | + options = {:page => 1}.merge(options) | ||
| 36 | if query.blank? | 38 | if query.blank? |
| 37 | - find(asset, query, options).size | 39 | + find(asset, query, options).total_entries |
| 38 | else | 40 | else |
| 39 | find(asset, query, options).total_hits | 41 | find(asset, query, options).total_hits |
| 40 | end | 42 | end |
app/models/environment_finder.rb
| @@ -15,22 +15,25 @@ class EnvironmentFinder | @@ -15,22 +15,25 @@ class EnvironmentFinder | ||
| 15 | product_category = options.delete(:product_category) | 15 | product_category = options.delete(:product_category) |
| 16 | product_category_ids = product_category.map_traversal(&:id) if product_category | 16 | product_category_ids = product_category.map_traversal(&:id) if product_category |
| 17 | 17 | ||
| 18 | + options = {:page => 1, :per_page => options.delete(:limit)}.merge(options) | ||
| 18 | if query.blank? | 19 | if query.blank? |
| 19 | - if product_category && asset == :products | ||
| 20 | - @environment.send(asset).find(:all, options.merge({:order => 'created_at desc, id desc', :conditions => ['product_category_id in (?)', product_category_ids]})) | ||
| 21 | - elsif product_category && asset == :enterprises | ||
| 22 | - @environment.send(asset).find(:all, options.merge(:order => 'profiles.created_at desc, profiles.id desc', :include => 'products', :conditions => ['products.product_category_id in (?)', product_category_ids])) | ||
| 23 | - else | ||
| 24 | - @environment.send(asset).find( :all, options.merge( {:order => 'created_at desc, id desc'} ) ) | ||
| 25 | - end | 20 | + options = {:order => 'created_at desc, id desc'}.merge(options) |
| 21 | + if product_category && asset == :products | ||
| 22 | + @environment.send(asset).paginate(:all, options.merge(:conditions => ['product_category_id in (?)', product_category_ids])) | ||
| 23 | + elsif product_category && asset == :enterprises | ||
| 24 | + @environment.send(asset).paginate(:all, options.merge(:order => 'profiles.created_at desc, profiles.id desc', :include => 'products', :conditions => ['products.product_category_id in (?)', product_category_ids])) | ||
| 25 | + else | ||
| 26 | + @environment.send(asset).paginate(:all, options) | ||
| 27 | + end | ||
| 26 | else | 28 | else |
| 29 | + ferret_options = {:page => options.delete(:page), :per_page => options.delete(:per_page)} | ||
| 27 | if product_category && asset == :products | 30 | if product_category && asset == :products |
| 28 | # SECURITY no risk of SQL injection, since product_category_ids comes from trusted source | 31 | # SECURITY no risk of SQL injection, since product_category_ids comes from trusted source |
| 29 | - @environment.send(asset).find_by_contents(query, {}, options.merge({:conditions => 'product_category_id in (%s)' % product_category_ids.join(',') })) | 32 | + @environment.send(asset).find_by_contents(query, ferret_options, options.merge({:conditions => 'product_category_id in (%s)' % product_category_ids.join(',') })) |
| 30 | elsif product_category && asset == :enterprises | 33 | elsif product_category && asset == :enterprises |
| 31 | - @environment.send(asset).find_by_contents(query + " +extra_data_for_index:#{product_category.name}", {}, options) | 34 | + @environment.send(asset).find_by_contents(query + " +extra_data_for_index:#{product_category.name}", ferret_options, options) |
| 32 | else | 35 | else |
| 33 | - @environment.send(asset).find_by_contents(query, {}, options) | 36 | + @environment.send(asset).find_by_contents(query, ferret_options, options) |
| 34 | end | 37 | end |
| 35 | end | 38 | end |
| 36 | end | 39 | end |
| @@ -44,9 +47,11 @@ class EnvironmentFinder | @@ -44,9 +47,11 @@ class EnvironmentFinder | ||
| 44 | end | 47 | end |
| 45 | 48 | ||
| 46 | def count(asset, query = '', options = {}) | 49 | def count(asset, query = '', options = {}) |
| 50 | + # because will_paginate needs a page | ||
| 51 | + options = {:page => 1}.merge(options) | ||
| 47 | if query.blank? | 52 | if query.blank? |
| 48 | # SLOW | 53 | # SLOW |
| 49 | - find(asset, query, options).size | 54 | + find(asset, query, options).total_entries |
| 50 | else | 55 | else |
| 51 | find(asset, query, options).total_hits | 56 | find(asset, query, options).total_hits |
| 52 | end | 57 | end |
app/views/search/products.rhtml
config/environment.rb
| @@ -103,6 +103,7 @@ require 'acts_as_having_settings' | @@ -103,6 +103,7 @@ require 'acts_as_having_settings' | ||
| 103 | require 'acts_as_having_image' | 103 | require 'acts_as_having_image' |
| 104 | require 'hacked_after_create' | 104 | require 'hacked_after_create' |
| 105 | require 'sqlite_extension' | 105 | require 'sqlite_extension' |
| 106 | +require 'will_paginate' | ||
| 106 | 107 | ||
| 107 | # load a local configuration if present, but not under test environment. | 108 | # load a local configuration if present, but not under test environment. |
| 108 | if ENV['RAILS_ENV'] != 'test' | 109 | if ENV['RAILS_ENV'] != 'test' |
test/functional/search_controller_test.rb
| @@ -283,6 +283,16 @@ class SearchControllerTest < Test::Unit::TestCase | @@ -283,6 +283,16 @@ class SearchControllerTest < Test::Unit::TestCase | ||
| 283 | assert_equal [prod1], assigns(:results)[:products] | 283 | assert_equal [prod1], assigns(:results)[:products] |
| 284 | end | 284 | end |
| 285 | 285 | ||
| 286 | + should 'paginate enterprise listing' do | ||
| 287 | + @controller.expects(:limit).returns(1) | ||
| 288 | + ent1 = Enterprise.create!(:name => 'teste 1', :identifier => 'teste_1') | ||
| 289 | + ent2 = Enterprise.create!(:name => 'teste 2', :identifier => 'teste_2') | ||
| 290 | + | ||
| 291 | + get :assets, :asset => 'enterprises', :page => '2' | ||
| 292 | + | ||
| 293 | + assert_equal [ent1], assigns(:results)[:enterprises] # older on page 2 | ||
| 294 | + end | ||
| 295 | + | ||
| 286 | should 'display search results' do | 296 | should 'display search results' do |
| 287 | ent = Enterprise.create!(:name => 'display enterprise', :identifier => 'teste1') | 297 | ent = Enterprise.create!(:name => 'display enterprise', :identifier => 'teste1') |
| 288 | product = ent.products.create!(:name => 'display product') | 298 | product = ent.products.create!(:name => 'display product') |
test/unit/category_finder_test.rb
| @@ -151,6 +151,24 @@ class CategoryFinderTest < ActiveSupport::TestCase | @@ -151,6 +151,24 @@ class CategoryFinderTest < ActiveSupport::TestCase | ||
| 151 | assert_includes recent, ent2 | 151 | assert_includes recent, ent2 |
| 152 | assert_not_includes recent, ent1 | 152 | assert_not_includes recent, ent1 |
| 153 | end | 153 | end |
| 154 | + | ||
| 155 | + should 'paginate the list of more enterprises than limit' do | ||
| 156 | + ent1 = Enterprise.create!(:name => 'teste1', :identifier => 'teste1', :category_ids => [@category.id]) | ||
| 157 | + ent2 = Enterprise.create!(:name => 'teste2', :identifier => 'teste2', :category_ids => [@category.id]) | ||
| 158 | + | ||
| 159 | + assert_equal [ent2], @finder.find('enterprises', nil, :per_page => 1, :page => 1) | ||
| 160 | + assert_equal [ent1], @finder.find('enterprises', nil, :per_page => 1, :page => 2) | ||
| 161 | + end | ||
| 162 | + | ||
| 163 | + should 'paginate the list of more enterprises than limit with query' do | ||
| 164 | + ent1 = Enterprise.create!(:name => 'teste 1', :identifier => 'teste1', :category_ids => [@category.id]) | ||
| 165 | + ent2 = Enterprise.create!(:name => 'teste 2', :identifier => 'teste2', :category_ids => [@category.id]) | ||
| 166 | + | ||
| 167 | + p1 = @finder.find('enterprises', 'teste', :per_page => 1, :page => 1) | ||
| 168 | + p2 = @finder.find('enterprises', 'teste', :per_page => 1, :page => 2) | ||
| 169 | + | ||
| 170 | + assert (p1 == [ent1] && p2 == [ent2]) || (p1 == [ent2] && p2 == [ent1]) # consistent paging | ||
| 171 | + end | ||
| 154 | 172 | ||
| 155 | should 'count enterprises' do | 173 | should 'count enterprises' do |
| 156 | count = @finder.count('enterprises') | 174 | count = @finder.count('enterprises') |
test/unit/environment_finder_test.rb
| @@ -49,6 +49,27 @@ class EnvironmentFinderTest < ActiveSupport::TestCase | @@ -49,6 +49,27 @@ class EnvironmentFinderTest < ActiveSupport::TestCase | ||
| 49 | recent = finder.recent('enterprises', 1) | 49 | recent = finder.recent('enterprises', 1) |
| 50 | assert_includes recent, ent2 # newer | 50 | assert_includes recent, ent2 # newer |
| 51 | assert_not_includes recent, ent1 # older | 51 | assert_not_includes recent, ent1 # older |
| 52 | + end | ||
| 53 | + | ||
| 54 | + should 'paginate the list of more enterprises than limit' do | ||
| 55 | + finder = EnvironmentFinder.new(Environment.default) | ||
| 56 | + ent1 = Enterprise.create!(:name => 'teste1', :identifier => 'teste1') | ||
| 57 | + ent2 = Enterprise.create!(:name => 'teste2', :identifier => 'teste2') | ||
| 58 | + | ||
| 59 | + assert_equal [ent2], finder.find('enterprises', nil, :per_page => 1, :page => 1) #newer | ||
| 60 | + assert_equal [ent1], finder.find('enterprises', nil, :per_page => 1, :page => 2) #older | ||
| 61 | + end | ||
| 62 | + | ||
| 63 | + should 'paginate the list of more enterprises than limit with query' do | ||
| 64 | + finder = EnvironmentFinder.new(Environment.default) | ||
| 65 | + | ||
| 66 | + ent1 = Enterprise.create!(:name => 'teste 1', :identifier => 'teste1') | ||
| 67 | + ent2 = Enterprise.create!(:name => 'teste 2', :identifier => 'teste2') | ||
| 68 | + | ||
| 69 | + p1 = finder.find('enterprises', 'teste', :per_page => 1, :page => 1) | ||
| 70 | + p2 = finder.find('enterprises', 'teste', :per_page => 1, :page => 2) | ||
| 71 | + | ||
| 72 | + assert (p1 == [ent1] && p2 == [ent2]) || (p1 == [ent2] && p2 == [ent1]) | ||
| 52 | end | 73 | end |
| 53 | 74 | ||
| 54 | should 'count enterprises' do | 75 | should 'count enterprises' do |
| @@ -57,6 +78,7 @@ class EnvironmentFinderTest < ActiveSupport::TestCase | @@ -57,6 +78,7 @@ class EnvironmentFinderTest < ActiveSupport::TestCase | ||
| 57 | Enterprise.create!(:name => 'teste1', :identifier => 'teste1') | 78 | Enterprise.create!(:name => 'teste1', :identifier => 'teste1') |
| 58 | assert_equal count+1, finder.count('enterprises') | 79 | assert_equal count+1, finder.count('enterprises') |
| 59 | end | 80 | end |
| 81 | + | ||
| 60 | should 'count people' do | 82 | should 'count people' do |
| 61 | finder = EnvironmentFinder.new(Environment.default) | 83 | finder = EnvironmentFinder.new(Environment.default) |
| 62 | count = finder.count('people') | 84 | count = finder.count('people') |