From 1d089aa7a33bcd4317e67ee2778920a31838ea24 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Fri, 1 Sep 2023 10:14:59 +0200 Subject: [PATCH] fix IntegrityErrors occuring when creating ResolutionNoteSlackMessage objects (#2933) # What this PR does ## Which issue(s) this PR fixes Closes https://github.com/grafana/oncall-private/issues/1822 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --- CHANGELOG.md | 4 +- .../migrations/0031_auto_20230831_1445.py | 21 + engine/apps/alerts/models/resolution_note.py | 5 + .../scenarios/slack_channel_integration.py | 61 +-- .../test_slack_channel_integration.py | 439 ++++++++++++++++++ 5 files changed, 491 insertions(+), 39 deletions(-) create mode 100644 engine/apps/alerts/migrations/0031_auto_20230831_1445.py create mode 100644 engine/apps/slack/tests/test_scenario_steps/test_slack_channel_integration.py diff --git a/CHANGELOG.md b/CHANGELOG.md index fb1e39cb..b929ab74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,10 +23,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fix issue with helm chart when specifying `broker.type=rabbitmq` where Redis environment variables - were not longer being injected @joeyorlando ([#2927](https://github.com/grafana/oncall/pull/2927)) + were not longer being injected by @joeyorlando ([#2927](https://github.com/grafana/oncall/pull/2927)) - Fix silence for alert groups with empty escalation chain @Ferril ([#2929](https://github.com/grafana/oncall/pull/2929)) - Fixed NPE when migrating legacy Grafana Alerting integrations ([#2908](https://github.com/grafana/oncall/issues/2908)) +- Fix `IntegrityError` exceptions that occasionally would occur when trying to create `ResolutionNoteSlackMessage` + objects by @joeyorlando ([#2933](https://github.com/grafana/oncall/pull/2933)) ## v1.3.29 (2023-08-29) diff --git a/engine/apps/alerts/migrations/0031_auto_20230831_1445.py b/engine/apps/alerts/migrations/0031_auto_20230831_1445.py new file mode 100644 index 00000000..ea6cdd80 --- /dev/null +++ b/engine/apps/alerts/migrations/0031_auto_20230831_1445.py @@ -0,0 +1,21 @@ +# Generated by Django 3.2.20 on 2023-08-31 14:45 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('alerts', '0030_auto_20230731_0341'), + ] + + operations = [ + migrations.AddIndex( + model_name='resolutionnoteslackmessage', + index=models.Index(fields=['ts', 'thread_ts', 'alert_group_id'], name='alerts_reso_ts_08f72c_idx'), + ), + migrations.AddIndex( + model_name='resolutionnoteslackmessage', + index=models.Index(fields=['ts', 'thread_ts', 'slack_channel_id'], name='alerts_reso_ts_a9bdf7_idx'), + ), + ] diff --git a/engine/apps/alerts/models/resolution_note.py b/engine/apps/alerts/models/resolution_note.py index da65ab94..af1317c9 100644 --- a/engine/apps/alerts/models/resolution_note.py +++ b/engine/apps/alerts/models/resolution_note.py @@ -83,6 +83,11 @@ class ResolutionNoteSlackMessage(models.Model): class Meta: unique_together = ("thread_ts", "ts") + indexes = [ + models.Index(fields=["ts", "thread_ts", "alert_group_id"]), + models.Index(fields=["ts", "thread_ts", "slack_channel_id"]), + ] + def get_resolution_note(self) -> typing.Optional["ResolutionNote"]: try: return self.resolution_note diff --git a/engine/apps/slack/scenarios/slack_channel_integration.py b/engine/apps/slack/scenarios/slack_channel_integration.py index 8e1cb787..7a2eea9c 100644 --- a/engine/apps/slack/scenarios/slack_channel_integration.py +++ b/engine/apps/slack/scenarios/slack_channel_integration.py @@ -82,48 +82,33 @@ class SlackChannelMessageEventStep(scenario_step.ScenarioStep): if result["permalink"] is not None: permalink = result["permalink"] - try: - slack_thread_message = ResolutionNoteSlackMessage.objects.get( - ts=message_ts, - thread_ts=thread_ts, - alert_group=alert_group, + if len(text) > 2900: + self._slack_client.api_call( + "chat.postEphemeral", + channel=channel, + user=slack_user_identity.slack_id, + text=":warning: Unable to show the <{}|message> in Resolution Note: the message is too long ({}). " + "Max length - 2900 symbols.".format(permalink, len(text)), ) - if len(text) > 2900: - if slack_thread_message.added_to_resolution_note: - self._slack_client.api_call( - "chat.postEphemeral", - channel=channel, - user=slack_user_identity.slack_id, - text=":warning: Unable to update the <{}|message> in Resolution Note: the message is too long ({}). " - "Max length - 2900 symbols.".format(permalink, len(text)), - ) - return + return + + slack_thread_message, created = ResolutionNoteSlackMessage.objects.get_or_create( + ts=message_ts, + thread_ts=thread_ts, + alert_group=alert_group, + defaults={ + "user": self.user, + "added_by_user": self.user, + "text": text, + "slack_channel_id": channel, + "permalink": permalink, + }, + ) + + if not created: slack_thread_message.text = text slack_thread_message.save() - except ResolutionNoteSlackMessage.DoesNotExist: - if len(text) > 2900: - self._slack_client.api_call( - "chat.postEphemeral", - channel=channel, - user=slack_user_identity.slack_id, - text=":warning: The <{}|message> will not be displayed in Resolution Note: " - "the message is too long ({}). Max length - 2900 symbols.".format(permalink, len(text)), - ) - return - - slack_thread_message = ResolutionNoteSlackMessage( - alert_group=alert_group, - user=self.user, - added_by_user=self.user, - text=text, - slack_channel_id=channel, - thread_ts=thread_ts, - ts=message_ts, - permalink=permalink, - ) - slack_thread_message.save() - def delete_thread_message_from_resolution_note( self, slack_user_identity: "SlackUserIdentity", payload: EventPayload ) -> None: diff --git a/engine/apps/slack/tests/test_scenario_steps/test_slack_channel_integration.py b/engine/apps/slack/tests/test_scenario_steps/test_slack_channel_integration.py new file mode 100644 index 00000000..6cc01525 --- /dev/null +++ b/engine/apps/slack/tests/test_scenario_steps/test_slack_channel_integration.py @@ -0,0 +1,439 @@ +from unittest.mock import Mock, call, patch + +import pytest + +from apps.alerts.models import ResolutionNoteSlackMessage +from apps.slack.scenarios.slack_channel_integration import SlackChannelMessageEventStep + + +@pytest.mark.django_db +class TestSlackChannelMessageEventStep: + @patch.object(SlackChannelMessageEventStep, "save_thread_message_for_resolution_note") + @patch.object(SlackChannelMessageEventStep, "delete_thread_message_from_resolution_note") + @pytest.mark.parametrize( + "payload,save_called,delete_called", + [ + ( + { + # does not have thread_ts attribute or subtype + "event": {}, + }, + False, + False, + ), + ( + { + # has thread_ts attribute but has subtype attribute that is not MESSAGE_CHANGED + "event": { + "thread_ts": "foo", + "subtype": "bar", + }, + }, + False, + False, + ), + ( + { + # has thread_ts attribute but not subtype attribute + "event": { + "thread_ts": "foo", + }, + }, + True, + False, + ), + # MESSAGE_CHANGED event.subtype scenarios + ( + { + "event": { + "subtype": "message_changed", + "message": { + "subtype": "bar", + "thread_ts": "hello", + }, + }, + }, + False, + False, + ), + ( + { + "event": { + "subtype": "message_changed", + "message": { + "subtype": "bar", + }, + }, + }, + False, + False, + ), + ( + { + "event": { + "subtype": "potato", + "message": { + "thread_ts": "bar", + }, + }, + }, + False, + False, + ), + ( + { + "event": { + "subtype": "message_changed", + "message": { + "thread_ts": "bar", + }, + }, + }, + True, + False, + ), + ( + { + "event": { + "subtype": "message_deleted", + "previous_message": {}, + }, + }, + False, + False, + ), + ( + { + "event": { + "subtype": "message_deleted", + "previous_message": {}, + }, + }, + False, + False, + ), + ( + { + "event": { + "subtype": "potato", + "previous_message": { + "thread_ts": "bar", + }, + }, + }, + False, + False, + ), + ( + { + "event": { + "subtype": "message_deleted", + "previous_message": { + "thread_ts": "bar", + }, + }, + }, + False, + True, + ), + ], + ) + def test_process_scenario( + self, + mock_delete_thread_message_from_resolution_note, + mock_save_thread_message_for_resolution_note, + make_organization_and_user_with_slack_identities, + payload, + save_called, + delete_called, + ) -> None: + ( + organization, + user, + slack_team_identity, + slack_user_identity, + ) = make_organization_and_user_with_slack_identities() + + step = SlackChannelMessageEventStep(slack_team_identity, organization, user) + step.process_scenario(slack_user_identity, slack_team_identity, payload) + + if save_called: + mock_save_thread_message_for_resolution_note.assert_called_once_with(slack_user_identity, payload) + else: + mock_save_thread_message_for_resolution_note.assert_not_called() + + if delete_called: + mock_delete_thread_message_from_resolution_note.assert_called_once_with(slack_user_identity, payload) + else: + mock_delete_thread_message_from_resolution_note.assert_not_called() + + @patch("apps.alerts.models.ResolutionNoteSlackMessage") + def test_save_thread_message_for_resolution_note_no_slack_user_identity( + self, MockResolutionNoteSlackMessage, make_organization_and_user_with_slack_identities + ) -> None: + organization, user, slack_team_identity, _ = make_organization_and_user_with_slack_identities() + + step = SlackChannelMessageEventStep(slack_team_identity, organization, user) + step._slack_client = Mock() + + step.save_thread_message_for_resolution_note(None, {}) + + step._slack_client.api_call.assert_not_called() + MockResolutionNoteSlackMessage.objects.get_or_create.assert_not_called() + + @patch("apps.alerts.models.ResolutionNoteSlackMessage") + def test_save_thread_message_for_resolution_note_no_slack_message( + self, MockResolutionNoteSlackMessage, make_organization_and_user_with_slack_identities + ) -> None: + ( + organization, + user, + slack_team_identity, + slack_user_identity, + ) = make_organization_and_user_with_slack_identities() + + step = SlackChannelMessageEventStep(slack_team_identity, organization, user) + step._slack_client = Mock() + + payload = { + "event": { + "channel": "potato", + "ts": 88945.4849, + "thread_ts": 16789.123, + "text": "hello", + }, + } + + step.save_thread_message_for_resolution_note(slack_user_identity, payload) + + step._slack_client.api_call.assert_not_called() + MockResolutionNoteSlackMessage.objects.get_or_create.assert_not_called() + + @patch("apps.alerts.models.ResolutionNoteSlackMessage") + def test_save_thread_message_for_resolution_note_really_long_text( + self, + MockResolutionNoteSlackMessage, + make_organization_and_user_with_slack_identities, + make_alert_receive_channel, + make_alert_group, + make_slack_message, + ) -> None: + ( + organization, + user, + slack_team_identity, + slack_user_identity, + ) = make_organization_and_user_with_slack_identities() + integration = make_alert_receive_channel(organization) + alert_group = make_alert_group(integration) + + channel = "potato" + ts = 88945.4849 + thread_ts = 16789.123 + + make_slack_message(alert_group, slack_id=thread_ts, channel_id=channel) + + mock_permalink = "http://example.com" + + step = SlackChannelMessageEventStep(slack_team_identity, organization, user) + step._slack_client = Mock() + step._slack_client.api_call.side_effect = [{"permalink": mock_permalink}, None] + + payload = { + "event": { + "channel": channel, + "ts": ts, + "thread_ts": thread_ts, + "text": "h" * 2901, + }, + } + + step.save_thread_message_for_resolution_note(slack_user_identity, payload) + + step._slack_client.api_call.assert_has_calls( + [ + call( + "chat.getPermalink", + channel=payload["event"]["channel"], + message_ts=payload["event"]["ts"], + ), + call( + "chat.postEphemeral", + channel=payload["event"]["channel"], + user=slack_user_identity.slack_id, + text=":warning: Unable to show the <{}|message> in Resolution Note: the message is too long ({}). " + "Max length - 2900 symbols.".format(mock_permalink, len(payload["event"]["text"])), + ), + ] + ) + MockResolutionNoteSlackMessage.objects.get_or_create.assert_not_called() + + @pytest.mark.parametrize("resolution_note_slack_message_already_exists", [True, False]) + def test_save_thread_message_for_resolution_note( + self, + make_organization_and_user_with_slack_identities, + make_alert_receive_channel, + make_alert_group, + make_slack_message, + make_resolution_note_slack_message, + resolution_note_slack_message_already_exists, + ) -> None: + ( + organization, + user, + slack_team_identity, + slack_user_identity, + ) = make_organization_and_user_with_slack_identities() + integration = make_alert_receive_channel(organization) + alert_group = make_alert_group(integration) + + original_text = "original text" + new_text = "new text" + + channel = "potato" + ts = 88945.4849 + thread_ts = 16789.123 + + make_slack_message(alert_group, slack_id=thread_ts, channel_id=channel) + + resolution_note_slack_message = None + if resolution_note_slack_message_already_exists: + resolution_note_slack_message = make_resolution_note_slack_message( + alert_group, user, user, ts=ts, thread_ts=thread_ts, text=original_text + ) + + mock_permalink = "http://example.com" + + step = SlackChannelMessageEventStep(slack_team_identity, organization, user) + step._slack_client = Mock() + step._slack_client.api_call.side_effect = [{"permalink": mock_permalink}, None] + + payload = { + "event": { + "channel": channel, + "ts": ts, + "thread_ts": thread_ts, + "text": new_text, + }, + } + + step.save_thread_message_for_resolution_note(slack_user_identity, payload) + + step._slack_client.api_call.assert_has_calls( + [ + call( + "chat.getPermalink", + channel=payload["event"]["channel"], + message_ts=payload["event"]["ts"], + ), + ] + ) + + if resolution_note_slack_message_already_exists: + resolution_note_slack_message.refresh_from_db() + resolution_note_slack_message.text = new_text + else: + assert ( + ResolutionNoteSlackMessage.objects.filter( + ts=ts, + thread_ts=thread_ts, + alert_group=alert_group, + ).count() + == 1 + ) + + @patch("apps.alerts.models.ResolutionNoteSlackMessage") + def test_delete_thread_message_from_resolution_note_no_slack_user_identity( + self, MockResolutionNoteSlackMessage, make_organization_and_user_with_slack_identities + ) -> None: + ( + organization, + user, + slack_team_identity, + slack_user_identity, + ) = make_organization_and_user_with_slack_identities() + + step = SlackChannelMessageEventStep(slack_team_identity, organization, user) + step.delete_thread_message_from_resolution_note(None, {}) + + MockResolutionNoteSlackMessage.objects.get.assert_not_called() + + def test_delete_thread_message_from_resolution_note_no_message_found( + self, make_organization_and_user_with_slack_identities + ) -> None: + ( + organization, + user, + slack_team_identity, + slack_user_identity, + ) = make_organization_and_user_with_slack_identities() + + channel = "potato" + ts = 88945.4849 + thread_ts = 16789.123 + + payload = { + "event": { + "channel": channel, + "previous_message": { + "ts": ts, + "thread_ts": thread_ts, + }, + }, + } + + step = SlackChannelMessageEventStep(slack_team_identity, organization, user) + step.alert_group_slack_service = Mock() + + step.delete_thread_message_from_resolution_note(slack_user_identity, payload) + + step.alert_group_slack_service.assert_not_called() + + def test_delete_thread_message_from_resolution_note( + self, + make_organization_and_user_with_slack_identities, + make_alert_receive_channel, + make_alert_group, + make_resolution_note_slack_message, + ) -> None: + ( + organization, + user, + slack_team_identity, + slack_user_identity, + ) = make_organization_and_user_with_slack_identities() + integration = make_alert_receive_channel(organization) + alert_group = make_alert_group(integration) + + channel = "potato" + ts = 88945.4849 + thread_ts = 16789.123 + + payload = { + "event": { + "channel": channel, + "previous_message": { + "ts": ts, + "thread_ts": thread_ts, + }, + }, + } + + make_resolution_note_slack_message( + alert_group, user, user, ts=ts, thread_ts=thread_ts, slack_channel_id=channel + ) + + step = SlackChannelMessageEventStep(slack_team_identity, organization, user) + step.alert_group_slack_service = Mock() + + step.delete_thread_message_from_resolution_note(slack_user_identity, payload) + + step.alert_group_slack_service.update_alert_group_slack_message.assert_called_once_with(alert_group) + assert ( + ResolutionNoteSlackMessage.objects.filter( + ts=ts, + thread_ts=thread_ts, + slack_channel_id=channel, + ).count() + == 0 + )