[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