Handle edited tech support messages

ebmbot/bot.py

@@ -7,6 +7,7 @@
 from slack_bolt.adapter.socket_mode import SocketModeHandler
 from slack_bolt.error import BoltUnhandledRequestError
 from slack_bolt.util.utils import get_boot_message
+from slack_sdk.errors import SlackApiError
 
 from workspace.techsupport.jobs import get_dates_from_config as get_tech_support_dates
 
@@ -102,6 +103,12 @@ def register_listeners(app, config, channels, bot_user_id):
     tech_support_regex = re.compile(
         r".*(^|[^\w\-/])tech-support($|[^\w\-]).*", flags=re.I
     )
+    # Only match messages posted outside of the tech support channel itself
+    # and messages that are not posted by a bot (to avoid reposting reminders etc)
+    tech_support_matchers = [
+        lambda message: message["channel"] != tech_support_channel_id,
+        lambda message: "bot_id" not in message,
+    ]
 
     @app.event(
         "app_mention",
@@ -172,21 +179,36 @@ def _listener(event, say):
         include_apology = text != "help"
         handle_help(event, say, config["help"], config["description"], include_apology)
 
+    @app.event(
+        {"type": "message", "subtype": "message_changed", "text": tech_support_regex},
+        matchers=tech_support_matchers,
+    )
+    def repost_edited_message_to_tech_support(event, say, ack):
+        message_to_handle = {
+            **event["previous_message"],
+            "channel": event["channel"],
+            "channel_type": event["channel_type"],
+        }
+        return _repost_to_tech_support(message_to_handle, say, ack)
+
     @app.message(
         tech_support_regex,
         # Only match messages posted outside of the tech support channel itself
         # and messages that are not posted by a bot (to avoid reposting reminders etc)
-        matchers=[
-            lambda message: message["channel"] != tech_support_channel_id,
-            lambda message: "bot_id" not in message,
-        ],
+        matchers=tech_support_matchers,
     )
     def repost_to_tech_support(message, say, ack):
-        ack()
+        return _repost_to_tech_support(message, say, ack)
 
+    def _repost_to_tech_support(message, say, ack):
+        ack()
         # Don't repost messages in DMs with the bot
         if message["channel_type"] in ["channel", "group"]:
             # Respond with SOS reaction
+            # If we've already responded, the attempt to react here will raise
+            # an exception; if this happens, then the user is editing something
+            # other than the tech-support keyword in the message, and we don't need to
+            # repost it again. We let the default error handler will deal with it.
             app.client.reactions_add(
                 channel=message["channel"], timestamp=message["ts"], name="sos"
             )
@@ -222,15 +244,28 @@ def join_channel(event, ack):
 
     @app.error
     def handle_errors(error, body):
+        message_text = body["event"].get("message", {}).get("text", "")
         if isinstance(error, BoltUnhandledRequestError):
             # Unhandled messages are common (anything that doesn't get matched
             # by one of the listeners).  We don't want to log those.
             return BoltResponse(status=200, body="Unhandled message")
+        elif (
+            isinstance(error, SlackApiError)
+            and error.response.data["error"] == "already_reacted"
+            and "tech-support" in message_text
+        ):
+            # If we're already reacted to a tech-support message and the
+            # user edits the message, we get a slack error, but that's OK
+            logger.info(
+                "Already reacted to tech-support message",
+                message=message_text,
+            )
+            return BoltResponse(
+                status=200, body="Already reacted to tech-support message"
+            )
         else:
             # other error patterns
             channel = body["event"]["channel"]
-            message_text = body["event"].get("message", {}).get("text", "")
-
             ts = body["event"]["ts"]
             logger.error("Unexpected error", error=error, body=body)
             app.client.reactions_add(channel=channel, timestamp=ts, name="x")

tests/test_bot.py

@@ -1,10 +1,11 @@
 import json
 import time
 from datetime import datetime, timedelta
-from unittest.mock import patch
+from unittest.mock import Mock, patch
 
 import pytest
 from slack_bolt.request import BoltRequest
+from slack_sdk.errors import SlackApiError
 from slack_sdk.signature import SignatureVerifier
 
 from ebmbot import bot, scheduler
@@ -330,6 +331,40 @@ def test_tech_support_listener(mock_app, text, channel, event_kwargs, repost_exp
         assert ("channel", "C0001") in post_message.items()
 
 
+def test_tech_support_edited_message(mock_app):
+    # the triggered tech support handler will first fetch the url for the message
+    # and then post it to the techsupport channel
+    # Before the dispatched message, neither of these paths have been called
+    recorder = mock_app.recorder
+    tech_support_call_paths = ["/chat.getPermalink", "/chat.postMessage"]
+    for path in tech_support_call_paths:
+        assert path not in recorder.mock_received_requests
+
+    handle_message(
+        mock_app,
+        "get tec-support",
+        channel="C0002",
+        reaction_count=0,
+        event_type="message",
+    )
+
+    # tech-support keyword typo, no tech support calls
+    for path in tech_support_call_paths:
+        assert path not in recorder.mock_received_requests
+
+    # Editing the same message to include tech-support does repost
+    handle_message(
+        mock_app,
+        "get tech-support",
+        channel="C0002",
+        reaction_count=1,
+        event_type="message_changed",
+    )
+
+    for path in tech_support_call_paths:
+        assert recorder.mock_received_requests[path] == 1
+
+
 @patch("ebmbot.bot.get_tech_support_dates")
 def test_tech_support_out_of_office_listener(tech_support_dates, mock_app):
     start = (datetime.today() - timedelta(1)).date()
@@ -454,7 +489,7 @@ def test_no_listener_found(mock_app):
     # A message must either start with "<@U1234>" (i.e. a user @'d the bot) OR must contain
     # the tech-support pattern
     text = "This message should not match any listener"
-    # We use an error handler to deal with unhandled messages, so the resonse status
+    # We use an error handler to deal with unhandled messages, so the response status
     # is 200
     resp = handle_message(
         mock_app,
@@ -488,6 +523,55 @@ def test_unexpected_error(mock_app):
     )
 
 
+def test_already_reacted_to_tech_support_error(mock_app):
+    # mock an error from a method that's called during the tech-support
+    # handling to return an "already reacted" SlackApiError
+    with patch(
+        "ebmbot.bot.tech_support_out_of_office",
+        side_effect=SlackApiError(
+            message="Error", response=Mock(data={"error": "already_reacted"})
+        ),
+    ):
+        handle_message(
+            mock_app,
+            "tech-support help",
+            channel="channel",
+            reaction_count=1,
+            event_type="message",
+            expected_status=200,
+        )
+
+    assert_slack_client_sends_messages(
+        mock_app.recorder,
+        messages_kwargs=[],
+    )
+
+
+def test_already_reacted_to_non_tech_support_error(mock_app):
+    # Only already-reacted to tech support messages are ignored;
+    # another sort of message that raises this error gets
+    # reported back to slack
+    with patch(
+        "ebmbot.bot.handle_namespace_help",
+        side_effect=SlackApiError(
+            message="Error", response=Mock(data={"error": "already_reacted"})
+        ),
+    ):
+        handle_message(
+            mock_app, "<@U1234> test help", reaction_count=1, expected_status=500
+        )
+
+    assert_slack_client_sends_messages(
+        mock_app.recorder,
+        messages_kwargs=[
+            {
+                "channel": "channel",
+                "text": "Unexpected error: SlackApiError",
+            }
+        ],
+    )
+
+
 def test_new_channel_created(mock_app):
     # When a channel_created event is received, the bot user joins that channel
     handle_event(
@@ -566,6 +650,18 @@ def get_mock_request(event_type, event_kwargs):
     event_kwargs = event_kwargs or {}
     event_kwargs.update({"message": {"text": event_kwargs.get("text", "")}})
 
+    if event_type == "message_changed":
+        event_kwargs.update(
+            {
+                "type": "message",
+                "subtype": "message_changed",
+                "previous_message": {
+                    "ts": "1596183880.004200",
+                    "text": event_kwargs["text"],
+                },
+            }
+        )
+
     body = {
         "token": "verification_token",
         "team_id": "T111",