Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Support pagination in mounted sub apps in padrino #251

Open
wants to merge 7 commits into from

8 participants

@lestercsp

Hi, I used kaminari on a mounted padrino sub application and found that the pagination helpers don't get the correct current_path.

In sinatra_helpers.rb it is using env['PATH_INFO'] for the current_path. However if the app is mounted on "/path", the mount point is not read in env['PATH_INFO']. Using env['REQUEST_PATH'] instead solves the issue.

@udzura
Collaborator

Hi, thank you for the request.
According to below, REQUEST_PATH is a legacy variable that I think would be deleted in the furture.
http://groups.google.com/group/rack-devel/browse_thread/thread/0a41ecc3ce2db50d%3E.

Cannot you use request.path instead?

request.path is a Rack method and should be defined in both Sinatra and Padrino.

@plribeiro3000

And plz make a regression test. =D

@lestercsp

Hi, Thanks for pointing that out. I have now used request.path instead of REQUEST_PATH. I also did a quick (and manual) test to see and confirmed that request.path is available in both sinatra and padrino.

@plribeiro3000

Well, maybe a regression test to see if current_path got the correct value. This test could prevent this bug to appear again in the future.

@lestercsp

I agree on adding a test, but I am no expert in writing tests. I tried to write one but I am wondering how I am going to mock mounting the application as a sub application. :(

@Spone

Hi there,
I need this feature too. How can I help to speed up its release?

@lestercsp

I think they wanted to have a test for this. My fork has a fix for this but I am not able to provide a test

@Spone

I'd like to help but unfortunately I do not know kaminari enough to be able to write a regression test.
Maybe if one of the authors could point me to the right direction?

@plribeiro3000

Hey, im not an author, But i can try to help you with the test. First, call the paginate method and assert its equality with the right answer (a link). Then, just to be sure, remove your fix, and run the test again, It should fail.

@yuki24
Collaborator

Hi guys, we would like to merge this PR. But since there is no additional specs, we can't merge it. Could anyone provide some specs?

@fsword

I want to add specs for this patch, but I cannot make the rspec working environment for this project. Does it need some special configuration?

@yuki24
Collaborator

@fsword, if you don't know how to run Rspec, the command you need is this:

bundle exec rake spec:all

To see the complete list of tasks, type this:

bundle exec rake -T

If the problem is different from above, tell me additional information about the problem you have.

@udzura udzura was assigned
@lestercsp

Hi guys, I added a somewhat crude mocking of padrino applications in sinatra_helpers_spec.rb. Checkout lestercsp@8103653

However, I am having trouble with line 67-69:

def app
  Rack::Lint.new(Padrino.application)
end

When I run:

get '/admin/users'

I get the following error:

Rack::Lint::LintError: Content-Length header was 30, but should be 444
from /Users/leicester/.rvm/gems/ruby-2.0.0-p0/gems/rack-1.4.5/lib/rack/lint.rb:19:in `assert'

If I remove the Rack::Lint portion I then get a 'not found' response when I emulate getting '/admin/users'. I hope someone could point me in the right direction so that we could complete writing the test for this.

@zzak zzak commented on the diff
spec/helpers/sinatra_helpers_spec.rb
((17 lines not shown))
+ @options = {}
+
+ erb ERB_TEMPLATE_FOR_PAGINATE
+ end
+ end
+
+ end
+
+ Padrino.mount('base_app').to('/')
+ Padrino.mount('mounted_app').to('/admin')
+ def app
+ Rack::Lint.new(Padrino.application)
+ end
+ end
+
+ it "should do ???" do
@zzak Collaborator
zzak added a note

Can you implement this spec?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@yuki24 yuki24 commented on the diff
gemfiles/sinatra.gemfile
@@ -8,5 +8,6 @@ gem 'sinatra-contrib', '>= 1.3'
gem 'nokogiri'
gem 'xpath'
gem 'mime-types'
+gem 'padrino-core'
@yuki24 Collaborator
yuki24 added a note

Is this mandatory? Why not just using Sinatra? If you add support for Sinatra, Padrino will also probably work as you expect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@zzak
Collaborator

I think this will have to wait until the release of 0.15.0

@holin

Hi @zzak, current release: kaminari-0.15.1, not include this fix.

@zzak
Collaborator

@holin There are still some work to be done on this patch, please see our comments on the diff.

@yuki24 yuki24 added this to the 1.0.0 milestone
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 28, 2012
  1. support pagination for mounted sub apps in padrino

    Lester Celestial authored
  2. support pagination in mounted sub apps

    Lester Celestial authored
Commits on Jun 29, 2012
  1. used request.path instead of REQUEST_PATH

    Lester Celestial authored
Commits on Sep 5, 2012
  1. Merge remote-tracking branch 'upstream/master'

    Lester Celestial authored
Commits on Mar 20, 2013
  1. @lestercsp
  2. @lestercsp
  3. @lestercsp
This page is out of date. Refresh to see the latest.
View
1  gemfiles/sinatra.gemfile
@@ -8,5 +8,6 @@ gem 'sinatra-contrib', '>= 1.3'
gem 'nokogiri'
gem 'xpath'
gem 'mime-types'
+gem 'padrino-core'
@yuki24 Collaborator
yuki24 added a note

Is this mandatory? Why not just using Sinatra? If you add support for Sinatra, Padrino will also probably work as you expect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
gemspec :path => '../'
View
4 lib/kaminari/helpers/sinatra_helpers.rb
@@ -84,7 +84,7 @@ module HelperMethods
# * <tt>:remote</tt> - Ajax? (false by default)
# * <tt>:ANY_OTHER_VALUES</tt> - Any other hash key & values would be directly passed into each tag as :locals value.
def paginate(scope, options = {}, &block)
- current_path = env['PATH_INFO'] rescue nil
+ current_path = request.path rescue nil
current_params = Rack::Utils.parse_query(env['QUERY_STRING']).symbolize_keys rescue {}
paginator = Kaminari::Helpers::Paginator.new(
ActionViewTemplateProxy.new(:current_params => current_params, :current_path => current_path, :param_name => options[:param_name] || Kaminari.config.param_name),
@@ -116,7 +116,7 @@ def link_to_next_page(scope, name, options = {})
placeholder = options.delete(:placeholder)
query = params.merge(param_name => (scope.current_page + 1))
unless scope.last_page?
- link_to name, env['PATH_INFO'] + (query.empty? ? '' : "?#{query.to_query}"), options.reverse_merge(:rel => 'next')
+ link_to name, request.path + (query.empty? ? '' : "?#{query.to_query}"), options.reverse_merge(:rel => 'next')
else
placeholder
end
View
57 spec/helpers/sinatra_helpers_spec.rb
@@ -41,6 +41,63 @@
end
end
+ context "mounted padrino app" do
+ before do
+ Padrino.clear!
+
+ class BaseApp < Padrino::Application; end
+ class MountedApp < Padrino::Application
+
+ register Kaminari::Helpers::SinatraHelpers
+
+ controllers :users do
+ get :index do
+ @page = params[:page] || 1
+ @users = User.page(@page)
+ @options = {}
+
+ erb ERB_TEMPLATE_FOR_PAGINATE
+ end
+ end
+
+ end
+
+ Padrino.mount('base_app').to('/')
+ Padrino.mount('mounted_app').to('/admin')
+ def app
+ Rack::Lint.new(Padrino.application)
+ end
+ end
+
+ it "should do ???" do
@zzak Collaborator
zzak added a note

Can you implement this spec?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ # get '/admin/users'
+ end
+ end
+
+ context "namespaced sinatra app" do
+ before do
+ mock_app do
+ register Kaminari::Helpers::SinatraHelpers
+ register Sinatra::Namespace
+
+ namespace '/admin' do
+ get '/users' do
+ @page = params[:page] || 1
+ @users = User.page(@page).per(5)
+ @options = {}
+ erb ERB_TEMPLATE_FOR_PAGINATE
+ end
+ end
+ end
+ get '/admin/users'
+ end
+
+ it "should get the current path correctly" do
+ current_path = last_document.search('.page a').map(&:attributes).map{|a| a['href']}.map(&:value).map{|x| x.split('?').first}.uniq.first
+ current_path.should eq '/admin/users'
+ end
+ end
+
context 'normal paginations with Sinatra' do
before { get '/users' }
Something went wrong with that request. Please try again.