Ovirt devel - Jun 2009 - One more fix

This resend includes the rebase, but fixes a problem I just noticed with
nil/empty checks.

Also includes a couple minor model changes to support better
error reporting.

This method was added so that a client could submit multiple
vms to the service layer in one call and get back appropriate
messages showing any failures without those failures also causing
the entire call to fail, or forcing the client to implement handling
(begin/rescue blocks) of these errors to continue through the
submitted vm list.

Signed-off-by: Jason Guiditta <jguiditt at redhat.com>
---
 src/app/models/vm.rb                      |    7 ++
 src/app/models/vm_task.rb                 |    4 +-
 src/app/services/service_module_helper.rb |   34 +++++++
 src/app/services/vm_service.rb            |   33 ++++++-
 src/test/fixtures/permissions.yml         |    6 +-
 src/test/fixtures/vms.yml                 |   13 +++
 src/test/unit/vm_service_test.rb          |  149 +++++++++++++++++++++++++++++
 src/test/unit/vm_task_test.rb             |   38 ++++++++
 src/test/unit/vm_test.rb                  |   23 ++++-
 9 files changed, 300 insertions(+), 7 deletions(-)
 create mode 100644 src/app/services/service_module_helper.rb
 create mode 100644 src/test/unit/vm_service_test.rb
 create mode 100644 src/test/unit/vm_task_test.rb

diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb
index f445219..df18c7e 100644
--- a/src/app/models/vm.rb
+++ b/src/app/models/vm.rb
@@ -280,6 +280,13 @@ class Vm < ActiveRecord::Base
     actions
   end
 
+  # Provide method to check if requested action exists, so caller can decide
+  # if they want to throw an error of some sort before continuing
+  # (ie in service api)
+  def valid_action?(action)
+    return VmTask::ACTIONS.include?(action) ? true : false
+  end
+
   # these resource checks are made at VM start/restore time
   # use pending here by default since this is used for queueing VM
   # creation/start operations
diff --git a/src/app/models/vm_task.rb b/src/app/models/vm_task.rb
index 762438f..649001d 100644
--- a/src/app/models/vm_task.rb
+++ b/src/app/models/vm_task.rb
@@ -146,10 +146,10 @@ class VmTask < Task
   end
 
   def self.action_privilege(action)
-    return ACTIONS[action][:privilege][0]
+    return ACTIONS[action].nil? ? nil : ACTIONS[action][:privilege][0]
   end
   def self.action_privilege_object(action, obj)
-    return obj.send(ACTIONS[action][:privilege][1])
+    return ACTIONS[action].nil? ? nil :
obj.send(ACTIONS[action][:privilege][1])
   end
   def self.action_label(action)
     return ACTIONS[action][:label]
diff --git a/src/app/services/service_module_helper.rb
b/src/app/services/service_module_helper.rb
new file mode 100644
index 0000000..dd04f99
--- /dev/null
+++ b/src/app/services/service_module_helper.rb
@@ -0,0 +1,34 @@
+#
+# Copyright (C) 2009 Red Hat, Inc.
+# Written by Jason Guiditta <jguiditt at redhat.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+# MA  02110-1301, USA.  A copy of the GNU General Public License is
+# also available at http://www.gnu.org/copyleft/gpl.html.
+
+
+# This module is used to bootstrap testing for the service api,
+# which depends on certain methods existing in the class which includes it.
+# Clearly this is not the case with unit tests on the modules themselves,
+# so any dependencies are set up here.
+module ServiceModuleHelper
+
+  def get_login_user
+    return @user
+  end
+
+  def set_login_user(user=nil)
+    @user = user
+  end
+end
\ No newline at end of file
diff --git a/src/app/services/vm_service.rb b/src/app/services/vm_service.rb
index 3d9777c..073b819 100644
--- a/src/app/services/vm_service.rb
+++ b/src/app/services/vm_service.rb
@@ -211,13 +211,42 @@ module VmService
   #   <tt>VmTask.action_privilege(@action)</tt>
   def svc_vm_action(id, vm_action, action_args)
     @vm = Vm.find(id)
+    unless @vm.valid_action?(vm_action)
+      raise ActionError.new("#{vm_action} is an invalid action.")
+    end
     authorized!(VmTask.action_privilege(vm_action),
                 VmTask.action_privilege_object(vm_action, at vm))
     @task = @vm.queue_action(@user, vm_action, action_args)
     unless @task
-      raise ActionError.new("#{vm_action} is an invalid action.")
+      raise ActionError.new("#{vm_action} cannot be performed on this
vm.")
+    end
+    return "#{@vm.description}: #{vm_action} was successfully
queued."
+  end
+
+  # Perform action +vm_action+ on vms identified by +vm_id+
+  #
+  # === Instance variables
+  # * <tt>@vms</tt> VMs identified by +vm_ids+
+  # === Required permissions
+  # permission is action-specific as determined by
+  # <tt>VmTask.action_privilege(@action)</tt>
+  # This method can be called to initiate an action on one or more vms
+  def svc_vm_actions(vm_ids, vm_action, action_args)
+    vm_ids = [vm_ids] unless vm_ids.is_a?(Array)
+    successful_vms = []
+    failed_vms = {}
+    vm_ids.each do |vm_id|
+      begin
+        successful_vms << svc_vm_action(vm_id, vm_action, action_args)
+      rescue Exception => ex
+        failed_vms[@vm.description] = ex.message
+      end
+    end
+    unless failed_vms.empty?
+      raise PartialSuccessError.new("Your request to #{vm_action}
encountered the following errors: ",
+                                    failed_vms, successful_vms)
     end
-    return "#{vm_action} was successfully queued."
+    return "#{vm_action} submitted."
   end
 
   #  Cancels queued tasks for for Vm with +id+
diff --git a/src/test/fixtures/permissions.yml
b/src/test/fixtures/permissions.yml
index 4895f3e..eed95fd 100644
--- a/src/test/fixtures/permissions.yml
+++ b/src/test/fixtures/permissions.yml
@@ -35,5 +35,9 @@ ovirtadmin_prodops_pool:
 ovirtadmin_corp_com_qa_pool:
   role:  monitor
   uid: ovirtadmin
-  pool: corp_com_qa
+  pool: corp_qa_vmpool
   parent_permission: ovirtadmin_root
+testuser_corp_com_qa_pool:
+  role:  user
+  uid: testuser
+  pool: corp_qa_vmpool
diff --git a/src/test/fixtures/vms.yml b/src/test/fixtures/vms.yml
index 69b1c2b..b2711b2 100644
--- a/src/test/fixtures/vms.yml
+++ b/src/test/fixtures/vms.yml
@@ -65,6 +65,19 @@ foobar_prod1_vm:
   boot_device: cdrom
   host: fedoraworkstation_foobar_com
   vm_resource_pool: corp_com_production_vmpool
+corp_com_qa_postgres_vm:
+  uuid: f6059569-a81f-4728-bb61-6f16b1c594e7
+  description: corp.com qa postgres vm
+  num_vcpus_allocated: 2
+  num_vcpus_used: 2
+  memory_allocated: 262144
+  memory_used: 131072
+  vnic_mac_addr: 00:16:3e:04:de:c8
+  state: running
+  needs_restart: 0
+  boot_device: hd
+  host: macworkstation_qa_corp_com
+  vm_resource_pool: corp_qa_vmpool
 foobar_prod2_vm:
   uuid: 24b9a994-d415-481d-ace8-1d810b601eb6
   description: foobar prod2
diff --git a/src/test/unit/vm_service_test.rb b/src/test/unit/vm_service_test.rb
new file mode 100644
index 0000000..7bd00ad
--- /dev/null
+++ b/src/test/unit/vm_service_test.rb
@@ -0,0 +1,149 @@
+#
+# Copyright (C) 2009 Red Hat, Inc.
+# Written by Jason Guiditta <jguiditt at redhat.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+# MA  02110-1301, USA.  A copy of the GNU General Public License is
+# also available at http://www.gnu.org/copyleft/gpl.html.
+
+
+require File.dirname(__FILE__) + '/../test_helper'
+
+class VmServiceTest < ActiveSupport::TestCase
+
+  include ServiceModuleHelper
+  include VmService
+  fixtures :vms
+
+  def setup
+    set_login_user('ovirtadmin')
+  end
+
+  # Ensure correct message is returned for a valid action requested
+  # by a user with the proper permissions
+  def test_svc_vm_action_valid_user
+    assert_equal("#{vms(:production_mysqld_vm).description}: shutdown_vm
was successfully queued.",
+      svc_vm_action(vms(:production_mysqld_vm).id, 'shutdown_vm', nil))
+  end
+
+  # Ensure that if a non-existant action is passed in, ActionError
+  # is thrown
+  def test_svc_vm_action_invalid_action
+    assert_raise ActionError do
+      svc_vm_action(vms(:production_httpd_vm).id, 'stop_vm', nil)
+    end
+  end
+
+  # Ensure that if a user with the wrong permissions is passed in,
+  # PermissionError is thrown
+  def test_svc_vm_action_invalid_user
+    set_login_user('fred')
+    assert_raise PermissionError do
+      svc_vm_action(vms(:production_mysqld_vm).id, 'shutdown_vm', nil)
+    end
+  end
+
+  # Ensure that if an invalid state change for a vm is requested,
+  # ActionError is thrown
+  def test_svc_vm_action_invalid_state_change
+    assert_raise ActionError do
+      svc_vm_action(vms(:production_httpd_vm).id, 'shutdown_vm', nil)
+    end
+  end
+
+  # If only one vm was passed into svc_vm_actions, and that action cannot
+  # be performed, ActionError should be returned
+  def test_failed_single_vm_returns_partial_success_error
+    assert_raise PartialSuccessError do
+      svc_vm_actions(vms(:production_httpd_vm).id, 'shutdown_vm', nil)
+    end
+  end
+
+  # If multiple vms were passed into svc_vm_actions, and one or more (but
+  # not all) actions cannot be performed, PartialSuccessError should be
returned
+  def test_failed_multiple_vms_return_partial_success
+    assert_raise PartialSuccessError do
+      svc_vm_actions([vms(:production_httpd_vm).id,
+                      vms(:production_mysqld_vm).id,
+                      vms(:production_ftpd_vm).id], 'shutdown_vm', nil)
+    end
+  end
+
+  # Ensure we receive the expected success message if all actions succeed
+  # (should be the same message if one or more, so we have one test for
+  # each of those cases)
+  def test_success_message_from_single_vm
+    assert_equal("shutdown_vm successful.",
+      svc_vm_actions(vms(:production_mysqld_vm).id, 'shutdown_vm',
nil))
+  end
+
+  # Ensure we receive the expected success message if all actions succeed
+  # (should be the same message if one or more, so we have one test for
+  # each of those cases)
+  def test_success_message_for_multiple_vms
+    assert_equal("shutdown_vm successful.",
+      svc_vm_actions([vms(:production_postgresql_vm).id,
+                      vms(:production_mysqld_vm).id,
+                      vms(:foobar_prod1_vm).id], 'shutdown_vm', nil))
+  end
+
+  # Ensure that if a non-existant action is passed in, PartialSuccessError
+  # is thrown
+  def test_svc_vm_actions_invalid_action
+    assert_raise PartialSuccessError do
+      svc_vm_actions(vms(:production_httpd_vm).id, 'stop_vm', nil)
+    end
+  end
+
+  # Ensure we receive the expected success message if all actions succeed
+  # (should be the same message if one or more, so we have one test for
+  # each of those cases)
+  def test_success_message_from_single_vm_with_less_privileged_user
+    set_login_user('testuser')
+    assert_equal("shutdown_vm successful.",
+      svc_vm_actions(vms(:corp_com_qa_postgres_vm).id, 'shutdown_vm',
nil))
+  end
+
+  # Ensure that if a user with the wrong permissions is passed in,
+  # PartialSuccessError is thrown.  This allows some vms to still pass
+  # while others with wrong perms fail.
+  def test_svc_vm_actions_invalid_user
+    set_login_user('testuser')
+    assert_raise PartialSuccessError do
+      svc_vm_actions([vms(:corp_com_qa_postgres_vm).id,
+                      vms(:production_mysqld_vm).id], 'shutdown_vm',
nil)
+    end
+  end
+
+  # Ensure that if a user with the wrong permissions is passed in,
+  # PartialSuccessError contains the message of success and permission error.
+  def test_error_for_svc_vm_actions_invalid_user
+    set_login_user('testuser')
+    actual_error = nil
+    expected_error = PartialSuccessError.new(
+                    "Your request to shutdown_vm encountered the following
errors: ",
+                    {vms(:production_mysqld_vm).description => "You
have insufficient privileges to perform action."},
+                    ["#{vms(:corp_com_qa_postgres_vm).description}:
shutdown_vm was successfully queued."])
+
+    begin
+      svc_vm_actions([vms(:corp_com_qa_postgres_vm).id,
+                      vms(:production_mysqld_vm).id], 'shutdown_vm',
nil)
+    rescue Exception => ex
+      actual_error = ex
+    end
+    assert_equal expected_error.message, actual_error.message
+    assert_equal expected_error.failures, actual_error.failures
+    assert_equal expected_error.successes, actual_error.successes
+  end
+end
diff --git a/src/test/unit/vm_task_test.rb b/src/test/unit/vm_task_test.rb
new file mode 100644
index 0000000..39be915
--- /dev/null
+++ b/src/test/unit/vm_task_test.rb
@@ -0,0 +1,38 @@
+#
+# Copyright (C) 2009 Red Hat, Inc.
+# Written by Jason Guiditta <jguiditt at redhat.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+# MA  02110-1301, USA.  A copy of the GNU General Public License is
+# also available at http://www.gnu.org/copyleft/gpl.html.
+
+
+require File.dirname(__FILE__) + '/../test_helper'
+
+class VmTaskTest < ActiveSupport::TestCase
+
+  fixtures :vms
+
+  # Ensure action_privilege method returns nil if it is passed an action
+  # that does not exist.
+  def test_action_privilege_with_bad_action
+    assert_equal(nil, VmTask.action_privilege('stop_vm'))
+  end
+
+  # Ensure action_privilege_object method returns nil if it is passed an action
+  # that does not exist.
+  def test_action_privilege_object_with_bad_action
+    assert_equal(nil, VmTask.action_privilege_object('stop_vm',
vms(:production_httpd_vm).id))
+  end
+end
diff --git a/src/test/unit/vm_test.rb b/src/test/unit/vm_test.rb
index 5e03715..a5d6b3d 100644
--- a/src/test/unit/vm_test.rb
+++ b/src/test/unit/vm_test.rb
@@ -1,6 +1,7 @@
 #
 # Copyright (C) 2008 Red Hat, Inc.
-# Written by Scott Seago <sseago at redhat.com>
+# Written by Scott Seago <sseago at redhat.com>,
+#            Jason Guiditta <jguiditt at redhat.com>
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -19,7 +20,7 @@
 
 require File.dirname(__FILE__) + '/../test_helper'
 
-class VmTest < Test::Unit::TestCase
+class VmTest < ActiveSupport::TestCase
   fixtures :vms
   fixtures :pools
 
@@ -173,6 +174,24 @@ class VmTest < Test::Unit::TestCase
     assert_equal 'stopped', vms(:production_httpd_vm).get_pending_state
   end
 
+  def test_get_action_list_with_no_user
+    empty_array = []
+    assert_equal(empty_array, vms(:production_httpd_vm).get_action_list)
+  end
+
+  def test_queue_action_returns_false_with_invalid_action
+    assert_equal(false,
vms(:production_httpd_vm).queue_action('ovirtadmin', 'stop_vm'))
+  end
+
+  def test_valid_action_with_invalid_param
+    assert_equal(false,
vms(:production_httpd_vm).valid_action?('stop_vm'))
+  end
+
+  # Ensure valid_action? returns true
+  def test_valid_action_with_valid_param
+    assert_equal(true,
vms(:production_httpd_vm).valid_action?('shutdown_vm'))
+  end
+
   def test_paginated_results
     assert_equal 5, Vm.paged_with_perms('ovirtadmin', Privilege::VIEW,
1, 'vms.id').size
   end
-- 
1.6.0.6

Signed-off-by: Jason Guiditta <jguiditt at redhat.com>
---
 src/app/controllers/cloud/cloud_controller.rb      |   20 +++++-
 src/app/controllers/cloud/instance_controller.rb   |   25 +++----
 src/app/views/cloud/instance/_show.rhtml           |    1 -
 src/app/views/cloud/instance/index.rhtml           |   30 +++++---
 src/app/views/layouts/cloud/_notification.rhtml    |   31 ++++++++
 src/public/stylesheets/cloud/layout.css            |   73 +++++++++++++++++++-
 .../functional/cloud/instance_controller_test.rb   |   65 +++++++++++++++++-
 7 files changed, 214 insertions(+), 31 deletions(-)
 create mode 100644 src/app/views/layouts/cloud/_notification.rhtml

diff --git a/src/app/controllers/cloud/cloud_controller.rb
b/src/app/controllers/cloud/cloud_controller.rb
index 154d15d..19c56b6 100644
--- a/src/app/controllers/cloud/cloud_controller.rb
+++ b/src/app/controllers/cloud/cloud_controller.rb
@@ -26,9 +26,27 @@ class Cloud::CloudController < ApplicationController
 
   protected
 
+  # Override the default in ApplicationController, cloud has its own
+  # template for rendering these errors
+  def html_error_page(title, msg)
+    flash[:error] = msg
+    redirect_to params
+  end
+
+  # Override the default in ApplicationController, cloud has its own
+  # way of handling these (hooked into VmService::svc_vm_actions
+  def handle_partial_success_error(error)
+    handle_error(:error => error, :status => :ok,
+                 :message => {:summary => error.message,
+                              :failures => error.failures,
+                              :successes => error.successes},
+                 :title => "Some actions failed")
+  end
 
   # NOTE: This probably will/should be moved to use set_perms in
-  # ApplicationService once that is ready to go.
+  # ApplicationService once that is ready to go. Only problem with that
+  # idea is that there is currently no before filter to make sure that
+  # gets called.
   def set_vars
     @user = get_login_user
   end
diff --git a/src/app/controllers/cloud/instance_controller.rb
b/src/app/controllers/cloud/instance_controller.rb
index a4d0fd8..130f30b 100644
--- a/src/app/controllers/cloud/instance_controller.rb
+++ b/src/app/controllers/cloud/instance_controller.rb
@@ -44,7 +44,7 @@ class Cloud::InstanceController < Cloud::CloudController
     task_order = find_in_params_or_default(:task_order, "tasks.id")
     @vm_details = Vm.find(ids) if ids
     if @vm_details
-      @tasks = VmTask.paginate(:conditions => ["task_target_id in
(:ids)",{:ids => ids}],
+      @tasks = VmTask.paginate(:include => :vm, :conditions =>
["task_target_id in (:ids)",{:ids => ids}],
                              :per_page => 5, :page => task_page, :order
=> task_order)
     end
   end
@@ -76,19 +76,18 @@ class Cloud::InstanceController < Cloud::CloudController
     return params[key] && params[key] != "" ? params[key] :
default
   end
 
-# This redirects the user to a get url if they are just trying to view details
for one or more
-# instances.
-# TODO: if the user is trying to submit an acton on selected instances, call
the service
-# layer and display the :flash (might still want to do :get redirect to keep
pagination/sorting
-# correct.
+# This implements the Post/Redirect/Get pattern, and redirects the user to
+# a get url so an action is not accidentally posted twice.
   def handle_form
-    case params[:submit_for_list]
-      when "Show Selected"
-        params.delete(:submit_for_list)
-        redirect_to :action => "index", :params => params
-        return
-#    Do this if we have submitted an action on one or more vms.
-#    svc_vm_action(params[:ids], params[:vm_action], params[:action_args])
+    if params[:submit_for_list]
+      submit_type = params[:submit_for_list]
+      params.delete(:submit_for_list)
+      if params[:ids].nil?
+        flash[:warning] = "You must select at least one instance to
perform an action."
+      elsif submit_type != "Show Selected"
+        flash[:notice] = svc_vm_actions(params[:ids], submit_type.downcase
<< '_vm', params[:action_args])
+      end
+      redirect_to params
     end
   end
 
diff --git a/src/app/views/cloud/instance/_show.rhtml
b/src/app/views/cloud/instance/_show.rhtml
index 001286e..186c4ba 100644
--- a/src/app/views/cloud/instance/_show.rhtml
+++ b/src/app/views/cloud/instance/_show.rhtml
@@ -1,7 +1,6 @@
   <div>
     <div id="detail_header">
       <%= submit_tag 'Show Selected', :id =>
'submit_for_list', :name => 'submit_for_list' %>
-      </form>
       <% if @vm_details %>
         <h3>
         <% if @vm_details.size == 1 %>
diff --git a/src/app/views/cloud/instance/index.rhtml
b/src/app/views/cloud/instance/index.rhtml
index 757d67c..a787e39 100644
--- a/src/app/views/cloud/instance/index.rhtml
+++ b/src/app/views/cloud/instance/index.rhtml
@@ -1,23 +1,29 @@
+<form action="<%= url_for({:action =>
'index'})%>" method="post">
 <div id="toolbar">
-  <!-- TODO: Make each li a submit button with same styling as current li.
-       Handlng of this will be implemented in InstanceController::handle_form
-  -->
   <ul>
     <li>New Instance</li>
     <li>
       Actions
-      <ul>
-        <% @actions.each {|action| %>
-          <li><%= image_tag action[2]%><%= action[0]
%></li>
-        <% } %>
-      </ul>
+        <%=%>
+        <ul>
+          <% @actions.each {|action| %>
+
+            <li>
+              <%= image_tag action[2]%>
+              <%= submit_tag action[0], :name =>
'submit_for_list', :class => 'button_as_link' %>
+            </li>
+          <% } %>
+        </ul>
     </li>
   </ul>
 </div>
-<form  action="<%= url_for({:action =>
'index'})%>" method="post"> <%# This form tag is
terminated in _show.rhtml %>
   <div id="list-view">
-      <%= render :partial => 'list' %>
+    <% if !flash.empty? %>
+      <%= render :partial => '/layouts/cloud/notification' %>
+    <% end %>
+    <%= render :partial => 'list' %>
   </div>
   <div id="detail-view">
-      <%= render :partial => 'show' %>
-  </div>
\ No newline at end of file
+    <%= render :partial => 'show' %>
+  </div>
+</form>
\ No newline at end of file
diff --git a/src/app/views/layouts/cloud/_notification.rhtml
b/src/app/views/layouts/cloud/_notification.rhtml
new file mode 100644
index 0000000..d90d827
--- /dev/null
+++ b/src/app/views/layouts/cloud/_notification.rhtml
@@ -0,0 +1,31 @@
+<div id="notification" <%if
flash[:error]%>class="error-align"<%end%>">
+  <%= link_to "", params, :id => "close-notify" %>
+  <% if flash[:error] %>
+      <% if !nil_or_empty(flash[:error][:successes]) %>
+      <div class="success">
+        <h4>The following items were successful:</h4>
+        <ul>
+        <% flash[:error][:successes].each {|item| %>
+          <li><%= item %></li>
+        <% } %>
+        </ul>
+      </div>
+    <% end %>
+    <% if !nil_or_empty(flash[:error][:failures]) %>
+      <div class="warning"><h4><%=
flash[:error][:summary]%></h4></div>
+      <div class="error">
+        <ul>
+          <% flash[:error][:failures].each {|key, value| %>
+            <li><%= key %>: <%= value %></li>
+          <% } %>
+        </ul>
+      </div>
+    <% end %>
+  <% end %>
+  <% if flash[:warning] %>
+    <div class="warning"><h4><%= flash[:warning]
%></h4></div>
+  <% end %>
+  <% if flash[:notice] %>
+    <div class="success"><ul><li><%=
flash[:notice] %></li></ul></div>
+  <% end %>
+</div>
\ No newline at end of file
diff --git a/src/public/stylesheets/cloud/layout.css
b/src/public/stylesheets/cloud/layout.css
index 0fd158d..1d1fa07 100644
--- a/src/public/stylesheets/cloud/layout.css
+++ b/src/public/stylesheets/cloud/layout.css
@@ -28,6 +28,14 @@ html {min-width:640px;}
   color: #337ACC;
 }
 
+/* Make buttons look like links */
+.button_as_link {
+  background: none;
+  color: #666666;
+  border: none;
+  cursor: pointer;
+}
+
 #navcontainer {
 	height:40px;
 }
@@ -205,8 +213,10 @@ input#submit_for_list {
 
 #toolbar li:hover {
   background: #337ACC;
+}
+
+#toolbar li:hover > .button_as_link {
   color: #FFFFFF;
-  cursor: pointer;
 }
 
 #toolbar li.current { /* This is not really needed right now, but will be
useful when we add filters. */
@@ -234,3 +244,64 @@ input#submit_for_list {
 }
 
 #toolbar li:hover ul {display: block;}
+
+/****** Notification styles ******/
+#notification {
+  -webkit-border-radius: 0.2em;
+  -moz-border-radius: 0.2em;
+  position: absolute;
+  z-index: 1;
+  top: 2em;
+  left: 35%;
+  margin: 0 auto;
+  padding: 1.5em;
+  background: #337ACC;
+  border: solid 0.2em;
+}
+
+.error-align {left: 25% !important;}
+
+#notification > div {
+  margin-bottom: 1em;
+}
+
+#notification h4 {
+  text-indent: 3em;
+}
+
+#close-notify {
+  position: absolute;
+  right: 0.2em;
+  top: 0.2em;
+  width:11px;
+  height:11px;
+  background: url(/images/closebox-11px.png) no-repeat;
+}
+
+.success {
+  width: 100%;
+  background:url(/images/icon_conf_success.png) left center no-repeat #CCFFCC;
+  border: solid 0.2em green;
+  -webkit-border-radius: 0.2em;
+  -moz-border-radius: 0.2em;
+}
+
+.success li, .error li {
+  margin-left:3em;
+}
+
+.error {
+  width: 100%;
+  background:url(/images/icon_conf_error.png) left center no-repeat #EC9999;
+  border: solid 0.2em red;
+  -webkit-border-radius: 0.2em;
+  -moz-border-radius: 0.2em;
+}
+
+.warning {
+  width: 100%;
+  background:url(/images/icon_conf_warning.png) left center no-repeat #FFFFCC;
+  border: solid 0.2em yellow;
+  -webkit-border-radius: 0.2em;
+  -moz-border-radius: 0.2em;
+}
diff --git a/src/test/functional/cloud/instance_controller_test.rb
b/src/test/functional/cloud/instance_controller_test.rb
index 213d2e0..71d132b 100644
--- a/src/test/functional/cloud/instance_controller_test.rb
+++ b/src/test/functional/cloud/instance_controller_test.rb
@@ -1,6 +1,27 @@
-require 'test_helper'
+#
+# Copyright (C) 2008 Red Hat, Inc.
+# Written by Jason Guiditta <jguiditt at redhat.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+# MA  02110-1301, USA.  A copy of the GNU General Public License is
+# also available at http://www.gnu.org/copyleft/gpl.html.
+
+require File.dirname(__FILE__) + '/../../test_helper'
 
 class Cloud::InstanceControllerTest < ActionController::TestCase
+
+  include ServiceModuleHelper
   fixtures :vms, :tasks
   def setup
     @controller = Cloud::InstanceController.new
@@ -14,10 +35,48 @@ class Cloud::InstanceControllerTest <
ActionController::TestCase
     assert_template 'index'
     assert_not_nil assigns(:vms)
     assert_not_nil assigns(:user)
+    assert_equal({}, flash, "flash object is not empty!")
+    assert_equal(true, flash.empty?)
+    assert_select "#notification", false, "This page must
contain no notification block"
   end
 
-  def test_should_redirect_if_request_for_selected
+  def test_should_redirect_if_form_submitted
     post(:index,{:submit_for_list => 'Show Selected'})
-    assert_response :redirect
+    assert_redirected_to :action => :index
+    #TODO: since this is a redirect, it returns a 302 and nil template,
+    # would be good to figure out how to get the actual result returned to
+    # browser so we can do an assert_select
+  end
+
+  def test_add_valid_task
+    post(:index,{:submit_for_list => 'Shutdown', :ids =>
[vms(:production_mysqld_vm).id]})
+    assert_equal('shutdown_vm successful.', flash[:notice])
+    assert_redirected_to :action => :index, :ids =>
vms(:production_mysqld_vm).id
+  end
+
+  def test_add_invalid_task
+    post(:index,{:submit_for_list => 'Shutdown',
+                 :ids =>
[vms(:production_mysqld_vm).id,vms(:production_httpd_vm).id]})
+    expected_failures = {:summary => "Your request to shutdown_vm
encountered the following errors: ",
+                         :failures => {vms(:production_httpd_vm).description
=> "shutdown_vm cannot be performed on this vm."},
+                         :successes =>
["#{vms(:production_mysqld_vm).description}: shutdown_vm was successfully
queued."]}
+    assert_equal(expected_failures, flash[:error])
+    assert_redirected_to :action => :index, :ids =>
[vms(:production_mysqld_vm).id,vms(:production_httpd_vm).id]
+  end
+
+  def test_add_task_no_perms_for_some_vms
+    post(:index,{:submit_for_list => 'Shutdown',
+                 :ids =>
[vms(:production_mysqld_vm).id,vms(:corp_com_qa_postgres_vm).id]})
+    expected_failures = {:summary => "Your request to shutdown_vm
encountered the following errors: ",
+                         :failures =>
{vms(:corp_com_qa_postgres_vm).description => "You have insufficient
privileges to perform action."},
+                         :successes =>
["#{vms(:production_mysqld_vm).description}: shutdown_vm was successfully
queued."]}
+    assert_equal(expected_failures, flash[:error])
+    assert_redirected_to :action => :index, :ids =>
[vms(:production_mysqld_vm).id,vms(:corp_com_qa_postgres_vm).id]
+  end
+
+  # Make sure we get warning back if no instance was chosen to perform an
action on.
+  def test_no_instance_chosen
+    post(:index,{:submit_for_list => 'Shutdown'})
+    assert_equal("You must select at least one instance to perform an
action.", flash[:warning])
   end
 end
-- 
1.6.0.6