Summoning heavenly logging to smite mismatches on priority assignment - #3046
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
Logging added in the cached-template code path dereferences relationships/hybrid properties that may not be safely available, and one new log is currently on an unreachable execution path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds additional runtime logging around template routing/priority decisions and slightly refactors choose_queue’s fallback logic to make “no priority queue provided” behavior more explicit, in support of investigating queue/priority mismatches.
Changes:
- Added info-level logs before queue selection / sending to capture template process-type and category-related routing inputs.
- Refactored
choose_queueto returnpriority_queueearly and use amatchfallback when it’s missing. - Added an additional log when
choose_queuefalls back due to a missingpriority_queue.
File summaries
| File | Description |
|---|---|
| app/v2/notifications/post_notifications.py | Adds a routing log (but currently placed on an unreachable path for simulated flow). |
| app/service/send_notification.py | Adds routing log before get_delivery_queue_for_template for one-off notifications. |
| app/notifications/rest.py | Adds routing log before sending to queue in the v1 notification endpoint. |
| app/notifications/process_notifications.py | Adds routing logs in persistence paths and refactors choose_queue fallback logic. |
Review details
Suppressed comments (1)
app/notifications/process_notifications.py:422
- Same issue as
persist_notification: this template is loaded withuse_cache=True, but the log message dereferencestemplate.template_categoryandtemplate.process_type. That can trigger detached/lazy-load errors (or at least extra DB work) and make the debug output misleading when the relationship isn’t available from cached JSON.
# TODO: Remove this logging statement once debugging is complete. It is useful for understanding how templates are routed to queues.
current_app.logger.info(
"persist_notifications: Routing template %s version %s: process_type_column=%r, "
"effective_process_type=%r, category_id=%s, category_email_process_type=%r, category_sms_process_type=%r",
template.id,
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| override_queue: str | ||
| match notification.notification_type: | ||
| case models.SMS_TYPE: | ||
| override_queue = QueueNames.SEND_SMS_MEDIUM | ||
| case models.EMAIL_TYPE: | ||
| override_queue = QueueNames.SEND_EMAIL_MEDIUM | ||
| case models.LETTER_TYPE: | ||
| override_queue = QueueNames.CREATE_LETTERS_PDF | ||
| case _: | ||
| raise ValueError(f"Could not determine queue for notification type {notification.notification_type!r}") | ||
|
|
||
| if override_queue is None: | ||
| raise ValueError(f"Could not determine queue for notification type {notification.notification_type!r}") | ||
| return override_queue | ||
| current_app.logger.info( | ||
| f"Notification {notification.id} had no priority queue; determined queue based on notification type and attributes to {override_queue}." | ||
| ) |
There was a problem hiding this comment.
That does not work, otherwise the variable in the match statement is considered like a capture variable for pattern matching.
| # TODO: Remove this logging statement once debugging is complete. It is useful for understanding how templates are routed to queues. | ||
| current_app.logger.info( | ||
| "process_sms_or_email_notification: Routing template %s version %s: process_type_column=%r, " | ||
| "effective_process_type=%r, category_id=%s, category_email_process_type=%r, category_sms_process_type=%r", | ||
| template.id, | ||
| template.version, | ||
| template.process_type_column, | ||
| template.process_type, | ||
| template.template_category_id, | ||
| template.template_category.email_process_type if template.template_category else None, | ||
| template.template_category.sms_process_type if template.template_category else None, | ||
| ) |
There was a problem hiding this comment.
Yes that seems right. I didn't add new logic here myself, but it wouldn't reach. I need a bit more clarity of mind and get back on this next week to know how to refactor, if we need to.
| current_app.logger.info( | ||
| f"Batch saving: {notification_type}/{template.process_type} {notification['id']} sent to buffer queue." | ||
| ) | ||
| else: |
There was a problem hiding this comment.
This would be a dead branch that isn't used. The first else could be activated, but not the inner condition if not simulated containing the business logic. Hence the whole parent condition is rendered useless.
P0NDER0SA
left a comment
P0NDER0SA
left a comment
There was a problem hiding this comment.
OK! let's get those logs and see how it looks :)
Summary | Résumé
get_delivery_queue_for_templatefunction calls, to clear out if the template content is proper.choose_queuewhen the process_type wasn't present and the logic make an override decision.choose_queuefunction a bit.Related Issues | Cartes liées
Test instructions | Instructions pour tester la modification
No regression with the unit-tests.
Release Instructions | Instructions pour le déploiement
None.
Reviewer checklist | Liste de vérification du réviseur