On 04/20/2011 01:43 PM, Hugh Brock wrote:
On Wed, Apr 20, 2011 at 01:06:18PM +0200, Jan Provaznik wrote:
On 04/20/2011 12:53 PM, Tomas Sedovic wrote:
On 04/20/2011 11:20 AM, jtomasek@redhat.com wrote:
From: Jiri Tomasekjtomasek@redhat.com
.../controllers/admin/pool_families_controller.rb | 29 ++++++++++++++++++- src/app/views/admin/pool_families/_list.haml | 2 +- .../admin/pool_families/_provider_accounts.haml | 25 +++++++++++++++++ src/config/routes.rb | 2 +- src/features/pool_family.feature | 22 +++++++++++++++ src/features/step_definitions/pool_family_steps.rb | 23 +++++++++++++++- src/features/support/paths.rb | 3 ++ 7 files changed, 101 insertions(+), 5 deletions(-)
diff --git a/src/app/controllers/admin/pool_families_controller.rb b/src/app/controllers/admin/pool_families_controller.rb index af53aac..ec21c53 100644 --- a/src/app/controllers/admin/pool_families_controller.rb +++ b/src/app/controllers/admin/pool_families_controller.rb @@ -2,6 +2,7 @@ class Admin::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_tab_captions_and_details_tab, :only => [:show]
def index @search_term = params[:q]
@@ -50,8 +51,7 @@ class Admin::PoolFamiliesController< ApplicationController def show @pool_family = PoolFamily.find(params[:id]) @url_params = params.clone
- @tab_captions = ['Properties', 'History', 'Permissions', 'Provider Accounts', 'Pools']
- @details_tab = params[:details_tab].blank? ? 'properties' : params[:details_tab]
respond_to do |format| format.js do if @url_params.delete :details_pane
@@ -63,6 +63,15 @@ class Admin::PoolFamiliesController< ApplicationController end end
- def add_provider_account
- @pool_family = PoolFamily.find(params[:id])
- @provider_account = ProviderAccount.find(params[:provider_account_id])
- @pool_family.provider_accounts<< @provider_account
- flash[:notice] = "Provider Account has been added"
- redirect_to admin_pool_family_path(@pool_family, :details_tab => 'provider_accounts')
- end
def multi_destroy deleted = [] not_deleted = []
@@ -82,8 +91,24 @@ class Admin::PoolFamiliesController< ApplicationController redirect_to admin_pool_families_path end
def multi_destroy_provider_accounts
@pool_family = PoolFamily.find(params[:pool_family_id])
ProviderAccount.find(params[:provider_account_selected]).each do |provider_account|
@pool_family.provider_accounts.delete provider_account
end
redirect_to admin_pool_family_path(@pool_family, :details_tab => 'provider_accounts')
end
protected
def load_tab_captions_and_details_tab
@tab_captions = ['Properties', 'History', 'Permissions', 'Provider Accounts', 'Pools']
@details_tab = params[:details_tab].blank? ? 'properties' : params[:details_tab]
@provider_accounts_header = [{ :name => "Provider Account", :sort_attr => :name}]
end
def set_params_and_header @url_params = params.clone @header = [{ :name => "Name", :sort_attr => :name},
diff --git a/src/app/views/admin/pool_families/_list.haml b/src/app/views/admin/pool_families/_list.haml index a824676..6250c66 100644 --- a/src/app/views/admin/pool_families/_list.haml +++ b/src/app/views/admin/pool_families/_list.haml @@ -8,7 +8,7 @@ %span> , = link_to "None", @url_params.merge(:select => 'none')
- %table#pool_famlies_table
- %table#pool_families_table = sortable_table_header @header - unless @pool_families.blank? - @pool_families.each do |pool_family|
diff --git a/src/app/views/admin/pool_families/_provider_accounts.haml b/src/app/views/admin/pool_families/_provider_accounts.haml index e69de29..358e3d2 100644 --- a/src/app/views/admin/pool_families/_provider_accounts.haml +++ b/src/app/views/admin/pool_families/_provider_accounts.haml @@ -0,0 +1,25 @@ +%h3
- Provider Accounts for
- = @pool_family.name
+- unless @pool_family.provider_accounts.blank?
- form_tag do
- = restful_submit_tag "Remove selected", 'destroy', multi_destroy_provider_accounts_admin_pool_families_path(:pool_family_id => @pool_family.id), 'DELETE', :id => 'delete_button'
- = #submit_tag "Remove selected"
- %table#provider_accounts_table
= sortable_table_header @provider_accounts_header
- @pool_family.provider_accounts.each do |provider_account|
%tr
%td
- selected = @url_params[:select_provider_accounts] == 'all'
%input{:name => "provider_account_selected[]", :type => "checkbox", :value => provider_account.id, :id => "provider_account_checkbox_#{provider_account.id}", :checked => selected }
= provider_account.name
+- unless @pool_family.provider_accounts.count == ProviderAccount.all.count
- form_for :pool_family, :url => add_provider_account_admin_pool_families_path(:id => @pool_family.id) do |f|
- unless @pool_family.provider_accounts.blank?
= select_tag :provider_account_id, options_for_select(ProviderAccount.find(:all, :conditions => [ "id NOT IN (?)", @pool_family.provider_account_ids ]).collect{ |pa| [pa.name, pa.id] })
- else
= select_tag :provider_account_id, options_for_select(ProviderAccount.all.collect{ |pa| [pa.name, pa.id] })
- = f.submit "Add to #{@pool_family.name}"
diff --git a/src/config/routes.rb b/src/config/routes.rb index 162260c..6aa9c8c 100644 --- a/src/config/routes.rb +++ b/src/config/routes.rb @@ -58,7 +58,7 @@ ActionController::Routing::Routes.draw do |map|
r.resources :roles, :collection => { :multi_destroy => :delete } r.resources :settings, :collection => { :self_service => :get, :general_settings => :get }
- r.resources :pool_families, :collection => { :multi_destroy => :delete }
- r.resources :pool_families, :collection => { :multi_destroy => :delete, :add_provider_account => :post, :multi_destroy_provider_accounts => :delete } r.resources :realms, :collection => { :multi_destroy => :delete } r.resources :realm_mappings, :collection => { :multi_destroy => :delete } end
diff --git a/src/features/pool_family.feature b/src/features/pool_family.feature index 6795d42..fd4eb55 100644 --- a/src/features/pool_family.feature +++ b/src/features/pool_family.feature @@ -78,3 +78,25 @@ Feature: Pool Families Then I should see "first_family" And I should see "second_family" And I should see "third_family"
- Scenario: Add provider account to pool family
- Given 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
- Then I should see "Provider Accounts for"
- When I select "testaccount" from "provider_account_id"
- And I press "pool_family_submit"
- Then there should be 1 provider accounts assigned to "testpoolfamily"
- And I should see "testaccount"
- Scenario: Remove provider account from pool family
- Given 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"
- And I am on the pool family provider accounts page
- Then I should see "testaccount"
- When I check "testaccount" provider account
- And I press "Remove selected"
- Then there should not exist a provider account assigned to "testpoolfamily"
diff --git a/src/features/step_definitions/pool_family_steps.rb b/src/features/step_definitions/pool_family_steps.rb index 9585a21..716d722 100644 --- a/src/features/step_definitions/pool_family_steps.rb +++ b/src/features/step_definitions/pool_family_steps.rb @@ -5,7 +5,7 @@ Given /^there are these pool families:$/ do |table| end
Given /^there is a pool family named "([^\"]*)"$/ do |name|
- @provider = Factory(:pool_family, :name => name)
@pool_family = Factory(:pool_family, :name => name) end
Given /^there is not a pool family named "([^"]*)"$/ do |name|
@@ -25,3 +25,24 @@ end Then /^there should not exist a pool family named "([^"]*)"$/ do |name| PoolFamily.find_by_name(name).should be_nil end
+Then /^there should be (\d+) provider accounts assigned to "([^"]*)"$/ do |count,name|
- @pool_family = PoolFamily.find_by_name(name)
- @pool_family.provider_accounts.count == count
+end
+Given /^there is a provider account "([^"]*)" related to pool family "([^"]*)"$/ do |provider_account, pool_family|
- @pool_family = PoolFamily.find_by_name(pool_family)
- @provider_account = ProviderAccount.find_by_label(provider_account)
- @pool_family.provider_accounts<< @provider_account
+end
+When /^I check "([^"]*)" provider account$/ do |label|
- provider_account = ProviderAccount.find_by_label(label)
- check("provider_account_checkbox_#{provider_account.id}")
+end
+Then /^there should not exist a provider account assigned to "([^"]*)"$/ do |name|
- @pool_family = PoolFamily.find_by_name(name)
- @pool_family.provider_accounts.count == 0
+end diff --git a/src/features/support/paths.rb b/src/features/support/paths.rb index 5af9b34..a313aa0 100644 --- a/src/features/support/paths.rb +++ b/src/features/support/paths.rb @@ -83,6 +83,9 @@ module NavigationHelpers when /the template builds page/ url_for image_factory_template_path(@template, :details_tab => 'builds')
- when /the pool family provider accounts page/
url_for admin_pool_family_path(@pool_family, :details_tab => 'provider_accounts')
when /the templates page/ templates_path
Hey Jiri,
It looks good, but I'm afraid there's been some misunderstanding regarding how pool families work.
As far as I'm aware, a provider account must be associated with *exactly one* pool family at all times.
That means:
User should not be able to remove an account from the default pool family
When user removes an account from a non-default pool family, it
should be moved to the default one automatically
- When user adds a provider account to a pool family, it must be
removed from the family it was associated with previously
Ideally, the patch should enforce these rules and have tests for them.
However.
It's entirely possible that my understanding of the pool families is incorrect. We should clarify this with Scott once he gets online. If your patch mimics the correct behaviour, I'll ack and push it immediatelly.
Take care, Thomas
Hm, adding my understanding too:
- a provider account for a pool family is optional (if there is no
account, condor chooses from all accounts)
- there can be multiple accounts for the pool family (then condor
chooses from accounts defined for the pool family)
- account can be associated with multiple pool families
This is correct except for the first point. If there's no provider account for pool family, then that pool family has 0 capacity -- in other words, you can't launch any instances there.
The main use case for these things (pool families) is to segment providers for different use cases. So for example the dev PF might be backed only by cheap providers with low QOS, while the prod PF would have the expensive providers.
Take care, --H
Thank you all for clearing this up.
The patch does work as expected, then. ACK and I've pushed it into next.
Thomas