[Ovirt-devel] [PATCH] Users are displayed from LDAP and filtered based on the current list of users.

[Scott Seago]

Darryl L. Pierce wrote:
> diff --git a/wui/src/app/controllers/permission_controller.rb b/wui/src/app/controllers/permission_controller.rb
> index 086e183..c0cbc38 100644
> --- a/wui/src/app/controllers/permission_controller.rb
> +++ b/wui/src/app/controllers/permission_controller.rb
> @@ -39,6 +39,8 @@ class PermissionController < ApplicationController
>    def new
>      @permission = Permission.new( { :pool_id => params[:pool_id]})
>      @perms = @permission.pool.permissions
> +    filter = Permission.find(:all).collect{ |permission| permission.uid }
> +    @users = Account.names(filter)
>      set_perms(@permission.pool)
>      # admin permission required to view permissions
>      unless @can_set_perms
>   
You're filtering out _all_ users that have a permission somewhere -- 
this should just filter out users that already have permissions on 
_this_ pool.
Something like this would be better:
@pool = Pool.find(params[:pool_id])
filter = @pool.permissions.collect{ |permission| permission.uid }

> diff --git a/wui/src/app/models/account.rb b/wui/src/app/models/account.rb
> index 2664f18..e3a1a54 100644
> --- a/wui/src/app/models/account.rb
> +++ b/wui/src/app/models/account.rb
> @@ -22,6 +22,8 @@
>  class Account < ActiveLdap::Base
>    ldap_mapping :dn_attribute => 'cn', :prefix => 'ou=Users', :scope => :one
>  
> +  @@users = nil
> +
>    # +query+ returns the set of all accounts that contain the given search value.
>    #
>    # This API requires that a previous connection be made using 
> @@ -29,13 +31,33 @@ class Account < ActiveLdap::Base
>    #
>    def Account.query(value)
>  
> -    @users = Account.find(:all, value)
> +    @@users ||= Account.find(:all, value)
>  
>      if block_given?
> -      @users.each { |user| yield(user) }
> +      @@users.each { |user| yield(user) }
>      end
>  
> -    return @users
> -    
> +    @@users    
>    end
>   
Do you really need a class variable here? This could cause unnecessary 
concurrency problems. Since you aren't referencing it anywhere else why 
not just a plain local variable?


Scott