From 85bce9a03a6fcb2eb9eb831745ab35b772932db3 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Thu, 10 Oct 2024 17:34:37 +0100 Subject: [PATCH 1/4] Refactor support channel config This gets rid of some duplicate code and makes it simpler to add additional support channels in future. --- bennettbot/bot.py | 59 ++++++++++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 24 deletions(-) diff --git a/bennettbot/bot.py b/bennettbot/bot.py index 3afa91e..8907563 100644 --- a/bennettbot/bot.py +++ b/bennettbot/bot.py @@ -108,27 +108,37 @@ def register_listeners(app, config, channels, bot_user_id, internal_user_ids): The listeners are defined inside this function to allow different config to be passed in for tests. """ - - bennett_admins_channel_id = channels[settings.SLACK_BENNETT_ADMINS_CHANNEL] - # Match "bennett-admins" or "bennet-admins" as a word (treating hyphens as - # word characters), except if it's preceded by a slash to avoid matching it - # in URLs - bennett_admins_regex = re.compile( - r".*(^|[^\w\-/])bennett?-admins($|[^\w\-]).*", flags=re.I - ) - - tech_support_channel_id = channels[settings.SLACK_TECH_SUPPORT_CHANNEL] - # Match "tech-support" as a word (treating hyphens as word characters), except if - # it's preceded by a slash to avoid matching it in URLs - tech_support_regex = re.compile( - r".*(^|[^\w\-/])tech-support($|[^\w\-]).*", flags=re.I - ) + support_config = { + "tech-support": { + "keyword": "tech-support", + "channel_id": channels[settings.SLACK_TECH_SUPPORT_CHANNEL], + # Match "tech-support" as a word (treating hyphens as word characters), except if + # it's preceded by a slash to avoid matching it in URLs + "regex": re.compile(r".*(^|[^\w\-/])tech-support($|[^\w\-]).*", flags=re.I), + "reaction": ":sos:", + }, + "bennett-admins": { + "keyword": "bennett-admins", + "channel_id": channels[settings.SLACK_BENNETT_ADMINS_CHANNEL], + # Match "bennett-admins" or "bennet-admins" as a word (treating hyphens as + # word characters), except if it's preceded by a slash to avoid matching it + # in URLs + "regex": re.compile( + r".*(^|[^\w\-/])bennett?-admins($|[^\w\-]).*", flags=re.I + ), + "reaction": ":flamingo:", + }, + } @app.event( "app_mention", # Don't match app mentions that include tech support keywords; these will be # matched by the tech-support listener - matchers=[lambda event: not tech_support_regex.match(event["text"])], + matchers=[ + lambda event: not support_config["tech-support"]["regex"].match( + event["text"] + ) + ], ) def job_listener(event, say, ack): """Respond to every Slack message that mentions the bot (and is not a @@ -147,7 +157,7 @@ def job_listener(event, say, ack): # only match DMs with the bot that are non-tech support messages matchers=[ lambda message: message["channel_type"] == "im" - and not tech_support_regex.match(message["text"]) + and not support_config["tech-support"]["regex"].match(message["text"]) ], ) def im_job_listener(event, say, ack): @@ -231,7 +241,7 @@ def _listener(event, say, is_im=False): include_apology = text != "help" handle_help(event, say, config, include_apology) - def build_matcher(regex, channel_id): + def build_matcher(*, regex, channel_id, **kwargs): """Builds matcher to match messages matching given regex but not posted in given channel.""" def matcher(event): @@ -256,23 +266,25 @@ def matcher(event): @app.event( {"type": "message"}, - matchers=[build_matcher(bennett_admins_regex, bennett_admins_channel_id)], + matchers=[build_matcher(**support_config["bennett-admins"])], ) def repost_to_bennett_admins(event, say, ack): repost_support_request_to_channel( - event, say, ack, "bennett-admins", bennett_admins_channel_id + event, say, ack, **support_config["bennett-admins"] ) @app.event( {"type": "message"}, - matchers=[build_matcher(tech_support_regex, tech_support_channel_id)], + matchers=[build_matcher(**support_config["tech-support"])], ) def repost_to_tech_support(event, say, ack): repost_support_request_to_channel( - event, say, ack, "tech-support", tech_support_channel_id + event, say, ack, **support_config["tech-support"] ) - def repost_support_request_to_channel(event, say, ack, keyword, channel_id): + def repost_support_request_to_channel( + event, say, ack, *, keyword, reaction, channel_id, **kwargs + ): # acknowledge the messages ack() # Our matcher filters only allows messages with no subtype (i.e. just @@ -290,7 +302,6 @@ def repost_support_request_to_channel(event, say, ack, keyword, channel_id): # an exception; if this happens, then the user is editing something # other than the keyword in the message, and we don't need to repost # it again. We let the default error handler will deal with it. - reaction = {"tech-support": "sos", "bennett-admins": "flamingo"}[keyword] app.client.reactions_add( channel=channel, timestamp=message["ts"], name=reaction ) From 0b93bdfa7ddf31fcaef3dbd1f8eed72c8b4a2259 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Thu, 10 Oct 2024 17:37:52 +0100 Subject: [PATCH 2/4] Allow support channel settings to be either channel name OR channel ID Most slack functions will accept either a channel name or a channel ID. We always use a channel ID for support channels for the few that require it. For readability, our settings have used channel names, but the disadvantage is that if a channel name changes, bennettbot will no longer be able to post to it. This allows settings to be either channel name or channel ID. If we decide to change them to channel IDs in prod, this means channel names can be changed with no interruption to how the support keywords work. --- bennettbot/bot.py | 19 +++++++++++++++++-- tests/test_bot.py | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/bennettbot/bot.py b/bennettbot/bot.py index 8907563..f50fd7a 100644 --- a/bennettbot/bot.py +++ b/bennettbot/bot.py @@ -111,7 +111,11 @@ def register_listeners(app, config, channels, bot_user_id, internal_user_ids): support_config = { "tech-support": { "keyword": "tech-support", - "channel_id": channels[settings.SLACK_TECH_SUPPORT_CHANNEL], + # Get the channel ID by channel name. If it doesn't exist, assume the setting is + # already the channel ID + "channel_id": channels.get( + settings.SLACK_TECH_SUPPORT_CHANNEL, settings.SLACK_TECH_SUPPORT_CHANNEL + ), # Match "tech-support" as a word (treating hyphens as word characters), except if # it's preceded by a slash to avoid matching it in URLs "regex": re.compile(r".*(^|[^\w\-/])tech-support($|[^\w\-]).*", flags=re.I), @@ -119,7 +123,12 @@ def register_listeners(app, config, channels, bot_user_id, internal_user_ids): }, "bennett-admins": { "keyword": "bennett-admins", - "channel_id": channels[settings.SLACK_BENNETT_ADMINS_CHANNEL], + # Get the channel ID by channel name. If it doesn't exist, assume the setting is + # already the channel ID + "channel_id": channels.get( + settings.SLACK_BENNETT_ADMINS_CHANNEL, + settings.SLACK_BENNETT_ADMINS_CHANNEL, + ), # Match "bennett-admins" or "bennet-admins" as a word (treating hyphens as # word characters), except if it's preceded by a slash to avoid matching it # in URLs @@ -129,6 +138,12 @@ def register_listeners(app, config, channels, bot_user_id, internal_user_ids): "reaction": ":flamingo:", }, } + # Check that channel settings mapped to valid channel IDs + for support in support_config.values(): + if support["channel_id"] not in channels.values(): + raise ValueError( + f"{support['keyword']} channel id '{support['channel_id']}' not found" + ) @app.event( "app_mention", diff --git a/tests/test_bot.py b/tests/test_bot.py index 20b4ec4..44d1f90 100644 --- a/tests/test_bot.py +++ b/tests/test_bot.py @@ -56,6 +56,52 @@ def test_joined_channels(mock_app): ) +@httpretty.activate(allow_net_connect=False) +@pytest.mark.parametrize( + "setting,value", + [ + # Note: channel names are set in pyproject.toml + # Channel ids are defined by the response from the response to + # the conversations.list endpoint, defined in mock_http_request.py + # support channel settings can be a channel name + ("SLACK_TECH_SUPPORT_CHANNEL", "techsupport"), + ("SLACK_BENNETT_ADMINS_CHANNEL", "bennettadmins"), + # support channel settings can be a channel ID + ("SLACK_TECH_SUPPORT_CHANNEL", "C0001"), + ("SLACK_BENNETT_ADMINS_CHANNEL", "C0000"), + ], +) +def test_register_listeners_support_channel_settings(setting, value): + register_bot_uris() + app = App(raise_error_for_unhandled_request=True) + channels = bot.get_channels(app.client) + bot_user_id, internal_user_ids = bot.get_users_info(app.client) + bot.join_all_channels(app.client, channels, bot_user_id) + with patch(f"bennettbot.settings.{setting}", value): + bot.register_listeners(app, config, channels, bot_user_id, internal_user_ids) + + +@httpretty.activate(allow_net_connect=False) +@pytest.mark.parametrize( + "setting,value", + [ + ("SLACK_TECH_SUPPORT_CHANNEL", "foo"), + ("SLACK_BENNETT_ADMINS_CHANNEL", "C9999"), + ], +) +def test_register_listeners_invalid_support_channel_settings(setting, value): + register_bot_uris() + app = App(raise_error_for_unhandled_request=True) + channels = bot.get_channels(app.client) + bot_user_id, internal_user_ids = bot.get_users_info(app.client) + bot.join_all_channels(app.client, channels, bot_user_id) + with ( + patch(f"bennettbot.settings.{setting}", value), + pytest.raises(ValueError, match=f"channel id '{value}' not found"), + ): + bot.register_listeners(app, config, channels, bot_user_id, internal_user_ids) + + def test_schedule_job(mock_app): handle_message(mock_app, "<@U1234> test do job 10") From 966056b2c0564b13ac00d33de4227555192cf451 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Tue, 15 Oct 2024 13:52:43 +0100 Subject: [PATCH 3/4] Move support config to a shared module Both the bot and the message checker in the dispatcher use elements of the support config, so move it out into a shared module. I'm not greatly happy about having a config.py that this lives in, but I don't want to pollute settings.py with functions that build config, and I don't want dispatcher.py to be dependent on bot.py Also fixes a bug with the reaction emoji config, and uses "support_channel" as the key in the support config to be more explicit about which channel we're referring to in functions that use this config. --- bennettbot/bot.py | 44 ++++++++-------------------------------- bennettbot/config.py | 39 +++++++++++++++++++++++++++++++++++ bennettbot/dispatcher.py | 14 +++---------- tests/test_dispatcher.py | 15 -------------- 4 files changed, 50 insertions(+), 62 deletions(-) create mode 100644 bennettbot/config.py diff --git a/bennettbot/bot.py b/bennettbot/bot.py index f50fd7a..f839bd2 100644 --- a/bennettbot/bot.py +++ b/bennettbot/bot.py @@ -12,6 +12,7 @@ from workspace.techsupport.jobs import get_dates_from_config as get_tech_support_dates from . import job_configs, scheduler, settings +from .config import get_support_config from .logger import log_call, logger from .slack import notify_slack @@ -108,41 +109,12 @@ def register_listeners(app, config, channels, bot_user_id, internal_user_ids): The listeners are defined inside this function to allow different config to be passed in for tests. """ - support_config = { - "tech-support": { - "keyword": "tech-support", - # Get the channel ID by channel name. If it doesn't exist, assume the setting is - # already the channel ID - "channel_id": channels.get( - settings.SLACK_TECH_SUPPORT_CHANNEL, settings.SLACK_TECH_SUPPORT_CHANNEL - ), - # Match "tech-support" as a word (treating hyphens as word characters), except if - # it's preceded by a slash to avoid matching it in URLs - "regex": re.compile(r".*(^|[^\w\-/])tech-support($|[^\w\-]).*", flags=re.I), - "reaction": ":sos:", - }, - "bennett-admins": { - "keyword": "bennett-admins", - # Get the channel ID by channel name. If it doesn't exist, assume the setting is - # already the channel ID - "channel_id": channels.get( - settings.SLACK_BENNETT_ADMINS_CHANNEL, - settings.SLACK_BENNETT_ADMINS_CHANNEL, - ), - # Match "bennett-admins" or "bennet-admins" as a word (treating hyphens as - # word characters), except if it's preceded by a slash to avoid matching it - # in URLs - "regex": re.compile( - r".*(^|[^\w\-/])bennett?-admins($|[^\w\-]).*", flags=re.I - ), - "reaction": ":flamingo:", - }, - } + support_config = get_support_config(channels) # Check that channel settings mapped to valid channel IDs for support in support_config.values(): - if support["channel_id"] not in channels.values(): + if support["support_channel"] not in channels.values(): raise ValueError( - f"{support['keyword']} channel id '{support['channel_id']}' not found" + f"{support['keyword']} channel id '{support['support_channel']}' not found" ) @app.event( @@ -256,12 +228,12 @@ def _listener(event, say, is_im=False): include_apology = text != "help" handle_help(event, say, config, include_apology) - def build_matcher(*, regex, channel_id, **kwargs): + def build_matcher(*, regex, support_channel, **kwargs): """Builds matcher to match messages matching given regex but not posted in given channel.""" def matcher(event): # Only match messages posted outside of the channel itself - if event["channel"] == channel_id: + if event["channel"] == support_channel: 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") @@ -298,7 +270,7 @@ def repost_to_tech_support(event, say, ack): ) def repost_support_request_to_channel( - event, say, ack, *, keyword, reaction, channel_id, **kwargs + event, say, ack, *, keyword, reaction, support_channel, **kwargs ): # acknowledge the messages ack() @@ -335,7 +307,7 @@ def repost_support_request_to_channel( message_url = app.client.chat_getPermalink( channel=channel, message_ts=message["ts"] )["permalink"] - say(message_url, channel=channel_id) + say(message_url, channel=support_channel) else: say( f"Sorry, I can't call {keyword} from this conversation.", diff --git a/bennettbot/config.py b/bennettbot/config.py new file mode 100644 index 0000000..b82c42e --- /dev/null +++ b/bennettbot/config.py @@ -0,0 +1,39 @@ +import re + +from . import settings + + +def get_support_config(channels=None): + channels = channels or {} + return { + "tech-support": { + "keyword": "tech-support", + # Use the support setting to get the channel. We use channel ID if it is + # available, otherwise we default to using the setting value (on the + # assumption that the setting value is the channel name). + "support_channel": channels.get( + settings.SLACK_TECH_SUPPORT_CHANNEL, settings.SLACK_TECH_SUPPORT_CHANNEL + ), + # Match "tech-support" as a word (treating hyphens as word characters), except if + # it's preceded by a slash to avoid matching it in URLs + "regex": re.compile(r".*(^|[^\w\-/])tech-support($|[^\w\-]).*", flags=re.I), + "reaction": "sos", + }, + "bennett-admins": { + "keyword": "bennett-admins", + # Use the support setting to get the channel. We use channel ID if it is + # available, otherwise we default to using the setting value (on the + # assumption that the setting value is the channel name). + "support_channel": channels.get( + settings.SLACK_BENNETT_ADMINS_CHANNEL, + settings.SLACK_BENNETT_ADMINS_CHANNEL, + ), + # Match "bennett-admins" or "bennet-admins" as a word (treating hyphens as + # word characters), except if it's preceded by a slash to avoid matching it + # in URLs + "regex": re.compile( + r".*(^|[^\w\-/])bennett?-admins($|[^\w\-]).*", flags=re.I + ), + "reaction": "flamingo", + }, + } diff --git a/bennettbot/dispatcher.py b/bennettbot/dispatcher.py index b2ee94c..92d217d 100644 --- a/bennettbot/dispatcher.py +++ b/bennettbot/dispatcher.py @@ -13,6 +13,7 @@ import requests from . import job_configs, scheduler, settings +from .config import get_support_config from .logger import logger from .slack import notify_slack, slack_web_client @@ -211,16 +212,7 @@ def __init__(self, bot_slack_client, user_slack_client): self.bot_slack_client = bot_slack_client self.user_slack_client = user_slack_client - self.config = { - "tech-support": { - "reaction": "sos", - "channel": settings.SLACK_TECH_SUPPORT_CHANNEL, - }, - "bennett-admins": { - "reaction": "flamingo", - "channel": settings.SLACK_BENNETT_ADMINS_CHANNEL, - }, - } + self.config = get_support_config() def run_check(self): # pragma: no cover """Start running the check in a new subprocess.""" @@ -248,7 +240,7 @@ def do_check(self, run_fn=lambda: True, delay=10): # pragma: no branch def check_messages(self, keyword, after): logger.debug("Checking %s messages", keyword) reaction = self.config[keyword]["reaction"] - channel = self.config[keyword]["channel"] + channel = self.config[keyword]["support_channel"] messages = self.user_slack_client.search_messages( query=( # Search for messages with the keyword but without the expected reaction diff --git a/tests/test_dispatcher.py b/tests/test_dispatcher.py index 9bbde0c..bccd5fe 100644 --- a/tests/test_dispatcher.py +++ b/tests/test_dispatcher.py @@ -524,21 +524,6 @@ def build_log_dir(job_type_with_namespace): ) -def test_message_checker_config(): - checker = MessageChecker(slack_web_client("bot"), slack_web_client("user")) - # channel IDs are retrieved from mock_web_api_server - assert checker.config == { - "tech-support": { - "reaction": "sos", - "channel": settings.SLACK_TECH_SUPPORT_CHANNEL, - }, - "bennett-admins": { - "reaction": "flamingo", - "channel": settings.SLACK_BENNETT_ADMINS_CHANNEL, - }, - } - - def test_message_checker_run(freezer): freezer.move_to("2024-10-08 23:30") httpretty_register( From 0950ef7ec50eac8c26980d82d005b0d733184d35 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Wed, 16 Oct 2024 15:13:39 +0100 Subject: [PATCH 4/4] Update comments --- bennettbot/config.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/bennettbot/config.py b/bennettbot/config.py index b82c42e..566b2dc 100644 --- a/bennettbot/config.py +++ b/bennettbot/config.py @@ -8,9 +8,9 @@ def get_support_config(channels=None): return { "tech-support": { "keyword": "tech-support", - # Use the support setting to get the channel. We use channel ID if it is - # available, otherwise we default to using the setting value (on the - # assumption that the setting value is the channel name). + # Use the support setting (a channel name or channel ID) to get the channel. + # We first attempt to retrieve channel ID from the channels dict (if provided), + # otherwise we default to using the setting value as is. "support_channel": channels.get( settings.SLACK_TECH_SUPPORT_CHANNEL, settings.SLACK_TECH_SUPPORT_CHANNEL ), @@ -21,9 +21,9 @@ def get_support_config(channels=None): }, "bennett-admins": { "keyword": "bennett-admins", - # Use the support setting to get the channel. We use channel ID if it is - # available, otherwise we default to using the setting value (on the - # assumption that the setting value is the channel name). + # Use the support setting (a channel name or channel ID) to get the channel. + # We first attempt to retrieve channel ID from the channels dict (if provided), + # otherwise we default to using the setting value as is. "support_channel": channels.get( settings.SLACK_BENNETT_ADMINS_CHANNEL, settings.SLACK_BENNETT_ADMINS_CHANNEL,