-
Notifications
You must be signed in to change notification settings - Fork 2
SLA Reminder #238
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
base: develop
Are you sure you want to change the base?
SLA Reminder #238
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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", {}) |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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' |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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." |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") | ||
| ) |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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> |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable timedeltavalue is not used.
This provides: