Add support for multiple runs per task

[promulo]

Currently, only one task run can be active (running or paused) at a given time. This PR intends to add support for multiple runs for a given task to the gem.

The approach here was to introduce as least disruption as possible and implement minimal changes to both backend and UI, still adhering to current patterns. The dashboard and individual task pages were tweaked a bit in order to allow displaying more than one run for the same task. This means we should rely less on the concept of last run and more on active runs.

I haven't experienced issues when manually testing this. Multiple task runs can be triggered using the usual Run button and individual runs can be paused and resumed accordingly.

This is how the "new" index page looks like with these changes (the list of "New Tasks" and "Completed Tasks" look the same):

The above screenshot shows a real example of two runs of the same task being active at the same time.

As for the individual task page, this is the current state:

That is an example of a task with multiple active runs listed on its own page.

I will keep pushing changes to this branch but wanted to have some early feedback. The proposed UI might not be what we want. I'm also considering splitting this in separate backend/frontend PRs if that favors iteration.

Thanks!

[dirceu]

Nice!

Couple of ideas for the UI:

1. On the individual task page, we could make the source code collapsable—maybe with a "Review code" or "See code" button right besides "Run". Especially for "reusable" tasks with multiple runs, the code is important but if you have multiple concurrent runs, it might be easier to follow their progress if you can start with the code already collapsed.
2. Speaking of following progress, I wonder if it would make sense to show the run ID somewhere to make it easier to identify what already finished. That could make it easier to identify how things are moving if, for example, if the third run out of 7 you started finished early.

[etiennebarrie]

Regarding the UI failures, most of them can be fixed easily:

• we don't need to check the Run button is disabled anymore
• the pause/cancel buttons should be available when the task is enqueued

I pushed commits for these.
The last issue remaining is dealing with double enqueues. We should be able to fix the test to first create a run before opening a new tab.

[promulo]

Pushed a fix for the failing test.

Do we think this UI be good enough as a first iteration of this feature? We could then build on top of it for a better UX.

[adrianna-chang-shopify]

This feature is SO SO exciting to see! 😍 A couple small comments, but the code looks good and so does the tophat!

I think we'll need to update the docs for a couple methods now that the "single-run-per-task" assumption no longer applies (some of the methods in TaskData might need to have their descriptions tweaked, like #available_tasks).

One thing I noticed while tophatting that we might want to tweak: currently, a task's status is determined by using the status of the last run:

maintenance_tasks/app/models/maintenance_tasks/task_data.rb

Lines 121 to 123 in 091d31c

	def status
	last_run&.status || "new"
	end

This leads to situations like this:

Where the last run may have a terminal status (succeeded, cancelled, etc.) but prior runs are still in an active state. Does it make sense here to consider the status of the last active run, if any, instead of just the last created run for the task? If there are multiple runs with active statuses (for example, one is paused, one is interrupted, one is running), should the task potentially report on all of these?

[promulo]

Thanks a lot @adrianna-chang-shopify, those are all fair points. I'll update the PR soon.

[promulo]

One thing I noticed while tophatting that we might want to tweak: currently, a task's status is determined by using the status of the last run [...] Does it make sense here to consider the status of the last active run, if any, instead of just the last created run for the task?

@adrianna-chang-shopify Nice catch! I do think it makes sense and we could keep it simple for this first iteration. Maybe in the future we drop the label from the task itself and add it to each run? 🤔

[adrianna-chang-shopify]

Nice catch! I do think it makes sense and we could keep it simple for this first iteration. Maybe in the future we drop the label from the task itself and add it to each run? 🤔

Yes, absolutely we can hold off for this iteration! It was just something that popped into my head while tophatting, but we might want to experiment with a couple different options to see which one is the most user friendly.

One last comment -- it won't let me add it directly into the changes because the code wasn't changed in this PR, but I think we also want to modify #category to align with the changes made to #status, and consider a task "active" if any of its runs are currently active:

diff --git a/app/models/maintenance_tasks/task_data.rb b/app/models/maintenance_tasks/task_data.rb
index c82a27f..41f9159 100644
--- a/app/models/maintenance_tasks/task_data.rb
+++ b/app/models/maintenance_tasks/task_data.rb
@@ -143,7 +143,7 @@ module MaintenanceTasks
     #
     # @return [Symbol] the category of the Task.
     def category
-      if last_run.present? && last_run.active?
+      if active_runs.any?
         :active
       elsif last_run.nil?
         :new

Great work on this! ❤️

[promulo]

@adrianna-chang-shopify After discussing with @etiennebarrie we thought it would be a good idea to already drop the label from the title in the #show page and rely only on individual runs' status labels.

That would avoid some confusion in the index page and we can keep the current implementation for TaskData#category and TaskData#status. Wdyt?

P.S.: Postgres is being funny with the ordering, that's why the build is red. Working on it.

[adrianna-chang-shopify]

After discussing with @etiennebarrie we thought it would be a good idea to already drop the label from the title in the #show page and rely only on individual runs' status labels.

Yep, I'm OK with this too! 👍

[etiennebarrie]

We want to remove that concept of a last_run since now we either want the active run or the completed runs. From the index, we go through TaskData.available_tasks, and the TaskData objects are initialized with a Run each (or nil).

We need to handle the case of TaskData.find, which right now is used to cause a 404 in tasks#show, but since we don't really want that usage of last_run, and completed_runs is paginated (and maybe one day active_runs too?), it can be hard to know whether there is a Run or not for the 404 to be paginated.

Maybe we could pass the cursor to the task data, and it would build the RunsPage, that way we would load the active runs, load the paginated completed runs, and if we don't have any of these, we could cause the 404. Alternatively we could add a line to the controller, so that the exception leading to the 404 is raised at the end of the controller action, once we know if there's any run or not because we've loaded the active and completed runs.
[These two solutions would add a 404 where there's not now: if you "paginate" past the last cursor of a deleted task, right now we will find the last run and show it at the top anyway, but if we only rely on loaded data to know whether to 404, we will get a 404. But it's not much of a problem, the pagination would never get you there, only by changing the cursor in the URL to a value before which there's no run would you get that 404.]

We could also live with an additional query just to check the existence of a run, but I find that a bit sad, since we're loading the active/completed runs anyway, we could rely on that.

[promulo]

I removed TaskData#last_run as a method but kept an alias for the old references. The instance variable is now called related_run. Updating all references can be done in a follow-up PR.

[adrianna-chang-shopify]

This looks pretty close to me, although there's still some confusion on my end about ordering the indexes.

I think @etiennebarrie had also mentioned getting rid of the concept of last_run on TaskData, now that we're storing active / completed runs?

[etiennebarrie]

Looking good!

[adrianna-chang-shopify]

LGTM, great work on this! I chatted with Étienne about some changes that will be made in a follow up PR, but I think this is good to go in the meantime

[etiennebarrie]

🎉