[Ovirt-devel] [PATCH] finished smart pools implementation.

Scott Seago sseago at redhat.com
Thu Sep 18 21:06:59 UTC 2008


Jason Guiditta wrote:
> As discussed in irc, there are a couple pieces to be updated in here.
> Additionally, I have some comments/questions sprinkled throughout the
> patch.
>
> -j
>
> On Wed, 2008-09-17 at 17:23 -0400, Scott Seago wrote:
>   
>> @@ -116,8 +116,8 @@ class ApplicationController < ActionController::Base
>>      json_hash
>>    end
>>    # don't define find_opts for array inputs
>> -  def json_list(full_items, attributes, arg_list=[], find_opts={})
>> -    render :json => json_hash(full_items, attributes, arg_list, find_opts).to_json
>> +  def json_list(full_items, attributes, arg_list=[], find_opts={}, id_method=:id)
>> +    render :json => json_hash(full_items, attributes, arg_list, find_opts, id_method).to_json
>>    end
>>  
>>     
> General comment, doesn't have to be done now, but we need to get better
> names for these json methods.  We started down a bad path naming thing
> json_[x], where [x] is not terribly descriptive of what it is for.  I
> for one am finding it increasingly difficult to keep track of which of
> these methods we are using where.  At the very least, I think a comment
> saying something like 'this list is used for all flexigrids' for each
> json function would be extremely helpful.  
>   
Yeah, some further refactoring of controller structure is needed, 
possibly in conjunction with lutter's api work. Although these x_json 
methods are fairly specific to flexigrid -- i.e. the columns need to 
match what flexigrid is expecting, so perhaps hosts_for_flexigrid, etc. 
would be appropriate. Although the above is actually not one of these 
method. json_list is more of a helper/generic function to support the 
various x_json methods. So this one might be better listed as 
format_list_for_flexigrid, or something similar.

But this should be done as general cleanup, not for the smart pools 
stuff -- I will comment the above method to make the intent clearer.
>> +
>>     
> Is this block of comments intentionally still in here?  If so, could you
> clarify what it is referring to?
>
>   
Ugh. no -- I was using that to track my progress in implementing the 
various tabs, flexigrid bit, etc -- I'll gut it now...
>> +  # handled
>>     
...

>> diff --git a/src/app/views/hardware/show_hosts.rhtml b/src/app/views/hardware/show_hosts.rhtml
>> index 31c575d..5c34af3 100644
>> --- a/src/app/views/hardware/show_hosts.rhtml
>> +++ b/src/app/views/hardware/show_hosts.rhtml
>> @@ -26,7 +26,7 @@
>>    {
>>      hosts = get_selected_hosts()
>>      if (validate_selected(hosts, "host")) {
>> -      $.post('<%= url_for :controller => "hardware", :action => "move_hosts", :id => @pool %>',
>> +      $.post('<%= url_for :controller => "hardware", :action => "remove_hosts", :id => @pool %>',
>>               { resource_ids: hosts.toString(), target_pool_id: <%= HardwarePool.get_default_pool.id %> },
>>                function(data,status){
>>                  $tabs.tabs("load",$tabs.data('selected.tabs'));
>> @@ -35,15 +35,15 @@
>>                  }
>>  		if (hosts.indexOf($('#hosts_selection_id').html()) != -1){
>>  		  empty_summary('hosts_selection', 'Host')
>> -		}   
>> +		}
>>  
>>                 }, 'json');
>>      }
>>    }
>>    function hosts_select(selected_rows)
>>    {
>>     
> Please add semicolons where appropriate to the javascript (pretty much
> any line that is not a conditional or loop).  While browsers will let us
> get away without them, they really are supposed to be there.
> Additionally, not having them can make debugging tricky, as the browser
> can sometimes guess your intent incorrectly. Just mentioning this in one
> spot rather than all over the patch
>   
Sure I can do this here. I've been doing it for new code -- the above 
only changed to remove whitespace. So anything that doesn't have  them 
is probably code I haven't touched, or something that was mostly 
copy-and-paste. I'll fix any obvious examples in the patch though.

>> diff --git a/src/app/views/hardware/show_hosts.rhtml b/src/app/views/smart_pools/show_hosts.rhtml
>> similarity index 50%
>> copy from src/app/views/hardware/show_hosts.rhtml
>> copy to src/app/views/smart_pools/show_hosts.rhtml
>> index 31c575d..706c84f 100644
>> --- a/src/app/views/hardware/show_hosts.rhtml
>> +++ b/src/app/views/smart_pools/show_hosts.rhtml
>> @@ -1,89 +1,77 @@
>>  <div id="toolbar_nav">
>>   <ul>
>> -    <li><a href="<%= url_for :controller => 'host', :action => 'addhost', :hardware_pool_id => @pool %>" rel="facebox[.bolder]"><%= image_tag "icon_addhost.png", :style=>"vertical-align:middle;" %>  Add Host</a></li>
>> -    <li>
>> -      <a id="move_link" href="#" onClick="return validate_for_move();"><%= image_tag "icon_move.png", :style=>"vertical-align:middle;" %>  Move</a>
>> -      <a id="move_link_hidden" href="<%= url_for :controller => 'hardware', :action => 'move', :id => @pool, :resource_type=>'hosts' %>" rel="facebox[.bolder]" style="display:none" ></a>
>> -    </li>
>> -    <% if @pool.id != HardwarePool.get_default_pool.id %>
>> -      <li><a href="#" onClick="remove_hosts()"><%= image_tag "icon_remove.png" %>  Remove</a></li>
>> -    <% end %>
>> +    <li><a href="<%= url_for :controller => 'host', :action => 'add_to_smart_pool', :smart_pool_id => @pool %>" rel="facebox[.bolder]"><%= image_tag "icon_addhost.png", :style=>"vertical-align:middle;" %>  Add Host</a></li>
>> +    <li><a href="#" onClick="remove_hosts_from_smart_pool()"><%= image_tag "icon_remove.png" %>  Remove</a></li>
>>   </ul>
>>  </div>
>>  
>>  <script type="text/javascript">
>> -  function get_selected_hosts()
>> +  function get_selected_hosts_for_smart_pool()
>>    {
>> -    return get_selected_checkboxes("hosts_grid_form")
>> +    return get_selected_checkboxes("smart_hosts_grid_form")
>>    }
>> -  function validate_for_move()
>> +  function remove_hosts_from_smart_pool()
>>    {
>> -    if (validate_selected(get_selected_hosts(), 'host')) {
>> -      $('#move_link_hidden').click()
>> -    }
>> -  }
>> -  function remove_hosts()
>> -  {
>>     
> unless 'hosts' is declared somewhere I am not seeing, this line should
> start with 'var hosts'
>   
no, that's not declared elsewhere
/me adds 'var ' and finds the other places where it's done that way 
(since this was based on other code)

>> -    hosts = get_selected_hosts()
>> +    hosts = get_selected_hosts_for_smart_pool()
>>      if (validate_selected(hosts, "host")) {
>>     
> Same as with 'hosts', should be 'var pools'
>   
done
>> +    pools = get_selected_pools_for_smart_pool()
>> +    if (validate_selected(pools, "pool")) {
>> +      $.post('<%= url_for :controller => "smart_pools", :action => "remove_pools", :id => @pool %>',
>> +             { resource_ids: pools.toString() },
>> +              function(data,status){
>> +                $tabs.tabs("load",$tabs.data('selected.tabs'));
>> +		if (data.alert) {
>> +		  $.jGrowl(data.alert);
>> +                }
>> +		if (pools.indexOf($('#smart_pools_selection_id').html()) != -1){
>> +		  empty_summary('smart_pools_selection', 'Pool')
>> +		}
>> +
>> +               }, 'json');
>> +    }
>> +  }
>>
>>     
> var vms
>   
it's been var'ed too...
>> +    vms = get_selected_vms_for_smart_pool()
>> +    if (validate_selected(vms, "vm")) {
>> +      $.post('<%= url_for :controller => "smart_pools", :action => "remove_vms", :id => @pool %>',
>> +             { resource_ids: vms.toString() },
>> +              function(data,status){
>> +                $tabs.tabs("load",$tabs.data('selected.tabs'));
>> +		if (data.alert) {
>> +		  $.jGrowl(data.alert);
>> +                }
>> +		if (vms.indexOf($('#smart_vms_selection_id').html()) != -1){
>> +		  empty_summary('smart_vms_selection', 'Vm')
>> +		}
>> +
>> +               }, 'json');
>> +    }
>> +  }
>>     

>
> for code consistency, this should be $(document).trigger
>   
>> +      jQuery(document).trigger('close.facebox');
>> +    }
>> +}
>>     
/me changes this and the places he copied it from...


updated patch sent...

Scott




More information about the ovirt-devel mailing list