Merge pull request #413 from ebmdatalab/edited-tech-support-messages

Respond to 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
 
@@ -154,7 +155,7 @@ def _listener(event, say, is_im=False):
         text = event["text"].replace("Reminder: ", "")
         # Remove the bot mention; this sometimes includes the bot's name as well as
         # id. In reminders, the bot mention is in the form <@AB1234|bot_name>; in user
-        # messages that @ the both, it is fjust in the form <@AB1234>. We need to match both.
+        # messages that @ the bot, it is just in the form <@AB1234>. We need to match both.
         text = re.sub(rf"<@{bot_user_id}(|.+)?>", "", text)
 
         # handle extra whitespace and punctuation
@@ -190,23 +191,46 @@ def _listener(event, say, is_im=False):
         include_apology = text != "help"
         handle_help(event, say, config, include_apology)
 
-    @app.message(
-        tech_support_regex,
+    def tech_support_matcher(event):
         # 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,
-        ],
-    )
-    def repost_to_tech_support(message, say, ack):
-        ack()
+        if event["channel"] == tech_support_channel_id:
+            return False
+        # only match messages that are not posted by a bot, to avoid reposting reminders etc
+        # (the event dict will include the key "bot_id")
+        if "bot_id" in event:
+            return False
+        # Of the available message subtypes, only match "message_changed"; we don't
+        # want to match e.g. "reminder_add" messages that include the word "tech-support"
+        subtype = event.get("subtype")
+        if subtype and subtype != "message_changed":
+            return False
+
+        # match the tech-support keyword
+        text = event.get("message", event)["text"]
+        return tech_support_regex.match(text) is not None
 
+    @app.event(
+        {"type": "message"},
+        matchers=[tech_support_matcher],
+    )
+    def repost_to_tech_support(event, say, ack):
+        # Our matcher filters only allows messages with no subtype (i.e. just
+        # straightforward posts) or message_changed subtype
+        # For edited messages (message_changed), the text is found in the event's
+        # "message"; for other messages, the event itself contains the text
+        message = event.get("message", event)
+        channel = event["channel"]
         # Don't repost messages in DMs with the bot
-        if message["channel_type"] in ["channel", "group"]:
+        # We don't use the matcher for this, because we want to tell users to
+        # call tech-support from a non-dm channel
+        if event["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"
+                channel=channel, timestamp=message["ts"], name="sos"
             )
             logger.info("Received tech-support message", message=message["text"])
             # If out of office, respond with an ooo message, but still repost to tech-support channel
@@ -216,18 +240,18 @@ def repost_to_tech_support(message, say, ack):
                 logger.info("Tech support OOO", until=out_of_office_until)
                 say(
                     f"tech-support is currently out of office and will respond after {out_of_office_until}",
-                    channel=message["channel"],
+                    channel=channel,
                     thread_ts=message["ts"],
                 )
 
             message_url = app.client.chat_getPermalink(
-                channel=message["channel"], message_ts=message["ts"]
+                channel=channel, message_ts=message["ts"]
             )["permalink"]
             say(message_url, channel=tech_support_channel_id)
         else:
             say(
                 "Sorry, I can't call tech-support from this conversation.",
-                channel=message["channel"],
+                channel=channel,
             )
 
     @app.event("channel_created")
@@ -240,15 +264,32 @@ def join_channel(event, ack):
 
     @app.error
     def handle_errors(error, body):
+        if "message" in body["event"]:
+            message_text = body["event"]["message"]["text"]
+        else:
+            message_text = body["event"].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
@@ -275,9 +276,8 @@ def test_pluralise():
     assert bot._pluralise(2, "bot") == "There are 2 bots"
 
 
-@pytest.mark.parametrize(
-    "text,channel,event_kwargs,repost_expected",
-    [
+def _tech_support_test_params():
+    return [
         # We only match the hyphenated keywords "tech-support"
         ("This message should not match the tech support listener", "C0002", {}, False),
         # We match only distinct words
@@ -317,12 +317,51 @@ def test_pluralise():
         ("This message should match the `tech-support` listener", "C0002", {}, True),
         ("tech-support - this message should match", "C0002", {}, True),
         ("This message should match - tech-support", "C0002", {}, True),
-    ],
+    ]
+
+
+@pytest.mark.parametrize(
+    "text,channel,event_kwargs,repost_expected",
+    _tech_support_test_params(),
 )
 def test_tech_support_listener(mock_app, text, channel, event_kwargs, repost_expected):
+    # test that we get the expected response with an initial tech-support message
+    assert_expected_tech_support_response(
+        mock_app, text, channel, event_kwargs, repost_expected
+    )
+
+
+@pytest.mark.parametrize(
+    "text,channel,event_kwargs,repost_expected",
+    _tech_support_test_params(),
+)
+def test_tech_support_listener_for_changed_messages(
+    mock_app, text, channel, event_kwargs, repost_expected
+):
+    # test that we also get the expected response for a changed message
+    event_kwargs.update({"subtype": "message_changed"})
+    assert_expected_tech_support_response(
+        mock_app, text, channel, event_kwargs, repost_expected
+    )
+
+
+def test_tech_support_listener_ignores_non_message_changed_subtypes(mock_app):
+    assert_expected_tech_support_response(
+        mock_app,
+        text="A tech-support message that would usually match",
+        channel="C0002",
+        event_kwargs={"subtype": "reminder_add"},
+        repost_expected=False,
+    )
+
+
+def assert_expected_tech_support_response(
+    mock_app, text, channel, event_kwargs, repost_expected
+):
     # 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:
@@ -334,7 +373,7 @@ def test_tech_support_listener(mock_app, text, channel, event_kwargs, repost_exp
         channel=channel,
         reaction_count=1 if repost_expected else 0,
         event_type="message",
-        event_kwargs=event_kwargs or {},
+        event_kwargs=event_kwargs,
     )
 
     # After the dispatched message, each path has been called once
@@ -353,6 +392,42 @@ 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",
+        event_kwargs={"subtype": "message_changed"},
+    )
+
+    # 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",
+        event_kwargs={"subtype": "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()
@@ -477,7 +552,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,
@@ -505,7 +580,56 @@ def test_unexpected_error(mock_app):
         messages_kwargs=[
             {
                 "channel": "channel",
-                "text": "Unexpected error: Exception()\nwhile responding to message `<@U1234> test help`",
+                "text": "Unexpected error: Exception()\nwhile responding to message `test help`",
+            }
+        ],
+    )
+
+
+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",
             }
         ],
     )
@@ -594,7 +718,17 @@ def handle_message(
     expected_status=200,
 ):
     event_kwargs = event_kwargs or {}
-    event_kwargs.update({"channel": channel, "text": text})
+    event_kwargs.update({"channel": channel})
+    # If it's a message_changed message event, has a "message"
+    # key with a dict containing the current message text and ts
+    # (and other elements that we don't use)
+    # Non-changed messages and other events,such as "app_mention",
+    # won't contain "message"
+    if event_kwargs.get("subtype") == "message_changed":
+        event_kwargs.update({"message": {"text": text, "ts": "1596183880.004200"}})
+    else:
+        event_kwargs.update({"text": text})
+
     resp = handle_event(
         mock_app,
         event_type=event_type,
@@ -619,9 +753,6 @@ def handle_event(mock_app, event_type, event_kwargs, expected_status=200):
 
 
 def get_mock_request(event_type, event_kwargs):
-    event_kwargs = event_kwargs or {}
-    event_kwargs.update({"message": {"text": event_kwargs.get("text", "")}})
-
     body = {
         "token": "verification_token",
         "team_id": "T111",