Commit e989d07087aeebea20f643e5ef1ef7e8459e286a
1 parent
d384598e
Exists in
master
and in
1 other branch
Fix pagination to properly link to oldest notice
- closes #164
- Kaminari was dropping the ?notice=1 from the
first page link. In the controller if notice is
not passed in we were assuming the user wanted
the latest notice instead of the first.
- Originally the Errs#show page would default to
displaying the last notice page since that was
the most recent notice. We changed the controller
to order notices by created_at so that the notice
we want to view is on page 1. That way, we do not
have to fight Kaminari.
- Rename Previous and Next to Older and Newer to
reflect user's intent.
Showing
8 changed files
with
53 additions
and
11 deletions
Show diff stats
app/controllers/errs_controller.rb
| @@ -29,9 +29,7 @@ class ErrsController < ApplicationController | @@ -29,9 +29,7 @@ class ErrsController < ApplicationController | ||
| 29 | end | 29 | end |
| 30 | 30 | ||
| 31 | def show | 31 | def show |
| 32 | - page = (params[:notice] || @problem.notices_count) | ||
| 33 | - page = 1 if page.to_i.zero? | ||
| 34 | - @notices = @problem.notices.page(page.to_i).per(1) | 32 | + @notices = @problem.notices.reverse_ordered.page(params[:notice]).per(1) |
| 35 | @notice = @notices.first | 33 | @notice = @notices.first |
| 36 | @comment = Comment.new | 34 | @comment = Comment.new |
| 37 | if request.headers['X-PJAX'] | 35 | if request.headers['X-PJAX'] |
app/helpers/navigation_helper.rb
| @@ -34,4 +34,15 @@ module NavigationHelper | @@ -34,4 +34,15 @@ module NavigationHelper | ||
| 34 | active | 34 | active |
| 35 | end | 35 | end |
| 36 | 36 | ||
| 37 | + # Returns the page number in reverse order. | ||
| 38 | + # Needed when reverse chronological paginating notices but | ||
| 39 | + # want to display the actual chronological occurrence number. | ||
| 40 | + # | ||
| 41 | + # E.G. - Given 6 notices, page 2 shows the second from last | ||
| 42 | + # occurrence indexed by 1, but it is diplaying the 5th ever | ||
| 43 | + # occurence of that error. | ||
| 44 | + def page_count_from_end(current_page, total_pages) | ||
| 45 | + (total_pages.to_i - current_page.to_i) + 1 | ||
| 46 | + end | ||
| 47 | + | ||
| 37 | end | 48 | end |
app/models/notice.rb
| @@ -30,6 +30,7 @@ class Notice | @@ -30,6 +30,7 @@ class Notice | ||
| 30 | validates_presence_of :backtrace, :server_environment, :notifier | 30 | validates_presence_of :backtrace, :server_environment, :notifier |
| 31 | 31 | ||
| 32 | scope :ordered, order_by(:created_at.asc) | 32 | scope :ordered, order_by(:created_at.asc) |
| 33 | + scope :reverse_ordered, order_by(:created_at.desc) | ||
| 33 | scope :for_errs, lambda {|errs| where(:err_id.in => errs.all.map(&:id))} | 34 | scope :for_errs, lambda {|errs| where(:err_id.in => errs.all.map(&:id))} |
| 34 | 35 | ||
| 35 | delegate :app, :problem, :to => :err | 36 | delegate :app, :problem, :to => :err |
app/views/kaminari/notices/_next_page.html.haml
| 1 | -~ link_to_unless current_page.last?, raw('Next →'), url, :class => "next_page", :rel => "next", :remote => remote do |name| | 1 | +~ link_to_unless current_page.last?, raw('← Older'), url, :class => "next_page", :rel => "next", :remote => remote do |name| |
| 2 | ~ content_tag :span, name, :class => 'next_page disabled' | 2 | ~ content_tag :span, name, :class => 'next_page disabled' |
app/views/kaminari/notices/_paginator.html.haml
| @@ -7,7 +7,8 @@ | @@ -7,7 +7,8 @@ | ||
| 7 | -# paginator: the paginator that renders the pagination tags inside | 7 | -# paginator: the paginator that renders the pagination tags inside |
| 8 | = paginator.render do | 8 | = paginator.render do |
| 9 | .notice-pagination< | 9 | .notice-pagination< |
| 10 | - = prev_page_tag | ||
| 11 | = next_page_tag | 10 | = next_page_tag |
| 11 | + | | ||
| 12 | + = prev_page_tag | ||
| 12 | .notice-pagination-loader= image_tag 'loader.gif' | 13 | .notice-pagination-loader= image_tag 'loader.gif' |
| 13 | -viewing occurrence #{current_page} of #{num_pages} | 14 | +viewing occurrence #{page_count_from_end(current_page, num_pages)} of #{num_pages} |
app/views/kaminari/notices/_prev_page.html.haml
| 1 | -~ link_to_unless current_page.first?, raw('← Previous'), url, :class => "previous_page", :rel => "prev", :remote => remote do |name| | 1 | +~ link_to_unless current_page.first?, raw('Newer →'), url, :class => "previous_page", :rel => "prev", :remote => remote do |name| |
| 2 | ~ content_tag :span, name, :class => 'previous_page disabled' | 2 | ~ content_tag :span, name, :class => 'previous_page disabled' |
spec/controllers/errs_controller_spec.rb
| @@ -138,10 +138,6 @@ describe ErrsController do | @@ -138,10 +138,6 @@ describe ErrsController do | ||
| 138 | describe "GET /apps/:app_id/errs/:id" do | 138 | describe "GET /apps/:app_id/errs/:id" do |
| 139 | render_views | 139 | render_views |
| 140 | 140 | ||
| 141 | - before do | ||
| 142 | - 3.times { Fabricate(:notice, :err => err)} | ||
| 143 | - end | ||
| 144 | - | ||
| 145 | context 'when logged in as an admin' do | 141 | context 'when logged in as an admin' do |
| 146 | before do | 142 | before do |
| 147 | sign_in Fabricate(:admin) | 143 | sign_in Fabricate(:admin) |
| @@ -162,6 +158,26 @@ describe ErrsController do | @@ -162,6 +158,26 @@ describe ErrsController do | ||
| 162 | response.should be_success | 158 | response.should be_success |
| 163 | end | 159 | end |
| 164 | 160 | ||
| 161 | + context 'pagination' do | ||
| 162 | + let!(:notices) do | ||
| 163 | + 3.times.reduce([]) do |coll, i| | ||
| 164 | + coll << Fabricate(:notice, :err => err, :created_at => (Time.now + i)) | ||
| 165 | + end | ||
| 166 | + end | ||
| 167 | + | ||
| 168 | + it "paginates the notices 1 at a time, starting with the most recent" do | ||
| 169 | + get :show, :app_id => app.id, :id => err.problem.id | ||
| 170 | + assigns(:notices).entries.count.should == 1 | ||
| 171 | + assigns(:notices).should include(notices.last) | ||
| 172 | + end | ||
| 173 | + | ||
| 174 | + it "paginates the notices 1 at a time, based on then notice param" do | ||
| 175 | + get :show, :app_id => app.id, :id => err.problem.id, :notice => 3 | ||
| 176 | + assigns(:notices).entries.count.should == 1 | ||
| 177 | + assigns(:notices).should include(notices.first) | ||
| 178 | + end | ||
| 179 | + end | ||
| 180 | + | ||
| 165 | context "create issue button" do | 181 | context "create issue button" do |
| 166 | let(:button_matcher) { match(/create issue/) } | 182 | let(:button_matcher) { match(/create issue/) } |
| 167 | 183 |
| @@ -0,0 +1,15 @@ | @@ -0,0 +1,15 @@ | ||
| 1 | +require 'spec_helper' | ||
| 2 | + | ||
| 3 | +describe NavigationHelper do | ||
| 4 | + describe '#page_count_from_end' do | ||
| 5 | + it 'returns the page number when counting from the last occurrence of a notice' do | ||
| 6 | + page_count_from_end(1, 6).should == 6 | ||
| 7 | + page_count_from_end(6, 6).should == 1 | ||
| 8 | + page_count_from_end(2, 6).should == 5 | ||
| 9 | + end | ||
| 10 | + | ||
| 11 | + it 'properly handles strings for input' do | ||
| 12 | + page_count_from_end('2', '6').should == 5 | ||
| 13 | + end | ||
| 14 | + end | ||
| 15 | +end |