From 7d5c25bd48b41aa5f11d7656fbce2a65f60a2f6b Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 20 Jun 2016 13:30:57 +0100 Subject: [PATCH 1/2] Update visibility timeout on staging and live --- config_live.py | 2 +- config_staging.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config_live.py b/config_live.py index 8149b724d..932db1e77 100644 --- a/config_live.py +++ b/config_live.py @@ -25,6 +25,6 @@ class Live(Config): BROKER_TRANSPORT_OPTIONS = { 'region': 'eu-west-1', 'polling_interval': 1, # 1 second - 'visibility_timeout': 60, # 60 seconds + 'visibility_timeout': 14410, # 60 seconds 'queue_name_prefix': os.environ['LIVE_NOTIFICATION_QUEUE_PREFIX'] + '-' } diff --git a/config_staging.py b/config_staging.py index 7c7d094f2..20bc762d6 100644 --- a/config_staging.py +++ b/config_staging.py @@ -21,6 +21,6 @@ class Staging(Config): BROKER_TRANSPORT_OPTIONS = { 'region': 'eu-west-1', 'polling_interval': 1, # 1 second - 'visibility_timeout': 60, # 60 seconds + 'visibility_timeout': 14410, # 60 seconds 'queue_name_prefix': os.environ['STAGING_NOTIFICATION_QUEUE_PREFIX'] + '-' } From 731bb19a9c566fcde4ae2862d80a861866c8727b Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Mon, 20 Jun 2016 16:23:56 +0100 Subject: [PATCH 2/2] Template and personalisation content is now merged and returned with notifications, when retrieved by notification id, or service id (i.e. all notifications for service). There is a new element returned at top level of notification json called body, which is the template content merged with personalisation. This is consistent with api to endpoint to create notification which returns what was sent as 'body' in json response. Merging of template with personalisation is done in the NotificationStatusSchema. Personalisation data in encrypted before storing in db. --- app/celery/tasks.py | 7 +- app/models.py | 17 +++- app/notifications/rest.py | 2 +- app/schemas.py | 24 ++++- .../versions/0031_store_personalisation.py | 26 +++++ tests/app/celery/test_tasks.py | 96 ++++++++++++++++--- tests/app/conftest.py | 6 +- tests/app/notifications/test_rest.py | 56 +++++++++++ 8 files changed, 210 insertions(+), 24 deletions(-) create mode 100644 migrations/versions/0031_store_personalisation.py diff --git a/app/celery/tasks.py b/app/celery/tasks.py index bbdc884b6..30326f9bd 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -18,7 +18,6 @@ from notifications_utils.template import Template from notifications_utils.recipients import ( RecipientCSV, - validate_and_format_phone_number, allowed_to_send_to ) @@ -233,7 +232,8 @@ def send_sms(self, service_id, notification_id, encrypted_notification, created_ job_id=notification.get('job', None), job_row_number=notification.get('row_number', None), status='sending', - created_at=datetime.strptime(created_at, DATETIME_FORMAT) + created_at=datetime.strptime(created_at, DATETIME_FORMAT), + personalisation=notification.get('personalisation') ) dao_create_notification(notification_db_object, TEMPLATE_TYPE_SMS) @@ -277,7 +277,8 @@ def send_email(service_id, notification_id, encrypted_notification, created_at, status='sending', created_at=datetime.strptime(created_at, DATETIME_FORMAT), sent_at=sent_at, - sent_by=provider.get_name() + sent_by=provider.get_name(), + personalisation=notification.get('personalisation') ) dao_create_notification(notification_db_object, TEMPLATE_TYPE_EMAIL) diff --git a/app/models.py b/app/models.py index 7a035e94e..ced18e390 100644 --- a/app/models.py +++ b/app/models.py @@ -13,7 +13,10 @@ from app.encryption import ( check_hash ) -from app import db +from app import ( + db, + encryption +) from app.history_meta import Versioned @@ -347,6 +350,18 @@ class Notification(db.Model): status = db.Column( db.Enum(*NOTIFICATION_STATUS_TYPES, name='notify_status_types'), nullable=False, default='sending') reference = db.Column(db.String, nullable=True, index=True) + _personalisation = db.Column(db.String, nullable=True) + + @property + def personalisation(self): + if self._personalisation: + return encryption.decrypt(self._personalisation) + return None + + @personalisation.setter + def personalisation(self, personalisation): + if personalisation: + self._personalisation = encryption.encrypt(personalisation) INVITED_USER_STATUS_TYPES = ['pending', 'accepted', 'cancelled'] diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 204dc789c..934f6e093 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -277,7 +277,7 @@ def send_notification(notification_type): service_stats = notifications_dao.dao_get_notification_statistics_for_service_and_day( service_id, - datetime.utcnow().strftime(DATE_FORMAT) + datetime.today().strftime(DATE_FORMAT) ) if service_stats: diff --git a/app/schemas.py b/app/schemas.py index a9d39c5c6..9ee71cf49 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -10,7 +10,8 @@ from marshmallow import ( validates, validates_schema, pre_load, - pre_dump + pre_dump, + post_dump ) from marshmallow_sqlalchemy import field_for @@ -110,6 +111,7 @@ class NotificationModelSchema(BaseSchema): class Meta: model = models.Notification strict = True + exclude = ("_personalisation",) class BaseTemplateSchema(BaseSchema): @@ -246,12 +248,30 @@ class SmsAdminNotificationSchema(SmsNotificationSchema): class NotificationStatusSchema(BaseSchema): - template = fields.Nested(TemplateSchema, only=["id", "name", "template_type"], dump_only=True) + template = fields.Nested(TemplateSchema, only=["id", "name", "template_type", "content"], dump_only=True) job = fields.Nested(JobSchema, only=["id", "original_file_name"], dump_only=True) + personalisation = fields.Dict(required=False) class Meta: model = models.Notification strict = True + exclude = ('_personalisation',) + + @pre_dump + def handle_personalisation_property(self, in_data): + if in_data.personalisation: + self.personalisation = in_data.personalisation + return in_data + + @post_dump + def handle_template_merge(self, in_data): + if in_data.get('personalisation'): + from notifications_utils.template import Template + merged = Template(in_data['template'], in_data['personalisation']).replaced + in_data['body'] = merged + in_data.pop('personalisation', None) + in_data['template'].pop('content', None) + return in_data class InvitedUserSchema(BaseSchema): diff --git a/migrations/versions/0031_store_personalisation.py b/migrations/versions/0031_store_personalisation.py new file mode 100644 index 000000000..560820bf8 --- /dev/null +++ b/migrations/versions/0031_store_personalisation.py @@ -0,0 +1,26 @@ +"""empty message + +Revision ID: 0031_store_personalisation +Revises: 0030_service_id_not_null +Create Date: 2016-06-20 10:39:50.892847 + +""" + +# revision identifiers, used by Alembic. +revision = '0031_store_personalisation' +down_revision = '0030_service_id_not_null' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.add_column('notifications', sa.Column('_personalisation', sa.String(), nullable=True)) + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.drop_column('notifications', '_personalisation') + ### end Alembic commands ### diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index dbcd52212..6d47c4307 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -45,6 +45,21 @@ class AnyStringWith(str): mmg_error = {'Error': '40', 'Description': 'error'} +def _notification_json(template, to, personalisation=None, job_id=None, row_number=None): + notification = { + "template": str(template.id), + "template_version": template.version, + "to": to, + } + if personalisation: + notification.update({"personalisation": personalisation}) + if job_id: + notification.update({"job": str(job_id)}) + if row_number: + notification['row_number'] = row_number + return notification + + # TODO moved to test_provider_tasks once send-email migrated def test_should_return_highest_priority_active_provider(notify_db, notify_db_session): providers = provider_details_dao.get_provider_details_by_notification_type('sms') @@ -894,21 +909,6 @@ def test_should_call_send_not_update_provider_email_stats_if_research_mode( providers=[ses_provider.identifier]).first() -def _notification_json(template, to, personalisation=None, job_id=None, row_number=None): - notification = { - "template": template.id, - "template_version": template.version, - "to": to, - } - if personalisation: - notification.update({"personalisation": personalisation}) - if job_id: - notification.update({"job": job_id}) - if row_number: - notification['row_number'] = row_number - return notification - - def test_update_status_of_notifications_after_timeout(notify_api, notify_db, notify_db_session, @@ -945,3 +945,69 @@ def test_not_update_status_of_notification_before_timeout(notify_api, seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') - 10)) timeout_notifications() assert not1.status == 'sending' + + +def test_email_template_personalisation_persisted(sample_email_template_with_placeholders, mocker): + encrypted_notification = encryption.encrypt(_notification_json( + sample_email_template_with_placeholders, + "my_email@my_email.com", + {"name": "Jo"}, + row_number=1)) + mocker.patch('app.statsd_client.incr') + mocker.patch('app.statsd_client.timing_with_dates') + mocker.patch('app.statsd_client.timing') + mocker.patch('app.aws_ses_client.get_name', return_value='ses') + mocker.patch('app.aws_ses_client.send_email', return_value='ses') + + notification_id = uuid.uuid4() + + send_email( + sample_email_template_with_placeholders.service_id, + notification_id, + encrypted_notification, + datetime.utcnow().strftime(DATETIME_FORMAT) + ) + + persisted_notification = notifications_dao.get_notification( + sample_email_template_with_placeholders.service_id, notification_id + ) + + assert persisted_notification.id == notification_id + assert persisted_notification.personalisation == {"name": "Jo"} + # personalisation data is encrypted in db + assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) + + +def test_sms_template_personalisation_persisted(sample_template_with_placeholders, mocker): + + encrypted_notification = encryption.encrypt(_notification_json(sample_template_with_placeholders, + to="+447234123123", personalisation={"name": "Jo"})) + mocker.patch('app.statsd_client.incr') + mocker.patch('app.statsd_client.timing_with_dates') + mocker.patch('app.statsd_client.timing') + mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') + + notification_id = uuid.uuid4() + + send_sms( + sample_template_with_placeholders.service_id, + notification_id, + encrypted_notification, + datetime.utcnow().strftime(DATETIME_FORMAT) + ) + + provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( + (sample_template_with_placeholders.service.id, + notification_id, + encrypted_notification), + queue="sms" + ) + persisted_notification = notifications_dao.get_notification( + sample_template_with_placeholders.service_id, notification_id + ) + + assert persisted_notification.id == notification_id + assert persisted_notification.to == '+447234123123' + assert persisted_notification.personalisation == {"name": "Jo"} + # personalisation data is encrypted in db + assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 47d56ef3c..8fe1a1e83 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -320,7 +320,8 @@ def sample_notification(notify_db, reference=None, created_at=datetime.utcnow(), content_char_count=160, - create=True): + create=True, + personalisation=None): if service is None: service = sample_service(notify_db, notify_db_session) if template is None: @@ -347,7 +348,8 @@ def sample_notification(notify_db, 'status': status, 'reference': reference, 'created_at': created_at, - 'content_char_count': content_char_count + 'content_char_count': content_char_count, + 'personalisation': personalisation } if job_row_number: data['job_row_number'] = job_row_number diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index aa4b05d1d..277762f27 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -72,6 +72,7 @@ def test_get_all_notifications(notify_api, sample_notification): 'id': str(sample_notification.job.id), 'original_file_name': sample_notification.job.original_file_name } + assert notifications['notifications'][0]['to'] == '+447700900855' assert notifications['notifications'][0]['service'] == str(sample_notification.service_id) assert response.status_code == 200 @@ -1227,6 +1228,61 @@ def test_firetext_callback_should_record_statsd(notify_api, notify_db, notify_db app.statsd_client.incr.assert_any_call("notifications.callback.firetext.delivered") +def test_get_notification_by_id_returns_merged_template_content(notify_db, + notify_db_session, + notify_api, + sample_template_with_placeholders): + + sample_notification = create_sample_notification(notify_db, + notify_db_session, + template=sample_template_with_placeholders, + personalisation={"name": "world"}) + with notify_api.test_request_context(): + with notify_api.test_client() as client: + auth_header = create_authorization_header(service_id=sample_notification.service_id) + + response = client.get( + '/notifications/{}'.format(sample_notification.id), + headers=[auth_header]) + + notification = json.loads(response.get_data(as_text=True))['data']['notification'] + assert response.status_code == 200 + assert notification['body'] == 'Hello world' + + +def test_get_notifications_for_service_returns_merged_template_content(notify_api, + notify_db, + notify_db_session, + sample_template_with_placeholders): + + create_sample_notification(notify_db, + notify_db_session, + service=sample_template_with_placeholders.service, + template=sample_template_with_placeholders, + personalisation={"name": "merged with first"}) + + create_sample_notification(notify_db, + notify_db_session, + service=sample_template_with_placeholders.service, + template=sample_template_with_placeholders, + personalisation={"name": "merged with second"}) + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header() + + response = client.get( + path='/service/{}/notifications'.format(sample_template_with_placeholders.service.id), + headers=[auth_header]) + assert response.status_code == 200 + + resp = json.loads(response.get_data(as_text=True)) + assert len(resp['notifications']) == 2 + assert resp['notifications'][0]['body'] == 'Hello merged with first' + assert resp['notifications'][1]['body'] == 'Hello merged with second' + + def ses_validation_code_callback(): return b'{\n "Type" : "Notification",\n "MessageId" : "ref",\n "TopicArn" : "arn:aws:sns:eu-west-1:123456789012:testing",\n "Message" : "{\\"notificationType\\":\\"Delivery\\",\\"mail\\":{\\"timestamp\\":\\"2016-03-14T12:35:25.909Z\\",\\"source\\":\\"valid-code@test.com\\",\\"sourceArn\\":\\"arn:aws:ses:eu-west-1:123456789012:identity/testing-notify\\",\\"sendingAccountId\\":\\"123456789012\\",\\"messageId\\":\\"ref\\",\\"destination\\":[\\"testing@digital.cabinet-office.gov.uk\\"]},\\"delivery\\":{\\"timestamp\\":\\"2016-03-14T12:35:26.567Z\\",\\"processingTimeMillis\\":658,\\"recipients\\":[\\"testing@digital.cabinet-office.gov.u\\"],\\"smtpResponse\\":\\"250 2.0.0 OK 1457958926 uo5si26480932wjc.221 - gsmtp\\",\\"reportingMTA\\":\\"a6-238.smtp-out.eu-west-1.amazonses.com\\"}}",\n "Timestamp" : "2016-03-14T12:35:26.665Z",\n "SignatureVersion" : "1",\n "Signature" : "X8d7eTAOZ6wlnrdVVPYanrAlsX0SMPfOzhoTEBnQqYkrNWTqQY91C0f3bxtPdUhUtOowyPAOkTQ4KnZuzphfhVb2p1MyVYMxNKcBFB05/qaCX99+92fjw4x9LeUOwyGwMv5F0Vkfi5qZCcEw69uVrhYLVSTFTrzi/yCtru+yFULMQ6UhbY09GwiP6hjxZMVr8aROQy5lLHglqQzOuSZ4KeD85JjifHdKzlx8jjQ+uj+FLzHXPMAPmPU1JK9kpoHZ1oPshAFgPDpphJe+HwcJ8ezmk+3AEUr3wWli3xF+49y8Z2anASSVp6YI2YP95UT8Rlh3qT3T+V9V8rbSVislxA==",\n "SigningCertURL" : "https://sns.eu-west-1.amazonaws.com/SimpleNotificationService-bb750dd426d95ee9390147a5624348ee.pem",\n "UnsubscribeURL" : "https://sns.eu-west-1.amazonaws.com/?Action=Unsubscribe&SubscriptionArn=arn:aws:sns:eu-west-1:302763885840:preview-emails:d6aad3ef-83d6-4cf3-a470-54e2e75916da"\n}' # noqa