Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/meshapi/tests/test_slack_notification.py
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ def test_slack_notification_for_name_change_no_env_var(self, requests_mocker):

@requests_mock.Mocker()
@patch("meshapi.util.admin_notifications.SLACK_ADMIN_NOTIFICATIONS_WEBHOOK_URL", "https://mock-slack-url")
@patch("meshapi.util.admin_notifications.SLACK_ADMIN_NOTIFICATIONS_RETRY_COUNT", 2)
def test_slack_notification_for_name_change_slack_failure(self, requests_mocker):
member = Member(
name="Stacy Maidenname",
Expand All @@ -293,6 +294,7 @@ def test_slack_notification_for_name_change_slack_failure(self, requests_mocker)
)

self.assertEqual(requests_mocker.request_history[0].url, "https://mock-slack-url/")
self.assertEqual(len(requests_mocker.request_history), 3)

@requests_mock.Mocker()
@patch("meshapi.util.admin_notifications.SLACK_ADMIN_NOTIFICATIONS_WEBHOOK_URL", "https://mock-slack-url")
Expand Down
31 changes: 21 additions & 10 deletions src/meshapi/util/admin_notifications.py
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this code run synchronously anywhere that having an exponential backoff like this could be a problem? What if we sent the slack messages to the celery worker?

If we want to have slack messages sent during a Join Form processing, for example, this could be problematic.

Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import json
import logging
import os
import time
from typing import Optional, Sequence, Type

import requests
Expand All @@ -13,6 +14,8 @@
SLACK_ADMIN_NOTIFICATIONS_WEBHOOK_URL = os.environ.get("SLACK_ADMIN_NOTIFICATIONS_WEBHOOK_URL")
SITE_BASE_URL = os.environ.get("SITE_BASE_URL")

SLACK_ADMIN_NOTIFICATIONS_RETRY_COUNT = int(os.environ.get("SLACK_ADMIN_NOTIFICATIONS_RETRY_COUNT", 3))


def escape_slack_text(text: str) -> str:
return text.replace("↔", "<->").replace("&", "&amp;").replace("<", "&lt;").replace(">", "&gt;")
Expand Down Expand Up @@ -69,13 +72,21 @@
)
return

response = requests.post(SLACK_ADMIN_NOTIFICATIONS_WEBHOOK_URL, json=slack_message)

if raise_exception_on_failure:
response.raise_for_status()
elif response.status_code != 200:
logging.error(
f"Got HTTP {response.status_code} while sending slack notification to slack admin. "
f"HTTP response was {response.text}. Unable to notify admins of "
f"the following message: {slack_message}"
)
for i in range(0, SLACK_ADMIN_NOTIFICATIONS_RETRY_COUNT + 1):
response = requests.post(SLACK_ADMIN_NOTIFICATIONS_WEBHOOK_URL, json=slack_message)
try:
response.raise_for_status()
return
except requests.exceptions.RequestException as e:
if i == SLACK_ADMIN_NOTIFICATIONS_RETRY_COUNT:
if raise_exception_on_failure:
raise e
else:
logging.error(

Check warning on line 85 in src/meshapi/util/admin_notifications.py

View check run for this annotation

Codecov / codecov/patch

src/meshapi/util/admin_notifications.py#L85

Added line #L85 was not covered by tests
f"Got HTTP {response.status_code} while sending slack notification to slack admin. "
f"HTTP response was {response.text}. Unable to notify admins of "
f"the following message: {slack_message}"
)
else:
# Exponential backoff to try to avoid triggering Slack's rate limiting logic
time.sleep(2**i)