Fixing a bug with the SRE bot (#428)

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

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)[

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