diff --git a/CHANGELOG.md b/CHANGELOG.md index 43d57fc7d9..5237128982 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/engine/apps/api/tests/test_live_settings.py b/engine/apps/api/tests/test_live_settings.py index 1cfecebdeb..dce8623677 100644 --- a/engine/apps/api/tests/test_live_settings.py +++ b/engine/apps/api/tests/test_live_settings.py @@ -5,6 +5,8 @@ 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") + ) diff --git a/engine/apps/api/views/live_setting.py b/engine/apps/api/views/live_setting.py index 02ca047cef..760e3f0ddb 100644 --- a/engine/apps/api/views/live_setting.py +++ b/engine/apps/api/views/live_setting.py @@ -70,9 +70,6 @@ def _post_update_hook(self, name, old_value): 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 diff --git a/engine/apps/base/models/live_setting.py b/engine/apps/base/models/live_setting.py index e6d1e70876..34600ddbd2 100644 --- a/engine/apps/base/models/live_setting.py +++ b/engine/apps/base/models/live_setting.py @@ -212,6 +212,7 @@ def populate_settings_if_needed(cls): 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 @@ def _get_setting_from_setting_file(setting_name): 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) diff --git a/engine/apps/base/utils.py b/engine/apps/base/utils.py index 98f593bf58..66f58e15d4 100644 --- a/engine/apps/base/utils.py +++ b/engine/apps/base/utils.py @@ -45,7 +45,7 @@ def __init__(self, live_setting): 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 @@ def _check_telegram_token(cls, telegram_token): @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)}" diff --git a/engine/apps/telegram/client.py b/engine/apps/telegram/client.py index 86e2de22fb..b5733bcaf7 100644 --- a/engine/apps/telegram/client.py +++ b/engine/apps/telegram/client.py @@ -39,6 +39,7 @@ def is_chat_member(self, chat_id: Union[int, str]) -> bool: 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