[Ovirt-devel] [PATCH] Add task list to dashboard
Scott Seago
sseago at redhat.com
Sat Jan 10 02:00:05 UTC 2009
David Lutterkort wrote:
> 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' ?
>
It's still 'dashboard' since the fact that tasks is the only tab is
temporary. Dashboard will eventually have alerts, user preferences, and
other stuff -- but the rest of it isn't ready yet. Although I'd be fine
w/ the tab saying 'Tasks' as long as the left nav still said 'Dashboard'
> * 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
>
Good point. I'll remove it -- it makes sense for VMs, hosts, etc but not
here.
> * 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
>
The icon would be useful, yes, but we may still want the column so we
can sort and filter by it. Anyway perhaps it's a point to work through
w/ Jeremy.
> 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')
>
Easy enough to do.
> 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
>
Hmm. Yeah it's empty for the basic VM actions, but migrate, etc can use
it. Joining it w/ action might be just the right thing though. So for
"start_vm" we still have just that. But for migrate_vm w/ the arg being
the host w/ ID 5 it would say "migrate_vm 5".
> 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
>
Yeah redoing the filter setup is another task on the sprint list which
will involve reworking filter display and adding date ranges. Perhaps,
time permitting, we could look into filtering by groups as well,
depending on how much work it is.
> 5. Conceptually, do users really care about the distinction
> between 'Storage' and 'Storage Volume' tasks ? Why not
> just lump them all into 'Storage tasks' ?
>
>
Well the task type distinction is more for the benefit of taskomatic
than for users -- the task type determines the behavior of the task.
Different object pointed to, different ruby class implementation, etc.
If there's a need for a more human-friendly categorization of tasks, we
could consider adding it, but we shouldn't mix that up w/ 'type' which
has model and class implications. In addition, a category-like 'type'
field would allow us to distinguish different types of VM tasks.
Note that I won't hold up pushing this patch on the above 5 points, as
they have more to do with the overall UI design of the existing task
list rather than the dashboard refactoring, and in some cases we have UI
design issues to work through before resolving them anyway.
> 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.
>
>
I tried to pull all of the generic stuff (that applies to more than one
of the 3 controllers that have task lists) into the task_actions module
which is included by the controllers that have task lists. I suppose we
could have a separate controller with 3 different actions relating to
dashboard, vm, and hw pool, but it doesn't really match the model we're
already using for VM pool and HW pool controllers -- where one
controller handles the various UI tabs for an object type. So the bits
common to all task lists are already pulled out into task_actions -- and
on the view side in task/_show and task/_grid. Some of the roundabout
nature here has to do with the way the flexigrid setup works -- we need
one action to generate the page itself -- with navigation, filters,
etc., and another action to return the json array for the table content.
The index vs. show_tasks confusion will go away once we move the task
list off the index action -- i.e. the dashboard index will be a
portal-like page and the task list will be a separate tab called
show_tasks just like elsewhere.
>> +
>> + 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.
>
>
Yeah -- mostly I was pulling the show action from the other controllers
-- in fact we're not really even using show directly -- it's mostly an
artifact of the way that show_tasks works for the other instances ,and I
wanted to reuse as much of this as possible.
In general the 'index' action is used a lot for the API, but none of the
web UI uses index in the current iteration (except dashboard)
>> --- 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
>
>
Oops. Looks like I didn't follow through with enough testing at the end.
I ran into a lot of refactoring bugs that broke either the dashboard
view or the HW pool view and when I finally came up with something that
worked for both I must have forgotten to re-test the VM pool UI again.
OK so of the above issues, the ones that I will rework into an updated
patch are as follows:
1) dashboard tab name changed to 'Tasks' (left nav is still 'dashboard')
2) fix the 'no tasks' view to remove the 'add' icon
3) Fix whatever's wrong with the VM pool template/controller in the
current patch.
Let me know if there's anything else in the above comments that ought to
be handled before pushing. And, of course, for some of the above the
discussion is ongoing :-)
Scott
More information about the ovirt-devel
mailing list