On 08/16/2010 11:25 PM, Jason Guiditta wrote:
> On Tue, 2010-08-03 at 15:44 +0200, [email protected] wrote:
>> From: Jan Provaznik<[email protected]>
>>
>> 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}?(o...@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
>
>
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
