From ac39cfcf224094cb3db90975a97a06cef27aad8d Mon Sep 17 00:00:00 2001 From: Jacob Fugal Date: Wed, 20 Nov 2013 17:16:42 -0700 Subject: [PATCH] clean up paginate calls fixes CNVS-8791 fixes CNVS-8795 * "without_count: true" -> "total_entries: nil" * move order clauses to the pagination target, rather than being in the pagination call * clean up implementation of first/last page links in Api.paginate test-plan: - have an account report with at least two instances - fetch /api/v1/accounts/:account_id/reports/:report - response should have most recent report instance first - have two subaccounts under an account - fetch /api/v1/accounts/:id/sub_accounts?recursive=true&per_page=1 - Links response header should not have a link with rel=last - fetch /api/v1/accounts/:id/sub_accounts?recursive=false&per_page=1 - Links response header should have a link with rel=last embedding page=2 - load /error_reports - should have most recent reports first - fetch /api/v1/conversations/batches - should have oldest batch first Change-Id: Ifef79b193720a09ad7fe059ed23e930c97d10f59 Reviewed-on: https://gerrit.instructure.com/26535 Reviewed-by: Cody Cutrer QA-Review: August Thornton Tested-by: Jenkins Product-Review: Jacob Fugal --- app/controllers/account_reports_controller.rb | 3 +-- app/controllers/accounts_controller.rb | 2 +- app/controllers/conversations_controller.rb | 5 ++--- app/controllers/errors_controller.rb | 6 +++++- app/controllers/page_views_controller.rb | 2 +- lib/api.rb | 2 -- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/controllers/account_reports_controller.rb b/app/controllers/account_reports_controller.rb index 68a9aca3524..8b1c465f180 100644 --- a/app/controllers/account_reports_controller.rb +++ b/app/controllers/account_reports_controller.rb @@ -147,8 +147,7 @@ class AccountReportsController < ApplicationController def index if authorized_action(@context, @current_user, :read_reports) - reports = Api.paginate(type_scope, self, url_for({:action => :index, :controller => :account_reports}), - :order => 'start_at DESC') + reports = Api.paginate(type_scope.order('start_at DESC'), self, url_for({:action => :index, :controller => :account_reports})) render :json => account_reports_json(reports, @current_user, session) end diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 8f747222ada..1ef0681a628 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -95,7 +95,7 @@ class AccountsController < ApplicationController end @accounts = Api.paginate(@accounts, self, api_v1_sub_accounts_url, - :without_count => recursive) + :total_entries => recursive ? nil : @accounts.count) render :json => @accounts.map { |a| account_json(a, @current_user, session, []) } end diff --git a/app/controllers/conversations_controller.rb b/app/controllers/conversations_controller.rb index 3468452a1b1..44565c007dd 100644 --- a/app/controllers/conversations_controller.rb +++ b/app/controllers/conversations_controller.rb @@ -303,10 +303,9 @@ class ConversationsController < ApplicationController # } # ] def batches - batches = Api.paginate(@current_user.conversation_batches.in_progress, + batches = Api.paginate(@current_user.conversation_batches.in_progress.order(:id), self, - api_v1_conversations_batches_url, - :order => :id) + api_v1_conversations_batches_url) render :json => batches.map{ |m| conversation_batch_json(m, @current_user, session) } end diff --git a/app/controllers/errors_controller.rb b/app/controllers/errors_controller.rb index ae5b0afcc89..8e4af69533c 100644 --- a/app/controllers/errors_controller.rb +++ b/app/controllers/errors_controller.rb @@ -38,7 +38,11 @@ class ErrorsController < ApplicationController @reports = @reports.where(:category => params[:category]) end - @reports = @reports.paginate(:per_page => PER_PAGE, :page => params[:page], :order => 'id DESC', :without_count => true) + # temporary handling of having total_entries nil, since the will_paginate + # view helper doesn't handle it yet (it's making its way through gerrit) + @reports = @reports.order('id DESC').paginate(:per_page => PER_PAGE, :page => params[:page] + 1, :total_entries => nil) + @reports.total_entries = @reports.offset + @reports.size + @reports.pop if @reports.size > params[:page] end def show diff --git a/app/controllers/page_views_controller.rb b/app/controllers/page_views_controller.rb index a7e9ebdc5ed..e0c90ab4b8e 100644 --- a/app/controllers/page_views_controller.rb +++ b/app/controllers/page_views_controller.rb @@ -171,10 +171,10 @@ class PageViewsController < ApplicationController end page_views = @user.page_views(date_options) url = api_v1_user_page_views_url(url_options) - @page_views = Api.paginate(page_views, self, url, :order => 'created_at DESC', :without_count => :true) respond_to do |format| format.json do + @page_views = Api.paginate(page_views, self, url, :total_entries => nil) render :json => page_views_json(@page_views, @current_user, session) end format.csv do diff --git a/lib/api.rb b/lib/api.rb index 525433aa451..293459e4b66 100644 --- a/lib/api.rb +++ b/lib/api.rb @@ -201,8 +201,6 @@ module Api # The collection needs to be a will_paginate collection (or act like one) # a new, paginated collection will be returned def self.paginate(collection, controller, base_url, pagination_args = {}) - pagination_args[:total_entries] = nil if pagination_args[:without_count] - pagination_args.reverse_merge!( page: controller.params[:page], per_page: per_page_for(controller,