Merge pull request #137 from sodabrew/patch-1

Handle out-of-range and bogus pagination inputs.
sodabrew · Nov 26, 2012 · 0eef69e · 0eef69e
2 parents 72d4dd6 + 8133354
commit 0eef69e
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 4 deletions.
diff --git a/app/helpers/paginate_scope_helper.rb b/app/helpers/paginate_scope_helper.rb
@@ -2,11 +2,28 @@ module PaginateScopeHelper
   # Return a paginated +scope+.
   def paginate_scope(scope, opts={})
     if ! params[:per_page]
-      scope.paginate( opts.reverse_merge(:page => params[:page], :per_page => (scope.first.class.per_page rescue nil)) )
+      per_page = scope.first.class.per_page rescue nil
-    elsif params[:per_page] != "all"
+      page = params[:page]
-      scope.paginate( opts.reverse_merge(:page => params[:page], :per_page => params[:per_page]) )
+    elsif params[:per_page] == 'all'
+      per_page = scope.count
+      page = 1
     else
-      scope.paginate( opts.reverse_merge(:page => 1, :per_page => scope.count) )
+      per_page = params[:per_page]
+      page = begin
+          if per_page.to_i * params[:page].to_i > scope.count
+            scope.count / per_page.to_i + 1 # last page
+          else
+            params[:page]
+          end
+        rescue StandardError
+          params[:page]
+        end
     end
+
+    # Handle negative / invalid page numbers gracefully
+    page = (page.to_i > 0 ? page : 1) rescue 1
+    per_page = (per_page.to_i > 0 ? per_page : nil) rescue nil
+
+    scope.paginate( opts.reverse_merge(:page => page, :per_page => per_page) )
   end
 end
diff --git a/spec/helpers/paginate_scope_helper_spec.rb b/spec/helpers/paginate_scope_helper_spec.rb
@@ -18,5 +18,49 @@
       paginated_scope.total_pages.should == 1
       paginated_scope.per_page.should == 3
     end
+
+    it "should return the last page if page * per_page > count" do
+      params[:page] = '3'
+      params[:per_page] = '2'
+      paginated_scope = helper.paginate_scope([1,2,3])
+      paginated_scope.should be_a_kind_of(WillPaginate::Collection)
+      paginated_scope.total_pages.should == 2
+      paginated_scope.count.should == 1
+      paginated_scope.current_page.should == 2
+      paginated_scope.per_page.should == 2
+    end
+
+    it "should return the first page if page is negative" do
+      params[:page] = '-5'
+      params[:per_page] = '2'
+      paginated_scope = helper.paginate_scope([1,2,3])
+      paginated_scope.should be_a_kind_of(WillPaginate::Collection)
+      paginated_scope.total_pages.should == 2
+      paginated_scope.count.should == 2
+      paginated_scope.current_page.should == 1
+      paginated_scope.per_page.should == 2
+    end
+
+    it "should return the first page if page is not a number" do
+      params[:page] = 'notanumber'
+      params[:per_page] = '2'
+      paginated_scope = helper.paginate_scope([1,2,3])
+      paginated_scope.should be_a_kind_of(WillPaginate::Collection)
+      paginated_scope.total_pages.should == 2
+      paginated_scope.count.should == 2
+      paginated_scope.current_page.should == 1
+      paginated_scope.per_page.should == 2
+    end
+
+    it "should use default per_page if per_page is not a number" do
+      params[:page] = 'notanumber'
+      params[:per_page] = 'notanumber'
+      paginated_scope = helper.paginate_scope([1,2,3])
+      paginated_scope.should be_a_kind_of(WillPaginate::Collection)
+      paginated_scope.total_pages.should == 1
+      paginated_scope.count.should == 3
+      paginated_scope.current_page.should == 1
+      paginated_scope.per_page.should == 30 # In will_paginate v3, it is WillPaginate.per_page
+    end
   end
 end