Fixing up a bug where if the incident creator is in the channel, then…

… don't invite them again (#343)

* Fixing up a bug where if the creator is already there, then itwill error out

* Linting

* Adding in an additional fix for the security people to not get invited twice as well

app/commands/incident.py

@@ -306,14 +306,24 @@ def submit(ack, view, say, body, client, logger):
     text = ":alphabet-yellow-question: Is someone `penetration or performance testing`? Please stop it to make your life easier."
     say(text=text, channel=channel_id)
 
-    # Invite oncall to channel
+    # Gather all user IDs in a list to ensure uniqueness
+    users_to_invite = []
+
+    # Add oncall users, excluding the user_id
     for user in oncall:
-        client.conversations_invite(channel=channel_id, users=user["id"])
+        if user["id"] != user_id:
+            users_to_invite.append(user["id"])
 
-    # Invite the @security users to channel
+    # Get users from the @security group
     response = client.usergroups_users_list(usergroup=SLACK_SECURITY_USER_GROUP_ID)
     if response.get("ok"):
-        client.conversations_invite(channel=channel_id, users=response["users"])
+        for security_user in response["users"]:
+            if security_user != user_id:
+                users_to_invite.append(security_user)
+
+    # Invite all collected users to the channel in a single API call
+    if users_to_invite:
+        client.conversations_invite(channel=channel_id, users=users_to_invite)
 
     text = "Run `/sre incident roles` to assign roles to the incident"
     say(text=text, channel=channel_id)

app/tests/commands/test_incident.py

@@ -733,14 +733,126 @@ def test_incident_submit_pulls_oncall_people_into_the_channel(
     client.conversations_invite.assert_has_calls(
         [
             call(channel="channel_id", users="creator_user_id"),
-            call(channel="channel_id", users="on_call_user_id"),
+            call(
+                channel="channel_id",
+                users=["on_call_user_id", "security_user_id_1", "security_user_id_2"],
+            ),
+        ]
+    )
+
+
+@patch("commands.incident.google_drive.update_incident_list")
+@patch("commands.incident.google_drive.merge_data")
+@patch("commands.incident.google_drive.create_new_incident")
+@patch("commands.incident.google_drive.list_metadata")
+@patch("commands.incident.opsgenie.get_on_call_users")
+@patch("commands.incident.log_to_sentinel")
+def test_incident_submit_does_not_invite_on_call_if_already_in_channel(
+    _log_to_sentinel_mock,
+    mock_get_on_call_users,
+    mock_list_metadata,
+    mock_create_new_incident,
+    mock_merge_data,
+    mock_update_incident_list,
+):
+    ack = MagicMock()
+    logger = MagicMock()
+    view = helper_generate_view()
+    say = MagicMock()
+    body = {"user": {"id": "creator_user_id"}, "trigger_id": "trigger_id", "view": view}
+    client = MagicMock()
+    client.conversations_create.return_value = {
+        "channel": {"id": "channel_id", "name": "channel_name"}
+    }
+    client.users_lookupByEmail.return_value = {
+        "ok": True,
+        "user": {
+            "id": "creator_user_id",
+            "profile": {"display_name_normalized": "name"},
+        },
+    }
+    client.usergroups_users_list.return_value = {
+        "ok": True,
+        "users": [
+            "security_user_id_1",
+            "security_user_id_2",
+        ],
+    }
+
+    mock_create_new_incident.return_value = "id"
+
+    mock_get_on_call_users.return_value = ["email"]
+    mock_list_metadata.return_value = {"appProperties": {"genie_schedule": "oncall"}}
+
+    incident.submit(ack, view, say, body, client, logger)
+    mock_get_on_call_users.assert_called_once_with("oncall")
+    client.users_lookupByEmail.assert_any_call(email="email")
+    client.usergroups_users_list(usergroup="SLACK_SECURITY_USER_GROUP_ID")
+    client.conversations_invite.assert_has_calls(
+        [
+            call(channel="channel_id", users="creator_user_id"),
             call(
                 channel="channel_id", users=["security_user_id_1", "security_user_id_2"]
             ),
         ]
     )
 
 
+@patch("commands.incident.google_drive.update_incident_list")
+@patch("commands.incident.google_drive.merge_data")
+@patch("commands.incident.google_drive.create_new_incident")
+@patch("commands.incident.google_drive.list_metadata")
+@patch("commands.incident.opsgenie.get_on_call_users")
+@patch("commands.incident.log_to_sentinel")
+def test_incident_submit_does_not_invite_security_group_members_already_in_channel(
+    _log_to_sentinel_mock,
+    mock_get_on_call_users,
+    mock_list_metadata,
+    mock_create_new_incident,
+    mock_merge_data,
+    mock_update_incident_list,
+):
+    ack = MagicMock()
+    logger = MagicMock()
+    view = helper_generate_view()
+    say = MagicMock()
+    body = {"user": {"id": "creator_user_id"}, "trigger_id": "trigger_id", "view": view}
+    client = MagicMock()
+    client.conversations_create.return_value = {
+        "channel": {"id": "channel_id", "name": "channel_name"}
+    }
+    client.users_lookupByEmail.return_value = {
+        "ok": True,
+        "user": {
+            "id": "on_call_user_id",
+            "profile": {"display_name_normalized": "name"},
+        },
+    }
+    client.usergroups_users_list.return_value = {
+        "ok": True,
+        "users": [
+            "creator_user_id",
+            "security_user_id_2",
+        ],
+    }
+
+    mock_create_new_incident.return_value = "id"
+
+    mock_get_on_call_users.return_value = ["email"]
+    mock_list_metadata.return_value = {"appProperties": {"genie_schedule": "oncall"}}
+
+    incident.submit(ack, view, say, body, client, logger)
+    mock_get_on_call_users.assert_called_once_with("oncall")
+    client.users_lookupByEmail.assert_any_call(email="email")
+    client.usergroups_users_list(usergroup="SLACK_SECURITY_USER_GROUP_ID")
+    client.conversations_invite.assert_has_calls(
+        [
+            call(channel="channel_id", users="creator_user_id"),
+            call(channel="channel_id", users=["on_call_user_id", "security_user_id_2"]),
+        ]
+    )
+
+
 def helper_options():
     return [{"text": {"type": "plain_text", "text": "name"}, "value": "id"}]