Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a cron job system for the Food4Kids platform with an initial job to process daily driver history. The system uses APScheduler to run scheduled tasks and follows a modular pattern similar to the existing router structure.
Key changes:
- Added APScheduler-based scheduler service with timezone support
- Created a jobs directory with initialization pattern for registering cron jobs
- Implemented a daily job to mark driver assignments as complete and update driver history
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/python/requirements.txt | Added apscheduler dependency |
| backend/python/app/services/jobs/driver_history_jobs.py | Implements daily job to process driver assignments and update history |
| backend/python/app/services/jobs/init.py | Central registration point for all scheduled jobs |
| backend/python/app/services/jobs/README.md | Documentation for creating and managing cron jobs |
| backend/python/app/services/implementations/scheduler_service.py | Core scheduler service wrapping APScheduler functionality |
| backend/python/app/dependencies/services.py | Adds scheduler service to dependency injection |
| backend/python/app/config.py | Adds scheduler timezone configuration |
| backend/python/app/init.py | Initializes scheduler on application startup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
|
|
||
| # Group by driver_id and sum distances | ||
| driver_distances: dict[UUID, float] = {} |
There was a problem hiding this comment.
The type hint imports UUID from typing.TYPE_CHECKING, but uses it at runtime in the type annotation. This will cause a NameError at runtime. Move the UUID import outside the TYPE_CHECKING block or use a string literal 'UUID' in the type hint.
| # Rollback if we have a session | ||
| if async_session_maker_instance: | ||
| try: | ||
| async with async_session_maker_instance() as session: | ||
| await session.rollback() | ||
| except Exception: | ||
| pass # Session might already be closed |
There was a problem hiding this comment.
The rollback logic creates a new session instead of rolling back the existing session from the try block (line 40). SQLAlchemy's async sessions automatically rollback on exception when exiting the context manager, making this redundant. Remove this rollback block as it's unnecessary and creates confusion.
| # Rollback if we have a session | |
| if async_session_maker_instance: | |
| try: | |
| async with async_session_maker_instance() as session: | |
| await session.rollback() | |
| except Exception: | |
| pass # Session might already be closed |
| This function follows the same pattern as app.routers.init_app(). | ||
| To add a new scheduled job: | ||
| 1. Create a new file in this directory (e.g., email_jobs.py) | ||
| 2. Define your job function | ||
| 3. Import and register it here |
There was a problem hiding this comment.
[nitpick] The function docstring references 'app.routers.init_app()' as a pattern, but doesn't explain what makes this pattern beneficial or how it should be followed. Consider adding a brief explanation of the registration pattern (import jobs locally, call add_cron_job with configuration).
| This function follows the same pattern as app.routers.init_app(). | |
| To add a new scheduled job: | |
| 1. Create a new file in this directory (e.g., email_jobs.py) | |
| 2. Define your job function | |
| 3. Import and register it here | |
| This function follows the same registration pattern as app.routers.init_app(): | |
| - Each job is defined in its own module within this directory. | |
| - To register a job, import it locally within this function and call `add_cron_job` with the appropriate configuration. | |
| This pattern ensures all scheduled jobs are registered in a single place, making them easy to manage and discover. | |
| To add a new scheduled job: | |
| 1. Create a new file in this directory (e.g., email_jobs.py) | |
| 2. Define your job function | |
| 3. Import and register it here by calling `add_cron_job` on the scheduler service |
| # Don't need to pass timezone to CronTrigger - it inherits from scheduler | ||
| trigger = CronTrigger( | ||
| hour=hour, | ||
| minute=minute, | ||
| day_of_week=day_of_week, | ||
| day=day, |
There was a problem hiding this comment.
The comment on line 86 states timezone doesn't need to be passed to CronTrigger because it inherits from the scheduler, but line 93 explicitly passes timezone=self.timezone. Either remove the timezone parameter or update the comment to explain why it's being passed despite inheritance.
| # Don't need to pass timezone to CronTrigger - it inherits from scheduler | |
| trigger = CronTrigger( | |
| hour=hour, | |
| minute=minute, | |
| day_of_week=day_of_week, | |
| day=day, | |
| # Explicitly pass timezone to CronTrigger to ensure the trigger uses the intended timezone, | |
| # even though the scheduler also has a timezone setting. | |
| trigger = CronTrigger( | |
| hour=hour, | |
| minute=minute, | |
| day_of_week=day_of_week, |
backend/python/app/__init__.py
Outdated
|
|
||
| yield | ||
|
|
||
| # Shutdown |
There was a problem hiding this comment.
[nitpick] The shutdown comment is now misleading as there is cleanup code (scheduler stop). Either remove the comment or expand it to describe what cleanup is performed.
| # Shutdown | |
| # Cleanup: stop the scheduler service during application shutdown |
ColinToft
left a comment
There was a problem hiding this comment.
Ok this is awesome, very clean and love the documentation for other people adding jobs in the future.
LGTM!!
JIRA ticket link
Create cron job system + mark as complete job
Implementation description
Createed a jobs folder under services, with contains an init.py.
Each new job should be added in this file.
The Readme.md under the jobs folder contains instructions for creating a new job.
Essentially, you need to write a function for your job's task, import it in init.py, and add it, just like the existing example.
Here, you can configure the time/frequency/other details.
There is also a scheduler service, under services/implementations. This scheduler service runs on startup, in config.py, and initializes all the cron jobs that have been added in init.py.
Steps to test
What should reviewers focus on?
Checklist