[Ovirt-devel] [PATCH server 05/10] PermissionController now uses the service layer

[David Lutterkort]

On Tue, 2009-05-19 at 14:23 +0000, Scott Seago wrote:
> Signed-off-by: Scott Seago <sseago at redhat.com>
> ---
>  src/app/controllers/permission_controller.rb |  129 +++++++++++---------------
>  src/app/models/permission.rb                 |    4 +
>  src/app/services/permission_service.rb       |  102 ++++++++++++++++++++
>  3 files changed, 160 insertions(+), 75 deletions(-)
>  create mode 100644 src/app/services/permission_service.rb

ACK, after fixing up the issue noted below:

> diff --git a/src/app/controllers/permission_controller.rb b/src/app/controllers/permission_controller.rb
> index d4c3fb5..55e7942 100644
> --- a/src/app/controllers/permission_controller.rb
> +++ b/src/app/controllers/permission_controller.rb
> @@ -18,105 +18,84 @@
>  # also available at http://www.gnu.org/copyleft/gpl.html.
>  
>  class PermissionController < ApplicationController

> -  #FIXME: we need permissions checks. user must have permission. We also need to fail
> -  # for pools that aren't currently empty
>    def update_roles
> -    role = params[:role_id]
> -    permission_ids_str = params[:permission_ids]
> -    permission_ids = permission_ids_str.split(",").collect {|x| x.to_i}
> -
> -    begin
> -      Permission.transaction do
> -        permissions = Permission.find(:all, :conditions => "id in (#{permission_ids.join(', ')})")
> -        permissions.each do |permission|
> -          permission.update_role(role) if permission.is_primary?
> -        end
> +    permission_ids = params[:permission_ids].split(",")
> +    role_id = params[:role_id]
> +    successes = []
> +    failures = {}
> +    permission_ids.each do |permission_id|
> +      begin
> +        svc_update_role(permission_id, role_id)
> +        successes << @permission
> +      rescue PermissionError => perm_error
> +        failures[@permission] = perm_error.message
> +      rescue ActionError => ex
> +        failures[@permission] = ex.message
> +     rescue Exception => ex
> +        failures[@permission] = ex.message

This assumes that @permission is set by svc_update_role in all cases -
that's not necessarily true, for example when permission_id doesn't
exist it in the DB.

Also, since this rescues Exception, there's no need for the other two
clauses.
 
> -  #FIXME: we need permissions checks. user must have permission. We also need to fail
> -  # for pools that aren't currently empty
>    def delete
> -    permission_ids_str = params[:permission_ids]
> -    permission_ids = permission_ids_str.split(",").collect {|x| x.to_i}
> -
> -    begin
> -      Permission.transaction do
> -        permissions = Permission.find(:all, :conditions => "id in (#{permission_ids.join(', ')})")
> -        permissions.each do |permission|
> -          permission.destroy if permission.is_primary?
> -        end
> +    permission_ids = params[:permission_ids].split(",")
> +    successes = []
> +    failures = {}
> +    permission_ids.each do |permission_id|
> +      begin
> +        svc_destroy(permission_id)
> +        successes << @permission
> +      rescue PermissionError => perm_error
> +        failures[@permission] = perm_error.message
> +      rescue Exception => ex
> +        failures[@permission] = ex.message
>        end

This has the same issue as above.

David