Skip to content

Commit bfc78e5

Browse files
feat(nimbus): Handle emojis added reaction (#14927)
### Because In production, the enrollment ending notification was failing with a Slack API error already has that reaction. The root cause is that `send_threaded_success_message()` is being called multiple times for the same enrollment ending request - either due to Kinto syncs running multiple times, task retries, or periodic checks. Each time it's called, it attempts to add the white_check_mark reaction to the original message. On subsequent calls, the reaction already exists, causing the API to reject the request with `already_reacted`. Previously, any Slack API error (including `already_reacted`) was treated as a fatal failure, logging the error and returning False. This caused the entire notification flow to fail, even though the actual desired state (the message exists and the reaction is on it) was already achieved. ### This commit Adds graceful error handling for the `already_reacted` Slack error. When `reactions_add` fails with `already_reacted`, the error is caught and ignored since the desired end state is already met - the reaction exists on the message. Other Slack API errors are still re-raised and properly logged as failures. Additionally, this commit adds the underlying support for enrollment ending and experiment ending request notifications with dedicated alert types (`END_ENROLLMENT_REQUEST` and `END_EXPERIMENT_REQUEST`), consolidates duplicate Slack notification logic, and adds comprehensive tests to verify the fix works correctly. Fixes #14921
1 parent c7aa70c commit bfc78e5

3 files changed

Lines changed: 69 additions & 5 deletions

File tree

experimenter/experimenter/slack/constants.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,10 @@ class EmojiReaction:
4949
CANCEL = "x"
5050
APPROVE = "eyes"
5151

52+
# Slack API error codes
53+
class ErrorCode:
54+
ALREADY_REACTED = "already_reacted"
55+
5256
# Slack message templates
5357
SLACK_DM_PREFIX = "🔔 Join {channel} to get slack notifications: {message}"
5458
SLACK_DM_CHANNEL_LINK_SUFFIX = "\n\n🔗 View in channel: {channel_message_link}"

experimenter/experimenter/slack/notification.py

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -193,11 +193,18 @@ def send_threaded_success_message(
193193
channel_id = post_response["channel"]
194194

195195
# Add reaction emoji to original message
196-
client.reactions_add(
197-
channel=channel_id,
198-
name="white_check_mark",
199-
timestamp=thread_ts,
200-
)
196+
try:
197+
client.reactions_add(
198+
channel=channel_id,
199+
name="white_check_mark",
200+
timestamp=thread_ts,
201+
)
202+
except SlackApiError as emoji_error:
203+
if (
204+
emoji_error.response.get("error")
205+
!= SlackConstants.ErrorCode.ALREADY_REACTED
206+
):
207+
raise
201208

202209
logger.info(success_log_message(experiment.slug))
203210
return True

experimenter/experimenter/slack/tests/test_notification.py

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -608,6 +608,59 @@ def test_send_experiment_launch_success_message_slack_error(self, mock_webclient
608608
self.assertFalse(result)
609609
mock_client.chat_postMessage.assert_called_once()
610610

611+
@override_settings(
612+
SLACK_AUTH_TOKEN="test-token",
613+
SLACK_NIMBUS_CHANNEL="C123456",
614+
)
615+
@patch("experimenter.slack.notification.WebClient")
616+
def test_send_experiment_launch_success_message_reaction_already_exists(
617+
self, mock_webclient
618+
):
619+
mock_client = Mock()
620+
mock_webclient.return_value = mock_client
621+
mock_client.chat_postMessage.return_value = {"ok": True, "channel": "C123456"}
622+
# Simulate reaction already exists error
623+
mock_client.reactions_add.side_effect = SlackApiError(
624+
message="Slack error",
625+
response={"ok": False, "error": SlackConstants.ErrorCode.ALREADY_REACTED},
626+
)
627+
628+
thread_ts = "1234567890.123456"
629+
result = send_threaded_success_message(
630+
experiment_id=self.experiment.id,
631+
thread_ts=thread_ts,
632+
message_template=SlackConstants.SLACK_LAUNCH_SUCCESS_MESSAGE,
633+
log_operation=SlackConstants.SLACK_OPERATION_LAUNCH_SUCCESS,
634+
success_log_message=lambda slug: f"Sent launch success message for {slug}",
635+
)
636+
637+
self.assertTrue(result)
638+
mock_client.chat_postMessage.assert_called_once()
639+
mock_client.reactions_add.assert_called_once()
640+
641+
@override_settings(
642+
SLACK_AUTH_TOKEN="test-token",
643+
SLACK_NIMBUS_CHANNEL="C123456",
644+
)
645+
@patch("experimenter.slack.notification.WebClient")
646+
def test_send_experiment_launch_success_message_reaction_error(self, mock_webclient):
647+
mock_client = Mock()
648+
mock_webclient.return_value = mock_client
649+
mock_client.chat_postMessage.return_value = {"ok": True, "channel": "C123456"}
650+
mock_client.reactions_add.side_effect = SlackApiError(
651+
message="Slack error", response={"ok": False, "error": "channel_not_found"}
652+
)
653+
654+
thread_ts = "1234567890.123456"
655+
result = send_threaded_success_message(
656+
experiment_id=self.experiment.id,
657+
thread_ts=thread_ts,
658+
message_template=SlackConstants.SLACK_LAUNCH_SUCCESS_MESSAGE,
659+
log_operation=SlackConstants.SLACK_OPERATION_LAUNCH_SUCCESS,
660+
success_log_message=lambda slug: f"Sent launch success message for {slug}",
661+
)
662+
self.assertFalse(result)
663+
611664
@override_settings(SLACK_AUTH_TOKEN="test-token")
612665
@patch("experimenter.slack.notification.WebClient")
613666
def test_send_experiment_launch_success_message_not_found(self, mock_webclient):

0 commit comments

Comments
 (0)