Nick: Thanks, I've merged this into `next`. -igal
On 09/21/2010 04:15 PM, Nick Lewis wrote: > +1, and sorry for taking so long to get around to it. I had this > change implemented ad-hoc in my working branch for adding 2.6 report > support (which is why I didn't remember it being an issue), and in > essentially the same way. > > On Fri, Sep 17, 2010 at 10:27 PM, Igal Koshevoy > <[email protected] <mailto:[email protected]>> wrote: > > 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] > <mailto:[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] > <mailto:[email protected]>. > To unsubscribe from this group, send email to > [email protected] > <mailto:puppet-dev%[email protected]>. > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > > > -- > 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. -- 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.
