From 779b8e941f536c6b352287bc0e16a2595c951df5 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 5 Apr 2022 12:20:25 +0100 Subject: [PATCH] Rewrite broadcast Zendesk alert at approval time The new alert happens earlier but is otherwise the same: - We only create a ticket in Production. - We only create a ticket on approval. I took this opportunity to refactor the alert as a private function and test this specifically in detail to avoid lots of repetitive mocks, which are required when calling the main "update" function. One test I haven't preserved was for when the "names" array is empty, as this was added for a legacy data integrity scenario [^1]. [^1]: https://github.com/alphagov/notifications-api/commit/bf0bf4e31cb909623341939c00ddb1742c7a96cb --- app/broadcast_message/utils.py | 38 ++++++ app/celery/broadcast_message_tasks.py | 36 +----- tests/app/broadcast_message/test_utils.py | 97 ++++++++++++++- .../celery/test_broadcast_message_tasks.py | 113 ------------------ 4 files changed, 135 insertions(+), 149 deletions(-) diff --git a/app/broadcast_message/utils.py b/app/broadcast_message/utils.py index 3694f8e0b..3707db1d2 100644 --- a/app/broadcast_message/utils.py +++ b/app/broadcast_message/utils.py @@ -1,7 +1,11 @@ from datetime import datetime from flask import current_app +from notifications_utils.clients.zendesk.zendesk_client import ( + NotifySupportTicket, +) +from app import zendesk_client from app.celery.broadcast_message_tasks import send_broadcast_event from app.config import QueueNames from app.dao.dao_utils import dao_save_object @@ -31,6 +35,7 @@ def update_broadcast_message_status(broadcast_message, new_status, updating_user broadcast_message.status = new_status dao_save_object(broadcast_message) + _create_p1_zendesk_alert(broadcast_message) if new_status in {BroadcastStatusType.BROADCASTING, BroadcastStatusType.CANCELLED}: _create_broadcast_event(broadcast_message) @@ -57,6 +62,39 @@ def _validate_broadcast_update(broadcast_message, new_status, updating_user): ) +def _create_p1_zendesk_alert(broadcast_message): + if current_app.config['NOTIFY_ENVIRONMENT'] != 'live': + return + + if broadcast_message.status != BroadcastStatusType.BROADCASTING: + return + + message = f""" + Broadcast Sent + + https://www.notifications.service.gov.uk/services/{broadcast_message.service_id}/current-alerts/{broadcast_message.id} + + This broacast has been sent on channel {broadcast_message.service.broadcast_channel}. + This broadcast is targeted at areas {broadcast_message.areas.get("names", [])}. + + This broadcast's content starts "{broadcast_message.content[:100]}". + + If this alert is not expected refer to the runbook for instructions. + https://docs.google.com/document/d/1J99yOlfp4nQz6et0w5oJVqi-KywtIXkxrEIyq_g2XUs + """.strip() + + ticket = NotifySupportTicket( + subject='Live broadcast sent', + message=message, + ticket_type=NotifySupportTicket.TYPE_INCIDENT, + technical_ticket=True, + org_id=current_app.config['BROADCAST_ORGANISATION_ID'], + org_type='central', + service_id=str(broadcast_message.service_id) + ) + zendesk_client.send_ticket_to_zendesk(ticket) + + def _create_broadcast_event(broadcast_message): """ If the service is live and the broadcast message is not stubbed, creates a broadcast event, stores it in the diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 35fb5e8a0..9cdea6375 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -1,11 +1,8 @@ from datetime import datetime from flask import current_app -from notifications_utils.clients.zendesk.zendesk_client import ( - NotifySupportTicket, -) -from app import cbc_proxy_client, notify_celery, zendesk_client +from app import cbc_proxy_client, notify_celery from app.clients.cbc_proxy import CBCProxyRetryableException from app.config import QueueNames, TaskNames from app.dao.broadcast_message_dao import ( @@ -126,37 +123,6 @@ def check_event_makes_sense_in_sequence(broadcast_event, provider): def send_broadcast_event(broadcast_event_id): broadcast_event = dao_get_broadcast_event_by_id(broadcast_event_id) - if ( - current_app.config['NOTIFY_ENVIRONMENT'] == 'live' and - broadcast_event.message_type == BroadcastEventMessageType.ALERT - ): - broadcast_message = broadcast_event.broadcast_message - # raise a zendesk ticket to alert team that broadcast is going out. - message = '\n'.join([ - 'Broadcast Sent', - '', - f'https://www.notifications.service.gov.uk/services/{broadcast_message.service_id}/current-alerts/{broadcast_message.id}', # noqa - '', - f'This broacast has been sent on channel {broadcast_message.service.broadcast_channel}.', - f'This broadcast is targeted at areas {broadcast_message.areas.get("names", [])}.', # noqa - '' - f'This broadcast\'s content starts "{broadcast_message.content[:100]}"' - '', - 'If this alert is not expected refer to the runbook for instructions.', - 'https://docs.google.com/document/d/1J99yOlfp4nQz6et0w5oJVqi-KywtIXkxrEIyq_g2XUs', - ]) - ticket = NotifySupportTicket( - subject='Live broadcast sent', - message=message, - ticket_type=NotifySupportTicket.TYPE_INCIDENT, - technical_ticket=True, - org_id=current_app.config['BROADCAST_ORGANISATION_ID'], - org_type='central', - service_id=str(broadcast_message.service_id) - ) - zendesk_client.send_ticket_to_zendesk(ticket) - current_app.logger.error(message) - notify_celery.send_task( name=TaskNames.PUBLISH_GOVUK_ALERTS, queue=QueueNames.GOVUK_ALERTS diff --git a/tests/app/broadcast_message/test_utils.py b/tests/app/broadcast_message/test_utils.py index d35654cb8..d13246693 100644 --- a/tests/app/broadcast_message/test_utils.py +++ b/tests/app/broadcast_message/test_utils.py @@ -1,6 +1,9 @@ import pytest -from app.broadcast_message.utils import update_broadcast_message_status +from app.broadcast_message.utils import ( + _create_p1_zendesk_alert, + update_broadcast_message_status, +) from app.errors import InvalidRequest from app.models import ( BROADCAST_TYPE, @@ -13,6 +16,7 @@ from tests.app.db import ( create_template, create_user, ) +from tests.conftest import set_config def test_update_broadcast_message_status_stores_approved_by_and_approved_at_and_queues_task( @@ -360,3 +364,94 @@ def test_update_broadcast_message_status_creates_event_with_correct_content_if_b mock_task.assert_called_once_with(kwargs={'broadcast_event_id': str(alert_event.id)}, queue='broadcast-tasks') assert alert_event.transmitted_content == {"body": "tailor made emergency broadcast content"} + + +def test_update_broadcast_message_status_creates_zendesk_ticket( + mocker, + notify_api, + sample_broadcast_service +): + broadcast_message = create_broadcast_message( + service=sample_broadcast_service, + content='tailor made emergency broadcast content', + status=BroadcastStatusType.PENDING_APPROVAL, + areas={"names": ["England", "Scotland"], "simple_polygons": ['polygons']} + ) + approver = create_user(email='approver@gov.uk') + sample_broadcast_service.users.append(approver) + + mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_event.apply_async') + mock_send_ticket_to_zendesk = mocker.patch( + 'app.broadcast_message.utils.zendesk_client.send_ticket_to_zendesk', + autospec=True, + ) + + with set_config(notify_api, 'NOTIFY_ENVIRONMENT', 'live'): + update_broadcast_message_status( + broadcast_message, BroadcastStatusType.BROADCASTING, approver + ) + + mock_send_ticket_to_zendesk.assert_called_once() + + +def test_create_p1_zendesk_alert(sample_broadcast_service, mocker, notify_api): + broadcast_message = create_broadcast_message( + service=sample_broadcast_service, + content='tailor made emergency broadcast content', + status=BroadcastStatusType.BROADCASTING, + areas={"names": ["England", "Scotland"]} + ) + + mock_send_ticket_to_zendesk = mocker.patch( + 'app.broadcast_message.utils.zendesk_client.send_ticket_to_zendesk', + autospec=True, + ) + + with set_config(notify_api, 'NOTIFY_ENVIRONMENT', 'live'): + _create_p1_zendesk_alert(broadcast_message) + + ticket = mock_send_ticket_to_zendesk.call_args_list[0].args[0] + assert ticket.subject == 'Live broadcast sent' + assert ticket.ticket_type == 'incident' + assert str(broadcast_message.id) in ticket.message + assert 'channel severe' in ticket.message + assert "areas ['England', 'Scotland']" in ticket.message + assert "tailor made emergency" in ticket.message + + +def test_create_p1_zendesk_alert_doesnt_alert_when_cancelling(mocker, notify_api, sample_broadcast_service): + broadcast_message = create_broadcast_message( + service=sample_broadcast_service, + content='tailor made emergency broadcast content', + status=BroadcastStatusType.CANCELLED, + areas={"names": ["England", "Scotland"]} + ) + + mock_send_ticket_to_zendesk = mocker.patch( + 'app.broadcast_message.utils.zendesk_client.send_ticket_to_zendesk', + autospec=True, + ) + + with set_config(notify_api, 'NOTIFY_ENVIRONMENT', 'live'): + _create_p1_zendesk_alert(broadcast_message) + + mock_send_ticket_to_zendesk.assert_not_called() + + +def test_create_p1_zendesk_alert_doesnt_alert_on_staging(mocker, notify_api, sample_broadcast_service): + broadcast_message = create_broadcast_message( + service=sample_broadcast_service, + content='tailor made emergency broadcast content', + status=BroadcastStatusType.BROADCASTING, + areas={"names": ["England", "Scotland"]} + ) + + mock_send_ticket_to_zendesk = mocker.patch( + 'app.broadcast_message.utils.zendesk_client.send_ticket_to_zendesk', + autospec=True, + ) + + with set_config(notify_api, 'NOTIFY_ENVIRONMENT', 'staging'): + _create_p1_zendesk_alert(broadcast_message) + + mock_send_ticket_to_zendesk.assert_not_called() diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 8c49b19ba..d67fcfe41 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -3,11 +3,7 @@ from unittest.mock import ANY, Mock, call import pytest from celery.exceptions import Retry -from flask import current_app from freezegun import freeze_time -from notifications_utils.clients.zendesk.zendesk_client import ( - NotifySupportTicket, -) from app.celery.broadcast_message_tasks import ( BroadcastIntegrityError, @@ -41,10 +37,6 @@ def test_send_broadcast_event_queues_up_for_active_providers(mocker, notify_api, mocker.patch('app.celery.broadcast_message_tasks.notify_celery.send_task') - mock_send_ticket_to_zendesk = mocker.patch( - 'app.celery.broadcast_message_tasks.zendesk_client.send_ticket_to_zendesk', - autospec=True, - ) mock_send_broadcast_provider_message = mocker.patch( 'app.celery.broadcast_message_tasks.send_broadcast_provider_message', ) @@ -57,9 +49,6 @@ def test_send_broadcast_event_queues_up_for_active_providers(mocker, notify_api, call(kwargs={'broadcast_event_id': event.id, 'provider': 'vodafone'}, queue='broadcast-tasks') ] - # we're on test env so this isn't called - assert mock_send_ticket_to_zendesk.called is False - @pytest.mark.parametrize('message_status', [ BroadcastStatusType.BROADCASTING, @@ -71,10 +60,6 @@ def test_send_broadcast_event_calls_publish_govuk_alerts_task( template = create_template(sample_broadcast_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message(template, status=message_status) event = create_broadcast_event(broadcast_message) - mocker.patch( - 'app.celery.broadcast_message_tasks.zendesk_client.send_ticket_to_zendesk', - autospec=True, - ) mocker.patch( 'app.celery.broadcast_message_tasks.send_broadcast_provider_message', ) @@ -137,104 +122,6 @@ def test_send_broadcast_event_does_nothing_if_provider_set_on_service_isnt_enabl assert mock_send_broadcast_provider_message.apply_async.called is False -@pytest.mark.parametrize('area_data,expected_message', [ - ({'names': ['England', 'Scotland']}, ['England', 'Scotland']), - ({}, []) -]) -def test_send_broadcast_event_creates_zendesk( - area_data, - expected_message, - mocker, - notify_api, - sample_broadcast_service -): - template = create_template(sample_broadcast_service, BROADCAST_TYPE) - broadcast_message = create_broadcast_message( - template, - status=BroadcastStatusType.BROADCASTING, - areas={**area_data, 'simple_polygons': []}, - ) - event = create_broadcast_event(broadcast_message) - mock_create_ticket = mocker.spy(NotifySupportTicket, '__init__') - mocker.patch('app.celery.broadcast_message_tasks.notify_celery.send_task') - - mock_send_ticket_to_zendesk = mocker.patch( - 'app.celery.broadcast_message_tasks.zendesk_client.send_ticket_to_zendesk', - autospec=True, - ) - - mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_provider_message') - - with set_config(notify_api, 'NOTIFY_ENVIRONMENT', 'live'): - send_broadcast_event(event.id) - - mock_create_ticket.assert_called_once_with( - ANY, - subject='Live broadcast sent', - message=ANY, - ticket_type='incident', - technical_ticket=True, - org_id=current_app.config['BROADCAST_ORGANISATION_ID'], - org_type='central', - service_id=str(sample_broadcast_service.id) - ) - ticket_message = mock_create_ticket.call_args_list[0][1]['message'] - - assert str(broadcast_message.id) in ticket_message - assert 'channel severe' in ticket_message - assert f"areas {expected_message}" in ticket_message - # the start of the content from the broadcast template - assert "Dear Sir/Madam" in ticket_message - - mock_send_ticket_to_zendesk.assert_called_once() - - -def test_send_broadcast_event_doesnt_create_zendesk_when_cancelling(mocker, notify_api, sample_broadcast_service): - template = create_template(sample_broadcast_service, BROADCAST_TYPE) - broadcast_message = create_broadcast_message( - template, - status=BroadcastStatusType.BROADCASTING, - areas={'areas': ['wd20-S13002775', 'wd20-S13002773'], 'simple_polygons': []}, - ) - create_broadcast_event(broadcast_message, message_type=BroadcastEventMessageType.ALERT) - cancel_event = create_broadcast_event(broadcast_message, message_type=BroadcastEventMessageType.CANCEL) - - mocker.patch('app.celery.broadcast_message_tasks.notify_celery.send_task') - - mock_send_ticket_to_zendesk = mocker.patch( - 'app.celery.broadcast_message_tasks.zendesk_client.send_ticket_to_zendesk', - autospec=True, - ) - mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_provider_message') - - with set_config(notify_api, 'NOTIFY_ENVIRONMENT', 'live'): - send_broadcast_event(cancel_event.id) - - assert mock_send_ticket_to_zendesk.called is False - - -def test_send_broadcast_event_doesnt_create_zendesk_on_staging(mocker, notify_api, sample_broadcast_service): - template = create_template(sample_broadcast_service, BROADCAST_TYPE) - broadcast_message = create_broadcast_message(template, status=BroadcastStatusType.BROADCASTING) - event = create_broadcast_event(broadcast_message) - - mocker.patch('app.celery.broadcast_message_tasks.notify_celery.send_task') - - mock_send_ticket_to_zendesk = mocker.patch( - 'app.celery.broadcast_message_tasks.zendesk_client.send_ticket_to_zendesk', - autospec=True, - ) - mock_send_broadcast_provider_message = mocker.patch( - 'app.celery.broadcast_message_tasks.send_broadcast_provider_message', - ) - - with set_config(notify_api, 'NOTIFY_ENVIRONMENT', 'staging'): - send_broadcast_event(event.id) - - assert mock_send_broadcast_provider_message.apply_async.called is True - assert mock_send_ticket_to_zendesk.called is False - - @freeze_time('2020-08-01 12:00') @pytest.mark.parametrize('provider,provider_capitalised', [ ['ee', 'EE'],