[django][admin] Ignore safely tasks already enqueued. - #1512
Conversation
Provides, in my opinion, a more user friendly experience, by allowing the submission to work,
even if duplicate jobs have been selected.
It could be tedious to expect users to perform a strict selection by eliminating duplicated entries.
Happy path scenario:
User selects 1 page of failed jobs to retry.
only the new one will be accepted for a retry, and
will safely keep the other ones for a later attempt.
User repeats until all failed jobs have been retried.
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThe retry flow in the Django admin module is wrapped within an atomic database transaction and configured to suppress Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
procrastinate/contrib/django/admin.py (1)
143-152:⚠️ Potential issue | 🟡 MinorNo feedback when jobs are skipped as already-enqueued.
When
suppresssilencesAlreadyEnqueuedfor one or more jobs, the admin action completes with no message. If all selected jobs are skipped, the user sees nothing — indistinguishable from a no-op. Consider tracking a counter and emitting aself.message_user()summary.💡 Example feedback pattern
`@admin.action`(description="Retry Job") def retry(self, request: HttpRequest, queryset: QuerySet[models.ProcrastinateJob]): app_config: ProcrastinateConfig = apps.get_app_config("procrastinate") p_app: App = app_config.app + retried, skipped = 0, 0 for job in queryset.filter( status__in=(Status.FAILED.value, Status.DOING.value) ): with suppress(AlreadyEnqueued), transaction.atomic(): - p_app.job_manager.retry_job_by_id( - job.id, utils.utcnow(), job.priority, job.queue_name, job.lock - ) + p_app.job_manager.retry_job_by_id( + job.id, utils.utcnow(), job.priority, job.queue_name, job.lock + ) + retried += 1 + else: + skipped += 1 + self.message_user( + request, + f"Retried {retried} job(s); {skipped} skipped (already enqueued).", + )(The
elseclause on thewithblock does not exist in Python — track the counter insidesuppresswith a flag or a dedicated counter approach instead.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@procrastinate/contrib/django/admin.py` around lines 143 - 152, The retry admin action currently suppresses AlreadyEnqueued silently; update ProcrastinateAdmin.retry to count how many jobs were retried vs skipped: iterate the same queryset.filter(status__in=(Status.FAILED.value, Status.DOING.value)) and for each job wrap the p_app.job_manager.retry_job_by_id(...) call in the existing with transaction.atomic(), suppress(AlreadyEnqueued): but set a local flag or increment a skipped counter inside the suppress scope when AlreadyEnqueued would have been raised (e.g. use a try/except around the call to detect AlreadyEnqueued and increment skipped), and increment a retried counter when the call succeeds; after the loop call self.message_user(request, f"Retried {retried} job(s); skipped {skipped} already-enqueued job(s).") to surface the result to the admin UI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@procrastinate/contrib/django/admin.py`:
- Around line 149-152: The context-manager order around the retry must be
reversed so the atomic block is inside the suppress: wrap
p_app.job_manager.retry_job_by_id(...) with suppress(AlreadyEnqueued) on the
outside and transaction.atomic() on the inside; i.e., change the current "with
transaction.atomic(), suppress(AlreadyEnqueued):" to have suppress as the outer
context and transaction.atomic() as the inner context so transaction.atomic()
sees the exception first and the suppress then swallows AlreadyEnqueued.
---
Outside diff comments:
In `@procrastinate/contrib/django/admin.py`:
- Around line 143-152: The retry admin action currently suppresses
AlreadyEnqueued silently; update ProcrastinateAdmin.retry to count how many jobs
were retried vs skipped: iterate the same
queryset.filter(status__in=(Status.FAILED.value, Status.DOING.value)) and for
each job wrap the p_app.job_manager.retry_job_by_id(...) call in the existing
with transaction.atomic(), suppress(AlreadyEnqueued): but set a local flag or
increment a skipped counter inside the suppress scope when AlreadyEnqueued would
have been raised (e.g. use a try/except around the call to detect
AlreadyEnqueued and increment skipped), and increment a retried counter when the
call succeeds; after the loop call self.message_user(request, f"Retried
{retried} job(s); skipped {skipped} already-enqueued job(s).") to surface the
result to the admin UI.
Provides, in my opinion, a more user friendly experience, by allowing the submission to work, even if duplicate jobs have been selected.
It could be tedious to expect users to perform a strict selection by eliminating duplicated entries. Happy path scenario:
User selects 1 page of failed jobs to retry.
only the new one will be accepted for a retry, and
will safely keep the other ones for a later attempt.
User repeats until all failed jobs have been retried.
Successful PR Checklist:
PR label(s):
Summary by CodeRabbit