[3.x] Ability to see if a job is delayed #755
Conversation
|
This looks okay but I would use a different color since blue is often associated with something that goes okay. Maybe the same yellow from the pause symbol? Also I'd just say "delayed" and not "delayed job". Maybe add that timestamp on hover? |
This reverts commit 78f83ea.
|
Like this? |
|
That looks better imo but let's see what others say :) |
|
To sum up the colors, my personal favorite is the purple (primary) one.
|
|
@Cannonb4ll first, very nice!! I like this PR!
|
|
Just committed the tooltip on hover: For this I had to move popper.js import above the bootstrap import because the docs say so and jQuery import to the app.js in order for the tooltips to work. Then I made a directive for the tooltip so each new record would also get the tooltip instance bound to its element. (Otherwise new records that show up dynamically will not have the tooltip anymore) This also resulted that I removed the jQuery imports from the specific components. I also took the liberty to reduce JS size by importing the specific lodash package "take" in "Stacktrace.vue" as an example. We could do this with more components as I noticed the full lodash library is imported there which gives a lot of overhead of JS. Let me know what you think! |
|
It's weird to me to show this label for a job that is already finished processing. It only makes sense to me to see it while pending and even better if it shows a human readable thing like "Delayed for 10 minutes" |
|
@taylorotwell i think you right. It is better to show the label when the delay is not passed. But where do you want to show the "Delayed for 10 minutes"? Only in the hover? I think it is better to say "xx minutes" in the hover. |
|
I agree with you @taylorotwell, which option do you prefer more? Option 1: Option 2: |
|
If you choose option 1 than remove the "Delayed for" in the hover. The label are say that it is a delayed job. |
|
I'm fine with the tooltip... I think purple is a bit "in your face" for this informational stuff... I would have gone with a gray background or something. |
|
We can grey these out, with |
|
Is this ready for review on your end? |
|
Yes, it is. Not sure why the travis builds fail randomly though. |
|
Sorry, quickly added 1 more small detail in the job detail screen, the Pushed At: |
| @@ -38,6 +38,8 @@ public function index(Request $request) | |||
| $jobs = $this->jobs->getRecent($request->query('starting_at', -1))->map(function ($job) { | |||
| $job->payload = json_decode($job->payload); | |||
|
|
|||
| $job->delayed = strtotime(optional(unserialize($job->payload->data->command))->delay); | |||
There was a problem hiding this comment.
If you are unserializing the entire command to get this won't that re-issue the database queries needed to get the Eloquent models / collections required by the job?
There was a problem hiding this comment.
@taylorotwell Just tried, indeed it will introduce queries to be run whenever a model is type hinted into the job class, this will introduce reduced performance once there are jobs with a lot of models.
Is there a way to disable this magic once we call an unserialise on the payload?
There was a problem hiding this comment.
Not that I can think of off of the top of my head. It may be possible to extract the delay from the raw serialized string but I'm not sure.
There was a problem hiding this comment.
I am going to think and try some stuff out. Will get back on this, thanks, good find 👍
|
Ok, requesting a review again for this one. What I did is remove the PHP way with unserialising as this was causing DB queries. Instead I used the phpunserialize JS package which was already included. In order to do this I exported each TR row to its own component to make proper use of computed records. Now there are no DB queries anymore. |
|
Thank you for this! |
Sometimes I find myself in a situation where I want to see if a job was dispatched with a delay, this PR allows you to see if it is via a badge.
Examples: