Fix Telegram ratelimit on live setting change (#2100)

# What this PR does

Fixes https://github.com/grafana/oncall/issues/1103, inspired by
https://github.com/grafana/oncall/pull/1934.

Makes sure that:
1. `LiveSettings.validate_settings` is only called once per update
request and not called for any individual LiveSetting instance save.
2. `telegram.Bot.set_webhook` is only called once per request when
changing `TELEGRAM_WEBHOOK_HOST`.


## Which issue(s) this PR fixes

https://github.com/grafana/oncall/issues/1103

## 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

@@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Fix + revert [#2057](https://github.com/grafana/oncall/pull/2057) which reverted a change which properly handles
   `Organization.DoesNotExist` exceptions for Slack events by @joeyorlando ([#TBD](https://github.com/grafana/oncall/pull/TBD))
+- Fix Telegram ratelimit on live setting change by @vadimkerr and @alexintech ([#2100](https://github.com/grafana/oncall/pull/2100))
 
 ## v1.2.39 (2023-06-06)
 

engine/apps/api/tests/test_live_settings.py

@@ -5,6 +5,8 @@ from django.urls import reverse
 from rest_framework.status import HTTP_200_OK
 from rest_framework.test import APIClient
 
+from apps.base.models import LiveSetting
+
 
 @pytest.mark.django_db
 def test_list_live_setting(
@@ -98,3 +100,63 @@ def test_live_settings_update_not_trigger_unpopulate_slack_identities(
     assert not mocked_unpopulate_task.called
 
     assert response.status_code == HTTP_200_OK
+
+
+@pytest.mark.django_db
+def test_live_settings_update_validate_settings_once(
+    make_organization_and_user_with_plugin_token,
+    make_user_auth_headers,
+    make_live_setting,
+    settings,
+):
+    """
+    Check that settings are validated only once per update.
+    """
+
+    settings.FEATURE_LIVE_SETTINGS_ENABLED = True
+
+    organization, user, token = make_organization_and_user_with_plugin_token()
+    LiveSetting.populate_settings_if_needed()
+    live_setting = LiveSetting.objects.get(name="EMAIL_HOST")  # random setting
+
+    client = APIClient()
+    url = reverse("api-internal:live_settings-detail", kwargs={"pk": live_setting.public_primary_key})
+    data = {"id": live_setting.public_primary_key, "value": "TEST_UPDATED_VALUE", "name": "EMAIL_HOST"}
+
+    with mock.patch.object(LiveSetting, "validate_settings") as mock_validate_settings:
+        response = client.put(url, data=data, format="json", **make_user_auth_headers(user, token))
+
+    assert response.status_code == HTTP_200_OK
+    mock_validate_settings.assert_called_once()
+
+
+@pytest.mark.django_db
+def test_live_settings_telegram_calls_set_webhook_once(
+    make_organization_and_user_with_plugin_token,
+    make_user_auth_headers,
+    make_live_setting,
+    settings,
+):
+    """
+    Check that when TELEGRAM_WEBHOOK_HOST live setting is updated, set_webhook method is called only once.
+    If set_webhook is called more than once in a short period of time, there will be a rate limit error.
+    """
+
+    settings.FEATURE_LIVE_SETTINGS_ENABLED = True
+
+    organization, user, token = make_organization_and_user_with_plugin_token()
+    LiveSetting.populate_settings_if_needed()
+    live_setting = LiveSetting.objects.get(name="TELEGRAM_WEBHOOK_HOST")
+
+    client = APIClient()
+    url = reverse("api-internal:live_settings-detail", kwargs={"pk": live_setting.public_primary_key})
+    data = {"id": live_setting.public_primary_key, "value": "TEST_UPDATED_VALUE", "name": "TELEGRAM_WEBHOOK_HOST"}
+
+    with mock.patch("telegram.Bot.get_webhook_info", return_value=mock.Mock(url="TEST_VALUE")):
+        with mock.patch("telegram.Bot.set_webhook") as mock_set_webhook:
+            response = client.put(url, data=data, format="json", **make_user_auth_headers(user, token))
+
+    assert response.status_code == HTTP_200_OK
+    mock_set_webhook.assert_called_once_with(
+        "TEST_UPDATED_VALUE/telegram/", allowed_updates=("message", "callback_query")
+    )

engine/apps/api/views/live_setting.py

@@ -70,9 +70,6 @@ class LiveSettingViewSet(PublicPrimaryKeyMixin, viewsets.ModelViewSet):
             self._reset_telegram_integration(old_token=old_value)
             register_telegram_webhook.delay()
 
-        if name == "TELEGRAM_WEBHOOK_HOST":
-            register_telegram_webhook.delay()
-
         if name in ["SLACK_CLIENT_OAUTH_ID", "SLACK_CLIENT_OAUTH_SECRET"]:
             organization = self.request.auth.organization
             slack_team_identity = organization.slack_team_identity

engine/apps/base/models/live_setting.py

@@ -212,6 +212,7 @@ class LiveSetting(models.Model):
     def validate_settings(cls):
         settings_to_validate = cls.objects.all()
         for setting in settings_to_validate:
+            setting.error = LiveSettingValidator(live_setting=setting).get_error()
             setting.save(update_fields=["error"])
 
     @staticmethod
@@ -219,14 +220,9 @@ class LiveSetting(models.Model):
         return getattr(settings, setting_name)
 
     def save(self, *args, **kwargs):
-        """
-        Save validates LiveSettings values and save them in database
-        """
         if self.name not in self.AVAILABLE_NAMES:
             raise ValueError(
                 f"Setting with name '{self.name}' is not in list of available names {self.AVAILABLE_NAMES}"
             )
 
-        self.error = LiveSettingValidator(live_setting=self).get_error()
-
         super().save(*args, **kwargs)

engine/apps/base/utils.py

@@ -45,7 +45,7 @@ class LiveSettingValidator:
     def get_error(self):
         check_fn_name = f"_check_{self.live_setting.name.lower()}"
 
-        if self.live_setting.value is None and self.live_setting.name not in self.EMPTY_VALID_NAMES:
+        if self.live_setting.value in (None, "") and self.live_setting.name not in self.EMPTY_VALID_NAMES:
             return "Empty"
 
         # skip validation if there's no handler for it
@@ -138,9 +138,11 @@ class LiveSettingValidator:
     @classmethod
     def _check_telegram_webhook_host(cls, telegram_webhook_host):
         try:
+            # avoid circular import
+            from apps.telegram.client import TelegramClient
+
             url = create_engine_url("/telegram/", override_base=telegram_webhook_host)
-            bot = Bot(token=live_settings.TELEGRAM_TOKEN)
-            bot.set_webhook(url)
+            TelegramClient().register_webhook(url)
         except Exception as e:
             return f"Telegram error: {str(e)}"
 

engine/apps/telegram/client.py

@@ -39,6 +39,7 @@ class TelegramClient:
     def register_webhook(self, webhook_url: Optional[str] = None) -> None:
         webhook_url = webhook_url or create_engine_url("/telegram/", override_base=live_settings.TELEGRAM_WEBHOOK_HOST)
 
+        # avoid unnecessary set_webhook calls to make sure Telegram rate limits are not exceeded
         webhook_info = self.api_client.get_webhook_info()
         if webhook_info.url == webhook_url:
             return