The NodesController#report action was failing because it tried to render a raw array as if it was a paginated array.
This error was introduced in 8124c51b as part of #4625 "Reports index should only fetch the data it needs" which had an insufficient refactoring of the pagination. The existing tests didn't fail because the value was never rendered. The controller specs set the value, but didn't render it; while the view specs set their own paginated value and rendered it. Solution: * Added `integrate_views` to NodesController to force rendering of templates, which caused them to start failing as expected. * Moved #paginate_scope from ApplicationHelper to PaginateScopeHelper and included it in ApplicationController so that all controllers could add pagination to their result sets. * Added pagination to the NodesController#report action for HTML. * Refactored controllers and mixins to use #paginate_scope, rather than reinventing it themselves. * Eliminated unnecessary #paginate_scope :per_page option. * Eliminated unused #items_per_page helper. Signed-off-by: Igal Koshevoy <[email protected]> --- app/controllers/application_controller.rb | 1 + app/controllers/nodes_controller.rb | 2 +- app/controllers/reports_controller.rb | 2 +- app/helpers/application_helper.rb | 17 ------------ app/helpers/paginate_scope_helper.rb | 7 +++++ app/mixins/searchable_index.rb | 2 +- spec/controllers/nodes_controller_spec.rb | 12 +++++++- spec/helpers/application_helper_spec.rb | 38 ---------------------------- spec/helpers/paginate_scope_helper_spec.rb | 9 ++++++ 9 files changed, 30 insertions(+), 60 deletions(-) create mode 100644 app/helpers/paginate_scope_helper.rb create mode 100644 spec/helpers/paginate_scope_helper_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index c1d41eb..aeb31fb 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -3,6 +3,7 @@ class ApplicationController < ActionController::Base include InheritedResources::DSL + include PaginateScopeHelper helper :all # include all helpers, all the time protect_from_forgery # See ActionController::RequestForgeryProtection for details diff --git a/app/controllers/nodes_controller.rb b/app/controllers/nodes_controller.rb index 71da253..af75e54 100644 --- a/app/controllers/nodes_controller.rb +++ b/app/controllers/nodes_controller.rb @@ -44,7 +44,7 @@ class NodesController < InheritedResources::Base @node = Node.find_by_name!(params[:id]) @reports = @node.reports respond_to do |format| - format.html { render 'reports/index' } + format.html { @reports = paginate_scope(@reports); render 'reports/index' } format.yaml { render :text => @reports.to_yaml, :content_type => 'application/x-yaml' } format.json { render :json => @reports.to_json } end diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index 92e77a4..1a48456 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -18,7 +18,7 @@ class ReportsController < InheritedResources::Base def collection get_collection_ivar || set_collection_ivar( request.format == :html ? - end_of_association_chain.paginate(:page => params[:page]) : + paginate_scope(end_of_association_chain) : end_of_association_chain ) end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 9736770..cb48755 100755 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -136,23 +136,6 @@ module ApplicationHelper (count > 0 && measures_failure) ? 'failure' : 'success' end - ITEMS_PER_PAGE_DEFAULT = 50 - - # Return the number of items to put on a paginated page. Override the default - # value by setting the ITEMS_PER_PAGE environmental variable. - def items_per_page - ENV['ITEMS_PER_PAGE'].try(:to_i) || ITEMS_PER_PAGE_DEFAULT - end - - # Return a paginated +scope+. - def paginate_scope(scope, opts={}) - opts.reverse_merge!( - :page => params[:page], - :per_page => items_per_page - ) - return scope.paginate(opts) - end - # Return +collection+ of Puppet::Util::Log objects sorted by their severity level. def puppet_log_sorter(collection) collection.sort_by do |instance| diff --git a/app/helpers/paginate_scope_helper.rb b/app/helpers/paginate_scope_helper.rb new file mode 100644 index 0000000..5760e46 --- /dev/null +++ b/app/helpers/paginate_scope_helper.rb @@ -0,0 +1,7 @@ +module PaginateScopeHelper + # Return a paginated +scope+. + def paginate_scope(scope, opts={}) + opts.reverse_merge!(:page => params[:page]) + return scope.paginate(opts) + end +end diff --git a/app/mixins/searchable_index.rb b/app/mixins/searchable_index.rb index 623853d..d315c2e 100644 --- a/app/mixins/searchable_index.rb +++ b/app/mixins/searchable_index.rb @@ -3,7 +3,7 @@ module SearchableIndex def collection coll = end_of_association_chain.search(params[:q] || params[:tag]) - coll = coll.paginate(:page => params[:page]) if request.format == :html + coll = paginate_scope(coll) if request.format == :html get_collection_ivar || set_collection_ivar(coll) end diff --git a/spec/controllers/nodes_controller_spec.rb b/spec/controllers/nodes_controller_spec.rb index f339757..4837695 100644 --- a/spec/controllers/nodes_controller_spec.rb +++ b/spec/controllers/nodes_controller_spec.rb @@ -1,6 +1,8 @@ require File.expand_path(File.dirname(__FILE__) + '/../spec_helper') describe NodesController do + integrate_views + describe "#index" do before :each do @node = Node.generate! @@ -165,7 +167,7 @@ describe NodesController do it 'should render the update action' do do_put - response.should render_template('update') + response.should render_template('edit') end end @@ -194,6 +196,12 @@ describe NodesController do specify { response.should be_success } end + shared_examples_for "a paginated reports collection" do + it "should be paginated" do + assigns[:reports].should be_a_kind_of(WillPaginate::Collection) + end + end + shared_examples_for "an un-paginated reports collection" do it "should not be paginated" do assigns[:reports].should_not be_a_kind_of(WillPaginate::Collection) @@ -212,7 +220,7 @@ describe NodesController do before { get :reports, :node => 123 } it_should_behave_like "a successful reports rendering" - it_should_behave_like "an un-paginated reports collection" + it_should_behave_like "a paginated reports collection" end context "for YAML" do diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 03f201c..25317c8 100755 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -100,44 +100,6 @@ describe ApplicationHelper do end end - describe "#items_per_page" do - context "when using defaults" do - before :each do - @per_page = ENV['ITEMS_PER_PAGE'] - ENV.delete('ITEMS_PER_PAGE') - end - - after do - ENV['ITEMS_PER_PAGE'] = @per_page if @per_page - end - - it "should return the default number of items to paginate by" do - helper.items_per_page.should == ApplicationHelper::ITEMS_PER_PAGE_DEFAULT - end - end - - context "when overriden by environmental variable" do - before :each do - @per_page = 51; - ENV['ITEMS_PER_PAGE'] = @per_page.to_s - end - - after do - ENV.delete('ITEMS_PER_PAGE') - end - - it "should return the overriden number of items to paginate by" do - helper.items_per_page.should == @per_page - end - end - end - - describe "#paginate_scope" do - it "should paginate the scope" do - helper.paginate_scope([1,2,3]).should be_a_kind_of(WillPaginate::Collection) - end - end - describe "#describe_search_if_present" do it "should describe an active search" do params[:q] = 'foo' diff --git a/spec/helpers/paginate_scope_helper_spec.rb b/spec/helpers/paginate_scope_helper_spec.rb new file mode 100644 index 0000000..c364f45 --- /dev/null +++ b/spec/helpers/paginate_scope_helper_spec.rb @@ -0,0 +1,9 @@ +require File.expand_path(File.dirname(__FILE__) + '/../spec_helper') + +describe PaginateScopeHelper do + describe "#paginate_scope" do + it "should paginate the scope" do + helper.paginate_scope([1,2,3]).should be_a_kind_of(WillPaginate::Collection) + end + end +end -- 1.7.2.3 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
