[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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;" %>&nbsp;&nbsp;Add Host</a></li>
-    <li>
-      <a id="move_link" href="#" onClick="return validate_for_move();"><%= image_tag "icon_move.png", :style=>"vertical-align:middle;" %>&nbsp;&nbsp;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" %>&nbsp;&nbsp;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;" %>&nbsp;&nbsp;Add Host</a></li>
+    <li><a href="#" onClick="remove_hosts_from_smart_pool()"><%= image_tag "icon_remove.png" %>&nbsp;&nbsp;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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]