Skip to content

Commit cfb7dbc

Browse files
feat(nimbus): Removes eyes and only show complete checkmark for the request (#15007)
Because - We show both eyes and checkmark emoji if the request is reviewed and approved, we should only show one emoji at a time, when request is pending-question mark, when it is under reviewing-eyes, when its done-checkmark This commit - Removes the eyes emoji when request is reviewed and approved, only shows the check mark emoji for that. Fixes #15001
1 parent 3e3d26a commit cfb7dbc

3 files changed

Lines changed: 28 additions & 4 deletions

File tree

experimenter/experimenter/slack/constants.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ class EmojiReaction:
4949
PENDING = "question"
5050
CANCEL = "x"
5151
APPROVE = "eyes"
52+
COMPLETE = "white_check_mark"
5253

5354
# Slack API error codes
5455
class ErrorCode:

experimenter/experimenter/slack/notification.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import contextlib
12
import logging
23

34
from django.conf import settings
@@ -192,11 +193,20 @@ def send_threaded_success_message(
192193

193194
channel_id = post_response["channel"]
194195

195-
# Add reaction emoji to original message
196+
# Remove eyes emoji (review done) and add checkmark (action complete)
197+
# Eyes emoji might not exist, so suppress any errors
198+
with contextlib.suppress(SlackApiError):
199+
client.reactions_remove(
200+
channel=channel_id,
201+
name=SlackConstants.EmojiReaction.APPROVE,
202+
timestamp=thread_ts,
203+
)
204+
205+
# Add checkmark emoji to original message
196206
try:
197207
client.reactions_add(
198208
channel=channel_id,
199-
name="white_check_mark",
209+
name=SlackConstants.EmojiReaction.COMPLETE,
200210
timestamp=thread_ts,
201211
)
202212
except SlackApiError as emoji_error:

experimenter/experimenter/slack/tests/test_notification.py

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -557,6 +557,7 @@ def test_send_experiment_launch_success_message(self, mock_webclient):
557557
mock_client = Mock()
558558
mock_webclient.return_value = mock_client
559559
mock_client.chat_postMessage.return_value = {"ok": True, "channel": "C123456"}
560+
mock_client.reactions_remove.return_value = {"ok": True}
560561
mock_client.reactions_add.return_value = {"ok": True}
561562

562563
thread_ts = "1234567890.123456"
@@ -578,11 +579,20 @@ def test_send_experiment_launch_success_message(self, mock_webclient):
578579
self.assertIn(self.experiment.slug, call_args.kwargs["text"])
579580
self.assertEqual(call_args.kwargs["thread_ts"], thread_ts)
580581

581-
# Verify reaction emoji was added with the correct channel ID
582+
# Verify eyes emoji was removed
583+
mock_client.reactions_remove.assert_called_once()
584+
remove_call = mock_client.reactions_remove.call_args
585+
self.assertEqual(remove_call.kwargs["channel"], "C123456")
586+
self.assertEqual(remove_call.kwargs["name"], SlackConstants.EmojiReaction.APPROVE)
587+
self.assertEqual(remove_call.kwargs["timestamp"], thread_ts)
588+
589+
# Verify checkmark emoji was added with the correct channel ID
582590
mock_client.reactions_add.assert_called_once()
583591
reaction_call = mock_client.reactions_add.call_args
584592
self.assertEqual(reaction_call.kwargs["channel"], "C123456")
585-
self.assertEqual(reaction_call.kwargs["name"], "white_check_mark")
593+
self.assertEqual(
594+
reaction_call.kwargs["name"], SlackConstants.EmojiReaction.COMPLETE
595+
)
586596
self.assertEqual(reaction_call.kwargs["timestamp"], thread_ts)
587597

588598
@override_settings(
@@ -620,6 +630,7 @@ def test_send_experiment_launch_success_message_reaction_already_exists(
620630
mock_client = Mock()
621631
mock_webclient.return_value = mock_client
622632
mock_client.chat_postMessage.return_value = {"ok": True, "channel": "C123456"}
633+
mock_client.reactions_remove.return_value = {"ok": True}
623634
# Simulate reaction already exists error
624635
mock_client.reactions_add.side_effect = SlackApiError(
625636
message="Slack error",
@@ -637,6 +648,7 @@ def test_send_experiment_launch_success_message_reaction_already_exists(
637648

638649
self.assertTrue(result)
639650
mock_client.chat_postMessage.assert_called_once()
651+
mock_client.reactions_remove.assert_called_once()
640652
mock_client.reactions_add.assert_called_once()
641653

642654
@override_settings(
@@ -648,6 +660,7 @@ def test_send_experiment_launch_success_message_reaction_error(self, mock_webcli
648660
mock_client = Mock()
649661
mock_webclient.return_value = mock_client
650662
mock_client.chat_postMessage.return_value = {"ok": True, "channel": "C123456"}
663+
mock_client.reactions_remove.return_value = {"ok": True}
651664
mock_client.reactions_add.side_effect = SlackApiError(
652665
message="Slack error", response={"ok": False, "error": "channel_not_found"}
653666
)

0 commit comments

Comments
 (0)