[Ovirt-devel] [PATCH server] Add of a button destroy for disabled hosts.

[Scott Seago]

Hi Silvain,

Overall this looks good -- a couple comments inline though.

Scott

sylvain.desbureaux at orange-ftgroup.com wrote:
> Add of a button destroy for disabled hosts.
> This button behave in a similar way than the delete button of a VM.
>
> Signed-off-by: Sylvain Desbureaux
> <sylvain.desbureaux at orange-ftgroup.com>
> ---
>  src/app/controllers/host_controller.rb |    5 +++++
>  src/app/services/host_service.rb       |   15 +++++++++++++++
>  src/app/views/host/show.rhtml          |   17 +++++++++++++++++
>  3 files changed, 37 insertions(+), 0 deletions(-)
>
> diff --git a/src/app/controllers/host_controller.rb
> b/src/app/controllers/host_controller.rb
> index 20e9fca..3ecb132 100644
> --- a/src/app/controllers/host_controller.rb
> +++ b/src/app/controllers/host_controller.rb
> @@ -80,6 +80,11 @@ class HostController < ApplicationController
>        :success => true
>      }
>    end
> +  
> +  def destroy
> +    alert = svc_destroy(params[:id])
> +    render :json => { :object => "host", :success => true, :alert =>
> alert  }
> +  end
>  
>    def enable
>      svc_enable(params[:id], "enabled")
> diff --git a/src/app/services/host_service.rb
> b/src/app/services/host_service.rb
> index 4ace9fb..2482f93 100644
> --- a/src/app/services/host_service.rb
> +++ b/src/app/services/host_service.rb
> @@ -109,6 +109,21 @@ module HostService
>        @host.save!
>      end
>    end
> +  
> +  # Destroys for the Host with +id+
> +  #
> +  # === Instance variables
> +  # [<tt>@host</tt>] stores the Host with +id+
> +  # === Required permissions
> +  # [<tt>Privilege::MODIFY</tt>] on host's HardwarePool
> +  def svc_destroy(id)
> +    lookup(id,Privilege::MODIFY)
> +    unless @host.disabled?
> +      raise ActionError.new("Host must be disabled to delete it")
> +    end
> +    @host.destroy
> +    return "Host was successfully deleted."
> +  end
>   
You'll also want to make sure that @host.vms returns nothing, as 
disabling a  host doesn't force all VMs to be migrated automatically.
>  
>    private
>    def lookup(id, priv)
> diff --git a/src/app/views/host/show.rhtml
> b/src/app/views/host/show.rhtml
> index ddc6481..fe34401 100644
> --- a/src/app/views/host/show.rhtml
> +++ b/src/app/views/host/show.rhtml
> @@ -7,6 +7,9 @@
>        <a href="#" onClick="host_action('enable')">
>          <%= image_tag "icon_start_11px.png" %> Enable Host
>        </a>
> +	  <a href="#confirm_delete" rel="facebox[.bolder]">
> +        <%= image_tag "icon_delete_11px.png" %> Delete Host
> +      </a>
>   
Also you should probably only show this link if the host is disabled and 
has no VMs.
>      <% else -%>
>        <a href="#" onClick="host_action('disable')">
>          <%= image_tag "icon_suspend_11px.png" %> Disable Host
> @@ -23,6 +26,7 @@
>                   :rel=>"facebox[.bolder]", :class=>"selection_facebox"
> %>
>    <%- end -%>
>  <%- end -%>
> +<%= confirmation_dialog("confirm_delete", "Are you sure?",
> "delete_host()") %>
>  <script type="text/javascript">
>    function host_action(action)
>    {
> @@ -39,6 +43,19 @@
>                  }
>                 }, 'json');
>    }
> +  function delete_host()
> +  {
> +    $(document).trigger('close.facebox');
> +    $.post('<%= url_for :controller => "host", :action => "destroy",
> :id => @host.id %>',
> +           {x: 1},
> +            function(data,status){
> +              $("#hosts_grid").flexReload();
> +              if (data.alert) {
> +                $.jGrowl(data.alert);
> +              }
> +	      empty_summary('hosts_selection', 'Host');
> +            }, 'json');
> +  }
>  </script>
>  
>      <div id="hosts_selection_id" style="display:none"><%= @host.id
> %></div>
>