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