Skip to content

feat: Slack on-call mentions and incident participant notifications - #121

Open
NisargPipaliya wants to merge 1 commit into
roxy-wi:release/2.4.0from
NisargPipaliya:feat/slack-mentions-participant-notifications
Open

NisargPipaliya wants to merge 1 commit into
roxy-wi:release/2.4.0from
NisargPipaliya:feat/slack-mentions-participant-notifications

Conversation

@NisargPipaliya

Copy link
Copy Markdown

Summary

  • Slack channel config flags to @-mention the assignee and on-call responder
  • Email/SMTP participant notifications (new app/modules/smtp.py, app/services/incidents/participant_notifications.py)
  • Responder and stakeholder notification wiring
  • Channels UI + i18n for the new Slack mention options
  • Tests, docs, and dev mail/webhook tooling

#120

- Slack channel config flags to @-mention the assignee and on-call responder
- Email/SMTP participant notifications (new app/modules/smtp.py,
  app/services/incidents/participant_notifications.py)
- Responder and stakeholder notification wiring
- Channels UI + i18n for the new Slack mention options
- Tests, docs, and dev mail/webhook tooling
@NisargPipaliya

Copy link
Copy Markdown
Author

/sign-cla

@Aidaho12

Copy link
Copy Markdown
Member

Thanks for the contribution! The proposed functionality is useful, and I appreciate the effort put into the implementation, tests, documentation, and localization.

I reviewed the changes. The opt-in Slack mention flags (mention_assignee and mention_oncall) look like a good addition, especially with both disabled by default.

However, there are several issues we should address before merging.

Required changes

  1. Avoid automatic fallback from Slack DM to channel messages (P1)

    Currently, send_direct_message() falls back to posting in the configured Slack channel when Bot API DM delivery is unavailable.

    Responder requests may contain information intended for a specific recipient. Automatically publishing that information to a shared channel can expose it to unintended users.

    Please keep personal notifications private by default. Channel fallback should only be allowed through an explicit opt-in configuration.

  2. Move participant notifications out of synchronous request processing (P1)

    notify_participants() sends SMTP, Slack, and browser push notifications sequentially.

    For team or rotation responders, this can produce many external network requests during a single HTTP operation, causing long response times and potentially duplicate notifications when requests are retried.

    Participant notifications should use a persistent asynchronous delivery mechanism with retries and idempotency. Notification delivery failures must not affect responder or stakeholder creation.

  3. Respect channel notification filters (P2)

    resolve_slack_channel() selects an enabled Slack channel without checking its configured severity restrictions.

    Please ensure participant notification channel selection respects the existing notification configuration and filtering rules, including notify_on_severities.

  4. Escape dynamic Slack message content (P2)

    The new responder and stakeholder Slack messages interpolate dynamic content such as alert titles, display names, and responder messages without escaping Slack mrkdwn.

    This can allow unintended mentions such as <!channel> or <!here>.

    Please escape all untrusted text while preserving explicitly generated, validated user mentions.

  5. Add proper browser push handling for stakeholder_added (P2)

    The new notification uses event_type="stakeholder_added", but the existing browser push payload builder does not handle this event explicitly.

    As a result, users receive a generic alert notification instead of a message explaining that they were added as stakeholders.

    Please add a dedicated payload variant and regression tests.

Additional considerations

Incident Management v2 boundaries

IncidentRelay 2.3 introduced separate AlertGroup and first-class Incident entities.

The responder and stakeholder records affected by this PR are currently AlertGroup-scoped. Canonical first-class Incident collaboration is planned for 2.5.

Please make this distinction explicit in the documentation and avoid introducing new dependencies that mix technical AlertGroup notifications with future operational Incident notifications.

Delivery results

For multi-user responder targets, a notification should not appear fully successful merely because one recipient received it. Please consider tracking delivery outcomes per recipient, including partial failures.

Shared SMTP implementation

The new SMTP module is a useful refactoring. However, app/services/notifications/shift_notifications.py still implements SMTP delivery separately. It would be better to reuse the common transport there as well.

Testing

Please add or extend regression tests for the scenarios above, especially privacy-safe Slack delivery, notification retries and idempotency, severity filtering, message escaping, and stakeholder browser push formatting.

I also noticed that the Tests, Quality, and Docker/Helm workflows currently show action_required. These may need maintainer approval because the PR comes from a fork; they should be completed before merging.


Overall, I support the functionality proposed in #120. The Slack mention implementation is a good foundation, but the participant notification delivery needs additional hardening to meet IncidentRelay's production reliability and security requirements.

Requesting changes before merge.

Thanks again for the contribution!

@Aidaho12
Aidaho12 changed the base branch from main to release/2.4.0 October 12, 2026 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants