Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds an SLA reminder system for events and a generic task runner endpoint. The SLA reminder sends email notifications for events approaching their service-level agreement deadlines.
Key changes:
- New
event_sla_remindertask that emails the team about unattended events approaching SLA deadlines - New
TaskRunViewAPI endpoint to manually trigger tasks (synchronously or asynchronously) - Email template for displaying unattended events in reminder notifications
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| project/urls.py | Adds the new /api/task/run endpoint for manual task execution |
| ngen/views/tools.py | Implements TaskRunView for triggering tasks via API |
| ngen/templates/reports/event_sla_reminder.html | Email template displaying unattended events with SLA approaching |
| ngen/tasks.py | Implements event_sla_reminder task for periodic SLA monitoring |
| ngen/serializers/tools.py | Adds TaskRunSerializer for task execution request validation |
| ngen/models/announcement.py | Adds send_event_sla_reminder method to Communication class |
Comments suppressed due to low confidence (2)
ngen/tasks.py:578
- Variable timedeltavalue is not used.
timedeltavalue = timezone.timedelta(**timedelta_kwargs)
ngen/tasks.py:581
- Variable timedeltavalue is not used.
timedeltavalue = timezone.timedelta(minutes=default_value)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| task = None | ||
| if task_name in available_tasks: | ||
| print("Looking for task:", task_name) | ||
| task = getattr(models.tasks, task_name, None) |
There was a problem hiding this comment.
The tasks module is not accessible as an attribute of models. This will raise an AttributeError. Import tasks directly at the top of the file with from ngen import tasks and change line 407 to task = getattr(tasks, task_name, None).
| task_name = request.data.get("task_name") | ||
| async_run = request.data.get("async_run", True) | ||
| params = request.data.get("params", {}) |
There was a problem hiding this comment.
The view uses serializer_class but never validates the request data with it. Add serializer validation before processing: serializer = self.serializer_class(data=request.data), serializer.is_valid(raise_exception=True), then use serializer.validated_data instead of accessing request.data directly.
| task_name = request.data.get("task_name") | |
| async_run = request.data.get("async_run", True) | |
| params = request.data.get("params", {}) | |
| serializer = self.serializer_class(data=request.data) | |
| serializer.is_valid(raise_exception=True) | |
| task_name = serializer.validated_data.get("task_name") | |
| async_run = serializer.validated_data.get("async_run", True) | |
| params = serializer.validated_data.get("params", {}) |
| # get function form the module | ||
| task = None | ||
| if task_name in available_tasks: | ||
| print("Looking for task:", task_name) |
There was a problem hiding this comment.
Debug print statement should be removed or replaced with proper logging using logger.debug() or logger.info().
| "send_contact_check_submitted", | ||
| "event_sla_reminder", | ||
| ] | ||
| # get function form the module |
There was a problem hiding this comment.
Corrected spelling of 'form' to 'from'.
| # get function form the module | |
| # get function from the module |
| interval = task.interval | ||
| period = ( | ||
| interval.period | ||
| ) # 'seconds', 'minutes', 'hours', 'minutes', 'weeks' |
There was a problem hiding this comment.
Duplicate 'minutes' in comment should be 'days'. The comment should read: 'seconds', 'minutes', 'hours', 'days', 'weeks'.
| ) # 'seconds', 'minutes', 'hours', 'minutes', 'weeks' | |
| ) # 'seconds', 'minutes', 'hours', 'days', 'weeks' |
| default_value = 14 | ||
| timedeltavalue = timezone.timedelta(minutes=default_value) | ||
| logger.warning( | ||
| f"No interval found for the periodic task 'ngen.tasks.event_sla_reminder'. Using default value: {default_value} days." |
There was a problem hiding this comment.
The warning message says 'days' but the timedelta is created with minutes=default_value. Change line 581 to timezone.timedelta(days=default_value) or update the warning message to say 'minutes'.
| f"No interval found for the periodic task 'ngen.tasks.event_sla_reminder'. Using default value: {default_value} days." | |
| f"No interval found for the periodic task 'ngen.tasks.event_sla_reminder'. Using default value: {default_value} minutes." |
| events = ( | ||
| ngen.models.Event.objects.filter( | ||
| case=None | ||
| # date__gt=timezone.now() - (F("priority__attend_time") - timedeltavalue), | ||
| # date__lte=timezone.now() - F("priority__attend_time"), | ||
| ) | ||
| .exclude(tags__slug="no_sla") | ||
| .order_by("priority__severity") | ||
| ) |
There was a problem hiding this comment.
The critical SLA filtering logic is commented out. This means the query returns ALL unattended events (case=None) regardless of their SLA status. The timedeltavalue parameter is calculated but never used. Either uncomment and fix lines 589-590, or remove the unused parameter calculation if this is intentional.
|
|
||
| {% block content_body %} | ||
| <div style="font-family: Arial, sans-serif; max-width: 600px; margin: 0 auto; padding: 20px; border: 1px solid #ddd; border-radius: 8px;"> | ||
| <h5>{% translate 'Unnatended Events' %}: {{ events|length }}</h5> |
There was a problem hiding this comment.
Corrected spelling of 'Unnatended' to 'Unattended'.
| <h5>{% translate 'Unnatended Events' %}: {{ events|length }}</h5> | |
| <h5>{% translate 'Unattended Events' %}: {{ events|length }}</h5> |
|
|
||
| class TaskRunSerializer(serializers.Serializer): | ||
| task_name = serializers.CharField(max_length=255) | ||
| params = serializers.DictField(child=serializers.CharField(), required=False) |
There was a problem hiding this comment.
The params field only accepts dictionary values that are strings due to child=serializers.CharField(). This is too restrictive for task parameters which may need integers, booleans, lists, or nested objects. Consider using params = serializers.JSONField(required=False) or params = serializers.DictField(required=False) without the child constraint to allow any JSON-serializable values.
| params = serializers.DictField(child=serializers.CharField(), required=False) | |
| params = serializers.JSONField(required=False) |
| minutes = int(minutes) | ||
| except ValueError: | ||
| raise TaskFailure("Minutes parameter must be an integer.") | ||
| timedeltavalue = timezone.timedelta(minutes=minutes) |
There was a problem hiding this comment.
Variable timedeltavalue is not used.
This provides: