Add Non-Standard delayed_job Columns to the Display #131
Conversation
…isplay. This is important for any gems that add columns to delayed jobs like https://github.com/codez/delayed_cron_job
|
Tagging @codez for their excellent DelayedCronJob. |
|
@andyatkinson @ejschmitt @nashby Should I tag someone else who is currently maintaining delayed_job_web to get this PR merged? Would love to eliminate my fork and give this unto the world. Thanks! |
|
@thegeorgeous are you able to merge this PR? Thanks! |
|
Would love to see this merged. |
| <% job.attribute_names.excluding('id', 'priority', 'attempts', 'queue', 'handler', 'last_error', 'run_at', 'locked_at', 'locked_by', 'failed_at').each do |column| %> | ||
| <dt><%= column %></dt> | ||
| <dd> | ||
| <%=h eval("job.#{column}") %> |
There was a problem hiding this comment.
Is there a security risk here someone could exploit with arbitrary code execution within the eval() block? Any safer alternatives to this with the same result?
I do it's scoped to job.attribute_names, but it feels like there's possible a safer alternative here.
There was a problem hiding this comment.
One idea (untested), is there externalized configuration (an initializer?) that can be added with the non-standard column names instead or using eval?
There was a problem hiding this comment.
This should be equivalent, I think.
| <%=h eval("job.#{column}") %> | |
| <%=h job.send(column) %> |
|
@markbeme @AmritD I don't use this anymore. I intended to merge it, but I raised a security concern here that gave me pause. Can you take a look? |
| <dt><%= column %></dt> | ||
| <dd> | ||
| <%=h eval("job.#{column}") %> | ||
| </dd> |
There was a problem hiding this comment.
I also saw that if the value is blank for the column, it breaks the layout. So, this should probably be:
| <dt><%= column %></dt> | |
| <dd> | |
| <%=h eval("job.#{column}") %> | |
| </dd> | |
| <% column_value = job.send(column) %> | |
| <% if column_value %> | |
| <dt><%= column %></dt> | |
| <dd> | |
| <%=h column_value %> | |
| </dd> | |
| <% end %> |
Adding non-standard column names to the display is important for any gems or site-specific modifications that add columns to delayed_jobs like https://github.com/codez/delayed_cron_job (which adds the cron column).
Interestingly, this also adds created_at and updated_at to the Display, something I personally find valuable.
Ideas for improvement on this contribution: