Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI improvements #187

Merged
merged 1 commit into from
Nov 27, 2020
Merged

UI improvements #187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions app/controllers/maintenance_tasks/tasks_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,41 +1,42 @@
# frozen_string_literal: true

module MaintenanceTasks
# Class handles rendering the maintenance_tasks page in the host application.
# It makes data about available, enqueued, performing, and completed
# tasks accessible to the views so it can be displayed in the UI.
#
# @api private
class TasksController < ApplicationController
before_action :set_refresh, only: [:index, :show]

# Renders the maintenance_tasks/tasks page, displaying
# available tasks to users.
def index
@tasks = Task.available_tasks
@pagy, @active_runs = pagy(Run.active.order(created_at: :desc))
set_refresh if @active_runs.present?
@latest_completed_runs = Run.latest_completed
volmer marked this conversation as resolved.
Show resolved Hide resolved
end

# Renders the page responsible for providing Task actions to users.
# Shows running and completed instances of the Task.
def show
@task = Task.named(params.fetch(:id))
@pagy, @runs = pagy(@task.runs.order(created_at: :desc))
@active_run = @task.active_run
set_refresh if @active_run
@last_run = @task.last_run
@pagy, @previous_runs = pagy(
@task.runs.where.not(id: @last_run&.id).order(created_at: :desc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this a scope on Run?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, the way the query is right now you need the last run as an argument.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could just use a lambda, no?

# tasks_controller.rb
pagy(@task.runs.all_but_last(@last_run&.id))

# run.rb
scope :all_but_last, -> (last_run_id) { where.not(id: last_run_id).order(created_at: :desc) }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't explain my main point. My worry is that it is weird for Run to define a scope that is purely focused on a UI decision of showing the last Run separately from the others.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a needless query that doesn't make much sense (since id can't be NULL, the second condition is always true) when there's no run:

SELECT COUNT(*) FROM "maintenance_tasks_runs"
WHERE "maintenance_tasks_runs"."task_name" = ?
AND "maintenance_tasks_runs"."id" IS NOT NULL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you suggest instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return early if there's no last_run`? Or wrap the query and pagy calls.

)
end

# Runs a given Task and redirects to the Task page.
def run
task = Runner.new.run(name: params.fetch(:id))
redirect_to(task_path(task), notice: "Task #{task.name} enqueued.")
redirect_to(task_path(task))
rescue ActiveRecord::RecordInvalid => error
redirect_to(task_path(error.record.task_name), alert: error.message)
end

private

def set_refresh
@refresh = 5
@refresh = 3
end
end
private_constant :TasksController
Expand Down
14 changes: 9 additions & 5 deletions app/helpers/maintenance_tasks/application_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,18 @@ def pagination(pagy)
raw(pagy_bulma_nav(pagy)) if pagy.pages > 1
end

# Renders a time element with the localized version of the datetime.
# Renders a time element with the given datetime, worded as relative to the
# current time.
#
# The ISO 8601 version of the datetime is shown on hover
# via a title attribute.
#
# @param datetime [ActiveSupport::TimeWithZone] the time to be formatted.
# @return [String] the HTML to render with the formatted datetime.
def formatted_datetime(datetime)
time_tag(datetime, title: datetime.utc.iso8601)
# @param datetime [ActiveSupport::TimeWithZone] the time to be presented.
# @return [String] the HTML to render with the relative datetime in words.
def time_ago(datetime)
time_tag(datetime, title: datetime.utc.iso8601, class: 'is-clickable') do
time_ago_in_words(datetime) + ' ago'
end
end
end
private_constant :ApplicationHelper
Expand Down
78 changes: 46 additions & 32 deletions app/helpers/maintenance_tasks/task_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,18 @@ module MaintenanceTasks
#
# @api private
module TaskHelper
STATUS_COLOURS = {
'enqueued' => ['is-primary is-light'],
'running' => ['is-info'],
'interrupted' => ['is-info', 'is-light'],
'pausing' => ['is-warning', 'is-light'],
'paused' => ['is-warning'],
'succeeded' => ['is-success'],
'cancelling' => ['is-light'],
'cancelled' => ['is-dark'],
'errored' => ['is-danger'],
}

# Formats a run backtrace.
#
# @param backtrace [Array<String>] the backtrace associated with an
Expand All @@ -14,26 +26,38 @@ def format_backtrace(backtrace)
safe_join(backtrace.to_a, tag.br)
end

# Formats the ticks.
# Renders the progress bar.
#
# The style of the progress tag depends on the Run status. It also renders
# an infinite progress when a Run is active but there is no total
# information to estimate completion.
#
# Only shows the ticks or if the total is available, shows the ticks,
# total and percentage. Does not show ticks if run has not started.
# @param run [Run] the Run which the progress bar will be based on.
#
# @param run [Run] the run for which the ticks are formatted.
# @return [String, nil] the progress information properly formatted, or
# nil if the run has not started yet.
def format_ticks(run)
# @return [String] the progress information properly formatted.
# @return [nil] if the run has not started yet.
def progress(run)
return unless run.started?

value = run.tick_count
max = run.tick_total
title = "Processed #{pluralize(run.tick_count, 'item')}."

if run.tick_total.to_i > 0
safe_join([
tag.progress(value: run.tick_count, max: run.tick_total,
class: 'progress is-small'),
progress_text(run),
], ' ')
percentage = 100.0 * run.tick_count / run.tick_total
title = "Processed #{run.tick_count} out of #{run.tick_total} "\
"(#{number_to_percentage(percentage.floor, precision: 0)})"
else
run.tick_count.to_s
max = run.tick_count
value = nil unless run.completed? || run.paused?
end

tag.progress(
value: value,
max: max,
title: title,
class: ['progress'] + STATUS_COLOURS.fetch(run.status)
)
end

# Renders a span with a Run's status, with the corresponding tag class
Expand All @@ -43,19 +67,7 @@ def format_ticks(run)
# @return [String] the span element containing the status, with the
# appropriate tag class attached.
def status_tag(status)
tag_labels = {
'enqueued' => 'primary',
'running' => 'info',
'interrupted' => 'info is-light',
'pausing' => 'warning is-light',
'paused' => 'warning',
'succeeded' => 'success',
'cancelling' => 'light',
'cancelled' => 'dark',
'errored' => 'danger',
}

tag.span(status, class: "tag is-#{tag_labels.fetch(status)}")
tag.span(status.capitalize, class: ['tag'] + STATUS_COLOURS.fetch(status))
end

# Returns the distance between now and the Run's expected completion time,
Expand All @@ -72,12 +84,14 @@ def estimated_time_to_completion(run)
end
end

private

def progress_text(run)
percentage = 100.0 * run.tick_count / run.tick_total
"#{run.tick_count} / #{run.tick_total} "\
"(#{number_to_percentage(percentage.floor, precision: 0)})"
# Reports the approximate elapsed time a Run has been processed so far based
# on the Run's time running attribute.
#
# @param run [Run] the source of the time to be reported.
#
# @return [String] the description of the time running attribute.
def time_running_in_words(run)
distance_of_time_in_words(0, run.time_running, include_seconds: true)
end
end
private_constant :TaskHelper
Expand Down
5 changes: 0 additions & 5 deletions app/models/maintenance_tasks/run.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,6 @@ class Run < ApplicationRecord
serialize :backtrace

scope :active, -> { where(status: ACTIVE_STATUSES) }
scope :latest_completed, -> {
where(status: COMPLETED_STATUSES)
.order(created_at: :desc)
.limit(COMPLETED_RUNS_LIMIT)
}

validates_with RunStatusValidator, on: :update

Expand Down
7 changes: 4 additions & 3 deletions app/tasks/maintenance_tasks/task.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,12 @@ def runs
Run.where(task_name: name)
end

# Returns the active Run associated with the Task, if any.
# Retrieves the latest Run associated with the Task.
#
# @return [MaintenanceTasks::Run] the Run record.
def active_run
runs.active.first
# @return [nil] if there are no Runs associated with the Task.
def last_run
runs.last
end

private
Expand Down
9 changes: 9 additions & 0 deletions app/views/layouts/maintenance_tasks/application.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
<!DOCTYPE html>
<html lang="<%= I18n.locale %>">
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1">

<title>
<% if content_for?(:page_title) %>
<%= content_for :page_title %> -
Expand Down Expand Up @@ -29,6 +32,12 @@

<section class="section">
<div class="container">
<% if notice %>
<div class="notification is-success"><%= notice %></div>
<% elsif alert %>
<div class="notification is-warning"><%= alert %></div>
<% end %>

<%= yield %>
</div>
</div>
Expand Down
5 changes: 5 additions & 0 deletions app/views/maintenance_tasks/runs/_info.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<%= progress run %>

<div class="content">
<%= render "maintenance_tasks/runs/info/#{run.status}", run: run %>
</div>
8 changes: 8 additions & 0 deletions app/views/maintenance_tasks/runs/_run.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<div class="box">
<h5 class="title is-5">
<%= time_tag run.created_at, title: run.created_at %>
<%= status_tag run.status %>
</h5>

<%= render 'maintenance_tasks/runs/info', run: run %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we could inline the partials, it would not be too long and be less jumping around and fewer 1-line files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree if those partials were used only in here, but they must be available in other places as well. I think having smaller partials that work as reusable view components are easier to maintain, which compensates for the indirection.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that applies to inlining inside the runs/info partial, the status partials are only called from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The info partial calls the status-specific partial with interpolation for their path. Are you suggesting me to inline all of them over there? Please feel free to simplify views later as you see fit, for now I'm not really following.

</div>
4 changes: 4 additions & 0 deletions app/views/maintenance_tasks/runs/info/_cancelled.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<p>
Ran for <%= time_running_in_words run %>,
cancelled <%= time_ago run.ended_at %>.
</p>
1 change: 1 addition & 0 deletions app/views/maintenance_tasks/runs/info/_cancelling.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<p>Cancelling…</p>
1 change: 1 addition & 0 deletions app/views/maintenance_tasks/runs/info/_enqueued.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<p>Waiting to start.</p>
23 changes: 23 additions & 0 deletions app/views/maintenance_tasks/runs/info/_errored.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<p>
Ran for <%= time_running_in_words run %> until an error happened
<%= time_ago run.ended_at %>.
</p>

</p>

<div class="card">
<header class="card-header">
<p class="card-header-title">
<%= run.error_class %>
</p>
</header>

<div class="card-content">
<div class="content">
<p><%= run.error_message %></p>

<code><%= format_backtrace(run.backtrace) %></code>

</div>
</div>
</div>
1 change: 1 addition & 0 deletions app/views/maintenance_tasks/runs/info/_interrupted.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<p>Interrupted momentarily, resuming shortly...</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most importantly, the ... into … 😄 then maybe:

Suggested change
<p>Interrupted momentarily, resuming shortly...</p>
<p>Interrupted momentarily, a job has been enqueued and will resume processing the task shortly…</p>

Sorry I can't add a comment to the other thread, but I disagree with not talking about jobs and workers. We should probably mention it in the README as well, but I feel like when users have installed and written a task nothing could lead them to know that the task may be interrupted, so it would really be confusing. Especially if there's some queuing going on and they don't know why they're looking at that sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather not show "interrupted" as a status in that case. We are striving towards abstracting jobs away.

8 changes: 8 additions & 0 deletions app/views/maintenance_tasks/runs/info/_paused.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<p>
Ran for <%= time_running_in_words run %> until paused,
<% if run.estimated_completion_time %>
<%= estimated_time_to_completion(run) %> remaining.
<% else %>
processed <%= pluralize run.tick_count, 'item' %> so far.
<% end %>
</p>
1 change: 1 addition & 0 deletions app/views/maintenance_tasks/runs/info/_pausing.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<p>Pausing, please hold...</p>
volmer marked this conversation as resolved.
Show resolved Hide resolved
7 changes: 7 additions & 0 deletions app/views/maintenance_tasks/runs/info/_running.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<p>
<% if run.estimated_completion_time %>
<%= estimated_time_to_completion(run).capitalize %> remaining.
<% else %>
Processed <%= pluralize run.tick_count, 'item' %> so far.
<% end %>
</p>
5 changes: 5 additions & 0 deletions app/views/maintenance_tasks/runs/info/_succeeded.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<p>
Ran for <%= time_running_in_words run %>,
finished <%= time_ago run.ended_at %>.
</p>

14 changes: 14 additions & 0 deletions app/views/maintenance_tasks/tasks/_task.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<div class="box">
<h3 class="title is-3">
<%= link_to task, task_path(task) %>

<% if run = task.last_run %>
Copy link
Member

@etiennebarrie etiennebarrie Nov 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a n+1.
Also it's ignoring runs of tasks that have been deleted. Right now we're able to show them, even if the tasks#show page is broken.

I can tackle this later, but I feel like loading all the active Runs, and showing them first would be valuable in any case (in this PR if you have a few tasks, you may not see one running that's below the fold).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used in the index page. This refresh intends to display Task information in that page, not Runs. We can discuss how we could show orphan Runs later on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The index page lists all the tasks, showing their last run status here in the title, and their last run info below in the card. So it does display Run information, I'm not sure I follow your comment here?

If we only wanted tasks here I wouldn't mind just showing the task, and keeping the active tasks and the completed tasks separate on the index page.

But we merged the concepts here, and I think it's awesome, but it's a bit limited by not surfacing the currently running tasks, they're just lost in the list.

I almost feel like we should show:

  • first the tasks that have an active run (something is happening right now)
  • then the tasks that have not run yet (they're new, so you may want to run them soon)
  • finally all the tasks that have a finished run (which may be good candidates to clean up)

And yes the runs for deleted tasks are out of scope, I just thought we could tackle the n+1 and surfacing the running tasks before shipping this. But it's fine I can look at it when doing the deleted tasks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about the order in which we rank the Tasks. We can improve it further in another PR.

<%= status_tag(run.status) %>
<% else %>
<span class="tag is-primary">New</span>
volmer marked this conversation as resolved.
Show resolved Hide resolved
<% end %>
Comment on lines +5 to +9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could also extract this into a helper to be reused between this and show.html.erb?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for now. I need the run instance to be in the view itself for later usage in the render below. We can improve it later on.

</h3>


volmer marked this conversation as resolved.
Show resolved Hide resolved
<%= render 'maintenance_tasks/runs/info', run: run if run %>
</div>
66 changes: 1 addition & 65 deletions app/views/maintenance_tasks/tasks/index.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,65 +1 @@
<h1 class="title is-1">Maintenance Tasks</h1>

<aside class="menu">
<ul class="menu-list">
<% @tasks.each do |task| %>
<li><%= link_to task, task_path(task) %></li>
<% end %>
</ul>
</aside>

<h2 class="title is-2">Running Tasks</h2>
<table class="table" id="running-tasks">
<thead>
<tr>
<th>Task name</th>
<th>Created at</th>
<th>Status</th>
<th>Progress</th>
<th>Estimated time to completion</th>
</tr>
</thead>

<tbody>
<% @active_runs.each do |run| %>
<tr>
<td><%= link_to run.task_name, task_path(run.task_name) %></td>
<td><%= formatted_datetime(run.created_at) %></td>
<td><%= status_tag(run.status) %></td>
<td><%= format_ticks(run) %></td>
<td><%= estimated_time_to_completion(run) %></td>
</tr>
<% end %>
</tbody>
</table>

<%= pagination(@pagy) %>

<h2 class="title is-2">Completed Tasks</h2>
<table class="table" id="completed-tasks">
<thead>
<tr>
<th>Task name</th>
<th>Created at</th>
<th>Started at</th>
<th>Status</th>
<th>Ended at</th>
</tr>
</thead>
<tbody>
<% @latest_completed_runs.each do |run| %>
<tr>
<td><%= link_to run.task_name, task_path(run.task_name) %></td>
<td><%= formatted_datetime(run.created_at)%></td>
<td><%= formatted_datetime(run.started_at)%></td>
<td><%= status_tag(run.status)%></td>
<td><%= formatted_datetime(run.ended_at)%></td>
</tr>
<% end %>
</tbody>
<tfoot>
<tr>
<td colspan="3">Showing the last <%= pluralize(@latest_completed_runs.size, 'completed task') %>.</td>
</tr>
</tfoot>
</table>
<%= render partial: 'task', collection: @tasks %>
Loading