UI improvements

[volmer]

Meet the all-new, refreshed, and modern Maintenance Tasks. This is the biggest visual improvement in Maintenance Tasks we've ever made.

Changes:

• The index page is now much simpler and tableless. It just lists your available Tasks, which status and progress information about their latest Runs, if present.
• The Task page now emphasizes the last Run's information on top, with its progress and status information.
• Statuses now dictates which information we show. For instance, we only show error data if a Run has errored. Then the exception info is displayed in a beautiful Bulma card.
• When a Task is enqueued we no longer render a needless notice. The change of status and progress bar should already give the user enough feedback.
• Removed the concept of "active" Run in favour of the last Run, regardless of its status. This is important because that's what our Users care about. Except for recurring Tasks, users will run Tasks only once, and want to check that one Run at all times.
• Progress bar is now displayed regardless of a count or tick_total. When a Run is active we will show an infinite progress, and when it is completed we use the tick count as max, showing a full bar. This provides a consistent and beautiful UI for all Runs.
• Tick information is now hidden from the UI. Users can still read them by hovering the progress bar.
• The progress bar now assumes different colours for different statuses. For example, an errored Run will show a red progress bar.
• Reduced the refresh rate to 3 seconds. Maintenance Tasks is now almost 2x faster!
• Pages are now responsive. Pages are now rendered much nicer in smaller screens. I simply love browsing and running Maintenance Tasks on my iPhone while having coffee in the morning.

cc @kateneely

Index Page

Before	After

Successful Task Page

Before	After

Error Task Page

Before	After

Fixes #185
Fixes #186

[adrianna-chang-shopify]

I'm a big fan of the new UI, awesome work Volmer! Couple nits but nothing major.

Just wanted to note that the changeset for this PR was pretty big, and I think it could have benefitted from being broken down into smaller commits to make reviewing a bit easier.

Appreciate the care that went into designing this new UI though ❤️

[adrianna-chang-shopify]

Can we make this a scope on Run?

[volmer]

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

[adrianna-chang-shopify]

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) }

[volmer]

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.

[adrianna-chang-shopify]

Can we extract this into a helper to it's reusable between this partial and _running?

[volmer]

The Running partial does not use the same wording, and we might want to change this content later on. This apparent duplication is on purpose for now.

[adrianna-chang-shopify]

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

[volmer]

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.

[adrianna-chang-shopify]

Oh I also just noticed that we've dropped showing the started_at and ended_at timestamps. IIRC this is something our users explicitly asked for, and I believe it's still valuable to show. Is there a way to present these in the cards?

Additionally, might be nice to have the created_at timestamp for the last Run too?

[etiennebarrie]

Really like the UI!

I agree with @adrianna-chang-shopify we are losing of the timestamp info that was important to some of our users though.

[etiennebarrie]

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

[volmer]

What would you suggest instead?

[etiennebarrie]

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

[etiennebarrie]

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

[volmer]

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.

[etiennebarrie]

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

[volmer]

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.

[etiennebarrie]

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).

[volmer]

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.

[etiennebarrie]

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.

[volmer]

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

[etiennebarrie]

One thing that wasn't mentioned is that it always refreshes now, not just when there's an active run. It's a bit annoying to be honest, but I understand it should help with always having an up-to-date UI.

One thing where it will definitely be annoying though, is if we add a form to upload a CSV for CSV tasks. But that's a problem for later I guess.

[volmer]

Oh I also just noticed that we've dropped showing the started_at and ended_at timestamps. IIRC this is something our users explicitly asked for, and I believe it's still valuable to show. Is there a way to present these in the cards?

Additionally, might be nice to have the created_at timestamp for the last Run too?

I omitted these timestamps on purpose in favour of showing the elapsed time and ETA. My assumption is that these are more useful and sufficient for now. I would go with this minimal info for 1.0 and evaluate if we get requests to render this data as well. cc @kateneely

[adrianna-chang-shopify]

My assumption is that these are more useful and sufficient for now. I would go with this minimal info for 1.0 and evaluate if we get requests to render this data as well.

I thought when we created the issue for adding started_at and ended_at it was in response to Partners requesting this information. With just the elapsed time and created_at, there's no way to infer when the task finished if it was paused / interrupted for part of that time. I also think it's helpful to have a timestamp associated with the time a task errored or was cancelled, which we don't have anymore (but was tracked with ended_at).

We're storing this information on the run, so I think it's valuable to show it. It's not a blocker for this PR, but I would like to add this info back in. We could maybe have an expandable "details" section to the cards, so that it doesn't clutter up the new UI.

There's also no information about when the most recently run task ran, I think we should at least show the created_at timestamp for these as well. Otherwise you have no idea when that task ran.

[etiennebarrie]

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

[etiennebarrie]

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

[etiennebarrie]

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.

[etiennebarrie]

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.

[volmer]

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

[volmer]

I thought when we created the issue for adding started_at and ended_at it was in response to Partners requesting this information. With just the elapsed time and created_at, there's no way to infer when the task finished if it was paused / interrupted for part of that time. I also think it's helpful to have a timestamp associated with the time a task errored or was cancelled, which we don't have anymore (but was tracked with ended_at).

We're storing this information on the run, so I think it's valuable to show it. It's not a blocker for this PR, but I would like to add this info back in. We could maybe have an expandable "details" section to the cards, so that it doesn't clutter up the new UI.

There's also no information about when the most recently run task ran, I think we should at least show the created_at timestamp for these as well. Otherwise you have no idea when that task ran.

Agreed that ended_at might be useful for some statuses, so I added it back. I still think it's premature to assume Users will need to know created_at and started_at, so I'm leaving these for another time.