Fix presentation of the Queue in the Recurring Jobs page #41
Conversation
Fryguy
force-pushed
the
fix_recurring_jobs_queue
branch
3 times, most recently
from
7c66168
to
937f0e1
Compare
|
mind rebasing this against main? |
Fryguy
force-pushed
the
fix_recurring_jobs_queue
branch
from
937f0e1
to
69de50a
Compare
|
Rebased - There are a handful of CI failures, but they all seem related to timeouts to Redis |
| @job_specs.each_value do |job_spec| | ||
| job_spec['cron'] ||= job_spec['every'] | ||
|
|
||
| job_spec['queue'] ||= begin |
There was a problem hiding this comment.
I'm wondering if this should be in the from_hash function in the Schedule class, so that this behavior is consistent even outside of this view?
There was a problem hiding this comment.
(i'm also wondering if we need to also support ActiveJob's queue_as
There was a problem hiding this comment.
Possibly? However, this code explicitly calls from_redis, so it appears to be redis specific to begin with (not sure why, just noting the observation)
There was a problem hiding this comment.
yeah, true. but makes me think this logic should be in the schedule class... iow -- there seems to be some manipulation of the state of the schedule before display, such as the queue, so that logic either is duplicated somewhere or what's being displayed isn't matching what's actually being done (like the queue). (one other option is that i just haven't had enough ☕ )
There was a problem hiding this comment.
I think the change to refactor into the from_hash method might be a bit out of my depth as I'm not as familiar with the code base. I'm wondering if it makes sense to merge this PR as is, since it fixes the problem, and then a follow up could be done to refactor into from_hash
Fixes #38
Built on top of #39 so I could actually test the fix via the web interface.EDIT: This was merged elsewhereBefore:
After:
