[PATCH conductor] BZ805909: Detect if paginated collection is out of bounds (rev. 3)
Jan Provaznik
jprovazn at redhat.com
Thu Apr 19 13:02:15 UTC 2012
On 04/13/2012 01:39 PM, ifarkas at redhat.com wrote:
> From: Imre Farkas<ifarkas at redhat.com>
>
> https://bugzilla.redhat.com/show_bug.cgi?id=805909
>
> Resending it because a part of the previous revision has already been committed.
> ---
> src/app/controllers/application_controller.rb | 8 +++++
> src/app/controllers/deployments_controller.rb | 22 +++++++-------
> src/app/controllers/images_controller.rb | 2 +-
> src/app/controllers/instances_controller.rb | 15 +++++----
> src/app/controllers/pools_controller.rb | 41 ++++++++++++++----------
> src/app/models/pool.rb | 6 ----
> src/app/views/pools/_pretty_list.html.haml | 2 +-
> 7 files changed, 53 insertions(+), 43 deletions(-)
>
> diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb
> index db471ba..9b89d0e 100644
> --- a/src/app/controllers/application_controller.rb
> +++ b/src/app/controllers/application_controller.rb
> @@ -36,6 +36,7 @@ class ApplicationController< ActionController::Base
> rescue_from Aeolus::Conductor::API::Error, :with => :handle_api_error
>
> helper_method :check_privilege
> + helper_method :paginate_collection
>
> protected
>
> @@ -369,4 +370,11 @@ class ApplicationController< ActionController::Base
> redirect_to path.merge(original_params).merge(extended_params)
> end
>
> + def paginate_collection(collection, page = nil, per_page = PER_PAGE)
> + result = collection.paginate(:page => page, :per_page => per_page)
> + result = collection.paginate(:page => result.total_pages, :per_page => per_page) if result.out_of_bounds?&& result.total_pages> 0
> +
> + result
> + end
> +
> end
> diff --git a/src/app/controllers/deployments_controller.rb b/src/app/controllers/deployments_controller.rb
> index 504a050..8bb76b3 100644
> --- a/src/app/controllers/deployments_controller.rb
> +++ b/src/app/controllers/deployments_controller.rb
> @@ -187,13 +187,13 @@ class DeploymentsController< ApplicationController
> if filter_view?
> @view = 'instances/list'
> params[:instances_preset_filter] = "" unless params[:instances_preset_filter]
> - @instances = Instance.apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
> - list(sort_column(Instance), sort_direction).where("instances.deployment_id" => @deployment.id).
> - paginate(:page => params[:page], :per_page => PER_PAGE)
> + @instances = paginate_collection(Instance.apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
> + list(sort_column(Instance), sort_direction).where("instances.deployment_id" => @deployment.id),
> + params[:page], PER_PAGE)
> else
> @view = 'pretty_view_show'
> - @instances = Instance.list(sort_column(Instance), sort_direction).where("instances.deployment_id" => @deployment.id).
> - paginate(:page => params[:page], :per_page => PER_PAGE)
> + @instances = paginate_collection(Instance.list(sort_column(Instance), sort_direction).where("instances.deployment_id" => @deployment.id),
> + params[:page], PER_PAGE)
> end
> #TODO add links to real data for history, permissions, services
> @tabs = [{:name => t('instances.instances.other'), :view => @view, :id => 'instances', :count => @deployment.instances.count, :pretty_view_toggle => 'enabled'},
> @@ -383,12 +383,12 @@ class DeploymentsController< ApplicationController
> { :name => t("providers.provider"), :sortable => false }
> ]
> @pools = Pool.list_for_user(current_user, Privilege::CREATE, Deployment)
> - @deployments = Deployment.includes(:owner, :pool, :instances).
> - apply_filters(:preset_filter_id => params[:deployments_preset_filter], :search_filter => params[:deployments_search]).
> - list_for_user(current_user, Privilege::VIEW).
> - where('deployments.pool_id' => @pools).
> - order(sort_column(Deployment, "deployments.name") +' '+ sort_direction).
> - paginate(:page => params[:page], :per_page => PER_PAGE)
> + @deployments = paginate_collection(Deployment.includes(:owner, :pool, :instances).
> + apply_filters(:preset_filter_id => params[:deployments_preset_filter], :search_filter => params[:deployments_search]).
> + list_for_user(current_user, Privilege::VIEW).
> + where('deployments.pool_id' => @pools).
> + order(sort_column(Deployment, "deployments.name") +' '+ sort_direction),
> + params[:page], PER_PAGE)
> end
>
> def count_additional_quota(deployment)
> diff --git a/src/app/controllers/images_controller.rb b/src/app/controllers/images_controller.rb
> index cad12f9..d71c1b1 100644
> --- a/src/app/controllers/images_controller.rb
> +++ b/src/app/controllers/images_controller.rb
> @@ -27,7 +27,7 @@ class ImagesController< ApplicationController
> { :name => t('images.index.architecture'), :sort_attr => :name },
> { :name => t('images.index.last_rebuild'), :sortable => false },
> ]
> - @images = Aeolus::Image::Warehouse::Image.all.paginate(:page => params[:page], :per_page => PER_PAGE)
> + @images = paginate_collection(Aeolus::Image::Warehouse::Image.all, params[:page], PER_PAGE)
> respond_to do |format|
> format.html
> format.js { render :partial => 'list' }
> diff --git a/src/app/controllers/instances_controller.rb b/src/app/controllers/instances_controller.rb
> index 7773d81..1153e96 100644
> --- a/src/app/controllers/instances_controller.rb
> +++ b/src/app/controllers/instances_controller.rb
> @@ -248,14 +248,15 @@ class InstancesController< ApplicationController
>
> def load_instances
> if params[:deployment_id].blank?
> - @instances = Instance.includes(:owner).apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
> - list_for_user(current_user, Privilege::VIEW).list(sort_column(Instance), sort_direction).
> - where("instances.pool_id" => @pools).paginate(:page => params[:page], :per_page => PER_PAGE)
> + @instances = paginate_collection(Instance.includes(:owner).apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
> + list_for_user(current_user, Privilege::VIEW).list(sort_column(Instance), sort_direction).
> + where("instances.pool_id" => @pools),
> + params[:page], PER_PAGE)
> else
> - @instances = Instance.includes(:owner).apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
> - list(sort_column(Instance), sort_direction).list_for_user(current_user, Privilege::VIEW).
> - where("instances.pool_id" => @pools, "instances.deployment_id" => params[:deployment_id]).
> - paginate(:page => params[:page], :per_page => PER_PAGE)
> + @instances = paginate_collection(Instance.includes(:owner).apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
> + list(sort_column(Instance), sort_direction).list_for_user(current_user, Privilege::VIEW).
> + where("instances.pool_id" => @pools, "instances.deployment_id" => params[:deployment_id]),
> + params[:page], PER_PAGE)
> end
> end
>
> diff --git a/src/app/controllers/pools_controller.rb b/src/app/controllers/pools_controller.rb
> index abc3203..347c180 100644
> --- a/src/app/controllers/pools_controller.rb
> +++ b/src/app/controllers/pools_controller.rb
> @@ -44,25 +44,30 @@ class PoolsController< ApplicationController
> @details_tab = @tabs.find {|t| t[:id] == details_tab_name} || @tabs.first[:name].downcase
>
> @user_pools = Pool.list_for_user(current_user, Privilege::CREATE, Deployment)
> +
> if filter_view?
> case @details_tab[:id]
> when 'pools'
> - @pools = Pool.includes(:deployments, :instances).apply_filters(:preset_filter_id => params[:pools_preset_filter], :search_filter => params[:pools_search]).
> - list_for_user(current_user, Privilege::VIEW).list(sort_column(Pool), sort_direction).
> - paginate(:page => params[:page], :per_page => PER_PAGE)
> + @pools = paginate_collection(Pool.includes(:deployments, :instances).
> + apply_filters(:preset_filter_id => params[:pools_preset_filter], :search_filter => params[:pools_search]).
> + list_for_user(current_user, Privilege::VIEW).list(sort_column(Pool), sort_direction),
> + params[:page], PER_PAGE)
> when 'instances'
> params[:instances_preset_filter] = "" unless params[:instances_preset_filter]
> - @instances = Instance.includes({:provider_account => :provider}).apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
> - list_for_user(current_user, Privilege::VIEW).list(sort_column(Instance), sort_direction).
> - paginate(:page => params[:page], :per_page => PER_PAGE)
> + @instances = paginate_collection(Instance.includes({:provider_account => :provider}).
> + apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
> + list_for_user(current_user, Privilege::VIEW).list(sort_column(Instance), sort_direction),
> + params[:page], PER_PAGE)
> when 'deployments'
> - @deployments = Deployment.includes(:pool, :instances).apply_filters(:preset_filter_id => params[:deployments_preset_filter], :search_filter => params[:deployments_search]).
> - list_for_user(current_user, Privilege::VIEW).list(sort_column(Deployment), sort_direction).
> - paginate(:page => params[:page], :per_page => PER_PAGE)
> + @deployments = paginate_collection(Deployment.includes(:pool, :instances).
> + apply_filters(:preset_filter_id => params[:deployments_preset_filter], :search_filter => params[:deployments_search]).
> + list_for_user(current_user, Privilege::VIEW).list(sort_column(Deployment), sort_direction),
> + params[:page], PER_PAGE)
> end
> else
> - @pools = Pool.includes(:deployments, :instances, :quota, :catalogs).list_for_user(current_user, Privilege::VIEW).list(sort_column(Pool), sort_direction).
> - paginate(:page => params[:page], :per_page => PER_PAGE)
> + @pools = paginate_collection(Pool.includes(:deployments, :instances, :quota, :catalogs).
> + list_for_user(current_user, Privilege::VIEW).list(sort_column(Pool), sort_direction),
> + params[:page], PER_PAGE)
> end
>
> statistics
> @@ -109,16 +114,18 @@ class PoolsController< ApplicationController
>
> details_tab_name = params[:details_tab].blank? ? 'deployments' : params[:details_tab]
> @details_tab = @tabs.find {|t| t[:id] == details_tab_name} || @tabs.first[:name].downcase
> - @deployments = @pool.deployments.includes(:owner, :pool, :instances, :events).
> - apply_filters(:preset_filter_id => params[:deployments_preset_filter], :search_filter => params[:deployments_search]).
> - list_for_user(current_user, Privilege::VIEW).
> - paginate(:page => params[:page], :per_page => PER_PAGE) if @details_tab[:id] == 'deployments'
> + @deployments = paginate_collection(@pool.deployments.includes(:owner, :pool, :instances, :events).
> + apply_filters(:preset_filter_id => params[:deployments_preset_filter], :search_filter => params[:deployments_search]).
> + list_for_user(current_user, Privilege::VIEW),
> + params[:page], PER_PAGE) if @details_tab[:id] == 'deployments'
> @view = @details_tab[:view]
> respond_to do |format|
> format.html { render :action => :show}
> format.js { render :partial => @view }
> - format.json { render :json => @pool.as_json(:with_deployments => true, :current_user => current_user,
> - :page => params[:page], :per_page => PER_PAGE) }
> + format.json do
> + deployments = paginate_collection(@pool.deployments.list_for_user(current_user, Privilege::VIEW), params[:page], PER_PAGE).map {|d| d.as_json}
> + render :json => @pool.as_json.merge({:deployments => deployments})
> + end
> end
> end
>
> diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
> index 9556dee..4604bd2 100644
> --- a/src/app/models/pool.rb
> +++ b/src/app/models/pool.rb
> @@ -137,12 +137,6 @@ class Pool< ActiveRecord::Base
>
> })
>
> - if options[:with_deployments]
> - result[:deployments] = deployments.list_for_user(options[:current_user], Privilege::VIEW).
> - paginate(:page => options[:page], :per_page => options[:per_page]).
> - map {|d| d.as_json}
> - end
> -
> result
> end
>
> diff --git a/src/app/views/pools/_pretty_list.html.haml b/src/app/views/pools/_pretty_list.html.haml
> index 47e7a8b..9b869b1 100644
> --- a/src/app/views/pools/_pretty_list.html.haml
> +++ b/src/app/views/pools/_pretty_list.html.haml
> @@ -25,7 +25,7 @@
> %a.control{:href => '#'}
> %span Expand/Collapse
> %div.content
> - - deployments = pool.deployments.includes(:owner, :pool, :instances, :events).list_for_user(current_user, Privilege::VIEW).ascending_by_name.paginate(:page => params[:page], :per_page => PER_PAGE)
> + - deployments = paginate_collection(pool.deployments.includes(:owner, :pool, :instances, :events).list_for_user(current_user, Privilege::VIEW).ascending_by_name, params[:page], PER_PAGE)
> = render :partial => 'deployments', :locals => {:pool => pool, :deployments => deployments}
> %ul.content.actions
> %li
ACK
More information about the aeolus-devel
mailing list