On Tue, 2010-08-03 at 15:44 +0200, jprov...@redhat.com wrote:
> From: Jan Provaznik <jprov...@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}?(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.
> 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
deltacloud-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
