[Ovirt-devel] [PATCH server 3/3] * app/controllers/host_controller.rb: factor biz logic out

[Scott Seago]

David Lutterkort wrote:
> On Thu, 2009-04-30 at 12:11 -0400, Scott Seago wrote:
>   
>> David Lutterkort wrote:
>>     
>>> (1) addhost should use PoolService somehow, need to look at Scott's
>>>     latest patch
>>>
>>>   
>>>       
>> I'm not sure it should use the service -- this is one of these wui-only 
>> form setup bits that I've been ignoring for now. I'm not sure how this 
>> fits into the service model.
>>     
>
> I've been following the line of 'no permission checks in the
> controllers' - maybe to a fault. That's the main reason why I'd like to
> have addhost use the Service layer - just for the permission check.
>
>   
Yes, see below -- it should be fine.
>>> (2) the way we handle unsupported actions is strange: we define them and
>>>     kick out a nice error - in reality, people can only get there either if
>>>     we have a bug somewhere or if they did URL surgery; either way, they
>>>     deserve a gory Rails error report. We should therefore remove the
>>>     actions and associated nice error messages
>>>       
>
> Any opinions on this ?
>
>   
I guess I"m not really sure. one line of thought is we should have a 
well-defined error message for all edge cases -- the other is "ugly 
errors for URL surgery".  I'm not sure it's important enough to spend 
too much time making it consistent, but perhaps we should for future 
issues not bother with nice errors for them.
>> OK, I think I understand what you're doing here -- I may have 
>> misunderstood svc_modify in my comments above -- is this meant as a 
>> distinct action from svc_update -- which we're using elsewhere to 
>> actually make changes? i.e. it's effectively svc_show with modify 
>> permissions enforcing? Does this belong in the svc layer? This seems 
>> more WUI-specific, as for the api/qmf case I'm not sure we'd be using 
>> this action. Looking below this is used for multiple modify actions, so 
>> perhaps it does belong here -- it just might not be exposed as a 
>> QMF-level API call -- am I understading this correctly now?
>>     
>
> My main intent with that was that all permission checking is pushed into
> the service layer, and when you write a controller you can just
> mindlessly call the 'right' service method - e.g., if you want to
> display something for modification to the user, you'd use svc_modify
> (obviously not the best name)
>
> I doubt we'd use this in the QMF API; but the main motivation is to make
> sure we have permission checks localized as much as possible so that
> changes to the permission model don't require use to hunt through the
> whole code base.
>
> Do you still think it's unnecessary ?
>
> David
>
>
>   
Sorry -- my reply was somewhat difficult to parse as I started to 
understand what you were doing there as I read down the patch. I do 
think we should be consistent with permission checks, and I hadn't yet 
figured out how I'd handle those wui-only actions, as the current state 
of the pool stuff is a bit confusing with some checks in before_filters 
and some in the service layer. I think the way you're doing it is fine 
here, and I'm thinking of doing the same to the pool and VM controllers 
in a follow-on patch when I add the rdoc stuff.

But what about controllers that have no QMF alternative? Do we go ahead 
and create a service layer for GraphController, DashboardController, etc.?


Scot