[PATCH] Fixed sort column
Tomas Sedovic
tsedovic at redhat.com
Fri Dec 16 12:54:22 UTC 2011
On 12/16/2011 10:50 AM, jprovazn at redhat.com wrote:
> From: Jan Provaznik<jprovazn at redhat.com>
>
> https://bugzilla.redhat.com/show_bug.cgi?id=761504
> sort_column method now includes table name, this was problem for
> non-admin users - selects were joined with columns from privilege table
> which contains 'name' column too so there was conflict with default sort
> column 'name'.
>
> Also removed pagination from listing pool families - pagination is obviusly
> not used in a view and privilege target_type changed to 'Pool' when listing
> pool families for a user.
> ---
> src/app/controllers/application_controller.rb | 5 ++-
> .../controllers/hardware_profiles_controller.rb | 2 +-
> src/app/controllers/pool_families_controller.rb | 7 +---
> src/features/pool_family.feature | 39 +++++++++++++++-----
> src/features/step_definitions/pool_family_steps.rb | 5 +++
> src/spec/factories/permission.rb | 6 +++
> src/spec/factories/user.rb | 3 ++
> 7 files changed, 49 insertions(+), 18 deletions(-)
>
> diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb
> index 053a87b..7e8394c 100644
> --- a/src/app/controllers/application_controller.rb
> +++ b/src/app/controllers/application_controller.rb
> @@ -272,8 +272,9 @@ class ApplicationController< ActionController::Base
> end
> end
>
> - def sort_column(model, default="name")
> - model.column_names.include?(params[:order_field]) ? params[:order_field] : default
> + def sort_column(model, default = nil)
> + return params[:order_field] if model.column_names.include?(params[:order_field])
> + return default || "#{model.quoted_table_name}.name"
> end
>
> def sort_direction
> diff --git a/src/app/controllers/hardware_profiles_controller.rb b/src/app/controllers/hardware_profiles_controller.rb
> index 8773d6a..2d408bf 100644
> --- a/src/app/controllers/hardware_profiles_controller.rb
> +++ b/src/app/controllers/hardware_profiles_controller.rb
> @@ -205,7 +205,7 @@ class HardwareProfilesController< ApplicationController
>
> def load_hardware_profiles
> sort_order = sort_direction
> - sort_field = sort_column(HardwareProfile)
> + sort_field = sort_column(HardwareProfile, 'name')
> if sort_field == "name"
> @hardware_profiles = HardwareProfile.list_for_user(current_user, Privilege::VIEW).where('provider_id IS NULL', {}).apply_filters(:preset_filter_id => params[:hardware_profiles_preset_filter], :search_filter => params[:hardware_profiles_search]).order("hardware_profiles.name #{sort_direction}")
> else
> diff --git a/src/app/controllers/pool_families_controller.rb b/src/app/controllers/pool_families_controller.rb
> index f400a1d..93dce2e 100644
> --- a/src/app/controllers/pool_families_controller.rb
> +++ b/src/app/controllers/pool_families_controller.rb
> @@ -19,14 +19,13 @@ require 'will_paginate/array'
> class PoolFamiliesController< ApplicationController
> before_filter :require_user
> before_filter :set_params_and_header, :only => [:index, :show]
> - before_filter :load_pool_families, :only =>[:show]
> + before_filter :load_pool_families, :only =>[:index, :show]
> before_filter :load_tab_captions_and_details_tab, :only => [:show]
>
> def index
> clear_breadcrumbs
> save_breadcrumb(pool_families_path)
> set_admin_environments_tabs 'pool_families'
> - load_pool_families
> respond_to do |format|
> format.html
> format.js { render :partial => 'list' }
> @@ -213,9 +212,7 @@ class PoolFamiliesController< ApplicationController
> end
>
> def load_pool_families
> - @pool_families = PoolFamily.list_for_user(current_user, Privilege::VIEW).paginate(
> - :page => params[:page] || 1,
> - :order => (sort_column(PoolFamily) + ' ' + sort_direction))
> + @pool_families = PoolFamily.list_for_user(current_user, Privilege::VIEW, Pool).order(sort_column(PoolFamily) + ' ' + sort_direction)
> end
>
> def load_pool_family_tabs
> diff --git a/src/features/pool_family.feature b/src/features/pool_family.feature
> index 5e96ca3..3d3d1d0 100644
> --- a/src/features/pool_family.feature
> +++ b/src/features/pool_family.feature
> @@ -4,12 +4,12 @@ Feature: Pool Families
> I want to manage pool families
>
> Background:
> - Given I am an authorised user
> - And I am logged in
> - And I'm connected to the warehouse
> + Given I'm connected to the warehouse
>
> Scenario: List pool families
> - Given I am on the homepage
> + Given I am an authorised user
> + And I am logged in
> + And I am on the homepage
> And there are these pool families:
> | name |
> | pool_family1 |
> @@ -21,14 +21,25 @@ Feature: Pool Families
> | pool_family2 |
> | pool_family3 |
>
> + Scenario: List pool families as unprivileged user
> + Given I am logged in
> + And I can view pool family "testpoolfamily"
> + When I go to the pool families page
> + Then I should see "Pool Families"
> + And I should see "testpoolfamily"
> +
> Scenario: Show pool family details
> - Given there is a pool family named "testpoolfamily"
> + Given I am an authorised user
> + And I am logged in
> + And there is a pool family named "testpoolfamily"
> And I am on the pool families page
> When I follow "testpoolfamily"
> Then I should see "Pools"
>
> Scenario: Create a new Pool family
> - Given I am on the pool families page
> + Given I am an authorised user
> + And I am logged in
> + And I am on the pool families page
> And there is not a pool family named "testpoolfamily"
> When I follow "new_pool_family_button"
> Then I should be on the new pool family page
> @@ -40,7 +51,9 @@ Feature: Pool Families
> And I should have a pool family named "testpoolfamily"
>
> Scenario: Delete a pool family
> - Given I am on the homepage
> + Given I am an authorised user
> + And I am logged in
> + And I am on the homepage
> And there is a pool family named "poolfamily1"
> When I go to the pool families page
> And I follow "poolfamily1"
> @@ -48,7 +61,9 @@ Feature: Pool Families
> Then there should not exist a pool family named "poolfamily1"
>
> Scenario: Disallow deletion of default pool family
> - Given I am on the pool families page
> + Given I am an authorised user
> + And I am logged in
> + And I am on the pool families page
> Then I should see "default"
> When I follow "default"
> And I press "delete_pool_family_button"
> @@ -82,7 +97,9 @@ Feature: Pool Families
> # And I should see "third_family"
>
> Scenario: Add provider account to pool family
> - Given there is a pool family named "testpoolfamily"
> + Given I am an authorised user
> + And I am logged in
> + And there is a pool family named "testpoolfamily"
> And there is a provider named "testprovider"
> And there is a provider account named "testaccount"
> And I am on the pool family provider accounts page
> @@ -94,7 +111,9 @@ Feature: Pool Families
> And I should see "testaccount"
>
> Scenario: Remove provider account from pool family
> - Given there is a pool family named "testpoolfamily"
> + Given I am an authorised user
> + And I am logged in
> + And there is a pool family named "testpoolfamily"
> And there is a provider named "testprovider"
> And there is a provider account named "testaccount"
> And there is a provider account "testaccount" related to pool family "testpoolfamily"
> diff --git a/src/features/step_definitions/pool_family_steps.rb b/src/features/step_definitions/pool_family_steps.rb
> index 70bb207..b2239dd 100644
> --- a/src/features/step_definitions/pool_family_steps.rb
> +++ b/src/features/step_definitions/pool_family_steps.rb
> @@ -66,3 +66,8 @@ Then /^there should not exist a provider account assigned to "([^"]*)"$/ do |nam
> @pool_family = PoolFamily.find_by_name(name)
> @pool_family.provider_accounts.count == 0
> end
> +
> +Given /^I can view pool family "([^"]*)"$/ do |arg1|
> + pool_family = PoolFamily.find_by_name(arg1) || FactoryGirl.create(:pool_family, :name => arg1)
> + perm = FactoryGirl.create(:pool_family_user_permission, :permission_object => pool_family, :user => @user)
> +end
> diff --git a/src/spec/factories/permission.rb b/src/spec/factories/permission.rb
> index d69930a..1a1f025 100644
> --- a/src/spec/factories/permission.rb
> +++ b/src/spec/factories/permission.rb
> @@ -50,4 +50,10 @@ FactoryGirl.define do
> user { |r| r.association(:pool_user2) }
> end
>
> + factory :pool_family_user_permission, :parent => :permission do
> + role { |r| Role.first(:conditions => ['name = ?', 'Pool Family User']) || FactoryGirl.create(:role, :name => 'Pool Family User') }
> + permission_object { |r| r.association(:pool_family) }
> + user { |r| r.association(:pool_family_user) }
> + end
> +
> end
> diff --git a/src/spec/factories/user.rb b/src/spec/factories/user.rb
> index b5e1369..5ca512d 100644
> --- a/src/spec/factories/user.rb
> +++ b/src/spec/factories/user.rb
> @@ -58,4 +58,7 @@ FactoryGirl.define do
> sequence(:login) { |n| "pool_user2#{n}" }
> end
>
> + factory :pool_family_user, :parent => :user do
> + sequence(:login) { |n| "pool_family_user#{n}" }
> + end
> end
ACK
Thomas
--
No trees were killed to send this message, but a large number of
electrons were terribly inconvenienced.
More information about the aeolus-devel
mailing list