From 704389b67f17740881300fd8aa73fac9d3757928 Mon Sep 17 00:00:00 2001 From: Sylvia McLaughlin <85905333+sylviamclaughlin@users.noreply.github.com> Date: Tue, 19 Dec 2023 14:31:08 -0800 Subject: [PATCH] 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 | 18 ++++- app/tests/commands/test_incident.py | 114 +++++++++++++++++++++++++++- 2 files changed, 127 insertions(+), 5 deletions(-) diff --git a/app/commands/incident.py b/app/commands/incident.py index 6589abf0..0752896d 100644 --- a/app/commands/incident.py +++ b/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) diff --git a/app/tests/commands/test_incident.py b/app/tests/commands/test_incident.py index 46885f58..c31727f5 100644 --- a/app/tests/commands/test_incident.py +++ b/app/tests/commands/test_incident.py @@ -733,7 +733,64 @@ 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"] ), @@ -741,6 +798,61 @@ def test_incident_submit_pulls_oncall_people_into_the_channel( ) +@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"}]