[Ovirt-devel] [PATCH] Add task list to dashboard

[David Lutterkort]

On Thu, 2009-01-08 at 18:43 +0000, Scott Seago wrote:
> Refactored task list functionality to allow for the dashboard-level task list.
> The dashboard task list shows all tasks initiated by the logged-in user, and is
> filterable by task type and state.

This causes trouble for me when looking at the task list for a VM pool
(see below)

Some minor nits on the presentation:

      * When I look at the dashboard now, I have a single tab that says
        'Dashboard' - shouldn't that say 'Tasks' ?
      * When there are no tasks to display, I get that empty splash
        screen with a '+' in it, as if I could add a task - that
        shouldn't be there
      * Some general comments on how task lists are displayed (nothing
        to do with this patch in particular):
             1. Could we use an icon to indicate state ? Using a whole
                column is kinda bulky, and not as informative as a nice
                icon
             2. The column 'Message' is only visible after scrolling,
                even though it's pretty important - that should be moved
                to a more prominent position (like right next to
                'Action')
             3. The column 'Args' is almost always empty - why not just
                drop it from the default view, or roll the info from
                that into another column, e.g. Action
             4. It would be nice if you could combine several of the
                Type resp. State filters (like look at all Storage and
                Storage Volume tasks) - and as somebody else mentioned,
                a clearer indication of the currently active filters
             5. Conceptually, do users really care about the distinction
                between 'Storage' and 'Storage Volume' tasks ? Why not
                just lump them all into 'Storage tasks' ?

Some more comments; most of them are more todo type items, though it
might be good to address as many of htem aspossible right away.

> Signed-off-by: Scott Seago <sseago at redhat.com>
> ---
>  src/app/controllers/dashboard_controller.rb        |   41 +++++++---
>  src/app/controllers/hardware_controller.rb         |   12 +---
>  src/app/controllers/pool_controller.rb             |   41 ++--------
>  src/app/controllers/task_actions.rb                |   50 ++++++++++++
>  src/app/models/task.rb                             |   13 +++
>  src/app/views/dashboard/index.html.erb             |   38 ++-------
>  src/app/views/hardware/show_tasks.rhtml            |   84 ++------------------
>  src/app/views/layouts/_navigation_tabs.rhtml       |   20 ++++-
>  src/app/views/resources/show_tasks.rhtml           |   68 ++--------------
>  src/app/views/task/_grid.rhtml                     |    2 +-
>  .../show_tasks.rhtml => task/_show.rhtml}          |   57 +++++++------
>  11 files changed, 169 insertions(+), 257 deletions(-)
>  create mode 100644 src/app/controllers/task_actions.rb
>  copy src/app/views/{hardware/show_tasks.rhtml => task/_show.rhtml} (59%)
> 
> diff --git a/src/app/controllers/dashboard_controller.rb b/src/app/controllers/dashboard_controller.rb
> index c4830df..00398a5 100644
> --- a/src/app/controllers/dashboard_controller.rb
> +++ b/src/app/controllers/dashboard_controller.rb
> @@ -18,19 +18,36 @@
>  # also available at http://www.gnu.org/copyleft/gpl.html.
>  
>  class DashboardController < ApplicationController

Not directly a comment about this patch: it would be cleaner if we had a
TaskController with an index action that accepts parameters to restrict
the list of tasks to a certain object, e.g. a hw pool or a vm pool.

The implementation with TaskActions and the index -> show_tasks -> show
call sequence seems a little roundabout.

> +
> +  include TaskActions
> +  def tasks_query_obj
> +    Task
> +  end
> +  def tasks_conditions
> +    {:user => get_login_user}
> +  end
> +
>    def index
> -    @default_pool = HardwarePool.get_default_pool
> -    set_perms(@default_pool)
> -    #remove these soon
> -    @hardware_pools = HardwarePool.find(:all)
> -    @available_hosts = Host.find(:all)
> -    @available_storage_volumes = StorageVolume.find(:all)
> -    @storage_pools = StoragePool.find(:all)
> -    @hosts = Host.find(:all)
> -    @storage_volumes = StorageVolume.find(:all)
> -    @vms = Vm.find(:all)
> -    if params[:ajax]
> -      render :layout => 'tabs-and-content' #:template => 'hardware/show.html.erb'
> +    @task_types = Task::TASK_TYPES_OPTIONS
> +    show_tasks
> +  end
> +
> +  def show
> +    respond_to do |format|
> +      format.html {
> +        render :layout => 'tabs-and-content' if params[:ajax]
> +        render :layout => 'help-and-content' if params[:nolayout]
> +      }
> +      format.xml {
> +        render :xml => @pool.to_xml(XML_OPTS)
> +      }
>      end
>    end

Thanks for thinking of the API ;) It seems though that this is missing a
route to really work - and for retrieving lists of items, the index
action should be used, not show.

> --- a/src/app/views/hardware/show_tasks.rhtml
> +++ b/src/app/views/task/_show.rhtml
> @@ -1,33 +1,36 @@
>  <div id="toolbar_nav">
> - <ul>
> -    <li>
> -       <%= image_tag "icon_move.png", :style => "vertical-align:middle;" %>  Type    <%= image_tag "icon_toolbar_arrow.gif", :style => "vertical-align:middle;" %>
> -       <ul>
> -        <% @task_types.each_index { |index| %>
> -            <li onclick="apply_task_filter('<%=@task_types[index][1]%>','<%=@task_state%>');";"
> -            <% if (index == @task_types.length - 1) or @task_types[index].length == 3 %>
> -                style="border-bottom: 1px solid #CCCCCC;"
> -            <% end %>
> -               >
> -<!--                 < % = image_tag ... -->
> -                 <%=  @task_type == @task_types[index][1] ? "X" : "  " %>
> -                 <%=@task_types[index][0]%>
> -            </li>
> -        <% } %>
> -       </ul>
> -    </li>
> +  <ul>
> +    <%if defined? task_types %>
> +      <li>
> +         <%= image_tag "icon_move.png", :style => "vertical-align:middle;" %>  Type    <%= image_tag "icon_toolbar_arrow.gif", :style => "vertical-align:middle;" %>
> +         <ul>
> +          <% task_types.each_index { |index| %>

When I look at the task list for a VM pool, this gives me

        ActionView::TemplateError (You have a nil object when you didn't expect it!
        You might have expected an instance of Array.
        The error occurred while evaluating nil.each_index) on line #7 of task/_show.rhtml:
        4:       <li>
        5:          <%= image_tag "icon_move.png", :style => "vertical-align:middle;" %>  Type    <%= image_tag "icon_toolbar_arrow.gif", :style => "vertical-align:middle;" %>
        6:          <ul>
        7:           <% @task_types.each_index { |index| %>
        8:               <li onclick="apply_task_filter('<%=task_types[index][1]%>','<%=task_state%>');";"
        9:               <% if (index == task_types.length - 1) or task_types[index].length == 3 %>
        10:                   style="border-bottom: 1px solid #CCCCCC;"

David