Dj celery panel#8 db based periodic tasks - #13
Conversation
📝 WalkthroughWalkthroughThe PR introduces a pluggable backend system for retrieving Celery periodic tasks from different sources, including beat_schedule configuration and django-celery-beat database. It replaces direct inspector calls with an interface-based approach, adds configurable backends, includes comprehensive test coverage, and updates documentation with configuration examples. Changes
Sequence DiagramsequenceDiagram
participant View as View Layer
participant Interface as CeleryPeriodicTasksInterface
participant Backend as Configured Backend
participant Source as Data Source
View->>Interface: get_periodic_tasks()
Interface->>Interface: Load configured backend<br/>(or DEFAULT_BACKEND)
Interface->>Backend: Instantiate with app
Backend->>Source: Fetch periodic tasks
Source-->>Backend: Return task data
Backend->>Backend: Parse & format tasks<br/>to PeriodicTaskListPage
Backend-->>Interface: PeriodicTaskListPage
Interface-->>View: PeriodicTaskListPage<br/>(with tasks & error)
View->>View: Display tasks & handle error<br/>if present
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (76.29%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #13 +/- ##
==========================================
+ Coverage 76.23% 76.29% +0.05%
==========================================
Files 12 13 +1
Lines 606 675 +69
Branches 74 82 +8
==========================================
+ Hits 462 515 +53
- Misses 112 129 +17
+ Partials 32 31 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@dj_celery_panel/celery_utils/periodic_tasks.py`:
- Around line 95-97: The select_related call on
PeriodicTask.objects.filter(enabled=True).select_related("interval", "crontab",
"solar") omits "clocked", causing an N+1 query when the code later accesses
task.clocked; update the select_related invocation to include "clocked" so the
clocked relation is fetched in the same query (locate the select_related call in
periodic_tasks.py and add "clocked" to the tuple/list of relations).
In `@tests/test_periodic_tasks_integration.py`:
- Around line 16-28: The Celery instance created in setUp (self.test_app) is
never wired to the view's global current_app; update setUp to replace the
module-level Celery current_app used by the view with self.test_app (and restore
it in tearDown) so the view reads the test beat_schedule; specifically, patch or
assign celery.current_app (or the view module's reference to current_app) to
self.test_app inside setUp and revert the change in tearDown so tests actually
exercise the periodic tasks in the view.
🧹 Nitpick comments (4)
tests/test_periodic_tasks.py (1)
125-144: Consider using underscore prefix for intentionally unused variables.The
task1andtask2variables are assigned but never directly used in the test assertions (the test queries them back from the database). While this is intentional, using underscore prefix makes the intent clearer and silences linter warnings.♻️ Suggested fix
# Create a periodic task with interval schedule - task1 = PeriodicTask.objects.create( + PeriodicTask.objects.create( name="db-task-1", task="app.tasks.db_task", interval=self.interval_schedule, enabled=True, args='[1, 2]', kwargs='{"key": "value"}', total_run_count=5, ) # Create a periodic task with crontab schedule - task2 = PeriodicTask.objects.create( + PeriodicTask.objects.create( name="db-task-2", task="app.tasks.another_db_task", crontab=self.crontab_schedule, enabled=True, args="[]", kwargs="{}", total_run_count=10, )dj_celery_panel/celery_utils/periodic_tasks.py (3)
62-63: Consider narrowing the exception scope and using f-string conversion.While catching broad exceptions provides robustness, it can mask unexpected errors. Additionally, using
{e!s}is more idiomatic thanstr(e).♻️ Suggested improvement
- except Exception as e: - error = f"Error reading beat_schedule: {str(e)}" + except Exception as e: + error = f"Error reading beat_schedule: {e!s}"
109-120: Moveimport jsonoutside the loop.The
jsonimport is placed inside the loop, causing unnecessary repeated module lookups. Move it to the top of the method or the module.♻️ Suggested fix
+ import json + try: from django_celery_beat.models import PeriodicTask # Query all enabled periodic tasks for task in PeriodicTask.objects.filter(enabled=True).select_related( "interval", "crontab", "solar", "clocked" ): # Determine the schedule string based on which schedule type is set schedule_str = "N/A" if task.interval: schedule_str = str(task.interval) elif task.crontab: schedule_str = str(task.crontab) elif task.solar: schedule_str = str(task.solar) elif task.clocked: schedule_str = str(task.clocked) # Parse args and kwargs (stored as JSON strings) - import json - try: args = json.loads(task.args) if task.args else []
137-138: Consider using f-string conversion flag.Using
{e!s}is more idiomatic thanstr(e)in f-strings.♻️ Suggested fix
except Exception as e: - error = f"Error reading periodic tasks from database: {str(e)}" + error = f"Error reading periodic tasks from database: {e!s}"
| for task in PeriodicTask.objects.filter(enabled=True).select_related( | ||
| "interval", "crontab", "solar" | ||
| ): |
There was a problem hiding this comment.
Missing clocked in select_related causes N+1 query.
The code checks for task.clocked at Line 106, but clocked is not included in select_related. This will trigger an additional database query for each task that uses a clocked schedule.
🔧 Proposed fix
for task in PeriodicTask.objects.filter(enabled=True).select_related(
- "interval", "crontab", "solar"
+ "interval", "crontab", "solar", "clocked"
):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for task in PeriodicTask.objects.filter(enabled=True).select_related( | |
| "interval", "crontab", "solar" | |
| ): | |
| for task in PeriodicTask.objects.filter(enabled=True).select_related( | |
| "interval", "crontab", "solar", "clocked" | |
| ): |
🤖 Prompt for AI Agents
In `@dj_celery_panel/celery_utils/periodic_tasks.py` around lines 95 - 97, The
select_related call on
PeriodicTask.objects.filter(enabled=True).select_related("interval", "crontab",
"solar") omits "clocked", causing an N+1 query when the code later accesses
task.clocked; update the select_related invocation to include "clocked" so the
clocked relation is fetched in the same query (locate the select_related call in
periodic_tasks.py and add "clocked" to the tuple/list of relations).
| def setUp(self): | ||
| """Set up test fixtures.""" | ||
| super().setUp() | ||
| # Create a test Celery app with beat schedule | ||
| self.test_app = Celery("test_app") | ||
| self.test_app.conf.beat_schedule = { | ||
| "test-periodic-task": { | ||
| "task": "app.tasks.test_periodic", | ||
| "schedule": crontab(minute="*/5"), | ||
| "args": [1, 2], | ||
| "kwargs": {"key": "value"}, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check how the views retrieve the Celery app and periodic tasks
rg -n "CeleryPeriodicTasksInterface" --type py -A 3 dj_celery_panel/views.py
# Check the base test case to see if it sets up the Celery app
rg -n "class CeleryPanelTestCase" --type py -A 20 tests/base.pyRepository: yassi/dj-celery-panel
Length of output: 1041
🏁 Script executed:
# Check imports and context of current_app in views.py
rg -n "from.*current_app|import.*current_app" --type py dj_celery_panel/views.py
# Look at the full test file to see what tests are defined
head -80 tests/test_periodic_tasks_integration.py
# Check if there's any monkey patching or app configuration in the test file
rg -n "current_app|patch|mock" --type py tests/test_periodic_tasks_integration.pyRepository: yassi/dj-celery-panel
Length of output: 3305
🏁 Script executed:
# Check if test_app is referenced anywhere else in the test file
rg -n "test_app\|current_app" --type py tests/test_periodic_tasks_integration.py
# Check for any patching or Django app configuration that might connect them
rg -n "patch\|mock\|CELERY\|current_app" --type py tests/
# Check the full views.py context around the periodic tasks call
sed -n '1,40p' dj_celery_panel/views.pyRepository: yassi/dj-celery-panel
Length of output: 1373
Verify that test_app is connected to the view being tested.
The test_app Celery instance is created with a beat_schedule in setUp(), but it is never used by any of the test methods. The view uses the global current_app from the Celery package (line 30 in views.py), which is a completely separate instance. The tests only verify the presence of text strings like "Periodic Tasks" and "Backend:", not whether the periodic tasks from test_app are actually displayed. This means the tests may pass for the wrong reasons and don't validate that periodic tasks are properly integrated with the view.
🤖 Prompt for AI Agents
In `@tests/test_periodic_tasks_integration.py` around lines 16 - 28, The Celery
instance created in setUp (self.test_app) is never wired to the view's global
current_app; update setUp to replace the module-level Celery current_app used by
the view with self.test_app (and restore it in tearDown) so the view reads the
test beat_schedule; specifically, patch or assign celery.current_app (or the
view module's reference to current_app) to self.test_app inside setUp and revert
the change in tearDown so tests actually exercise the periodic tasks in the
view.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.