Hi, some minor fixes/changes, mostly in controllers, which I found when writing cucumber test for provider. It depends on "Removed cloud type field from new provider form" patch.
From: Jan Provaznik jprovazn@redhat.com
--- src/app/controllers/provider_controller.rb | 12 ++++++++++-- src/app/views/provider/_form.html.erb | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/app/controllers/provider_controller.rb b/src/app/controllers/provider_controller.rb index 7422c95..8ead3f3 100644 --- a/src/app/controllers/provider_controller.rb +++ b/src/app/controllers/provider_controller.rb @@ -35,10 +35,18 @@ class ProviderController < ApplicationController def new require_privilege(Privilege::PROVIDER_MODIFY) @provider = Provider.new(params[:provider]) - if request.post? && @provider.set_cloud_type && - @provider.save && @provider.populate_hardware_profiles + condormatic_classads_sync + end + + def create + require_privilege(Privilege::PROVIDER_MODIFY) + @provider = Provider.new(params[:provider]) + if @provider.set_cloud_type && @provider.save && + @provider.populate_hardware_profiles flash[:notice] = "Provider added." redirect_to :action => "show", :id => @provider + else + render :action => "new" end condormatic_classads_sync end diff --git a/src/app/views/provider/_form.html.erb b/src/app/views/provider/_form.html.erb index 6058894..789f34b 100644 --- a/src/app/views/provider/_form.html.erb +++ b/src/app/views/provider/_form.html.erb @@ -2,7 +2,7 @@ <%= error_messages_for 'provider' %>
<h2>Add a cloud provider</h2><br /> - <% form_tag :controller => :provider, :action => 'new' do %> + <% form_tag :controller => :provider, :action => 'create' do %> <ul> <li><label>Name<span>Provide a descriptive name for this provider connection.</span></label><%= text_field :provider, :name, :class => "txtfield" %></li> <li><label>URL<span>Enter the URL of the cloud provider.</span></label><%= text_field:provider, :url, :class => "txtfield" %></li>
On Tue, 2010-08-03 at 15:44 +0200, jprovazn@redhat.com wrote:
From: Jan Provaznik jprovazn@redhat.com
src/app/controllers/provider_controller.rb | 12 ++++++++++-- src/app/views/provider/_form.html.erb | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-)
This looks reasonable, but did not apply for me against current HEAD of next branch.
On 08/16/2010 10:48 PM, Jason Guiditta wrote:
On Tue, 2010-08-03 at 15:44 +0200, jprovazn@redhat.com wrote:
From: Jan Provaznikjprovazn@redhat.com
src/app/controllers/provider_controller.rb | 12 ++++++++++-- src/app/views/provider/_form.html.erb | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-)
This looks reasonable, but did not apply for me against current HEAD of next branch.
Sorry, I didn't realize this patch depends on patch "Removed cloud type field from new provider form". Could you please review that patch first (rspec test included)?
On Tue, 2010-08-17 at 16:28 +0200, Jan Provaznik wrote:
On 08/16/2010 10:48 PM, Jason Guiditta wrote:
On Tue, 2010-08-03 at 15:44 +0200, jprovazn@redhat.com wrote:
From: Jan Provaznikjprovazn@redhat.com
src/app/controllers/provider_controller.rb | 12 ++++++++++-- src/app/views/provider/_form.html.erb | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-)
This looks reasonable, but did not apply for me against current HEAD of next branch.
Sorry, I didn't realize this patch depends on patch "Removed cloud type field from new provider form". Could you please review that patch first (rspec test included)?
Yes, that makes it apply, and ACK to this one.
-j
From: Jan Provaznik jprovazn@redhat.com
Till now provider new form was displayed after successful delete. --- src/app/controllers/provider_controller.rb | 2 -- src/app/views/provider/_providers.haml | 7 +++++++ src/app/views/provider/index.haml | 1 + src/app/views/provider/index.html.erb | 1 - src/app/views/settings/index.haml | 9 +-------- 5 files changed, 9 insertions(+), 11 deletions(-) create mode 100644 src/app/views/provider/_providers.haml create mode 100644 src/app/views/provider/index.haml delete mode 100644 src/app/views/provider/index.html.erb
diff --git a/src/app/controllers/provider_controller.rb b/src/app/controllers/provider_controller.rb index 8ead3f3..eaf1d53 100644 --- a/src/app/controllers/provider_controller.rb +++ b/src/app/controllers/provider_controller.rb @@ -23,8 +23,6 @@ class ProviderController < ApplicationController before_filter :require_user
def index - # FIXME: this should be something other than 'new' here - render :action => 'new' end
def show diff --git a/src/app/views/provider/_providers.haml b/src/app/views/provider/_providers.haml new file mode 100644 index 0000000..4b8008e --- /dev/null +++ b/src/app/views/provider/_providers.haml @@ -0,0 +1,7 @@ +%h2 Providers +%ul#providers + - @providers.each do |provider| + %li + = link_to provider.name, {:controller => "provider", :action => "show", :id => provider }, :class => "providername" +- if BasePermissionObject.general_permission_scope.has_privilege(@current_user, Privilege::PROVIDER_MODIFY) + = link_to "Add a provider", {:controller => "provider", :action => "new"}, :class => "actionlink" diff --git a/src/app/views/provider/index.haml b/src/app/views/provider/index.haml new file mode 100644 index 0000000..9823f42 --- /dev/null +++ b/src/app/views/provider/index.haml @@ -0,0 +1 @@ += render :partial => 'providers' diff --git a/src/app/views/provider/index.html.erb b/src/app/views/provider/index.html.erb deleted file mode 100644 index 786950e..0000000 --- a/src/app/views/provider/index.html.erb +++ /dev/null @@ -1 +0,0 @@ -<%= render :partial => 'form' %> diff --git a/src/app/views/settings/index.haml b/src/app/views/settings/index.haml index e7030de..b7a708c 100644 --- a/src/app/views/settings/index.haml +++ b/src/app/views/settings/index.haml @@ -1,9 +1,2 @@ %h1 Settings - -%h2 Providers -%ul#providers - - @providers.each do |provider| - %li - = link_to provider.name, {:controller => "provider", :action => "show", :id => provider }, :class => "providername" -- if BasePermissionObject.general_permission_scope.has_privilege(@current_user, Privilege::PROVIDER_MODIFY) - = link_to "Add a provider", {:controller => "provider", :action => "new"}, :class => "actionlink" += render :partial => 'provider/providers'
On Tue, 2010-08-03 at 15:44 +0200, jprovazn@redhat.com wrote:
From: Jan Provaznik jprovazn@redhat.com
Till now provider new form was displayed after successful delete.
src/app/controllers/provider_controller.rb | 2 -- src/app/views/provider/_providers.haml | 7 +++++++ src/app/views/provider/index.haml | 1 + src/app/views/provider/index.html.erb | 1 - src/app/views/settings/index.haml | 9 +-------- 5 files changed, 9 insertions(+), 11 deletions(-) create mode 100644 src/app/views/provider/_providers.haml create mode 100644 src/app/views/provider/index.haml delete mode 100644 src/app/views/provider/index.html.erb
ACK, though with my usual response of 'where is the test?'. This should be a cucumber scenario. It could be as simple as verifying that the user is redirected to the list, and that the just-deleted provider is no longer shown. Not related to your patch, but that javascript alert of 'do you really want to delete provider' is icky.
-j
On 08/16/2010 11:00 PM, Jason Guiditta wrote:
On Tue, 2010-08-03 at 15:44 +0200, jprovazn@redhat.com wrote:
From: Jan Provaznikjprovazn@redhat.com
Till now provider new form was displayed after successful delete.
src/app/controllers/provider_controller.rb | 2 -- src/app/views/provider/_providers.haml | 7 +++++++ src/app/views/provider/index.haml | 1 + src/app/views/provider/index.html.erb | 1 - src/app/views/settings/index.haml | 9 +-------- 5 files changed, 9 insertions(+), 11 deletions(-) create mode 100644 src/app/views/provider/_providers.haml create mode 100644 src/app/views/provider/index.haml delete mode 100644 src/app/views/provider/index.html.erb
ACK, though with my usual response of 'where is the test?'. This should be a cucumber scenario. It could be as simple as verifying that the user is redirected to the list, and that the just-deleted provider is no longer shown. Not related to your patch, but that javascript alert of 'do you really want to delete provider' is icky.
-j
Sorry, I sent cucumber provider test in separate thread "Cucumber test for provider". Sent again revisioned version of cuke test to this thread.
From: Jan Provaznik jprovazn@redhat.com
As we dropped side nav panel, it's not necessary to load providers and pools for each request but only when it's needed.
get_nav_items before_filter is not removed completely because of side_panel is still used in instances and pools controllers. I also moved get_nav_items between protected methods and added require_user before_filter into settings and quota controllers (side effect of get_nav_items is setting @current_user) --- src/app/controllers/application_controller.rb | 16 +++++++--------- src/app/controllers/dashboard_controller.rb | 2 ++ src/app/controllers/instance_controller.rb | 2 +- src/app/controllers/pool_controller.rb | 2 +- src/app/controllers/provider_controller.rb | 1 + src/app/controllers/quota_controller.rb | 1 + src/app/controllers/settings_controller.rb | 5 +++++ 7 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb index a563d57..948aa14 100644 --- a/src/app/controllers/application_controller.rb +++ b/src/app/controllers/application_controller.rb @@ -30,8 +30,6 @@ class ApplicationController < ActionController::Base init_gettext "ovirt" layout :choose_layout
- before_filter :get_nav_items - # General error handlers, must be in order from least specific # to most specific rescue_from Exception, :with => :handle_general_error @@ -48,13 +46,6 @@ class ApplicationController < ActionController::Base return @layout end
- def get_nav_items - if !current_user.nil? - @providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW) - @pools = Pool.list_for_user(@current_user, Privilege::POOL_VIEW) - end - end - perm_helper_string = "" Privilege::FULL_PRIVILEGE_LIST.each do |privilege| perm_helper_string += "def has_#{privilege}?(obj=@perm_obj); " + @@ -155,6 +146,13 @@ class ApplicationController < ActionController::Base render :json => json_hash(full_items, attributes, arg_list, find_opts, id_method).to_json end
+ def get_nav_items + if !current_user.nil? + @providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW) + @pools = Pool.list_for_user(@current_user, Privilege::POOL_VIEW) + end + end + private def json_error_hash(msg, status) json = {} diff --git a/src/app/controllers/dashboard_controller.rb b/src/app/controllers/dashboard_controller.rb index 757598d..3786896 100644 --- a/src/app/controllers/dashboard_controller.rb +++ b/src/app/controllers/dashboard_controller.rb @@ -77,6 +77,8 @@ class DashboardController < ApplicationController @current_users_pool = Pool.find(:first, :conditions => ['name = ?', @current_user.login]) @cloud_accounts = CloudAccount.list_for_user(@current_user, Privilege::ACCOUNT_VIEW) @stats = Instance.get_user_instances_stats(@current_user) + @providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW) + @pools = Pool.list_for_user(@current_user, Privilege::POOL_VIEW) render :action => :summary end
diff --git a/src/app/controllers/instance_controller.rb b/src/app/controllers/instance_controller.rb index f65f7c1..fcb237e 100644 --- a/src/app/controllers/instance_controller.rb +++ b/src/app/controllers/instance_controller.rb @@ -22,7 +22,7 @@ require 'util/condormatic'
class InstanceController < ApplicationController - before_filter :require_user + before_filter :require_user, :get_nav_items layout :layout
def layout diff --git a/src/app/controllers/pool_controller.rb b/src/app/controllers/pool_controller.rb index 19effa9..41c5369 100644 --- a/src/app/controllers/pool_controller.rb +++ b/src/app/controllers/pool_controller.rb @@ -23,7 +23,7 @@ require 'util/taskomatic' require 'util/condormatic'
class PoolController < ApplicationController - before_filter :require_user + before_filter :require_user, :get_nav_items
def index render :action => 'new' diff --git a/src/app/controllers/provider_controller.rb b/src/app/controllers/provider_controller.rb index eaf1d53..9058c3e 100644 --- a/src/app/controllers/provider_controller.rb +++ b/src/app/controllers/provider_controller.rb @@ -23,6 +23,7 @@ class ProviderController < ApplicationController before_filter :require_user
def index + @providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW) end
def show diff --git a/src/app/controllers/quota_controller.rb b/src/app/controllers/quota_controller.rb index c462f45..48cb997 100644 --- a/src/app/controllers/quota_controller.rb +++ b/src/app/controllers/quota_controller.rb @@ -21,6 +21,7 @@
class QuotaController < ApplicationController + before_filter :require_user
def show @parent = get_parent_object(params) diff --git a/src/app/controllers/settings_controller.rb b/src/app/controllers/settings_controller.rb index 7d41203..efeec2d 100644 --- a/src/app/controllers/settings_controller.rb +++ b/src/app/controllers/settings_controller.rb @@ -20,5 +20,10 @@ # Likewise, all the methods added will be available for all controllers.
class SettingsController < ApplicationController + before_filter :require_user + + def index + @providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW) + end
end
On Tue, 2010-08-03 at 15:44 +0200, jprovazn@redhat.com wrote:
From: Jan Provaznik jprovazn@redhat.com
As we dropped side nav panel, it's not necessary to load providers and pools for each request but only when it's needed.
get_nav_items before_filter is not removed completely because of side_panel is still used in instances and pools controllers. I also moved get_nav_items between protected methods and added require_user before_filter into settings and quota controllers (side effect of get_nav_items is setting @current_user)
Mostly ACK, minor fix suggestion inline. Also, maybe it would make sense to move the require_user filter up to application_controller, and just have a few exceptions, as there are not many pages you can get to in the app w/o being logged in? If so, that would be a simple scenario added to the authorization feature checking if the user is redirected to login page and receives appropriate error message if he attempts to access a secure page.
src/app/controllers/application_controller.rb | 16 +++++++--------- src/app/controllers/dashboard_controller.rb | 2 ++ src/app/controllers/instance_controller.rb | 2 +- src/app/controllers/pool_controller.rb | 2 +- src/app/controllers/provider_controller.rb | 1 + src/app/controllers/quota_controller.rb | 1 + src/app/controllers/settings_controller.rb | 5 +++++ 7 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb index a563d57..948aa14 100644 --- a/src/app/controllers/application_controller.rb +++ b/src/app/controllers/application_controller.rb @@ -30,8 +30,6 @@ class ApplicationController < ActionController::Base init_gettext "ovirt" layout :choose_layout
- before_filter :get_nav_items
- # General error handlers, must be in order from least specific # to most specific rescue_from Exception, :with => :handle_general_error
@@ -48,13 +46,6 @@ class ApplicationController < ActionController::Base return @layout end
- def get_nav_items
- if !current_user.nil?
@providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW)@pools = Pool.list_for_user(@current_user, Privilege::POOL_VIEW)- end
- end
- perm_helper_string = "" Privilege::FULL_PRIVILEGE_LIST.each do |privilege| perm_helper_string += "def has_#{privilege}?(obj=@perm_obj); " +
@@ -155,6 +146,13 @@ class ApplicationController < ActionController::Base render :json => json_hash(full_items, attributes, arg_list, find_opts, id_method).to_json end
- def get_nav_items
- if !current_user.nil?
@providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW)@pools = Pool.list_for_user(@current_user, Privilege::POOL_VIEW)- end
- end
- private def json_error_hash(msg, status) json = {}
diff --git a/src/app/controllers/dashboard_controller.rb b/src/app/controllers/dashboard_controller.rb index 757598d..3786896 100644 --- a/src/app/controllers/dashboard_controller.rb +++ b/src/app/controllers/dashboard_controller.rb @@ -77,6 +77,8 @@ class DashboardController < ApplicationController @current_users_pool = Pool.find(:first, :conditions => ['name = ?', @current_user.login]) @cloud_accounts = CloudAccount.list_for_user(@current_user, Privilege::ACCOUNT_VIEW) @stats = Instance.get_user_instances_stats(@current_user)
- @providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW)
- @pools = Pool.list_for_user(@current_user, Privilege::POOL_VIEW) render :action => :summary end
Instead of adding the 2 lines above, this should probably just have :get_nav_items added as a filter, like you do below with Instance controller.
diff --git a/src/app/controllers/instance_controller.rb b/src/app/controllers/instance_controller.rb index f65f7c1..fcb237e 100644 --- a/src/app/controllers/instance_controller.rb +++ b/src/app/controllers/instance_controller.rb @@ -22,7 +22,7 @@ require 'util/condormatic'
class InstanceController < ApplicationController
- before_filter :require_user
before_filter :require_user, :get_nav_items layout :layout
def layout
diff --git a/src/app/controllers/pool_controller.rb b/src/app/controllers/pool_controller.rb index 19effa9..41c5369 100644 --- a/src/app/controllers/pool_controller.rb +++ b/src/app/controllers/pool_controller.rb @@ -23,7 +23,7 @@ require 'util/taskomatic' require 'util/condormatic'
class PoolController < ApplicationController
- before_filter :require_user
before_filter :require_user, :get_nav_items
def index render :action => 'new'
diff --git a/src/app/controllers/provider_controller.rb b/src/app/controllers/provider_controller.rb index eaf1d53..9058c3e 100644 --- a/src/app/controllers/provider_controller.rb +++ b/src/app/controllers/provider_controller.rb @@ -23,6 +23,7 @@ class ProviderController < ApplicationController before_filter :require_user
def index
@providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW) end
def show
diff --git a/src/app/controllers/quota_controller.rb b/src/app/controllers/quota_controller.rb index c462f45..48cb997 100644 --- a/src/app/controllers/quota_controller.rb +++ b/src/app/controllers/quota_controller.rb @@ -21,6 +21,7 @@
class QuotaController < ApplicationController
before_filter :require_user
def show @parent = get_parent_object(params)
diff --git a/src/app/controllers/settings_controller.rb b/src/app/controllers/settings_controller.rb index 7d41203..efeec2d 100644 --- a/src/app/controllers/settings_controller.rb +++ b/src/app/controllers/settings_controller.rb @@ -20,5 +20,10 @@ # Likewise, all the methods added will be available for all controllers.
class SettingsController < ApplicationController
- before_filter :require_user
- def index
- @providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW)
- end
end
On 08/16/2010 11:25 PM, Jason Guiditta wrote:
On Tue, 2010-08-03 at 15:44 +0200, jprovazn@redhat.com wrote:
From: Jan Provaznikjprovazn@redhat.com
As we dropped side nav panel, it's not necessary to load providers and pools for each request but only when it's needed.
get_nav_items before_filter is not removed completely because of side_panel is still used in instances and pools controllers. I also moved get_nav_items between protected methods and added require_user before_filter into settings and quota controllers (side effect of get_nav_items is setting @current_user)
Mostly ACK, minor fix suggestion inline. Also, maybe it would make sense to move the require_user filter up to application_controller, and just have a few exceptions, as there are not many pages you can get to in the app w/o being logged in? If so, that would be a simple scenario added to the authorization feature checking if the user is redirected to login page and receives appropriate error message if he attempts to access a secure page.
Do you mind, if moving require_user to application_controller will be separate patch?
src/app/controllers/application_controller.rb | 16 +++++++--------- src/app/controllers/dashboard_controller.rb | 2 ++ src/app/controllers/instance_controller.rb | 2 +- src/app/controllers/pool_controller.rb | 2 +- src/app/controllers/provider_controller.rb | 1 + src/app/controllers/quota_controller.rb | 1 + src/app/controllers/settings_controller.rb | 5 +++++ 7 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb index a563d57..948aa14 100644 --- a/src/app/controllers/application_controller.rb +++ b/src/app/controllers/application_controller.rb @@ -30,8 +30,6 @@ class ApplicationController< ActionController::Base init_gettext "ovirt" layout :choose_layout
- before_filter :get_nav_items
- # General error handlers, must be in order from least specific # to most specific rescue_from Exception, :with => :handle_general_error
@@ -48,13 +46,6 @@ class ApplicationController< ActionController::Base return @layout end
- def get_nav_items
- if !current_user.nil?
@providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW)@pools = Pool.list_for_user(@current_user, Privilege::POOL_VIEW)- end
- end
- perm_helper_string = "" Privilege::FULL_PRIVILEGE_LIST.each do |privilege| perm_helper_string += "def has_#{privilege}?(obj=@perm_obj); " +
@@ -155,6 +146,13 @@ class ApplicationController< ActionController::Base render :json => json_hash(full_items, attributes, arg_list, find_opts, id_method).to_json end
- def get_nav_items
- if !current_user.nil?
@providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW)@pools = Pool.list_for_user(@current_user, Privilege::POOL_VIEW)- end
- end
- private def json_error_hash(msg, status) json = {}
diff --git a/src/app/controllers/dashboard_controller.rb b/src/app/controllers/dashboard_controller.rb index 757598d..3786896 100644 --- a/src/app/controllers/dashboard_controller.rb +++ b/src/app/controllers/dashboard_controller.rb @@ -77,6 +77,8 @@ class DashboardController< ApplicationController @current_users_pool = Pool.find(:first, :conditions => ['name = ?', @current_user.login]) @cloud_accounts = CloudAccount.list_for_user(@current_user, Privilege::ACCOUNT_VIEW) @stats = Instance.get_user_instances_stats(@current_user)
- @providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW)
- @pools = Pool.list_for_user(@current_user, Privilege::POOL_VIEW) render :action => :summary end
Instead of adding the 2 lines above, this should probably just have :get_nav_items added as a filter, like you do below with Instance controller.
Right. Will replace with: before_filter :get_nav_items, :only => [:index]
in final push.
diff --git a/src/app/controllers/instance_controller.rb b/src/app/controllers/instance_controller.rb index f65f7c1..fcb237e 100644 --- a/src/app/controllers/instance_controller.rb +++ b/src/app/controllers/instance_controller.rb @@ -22,7 +22,7 @@ require 'util/condormatic'
class InstanceController< ApplicationController
- before_filter :require_user
before_filter :require_user, :get_nav_items layout :layout
def layout
diff --git a/src/app/controllers/pool_controller.rb b/src/app/controllers/pool_controller.rb index 19effa9..41c5369 100644 --- a/src/app/controllers/pool_controller.rb +++ b/src/app/controllers/pool_controller.rb @@ -23,7 +23,7 @@ require 'util/taskomatic' require 'util/condormatic'
class PoolController< ApplicationController
- before_filter :require_user
before_filter :require_user, :get_nav_items
def index render :action => 'new'
diff --git a/src/app/controllers/provider_controller.rb b/src/app/controllers/provider_controller.rb index eaf1d53..9058c3e 100644 --- a/src/app/controllers/provider_controller.rb +++ b/src/app/controllers/provider_controller.rb @@ -23,6 +23,7 @@ class ProviderController< ApplicationController before_filter :require_user
def index
@providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW) end
def show
diff --git a/src/app/controllers/quota_controller.rb b/src/app/controllers/quota_controller.rb index c462f45..48cb997 100644 --- a/src/app/controllers/quota_controller.rb +++ b/src/app/controllers/quota_controller.rb @@ -21,6 +21,7 @@
class QuotaController< ApplicationController
before_filter :require_user
def show @parent = get_parent_object(params)
diff --git a/src/app/controllers/settings_controller.rb b/src/app/controllers/settings_controller.rb index 7d41203..efeec2d 100644 --- a/src/app/controllers/settings_controller.rb +++ b/src/app/controllers/settings_controller.rb @@ -20,5 +20,10 @@ # Likewise, all the methods added will be available for all controllers.
class SettingsController< ApplicationController
before_filter :require_user
def index
@providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_VIEW)
end
end
On Tue, 2010-08-17 at 16:57 +0200, Jan Provaznik wrote:
On 08/16/2010 11:25 PM, Jason Guiditta wrote:
On Tue, 2010-08-03 at 15:44 +0200, jprovazn@redhat.com wrote:
From: Jan Provaznikjprovazn@redhat.com
As we dropped side nav panel, it's not necessary to load providers and pools for each request but only when it's needed.
get_nav_items before_filter is not removed completely because of side_panel is still used in instances and pools controllers. I also moved get_nav_items between protected methods and added require_user before_filter into settings and quota controllers (side effect of get_nav_items is setting @current_user)
Mostly ACK, minor fix suggestion inline. Also, maybe it would make sense to move the require_user filter up to application_controller, and just have a few exceptions, as there are not many pages you can get to in the app w/o being logged in? If so, that would be a simple scenario added to the authorization feature checking if the user is redirected to login page and receives appropriate error message if he attempts to access a secure page.
Do you mind, if moving require_user to application_controller will be separate patch?
Separate patch for that is fine sure, just occurred to me as I was looking through this yesterday.
-j
From: Jan Provaznik jprovazn@redhat.com
--- src/features/provider.feature | 56 +++++++++++++++++++++++ src/features/step_definitions/pool_steps.rb | 2 +- src/features/step_definitions/provider_steps.rb | 28 +++++++++++ src/features/support/custom.rb | 15 ++++++- src/features/support/paths.rb | 17 +++++-- 5 files changed, 112 insertions(+), 6 deletions(-) create mode 100644 src/features/provider.feature create mode 100644 src/features/step_definitions/provider_steps.rb
diff --git a/src/features/provider.feature b/src/features/provider.feature new file mode 100644 index 0000000..d1b10a6 --- /dev/null +++ b/src/features/provider.feature @@ -0,0 +1,56 @@ +Feature: Manage Providers + In order to manage my cloud infrastructure + As a user + I want to manage cloud providers + + Background: + Given I am an authorised user + And I am logged in + + Scenario: List providers + Given I am on the homepage + And there are these providers: + | name | + | provider1 | + | provider2 | + | provider3 | + When I follow "Settings" + Then I should see the following: + | provider1 | + | provider2 | + | provider3 | + + Scenario: Show provider details + Given there is a provider named "testprovider" + And I am on the settings page + When I follow "testprovider" + Then I should see "Accounts" + And I should see "Realms" + And I should see "User access" + And I should see "Settings" + + Scenario: Create a new Provider + Given I am on the settings page + And there is not a provider named "testprovider" + When I follow "Add a provider" + Then I should be on the new provider page + And I should see "Add a cloud provider" + When I fill in "provider[name]" with "testprovider" + And I fill in "provider[url]" with "http://localhost:3001/api" + And I press "Save" + Then I should be on the show provider page + And I should see "Provider added" + And I should have a provider named "testprovider" + + Scenario: Delete a provider + Given there is a provider named "testprovider" + And I am on the settings page + Then I should see "testprovider" + When I follow "testprovider" + Then I should be on the show provider page + When I follow provider settings link + Then I should be on the provider settings page + When I delete provider + Then I should be on the providers page + And I should see "Providers" + And I should not see "testprovider" diff --git a/src/features/step_definitions/pool_steps.rb b/src/features/step_definitions/pool_steps.rb index af0cba1..75de2e2 100644 --- a/src/features/step_definitions/pool_steps.rb +++ b/src/features/step_definitions/pool_steps.rb @@ -67,4 +67,4 @@ Given /^the Pool has a quota with following capacities:$/ do |table| @quota = Factory(:quota, quota_hash) @pool.quota_id = @quota.id @pool.save -end \ No newline at end of file +end diff --git a/src/features/step_definitions/provider_steps.rb b/src/features/step_definitions/provider_steps.rb new file mode 100644 index 0000000..e593d43 --- /dev/null +++ b/src/features/step_definitions/provider_steps.rb @@ -0,0 +1,28 @@ +Given /^there is not a provider named "([^"]*)"$/ do |name| + Provider.find_by_name(name).should be_nil +end + +Given /^there is a provider named "([^"]*)"$/ do |name| + @provider = Factory(:mock_provider, :name => name) +end + +Then /^I should have a provider named "([^"]*)"$/ do |name| + Provider.find_by_name(name).should_not be_nil +end + +When /^I follow provider settings link$/ do + within '#provider-tabs' do |scope| + scope.click_link "Settings" + end +end + +When /^I delete provider$/ do + click_button "Delete provider" +end + + +Given /^there are these providers:$/ do |table| + table.hashes.each do |hash| + Factory(:mock_provider, :name => hash['name']) + end +end diff --git a/src/features/support/custom.rb b/src/features/support/custom.rb index e8801ba..446aa89 100644 --- a/src/features/support/custom.rb +++ b/src/features/support/custom.rb @@ -11,10 +11,23 @@ Provider.class_eval do def valid_framework? true end + + def set_cloud_type + self.cloud_type = Factory(:mock_provider).cloud_type + end + + def populate_hardware_profiles + [[:mock_hwp1, :agg_hwp1], [:mock_hwp2, :agg_hwp2]].each do |mp_name, ap_name| + mock = Factory(mp_name, :provider_id => self.id) + agg_mock = Factory(ap_name, :external_key => self.name + Realm::AGGREGATOR_REALM_ACCOUNT_DELIMITER + mock.name) + agg_mock.provider_hardware_profiles << mock + end + return true + end end
CloudAccount.class_eval do def valid_credentials? true end -end \ No newline at end of file +end diff --git a/src/features/support/paths.rb b/src/features/support/paths.rb index db20948..b7adf16 100644 --- a/src/features/support/paths.rb +++ b/src/features/support/paths.rb @@ -14,9 +14,6 @@ module NavigationHelpers when /the new account page/ register_path
- when /the provider list page/ - '/' - when /the login page/ login_path
@@ -29,8 +26,17 @@ module NavigationHelpers when /the login error page/ user_session_path
+ when /the providers page/ + url_for :controller => 'provider', :action => 'index', :only_path => true + when /the new provider page/ - new_provider_path + url_for :controller => 'provider', :action => 'new', :only_path => true + + when /the show provider page/ + url_for :controller => 'provider', :action => 'show', :only_path => true + + when /the provider settings page/ + url_for :controller => 'provider', :action => 'settings', :only_path => true
when /the new pool page/ new_pool_path @@ -50,6 +56,9 @@ module NavigationHelpers when /the pool hardware profiles page/ hardware_profiles_pool_path
+ when /the settings page/ + settings_path + # Add more mappings here. # Here is an example that pulls values out of the Regexp: #
ACK
Needs rebasing, but the cucumber tests pass.
Thomas
----- Original Message ----- From: jprovazn@redhat.com To: deltacloud-devel@lists.fedorahosted.org Sent: Tuesday, August 17, 2010 4:22:04 PM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna Subject: [deltacloud-devel] [PATCH aggregator] Cucumber test for provider (rev 1)
From: Jan Provaznik jprovazn@redhat.com
--- src/features/provider.feature | 56 +++++++++++++++++++++++ src/features/step_definitions/pool_steps.rb | 2 +- src/features/step_definitions/provider_steps.rb | 28 +++++++++++ src/features/support/custom.rb | 15 ++++++- src/features/support/paths.rb | 17 +++++-- 5 files changed, 112 insertions(+), 6 deletions(-) create mode 100644 src/features/provider.feature create mode 100644 src/features/step_definitions/provider_steps.rb
diff --git a/src/features/provider.feature b/src/features/provider.feature new file mode 100644 index 0000000..d1b10a6 --- /dev/null +++ b/src/features/provider.feature @@ -0,0 +1,56 @@ +Feature: Manage Providers + In order to manage my cloud infrastructure + As a user + I want to manage cloud providers + + Background: + Given I am an authorised user + And I am logged in + + Scenario: List providers + Given I am on the homepage + And there are these providers: + | name | + | provider1 | + | provider2 | + | provider3 | + When I follow "Settings" + Then I should see the following: + | provider1 | + | provider2 | + | provider3 | + + Scenario: Show provider details + Given there is a provider named "testprovider" + And I am on the settings page + When I follow "testprovider" + Then I should see "Accounts" + And I should see "Realms" + And I should see "User access" + And I should see "Settings" + + Scenario: Create a new Provider + Given I am on the settings page + And there is not a provider named "testprovider" + When I follow "Add a provider" + Then I should be on the new provider page + And I should see "Add a cloud provider" + When I fill in "provider[name]" with "testprovider" + And I fill in "provider[url]" with "http://localhost:3001/api" + And I press "Save" + Then I should be on the show provider page + And I should see "Provider added" + And I should have a provider named "testprovider" + + Scenario: Delete a provider + Given there is a provider named "testprovider" + And I am on the settings page + Then I should see "testprovider" + When I follow "testprovider" + Then I should be on the show provider page + When I follow provider settings link + Then I should be on the provider settings page + When I delete provider + Then I should be on the providers page + And I should see "Providers" + And I should not see "testprovider" diff --git a/src/features/step_definitions/pool_steps.rb b/src/features/step_definitions/pool_steps.rb index af0cba1..75de2e2 100644 --- a/src/features/step_definitions/pool_steps.rb +++ b/src/features/step_definitions/pool_steps.rb @@ -67,4 +67,4 @@ Given /^the Pool has a quota with following capacities:$/ do |table| @quota = Factory(:quota, quota_hash) @pool.quota_id = @quota.id @pool.save -end \ No newline at end of file +end diff --git a/src/features/step_definitions/provider_steps.rb b/src/features/step_definitions/provider_steps.rb new file mode 100644 index 0000000..e593d43 --- /dev/null +++ b/src/features/step_definitions/provider_steps.rb @@ -0,0 +1,28 @@ +Given /^there is not a provider named "([^"]*)"$/ do |name| + Provider.find_by_name(name).should be_nil +end + +Given /^there is a provider named "([^"]*)"$/ do |name| + @provider = Factory(:mock_provider, :name => name) +end + +Then /^I should have a provider named "([^"]*)"$/ do |name| + Provider.find_by_name(name).should_not be_nil +end + +When /^I follow provider settings link$/ do + within '#provider-tabs' do |scope| + scope.click_link "Settings" + end +end + +When /^I delete provider$/ do + click_button "Delete provider" +end + + +Given /^there are these providers:$/ do |table| + table.hashes.each do |hash| + Factory(:mock_provider, :name => hash['name']) + end +end diff --git a/src/features/support/custom.rb b/src/features/support/custom.rb index e8801ba..446aa89 100644 --- a/src/features/support/custom.rb +++ b/src/features/support/custom.rb @@ -11,10 +11,23 @@ Provider.class_eval do def valid_framework? true end + + def set_cloud_type + self.cloud_type = Factory(:mock_provider).cloud_type + end + + def populate_hardware_profiles + [[:mock_hwp1, :agg_hwp1], [:mock_hwp2, :agg_hwp2]].each do |mp_name, ap_name| + mock = Factory(mp_name, :provider_id => self.id) + agg_mock = Factory(ap_name, :external_key => self.name + Realm::AGGREGATOR_REALM_ACCOUNT_DELIMITER + mock.name) + agg_mock.provider_hardware_profiles << mock + end + return true + end end
CloudAccount.class_eval do def valid_credentials? true end -end \ No newline at end of file +end diff --git a/src/features/support/paths.rb b/src/features/support/paths.rb index db20948..b7adf16 100644 --- a/src/features/support/paths.rb +++ b/src/features/support/paths.rb @@ -14,9 +14,6 @@ module NavigationHelpers when /the new account page/ register_path
- when /the provider list page/ - '/' - when /the login page/ login_path
@@ -29,8 +26,17 @@ module NavigationHelpers when /the login error page/ user_session_path
+ when /the providers page/ + url_for :controller => 'provider', :action => 'index', :only_path => true + when /the new provider page/ - new_provider_path + url_for :controller => 'provider', :action => 'new', :only_path => true + + when /the show provider page/ + url_for :controller => 'provider', :action => 'show', :only_path => true + + when /the provider settings page/ + url_for :controller => 'provider', :action => 'settings', :only_path => true
when /the new pool page/ new_pool_path @@ -50,6 +56,9 @@ module NavigationHelpers when /the pool hardware profiles page/ hardware_profiles_pool_path
+ when /the settings page/ + settings_path + # Add more mappings here. # Here is an example that pulls values out of the Regexp: #
deltacloud-devel@lists.fedorahosted.org
-
Jan Provazník -
Jason Guiditta -
Tomas Sedovic