From 82dfbb895f1f73fe5956bbb5da0362575ba5c4e1 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Mon, 15 Jan 2024 15:40:35 +0000 Subject: [PATCH] Revert "Merge pull request #395 from ebmdatalab/edited-tech-support-messages" This reverts commit 3c02e129391767e62610b92aa58870cfb29d0c6a, reversing changes made to 03d619d6f6ea6af582baf2e9ac984387aa2e5549. --- ebmbot/bot.py | 49 ++++------------------- tests/test_bot.py | 100 +--------------------------------------------- 2 files changed, 9 insertions(+), 140 deletions(-) diff --git a/ebmbot/bot.py b/ebmbot/bot.py index e7ec48f3..69e62410 100644 --- a/ebmbot/bot.py +++ b/ebmbot/bot.py @@ -7,7 +7,6 @@ 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 @@ -103,12 +102,6 @@ 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", @@ -179,36 +172,21 @@ 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=tech_support_matchers, + matchers=[ + lambda message: message["channel"] != tech_support_channel_id, + lambda message: "bot_id" not in message, + ], ) def repost_to_tech_support(message, say, 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" ) @@ -244,28 +222,15 @@ 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") diff --git a/tests/test_bot.py b/tests/test_bot.py index 1ac18b9d..c0bd0129 100644 --- a/tests/test_bot.py +++ b/tests/test_bot.py @@ -1,11 +1,10 @@ import json import time from datetime import datetime, timedelta -from unittest.mock import Mock, patch +from unittest.mock import 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 @@ -340,40 +339,6 @@ 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() @@ -498,7 +463,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 response status + # We use an error handler to deal with unhandled messages, so the resonse status # is 200 resp = handle_message( mock_app, @@ -532,55 +497,6 @@ 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( @@ -659,18 +575,6 @@ 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",