From 2c867b5143dd2345b62b132c3696aec6153e70f0 Mon Sep 17 00:00:00 2001 From: Yuhong Sun Date: Fri, 13 Oct 2023 22:52:21 -0700 Subject: [PATCH] Fix Slack premature Reacts and Notification (#571) --- .../bots/slack/handlers/handle_message.py | 53 +++++++++++++++++-- backend/danswer/bots/slack/listener.py | 50 ++--------------- backend/danswer/bots/slack/utils.py | 15 ++++-- 3 files changed, 63 insertions(+), 55 deletions(-) diff --git a/backend/danswer/bots/slack/handlers/handle_message.py b/backend/danswer/bots/slack/handlers/handle_message.py index 7d462fd61..7d7e4e524 100644 --- a/backend/danswer/bots/slack/handlers/handle_message.py +++ b/backend/danswer/bots/slack/handlers/handle_message.py @@ -3,6 +3,7 @@ from typing import cast from retry import retry from slack_sdk import WebClient +from slack_sdk.errors import SlackApiError from sqlalchemy.orm import Session from danswer.bots.slack.blocks import build_documents_blocks @@ -19,7 +20,9 @@ from danswer.configs.danswerbot_configs import DANSWER_BOT_ANSWER_GENERATION_TIM from danswer.configs.danswerbot_configs import DANSWER_BOT_DISABLE_DOCS_ONLY_ANSWER from danswer.configs.danswerbot_configs import DANSWER_BOT_DISPLAY_ERROR_MSGS from danswer.configs.danswerbot_configs import DANSWER_BOT_NUM_RETRIES +from danswer.configs.danswerbot_configs import DANSWER_REACT_EMOJI from danswer.configs.danswerbot_configs import ENABLE_DANSWERBOT_REFLEXION +from danswer.connectors.slack.utils import make_slack_api_rate_limited from danswer.db.engine import get_sqlalchemy_engine from danswer.db.models import SlackBotConfig from danswer.direct_qa.answer_question import answer_qa_query @@ -30,6 +33,37 @@ from danswer.utils.logger import setup_logger logger_base = setup_logger() +def send_msg_ack_to_user(details: SlackMessageInfo, client: WebClient) -> None: + if details.is_bot_msg and details.sender: + respond_in_thread( + client=client, + channel=details.channel_to_respond, + thread_ts=details.msg_to_respond, + receiver_ids=[details.sender], + text="Hi, we're evaluating your query :face_with_monocle:", + ) + return + + slack_call = make_slack_api_rate_limited(client.reactions_add) + slack_call( + name=DANSWER_REACT_EMOJI, + channel=details.channel_to_respond, + timestamp=details.msg_to_respond, + ) + + +def remove_react(details: SlackMessageInfo, client: WebClient) -> None: + if details.is_bot_msg: + return + + slack_call = make_slack_api_rate_limited(client.reactions_remove) + slack_call( + name=DANSWER_REACT_EMOJI, + channel=details.channel_to_respond, + timestamp=details.msg_to_respond, + ) + + def handle_message( message_info: SlackMessageInfo, channel_config: SlackBotConfig | None, @@ -114,6 +148,11 @@ def handle_message( thread_ts=None, ) + try: + send_msg_ack_to_user(message_info, client) + except SlackApiError as e: + logger.error(f"Was not able to react to user message due to: {e}") + @retry( tries=num_retries, delay=0.25, @@ -137,7 +176,9 @@ def handle_message( else: raise RuntimeError(answer.error_msg) + answer_failed = False try: + # This includes throwing out answer via reflexion answer = _get_answer( QuestionRequest( query=msg, @@ -150,6 +191,7 @@ def handle_message( ) ) except Exception as e: + answer_failed = True logger.exception( f"Unable to process message - did not successfully answer " f"in {num_retries} attempts" @@ -164,6 +206,13 @@ def handle_message( text=f"Encountered exception when trying to answer: \n\n```{e}```", thread_ts=message_ts_to_respond_to, ) + + try: + remove_react(message_info, client) + except SlackApiError as e: + logger.error(f"Failed to remove Reaction due to: {e}") + + if answer_failed: return True if answer.eval_res_valid is False: @@ -195,8 +244,6 @@ def handle_message( ) return True - # convert raw response into "nicely" formatted Slack message - # If called with the DanswerBot slash command, the question is lost so we have to reshow it restate_question_block = get_restate_blocks(msg, is_bot_msg) @@ -215,7 +262,7 @@ def handle_message( client=client, channel=channel, receiver_ids=send_to, - text="Something has gone wrong! The Slack blocks failed to load...", + text="Hello! Danswer has some results for you!", blocks=restate_question_block + answer_blocks + document_blocks, thread_ts=message_ts_to_respond_to, # don't unfurl, since otherwise we will have 5+ previews which makes the message very long diff --git a/backend/danswer/bots/slack/listener.py b/backend/danswer/bots/slack/listener.py index 6406e119c..81843dbea 100644 --- a/backend/danswer/bots/slack/listener.py +++ b/backend/danswer/bots/slack/listener.py @@ -4,7 +4,6 @@ from typing import Any from typing import cast from slack_sdk import WebClient -from slack_sdk.errors import SlackApiError from slack_sdk.socket_mode import SocketModeClient from slack_sdk.socket_mode.request import SocketModeRequest from slack_sdk.socket_mode.response import SocketModeResponse @@ -21,9 +20,7 @@ from danswer.bots.slack.utils import decompose_block_id from danswer.bots.slack.utils import get_channel_name_from_id from danswer.bots.slack.utils import respond_in_thread from danswer.configs.danswerbot_configs import DANSWER_BOT_RESPOND_EVERY_CHANNEL -from danswer.configs.danswerbot_configs import DANSWER_REACT_EMOJI from danswer.configs.danswerbot_configs import NOTIFY_SLACKBOT_NO_ANSWER -from danswer.connectors.slack.utils import make_slack_api_rate_limited from danswer.db.engine import get_sqlalchemy_engine from danswer.dynamic_configs.interface import ConfigNotFoundError from danswer.utils.logger import setup_logger @@ -207,37 +204,6 @@ def build_request_details( raise RuntimeError("Programming fault, this should never happen.") -def send_msg_ack_to_user(details: SlackMessageInfo, client: SocketModeClient) -> None: - if details.is_bot_msg and details.sender: - respond_in_thread( - client=client.web_client, - channel=details.channel_to_respond, - thread_ts=details.msg_to_respond, - receiver_ids=[details.sender], - text="Hi, we're evaluating your query :face_with_monocle:", - ) - return - - slack_call = make_slack_api_rate_limited(client.web_client.reactions_add) - slack_call( - name=DANSWER_REACT_EMOJI, - channel=details.channel_to_respond, - timestamp=details.msg_to_respond, - ) - - -def remove_react(details: SlackMessageInfo, client: SocketModeClient) -> None: - if details.is_bot_msg: - return - - slack_call = make_slack_api_rate_limited(client.web_client.reactions_remove) - slack_call( - name=DANSWER_REACT_EMOJI, - channel=details.channel_to_respond, - timestamp=details.msg_to_respond, - ) - - def apologize_for_fail( details: SlackMessageInfo, client: SocketModeClient, @@ -264,7 +230,7 @@ def process_message( details = build_request_details(req, client) channel = details.channel_to_respond - channel_name = get_channel_name_from_id( + channel_name, is_dm = get_channel_name_from_id( client=client.web_client, channel_id=channel ) @@ -279,18 +245,13 @@ def process_message( if ( slack_bot_config is None and not respond_every_channel - # DMs are unnamed, don't filter those out - and channel_name is not None + # Can't have configs for DMs so don't toss them out + and not is_dm # If @DanswerBot or /DanswerBot, always respond with the default configs and not (details.is_bot_msg or details.bipass_filters) ): return - try: - send_msg_ack_to_user(details, client) - except SlackApiError as e: - logger.error(f"Was not able to react to user message due to: {e}") - failed = handle_message( message_info=details, channel_config=slack_bot_config, @@ -301,11 +262,6 @@ def process_message( if failed and notify_no_answer: apologize_for_fail(details, client) - try: - remove_react(details, client) - except SlackApiError as e: - logger.error(f"Failed to remove Reaction due to: {e}") - def acknowledge_message(req: SocketModeRequest, client: SocketModeClient) -> None: response = SocketModeResponse(envelope_id=req.envelope_id) diff --git a/backend/danswer/bots/slack/utils.py b/backend/danswer/bots/slack/utils.py index 9d8ea6f4a..c7ecc2346 100644 --- a/backend/danswer/bots/slack/utils.py +++ b/backend/danswer/bots/slack/utils.py @@ -172,12 +172,17 @@ def get_channel_from_id(client: WebClient, channel_id: str) -> dict[str, Any]: return response["channel"] -def get_channel_name_from_id(client: WebClient, channel_id: str) -> str | None: +def get_channel_name_from_id( + client: WebClient, channel_id: str +) -> tuple[str | None, bool]: try: - return get_channel_from_id(client, channel_id).get("name") - except SlackApiError: - # Private channels such as DMs don't have a name - return None + channel_info = get_channel_from_id(client, channel_id) + name = channel_info.get("name") + is_dm = any([channel_info.get("is_im"), channel_info.get("is_mpim")]) + return name, is_dm + except SlackApiError as e: + logger.exception(f"Couldn't fetch channel name from id: {channel_id}") + raise e def fetch_userids_from_emails(user_emails: list[str], client: WebClient) -> list[str]: