From 91dae10299957ea203067a092da5f23416c06e51 Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Fri, 29 May 2026 16:29:19 -0400 Subject: [PATCH 01/10] Add inline affected service and region tag creation from Slack lifecycle modals --- .../slack_app/handlers/new_incident.py | 23 +++- src/firetower/slack_app/handlers/utils.py | 48 +++++++- .../tests/handlers/test_new_incident.py | 106 ++++++++++++++++++ 3 files changed, 174 insertions(+), 3 deletions(-) diff --git a/src/firetower/slack_app/handlers/new_incident.py b/src/firetower/slack_app/handlers/new_incident.py index 0b65bc99..1f7a5131 100644 --- a/src/firetower/slack_app/handlers/new_incident.py +++ b/src/firetower/slack_app/handlers/new_incident.py @@ -17,6 +17,7 @@ from firetower.integrations.services.slack import escape_slack_text from firetower.slack_app.handlers.utils import ( _DEFAULT_SEVERITY, + CREATE_TAG_PREFIX, build_incident_form_blocks, parse_incident_form_values, ) @@ -170,6 +171,10 @@ def _build_new_incident_modal(channel_id: str = "", user_id: str = "") -> dict: "affected_region_tags": TagType.AFFECTED_REGION, } +# Tag types users can create inline from the lifecycle modals. impact_type stays +# curated since it powers dashboards, so it is intentionally excluded. +INLINE_CREATABLE_TAG_TYPES = {TagType.AFFECTED_SERVICE, TagType.AFFECTED_REGION} + def handle_tag_options(ack: Any, payload: dict) -> None: close_old_connections() @@ -185,10 +190,26 @@ def handle_tag_options(ack: Any, payload: dict) -> None: if keyword: qs = qs.filter(name__icontains=keyword) + matches = list(qs.order_by("name")[:100]) options = [ {"text": {"type": "plain_text", "text": tag.name}, "value": tag.name} - for tag in qs.order_by("name")[:100] + for tag in matches ] + + stripped = keyword.strip() + if tag_type in INLINE_CREATABLE_TAG_TYPES and stripped: + exact_match = any(tag.name.lower() == stripped.lower() for tag in matches) + if not exact_match: + options.append( + { + "text": { + "type": "plain_text", + "text": f'+ Create "{stripped}"', + }, + "value": f"{CREATE_TAG_PREFIX}{stripped}", + } + ) + ack(options=options) diff --git a/src/firetower/slack_app/handlers/utils.py b/src/firetower/slack_app/handlers/utils.py index 16f9a2c4..ad73c76c 100644 --- a/src/firetower/slack_app/handlers/utils.py +++ b/src/firetower/slack_app/handlers/utils.py @@ -1,3 +1,4 @@ +import logging from typing import Any from django.conf import settings @@ -10,10 +11,47 @@ IncidentSeverity, IncidentStatus, ServiceTier, + Tag, + TagType, ) +logger = logging.getLogger(__name__) + _DEFAULT_SEVERITY = IncidentSeverity.P3 +CREATE_TAG_PREFIX = "__create__:" + + +def _resolve_tag_values(values: list[str], tag_type: TagType) -> list[str]: + """Resolve selected tag values, creating any prefixed with CREATE_TAG_PREFIX. + + Values prefixed with ``__create__:`` are stripped and get_or_create'd as Tags + of the given type; everything else passes through unchanged. The returned list + preserves order and is de-duplicated case-insensitively. + """ + resolved: list[str] = [] + seen: set[str] = set() + for value in values: + name = value + if value.startswith(CREATE_TAG_PREFIX): + name = value[len(CREATE_TAG_PREFIX) :].strip() + if not name: + continue + tag, created = Tag.objects.get_or_create( + name__iexact=name, + type=tag_type, + defaults={"name": name, "type": tag_type}, + ) + if created: + logger.info("Created %s tag %r from Slack modal", tag_type, tag.name) + name = tag.name + key = name.lower() + if key in seen: + continue + seen.add(key) + resolved.append(name) + return resolved + def get_incident_from_channel(channel_id: str) -> Incident | None: link = ( @@ -150,7 +188,10 @@ def parse_incident_form_values(view: dict) -> dict[str, Any]: .get("selected_options") or [] ) - affected_service_tags = [opt["value"] for opt in affected_service_selections] + affected_service_tags = _resolve_tag_values( + [opt["value"] for opt in affected_service_selections], + TagType.AFFECTED_SERVICE, + ) affected_region_selections = ( values.get("affected_region_block", {}) @@ -158,7 +199,10 @@ def parse_incident_form_values(view: dict) -> dict[str, Any]: .get("selected_options") or [] ) - affected_region_tags = [opt["value"] for opt in affected_region_selections] + affected_region_tags = _resolve_tag_values( + [opt["value"] for opt in affected_region_selections], + TagType.AFFECTED_REGION, + ) captain_slack_id = ( values.get("captain_block", {}).get("captain_select", {}).get("selected_user") diff --git a/src/firetower/slack_app/tests/handlers/test_new_incident.py b/src/firetower/slack_app/tests/handlers/test_new_incident.py index 371601e7..bb36bc9b 100644 --- a/src/firetower/slack_app/tests/handlers/test_new_incident.py +++ b/src/firetower/slack_app/tests/handlers/test_new_incident.py @@ -78,6 +78,20 @@ def test_new_modal_has_minimal_blocks(self, mock_get_bolt_app): "private_block", } + @patch("firetower.slack_app.bolt.get_bolt_app") + def test_modal_omits_incident_page_hint(self, mock_get_bolt_app): + ack = MagicMock() + body = {"trigger_id": "T12345"} + command = {"text": "new"} + respond = MagicMock() + + handle_new_command(ack, body, command, respond) + + view = mock_get_bolt_app.return_value.client.views_open.call_args[1]["view"] + rendered = str(view["blocks"]) + assert "incident page" not in rendered + assert "Missing a service or region" not in rendered + @pytest.mark.django_db class TestNewIncidentSubmission: @@ -126,6 +140,53 @@ def test_creates_incident(self, mock_get_user, mock_hook): assert incident.reporter == self.user client.chat_postMessage.assert_called_once() + @patch("firetower.incidents.serializers.on_incident_created") + @patch("firetower.slack_app.handlers.new_incident.get_or_create_user_from_slack_id") + def test_creates_tag_inline_from_submission(self, mock_get_user, mock_hook): + mock_get_user.return_value = self.user + + ack = MagicMock() + client = MagicMock() + body = {"user": {"id": "U_TEST"}} + view = { + "state": { + "values": { + "title_block": {"title": {"value": "Test Incident"}}, + "severity_block": { + "severity": {"selected_option": {"value": "P1"}} + }, + "description_block": {"description": {"value": "Description"}}, + "affected_service_block": { + "affected_service_tags": { + "selected_options": [ + {"value": "__create__:payments"}, + ] + } + }, + "affected_region_block": { + "affected_region_tags": { + "selected_options": [ + {"value": "__create__:ap-south-1"}, + ] + } + }, + "private_block": {"is_private": {"selected_options": []}}, + } + } + } + + handle_new_incident_submission(ack, body, view, client) + + ack.assert_called_once_with() + service_tag = Tag.objects.get(name="payments", type=TagType.AFFECTED_SERVICE) + assert service_tag.type == TagType.AFFECTED_SERVICE + region_tag = Tag.objects.get(name="ap-south-1", type=TagType.AFFECTED_REGION) + assert region_tag.type == TagType.AFFECTED_REGION + + incident = Incident.objects.get(title="Test Incident") + assert "payments" in incident.affected_service_tag_names + assert "ap-south-1" in incident.affected_region_tag_names + @patch("firetower.slack_app.handlers.new_incident._slack_service") @patch("firetower.incidents.serializers.on_incident_created") @patch("firetower.slack_app.handlers.new_incident.get_or_create_user_from_slack_id") @@ -434,6 +495,51 @@ def test_filters_by_tag_type(self, _mock_close): assert len(options) == 1 assert options[0]["text"]["text"] == "us-east-1" + def test_offers_create_option_for_service(self, _mock_close): + ack = MagicMock() + payload = {"action_id": "affected_service_tags", "value": "payments"} + handle_tag_options(ack, payload) + + options = ack.call_args[1]["options"] + assert options[-1] == { + "text": {"type": "plain_text", "text": '+ Create "payments"'}, + "value": "__create__:payments", + } + + def test_offers_create_option_for_region(self, _mock_close): + ack = MagicMock() + payload = {"action_id": "affected_region_tags", "value": "ap-south-1"} + handle_tag_options(ack, payload) + + options = ack.call_args[1]["options"] + assert options[-1]["value"] == "__create__:ap-south-1" + + def test_no_create_option_for_impact_type(self, _mock_close): + ack = MagicMock() + payload = {"action_id": "impact_type_tags", "value": "Brand New"} + handle_tag_options(ack, payload) + + options = ack.call_args[1]["options"] + assert all(not o["value"].startswith("__create__:") for o in options) + + def test_no_create_option_for_empty_keyword(self, _mock_close): + ack = MagicMock() + payload = {"action_id": "affected_service_tags", "value": " "} + handle_tag_options(ack, payload) + + options = ack.call_args[1]["options"] + assert all(not o["value"].startswith("__create__:") for o in options) + + def test_no_create_option_for_exact_match(self, _mock_close): + Tag.objects.create(name="API", type=TagType.AFFECTED_SERVICE) + + ack = MagicMock() + payload = {"action_id": "affected_service_tags", "value": "api"} + handle_tag_options(ack, payload) + + options = ack.call_args[1]["options"] + assert all(not o["value"].startswith("__create__:") for o in options) + class TestFallbackChannel: def _base_form_data(self): From 245df92f771a04ebac523c0a8f4ae2365722cd14 Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Fri, 29 May 2026 16:34:07 -0400 Subject: [PATCH 02/10] Update matching-tags test to expect inline create option --- src/firetower/slack_app/tests/handlers/test_new_incident.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/firetower/slack_app/tests/handlers/test_new_incident.py b/src/firetower/slack_app/tests/handlers/test_new_incident.py index bb36bc9b..714435e2 100644 --- a/src/firetower/slack_app/tests/handlers/test_new_incident.py +++ b/src/firetower/slack_app/tests/handlers/test_new_incident.py @@ -461,9 +461,10 @@ def test_returns_matching_tags(self, _mock_close): ack.assert_called_once() options = ack.call_args[1]["options"] - assert len(options) == 2 + assert len(options) == 3 names = {o["text"]["text"] for o in options} - assert names == {"us-east-1", "us-west-2"} + assert names == {"us-east-1", "us-west-2", '+ Create "us"'} + assert options[-1]["value"] == "__create__:us" def test_empty_query_returns_all(self, _mock_close): Tag.objects.create(name="us-east-1", type=TagType.AFFECTED_REGION) From c1a8065bc729d73e2a4c18eb4a94d67395804133 Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Fri, 5 Jun 2026 14:52:27 -0400 Subject: [PATCH 03/10] Handle DB-down gracefully when creating inline tags during incident submission --- .../slack_app/handlers/new_incident.py | 7 +++-- src/firetower/slack_app/handlers/utils.py | 31 ++++++++++++------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/src/firetower/slack_app/handlers/new_incident.py b/src/firetower/slack_app/handlers/new_incident.py index 1f7a5131..7e38f9cf 100644 --- a/src/firetower/slack_app/handlers/new_incident.py +++ b/src/firetower/slack_app/handlers/new_incident.py @@ -280,8 +280,6 @@ def _create_incident_via_db( def handle_new_incident_submission( ack: Any, body: dict, view: dict, client: Any ) -> None: - form = parse_incident_form_values(view) - values = view.get("state", {}).get("values", {}) private_selections = ( values.get("private_block", {}).get("is_private", {}).get("selected_options") @@ -289,7 +287,8 @@ def handle_new_incident_submission( ) is_private = any(opt.get("value") == "private" for opt in private_selections) - if not form["title"]: + title = values.get("title_block", {}).get("title", {}).get("value", "").strip() + if not title: ack( response_action="errors", errors={"title_block": "This field is required."}, @@ -301,11 +300,13 @@ def handle_new_incident_submission( slack_user_id = body.get("user", {}).get("id", "") try: + form = parse_incident_form_values(view) incident = _create_incident_via_db(form, slack_user_id, is_private, client) except (OperationalError, InterfaceError): logger.exception( "Database unreachable during incident creation from Slack modal" ) + form = parse_incident_form_values(view, resolve_tags=False) form_data = { "title": form["title"], "severity": form["severity"] or _DEFAULT_SEVERITY.value, diff --git a/src/firetower/slack_app/handlers/utils.py b/src/firetower/slack_app/handlers/utils.py index ad73c76c..b829cf97 100644 --- a/src/firetower/slack_app/handlers/utils.py +++ b/src/firetower/slack_app/handlers/utils.py @@ -22,12 +22,16 @@ CREATE_TAG_PREFIX = "__create__:" -def _resolve_tag_values(values: list[str], tag_type: TagType) -> list[str]: +def _resolve_tag_values( + values: list[str], tag_type: TagType, resolve_tags: bool = True +) -> list[str]: """Resolve selected tag values, creating any prefixed with CREATE_TAG_PREFIX. Values prefixed with ``__create__:`` are stripped and get_or_create'd as Tags of the given type; everything else passes through unchanged. The returned list - preserves order and is de-duplicated case-insensitively. + preserves order and is de-duplicated case-insensitively. When ``resolve_tags`` + is False, prefixed values are stripped to their raw name without touching the + database (used for degraded-mode fallbacks when the DB is unreachable). """ resolved: list[str] = [] seen: set[str] = set() @@ -37,14 +41,17 @@ def _resolve_tag_values(values: list[str], tag_type: TagType) -> list[str]: name = value[len(CREATE_TAG_PREFIX) :].strip() if not name: continue - tag, created = Tag.objects.get_or_create( - name__iexact=name, - type=tag_type, - defaults={"name": name, "type": tag_type}, - ) - if created: - logger.info("Created %s tag %r from Slack modal", tag_type, tag.name) - name = tag.name + if resolve_tags: + tag, created = Tag.objects.get_or_create( + name__iexact=name, + type=tag_type, + defaults={"name": name, "type": tag_type}, + ) + if created: + logger.info( + "Created %s tag %r from Slack modal", tag_type, tag.name + ) + name = tag.name key = name.lower() if key in seen: continue @@ -151,7 +158,7 @@ def build_incident_form_blocks(user_id: str = "") -> list[dict[str, Any]]: ] -def parse_incident_form_values(view: dict) -> dict[str, Any]: +def parse_incident_form_values(view: dict, resolve_tags: bool = True) -> dict[str, Any]: values = view.get("state", {}).get("values", {}) title = values.get("title_block", {}).get("title", {}).get("value", "").strip() @@ -191,6 +198,7 @@ def parse_incident_form_values(view: dict) -> dict[str, Any]: affected_service_tags = _resolve_tag_values( [opt["value"] for opt in affected_service_selections], TagType.AFFECTED_SERVICE, + resolve_tags=resolve_tags, ) affected_region_selections = ( @@ -202,6 +210,7 @@ def parse_incident_form_values(view: dict) -> dict[str, Any]: affected_region_tags = _resolve_tag_values( [opt["value"] for opt in affected_region_selections], TagType.AFFECTED_REGION, + resolve_tags=resolve_tags, ) captain_slack_id = ( From fc835278cf687759ab24742b3bf6958b71dda6b8 Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Fri, 5 Jun 2026 15:19:01 -0400 Subject: [PATCH 04/10] Harden inline tag creation against tag uniqueness races and add tests --- .../slack_app/handlers/new_incident.py | 5 +- src/firetower/slack_app/handlers/utils.py | 38 ++++++--- .../tests/handlers/test_new_incident.py | 81 ++++++++++++++++++- 3 files changed, 110 insertions(+), 14 deletions(-) diff --git a/src/firetower/slack_app/handlers/new_incident.py b/src/firetower/slack_app/handlers/new_incident.py index 7e38f9cf..68620454 100644 --- a/src/firetower/slack_app/handlers/new_incident.py +++ b/src/firetower/slack_app/handlers/new_incident.py @@ -18,6 +18,7 @@ from firetower.slack_app.handlers.utils import ( _DEFAULT_SEVERITY, CREATE_TAG_PREFIX, + _extract_title, build_incident_form_blocks, parse_incident_form_values, ) @@ -198,7 +199,7 @@ def handle_tag_options(ack: Any, payload: dict) -> None: stripped = keyword.strip() if tag_type in INLINE_CREATABLE_TAG_TYPES and stripped: - exact_match = any(tag.name.lower() == stripped.lower() for tag in matches) + exact_match = Tag.objects.filter(type=tag_type, name__iexact=stripped).exists() if not exact_match: options.append( { @@ -287,7 +288,7 @@ def handle_new_incident_submission( ) is_private = any(opt.get("value") == "private" for opt in private_selections) - title = values.get("title_block", {}).get("title", {}).get("value", "").strip() + title = _extract_title(view) if not title: ack( response_action="errors", diff --git a/src/firetower/slack_app/handlers/utils.py b/src/firetower/slack_app/handlers/utils.py index b829cf97..1a8ab1ce 100644 --- a/src/firetower/slack_app/handlers/utils.py +++ b/src/firetower/slack_app/handlers/utils.py @@ -2,6 +2,8 @@ from typing import Any from django.conf import settings +from django.core.exceptions import ValidationError +from django.db import IntegrityError from firetower.auth.models import ExternalProfileType from firetower.incidents.models import ( @@ -42,16 +44,25 @@ def _resolve_tag_values( if not name: continue if resolve_tags: - tag, created = Tag.objects.get_or_create( - name__iexact=name, - type=tag_type, - defaults={"name": name, "type": tag_type}, - ) - if created: - logger.info( - "Created %s tag %r from Slack modal", tag_type, tag.name - ) - name = tag.name + existing = Tag.objects.filter(type=tag_type, name__iexact=name).first() + if existing: + name = existing.name + else: + try: + # Inline-created tags are intentionally left + # approved=False so they surface for admin audit. + tag = Tag.objects.create(name=name, type=tag_type) + logger.info( + "Created %s tag %r from Slack modal", tag_type, tag.name + ) + name = tag.name + except (ValidationError, IntegrityError): + existing = Tag.objects.filter( + type=tag_type, name__iexact=name + ).first() + if existing is None: + raise + name = existing.name key = name.lower() if key in seen: continue @@ -158,10 +169,15 @@ def build_incident_form_blocks(user_id: str = "") -> list[dict[str, Any]]: ] +def _extract_title(view: dict) -> str: + values = view.get("state", {}).get("values", {}) + return values.get("title_block", {}).get("title", {}).get("value", "").strip() + + def parse_incident_form_values(view: dict, resolve_tags: bool = True) -> dict[str, Any]: values = view.get("state", {}).get("values", {}) - title = values.get("title_block", {}).get("title", {}).get("value", "").strip() + title = _extract_title(view) severity_block = values.get("severity_block", {}) selected_option = severity_block.get("severity", {}).get( "selected_option" diff --git a/src/firetower/slack_app/tests/handlers/test_new_incident.py b/src/firetower/slack_app/tests/handlers/test_new_incident.py index 714435e2..f3f43355 100644 --- a/src/firetower/slack_app/tests/handlers/test_new_incident.py +++ b/src/firetower/slack_app/tests/handlers/test_new_incident.py @@ -3,7 +3,7 @@ import pytest from django.conf import settings from django.contrib.auth.models import User -from django.db import OperationalError +from django.db import IntegrityError, OperationalError from firetower.auth.models import ExternalProfile, ExternalProfileType from firetower.incidents.models import Incident, IncidentSeverity, Tag, TagType @@ -14,6 +14,7 @@ handle_new_incident_submission, handle_tag_options, ) +from firetower.slack_app.handlers.utils import _resolve_tag_values @pytest.mark.django_db @@ -737,3 +738,81 @@ def test_non_private_posts_to_feed_channel(self, mock_slack_svc, settings): assert len(feed_calls) == 1 assert "degraded mode" in feed_calls[0][0][1] assert "DB is on fire" in feed_calls[0][0][1] + + +@pytest.mark.django_db +class TestResolveTagValues: + def test_collapses_existing_and_create_of_same_name(self): + Tag.objects.create(name="Payments", type=TagType.AFFECTED_SERVICE) + + resolved = _resolve_tag_values( + ["Payments", "__create__:payments"], + TagType.AFFECTED_SERVICE, + ) + + assert resolved == ["Payments"] + + def test_collapses_create_values_differing_only_in_case(self): + resolved = _resolve_tag_values( + ["__create__:Payments", "__create__:payments"], + TagType.AFFECTED_SERVICE, + ) + + assert resolved == ["Payments"] + assert ( + Tag.objects.filter( + type=TagType.AFFECTED_SERVICE, name__iexact="payments" + ).count() + == 1 + ) + + def test_preserves_order(self): + resolved = _resolve_tag_values( + ["__create__:beta", "__create__:alpha"], + TagType.AFFECTED_SERVICE, + ) + + assert resolved == ["beta", "alpha"] + + def test_resolve_false_strips_prefix_without_db(self): + resolved = _resolve_tag_values( + ["__create__:payments", "us-east-1", "__create__: "], + TagType.AFFECTED_SERVICE, + resolve_tags=False, + ) + + assert resolved == ["payments", "us-east-1"] + assert not Tag.objects.exists() + + def test_skips_empty_after_strip(self): + resolved = _resolve_tag_values( + ["__create__: ", "__create__:payments"], + TagType.AFFECTED_SERVICE, + ) + + assert resolved == ["payments"] + + def test_recovers_existing_tag_when_create_races(self): + Tag.objects.create(name="Payments", type=TagType.AFFECTED_SERVICE) + + with patch( + "firetower.slack_app.handlers.utils.Tag.objects.create", + side_effect=IntegrityError("duplicate key"), + ): + resolved = _resolve_tag_values( + ["__create__:payments"], + TagType.AFFECTED_SERVICE, + ) + + assert resolved == ["Payments"] + + def test_reraises_when_create_fails_and_no_existing_tag(self): + with patch( + "firetower.slack_app.handlers.utils.Tag.objects.create", + side_effect=IntegrityError("boom"), + ): + with pytest.raises(IntegrityError): + _resolve_tag_values( + ["__create__:payments"], + TagType.AFFECTED_SERVICE, + ) From 0c4d317ca8dc220b7a5dc8e4af0e7cc9c1ec20fb Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Fri, 5 Jun 2026 15:32:12 -0400 Subject: [PATCH 05/10] Move inline tag creation submission test to the resolved flow --- .../tests/handlers/test_new_incident.py | 47 ------------------- .../slack_app/tests/handlers/test_resolved.py | 32 +++++++++++++ 2 files changed, 32 insertions(+), 47 deletions(-) diff --git a/src/firetower/slack_app/tests/handlers/test_new_incident.py b/src/firetower/slack_app/tests/handlers/test_new_incident.py index f3f43355..1efcc82c 100644 --- a/src/firetower/slack_app/tests/handlers/test_new_incident.py +++ b/src/firetower/slack_app/tests/handlers/test_new_incident.py @@ -141,53 +141,6 @@ def test_creates_incident(self, mock_get_user, mock_hook): assert incident.reporter == self.user client.chat_postMessage.assert_called_once() - @patch("firetower.incidents.serializers.on_incident_created") - @patch("firetower.slack_app.handlers.new_incident.get_or_create_user_from_slack_id") - def test_creates_tag_inline_from_submission(self, mock_get_user, mock_hook): - mock_get_user.return_value = self.user - - ack = MagicMock() - client = MagicMock() - body = {"user": {"id": "U_TEST"}} - view = { - "state": { - "values": { - "title_block": {"title": {"value": "Test Incident"}}, - "severity_block": { - "severity": {"selected_option": {"value": "P1"}} - }, - "description_block": {"description": {"value": "Description"}}, - "affected_service_block": { - "affected_service_tags": { - "selected_options": [ - {"value": "__create__:payments"}, - ] - } - }, - "affected_region_block": { - "affected_region_tags": { - "selected_options": [ - {"value": "__create__:ap-south-1"}, - ] - } - }, - "private_block": {"is_private": {"selected_options": []}}, - } - } - } - - handle_new_incident_submission(ack, body, view, client) - - ack.assert_called_once_with() - service_tag = Tag.objects.get(name="payments", type=TagType.AFFECTED_SERVICE) - assert service_tag.type == TagType.AFFECTED_SERVICE - region_tag = Tag.objects.get(name="ap-south-1", type=TagType.AFFECTED_REGION) - assert region_tag.type == TagType.AFFECTED_REGION - - incident = Incident.objects.get(title="Test Incident") - assert "payments" in incident.affected_service_tag_names - assert "ap-south-1" in incident.affected_region_tag_names - @patch("firetower.slack_app.handlers.new_incident._slack_service") @patch("firetower.incidents.serializers.on_incident_created") @patch("firetower.slack_app.handlers.new_incident.get_or_create_user_from_slack_id") diff --git a/src/firetower/slack_app/tests/handlers/test_resolved.py b/src/firetower/slack_app/tests/handlers/test_resolved.py index 6aa45da0..88407cd0 100644 --- a/src/firetower/slack_app/tests/handlers/test_resolved.py +++ b/src/firetower/slack_app/tests/handlers/test_resolved.py @@ -381,6 +381,38 @@ def test_missing_affected_region_returns_error(self, incident): assert call_kwargs["response_action"] == "errors" assert "affected_region_block" in call_kwargs["errors"] + @patch("firetower.incidents.serializers.on_incident_updated") + @patch("firetower.slack_app.handlers.resolved.get_or_create_user_from_slack_id") + def test_creates_tag_inline_from_resolved_submission( + self, + mock_get_user, + mock_hook, + user, + incident, + impact_type_tag, + ): + mock_get_user.return_value = user + ack = MagicMock() + client = MagicMock() + body = {"user": {"id": "U_CAPTAIN"}} + view = _make_resolved_view( + severity="P1", + affected_service_tags=("__create__:payments",), + affected_region_tags=("__create__:ap-south-1",), + ) + + handle_resolved_submission(ack, body, view, client) + + ack.assert_called_once_with() + service_tag = Tag.objects.get(name="payments", type=TagType.AFFECTED_SERVICE) + assert service_tag.type == TagType.AFFECTED_SERVICE + region_tag = Tag.objects.get(name="ap-south-1", type=TagType.AFFECTED_REGION) + assert region_tag.type == TagType.AFFECTED_REGION + + incident.refresh_from_db() + assert "payments" in incident.affected_service_tag_names + assert "ap-south-1" in incident.affected_region_tag_names + @patch("firetower.slack_app.handlers.resolved.get_or_create_user_from_slack_id") def test_captain_resolution_failure(self, mock_get_user, incident): mock_get_user.return_value = None From 3ea47aca9260a958fe29a03358e38b6b6f033712 Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Thu, 11 Jun 2026 15:38:50 -0400 Subject: [PATCH 06/10] Defer inline tag creation past ack and validation and enforce case-insensitive tag uniqueness at the database level --- ...0023_alter_tag_unique_together_and_more.py | 25 +++++++++++++++++++ src/firetower/incidents/models.py | 9 ++++++- src/firetower/slack_app/handlers/mitigated.py | 7 +++++- src/firetower/slack_app/handlers/resolved.py | 7 +++++- .../slack_app/handlers/update_incident.py | 7 +++++- .../tests/handlers/test_new_incident.py | 11 ++++++-- .../slack_app/tests/handlers/test_resolved.py | 14 +++++++++++ 7 files changed, 74 insertions(+), 6 deletions(-) create mode 100644 src/firetower/incidents/migrations/0023_alter_tag_unique_together_and_more.py diff --git a/src/firetower/incidents/migrations/0023_alter_tag_unique_together_and_more.py b/src/firetower/incidents/migrations/0023_alter_tag_unique_together_and_more.py new file mode 100644 index 00000000..616e1704 --- /dev/null +++ b/src/firetower/incidents/migrations/0023_alter_tag_unique_together_and_more.py @@ -0,0 +1,25 @@ +# Generated by Django 5.2.14 on 2026-06-11 19:30 + +import django.db.models.functions.text +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("incidents", "0022_actionitem_last_nag"), + ] + + operations = [ + migrations.AlterUniqueTogether( + name="tag", + unique_together=set(), + ), + migrations.AddConstraint( + model_name="tag", + constraint=models.UniqueConstraint( + django.db.models.functions.text.Lower("name"), + models.F("type"), + name="unique_tag_name_lower_type", + ), + ), + ] diff --git a/src/firetower/incidents/models.py b/src/firetower/incidents/models.py index 478a454e..7bb35858 100644 --- a/src/firetower/incidents/models.py +++ b/src/firetower/incidents/models.py @@ -6,6 +6,7 @@ from django.core.exceptions import ValidationError from django.db import models, transaction from django.db.models import Q, QuerySet +from django.db.models.functions import Lower INCIDENT_ID_START = 2000 @@ -107,7 +108,13 @@ class Tag(models.Model): created_at = models.DateTimeField(auto_now_add=True) class Meta: - unique_together = [("name", "type")] + constraints = [ + models.UniqueConstraint( + Lower("name"), + "type", + name="unique_tag_name_lower_type", + ) + ] ordering = ["name"] def clean(self) -> None: diff --git a/src/firetower/slack_app/handlers/mitigated.py b/src/firetower/slack_app/handlers/mitigated.py index f597d49c..5c2cf384 100644 --- a/src/firetower/slack_app/handlers/mitigated.py +++ b/src/firetower/slack_app/handlers/mitigated.py @@ -49,7 +49,8 @@ def handle_mitigated_command(ack: Any, body: dict, command: dict, respond: Any) def handle_mitigated_submission(ack: Any, body: dict, view: dict, client: Any) -> None: - form = parse_incident_form_values(view) + # Parse without resolving tags so validation touches no DB before ack(). + form = parse_incident_form_values(view, resolve_tags=False) channel_id = view.get("private_metadata", "") errors = validate_lifecycle_form(form) @@ -76,6 +77,10 @@ def handle_mitigated_submission(ack: Any, body: dict, view: dict, client: Any) - ) return + # Resolve inline tags only now: after ack() and validation, so a rejected + # submission never persists orphaned tags and tag writes can't stall ack(). + form = parse_incident_form_values(view) + data = build_incident_update_data( form, IncidentStatus.MITIGATED, captain_user.email ) diff --git a/src/firetower/slack_app/handlers/resolved.py b/src/firetower/slack_app/handlers/resolved.py index 9b05806c..528e393f 100644 --- a/src/firetower/slack_app/handlers/resolved.py +++ b/src/firetower/slack_app/handlers/resolved.py @@ -47,7 +47,8 @@ def handle_resolved_command(ack: Any, body: dict, command: dict, respond: Any) - def handle_resolved_submission(ack: Any, body: dict, view: dict, client: Any) -> None: - form = parse_incident_form_values(view) + # Parse without resolving tags so validation touches no DB before ack(). + form = parse_incident_form_values(view, resolve_tags=False) channel_id = view.get("private_metadata", "") errors = validate_lifecycle_form(form) @@ -74,6 +75,10 @@ def handle_resolved_submission(ack: Any, body: dict, view: dict, client: Any) -> ) return + # Resolve inline tags only now: after ack() and validation, so a rejected + # submission never persists orphaned tags and tag writes can't stall ack(). + form = parse_incident_form_values(view) + severity = form["severity"] if severity in ("P0", "P1", "P2"): target_status = IncidentStatus.POSTMORTEM diff --git a/src/firetower/slack_app/handlers/update_incident.py b/src/firetower/slack_app/handlers/update_incident.py index e7a644b5..de711403 100644 --- a/src/firetower/slack_app/handlers/update_incident.py +++ b/src/firetower/slack_app/handlers/update_incident.py @@ -232,7 +232,8 @@ def handle_update_command(ack: Any, body: dict, command: dict, respond: Any) -> def handle_update_incident_submission( ack: Any, body: dict, view: dict, client: Any ) -> None: - form = parse_incident_form_values(view) + # Parse without resolving tags so validation touches no DB before ack(). + form = parse_incident_form_values(view, resolve_tags=False) channel_id = view.get("private_metadata", "") values = view.get("state", {}).get("values", {}) @@ -256,6 +257,10 @@ def handle_update_incident_submission( logger.error("Update submission: no incident for channel %s", channel_id) return + # Resolve inline tags only now: after ack() and validation, so a rejected + # submission never persists orphaned tags and tag writes can't stall ack(). + form = parse_incident_form_values(view) + data: dict[str, Any] = { "title": form["title"], "description": form["description"], diff --git a/src/firetower/slack_app/tests/handlers/test_new_incident.py b/src/firetower/slack_app/tests/handlers/test_new_incident.py index 1efcc82c..9224e7f9 100644 --- a/src/firetower/slack_app/tests/handlers/test_new_incident.py +++ b/src/firetower/slack_app/tests/handlers/test_new_incident.py @@ -746,11 +746,17 @@ def test_skips_empty_after_strip(self): assert resolved == ["payments"] def test_recovers_existing_tag_when_create_races(self): - Tag.objects.create(name="Payments", type=TagType.AFFECTED_SERVICE) + # No pre-existing tag, so the initial lookup misses and we enter the + # create branch. A concurrent writer inserts the row first (simulated in + # the side_effect) and our create() then raises IntegrityError, forcing + # the recovery re-query to find the winner. + def racing_create(**kwargs): + Tag(name="Payments", type=TagType.AFFECTED_SERVICE).save() + raise IntegrityError("duplicate key") with patch( "firetower.slack_app.handlers.utils.Tag.objects.create", - side_effect=IntegrityError("duplicate key"), + side_effect=racing_create, ): resolved = _resolve_tag_values( ["__create__:payments"], @@ -758,6 +764,7 @@ def test_recovers_existing_tag_when_create_races(self): ) assert resolved == ["Payments"] + assert Tag.objects.filter(type=TagType.AFFECTED_SERVICE).count() == 1 def test_reraises_when_create_fails_and_no_existing_tag(self): with patch( diff --git a/src/firetower/slack_app/tests/handlers/test_resolved.py b/src/firetower/slack_app/tests/handlers/test_resolved.py index 88407cd0..d232e032 100644 --- a/src/firetower/slack_app/tests/handlers/test_resolved.py +++ b/src/firetower/slack_app/tests/handlers/test_resolved.py @@ -279,6 +279,20 @@ def test_missing_captain_returns_error(self, incident): assert call_kwargs["response_action"] == "errors" assert "captain_block" in call_kwargs["errors"] + def test_rejected_submission_creates_no_inline_tags(self, incident): + ack = MagicMock() + client = MagicMock() + body = {"user": {"id": "U_CAPTAIN"}} + view = _make_resolved_view( + captain=None, + affected_service_tags=("__create__:payments",), + ) + + handle_resolved_submission(ack, body, view, client) + + assert ack.call_args[1]["response_action"] == "errors" + assert not Tag.objects.filter(name__iexact="payments").exists() + @patch("firetower.incidents.serializers.on_incident_updated") @patch("firetower.slack_app.handlers.resolved.get_or_create_user_from_slack_id") def test_missing_description_succeeds( From 0efd42ad259e2a280e81d38c3ba33eeb3a61f178 Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Thu, 11 Jun 2026 16:29:56 -0400 Subject: [PATCH 07/10] Keep the + Create tag option first and cap Slack autocomplete at 100 options --- .../slack_app/handlers/new_incident.py | 10 +++++++--- .../tests/handlers/test_new_incident.py | 18 +++++++++++++++--- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/firetower/slack_app/handlers/new_incident.py b/src/firetower/slack_app/handlers/new_incident.py index 68620454..d4234245 100644 --- a/src/firetower/slack_app/handlers/new_incident.py +++ b/src/firetower/slack_app/handlers/new_incident.py @@ -201,17 +201,21 @@ def handle_tag_options(ack: Any, payload: dict) -> None: if tag_type in INLINE_CREATABLE_TAG_TYPES and stripped: exact_match = Tag.objects.filter(type=tag_type, name__iexact=stripped).exists() if not exact_match: - options.append( + # Put "+ Create" first so it survives the 100-option Slack cap even + # when the keyword already matches a full page of existing tags. + options.insert( + 0, { "text": { "type": "plain_text", "text": f'+ Create "{stripped}"', }, "value": f"{CREATE_TAG_PREFIX}{stripped}", - } + }, ) - ack(options=options) + # Slack rejects block_suggestion responses with more than 100 options. + ack(options=options[:100]) def handle_new_command(ack: Any, body: dict, command: dict, respond: Any) -> None: diff --git a/src/firetower/slack_app/tests/handlers/test_new_incident.py b/src/firetower/slack_app/tests/handlers/test_new_incident.py index 9224e7f9..860b318c 100644 --- a/src/firetower/slack_app/tests/handlers/test_new_incident.py +++ b/src/firetower/slack_app/tests/handlers/test_new_incident.py @@ -418,7 +418,7 @@ def test_returns_matching_tags(self, _mock_close): assert len(options) == 3 names = {o["text"]["text"] for o in options} assert names == {"us-east-1", "us-west-2", '+ Create "us"'} - assert options[-1]["value"] == "__create__:us" + assert options[0]["value"] == "__create__:us" def test_empty_query_returns_all(self, _mock_close): Tag.objects.create(name="us-east-1", type=TagType.AFFECTED_REGION) @@ -456,7 +456,7 @@ def test_offers_create_option_for_service(self, _mock_close): handle_tag_options(ack, payload) options = ack.call_args[1]["options"] - assert options[-1] == { + assert options[0] == { "text": {"type": "plain_text", "text": '+ Create "payments"'}, "value": "__create__:payments", } @@ -467,7 +467,7 @@ def test_offers_create_option_for_region(self, _mock_close): handle_tag_options(ack, payload) options = ack.call_args[1]["options"] - assert options[-1]["value"] == "__create__:ap-south-1" + assert options[0]["value"] == "__create__:ap-south-1" def test_no_create_option_for_impact_type(self, _mock_close): ack = MagicMock() @@ -495,6 +495,18 @@ def test_no_create_option_for_exact_match(self, _mock_close): options = ack.call_args[1]["options"] assert all(not o["value"].startswith("__create__:") for o in options) + def test_create_option_first_and_capped_at_100(self, _mock_close): + for i in range(120): + Tag.objects.create(name=f"svc-{i:03d}", type=TagType.AFFECTED_SERVICE) + + ack = MagicMock() + payload = {"action_id": "affected_service_tags", "value": "svc"} + handle_tag_options(ack, payload) + + options = ack.call_args[1]["options"] + assert len(options) == 100 + assert options[0]["value"] == "__create__:svc" + class TestFallbackChannel: def _base_form_data(self): From 5cc2347de695401e88cd12b0504a154951d2ab70 Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Fri, 12 Jun 2026 12:18:19 -0400 Subject: [PATCH 08/10] Guard post-ack DB work in lifecycle handlers and use a savepoint for inline tag creation race recovery --- src/firetower/slack_app/handlers/mitigated.py | 75 ++++++++++------ src/firetower/slack_app/handlers/resolved.py | 86 ++++++++++++------- .../slack_app/handlers/update_incident.py | 85 ++++++++++-------- src/firetower/slack_app/handlers/utils.py | 10 ++- .../tests/handlers/test_new_incident.py | 33 ++++--- .../slack_app/tests/handlers/test_resolved.py | 18 ++++ 6 files changed, 199 insertions(+), 108 deletions(-) diff --git a/src/firetower/slack_app/handlers/mitigated.py b/src/firetower/slack_app/handlers/mitigated.py index 5c2cf384..e0b3c613 100644 --- a/src/firetower/slack_app/handlers/mitigated.py +++ b/src/firetower/slack_app/handlers/mitigated.py @@ -2,6 +2,7 @@ from typing import Any from django.conf import settings +from django.db import InterfaceError, OperationalError from firetower.auth.services import get_or_create_user_from_slack_id from firetower.incidents.models import Incident, IncidentStatus @@ -60,43 +61,63 @@ def handle_mitigated_submission(ack: Any, body: dict, view: dict, client: Any) - ack() - incident = get_incident_from_channel(channel_id) - if not incident: - logger.error("Mitigated submission: no incident for channel %s", channel_id) - return + # All DB work runs after ack(); guard it so a DB error surfaces to the + # channel instead of silently dropping the update behind a successful ack. + try: + incident = get_incident_from_channel(channel_id) + if not incident: + logger.error("Mitigated submission: no incident for channel %s", channel_id) + return + + captain_user = get_or_create_user_from_slack_id(form["captain_slack_id"]) + if not captain_user: + logger.error( + "Could not resolve Slack user %s to a Firetower user", + form["captain_slack_id"], + ) + client.chat_postMessage( + channel=channel_id, + text="Failed to resolve the selected captain to a Firetower user.", + ) + return + + # Resolve inline tags only now: after ack() and validation, so a + # rejected submission never persists orphaned tags. + form = parse_incident_form_values(view) + + data = build_incident_update_data( + form, IncidentStatus.MITIGATED, captain_user.email + ) - captain_user = get_or_create_user_from_slack_id(form["captain_slack_id"]) - if not captain_user: - logger.error( - "Could not resolve Slack user %s to a Firetower user", - form["captain_slack_id"], + acting_user = get_or_create_user_from_slack_id(body["user"]["id"]) + serializer = IncidentWriteSerializer( + instance=incident, + data=data, + partial=True, + context={"acting_user": acting_user}, ) + if not serializer.is_valid(): + logger.error("Mitigated update failed: %s", serializer.errors) + client.chat_postMessage( + channel=channel_id, + text=f"Failed to update incident: {serializer.errors}", + ) + return + serializer.save() + except (OperationalError, InterfaceError): + logger.exception("Database unreachable during mitigated submission") client.chat_postMessage( channel=channel_id, - text="Failed to resolve the selected captain to a Firetower user.", + text="Something went wrong updating the incident (database issue). Please try again.", ) return - - # Resolve inline tags only now: after ack() and validation, so a rejected - # submission never persists orphaned tags and tag writes can't stall ack(). - form = parse_incident_form_values(view) - - data = build_incident_update_data( - form, IncidentStatus.MITIGATED, captain_user.email - ) - - acting_user = get_or_create_user_from_slack_id(body["user"]["id"]) - serializer = IncidentWriteSerializer( - instance=incident, data=data, partial=True, context={"acting_user": acting_user} - ) - if not serializer.is_valid(): - logger.error("Mitigated update failed: %s", serializer.errors) + except Exception: + logger.exception("Failed to process mitigated submission") client.chat_postMessage( channel=channel_id, - text=f"Failed to update incident: {serializer.errors}", + text="Something went wrong updating the incident. Please try again.", ) return - serializer.save() incident_url = f"{settings.FIRETOWER_BASE_URL}/{incident.incident_number}" client.chat_postMessage( diff --git a/src/firetower/slack_app/handlers/resolved.py b/src/firetower/slack_app/handlers/resolved.py index 528e393f..ff37ed35 100644 --- a/src/firetower/slack_app/handlers/resolved.py +++ b/src/firetower/slack_app/handlers/resolved.py @@ -1,6 +1,8 @@ import logging from typing import Any +from django.db import InterfaceError, OperationalError + from firetower.auth.services import get_or_create_user_from_slack_id from firetower.incidents.models import Incident, IncidentStatus from firetower.incidents.serializers import IncidentWriteSerializer @@ -58,47 +60,67 @@ def handle_resolved_submission(ack: Any, body: dict, view: dict, client: Any) -> ack() - incident = get_incident_from_channel(channel_id) - if not incident: - logger.error("Resolved submission: no incident for channel %s", channel_id) - return - - captain_user = get_or_create_user_from_slack_id(form["captain_slack_id"]) - if not captain_user: - logger.error( - "Could not resolve Slack user %s to a Firetower user", - form["captain_slack_id"], + # All DB work runs after ack(); guard it so a DB error surfaces to the + # channel instead of silently dropping the update behind a successful ack. + try: + incident = get_incident_from_channel(channel_id) + if not incident: + logger.error("Resolved submission: no incident for channel %s", channel_id) + return + + captain_user = get_or_create_user_from_slack_id(form["captain_slack_id"]) + if not captain_user: + logger.error( + "Could not resolve Slack user %s to a Firetower user", + form["captain_slack_id"], + ) + client.chat_postMessage( + channel=channel_id, + text="Failed to resolve the selected captain to a Firetower user.", + ) + return + + # Resolve inline tags only now: after ack() and validation, so a + # rejected submission never persists orphaned tags. + form = parse_incident_form_values(view) + + severity = form["severity"] + if severity in ("P0", "P1", "P2"): + target_status = IncidentStatus.POSTMORTEM + else: + target_status = IncidentStatus.DONE + + data = build_incident_update_data(form, target_status, captain_user.email) + + acting_user = get_or_create_user_from_slack_id(body["user"]["id"]) + serializer = IncidentWriteSerializer( + instance=incident, + data=data, + partial=True, + context={"acting_user": acting_user}, ) + if not serializer.is_valid(): + logger.error("Resolved update failed: %s", serializer.errors) + client.chat_postMessage( + channel=channel_id, + text=f"Failed to resolve incident: {serializer.errors}", + ) + return + serializer.save() + except (OperationalError, InterfaceError): + logger.exception("Database unreachable during resolved submission") client.chat_postMessage( channel=channel_id, - text="Failed to resolve the selected captain to a Firetower user.", + text="Something went wrong updating the incident (database issue). Please try again.", ) return - - # Resolve inline tags only now: after ack() and validation, so a rejected - # submission never persists orphaned tags and tag writes can't stall ack(). - form = parse_incident_form_values(view) - - severity = form["severity"] - if severity in ("P0", "P1", "P2"): - target_status = IncidentStatus.POSTMORTEM - else: - target_status = IncidentStatus.DONE - - data = build_incident_update_data(form, target_status, captain_user.email) - - acting_user = get_or_create_user_from_slack_id(body["user"]["id"]) - serializer = IncidentWriteSerializer( - instance=incident, data=data, partial=True, context={"acting_user": acting_user} - ) - if not serializer.is_valid(): - logger.error("Resolved update failed: %s", serializer.errors) + except Exception: + logger.exception("Failed to process resolved submission") client.chat_postMessage( channel=channel_id, - text=f"Failed to resolve incident: {serializer.errors}", + text="Something went wrong updating the incident. Please try again.", ) return - serializer.save() client.chat_postMessage( channel=channel_id, diff --git a/src/firetower/slack_app/handlers/update_incident.py b/src/firetower/slack_app/handlers/update_incident.py index de711403..b357e9c5 100644 --- a/src/firetower/slack_app/handlers/update_incident.py +++ b/src/firetower/slack_app/handlers/update_incident.py @@ -1,6 +1,8 @@ import logging from typing import Any +from django.db import InterfaceError, OperationalError + from firetower.auth.models import ExternalProfileType from firetower.auth.services import get_or_create_user_from_slack_id from firetower.incidents.models import Incident, IncidentSeverity @@ -252,46 +254,57 @@ def handle_update_incident_submission( ack() - incident = get_incident_from_channel(channel_id) - if not incident: - logger.error("Update submission: no incident for channel %s", channel_id) - return - - # Resolve inline tags only now: after ack() and validation, so a rejected - # submission never persists orphaned tags and tag writes can't stall ack(). - form = parse_incident_form_values(view) - - data: dict[str, Any] = { - "title": form["title"], - "description": form["description"], - "impact_summary": form["impact_summary"], - "is_private": is_private, - "impact_type_tags": form["impact_type_tags"], - "affected_service_tags": form["affected_service_tags"], - "affected_region_tags": form["affected_region_tags"], - } - if form["severity"]: - data["severity"] = form["severity"] - - if form["captain_slack_id"]: - captain_user = get_or_create_user_from_slack_id(form["captain_slack_id"]) - if captain_user: - data["captain"] = captain_user.email - - acting_user = get_or_create_user_from_slack_id(body["user"]["id"]) - serializer = IncidentWriteSerializer( - instance=incident, data=data, partial=True, context={"acting_user": acting_user} - ) - if not serializer.is_valid(): - logger.error("Incident update validation failed: %s", serializer.errors) + # All DB work runs after ack(); guard it so a DB error surfaces to the + # channel instead of silently dropping the update behind a successful ack. + try: + incident = get_incident_from_channel(channel_id) + if not incident: + logger.error("Update submission: no incident for channel %s", channel_id) + return + + # Resolve inline tags only now: after ack() and validation, so a + # rejected submission never persists orphaned tags. + form = parse_incident_form_values(view) + + data: dict[str, Any] = { + "title": form["title"], + "description": form["description"], + "impact_summary": form["impact_summary"], + "is_private": is_private, + "impact_type_tags": form["impact_type_tags"], + "affected_service_tags": form["affected_service_tags"], + "affected_region_tags": form["affected_region_tags"], + } + if form["severity"]: + data["severity"] = form["severity"] + + if form["captain_slack_id"]: + captain_user = get_or_create_user_from_slack_id(form["captain_slack_id"]) + if captain_user: + data["captain"] = captain_user.email + + acting_user = get_or_create_user_from_slack_id(body["user"]["id"]) + serializer = IncidentWriteSerializer( + instance=incident, + data=data, + partial=True, + context={"acting_user": acting_user}, + ) + if not serializer.is_valid(): + logger.error("Incident update validation failed: %s", serializer.errors) + client.chat_postMessage( + channel=channel_id, + text=f"Failed to update incident: {serializer.errors}", + ) + return + serializer.save() + except (OperationalError, InterfaceError): + logger.exception("Database unreachable during incident update") client.chat_postMessage( channel=channel_id, - text=f"Failed to update incident: {serializer.errors}", + text="Something went wrong updating the incident (database issue). Please try again.", ) return - - try: - serializer.save() except Exception: logger.exception("Failed to update incident from Slack modal") client.chat_postMessage( diff --git a/src/firetower/slack_app/handlers/utils.py b/src/firetower/slack_app/handlers/utils.py index 1a8ab1ce..606b80ec 100644 --- a/src/firetower/slack_app/handlers/utils.py +++ b/src/firetower/slack_app/handlers/utils.py @@ -3,7 +3,7 @@ from django.conf import settings from django.core.exceptions import ValidationError -from django.db import IntegrityError +from django.db import IntegrityError, transaction from firetower.auth.models import ExternalProfileType from firetower.incidents.models import ( @@ -51,7 +51,13 @@ def _resolve_tag_values( try: # Inline-created tags are intentionally left # approved=False so they surface for admin audit. - tag = Tag.objects.create(name=name, type=tag_type) + # Wrap in a savepoint so a concurrent insert that trips + # the DB uniqueness constraint rolls back only this + # create — on PostgreSQL an IntegrityError otherwise + # aborts the whole transaction and the recovery query + # below would fail. + with transaction.atomic(): + tag = Tag.objects.create(name=name, type=tag_type) logger.info( "Created %s tag %r from Slack modal", tag_type, tag.name ) diff --git a/src/firetower/slack_app/tests/handlers/test_new_incident.py b/src/firetower/slack_app/tests/handlers/test_new_incident.py index 860b318c..daa9b23e 100644 --- a/src/firetower/slack_app/tests/handlers/test_new_incident.py +++ b/src/firetower/slack_app/tests/handlers/test_new_incident.py @@ -758,20 +758,31 @@ def test_skips_empty_after_strip(self): assert resolved == ["payments"] def test_recovers_existing_tag_when_create_races(self): - # No pre-existing tag, so the initial lookup misses and we enter the - # create branch. A concurrent writer inserts the row first (simulated in - # the side_effect) and our create() then raises IntegrityError, forcing - # the recovery re-query to find the winner. - def racing_create(**kwargs): - Tag(name="Payments", type=TagType.AFFECTED_SERVICE).save() - raise IntegrityError("duplicate key") + # A row already exists; simulate the race where our initial lookup + # misses it and our own create() then trips the DB unique constraint + # for real. The savepoint must contain that IntegrityError so the + # recovery re-query can still run — on PostgreSQL the surrounding + # transaction is aborted otherwise and this raises InternalError. + Tag.objects.create(name="Payments", type=TagType.AFFECTED_SERVICE) - with patch( - "firetower.slack_app.handlers.utils.Tag.objects.create", - side_effect=racing_create, + real_filter = Tag.objects.filter + calls = {"n": 0} + + def racing_filter(*args, **kwargs): + calls["n"] += 1 + if calls["n"] == 1: # initial existence check misses the row + return Tag.objects.none() + return real_filter(*args, **kwargs) + + # Skip model validation so the conflict surfaces as a real DB-level + # IntegrityError (the savepoint path) rather than a pre-INSERT + # ValidationError from full_clean(). + with ( + patch.object(Tag, "full_clean", lambda self, *a, **k: None), + patch.object(Tag.objects, "filter", side_effect=racing_filter), ): resolved = _resolve_tag_values( - ["__create__:payments"], + ["__create__:Payments"], TagType.AFFECTED_SERVICE, ) diff --git a/src/firetower/slack_app/tests/handlers/test_resolved.py b/src/firetower/slack_app/tests/handlers/test_resolved.py index d232e032..72165315 100644 --- a/src/firetower/slack_app/tests/handlers/test_resolved.py +++ b/src/firetower/slack_app/tests/handlers/test_resolved.py @@ -1,6 +1,7 @@ from unittest.mock import MagicMock, patch import pytest +from django.db import OperationalError from firetower.incidents.models import IncidentSeverity, IncidentStatus, Tag, TagType from firetower.slack_app.handlers.resolved import ( @@ -441,3 +442,20 @@ def test_captain_resolution_failure(self, mock_get_user, incident): client.chat_postMessage.assert_called_once() msg = client.chat_postMessage.call_args[1]["text"] assert "Failed to resolve the selected captain" in msg + + @patch("firetower.slack_app.handlers.resolved.get_incident_from_channel") + def test_db_error_after_ack_notifies_channel(self, mock_get_incident): + # A DB failure after ack() must surface to the channel, not vanish + # behind the successful ack. + mock_get_incident.side_effect = OperationalError("db down") + ack = MagicMock() + client = MagicMock() + body = {"user": {"id": "U_CAPTAIN"}} + view = _make_resolved_view() + + handle_resolved_submission(ack, body, view, client) + + ack.assert_called_once_with() + client.chat_postMessage.assert_called_once() + msg = client.chat_postMessage.call_args[1]["text"] + assert "database issue" in msg From 59684126679f961fe658e71e90a0be9a5fb4cfb2 Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Fri, 12 Jun 2026 12:26:07 -0400 Subject: [PATCH 09/10] Simplify inline tag creation: rely on clean() and the handler guard instead of a DB constraint and race recovery --- ...0023_alter_tag_unique_together_and_more.py | 25 ----------- src/firetower/incidents/models.py | 9 +--- src/firetower/slack_app/handlers/utils.py | 33 +++++--------- .../tests/handlers/test_new_incident.py | 44 +++---------------- 4 files changed, 17 insertions(+), 94 deletions(-) delete mode 100644 src/firetower/incidents/migrations/0023_alter_tag_unique_together_and_more.py diff --git a/src/firetower/incidents/migrations/0023_alter_tag_unique_together_and_more.py b/src/firetower/incidents/migrations/0023_alter_tag_unique_together_and_more.py deleted file mode 100644 index 616e1704..00000000 --- a/src/firetower/incidents/migrations/0023_alter_tag_unique_together_and_more.py +++ /dev/null @@ -1,25 +0,0 @@ -# Generated by Django 5.2.14 on 2026-06-11 19:30 - -import django.db.models.functions.text -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("incidents", "0022_actionitem_last_nag"), - ] - - operations = [ - migrations.AlterUniqueTogether( - name="tag", - unique_together=set(), - ), - migrations.AddConstraint( - model_name="tag", - constraint=models.UniqueConstraint( - django.db.models.functions.text.Lower("name"), - models.F("type"), - name="unique_tag_name_lower_type", - ), - ), - ] diff --git a/src/firetower/incidents/models.py b/src/firetower/incidents/models.py index 7bb35858..478a454e 100644 --- a/src/firetower/incidents/models.py +++ b/src/firetower/incidents/models.py @@ -6,7 +6,6 @@ from django.core.exceptions import ValidationError from django.db import models, transaction from django.db.models import Q, QuerySet -from django.db.models.functions import Lower INCIDENT_ID_START = 2000 @@ -108,13 +107,7 @@ class Tag(models.Model): created_at = models.DateTimeField(auto_now_add=True) class Meta: - constraints = [ - models.UniqueConstraint( - Lower("name"), - "type", - name="unique_tag_name_lower_type", - ) - ] + unique_together = [("name", "type")] ordering = ["name"] def clean(self) -> None: diff --git a/src/firetower/slack_app/handlers/utils.py b/src/firetower/slack_app/handlers/utils.py index 606b80ec..0ac843ef 100644 --- a/src/firetower/slack_app/handlers/utils.py +++ b/src/firetower/slack_app/handlers/utils.py @@ -2,8 +2,6 @@ from typing import Any from django.conf import settings -from django.core.exceptions import ValidationError -from django.db import IntegrityError, transaction from firetower.auth.models import ExternalProfileType from firetower.incidents.models import ( @@ -48,27 +46,16 @@ def _resolve_tag_values( if existing: name = existing.name else: - try: - # Inline-created tags are intentionally left - # approved=False so they surface for admin audit. - # Wrap in a savepoint so a concurrent insert that trips - # the DB uniqueness constraint rolls back only this - # create — on PostgreSQL an IntegrityError otherwise - # aborts the whole transaction and the recovery query - # below would fail. - with transaction.atomic(): - tag = Tag.objects.create(name=name, type=tag_type) - logger.info( - "Created %s tag %r from Slack modal", tag_type, tag.name - ) - name = tag.name - except (ValidationError, IntegrityError): - existing = Tag.objects.filter( - type=tag_type, name__iexact=name - ).first() - if existing is None: - raise - name = existing.name + # Inline-created tags are intentionally left approved=False + # so they surface for admin audit. Tag.clean() guards + # case-insensitive uniqueness; a rare simultaneous duplicate + # create just errors out and the caller's guard asks the + # user to retry. + tag = Tag.objects.create(name=name, type=tag_type) + logger.info( + "Created %s tag %r from Slack modal", tag_type, tag.name + ) + name = tag.name key = name.lower() if key in seen: continue diff --git a/src/firetower/slack_app/tests/handlers/test_new_incident.py b/src/firetower/slack_app/tests/handlers/test_new_incident.py index daa9b23e..bc3b70c5 100644 --- a/src/firetower/slack_app/tests/handlers/test_new_incident.py +++ b/src/firetower/slack_app/tests/handlers/test_new_incident.py @@ -3,7 +3,7 @@ import pytest from django.conf import settings from django.contrib.auth.models import User -from django.db import IntegrityError, OperationalError +from django.db import OperationalError from firetower.auth.models import ExternalProfile, ExternalProfileType from firetower.incidents.models import Incident, IncidentSeverity, Tag, TagType @@ -757,45 +757,13 @@ def test_skips_empty_after_strip(self): assert resolved == ["payments"] - def test_recovers_existing_tag_when_create_races(self): - # A row already exists; simulate the race where our initial lookup - # misses it and our own create() then trips the DB unique constraint - # for real. The savepoint must contain that IntegrityError so the - # recovery re-query can still run — on PostgreSQL the surrounding - # transaction is aborted otherwise and this raises InternalError. + def test_reuses_existing_tag_case_insensitively(self): Tag.objects.create(name="Payments", type=TagType.AFFECTED_SERVICE) - real_filter = Tag.objects.filter - calls = {"n": 0} - - def racing_filter(*args, **kwargs): - calls["n"] += 1 - if calls["n"] == 1: # initial existence check misses the row - return Tag.objects.none() - return real_filter(*args, **kwargs) - - # Skip model validation so the conflict surfaces as a real DB-level - # IntegrityError (the savepoint path) rather than a pre-INSERT - # ValidationError from full_clean(). - with ( - patch.object(Tag, "full_clean", lambda self, *a, **k: None), - patch.object(Tag.objects, "filter", side_effect=racing_filter), - ): - resolved = _resolve_tag_values( - ["__create__:Payments"], - TagType.AFFECTED_SERVICE, - ) + resolved = _resolve_tag_values( + ["__create__:payments"], + TagType.AFFECTED_SERVICE, + ) assert resolved == ["Payments"] assert Tag.objects.filter(type=TagType.AFFECTED_SERVICE).count() == 1 - - def test_reraises_when_create_fails_and_no_existing_tag(self): - with patch( - "firetower.slack_app.handlers.utils.Tag.objects.create", - side_effect=IntegrityError("boom"), - ): - with pytest.raises(IntegrityError): - _resolve_tag_values( - ["__create__:payments"], - TagType.AFFECTED_SERVICE, - ) From 93c36eccaa6dce40356f5cef5ffe2b8042f44d64 Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Fri, 12 Jun 2026 12:49:48 -0400 Subject: [PATCH 10/10] Acknowledge the lifecycle modal before the update and report failures ephemerally --- src/firetower/slack_app/handlers/mitigated.py | 52 ++++++------------ src/firetower/slack_app/handlers/resolved.py | 53 ++++++------------- .../slack_app/handlers/update_incident.py | 35 ++++-------- src/firetower/slack_app/handlers/utils.py | 13 +++++ .../tests/handlers/test_mitigated.py | 6 +-- .../slack_app/tests/handlers/test_resolved.py | 18 +++---- 6 files changed, 68 insertions(+), 109 deletions(-) diff --git a/src/firetower/slack_app/handlers/mitigated.py b/src/firetower/slack_app/handlers/mitigated.py index e0b3c613..c3cd2c1f 100644 --- a/src/firetower/slack_app/handlers/mitigated.py +++ b/src/firetower/slack_app/handlers/mitigated.py @@ -2,7 +2,6 @@ from typing import Any from django.conf import settings -from django.db import InterfaceError, OperationalError from firetower.auth.services import get_or_create_user_from_slack_id from firetower.incidents.models import Incident, IncidentStatus @@ -11,6 +10,7 @@ build_incident_lifecycle_modal, build_incident_update_data, get_incident_from_channel, + notify_submission_error, parse_incident_form_values, validate_lifecycle_form, ) @@ -61,35 +61,28 @@ def handle_mitigated_submission(ack: Any, body: dict, view: dict, client: Any) - ack() - # All DB work runs after ack(); guard it so a DB error surfaces to the - # channel instead of silently dropping the update behind a successful ack. + user_id = body["user"]["id"] + + # Work runs after ack() so the modal closes immediately; any failure is + # reported back to the submitter as an ephemeral message. try: incident = get_incident_from_channel(channel_id) - if not incident: - logger.error("Mitigated submission: no incident for channel %s", channel_id) - return - - captain_user = get_or_create_user_from_slack_id(form["captain_slack_id"]) - if not captain_user: - logger.error( - "Could not resolve Slack user %s to a Firetower user", - form["captain_slack_id"], - ) - client.chat_postMessage( - channel=channel_id, - text="Failed to resolve the selected captain to a Firetower user.", - ) + captain_user = ( + get_or_create_user_from_slack_id(form["captain_slack_id"]) + if incident + else None + ) + if incident is None or captain_user is None: + notify_submission_error(client, channel_id, user_id) return - # Resolve inline tags only now: after ack() and validation, so a - # rejected submission never persists orphaned tags. + # Resolve/create inline tags only after validation passed. form = parse_incident_form_values(view) data = build_incident_update_data( form, IncidentStatus.MITIGATED, captain_user.email ) - - acting_user = get_or_create_user_from_slack_id(body["user"]["id"]) + acting_user = get_or_create_user_from_slack_id(user_id) serializer = IncidentWriteSerializer( instance=incident, data=data, @@ -98,25 +91,12 @@ def handle_mitigated_submission(ack: Any, body: dict, view: dict, client: Any) - ) if not serializer.is_valid(): logger.error("Mitigated update failed: %s", serializer.errors) - client.chat_postMessage( - channel=channel_id, - text=f"Failed to update incident: {serializer.errors}", - ) + notify_submission_error(client, channel_id, user_id) return serializer.save() - except (OperationalError, InterfaceError): - logger.exception("Database unreachable during mitigated submission") - client.chat_postMessage( - channel=channel_id, - text="Something went wrong updating the incident (database issue). Please try again.", - ) - return except Exception: logger.exception("Failed to process mitigated submission") - client.chat_postMessage( - channel=channel_id, - text="Something went wrong updating the incident. Please try again.", - ) + notify_submission_error(client, channel_id, user_id) return incident_url = f"{settings.FIRETOWER_BASE_URL}/{incident.incident_number}" diff --git a/src/firetower/slack_app/handlers/resolved.py b/src/firetower/slack_app/handlers/resolved.py index ff37ed35..f83ad089 100644 --- a/src/firetower/slack_app/handlers/resolved.py +++ b/src/firetower/slack_app/handlers/resolved.py @@ -1,8 +1,6 @@ import logging from typing import Any -from django.db import InterfaceError, OperationalError - from firetower.auth.services import get_or_create_user_from_slack_id from firetower.incidents.models import Incident, IncidentStatus from firetower.incidents.serializers import IncidentWriteSerializer @@ -10,6 +8,7 @@ build_incident_lifecycle_modal, build_incident_update_data, get_incident_from_channel, + notify_submission_error, parse_incident_form_values, validate_lifecycle_form, ) @@ -60,28 +59,22 @@ def handle_resolved_submission(ack: Any, body: dict, view: dict, client: Any) -> ack() - # All DB work runs after ack(); guard it so a DB error surfaces to the - # channel instead of silently dropping the update behind a successful ack. + user_id = body["user"]["id"] + + # Work runs after ack() so the modal closes immediately; any failure is + # reported back to the submitter as an ephemeral message. try: incident = get_incident_from_channel(channel_id) - if not incident: - logger.error("Resolved submission: no incident for channel %s", channel_id) - return - - captain_user = get_or_create_user_from_slack_id(form["captain_slack_id"]) - if not captain_user: - logger.error( - "Could not resolve Slack user %s to a Firetower user", - form["captain_slack_id"], - ) - client.chat_postMessage( - channel=channel_id, - text="Failed to resolve the selected captain to a Firetower user.", - ) + captain_user = ( + get_or_create_user_from_slack_id(form["captain_slack_id"]) + if incident + else None + ) + if incident is None or captain_user is None: + notify_submission_error(client, channel_id, user_id) return - # Resolve inline tags only now: after ack() and validation, so a - # rejected submission never persists orphaned tags. + # Resolve/create inline tags only after validation passed. form = parse_incident_form_values(view) severity = form["severity"] @@ -91,8 +84,7 @@ def handle_resolved_submission(ack: Any, body: dict, view: dict, client: Any) -> target_status = IncidentStatus.DONE data = build_incident_update_data(form, target_status, captain_user.email) - - acting_user = get_or_create_user_from_slack_id(body["user"]["id"]) + acting_user = get_or_create_user_from_slack_id(user_id) serializer = IncidentWriteSerializer( instance=incident, data=data, @@ -101,25 +93,12 @@ def handle_resolved_submission(ack: Any, body: dict, view: dict, client: Any) -> ) if not serializer.is_valid(): logger.error("Resolved update failed: %s", serializer.errors) - client.chat_postMessage( - channel=channel_id, - text=f"Failed to resolve incident: {serializer.errors}", - ) + notify_submission_error(client, channel_id, user_id) return serializer.save() - except (OperationalError, InterfaceError): - logger.exception("Database unreachable during resolved submission") - client.chat_postMessage( - channel=channel_id, - text="Something went wrong updating the incident (database issue). Please try again.", - ) - return except Exception: logger.exception("Failed to process resolved submission") - client.chat_postMessage( - channel=channel_id, - text="Something went wrong updating the incident. Please try again.", - ) + notify_submission_error(client, channel_id, user_id) return client.chat_postMessage( diff --git a/src/firetower/slack_app/handlers/update_incident.py b/src/firetower/slack_app/handlers/update_incident.py index b357e9c5..32c121f6 100644 --- a/src/firetower/slack_app/handlers/update_incident.py +++ b/src/firetower/slack_app/handlers/update_incident.py @@ -1,14 +1,13 @@ import logging from typing import Any -from django.db import InterfaceError, OperationalError - from firetower.auth.models import ExternalProfileType from firetower.auth.services import get_or_create_user_from_slack_id from firetower.incidents.models import Incident, IncidentSeverity from firetower.incidents.serializers import IncidentWriteSerializer from firetower.slack_app.handlers.utils import ( get_incident_from_channel, + notify_submission_error, parse_incident_form_values, ) @@ -254,16 +253,17 @@ def handle_update_incident_submission( ack() - # All DB work runs after ack(); guard it so a DB error surfaces to the - # channel instead of silently dropping the update behind a successful ack. + user_id = body["user"]["id"] + + # Work runs after ack() so the modal closes immediately; any failure is + # reported back to the submitter as an ephemeral message. try: incident = get_incident_from_channel(channel_id) - if not incident: - logger.error("Update submission: no incident for channel %s", channel_id) + if incident is None: + notify_submission_error(client, channel_id, user_id) return - # Resolve inline tags only now: after ack() and validation, so a - # rejected submission never persists orphaned tags. + # Resolve/create inline tags only after validation passed. form = parse_incident_form_values(view) data: dict[str, Any] = { @@ -283,7 +283,7 @@ def handle_update_incident_submission( if captain_user: data["captain"] = captain_user.email - acting_user = get_or_create_user_from_slack_id(body["user"]["id"]) + acting_user = get_or_create_user_from_slack_id(user_id) serializer = IncidentWriteSerializer( instance=incident, data=data, @@ -292,25 +292,12 @@ def handle_update_incident_submission( ) if not serializer.is_valid(): logger.error("Incident update validation failed: %s", serializer.errors) - client.chat_postMessage( - channel=channel_id, - text=f"Failed to update incident: {serializer.errors}", - ) + notify_submission_error(client, channel_id, user_id) return serializer.save() - except (OperationalError, InterfaceError): - logger.exception("Database unreachable during incident update") - client.chat_postMessage( - channel=channel_id, - text="Something went wrong updating the incident (database issue). Please try again.", - ) - return except Exception: logger.exception("Failed to update incident from Slack modal") - client.chat_postMessage( - channel=channel_id, - text="Something went wrong updating the incident. Please try again.", - ) + notify_submission_error(client, channel_id, user_id) return client.chat_postMessage( diff --git a/src/firetower/slack_app/handlers/utils.py b/src/firetower/slack_app/handlers/utils.py index 0ac843ef..8da3ada8 100644 --- a/src/firetower/slack_app/handlers/utils.py +++ b/src/firetower/slack_app/handlers/utils.py @@ -78,6 +78,19 @@ def get_incident_from_channel(channel_id: str) -> Incident | None: return None +def notify_submission_error( + client: Any, + channel_id: str, + user_id: str, + text: str = "Something went wrong updating the incident. Please try again.", +) -> None: + """Send the submitting user an ephemeral failure notice (visible only to them).""" + try: + client.chat_postEphemeral(channel=channel_id, user=user_id, text=text) + except Exception: + logger.exception("Failed to post ephemeral submission error to %s", channel_id) + + def build_incident_form_blocks(user_id: str = "") -> list[dict[str, Any]]: severity_options = [ { diff --git a/src/firetower/slack_app/tests/handlers/test_mitigated.py b/src/firetower/slack_app/tests/handlers/test_mitigated.py index 86a3f07a..003d1a37 100644 --- a/src/firetower/slack_app/tests/handlers/test_mitigated.py +++ b/src/firetower/slack_app/tests/handlers/test_mitigated.py @@ -350,9 +350,9 @@ def test_captain_resolution_failure(self, mock_get_user, incident): handle_mitigated_submission(ack, body, view, client) ack.assert_called_once_with() - client.chat_postMessage.assert_called_once() - msg = client.chat_postMessage.call_args[1]["text"] - assert "Failed to resolve the selected captain" in msg + client.chat_postMessage.assert_not_called() + client.chat_postEphemeral.assert_called_once() + assert client.chat_postEphemeral.call_args[1]["user"] == "U_CAPTAIN" @pytest.mark.django_db diff --git a/src/firetower/slack_app/tests/handlers/test_resolved.py b/src/firetower/slack_app/tests/handlers/test_resolved.py index 72165315..60e08f64 100644 --- a/src/firetower/slack_app/tests/handlers/test_resolved.py +++ b/src/firetower/slack_app/tests/handlers/test_resolved.py @@ -439,14 +439,14 @@ def test_captain_resolution_failure(self, mock_get_user, incident): handle_resolved_submission(ack, body, view, client) ack.assert_called_once_with() - client.chat_postMessage.assert_called_once() - msg = client.chat_postMessage.call_args[1]["text"] - assert "Failed to resolve the selected captain" in msg + client.chat_postMessage.assert_not_called() + client.chat_postEphemeral.assert_called_once() + assert client.chat_postEphemeral.call_args[1]["user"] == "U_CAPTAIN" @patch("firetower.slack_app.handlers.resolved.get_incident_from_channel") - def test_db_error_after_ack_notifies_channel(self, mock_get_incident): - # A DB failure after ack() must surface to the channel, not vanish - # behind the successful ack. + def test_db_error_posts_ephemeral_failure(self, mock_get_incident): + # A DB failure must surface to the submitter (ephemerally), not vanish + # behind a closed modal. mock_get_incident.side_effect = OperationalError("db down") ack = MagicMock() client = MagicMock() @@ -456,6 +456,6 @@ def test_db_error_after_ack_notifies_channel(self, mock_get_incident): handle_resolved_submission(ack, body, view, client) ack.assert_called_once_with() - client.chat_postMessage.assert_called_once() - msg = client.chat_postMessage.call_args[1]["text"] - assert "database issue" in msg + client.chat_postMessage.assert_not_called() + client.chat_postEphemeral.assert_called_once() + assert client.chat_postEphemeral.call_args[1]["user"] == "U_CAPTAIN"