Introduce support for task staleness threshold#1406
Introduce support for task staleness threshold#1406joshmfrankel wants to merge 2 commits intoShopify:mainfrom
Conversation
joshmfrankel
force-pushed
the
joshmfrankel/add-support-for-outdated-tasks
branch
from
217b6cd to
039de31
Compare
| within page.find("a", text: "Maintenance::OutdatedTask").find(:xpath, "..").sibling(".has-text-warning") do |element| | ||
| assert_text "This task last ran 1 day ago. Consider removing it as it may be outdated." | ||
| end |
There was a problem hiding this comment.
Future: Might be worthy of a follow-up PR to separate the various sections on the Task#index page within individual parent divs. That would make the lookup easier for testing as well as organizing the DOM.
jenshenny
left a comment
jenshenny
left a comment
There was a problem hiding this comment.
I think having a way to surface stale tasks would be valuable to have!
| # Returns whether the run is beyond the outdated task threshold. | ||
| # | ||
| # @return [Boolean] | ||
| def outdated? |
There was a problem hiding this comment.
outdated doesn't seem like the best fit for this feature as it implies that the task should be updated while I believe we want to convey that the task was run and may not be used anymore.
How about stale ?
There was a problem hiding this comment.
I love the clarity here. I'll make the update to stale terminology 🎉
lib/maintenance_tasks.rb
Outdated
| # Defaults to 30 days. | ||
| # | ||
| # @return [ActiveSupport::Duration] the threshold after which a task is considered outdated. | ||
| mattr_accessor :outdated_task_threshold, default: 30.days |
There was a problem hiding this comment.
Is there a way to opt out of this feature?
There was a problem hiding this comment.
Another great catch. I'll build this in to allow opt-out
|
|
||
| alias_method :to_s, :name | ||
|
|
||
| delegate :outdated?, to: :related_run |
There was a problem hiding this comment.
| delegate :outdated?, to: :related_run | |
| delegate :outdated?, to: :related_run, allow_nil: true |
This would raise if related_run is nil. Can we also add a test for if there is no run?
There was a problem hiding this comment.
Great catch! I didn't catch the default arg of nil above. I wasn't able to get allow_nil: true to work how I wanted since it would return nil for the TaskDataIndex#stale? delegation where I want it to be a Boolean. That said maybe there is a default value that could be set using this (couldn't find anything in the docs).
I'm making an update to remove the delegation in favor of a full method definition which handles the above nicely.
| def outdated? | ||
| return false unless ended_at.present? | ||
|
|
||
| ended_at < MaintenanceTasks.outdated_task_threshold.ago |
There was a problem hiding this comment.
Can we only scope this to succeeded runs only?
* Changed all language of `outdated` to `stale` * Provided an opt-out mechanism by setting MaintenanceTasks.task_staleness_threshold to false * Protected delegated calls to related run being NilClass * Scoped staleness checks to only succeeded Runs
|
@jenshenny Great feedback! I believe I've incorporated all suggestions 🎉 |
joshmfrankel
changed the title
2 participants
Why
Maintenance tasks are ephermeral by nature. This pull request introduces a mechanism to display a gentle message to encourage removal of Tasks which have not run within the configured threshold.
"Maintenance tasks aren't meant to happen on a regular basis. They're used as needed, or as one-offs. Normally maintenance tasks are ephemeral, so they are used briefly and then deleted."
What
task_staleness_thresholdconfigurationOpen Questions