Skip to content

feat(incidents): Set channel topic on status channels #243

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.

Sign up for GitHub

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

Open
rgibert wants to merge 10 commits into
base: main
Choose a base branch
from
+66 −16
Open

feat(incidents): Set channel topic on status channels #243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f9cb002
feat(incidents): Set channel topic on status channels
rgibert Jun 5, 2026
85fe385
fix(incidents): Isolate status channel topic errors from other operat…
rgibert Jun 5, 2026
9a5b4ef
fix(incidents): Wrap status channel lookup in on_incident_updated wit…
rgibert Jun 5, 2026
160fffa
fix(incidents): Move build_channel_topic back inside try/except in on…
rgibert Jun 5, 2026
80614be
fix(incidents): Isolate main and status channel topic updates from ea…
rgibert Jun 5, 2026
1db27a5
fix(incidents): Isolate main channel topic update in on_captain_changed
rgibert Jun 5, 2026
51ac16b
reduce duplication
rgibert Jun 5, 2026
9e20d87
early exit when client on initialized
rgibert Jun 5, 2026
a641bf4
add missing try
rgibert Jun 5, 2026
503c765
fix ci
rgibert Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 48 additions & 6 deletions src/firetower/incidents/hooks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@
description: str | None = None
incident_url: str | None = None
incident_number: str | None = None
topic: str | None = None


def page_for_channel(
Expand Down Expand Up @@ -258,6 +259,24 @@
return _slack_service.parse_channel_id_from_url(slack_link.url)


def _set_topic_on_all_channels(incident: Incident, topic: str) -> None:
channel_ids: list[str] = []
try:
channel_id = _get_channel_id(incident)
if channel_id:
channel_ids.append(channel_id)
except Exception:
logger.exception(f"Failed to get channel id for incident {incident.id}")
try:
status_channel_id = _get_status_channel_id(incident)
if status_channel_id:
channel_ids.append(status_channel_id)
except Exception:
logger.exception(f"Failed to get status channel id for incident {incident.id}")
if channel_ids:

Check warning on line 276 in src/firetower/incidents/hooks.py

View check run for this annotation

@sentry/warden / warden: find-bugs

Unhandled exception from `set_all_channel_topics` can crash callers 

The call to `_slack_service.set_all_channel_topics` on line 276 has no exception handling, despite both channel ID lookups above being carefully guarded. `set_channel_topic` only catches `SlackApiError`; any other exception (e.g. network timeout, unexpected SDK error) propagates out of `_set_topic_on_all_channels` unguarded, and callers like `on_incident_updated` have no enclosing try/except at that call site either, causing the entire incident-update hook to fail silently.
_slack_service.set_all_channel_topics(channel_ids, topic)
Comment thread
sentry-warden[bot] marked this conversation as resolved.


def _invite_user_to_channel(
channel_id: str, user: User, slack_user_id: str | None = None
) -> None:
Expand Down Expand Up @@ -436,7 +455,8 @@


def _create_status_channel_for_context(
ctx: ChannelSetupContext, slack_service: SlackService
ctx: ChannelSetupContext,
slack_service: SlackService,
) -> str | None:
"""Create a companion status channel. No DB access.

Expand All @@ -463,6 +483,14 @@
)
return None

if ctx.topic:
try:
slack_service.set_channel_topic(status_channel_id, ctx.topic)
except Exception:
logger.exception(
f"Failed to set topic on status channel {status_channel_name}"
)

label = ctx.incident_number or ctx.channel_name
message_lines = [f"This is the status channel for *{label}*."]
if ctx.incident_url and ctx.incident_number:
Expand Down Expand Up @@ -525,6 +553,7 @@
reporter_slack_id=reporter_slack_id,
incident_url=_build_incident_url(incident),
incident_number=incident.incident_number,
topic=build_channel_topic(incident, captain_slack_id),
)
status_channel_id = _create_status_channel_for_context(ctx, _slack_service)
if status_channel_id:
Expand Down Expand Up @@ -1247,6 +1276,7 @@
description=incident.description,
incident_url=incident_url,
incident_number=incident.incident_number,
topic=build_channel_topic(incident, captain_slack_id),
)
status_channel_id = decorate_incident_channel(
ctx,
Expand Down Expand Up @@ -1313,7 +1343,13 @@
try:
channel_id = _get_channel_id(incident)
if channel_id:
Comment thread
rgibert marked this conversation as resolved.
_slack_service.set_channel_topic(channel_id, build_channel_topic(incident))
try:
topic = build_channel_topic(incident)
_set_topic_on_all_channels(incident, topic)
except Exception:
logger.exception(
f"Failed to set channel topic for incident {incident.id}"
)
incident_url = _build_incident_url(incident)
_slack_service.post_message(
channel_id,
Expand Down Expand Up @@ -1377,7 +1413,8 @@
try:
channel_id = _get_channel_id(incident)
if channel_id:
_slack_service.set_channel_topic(channel_id, build_channel_topic(incident))
topic = build_channel_topic(incident)
_set_topic_on_all_channels(incident, topic)
except Exception:
logger.exception(f"Error in on_title_changed for incident {incident.id}")

Expand Down Expand Up @@ -1408,7 +1445,8 @@
if not channel_id:
return

_slack_service.set_channel_topic(channel_id, build_channel_topic(incident))
topic = build_channel_topic(incident)
_set_topic_on_all_channels(incident, topic)

incident_url = _build_incident_url(incident)
if incident.captain:
Expand Down Expand Up @@ -1458,12 +1496,16 @@
old_title is not None or old_severity is not None or captain_changed
):
try:
_slack_service.set_channel_topic(channel_id, build_channel_topic(incident))
topic = build_channel_topic(incident)
except Exception:
topic = None
Comment thread
cursor[bot] marked this conversation as resolved.
logger.exception(
f"Error setting channel topic in on_incident_updated for incident {incident.id}"
f"Error building channel topic in on_incident_updated for incident {incident.id}"
)

if topic:
_set_topic_on_all_channels(incident, topic)

Check warning on line 1507 in src/firetower/incidents/hooks.py

View check run for this annotation

@sentry/warden / warden: find-bugs

[JU2-V7U] Unhandled exception from `set_all_channel_topics` can crash callers (additional location) 

The call to `_slack_service.set_all_channel_topics` on line 276 has no exception handling, despite both channel ID lookups above being carefully guarded. `set_channel_topic` only catches `SlackApiError`; any other exception (e.g. network timeout, unexpected SDK error) propagates out of `_set_topic_on_all_channels` unguarded, and callers like `on_incident_updated` have no enclosing try/except at that call site either, causing the entire incident-update hook to fail silently.
Comment thread
rgibert marked this conversation as resolved.
Comment thread
rgibert marked this conversation as resolved.

# --- Build combined notification lines ---
lines: list[str] = []

Expand Down
20 changes: 10 additions & 10 deletions src/firetower/incidents/tests/test_hooks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ def test_posts_status_update_message(self, mock_slack, mock_dump_async):
mock_slack.post_message.assert_called_once()
assert "Active" in mock_slack.post_message.call_args[0][1]
assert "Mitigated" in mock_slack.post_message.call_args[0][1]
mock_slack.set_channel_topic.assert_not_called()
mock_slack.set_all_channel_topics.assert_not_called()

@patch("firetower.incidents.hooks._slack_service")
def test_noop_without_slack_link(self, mock_slack):
Expand Down Expand Up @@ -509,7 +509,7 @@ def test_posts_severity_update_message(
mock_slack.post_message.assert_called_once()
assert "P2" in mock_slack.post_message.call_args[0][1]
assert "P0" in mock_slack.post_message.call_args[0][1]
mock_slack.set_channel_topic.assert_called_once()
mock_slack.set_all_channel_topics.assert_called_once()

@patch("firetower.incidents.hooks._slack_service")
def test_noop_without_slack_link(self, mock_slack):
Expand Down Expand Up @@ -541,7 +541,7 @@ def test_updates_topic(self, mock_slack):

on_title_changed(incident)

mock_slack.set_channel_topic.assert_called_once()
mock_slack.set_all_channel_topics.assert_called_once()

@patch("firetower.incidents.hooks._slack_service")
def test_noop_without_slack_link(self, mock_slack):
Expand All @@ -552,7 +552,7 @@ def test_noop_without_slack_link(self, mock_slack):

on_title_changed(incident)

mock_slack.set_channel_topic.assert_not_called()
mock_slack.set_all_channel_topics.assert_not_called()


@pytest.mark.django_db
Expand Down Expand Up @@ -642,7 +642,7 @@ def test_updates_topic_and_invites(self, mock_slack):

on_captain_changed(incident)

mock_slack.set_channel_topic.assert_called_once()
mock_slack.set_all_channel_topics.assert_called_once()
mock_slack.post_message.assert_called_once()
assert "<@U_NEW>" in mock_slack.post_message.call_args[0][1]
mock_slack.invite_to_channel.assert_called_once_with("C12345", ["U_NEW"])
Expand Down Expand Up @@ -671,7 +671,7 @@ def test_updates_topic_and_posts_name_when_no_slack_profile(self, mock_slack):

on_captain_changed(incident)

mock_slack.set_channel_topic.assert_called_once()
mock_slack.set_all_channel_topics.assert_called_once()
mock_slack.post_message.assert_called_once()
assert "New Captain" in mock_slack.post_message.call_args[0][1]

Expand All @@ -692,7 +692,7 @@ def test_updates_topic_only_when_captain_cleared(self, mock_slack):

on_captain_changed(incident)

mock_slack.set_channel_topic.assert_called_once()
mock_slack.set_all_channel_topics.assert_called_once()
mock_slack.post_message.assert_not_called()

@patch("firetower.incidents.hooks._slack_service")
Expand Down Expand Up @@ -780,7 +780,7 @@ def test_noop_without_slack_link(self, mock_slack):

on_captain_changed(incident)

mock_slack.set_channel_topic.assert_not_called()
mock_slack.set_all_channel_topics.assert_not_called()


MOCK_PD_CONFIG = {
Expand Down Expand Up @@ -3347,7 +3347,7 @@ def test_sets_topic_once_when_severity_and_captain_change(self, mock_slack):
incident, old_severity=IncidentSeverity.P4, captain_changed=True
)

mock_slack.set_channel_topic.assert_called_once()
mock_slack.set_all_channel_topics.assert_called_once()

@patch("firetower.incidents.hooks._slack_service")
def test_no_message_when_nothing_changed(self, mock_slack):
Expand Down Expand Up @@ -3397,7 +3397,7 @@ def test_no_captain_line_when_captain_cleared(self, mock_slack):

on_incident_updated(incident, captain_changed=True)

mock_slack.set_channel_topic.assert_called_once()
mock_slack.set_all_channel_topics.assert_called_once()
mock_slack.post_message.assert_not_called()

@patch("firetower.slack_app.handlers.dumpslack.trigger_slack_dump_async")
Expand Down
8 changes: 8 additions & 0 deletions src/firetower/integrations/services/slack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,14 @@ def set_channel_topic(self, channel_id: str, topic: str) -> bool:
)
return False

def set_all_channel_topics(
self, channel_ids: list[str], topic: str
) -> dict[str, bool]:
if not self.client:
logger.warning("Cannot set topics - Slack client not initialized")
return dict.fromkeys(channel_ids, False)
return {cid: self.set_channel_topic(cid, topic) for cid in channel_ids}

Comment thread
sentry-warden[bot] marked this conversation as resolved.
def invite_to_channel(self, channel_id: str, user_ids: list[str]) -> bool:
if not self.client:
logger.warning("Cannot invite to channel - Slack client not initialized")
Expand Down
Loading