On Tue, 2010-08-03 at 15:44 +0200, jprovazn(a)redhat.com wrote:
From: Jan Provaznik <jprovazn(a)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