Skip to content

Commit

Permalink
Revert "Merge pull request #395 from ebmdatalab/edited-tech-support-m…
Browse files Browse the repository at this point in the history 
…essages"

This reverts commit 3c02e12, reversing
changes made to 03d619d.
  • Loading branch information
@rebkwok
rebkwok committed Jan 15, 2024
1 parent 4a502cc commit 82dfbb8
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 140 deletions.
49 changes: 7 additions & 42 deletions ebmbot/bot.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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"
)
Expand Down Expand Up @@ -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")
Expand Down
100 changes: 2 additions & 98 deletions tests/test_bot.py
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit 82dfbb8

Please sign in to comment.