From 2e929754ff572e147eb3ffd214b42a9791861d0c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 8 Jan 2021 16:38:51 +0000 Subject: [PATCH] add content to broadcast_message and make template fields nullable we want to be able to create broadcast messages without templates. To start with, these will come from the API, but in future we may want to let people create via the admin interface without creating a template too. populate a non-nullable content field with the values supplied via the template (or supplied directly if via api). --- app/broadcast_message/rest.py | 13 ++- app/models.py | 17 ++-- .../versions/0335_broadcast_msg_content.py | 27 ++++++ tests/app/broadcast_message/test_rest.py | 83 +++++++++++++++++++ .../celery/test_broadcast_message_tasks.py | 38 +++++++++ tests/app/db.py | 34 ++++++-- 6 files changed, 188 insertions(+), 24 deletions(-) create mode 100644 migrations/versions/0335_broadcast_msg_content.py diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index 3f2df6c64..141461039 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -101,16 +101,20 @@ def create_broadcast_message(service_id): user = get_user_by_id(data['created_by']) template = dao_get_template_by_id_and_service_id(data['template_id'], data['service_id']) + personalisation = data.get('personalisation', {}) broadcast_message = BroadcastMessage( service_id=service.id, template_id=template.id, template_version=template.version, - personalisation=data.get('personalisation', {}), + personalisation=personalisation, areas={"areas": data.get("areas", []), "simple_polygons": data.get("simple_polygons", [])}, status=BroadcastStatusType.DRAFT, starts_at=_parse_nullable_datetime(data.get('starts_at')), finishes_at=_parse_nullable_datetime(data.get('finishes_at')), created_by_id=user.id, + content=template._as_utils_template_with_personalisation( + personalisation + ).content_with_placeholders_filled_in, ) dao_save_object(broadcast_message) @@ -185,16 +189,11 @@ def _create_broadcast_event(broadcast_message): else: transmitted_finishes_at = broadcast_message.finishes_at - template = broadcast_message.template._as_utils_template_with_personalisation( - # Broadcast events don’t support personalisation yet - values={} - ) - event = BroadcastEvent( service=broadcast_message.service, broadcast_message=broadcast_message, message_type=msg_types[broadcast_message.status], - transmitted_content={"body": str(template)}, + transmitted_content={"body": broadcast_message.content}, transmitted_areas=broadcast_message.areas, # TODO: Probably move this somewhere more standalone too and imply that it shouldn't change. Should it include # a service based identifier too? eg "flood-warnings@notifications.service.gov.uk" or similar diff --git a/app/models.py b/app/models.py index 8102a058c..d61f725cd 100644 --- a/app/models.py +++ b/app/models.py @@ -2219,11 +2219,12 @@ class BroadcastMessage(db.Model): service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id')) service = db.relationship('Service', backref='broadcast_messages') - template_id = db.Column(UUID(as_uuid=True), nullable=False) - template_version = db.Column(db.Integer, nullable=False) + template_id = db.Column(UUID(as_uuid=True), nullable=True) + template_version = db.Column(db.Integer, nullable=True) template = db.relationship('TemplateHistory', backref='broadcast_messages') _personalisation = db.Column(db.String, nullable=True) + content = db.Column(db.String, nullable=False) # defaults to empty list areas = db.Column(JSONB(none_as_null=True), nullable=False, default=list) @@ -2252,12 +2253,6 @@ class BroadcastMessage(db.Model): approved_by = db.relationship('User', foreign_keys=[approved_by_id]) cancelled_by = db.relationship('User', foreign_keys=[cancelled_by_id]) - @property - def content(self): - return self.template._as_utils_template_with_personalisation( - self.personalisation - ).content_with_placeholders_filled_in - @property def personalisation(self): if self._personalisation: @@ -2274,12 +2269,12 @@ class BroadcastMessage(db.Model): 'service_id': str(self.service_id), - 'template_id': str(self.template_id), + 'template_id': str(self.template_id) if self.template else None, 'template_version': self.template_version, - 'template_name': self.template.name, + 'template_name': self.template.name if self.template else None, + 'personalisation': self.personalisation if self.template else None, 'content': self.content, - 'personalisation': self.personalisation, 'areas': self.areas.get("areas", []), 'simple_polygons': self.areas.get("simple_polygons", []), diff --git a/migrations/versions/0335_broadcast_msg_content.py b/migrations/versions/0335_broadcast_msg_content.py new file mode 100644 index 000000000..b3e25c75c --- /dev/null +++ b/migrations/versions/0335_broadcast_msg_content.py @@ -0,0 +1,27 @@ +""" + +Revision ID: 0335_broadcast_msg_content +Revises: 0334_broadcast_message_number +Create Date: 2020-12-04 15:06:22.544803 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0335_broadcast_msg_content' +down_revision = '0334_broadcast_message_number' + + +def upgrade(): + op.add_column('broadcast_message', sa.Column('content', sa.Text(), nullable=False)) + op.alter_column('broadcast_message', 'template_id', nullable=True) + op.alter_column('broadcast_message', 'template_version', nullable=True) + + +def downgrade(): + # downgrade fails if there are broadcasts without a template. This is deliberate cos I don't feel comfortable + # deleting broadcasts. + op.alter_column('broadcast_message', 'template_id', nullable=False) + op.alter_column('broadcast_message', 'template_version', nullable=False) + op.drop_column('broadcast_message', 'content') diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index 8cd105ebe..f1dfb0d03 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -44,6 +44,35 @@ def test_get_broadcast_message(admin_request, sample_broadcast_service): assert response['personalisation'] == {'thing': 'test'} +def test_get_broadcast_message_without_template(admin_request, sample_broadcast_service): + bm = create_broadcast_message( + service=sample_broadcast_service, + content='emergency broadcast content', + areas={ + "areas": ['place A', 'region B'], + "simple_polygons": [[[50.1, 1.2], [50.12, 1.2], [50.13, 1.2]]], + }, + ) + + response = admin_request.get( + 'broadcast_message.get_broadcast_message', + service_id=sample_broadcast_service.id, + broadcast_message_id=bm.id, + _expected_status=200 + ) + + assert response['id'] == str(bm.id) + assert response['template_id'] is None + assert response['template_version'] is None + assert response['template_name'] is None + assert response['content'] == 'emergency broadcast content' + assert response['status'] == BroadcastStatusType.DRAFT + assert response['created_at'] is not None + assert response['starts_at'] is None + assert response['areas'] == ['place A', 'region B'] + assert response['personalisation'] is None + + def test_get_broadcast_message_404s_if_message_doesnt_exist(admin_request, sample_broadcast_service): err = admin_request.get( 'broadcast_message.get_broadcast_message', @@ -148,6 +177,26 @@ def test_create_broadcast_message_400s_if_json_schema_fails_validation( assert response['errors'] == expected_errors +@freeze_time('2020-01-01') +def test_create_broadcast_message_400s_if_content_provided_but_no_template(admin_request, sample_broadcast_service): + # we don't currently support this, but might in the future + response = admin_request.post( + 'broadcast_message.create_broadcast_message', + _data={ + 'template_id': None, + 'content': 'Some tailor made broadcast content', + 'service_id': str(sample_broadcast_service.id), + 'created_by': str(sample_broadcast_service.created_by_id), + }, + service_id=sample_broadcast_service.id, + _expected_status=400 + ) + assert response['errors'] ==[ + {'error': 'ValidationError', 'message': 'template_id is not a valid UUID'}, + {'error': 'ValidationError', 'message': 'Additional properties are not allowed (content was unexpected)'}, + ] + + @pytest.mark.parametrize('status', [ BroadcastStatusType.DRAFT, BroadcastStatusType.PENDING_APPROVAL, @@ -394,6 +443,40 @@ def test_update_broadcast_message_status_stores_approved_by_and_approved_at_and_ assert alert_event.transmitted_content == {"body": "emergency broadcast"} +def test_update_broadcast_message_status_creates_event_with_correct_content_if_broadcast_has_no_template( + admin_request, + sample_broadcast_service, + mocker +): + bm = create_broadcast_message( + service=sample_broadcast_service, + template=None, + content='tailor made emergency broadcast content', + status=BroadcastStatusType.PENDING_APPROVAL, + areas={"areas": ["london"], "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]]} + ) + approver = create_user(email='approver@gov.uk') + sample_broadcast_service.users.append(approver) + mock_task = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_event.apply_async') + + response = admin_request.post( + 'broadcast_message.update_broadcast_message_status', + _data={'status': BroadcastStatusType.BROADCASTING, 'created_by': str(approver.id)}, + service_id=sample_broadcast_service.id, + broadcast_message_id=bm.id, + _expected_status=200 + ) + + assert response['status'] == BroadcastStatusType.BROADCASTING + + assert len(bm.events) == 1 + alert_event = bm.events[0] + + mock_task.assert_called_once_with(kwargs={'broadcast_event_id': str(alert_event.id)}, queue='notify-internal-tasks') + + assert alert_event.transmitted_content == {"body": "tailor made emergency broadcast content"} + + def test_update_broadcast_message_status_rejects_approval_from_creator( admin_request, sample_broadcast_service, diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index ab4e337df..cee968bb4 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -156,6 +156,44 @@ def test_send_broadcast_provider_message_sends_data_correctly( ) +@freeze_time('2020-08-01 12:00') +def test_send_broadcast_provider_message_sends_data_correctly_when_broadcast_message_has_no_template( + mocker, sample_service, +): + broadcast_message = create_broadcast_message( + service=sample_service, + template=None, + content='this is an emergency broadcast message', + areas={ + 'areas': ['london', 'glasgow'], + 'simple_polygons': [ + [[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]], + [[-4.53, 55.72], [-3.88, 55.72], [-3.88, 55.96], [-4.53, 55.96]], + ], + }, + status=BroadcastStatusType.BROADCASTING + ) + event = create_broadcast_event(broadcast_message) + + mock_create_broadcast = mocker.patch( + f'app.clients.cbc_proxy.CBCProxyEE.create_and_send_broadcast', + ) + + send_broadcast_provider_message(provider='ee', broadcast_event_id=str(event.id)) + + broadcast_provider_message = event.get_provider_message('ee') + + mock_create_broadcast.assert_called_once_with( + identifier=str(broadcast_provider_message.id), + message_number=mocker.ANY, + headline='GOV.UK Notify Broadcast', + description='this is an emergency broadcast message', + areas=mocker.ANY, + sent=mocker.ANY, + expires=mocker.ANY, + ) + + @pytest.mark.parametrize('provider,provider_capitalised', [ ['ee', 'EE'], ['vodafone', 'Vodafone'], diff --git a/tests/app/db.py b/tests/app/db.py index 432aba1fd..72824a24d 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -2,6 +2,8 @@ import random import uuid from datetime import datetime, date, timedelta +import pytest + from app import db from app.dao.email_branding_dao import dao_create_email_branding from app.dao.inbound_sms_dao import dao_create_inbound_sms @@ -1010,24 +1012,44 @@ def create_service_contact_list( def create_broadcast_message( - template, + template=None, + *, + service=None, # only used if template is not provided created_by=None, personalisation=None, + content=None, status=BroadcastStatusType.DRAFT, starts_at=None, finishes_at=None, areas=None, ): + if template: + service = template.service + template_id = template.id + template_version = template.version + personalisation = personalisation or {} + content = template._as_utils_template_with_personalisation( + personalisation + ).content_with_placeholders_filled_in + elif content: + template_id = None + template_version = None + personalisation = None + content = content + else: + pytest.fail('Provide template or content') + broadcast_message = BroadcastMessage( - service_id=template.service_id, - template_id=template.id, - template_version=template.version, - personalisation=personalisation or {}, + service_id=service.id, + template_id=template_id, + template_version=template_version, + personalisation=personalisation, status=status, starts_at=starts_at, finishes_at=finishes_at, - created_by_id=created_by.id if created_by else template.created_by_id, + created_by_id=created_by.id if created_by else service.created_by_id, areas=areas or {}, + content=content ) db.session.add(broadcast_message) db.session.commit()