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.

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

Dashboard: show more details about jobs #575

Merged
merged 21 commits into from
May 18, 2022

Conversation

bkeepers
Copy link
Contributor

@bkeepers bkeepers commented Apr 20, 2022

This fixes #547 by updating jobs#show page to include status and other details about the job.

image

Copy link
Owner

@bensheldon bensheldon left a comment

Choose a reason for hiding this comment

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

@bkeepers This is fantastic!

One request: would you be able to fit in the action buttons (reschedule discard, retry)?

@bkeepers
Copy link
Contributor Author

One request: would you be able to fit in the action buttons (reschedule discard, retry)?

Absolutely. I was thinking actions could go on the top right.

Copy link
Owner

@bensheldon bensheldon left a comment

Choose a reason for hiding this comment

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

👍 This looks good to me. I really appreciate you building out the page.

I'd love to land this in the same version with #576. Could you confirm that you're ok with me merging?

@bkeepers
Copy link
Contributor Author

One request: would you be able to fit in the action buttons (reschedule discard, retry)?

Actions have been added.

image

I'd love to land this in the same version with #576. Could you confirm that you're ok with me merging?

I'd love to do a little more work on it if you're ok with waiting a few hours. There are a few things I still find awkward (Error always shows up, even if the most recent execution passes).

* origin/main:
  Remove executions from the dashboard (bensheldon#576)
@bensheldon
Copy link
Owner

@bkeepers no prob 👍

@bensheldon bensheldon added the enhancement New feature or request label Apr 25, 2022
* origin/main:
  Release good_job v2.14.1
  Temporarily disable Mass Action "Apply to all" because the action is badly scoped (bensheldon#583)
  Release good_job v2.14.0
  Add mass update operations for jobs to Dashboard (bensheldon#578)
  Track down incompatibility/race condition between JRuby and RSpec mocks in tests (bensheldon#581)
  Release good_job v2.13.2
  Namespaces assets per Rails docs (bensheldon#580)
  Release good_job v2.13.1
  Set up javascript importmaps for Dashboard; refactor Polling (bensheldon#574)
  Use toasts to show notices and alerts (bensheldon#577)
* origin/main:
  Improve development instructions and tooling (rename bin/rails, add bin/appraisal) (bensheldon#590)
  Fix name of main branch in GH Action
  Include `ruby` version in development Gemfile; update to Ruby 3.0.4 (bensheldon#588)
  Replace test Instrumentation mocking with temporary subscriptions (bensheldon#589)
  Release good_job v2.14.2
  Reintroduce fixed "Apply to all" mass action (bensheldon#586)
  Refactor Dashboard Live Poll javascript (bensheldon#582)
  Update development dependencies (bensheldon#584)
engine/app/helpers/good_job/application_helper.rb Outdated Show resolved Hide resolved
engine/app/helpers/good_job/application_helper.rb Outdated Show resolved Hide resolved
<tr>
<td colspan="8" class="py-2 text-center text-muted">No executions found.</td>
</tr>
<h5>Executions</h5>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIXME: translation

<%= relative_time execution.finished_at || execution.performed_at || execution.scheduled_at || execution.created_at, include_seconds: true %>

<% if execution.runtime %>
• <div><%= format_duration execution.runtime %> runtime</div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIXME: translation for "runtime"

• <div><%= format_duration execution.runtime %> runtime</div>
<% end %>
<% if execution.latency %>
• <div><%= format_duration execution.latency %> latency</div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIXME: translation for "latency"

Discard
<% end %>

<%= button_to retry_job_path(@job.id), method: :put, class: "btn btn-sm #{@job.status == :discarded ? 'btn-outline-primary' : 'btn-outline-secondary'}", form_class: "d-inline-block", disabled: @job.status != :discarded, aria: { label: "Retry job" }, title: "Retry job", data: { confirm: "Confirm retry" } do %>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIXME: translation for "Retry job" and "Confirm retry"

</div>

<div class="my-4">
<h5>Arguments</h5>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIXME: translation for "Arguments"


def running?
performed_at? && !finished_at?
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bensheldon I moved #status to Execution because I wanted it for the jobs#show view, but this definition of #running? is different than in Job which uses advisory locks. Is this ok?

Copy link
Owner

Choose a reason for hiding this comment

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

👍 Yep, that's ok. It's possible an execution record will be in a weird state as a result of a SIGKILL, but under normal operation this is the correct logic.

end

def relative_time(timestamp, **args)
text = timestamp.future? ? "in #{time_ago_in_words(timestamp, **args)}" : "#{time_ago_in_words(timestamp, **args)} ago"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIXME: not new to this PR, but "in …" and "… ago" need translations

STATUS_ICONS = {
discarded: "exclamation",
finished: "check",
queued: "clock",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this use a different icon here?

Copy link
Owner

Choose a reason for hiding this comment

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

What do you think of this icon for queued? https://icons.getbootstrap.com/icons/list-nested/

Copy link
Owner

Choose a reason for hiding this comment

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

Another option would be to change "Scheduled" to be something like this calendar event and then use the Clock icon for queued.

This is like an advanced naming problem 🤣

Copy link
Owner

Choose a reason for hiding this comment

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

Hmmm. The nested-list looks a little odd because everything else is in a circle:

Screen Shot 2022-05-17 at 8 26 22 AM

I think I'm going with dash-circle. I also changed the color to warning (which is waaaay to bright, but we can color them later:

Screen Shot 2022-05-17 at 8 32 43 AM

@bensheldon
Copy link
Owner

@bkeepers don't sweat tokenizing the strings for i18n; that can be deferred till later.

@bensheldon bensheldon marked this pull request as ready for review May 18, 2022 13:47
Copy link
Owner

@bensheldon bensheldon left a comment

Choose a reason for hiding this comment

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

@bkeepers Thank you for the huge improvement 🎉

I'm going to accept this now and please point out any dangling problems I missed 😅

@bensheldon bensheldon merged commit ee6716b into bensheldon:main May 18, 2022
bensheldon added a commit that referenced this pull request May 18, 2022
…ded_jobs` option to retain discarded jobs during cleanup (#597)

* Adds the ability to delete jobs to the dashboard

* Adds an option to preserve discarded jobs on cleanup

* Adds ActiveJobJob#destroy_job and uses it

* Renames delete -> destroy everywhere

* Add Destroy button to top of Jobs#show page (redesigned in #575)

Co-authored-by: Ben Sheldon [he/him] <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show status on jobs#show page
2 participants