From 329cdd89385fec1b510973598f9a042a3762d214 Mon Sep 17 00:00:00 2001 From: Sylvia McLaughlin <85905333+sylviamclaughlin@users.noreply.github.com> Date: Thu, 14 Mar 2024 22:15:49 +0000 Subject: [PATCH] Fixing a bug with the SRE bot --- app/integrations/slack/users.py | 24 +++++---- app/modules/incident/incident.py | 10 ++-- app/tests/integrations/slack/test_users.py | 57 ++++++++++++++++------ 3 files changed, 60 insertions(+), 31 deletions(-) diff --git a/app/integrations/slack/users.py b/app/integrations/slack/users.py index da0a6566..ba4e488c 100644 --- a/app/integrations/slack/users.py +++ b/app/integrations/slack/users.py @@ -3,7 +3,6 @@ This module contains the user related functionality for the Slack integration. """ import re -import logging SLACK_USER_ID_REGEX = r"^[A-Z0-9]+$" @@ -48,13 +47,18 @@ def get_user_locale(client, user_id=None): return default_locale -def replace_user_id_with_handle(user_handle, message): - """Function to replace the user id with the user handle in a message:w""" - if not user_handle or not message: - logging.error("User handle or message is empty or None") - return None +def replace_user_id_with_handle(client, message): + """Function to replace the user id with the user handle in a message.""" + user_id_pattern = r"<@(\w+)>" - user_id_pattern = r"<@\w+>" - if re.search(user_id_pattern, message): - message = re.sub(user_id_pattern, user_handle, message) - return message + # Callback function to process each match + def replace_with_handle(match): + user_id = match.group(1) # Extract the actual user ID without <@ and > + # Fetch user details using the provided client; adjust this to fit your actual method + user = client.users_profile_get(user=user_id) + user_handle = "@" + user["profile"]["display_name"] # Construct user handle + return user_handle # Return the user handle to replace the original match + + # Use re.sub() with the callback function to replace all matches + updated_message = re.sub(user_id_pattern, replace_with_handle, message) + return updated_message diff --git a/app/modules/incident/incident.py b/app/modules/incident/incident.py index 027073b2..f0bc8f01 100644 --- a/app/modules/incident/incident.py +++ b/app/modules/incident/incident.py @@ -460,13 +460,13 @@ def handle_reaction_added(client, ack, body, logger): user = client.users_profile_get(user=message["user"]) # get the full name of the user so that we include it into the timeline user_full_name = user["profile"]["real_name"] - user_handle = "@" + user["profile"]["display_name"] # get the current timeline section content content = get_timeline_section(document_id) + # if the message contains mentions to other slack users, replace those mentions with their name message = slack_users.replace_user_id_with_handle( - user_handle, message["text"] + client, message["text"] ) # if the message already exists in the timeline, then don't put it there again @@ -522,7 +522,6 @@ def handle_reaction_removed(client, ack, body, logger): user = client.users_profile_get(user=message["user"]) # get the user's full name user_full_name = user["profile"]["real_name"] - user_handle = "@" + user["profile"]["display_name"] # get the incident report document id from the incident channel # get and update the incident document @@ -539,9 +538,8 @@ def handle_reaction_removed(client, ack, body, logger): # Retrieve the current content of the timeline content = get_timeline_section(document_id) - message = slack_users.replace_user_id_with_handle( - user_handle, message["text"] - ) + # if the message contains mentions to other slack users, replace those mentions with their name + message = slack_users.replace_user_id_with_handle(client, message["text"]) # get a link to the message link = client.chat_getPermalink(channel=channel_id, message_ts=message_ts)[ diff --git a/app/tests/integrations/slack/test_users.py b/app/tests/integrations/slack/test_users.py index c3babe27..0aadddc9 100644 --- a/app/tests/integrations/slack/test_users.py +++ b/app/tests/integrations/slack/test_users.py @@ -62,41 +62,68 @@ def test_get_user_locale_without_user_id(): def test_replace_user_id_with_valid_handle(): + client = MagicMock() + client.users_profile_get.return_value = {"profile": {"display_name": "user"}} assert ( - users.replace_user_id_with_handle("@user", "Hello <@U12345>, how are you?") + users.replace_user_id_with_handle(client, "Hello <@U12345>, how are you?") == "Hello @user, how are you?" ) def test_replace_user_id_with_no_pattern_in_message(): + client = MagicMock() + client.users_profile_get.return_value = {"profile": {"display_name": "user"}} assert ( - users.replace_user_id_with_handle("@user", "Hello user, how are you?") + users.replace_user_id_with_handle(client, "Hello user, how are you?") == "Hello user, how are you?" ) def test_replace_user_id_with_empty_handle(): + client = MagicMock() + client.users_profile_get.return_value = {"profile": {"display_name": ""}} assert ( - users.replace_user_id_with_handle("", "Hello <@U12345>, how are you?") is None + users.replace_user_id_with_handle(client, "Hello <@U12345>, how are you?") + == "Hello @, how are you?" ) def test_replace_user_id_with_empty_message(): - assert users.replace_user_id_with_handle("@user", "") is None + client = MagicMock() + assert users.replace_user_id_with_handle(client, "") == "" -def test_replace_user_id_with_none_handle(): - assert ( - users.replace_user_id_with_handle(None, "Hello <@U12345>, how are you?") is None - ) +def test_replace_user_id_with_two_users(): + client = MagicMock() + def users_profile_get(user): + if user == "U1234": + return {"profile": {"display_name": "john_doe"}} + elif user == "U5678": + return {"profile": {"display_name": "jane_smith"}} -def test_replace_user_id_with_none_message(): - assert users.replace_user_id_with_handle("@user", None) is None + client.users_profile_get.side_effect = users_profile_get + message = "Hello, <@U1234> and <@U5678>! Welcome to the team." + expected_message = "Hello, @john_doe and @jane_smith! Welcome to the team." + assert users.replace_user_id_with_handle(client, message) == expected_message -def test_replace_multiple_user_ids_in_message(): - assert ( - users.replace_user_id_with_handle("@user", "Hi <@U12345>, meet <@U67890>") - == "Hi @user, meet @user" - ) + +def test_replace_user_id_with_multiple_users(): + client = MagicMock() + + def users_profile_get(user): + if user == "U1234": + return {"profile": {"display_name": "john_doe"}} + elif user == "U5678": + return {"profile": {"display_name": "jane_smith"}} + elif user == "U9101": + return {"profile": {"display_name": "joe_smith"}} + elif user == "U1121": + return {"profile": {"display_name": "jenn_smith"}} + + client.users_profile_get.side_effect = users_profile_get + + message = "Hello, <@U1234> and <@U5678>! Welcome to the team. Also welcome <@U9101> and <@U1121>." + expected_message = "Hello, @john_doe and @jane_smith! Welcome to the team. Also welcome @joe_smith and @jenn_smith." + assert users.replace_user_id_with_handle(client, message) == expected_message