diff --git a/CHANGELOG.md b/CHANGELOG.md index 16348546..23aac416 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +## v1.3.38 (2023-09-19) + +### Fixed + +- Fix Slack access token length issue by @toolchainX ([#3016](https://github.com/grafana/oncall/pull/3016)) +- Fix shifts for current user internal endpoint to return the right shift PK ([#3036](https://github.com/grafana/oncall/pull/3036)) +- Handle Slack ratelimit on alert group deletion by @vadimkerr ([#3038](https://github.com/grafana/oncall/pull/3038)) + ## v1.3.37 (2023-09-12) ### Added diff --git a/dev/README.md b/dev/README.md index af3ecc71..30c317e3 100644 --- a/dev/README.md +++ b/dev/README.md @@ -1,6 +1,6 @@ # Developer quickstart -- [Developing using kubernetes (beta)](#developing-using-kubernetes-beta) +- [Developing using kubernetes and tilt (beta)](#developing-using-kubernetes-and-tilt-beta) - [Running the project with docker-compose](#running-the-project-with-docker-compose) - [`COMPOSE_PROFILES`](#compose_profiles) - [`GRAFANA_IMAGE`](#grafana_image) @@ -28,7 +28,9 @@ Related: [How to develop integrations](/engine/config_integrations/README.md) -## Developing using kubernetes (beta) +## Developing using kubernetes and tilt (beta) + +> It is recommended to use alternative [docker-compose environment](#running-the-project-with-docker-compose). ### Install @@ -37,26 +39,31 @@ This project uses: - [Tilt | Kubernetes for Prod, Tilt for Dev](https://tilt.dev/) - [tilt-dev/ctlptl: Making local Kubernetes clusters fun and easy to set up](https://github.com/tilt-dev/ctlptl) - [Kind](https://kind.sigs.k8s.io) +- [Yarn](https://classic.yarnpkg.com/lang/en/docs/install/#mac-stable) ### Quick Start -Create local k8s cluster: +1. Create local k8s cluster: -```bash -make cluster/up -``` + ```bash + make cluster/up + ``` -Deploy the project: +2. Deploy the project: -```bash -tilt up -``` + ```bash + tilt up + ``` -Clean up the project by deleting the local k8s cluster: +3. Wait until all resources are green and open -```bash -make cluster/down -``` +4. Modify source code, backend and frontend will be hot reloaded + +5. Clean up the project by deleting the local k8s cluster: + + ```bash + make cluster/down + ``` ## Running the project with docker-compose diff --git a/docs/make-docs b/docs/make-docs index 60759480..97dcbf5b 100755 --- a/docs/make-docs +++ b/docs/make-docs @@ -6,6 +6,12 @@ # [Semantic versioning](https://semver.org/) is used to help the reader identify the significance of changes. # Changes are relevant to this script and the support docs.mk GNU Make interface. +# ## 4.2.1 (2023-09-13) + +# ## Fixed + +# - Improved consistency of the webserver request loop by polling the Hugo port rather than the proxy port. + # ## 4.2.0 (2023-09-01) # ### Added @@ -469,7 +475,7 @@ POSIX_HERESTRING fi done echo - echo 'Press Ctrl+C to stop the server' + echo 'Press Ctrl+c to stop the server' unset i max req url return @@ -478,6 +484,8 @@ POSIX_HERESTRING echo errr 'The build was interrupted or a build error occurred, check the previous logs for possible causes.' + note 'You might need to use Ctrl+c to end the process.' + unset i max req url } @@ -626,7 +634,7 @@ ${PODMAN} run \ ${DOCS_IMAGE} \ /entrypoint EOF - await_build http://localhost:3002 & + await_build http://localhost:3003 & if [ -n "${DEBUG}" ]; then ${cmd} diff --git a/engine/apps/alerts/tasks/delete_alert_group.py b/engine/apps/alerts/tasks/delete_alert_group.py index 452fe3fc..f8a1d2ae 100644 --- a/engine/apps/alerts/tasks/delete_alert_group.py +++ b/engine/apps/alerts/tasks/delete_alert_group.py @@ -1,6 +1,7 @@ from celery.utils.log import get_task_logger from django.conf import settings +from apps.slack.errors import SlackAPIRatelimitError from common.custom_celery_tasks import shared_dedicated_queue_retry_task logger = get_task_logger(__name__) @@ -23,4 +24,8 @@ def delete_alert_group(alert_group_pk, user_pk): logger.debug("User not found, skipping delete_alert_group") return - alert_group.delete_by_user(user) + try: + alert_group.delete_by_user(user) + except SlackAPIRatelimitError as e: + # Handle Slack API ratelimit raised in apps.slack.scenarios.distribute_alerts.DeleteGroupStep.process_signal + delete_alert_group.apply_async((alert_group_pk, user_pk), countdown=e.retry_after) diff --git a/engine/apps/alerts/tests/test_alert_group.py b/engine/apps/alerts/tests/test_alert_group.py index 450665e7..5748c44f 100644 --- a/engine/apps/alerts/tests/test_alert_group.py +++ b/engine/apps/alerts/tests/test_alert_group.py @@ -1,11 +1,14 @@ -from unittest.mock import patch +from unittest.mock import call, patch import pytest from apps.alerts.incident_appearance.renderers.phone_call_renderer import AlertGroupPhoneCallRenderer from apps.alerts.models import AlertGroup, AlertGroupLogRecord from apps.alerts.tasks.delete_alert_group import delete_alert_group +from apps.slack.client import SlackClient +from apps.slack.errors import SlackAPIMessageNotFoundError, SlackAPIRatelimitError from apps.slack.models import SlackMessage +from apps.slack.tests.conftest import build_slack_response @pytest.mark.django_db @@ -46,56 +49,180 @@ def test_render_for_phone_call( assert expected_verbose_name in rendered_text +@patch.object(SlackClient, "reactions_remove") +@patch.object(SlackClient, "chat_delete") @pytest.mark.django_db def test_delete( + mock_chat_delete, + mock_reactions_remove, make_organization_with_slack_team_identity, make_user, - make_slack_channel, make_alert_receive_channel, make_alert_group, make_alert, + make_slack_message, + make_resolution_note_slack_message, ): """test alert group deleting""" organization, slack_team_identity = make_organization_with_slack_team_identity() - slack_channel = make_slack_channel(slack_team_identity, name="general", slack_id="CWER1ASD") user = make_user(organization=organization) alert_receive_channel = make_alert_receive_channel(organization) alert_group = make_alert_group(alert_receive_channel) - SlackMessage.objects.create(channel_id="CWER1ASD", alert_group=alert_group) + make_alert(alert_group, raw_request_data={}) - make_alert( - alert_group, - raw_request_data={ - "evalMatches": [ - {"value": 100, "metric": "High value", "tags": None}, - {"value": 200, "metric": "Higher Value", "tags": None}, - ], - "message": "Someone is testing the alert notification within grafana.", - "ruleId": 0, - "ruleName": "Test notification", - "ruleUrl": "http://localhost:3000/", - "state": "alerting", - "title": f"Incident for channel <#{slack_channel.slack_id}> Where a > b & c < d", - }, + # Create Slack messages + slack_message = make_slack_message(alert_group=alert_group, channel_id="test_channel_id", slack_id="test_slack_id") + resolution_note_1 = make_resolution_note_slack_message( + alert_group=alert_group, + user=user, + added_by_user=user, + posted_by_bot=True, + slack_channel_id="test1_channel_id", + ts="test1_ts", + ) + resolution_note_2 = make_resolution_note_slack_message( + alert_group=alert_group, + user=user, + added_by_user=user, + added_to_resolution_note=True, + slack_channel_id="test2_channel_id", + ts="test2_ts", ) - alerts = alert_group.alerts - slack_messages = alert_group.slack_messages - - assert alerts.count() > 0 - assert slack_messages.count() > 0 + assert alert_group.alerts.count() == 1 + assert alert_group.slack_messages.count() == 1 + assert alert_group.resolution_note_slack_messages.count() == 2 delete_alert_group(alert_group.pk, user.pk) - assert alerts.count() == 0 - assert slack_messages.count() == 0 + assert not alert_group.alerts.exists() + assert not alert_group.slack_messages.exists() + assert not alert_group.resolution_note_slack_messages.exists() with pytest.raises(AlertGroup.DoesNotExist): alert_group.refresh_from_db() + # Check that appropriate Slack API calls are made + assert mock_chat_delete.call_count == 2 + assert mock_chat_delete.call_args_list[0] == call( + channel=resolution_note_1.slack_channel_id, ts=resolution_note_1.ts + ) + assert mock_chat_delete.call_args_list[1] == call(channel=slack_message.channel_id, ts=slack_message.slack_id) + mock_reactions_remove.assert_called_once_with( + channel=resolution_note_2.slack_channel_id, name="memo", timestamp=resolution_note_2.ts + ) + + +@pytest.mark.parametrize("api_method", ["reactions_remove", "chat_delete"]) +@patch.object(delete_alert_group, "apply_async") +@pytest.mark.django_db +def test_delete_slack_ratelimit( + mock_delete_alert_group, + api_method, + make_organization_with_slack_team_identity, + make_user, + make_alert_receive_channel, + make_alert_group, + make_alert, + make_slack_message, + make_resolution_note_slack_message, +): + organization, slack_team_identity = make_organization_with_slack_team_identity() + user = make_user(organization=organization) + + alert_receive_channel = make_alert_receive_channel(organization) + + alert_group = make_alert_group(alert_receive_channel) + make_alert(alert_group, raw_request_data={}) + + # Create Slack messages + make_slack_message(alert_group=alert_group, channel_id="test_channel_id", slack_id="test_slack_id") + make_resolution_note_slack_message( + alert_group=alert_group, + user=user, + added_by_user=user, + posted_by_bot=True, + slack_channel_id="test1_channel_id", + ts="test1_ts", + ) + make_resolution_note_slack_message( + alert_group=alert_group, + user=user, + added_by_user=user, + added_to_resolution_note=True, + slack_channel_id="test2_channel_id", + ts="test2_ts", + ) + + with patch.object( + SlackClient, + api_method, + side_effect=SlackAPIRatelimitError( + response=build_slack_response({"ok": False, "error": "ratelimited"}, headers={"Retry-After": 42}) + ), + ): + delete_alert_group(alert_group.pk, user.pk) + + # Check task is retried gracefully + mock_delete_alert_group.assert_called_once_with((alert_group.pk, user.pk), countdown=42) + + +@pytest.mark.parametrize("api_method", ["reactions_remove", "chat_delete"]) +@patch.object(delete_alert_group, "apply_async") +@pytest.mark.django_db +def test_delete_slack_api_error_other_than_ratelimit( + mock_delete_alert_group, + api_method, + make_organization_with_slack_team_identity, + make_user, + make_alert_receive_channel, + make_alert_group, + make_alert, + make_slack_message, + make_resolution_note_slack_message, +): + organization, slack_team_identity = make_organization_with_slack_team_identity() + user = make_user(organization=organization) + + alert_receive_channel = make_alert_receive_channel(organization) + + alert_group = make_alert_group(alert_receive_channel) + make_alert(alert_group, raw_request_data={}) + + # Create Slack messages + make_slack_message(alert_group=alert_group, channel_id="test_channel_id", slack_id="test_slack_id") + make_resolution_note_slack_message( + alert_group=alert_group, + user=user, + added_by_user=user, + posted_by_bot=True, + slack_channel_id="test1_channel_id", + ts="test1_ts", + ) + make_resolution_note_slack_message( + alert_group=alert_group, + user=user, + added_by_user=user, + added_to_resolution_note=True, + slack_channel_id="test2_channel_id", + ts="test2_ts", + ) + + with patch.object( + SlackClient, + api_method, + side_effect=SlackAPIMessageNotFoundError( + response=build_slack_response({"ok": False, "error": "message_not_found"}) + ), + ): + delete_alert_group(alert_group.pk, user.pk) # check no exception is raised + + # Check task is not retried + mock_delete_alert_group.assert_not_called() + @pytest.mark.django_db def test_alerts_count_gt( diff --git a/engine/apps/api/tests/test_schedules.py b/engine/apps/api/tests/test_schedules.py index f5ffb8ce..50e6d508 100644 --- a/engine/apps/api/tests/test_schedules.py +++ b/engine/apps/api/tests/test_schedules.py @@ -2115,8 +2115,8 @@ def test_current_user_events( shifts = ( # schedule, user, priority, start time (h), duration (seconds) - (schedule_with_current_user, current_user, 1, 0, (24 * 60 * 60) - 1), # r1-1: 0-23:59:59 (other_schedule, other_user, 1, 0, (24 * 60 * 60) - 1), # r1-1: 0-23:59:59 + (schedule_with_current_user, current_user, 1, 0, (24 * 60 * 60) - 1), # r1-1: 0-23:59:59 ) now = timezone.now() today = now.replace(hour=0, minute=0, second=0, microsecond=0) @@ -2146,6 +2146,9 @@ def test_current_user_events( assert result["schedules"][0]["id"] == schedule_with_current_user.public_primary_key assert result["schedules"][0]["name"] == schedule_with_current_user.name assert len(result["schedules"][0]["events"]) > 0 + for event in result["schedules"][0]["events"]: + # check the current user shift pk is set in the event + assert event["shift"]["pk"] == on_call_shift.public_primary_key @pytest.mark.django_db diff --git a/engine/apps/api_for_grafana_incident/serializers.py b/engine/apps/api_for_grafana_incident/serializers.py index eda49c90..5cca29c3 100644 --- a/engine/apps/api_for_grafana_incident/serializers.py +++ b/engine/apps/api_for_grafana_incident/serializers.py @@ -2,7 +2,9 @@ import logging from rest_framework import serializers +from apps.alerts.incident_appearance.renderers.web_renderer import AlertGroupWebRenderer from apps.alerts.models import Alert, AlertGroup +from apps.api.serializers.alert_group import AlertGroupFieldsCacheSerializerMixin logger = logging.getLogger(__name__) @@ -26,9 +28,22 @@ class AlertGroupSerializer(serializers.ModelSerializer): title = serializers.CharField(read_only=True, source="long_verbose_name_without_formatting") alerts = AlertSerializer(many=True, read_only=True) + render_for_web = serializers.SerializerMethodField() + def get_status(self, obj): return next(filter(lambda status: status[0] == obj.status, AlertGroup.STATUS_CHOICES))[1].lower() + def get_render_for_web(self, obj): + last_alert = obj.alerts.last() + if last_alert is None: + return {} + return AlertGroupFieldsCacheSerializerMixin.get_or_set_web_template_field( + obj, + last_alert, + AlertGroupFieldsCacheSerializerMixin.RENDER_FOR_WEB_FIELD_NAME, + AlertGroupWebRenderer, + ) + class Meta: model = AlertGroup fields = [ @@ -37,4 +52,5 @@ class AlertGroupSerializer(serializers.ModelSerializer): "status", "alerts", "title", + "render_for_web", ] diff --git a/engine/apps/api_for_grafana_incident/tests/__init__.py b/engine/apps/api_for_grafana_incident/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/engine/apps/api_for_grafana_incident/tests/test_views.py b/engine/apps/api_for_grafana_incident/tests/test_views.py new file mode 100644 index 00000000..0b270245 --- /dev/null +++ b/engine/apps/api_for_grafana_incident/tests/test_views.py @@ -0,0 +1,50 @@ +import pytest +from django.urls import reverse +from rest_framework.test import APIClient + + +@pytest.mark.django_db +def test_alert_group_details( + make_organization, + make_alert_receive_channel, + make_alert_group, + make_alert, + settings, +): + settings.GRAFANA_INCIDENT_STATIC_API_KEY = "test-key" + headers = {"HTTP_AUTHORIZATION": "test-key"} + organization = make_organization() + alert_receive_channel = make_alert_receive_channel( + organization, + slack_title_template=None, + web_title_template="title: {{ payload.field2 }}", + web_message_template="Something {{ payload.field1 }} + {{ payload.field3 }}", + web_image_url_template="http://{{ payload.field1 }}", + ) + alert_group = make_alert_group(alert_receive_channel) + alert_payload = {"field1": "foo", "field2": "bar", "field3": "baz"} + alert = make_alert(alert_group, alert_payload) + + client = APIClient() + + url = reverse("api-gi:alert-groups-detail", kwargs={"public_primary_key": alert_group.public_primary_key}) + response = client.get(url, format="json", **headers) + expected = { + "id": alert_group.public_primary_key, + "link": alert_group.web_link, + "status": "new", + "title": alert_group.long_verbose_name_without_formatting, + "alerts": [ + { + "id_oncall": alert.public_primary_key, + "payload": alert_payload, + } + ], + "render_for_web": { + "title": "title: bar", + "message": "

Something foo + baz

", + "image_url": "http://foo", + "source_link": None, + }, + } + assert response.json() == expected diff --git a/engine/apps/schedules/constants.py b/engine/apps/schedules/constants.py index 317a609f..818ddeb1 100644 --- a/engine/apps/schedules/constants.py +++ b/engine/apps/schedules/constants.py @@ -18,6 +18,7 @@ ICAL_STATUS = "STATUS" ICAL_STATUS_CANCELLED = "CANCELLED" ICAL_COMPONENT_VEVENT = "VEVENT" RE_PRIORITY = re.compile(r"^\[L(\d+)\]") +RE_EVENT_UID_EXPORT = re.compile(r"([\w\d]+)-(\d+)-([\w\d]+)") RE_EVENT_UID_V1 = re.compile(r"amixr-([\w\d-]+)-U(\d+)-E(\d+)-S(\d+)") RE_EVENT_UID_V2 = re.compile(r"oncall-([\w\d-]+)-PK([\w\d]+)-U(\d+)-E(\d+)-S(\d+)") diff --git a/engine/apps/schedules/ical_utils.py b/engine/apps/schedules/ical_utils.py index 5f3c82fa..5a487677 100644 --- a/engine/apps/schedules/ical_utils.py +++ b/engine/apps/schedules/ical_utils.py @@ -29,6 +29,7 @@ from apps.schedules.constants import ( ICAL_STATUS_CANCELLED, ICAL_SUMMARY, ICAL_UID, + RE_EVENT_UID_EXPORT, RE_EVENT_UID_V1, RE_EVENT_UID_V2, RE_PRIORITY, @@ -421,21 +422,25 @@ def parse_event_uid(string: str, sequence: str = None, recurrence_id: str = None if match: _, pk, _, _, source = match.groups() else: - # eventually this path would be automatically deprecated - # once all ical representations are refreshed - match = RE_EVENT_UID_V1.match(string) + match = RE_EVENT_UID_EXPORT.match(string) if match: - _, _, _, source = match.groups() + pk, _, _ = match.groups() else: - # fallback to use the UID string as the rotation ID - pk = string - # in ical imported calendars, sequence and/or recurrence_id - # distinguish main recurring event vs instance modification - # (see https://icalendar.org/iCalendar-RFC-5545/3-8-4-4-recurrence-id.html) - if sequence: - pk = f"{pk}_{sequence}" - if recurrence_id: - pk = f"{pk}_{recurrence_id}" + # eventually this path would be automatically deprecated + # once all ical representations are refreshed + match = RE_EVENT_UID_V1.match(string) + if match: + _, _, _, source = match.groups() + else: + # fallback to use the UID string as the rotation ID + pk = string + # in ical imported calendars, sequence and/or recurrence_id + # distinguish main recurring event vs instance modification + # (see https://icalendar.org/iCalendar-RFC-5545/3-8-4-4-recurrence-id.html) + if sequence: + pk = f"{pk}_{sequence}" + if recurrence_id: + pk = f"{pk}_{recurrence_id}" if source is not None: source = int(source) diff --git a/engine/apps/schedules/tests/test_ical_utils.py b/engine/apps/schedules/tests/test_ical_utils.py index c6520c96..77368fe3 100644 --- a/engine/apps/schedules/tests/test_ical_utils.py +++ b/engine/apps/schedules/tests/test_ical_utils.py @@ -317,6 +317,15 @@ def test_shifts_dict_from_cached_final( assert shifts == expected_events +def test_parse_event_uid_from_export(): + shift_pk = "OUCE6WAHL35PP" + user_pk = "UHZ38D6AQXXBY" + event_uid = f"{shift_pk}-202309200300-{user_pk}" + pk, source = parse_event_uid(event_uid) + assert pk == shift_pk + assert source is None + + def test_parse_event_uid_v1(): uuid = uuid4() event_uid = f"amixr-{uuid}-U1-E2-S1" diff --git a/engine/apps/slack/migrations/0004_auto_20230913_1020.py b/engine/apps/slack/migrations/0004_auto_20230913_1020.py new file mode 100644 index 00000000..9e58ced1 --- /dev/null +++ b/engine/apps/slack/migrations/0004_auto_20230913_1020.py @@ -0,0 +1,23 @@ +# Generated by Django 3.2.20 on 2023-09-13 10:20 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('slack', '0003_delete_slackactionrecord'), + ] + + operations = [ + migrations.AlterField( + model_name='slackteamidentity', + name='access_token', + field=models.CharField(default=None, max_length=255, null=True), + ), + migrations.AlterField( + model_name='slackteamidentity', + name='bot_access_token', + field=models.CharField(default=None, max_length=255, null=True), + ), + ] diff --git a/engine/apps/slack/models/slack_team_identity.py b/engine/apps/slack/models/slack_team_identity.py index 60d8b19a..6fe09320 100644 --- a/engine/apps/slack/models/slack_team_identity.py +++ b/engine/apps/slack/models/slack_team_identity.py @@ -31,9 +31,9 @@ class SlackTeamIdentity(models.Model): slack_id = models.CharField(max_length=100) cached_name = models.CharField(max_length=100, null=True, default=None) cached_app_id = models.CharField(max_length=100, null=True, default=None) - access_token = models.CharField(max_length=100, null=True, default=None) + access_token = models.CharField(max_length=255, null=True, default=None) bot_user_id = models.CharField(max_length=100, null=True, default=None) - bot_access_token = models.CharField(max_length=100, null=True, default=None) + bot_access_token = models.CharField(max_length=255, null=True, default=None) oauth_scope = models.TextField(max_length=30000, null=True, default=None) detected_token_revoked = models.DateTimeField(null=True, default=None, verbose_name="Deleted At") is_profile_populated = models.BooleanField(default=False) diff --git a/engine/apps/slack/scenarios/distribute_alerts.py b/engine/apps/slack/scenarios/distribute_alerts.py index 1718ce43..3f551025 100644 --- a/engine/apps/slack/scenarios/distribute_alerts.py +++ b/engine/apps/slack/scenarios/distribute_alerts.py @@ -880,34 +880,38 @@ class DeleteGroupStep(scenario_step.ScenarioStep): def process_signal(self, log_record: AlertGroupLogRecord) -> None: alert_group = log_record.alert_group - self.remove_resolution_note_reaction(alert_group) - - bot_messages_ts: typing.List[str] = [] - bot_messages_ts.extend(alert_group.slack_messages.values_list("slack_id", flat=True)) - bot_messages_ts.extend( - alert_group.resolution_note_slack_messages.filter(posted_by_bot=True).values_list("ts", flat=True) - ) - channel_id = alert_group.slack_message.channel_id - - for message_ts in bot_messages_ts: - try: - self._slack_client.chat_delete(channel=channel_id, ts=message_ts) - except ( - SlackAPITokenError, - SlackAPIChannelNotFoundError, - SlackAPIMessageNotFoundError, - SlackAPIChannelArchivedError, - ): - pass - - def remove_resolution_note_reaction(self, alert_group: AlertGroup) -> None: + # Remove "memo" emoji from resolution note messages for message in alert_group.resolution_note_slack_messages.filter(added_to_resolution_note=True): - message.added_to_resolution_note = False - message.save(update_fields=["added_to_resolution_note"]) try: self._slack_client.reactions_remove(channel=message.slack_channel_id, name="memo", timestamp=message.ts) + except SlackAPIRatelimitError: + # retries on ratelimit are handled in apps.alerts.tasks.delete_alert_group.delete_alert_group + raise except SlackAPIError: pass + message.delete() + + # Remove resolution note messages posted by OnCall bot + for message in alert_group.resolution_note_slack_messages.filter(posted_by_bot=True): + try: + self._slack_client.chat_delete(channel=message.slack_channel_id, ts=message.ts) + except SlackAPIRatelimitError: + # retries on ratelimit are handled in apps.alerts.tasks.delete_alert_group.delete_alert_group + raise + except SlackAPIError: + pass + message.delete() + + # Remove alert group Slack messages + for message in alert_group.slack_messages.all(): + try: + self._slack_client.chat_delete(channel=message.channel_id, ts=message.slack_id) + except SlackAPIRatelimitError: + # retries on ratelimit are handled in apps.alerts.tasks.delete_alert_group.delete_alert_group + raise + except SlackAPIError: + pass + message.delete() class UpdateLogReportMessageStep(scenario_step.ScenarioStep): diff --git a/engine/apps/telegram/models/connectors/personal.py b/engine/apps/telegram/models/connectors/personal.py index a506f2fb..8dfdc6f7 100644 --- a/engine/apps/telegram/models/connectors/personal.py +++ b/engine/apps/telegram/models/connectors/personal.py @@ -4,11 +4,12 @@ from telegram import error from apps.alerts.models import AlertGroup from apps.base.models import UserNotificationPolicy, UserNotificationPolicyLogRecord from apps.telegram.client import TelegramClient +from apps.telegram.decorators import ignore_reply_to_message_deleted from apps.telegram.models import TelegramMessage, TelegramToOrganizationConnector from apps.telegram.tasks import send_link_to_channel_message_or_fallback_to_full_alert_group from apps.user_management.models import User -ONE_MORE_NOTIFICATION = "One more notification about this ☝" +ONE_MORE_NOTIFICATION = "One more notification about this 👆" ALERT_CANT_BE_RENDERED = ( "You have a new alert group, but Telegram can't render its content! Please check it out: {link}" ) @@ -123,11 +124,7 @@ class TelegramToUserConnector(models.Model): else: raise e else: - telegram_client.send_raw_message( - chat_id=old_alert_group_message.chat_id, - text=ONE_MORE_NOTIFICATION, - reply_to_message_id=old_alert_group_message.message_id, - ) + self._nudge_about_alert_group_message(telegram_client, old_alert_group_message) # send DM message with the link to the alert group post in channel def send_link_to_channel_message(self, alert_group: AlertGroup, notification_policy: UserNotificationPolicy): @@ -172,3 +169,11 @@ class TelegramToUserConnector(models.Model): ) else: raise e + + @staticmethod + @ignore_reply_to_message_deleted + def _nudge_about_alert_group_message(telegram_client: TelegramClient, message: TelegramMessage) -> None: + """Nudge the user about existing alert group message, without sending the full alert group content again.""" + telegram_client.send_raw_message( + chat_id=message.chat_id, reply_to_message_id=message.message_id, text=ONE_MORE_NOTIFICATION + ) diff --git a/engine/apps/telegram/tests/test_personal_connector.py b/engine/apps/telegram/tests/test_personal_connector.py new file mode 100644 index 00000000..b1ea07a0 --- /dev/null +++ b/engine/apps/telegram/tests/test_personal_connector.py @@ -0,0 +1,49 @@ +from unittest.mock import patch + +import pytest +from telegram import error + +from apps.base.models import UserNotificationPolicy +from apps.telegram.client import TelegramClient +from apps.telegram.models import TelegramMessage + + +@patch.object(TelegramClient, "send_raw_message", side_effect=error.BadRequest("Replied message not found")) +@pytest.mark.django_db +def test_personal_connector_replied_message_not_found( + mock_send_message, + make_organization_and_user, + make_telegram_user_connector, + make_user_notification_policy, + make_alert_receive_channel, + make_alert_group, + make_alert, + make_telegram_message, +): + # set up a user with Telegram account connected + organization, user = make_organization_and_user() + make_telegram_user_connector(user) + notification_policy = make_user_notification_policy( + user, + UserNotificationPolicy.Step.NOTIFY, + notify_by=UserNotificationPolicy.NotificationChannel.TELEGRAM, + important=False, + ) + + # create an alert group with an existing Telegram message in user's DM + alert_receive_channel = make_alert_receive_channel(organization) + alert_group = make_alert_group(alert_receive_channel) + make_alert(alert_group=alert_group, raw_request_data=alert_receive_channel.config.example_payload) + telegram_message = make_telegram_message( + alert_group=alert_group, + message_type=TelegramMessage.PERSONAL_MESSAGE, + chat_id=str(user.telegram_connection.telegram_chat_id), + ) + + # make sure no exception is raised when replying to the message that has been deleted + user.telegram_connection.notify(alert_group=alert_group, notification_policy=notification_policy) + mock_send_message.assert_called_once_with( + chat_id=telegram_message.chat_id, + text="One more notification about this 👆", + reply_to_message_id=telegram_message.message_id, + )