From 949b628337f9509af0af83d56fd0dfacf6ff97df Mon Sep 17 00:00:00 2001 From: Guillaume Charest <1690085+gcharest@users.noreply.github.com> Date: Wed, 7 Aug 2024 17:37:22 +0000 Subject: [PATCH] fix: refactor using integrations instead of google_drive custom module --- app/modules/incident/incident_helper.py | 136 ++++++++++++++---- .../modules/incident/test_incident_helper.py | 107 ++++++++------ 2 files changed, 170 insertions(+), 73 deletions(-) diff --git a/app/modules/incident/incident_helper.py b/app/modules/incident/incident_helper.py index ae0f6695..2b9ce2c6 100644 --- a/app/modules/incident/incident_helper.py +++ b/app/modules/incident/incident_helper.py @@ -1,13 +1,21 @@ import json import logging +import os from slack_sdk import WebClient -from integrations import google_drive -from integrations.google_workspace import google_docs +from slack_bolt import Ack, Respond, App +from integrations.google_workspace import google_docs, google_drive, sheets from integrations.slack import channels as slack_channels from integrations.sentinel import log_to_sentinel from modules.incident import schedule_retro INCIDENT_CHANNELS_PATTERN = r"^incident-\d{4}-" +SRE_DRIVE_ID = os.environ.get("SRE_DRIVE_ID") +SRE_INCIDENT_FOLDER = os.environ.get("SRE_INCIDENT_FOLDER") +INCIDENT_TEMPLATE = os.environ.get("INCIDENT_TEMPLATE") +INCIDENT_LIST = os.environ.get("INCIDENT_LIST") +START_HEADING = "DO NOT REMOVE this line as the SRE bot needs it as a placeholder." +END_HEADING = "Trigger" + help_text = """ \n `/sre incident create-folder ` @@ -34,7 +42,7 @@ """ -def register(bot): +def register(bot: App): bot.action("add_folder_metadata")(add_folder_metadata) bot.action("view_folder_metadata")(view_folder_metadata) bot.view("view_folder_metadata_modal")(list_folders) @@ -46,7 +54,7 @@ def register(bot): bot.action("confirm_click")(confirm_click) -def handle_incident_command(args, client: WebClient, body, respond, ack): +def handle_incident_command(args, client: WebClient, body, respond: Respond, ack: Ack): if len(args) == 0: respond(help_text) return @@ -55,7 +63,11 @@ def handle_incident_command(args, client: WebClient, body, respond, ack): match action: case "create-folder": name = " ".join(args) - respond(google_drive.create_folder(name)) + folder = google_drive.create_folder(name, SRE_INCIDENT_FOLDER) + if folder: + respond(f"Folder `{folder['name']}` created.") + else: + respond(f"Failed to create folder `{name}`.") case "help": respond(help_text) case "list-folders": @@ -63,7 +75,7 @@ def handle_incident_command(args, client: WebClient, body, respond, ack): case "roles": manage_roles(client, body, ack, respond) case "close": - close_incident(client, body, ack) + close_incident(client, body, ack, respond) case "stale": stale_incidents(client, body, ack) case "schedule": @@ -74,7 +86,7 @@ def handle_incident_command(args, client: WebClient, body, respond, ack): ) -def add_folder_metadata(client, body, ack): +def add_folder_metadata(client: WebClient, body, ack): ack() folder_id = body["actions"][0]["value"] blocks = { @@ -126,7 +138,7 @@ def add_folder_metadata(client, body, ack): ) -def archive_channel_action(client, body, ack): +def archive_channel_action(client: WebClient, body, ack, respond): ack() channel_id = body["channel"]["id"] action = body["actions"][0]["value"] @@ -150,7 +162,7 @@ def archive_channel_action(client, body, ack): log_to_sentinel("incident_channel_archive_delayed", body) elif action == "archive": # Call the close_incident function to update the incident document to closed, update the spreadsheet and archive the channel - close_incident(client, channel_info, ack) + close_incident(client, channel_info, ack, respond) # log the event to sentinel log_to_sentinel("incident_channel_archived", body) elif action == "schedule_retro": @@ -160,18 +172,24 @@ def archive_channel_action(client, body, ack): log_to_sentinel("incident_retro_scheduled", body) -def delete_folder_metadata(client, body, ack): +def delete_folder_metadata(client: WebClient, body, ack): ack() folder_id = body["view"]["private_metadata"] key = body["actions"][0]["value"] - google_drive.delete_metadata(folder_id, key) + response = google_drive.delete_metadata(folder_id, key) + if not response: + logging.info(f"Failed to delete metadata `{key}`.\nResponse: {str(response)}") + else: + logging.info(f"Response: {str(response)}") body["actions"] = [{"value": folder_id}] view_folder_metadata(client, body, ack) -def list_folders(client, body, ack): +def list_folders(client: WebClient, body, ack): ack() - folders = google_drive.list_folders() + folders = google_drive.list_folders_in_folder( + SRE_INCIDENT_FOLDER, "not name contains 'Templates'" + ) folders.sort(key=lambda x: x["name"]) blocks = { "type": "modal", @@ -185,13 +203,13 @@ def list_folders(client, body, ack): client.views_open(trigger_id=body["trigger_id"], view=blocks) -def manage_roles(client, body, ack, respond): +def manage_roles(client: WebClient, body, ack, respond): ack() channel_name = body["channel_name"] channel_name = channel_name[ channel_name.startswith("incident-") and len("incident-") : ] - documents = google_drive.get_document_by_channel_name(channel_name) + documents = google_drive.get_file_by_name(channel_name) if len(documents) == 0: respond( @@ -281,7 +299,7 @@ def manage_roles(client, body, ack, respond): client.views_open(trigger_id=body["trigger_id"], view=blocks) -def save_metadata(client, body, ack, view): +def save_metadata(client: WebClient, body, ack, view): ack() folder_id = view["private_metadata"] key = view["state"]["values"]["key"]["key"]["value"] @@ -292,7 +310,7 @@ def save_metadata(client, body, ack, view): view_folder_metadata(client, body, ack) -def save_incident_roles(client, ack, view): +def save_incident_roles(client: WebClient, ack, view): ack() selected_ic = view["state"]["values"]["ic_name"]["ic_select"]["selected_user"] selected_ol = view["state"]["values"]["ol_name"]["ol_select"]["selected_user"] @@ -316,7 +334,7 @@ def save_incident_roles(client, ack, view): ) -def close_incident(client, body, ack): +def close_incident(client: WebClient, body, ack, respond): ack() # get the current chanel id and name channel_id = body["channel_id"] @@ -347,23 +365,31 @@ def close_incident(client, body, ack): response["bookmarks"][item]["link"] ) else: - logging.warning( - "No bookmark link for the incident document found for channel %s", - channel_name, - ) + warning_message = f"No bookmark link for the incident document found for channel {channel_name}" + logging.warning(warning_message) + respond(warning_message) # Update the document status to "Closed" if we can get the document if document_id != "": - google_drive.close_incident_document(document_id) + close_incident_document(document_id) else: - logging.warning( + warning_message = ( "Could not close the incident document - the document was not found." ) + logging.warning(warning_message) + respond(warning_message) # Update the spreadsheet with the current incident with status = closed - google_drive.update_spreadsheet_close_incident(return_channel_name(channel_name)) + update_succeeded = update_spreadsheet_incident_status( + return_channel_name(channel_name), "Closed" + ) + + if not update_succeeded: + warning_message = f"Could not update the incident status in the spreadsheet for channel {channel_name}" + logging.warning(warning_message) + respond(warning_message) - # Need to post the message before the channe is archived so that the message can be delivered. + # Need to post the message before the channel is archived so that the message can be delivered. client.chat_postMessage( channel=channel_id, text=f"<@{user_id}> has archived this channel 👋", @@ -384,6 +410,58 @@ def close_incident(client, body, ack): ) +def close_incident_document(document_id): + # List of possible statuses to be replaced + possible_statuses = ["In Progress", "Open", "Ready to be Reviewed", "Reviewed"] + + # Replace all possible statuses with "Closed" + changes = [ + { + "replaceAllText": { + "containsText": {"text": f"Status: {status}", "matchCase": "false"}, + "replaceText": "Status: Closed", + } + } + for status in possible_statuses + ] + return google_docs.batch_update(document_id, changes) + + +def update_spreadsheet_incident_status(channel_name, status="Closed"): + """Update the status of an incident in the incident list spreadsheet. + + Args: + channel_name (str): The name of the channel to update. + status (str): The status to update the incident to. + + Returns: + bool: True if the status was updated successfully, False otherwise. + """ + valid_statuses = ["Open", "Closed", "In Progress", "Resolved"] + if status not in valid_statuses: + logging.warning("Invalid status %s", status) + return False + sheet_name = "Sheet1" + sheet = sheets.get_values(INCIDENT_LIST, range=sheet_name) + values = sheet.get("values", []) + if len(values) == 0: + logging.warning("No incident found for channel %s", channel_name) + return False + # Find the row with the search value + for i, row in enumerate(values): + if channel_name in row: + # Update the 4th column (index 3) of the found row + update_range = ( + f"{sheet_name}!D{i+1}" # Column D, Rows are 1-indexed in Sheets + ) + updated_sheet = sheets.batch_update_values( + INCIDENT_LIST, update_range, [[status]] + ) + if updated_sheet: + return True + return False + + def stale_incidents(client, body, ack): ack() @@ -678,6 +756,7 @@ def confirm_click(ack, body, client): def view_folder_metadata(client, body, ack): ack() folder_id = body["actions"][0]["value"] + logging.info(f"Viewing metadata for folder {folder_id}") folder = google_drive.list_metadata(folder_id) blocks = { "type": "modal", @@ -804,9 +883,12 @@ def metadata_items(folder): ] -def return_channel_name(input_str): +def return_channel_name(input_str: str): # return the channel name without the incident- prefix and appending a # to the channel name prefix = "incident-" + dev_prefix = prefix + "dev-" + if input_str.startswith(dev_prefix): + return "#" + input_str[len(dev_prefix) :] if input_str.startswith(prefix): return "#" + input_str[len(prefix) :] return input_str diff --git a/app/tests/modules/incident/test_incident_helper.py b/app/tests/modules/incident/test_incident_helper.py index d7cd588f..6a6b9132 100644 --- a/app/tests/modules/incident/test_incident_helper.py +++ b/app/tests/modules/incident/test_incident_helper.py @@ -18,13 +18,24 @@ def test_handle_incident_command_with_empty_args(): @patch("modules.incident.incident_helper.google_drive.create_folder") def test_handle_incident_command_with_create_command(create_folder_mock): - create_folder_mock.return_value = "folder created" + create_folder_mock.return_value = {"id": "test_id", "name": "foo bar"} respond = MagicMock() ack = MagicMock() incident_helper.handle_incident_command( ["create-folder", "foo", "bar"], MagicMock(), MagicMock(), respond, ack ) - respond.assert_called_once_with("folder created") + respond.assert_called_once_with("Folder `foo bar` created.") + + +@patch("modules.incident.incident_helper.google_drive.create_folder") +def test_handle_incident_command_with_create_command_error(create_folder_mock): + create_folder_mock.return_value = None + respond = MagicMock() + ack = MagicMock() + incident_helper.handle_incident_command( + ["create-folder", "foo", "bar"], MagicMock(), MagicMock(), respond, ack + ) + respond.assert_called_once_with("Failed to create folder `foo bar`.") def test_handle_incident_command_with_help(): @@ -98,7 +109,8 @@ def test_archive_channel_action_ignore(mock_log_to_sentinel): "user": {"id": "user_id"}, } ack = MagicMock() - incident_helper.archive_channel_action(client, body, ack) + respond = MagicMock() + incident_helper.archive_channel_action(client, body, ack, respond) ack.assert_called_once() client.chat_update( channel="channel_id", @@ -111,10 +123,8 @@ def test_archive_channel_action_ignore(mock_log_to_sentinel): ) -@patch("modules.incident.incident_helper.google_drive.close_incident_document") -@patch( - "modules.incident.incident_helper.google_drive.update_spreadsheet_close_incident" -) +@patch("modules.incident.incident_helper.close_incident_document") +@patch("modules.incident.incident_helper.update_spreadsheet_incident_status") @patch( "integrations.google_workspace.google_docs.extract_google_doc_id", return_value="dummy_document_id", @@ -131,7 +141,8 @@ def test_archive_channel_action_archive( "user": {"id": "user_id"}, } ack = MagicMock() - incident_helper.archive_channel_action(client, body, ack) + respond = MagicMock() + incident_helper.archive_channel_action(client, body, ack, respond) assert ack.call_count == 2 mock_log_to_sentinel.assert_called_once_with("incident_channel_archived", body) @@ -166,22 +177,22 @@ def test_delete_folder_metadata(view_folder_metadata_mock, delete_metadata_mock) ) -@patch("modules.incident.incident_helper.google_drive.list_folders") +@patch("modules.incident.incident_helper.google_drive.list_folders_in_folder") @patch("modules.incident.incident_helper.folder_item") -def test_list_folders(folder_item_mock, list_folders_mock): +def test_list_folders(folder_item_mock, list_folders_in_folder_mock): client = MagicMock() body = {"trigger_id": "foo"} ack = MagicMock() - list_folders_mock.return_value = [{"id": "foo", "name": "bar"}] + list_folders_in_folder_mock.return_value = [{"id": "foo", "name": "bar"}] folder_item_mock.return_value = [["folder item"]] incident_helper.list_folders(client, body, ack) - list_folders_mock.assert_called_once() + list_folders_in_folder_mock.assert_called_once() folder_item_mock.assert_called_once_with({"id": "foo", "name": "bar"}) ack.assert_called_once() client.views_open.assert_called_once_with(trigger_id="foo", view=ANY) -@patch("modules.incident.incident_helper.google_drive.get_document_by_channel_name") +@patch("modules.incident.incident_helper.google_drive.get_file_by_name") def test_manage_roles(get_document_by_channel_name_mock): client = MagicMock() body = { @@ -200,7 +211,7 @@ def test_manage_roles(get_document_by_channel_name_mock): client.views_open.assert_called_once_with(trigger_id="trigger_id", view=ANY) -@patch("modules.incident.incident_helper.google_drive.get_document_by_channel_name") +@patch("modules.incident.incident_helper.google_drive.get_file_by_name") def test_manage_roles_with_no_result(get_document_by_channel_name_mock): client = MagicMock() body = { @@ -427,10 +438,8 @@ def test_metadata_items(): ] -@patch("modules.incident.incident_helper.google_drive.close_incident_document") -@patch( - "modules.incident.incident_helper.google_drive.update_spreadsheet_close_incident" -) +@patch("modules.incident.incident_helper.close_incident_document") +@patch("modules.incident.incident_helper.update_spreadsheet_incident_status") @patch( "integrations.google_workspace.google_docs.extract_google_doc_id", return_value="dummy_document_id", @@ -438,6 +447,7 @@ def test_metadata_items(): def test_close_incident(mock_extract_id, mock_update_spreadsheet, mock_close_document): mock_client = MagicMock() mock_ack = MagicMock() + mock_respond = MagicMock() # Mock the response of client.bookmarks_list mock_client.bookmarks_list.return_value = { @@ -462,6 +472,7 @@ def test_close_incident(mock_extract_id, mock_update_spreadsheet, mock_close_doc "user_id": "U12345", }, mock_ack, + mock_respond, ) # Assert that ack was called @@ -474,16 +485,14 @@ def test_close_incident(mock_extract_id, mock_update_spreadsheet, mock_close_doc # Assert that the Google Drive document and spreadsheet update methods were called mock_close_document.assert_called_once_with("dummy_document_id") - mock_update_spreadsheet.assert_called_once_with("#2024-01-12-test") + mock_update_spreadsheet.assert_called_once_with("#2024-01-12-test", "Closed") # Assert that the Slack client's conversations_archive method was called with the correct channel ID mock_client.conversations_archive.assert_called_once_with(channel="C12345") -@patch("modules.incident.incident_helper.google_drive.close_incident_document") -@patch( - "modules.incident.incident_helper.google_drive.update_spreadsheet_close_incident" -) +@patch("modules.incident.incident_helper.close_incident_document") +@patch("modules.incident.incident_helper.update_spreadsheet_incident_status") @patch( "integrations.google_workspace.google_docs.extract_google_doc_id", return_value=None ) @@ -492,6 +501,7 @@ def test_close_incident_no_bookmarks( ): mock_client = MagicMock() mock_ack = MagicMock() + mock_respond = MagicMock() # Mock client.bookmarks_list to return no bookmarks mock_client.bookmarks_list.return_value = {"ok": True, "bookmarks": []} @@ -505,18 +515,17 @@ def test_close_incident_no_bookmarks( "user_id": "U12345", }, mock_ack, + mock_respond, ) # Assertions to ensure that document update functions are not called as there are no bookmarks mock_extract_id.assert_not_called() mock_close_document.assert_not_called() - mock_update_spreadsheet.assert_called_once_with("#2024-01-12-test") + mock_update_spreadsheet.assert_called_once_with("#2024-01-12-test", "Closed") -@patch("modules.incident.incident_helper.google_drive.close_incident_document") -@patch( - "modules.incident.incident_helper.google_drive.update_spreadsheet_close_incident" -) +@patch("modules.incident.incident_helper.close_incident_document") +@patch("modules.incident.incident_helper.update_spreadsheet_incident_status") @patch( "integrations.google_workspace.google_docs.extract_google_doc_id", return_value=None ) @@ -525,6 +534,7 @@ def test_close_incident_no_bookmarks_error( ): mock_client = MagicMock() mock_ack = MagicMock() + mock_respond = MagicMock() # Mock client.bookmarks_list to return no bookmarks mock_client.bookmarks_list.return_value = {"ok": False, "error": "not_in_channel"} @@ -538,19 +548,20 @@ def test_close_incident_no_bookmarks_error( "user_id": "U12345", }, mock_ack, + mock_respond, ) # Assertions to ensure that document update functions are not called as there are no bookmarks mock_extract_id.assert_not_called() mock_close_document.assert_not_called() - mock_update_spreadsheet.assert_called_once_with("#2024-01-12-test") + mock_update_spreadsheet.assert_called_once_with("#2024-01-12-test", "Closed") # Test that the channel that the command is ran in, is not an incident channel. def test_close_incident_not_incident_channel(): mock_client = MagicMock() mock_ack = MagicMock() - + mock_respond = MagicMock() # Mock the response of the private message to have been posted as expected mock_client.chat_postEphemeral.return_value = {"ok": True} @@ -563,6 +574,7 @@ def test_close_incident_not_incident_channel(): "channel_name": "some-other-channel", }, mock_ack, + mock_respond, ) # Assert that ack was called @@ -579,6 +591,7 @@ def test_close_incident_not_incident_channel(): def test_close_incident_cant_send_private_message(caplog): mock_client = MagicMock() mock_ack = MagicMock() + mock_respond = MagicMock() # Mock the response of the private message to have been posted as expected mock_client.chat_postEphemeral.return_value = { @@ -601,7 +614,9 @@ def test_close_incident_cant_send_private_message(caplog): # Use the caplog fixture to capture logging with caplog.at_level(logging.ERROR): # Call the function being tested - incident_helper.close_incident(client=mock_client, body=body, ack=mock_ack) + incident_helper.close_incident( + client=mock_client, body=body, ack=mock_ack, respond=mock_respond + ) # Check that the expected error message was logged assert caplog.records # Ensure there is at least one log record @@ -617,10 +632,8 @@ def test_close_incident_cant_send_private_message(caplog): ), "Expected error message not found in log records" -@patch("modules.incident.incident_helper.google_drive.close_incident_document") -@patch( - "modules.incident.incident_helper.google_drive.update_spreadsheet_close_incident" -) +@patch("modules.incident.incident_helper.close_incident_document") +@patch("modules.incident.incident_helper.update_spreadsheet_incident_status") @patch( "integrations.google_workspace.google_docs.extract_google_doc_id", return_value="dummy_document_id", @@ -630,6 +643,8 @@ def test_conversations_archive_fail( ): mock_client = MagicMock() mock_ack = MagicMock() + mock_respond = MagicMock() + # Mock the response of client.bookmarks_list with a valid bookmark mock_client.bookmarks_list.return_value = { "ok": True, @@ -656,21 +671,20 @@ def test_conversations_archive_fail( "user_id": "U12345", }, mock_ack, + mock_respond, ) # Assertions # Ensure that the Google Drive document update method was called even if archiving fails mock_close_document.assert_called_once_with("dummy_document_id") - mock_update_spreadsheet.assert_called_once_with("#2024-01-12-test") + mock_update_spreadsheet.assert_called_once_with("#2024-01-12-test", "Closed") # Ensure that the client's conversations_archive method was called mock_client.conversations_archive.assert_called_once_with(channel="C12345") -@patch("modules.incident.incident_helper.google_drive.close_incident_document") -@patch( - "modules.incident.incident_helper.google_drive.update_spreadsheet_close_incident" -) +@patch("modules.incident.incident_helper.close_incident_document") +@patch("modules.incident.incident_helper.update_spreadsheet_incident_status") @patch( "integrations.google_workspace.google_docs.extract_google_doc_id", return_value="dummy_document_id", @@ -680,6 +694,7 @@ def test_conversations_archive_fail_error_message( ): mock_client = MagicMock() mock_ack = MagicMock() + mock_respond = MagicMock() # Mock the response of client.bookmarks_list with a valid bookmark mock_client.bookmarks_list.return_value = { "ok": True, @@ -707,12 +722,13 @@ def test_conversations_archive_fail_error_message( "user_id": "U12345", }, mock_ack, + mock_respond, ) # Assertions # Ensure that the Google Drive document update method was called even if archiving fails mock_close_document.assert_called_once_with("dummy_document_id") - mock_update_spreadsheet.assert_called_once_with("#2024-01-12-test") + mock_update_spreadsheet.assert_called_once_with("#2024-01-12-test", "Closed") # Ensure that the client's conversations_archive method was called mock_client.conversations_archive.assert_called_once_with(channel="C12345") @@ -723,10 +739,8 @@ def test_conversations_archive_fail_error_message( ) -@patch("modules.incident.incident_helper.google_drive.close_incident_document") -@patch( - "modules.incident.incident_helper.google_drive.update_spreadsheet_close_incident" -) +@patch("modules.incident.incident_helper.close_incident_document") +@patch("modules.incident.incident_helper.update_spreadsheet_incident_status") @patch( "integrations.google_workspace.google_docs.extract_google_doc_id", return_value="dummy_document_id", @@ -736,12 +750,13 @@ def test_conversations_archive_succeeds_post_message_who_archived( ): mock_client = MagicMock() mock_ack = MagicMock() + mock_respond = MagicMock() body = { "channel_id": "channel_id", "channel_name": "incident-channel_name", "user_id": "user_id", } - incident_helper.close_incident(mock_client, body, mock_ack) + incident_helper.close_incident(mock_client, body, mock_ack, mock_respond) # Mock the response of client.bookmarks_list with a valid bookmark mock_client.bookmarks_list.return_value = {