From 917110870de7906db5430ee0a286d4426529603b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 13 May 2016 16:25:05 +0100 Subject: [PATCH] Use the template version at the time the notification is created or at the time the job is created. Update notifications/sms|email endpoint to send the template version to the queue. Update the process_job celery talk to send the template version to the queue. When the send_sms|send_email task runs it will get the template by id and version. Created a data migration script to add the template_vesion column for jobs and notifications. The existing jobs and notifications are given the template_version of the current template. There is a chance this is the wrong template version, but deemed okay since the application is not live. Create unit test for the dao_get_template_versions method. Rename /template//version to /template//versions which returns all versions for that template id and service id. --- app/celery/tasks.py | 5 +- app/dao/templates_dao.py | 3 +- app/notifications/rest.py | 2 +- app/template/rest.py | 8 +- ...ersion.py => 0014_add_template_version.py} | 8 +- tests/app/celery/test_tasks.py | 195 ++++++++++++------ tests/app/conftest.py | 4 +- tests/app/dao/test_jobs_dao.py | 1 + tests/app/dao/test_notification_dao.py | 48 ++--- tests/app/dao/test_templates_dao.py | 21 +- tests/app/notifications/test_rest.py | 15 +- tests/app/template/test_rest.py | 24 +++ 12 files changed, 220 insertions(+), 114 deletions(-) rename migrations/versions/{0013_add_template_version.py => 0014_add_template_version.py} (84%) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index cee8ab46e..d05d60941 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -285,6 +285,7 @@ def send_email(service_id, notification_id, from_address, encrypted_notification notification_db_object = Notification( id=notification_id, template_id=notification['template'], + template_version=notification['template_version'], to=notification['to'], service_id=service_id, job_id=notification.get('job', None), @@ -301,7 +302,7 @@ def send_email(service_id, notification_id, from_address, encrypted_notification try: template = Template( - dao_get_template_by_id(notification['template']).__dict__, + dao_get_template_by_id(notification['template'], notification['template_version']).__dict__, values=notification.get('personalisation', {}) ) reference = provider.send_email( @@ -315,8 +316,8 @@ def send_email(service_id, notification_id, from_address, encrypted_notification except EmailClientException as e: current_app.logger.exception(e) notification_db_object.status = 'failed' + dao_update_notification(notification_db_object) - dao_update_notification(notification_db_object) current_app.logger.info( "Email {} created at {} sent at {}".format(notification_id, created_at, sent_at) ) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index 794754b82..09b35eb11 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -49,4 +49,5 @@ def dao_get_all_templates_for_service(service_id): def dao_get_template_versions(service_id, template_id): history_model = Template.get_history_model() - return history_model.query.filter_by(service_id=service_id, id=template_id).order_by(desc(history_model.version)) + return history_model.query.filter_by(service_id=service_id, id=template_id).order_by( + desc(history_model.version)).all() diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 4749203c9..e22935fe2 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -357,7 +357,7 @@ def send_notification(notification_type): ), 400 notification_id = create_uuid() - + notification.update({"template_version": template.version}) if notification_type == 'sms': send_sms.apply_async(( service_id, diff --git a/app/template/rest.py b/app/template/rest.py index d520a3e06..8ec5e85de 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -107,18 +107,18 @@ def get_template_version(service_id, template_id, version): ) ) if errors: - return json_resp(result='error', message=errors), 400 + return jsonify(result='error', message=errors), 400 return jsonify(data=data) -@template.route('//version') +@template.route('//versions') def get_template_versions(service_id, template_id): data, errors = template_history_schema.dump( - dao_get_template_versions(service_id, template_id), + dao_get_template_versions(service_id=service_id, template_id=template_id), many=True ) if errors: - return json_resp(result='error', message=errors), 400 + return jsonify(result='error', message=errors), 400 return jsonify(data=data) diff --git a/migrations/versions/0013_add_template_version.py b/migrations/versions/0014_add_template_version.py similarity index 84% rename from migrations/versions/0013_add_template_version.py rename to migrations/versions/0014_add_template_version.py index 69f5a6694..fac318797 100644 --- a/migrations/versions/0013_add_template_version.py +++ b/migrations/versions/0014_add_template_version.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 0013_add_template_version -Revises: 0010_events_table +Revision ID: 0014_add_template_version +Revises: 0012_complete_provider_details Create Date: 2016-05-11 16:00:51.478012 """ # revision identifiers, used by Alembic. -revision = '0013_add_template_version' -down_revision = '0010_events_table' +revision = '0014_add_template_version' +down_revision = '0012_complete_provider_details' from alembic import op import sqlalchemy as sa diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 31bbd4ad9..dd7ffc8c5 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -114,6 +114,8 @@ def test_should_process_sms_job(sample_job, mocker, mock_celery_remove_job): str(sample_job.id) ) assert encryption.encrypt.call_args[0][0]['to'] == '+441234123123' + assert encryption.encrypt.call_args[0][0]['template'] == str(sample_job.template.id) + assert encryption.encrypt.call_args[0][0]['template_version'] == sample_job.template.version assert encryption.encrypt.call_args[0][0]['personalisation'] == {} tasks.send_sms.apply_async.assert_called_once_with( (str(sample_job.service_id), @@ -278,6 +280,8 @@ def test_should_process_email_job(sample_email_job, mocker, mock_celery_remove_j str(sample_email_job.id) ) assert encryption.encrypt.call_args[0][0]['to'] == 'test@test.com' + assert encryption.encrypt.call_args[0][0]['template'] == str(sample_email_job.template.id) + assert encryption.encrypt.call_args[0][0]['template_version'] == sample_email_job.template.version assert encryption.encrypt.call_args[0][0]['personalisation'] == {} tasks.send_email.apply_async.assert_called_once_with( ( @@ -314,6 +318,9 @@ def test_should_process_all_sms_job(sample_job, str(sample_job_with_placeholdered_template.id) ) assert encryption.encrypt.call_args[0][0]['to'] == '+441234123120' + assert encryption.encrypt.call_args[0][0]['template'] == str(sample_job_with_placeholdered_template.template.id) + assert encryption.encrypt.call_args[0][0][ + 'template_version'] == sample_job_with_placeholdered_template.template.version assert encryption.encrypt.call_args[0][0]['personalisation'] == {'name': 'chris'} tasks.send_sms.apply_async.call_count == 10 job = jobs_dao.dao_get_job_by_id(sample_job_with_placeholdered_template.id) @@ -321,11 +328,8 @@ def test_should_process_all_sms_job(sample_job, def test_should_send_template_to_correct_sms_provider_and_persist(sample_template_with_placeholders, mocker): - notification = { - "template": sample_template_with_placeholders.id, - "to": "+447234123123", - "personalisation": {"name": "Jo"} - } + notification = _notification_json(sample_template_with_placeholders, + to="+447234123123", personalisation={"name": "Jo"}) mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") @@ -350,6 +354,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat assert persisted_notification.id == notification_id assert persisted_notification.to == '+447234123123' assert persisted_notification.template_id == sample_template_with_placeholders.id + assert persisted_notification.template_version == sample_template_with_placeholders.version assert persisted_notification.status == 'sending' assert persisted_notification.created_at == now assert persisted_notification.sent_at > now @@ -358,10 +363,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat def test_should_send_sms_without_personalisation(sample_template, mocker): - notification = { - "template": sample_template.id, - "to": "+447234123123" - } + notification = _notification_json(sample_template, "+447234123123") mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") @@ -387,10 +389,8 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif service = sample_service(notify_db, notify_db_session, user=user, restricted=True) template = sample_template(notify_db, notify_db_session, service=service) - notification = { - "template": template.id, - "to": "+447700900890" # The user’s own number, but in a different format - } + notification = _notification_json(template, "+447700900890") # The user’s own number, but in a different format + mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") @@ -416,10 +416,7 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, service = sample_service(notify_db, notify_db_session, user=user, restricted=True) template = sample_template(notify_db, notify_db_session, service=service) - notification = { - "template": template.id, - "to": "07700 900849" - } + notification = _notification_json(template, "07700 900849") mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") @@ -436,6 +433,48 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, mmg_client.send_sms.assert_not_called() +def test_send_sms_should_use_template_version_from_job_not_latest(sample_template, mocker): + notification = _notification_json(sample_template, '+447234123123') + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + version_on_notification = sample_template.version + + # Change the template + from app.dao.templates_dao import dao_update_template, dao_get_template_by_id + sample_template.content = sample_template.content + " another version of the template" + dao_update_template(sample_template) + t = dao_get_template_by_id(sample_template.id) + assert t.version > version_on_notification + + notification_id = uuid.uuid4() + now = datetime.utcnow() + send_sms( + sample_template.service_id, + notification_id, + "encrypted-in-reality", + now.strftime(DATETIME_FORMAT) + ) + + mmg_client.send_sms.assert_called_once_with( + to=format_phone_number(validate_phone_number("+447234123123")), + content="Sample service: This is a template", + reference=str(notification_id) + ) + + persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id) + assert persisted_notification.id == notification_id + assert persisted_notification.to == '+447234123123' + assert persisted_notification.template_id == sample_template.id + assert persisted_notification.template_version == version_on_notification + assert persisted_notification.template_version != sample_template.version + assert persisted_notification.created_at == now + assert persisted_notification.sent_at > now + assert persisted_notification.status == 'sending' + assert persisted_notification.sent_by == 'mmg' + assert persisted_notification.content_char_count == len("Sample service: This is a template") + + def test_should_send_email_if_restricted_service_and_valid_email(notify_db, notify_db_session, mocker): user = sample_user(notify_db, notify_db_session, email="test@restricted.com") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) @@ -445,10 +484,7 @@ def test_should_send_email_if_restricted_service_and_valid_email(notify_db, noti service=service, template_type='email') - notification = { - "template": template.id, - "to": "test@restricted.com" - } + notification = _notification_json(template, "test@restricted.com") mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email') @@ -478,10 +514,7 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address(n notify_db, notify_db_session, service=service, template_type='email', subject_line='Hello' ) - notification = { - "template": template.id, - "to": "test@example.com" - } + notification = _notification_json(template, to="test@example.com") mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email') @@ -499,11 +532,7 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address(n def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sample_job, mocker): - notification = { - "template": sample_job.template.id, - "job": sample_job.id, - "to": "+447234123123" - } + notification = _notification_json(sample_job.template, to="+447234123123", job_id=sample_job.id) mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") @@ -533,11 +562,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa def test_should_use_email_template_and_persist(sample_email_template_with_placeholders, mocker): - notification = { - "template": sample_email_template_with_placeholders.id, - "to": "my_email@my_email.com", - "personalisation": {"name": "Jo"} - } + notification = _notification_json(sample_email_template_with_placeholders, "my_email@my_email.com", {"name": "Jo"}) mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email') mocker.patch('app.aws_ses_client.get_name', return_value='ses') @@ -564,6 +589,48 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh assert persisted_notification.id == notification_id assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template_with_placeholders.id + assert persisted_notification.template_version == sample_email_template_with_placeholders.version + assert persisted_notification.created_at == now + assert persisted_notification.sent_at > now + assert persisted_notification.status == 'sending' + assert persisted_notification.sent_by == 'ses' + + +def test_send_email_should_use_template_version_from_job_not_latest(sample_email_template, mocker): + notification = _notification_json(sample_email_template, 'my_email@my_email.com') + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.aws_ses_client.send_email') + mocker.patch('app.aws_ses_client.get_name', return_value='ses') + version_on_notification = sample_email_template.version + # Change the template + from app.dao.templates_dao import dao_update_template, dao_get_template_by_id + sample_email_template.content = sample_email_template.content + " another version of the template" + + dao_update_template(sample_email_template) + t = dao_get_template_by_id(sample_email_template.id) + assert t.version > version_on_notification + notification_id = uuid.uuid4() + now = datetime.utcnow() + send_email( + sample_email_template.service_id, + notification_id, + 'email_from', + "encrypted-in-reality", + now.strftime(DATETIME_FORMAT) + ) + aws_ses_client.send_email.assert_called_once_with( + "email_from", + "my_email@my_email.com", + sample_email_template.subject, + body="This is a template", + html_body=AnyStringWith("This is a template") + ) + + persisted_notification = notifications_dao.get_notification(sample_email_template.service_id, notification_id) + assert persisted_notification.id == notification_id + assert persisted_notification.to == 'my_email@my_email.com' + assert persisted_notification.template_id == sample_email_template.id + assert persisted_notification.template_version == version_on_notification assert persisted_notification.created_at == now assert persisted_notification.sent_at > now assert persisted_notification.status == 'sending' @@ -571,11 +638,8 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh def test_should_use_email_template_subject_placeholders(sample_email_template_with_placeholders, mocker): - notification = { - "template": sample_email_template_with_placeholders.id, - "to": "my_email@my_email.com", - "personalisation": {"name": "Jo"} - } + notification = _notification_json(sample_email_template_with_placeholders, + "my_email@my_email.com", {"name": "Jo"}) mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email') mocker.patch('app.aws_ses_client.get_name', return_value='ses') @@ -609,11 +673,7 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi def test_should_use_email_template_and_persist_ses_reference(sample_email_template_with_placeholders, mocker): - notification = { - "template": sample_email_template_with_placeholders.id, - "to": "my_email@my_email.com", - "personalisation": {"name": "Jo"} - } + notification = _notification_json(sample_email_template_with_placeholders, "my_email@my_email.com", {"name": "Jo"}) mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email', return_value='reference') @@ -632,13 +692,9 @@ def test_should_use_email_template_and_persist_ses_reference(sample_email_templa assert persisted_notification.reference == 'reference' -def test_should_use_email_template_and_persist_without_personalisation( - sample_email_template, mocker -): - mocker.patch('app.encryption.decrypt', return_value={ - "template": sample_email_template.id, - "to": "my_email@my_email.com", - }) +def test_should_use_email_template_and_persist_without_personalisation(sample_email_template, mocker): + mocker.patch('app.encryption.decrypt', + return_value=_notification_json(sample_email_template, "my_email@my_email.com")) mocker.patch('app.aws_ses_client.send_email', return_value="ref") mocker.patch('app.aws_ses_client.get_name', return_value='ses') @@ -661,10 +717,7 @@ def test_should_use_email_template_and_persist_without_personalisation( def test_should_persist_notification_as_failed_if_sms_client_fails(sample_template, mocker): - notification = { - "template": sample_template.id, - "to": "+447234123123" - } + notification = _notification_json(sample_template, "+447234123123") mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms', side_effect=MMGClientException(mmg_error)) mocker.patch('app.mmg_client.get_name', return_value="mmg") @@ -687,6 +740,7 @@ def test_should_persist_notification_as_failed_if_sms_client_fails(sample_templa assert persisted_notification.id == notification_id assert persisted_notification.to == '+447234123123' assert persisted_notification.template_id == sample_template.id + assert persisted_notification.template_version == sample_template.version assert persisted_notification.status == 'failed' assert persisted_notification.created_at == now assert persisted_notification.sent_at > now @@ -694,10 +748,7 @@ def test_should_persist_notification_as_failed_if_sms_client_fails(sample_templa def test_should_persist_notification_as_failed_if_email_client_fails(sample_email_template, mocker): - notification = { - "template": sample_email_template.id, - "to": "my_email@my_email.com" - } + notification = _notification_json(sample_email_template, "my_email@my_email.com") mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email', side_effect=AwsSesClientException()) mocker.patch('app.aws_ses_client.get_name', return_value="ses") @@ -724,6 +775,7 @@ def test_should_persist_notification_as_failed_if_email_client_fails(sample_emai assert persisted_notification.id == notification_id assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template.id + assert persisted_notification.template_version == sample_email_template.version assert persisted_notification.status == 'failed' assert persisted_notification.created_at == now assert persisted_notification.sent_by == 'ses' @@ -731,10 +783,7 @@ def test_should_persist_notification_as_failed_if_email_client_fails(sample_emai def test_should_not_send_sms_if_db_peristance_failed(sample_template, mocker): - notification = { - "template": sample_template.id, - "to": "+447234123123" - } + notification = _notification_json(sample_template, "+447234123123") mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.db.session.add', side_effect=SQLAlchemyError()) @@ -755,10 +804,7 @@ def test_should_not_send_sms_if_db_peristance_failed(sample_template, mocker): def test_should_not_send_email_if_db_peristance_failed(sample_email_template, mocker): - notification = { - "template": sample_email_template.id, - "to": "my_email@my_email.com" - } + notification = _notification_json(sample_email_template, "my_email@my_email.com") mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email') mocker.patch('app.db.session.add', side_effect=SQLAlchemyError()) @@ -853,3 +899,16 @@ def test_email_reset_password_should_send_email(notify_db, notify_db_session, no reset_password_message['to'], "Reset password for GOV.UK Notify", message) + + +def _notification_json(template, to, personalisation=None, job_id=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}) + return notification diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 50b02bf43..0d9aeb8ff 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -237,13 +237,12 @@ def sample_job(notify_db, if template is None: template = sample_template(notify_db, notify_db_session, service=service) - job_id = uuid.uuid4() data = { 'id': uuid.uuid4(), 'service_id': service.id, 'service': service, 'template_id': template.id, - 'template_version': 1, + 'template_version': template.version, 'original_file_name': 'some.csv', 'notification_count': notification_count, 'created_at': created_at, @@ -286,6 +285,7 @@ def sample_email_job(notify_db, 'service_id': service.id, 'service': service, 'template_id': template.id, + 'template_version': template.version, 'original_file_name': 'some.csv', 'notification_count': 1, 'created_by': service.created_by diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index b8b03c231..5628cd73f 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -19,6 +19,7 @@ def test_create_job(sample_template): 'id': job_id, 'service_id': sample_template.service.id, 'template_id': sample_template.id, + 'template_version': sample_template.version, 'original_file_name': 'some.csv', 'notification_count': 1, 'created_by': sample_template.created_by diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 2732ece42..65ec9d7be 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -271,7 +271,7 @@ def test_save_notification_creates_sms_and_template_stats(sample_template, sampl assert NotificationStatistics.query.count() == 0 assert TemplateStatistics.query.count() == 0 - data = _notification_json(sample_template, sample_job.id) + data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) @@ -283,6 +283,7 @@ def test_save_notification_creates_sms_and_template_stats(sample_template, sampl assert data['job_id'] == notification_from_db.job_id assert data['service'] == notification_from_db.service assert data['template'] == notification_from_db.template + assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at assert 'sending' == notification_from_db.status assert Job.query.get(sample_job.id).notifications_sent == 1 @@ -308,7 +309,7 @@ def test_save_notification_and_create_email_and_template_stats(sample_email_temp assert NotificationStatistics.query.count() == 0 assert TemplateStatistics.query.count() == 0 - data = _notification_json(sample_email_template, sample_job.id) + data = _notification_json(sample_email_template, job_id=sample_job.id) notification = Notification(**data) dao_create_notification(notification, sample_email_template.template_type, ses_provider.identifier) @@ -320,6 +321,7 @@ def test_save_notification_and_create_email_and_template_stats(sample_email_temp assert data['job_id'] == notification_from_db.job_id assert data['service'] == notification_from_db.service assert data['template'] == notification_from_db.template + assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at assert 'sending' == notification_from_db.status assert Job.query.get(sample_job.id).notifications_sent == 1 @@ -359,7 +361,7 @@ def test_save_notification_handles_midnight_properly(sample_template, sample_job @freeze_time("2016-01-01 23:59:59.999999") def test_save_notification_handles_just_before_midnight_properly(sample_template, sample_job, mmg_provider): assert Notification.query.count() == 0 - data = _notification_json(sample_template, sample_job.id) + data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) @@ -375,7 +377,7 @@ def test_save_notification_handles_just_before_midnight_properly(sample_template def test_save_notification_and_increment_email_stats(sample_email_template, sample_job, ses_provider): assert Notification.query.count() == 0 - data = _notification_json(sample_email_template, sample_job.id) + data = _notification_json(sample_email_template, job_id=sample_job.id) notification_1 = Notification(**data) notification_2 = Notification(**data) @@ -435,7 +437,7 @@ def test_not_save_notification_and_not_create_stats_on_commit_error(sample_templ random_id = str(uuid.uuid4()) assert Notification.query.count() == 0 - data = _notification_json(sample_template, random_id) + data = _notification_json(sample_template, job_id=random_id) notification = Notification(**data) with pytest.raises(SQLAlchemyError): @@ -449,7 +451,7 @@ def test_not_save_notification_and_not_create_stats_on_commit_error(sample_templ def test_save_notification_and_increment_job(sample_template, sample_job, mmg_provider): assert Notification.query.count() == 0 - data = _notification_json(sample_template, sample_job.id) + data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) @@ -461,6 +463,7 @@ def test_save_notification_and_increment_job(sample_template, sample_job, mmg_pr assert data['job_id'] == notification_from_db.job_id assert data['service'] == notification_from_db.service assert data['template'] == notification_from_db.template + assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at assert 'sending' == notification_from_db.status assert Job.query.get(sample_job.id).notifications_sent == 1 @@ -474,18 +477,7 @@ def test_save_notification_and_increment_job(sample_template, sample_job, mmg_pr def test_should_not_increment_job_if_notification_fails_to_persist(sample_template, sample_job, mmg_provider): random_id = str(uuid.uuid4()) assert Notification.query.count() == 0 - data = { - 'id': random_id, - 'to': '+44709123456', - 'job_id': sample_job.id, - 'service_id': sample_template.service.id, - 'service': sample_template.service, - 'template': sample_template, - 'template_id': sample_template.id, - 'template_version': sample_template.version, - 'created_at': datetime.utcnow(), - 'content_char_count': 160 - } + data = _notification_json(sample_template, job_id=sample_job.id, id=random_id) notification_1 = Notification(**data) dao_create_notification(notification_1, sample_template.template_type, mmg_provider.identifier) @@ -508,7 +500,7 @@ def test_save_notification_and_increment_correct_job(notify_db, notify_db_sessio job_2 = sample_job(notify_db, notify_db_session, sample_template.service) assert Notification.query.count() == 0 - data = _notification_json(sample_template, job_1.id) + data = _notification_json(sample_template, job_id=job_1.id) notification = Notification(**data) dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) @@ -520,6 +512,7 @@ def test_save_notification_and_increment_correct_job(notify_db, notify_db_sessio assert data['job_id'] == notification_from_db.job_id assert data['service'] == notification_from_db.service assert data['template'] == notification_from_db.template + assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at assert 'sending' == notification_from_db.status assert Job.query.get(job_1.id).notifications_sent == 1 @@ -539,6 +532,7 @@ def test_save_notification_with_no_job(sample_template, mmg_provider): assert data['to'] == notification_from_db.to assert data['service'] == notification_from_db.service assert data['template'] == notification_from_db.template + assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at assert 'sending' == notification_from_db.status @@ -552,7 +546,6 @@ def test_get_notification(sample_notification): def test_save_notification_no_job_id(sample_template, mmg_provider): assert Notification.query.count() == 0 - to = '+44709123456' data = _notification_json(sample_template) notification = Notification(**data) @@ -564,6 +557,7 @@ def test_save_notification_no_job_id(sample_template, mmg_provider): assert data['to'] == notification_from_db.to assert data['service'] == notification_from_db.service assert data['template'] == notification_from_db.template + assert data['template_version'] == notification_from_db.template_version assert 'sending' == notification_from_db.status @@ -643,7 +637,7 @@ def test_should_not_delete_failed_notifications_before_seven_days(notify_db, not def test_save_new_notification_creates_template_stats(sample_template, sample_job, mmg_provider): assert Notification.query.count() == 0 assert TemplateStatistics.query.count() == 0 - data = _notification_json(sample_template, sample_job.id) + data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) @@ -661,7 +655,7 @@ def test_save_new_notification_creates_template_stats(sample_template, sample_jo def test_save_new_notification_creates_template_stats_per_day(sample_template, sample_job, mmg_provider): assert Notification.query.count() == 0 assert TemplateStatistics.query.count() == 0 - data = _notification_json(sample_template, sample_job.id) + data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) @@ -697,7 +691,7 @@ def test_save_new_notification_creates_template_stats_per_day(sample_template, s def test_save_another_notification_increments_template_stats(sample_template, sample_job, mmg_provider): assert Notification.query.count() == 0 assert TemplateStatistics.query.count() == 0 - data = _notification_json(sample_template, sample_job.id) + data = _notification_json(sample_template, job_id=sample_job.id) notification_1 = Notification(**data) notification_2 = Notification(**data) @@ -725,7 +719,7 @@ def test_successful_notification_inserts_followed_by_failure_does_not_increment_ assert NotificationStatistics.query.count() == 0 assert TemplateStatistics.query.count() == 0 - data = _notification_json(sample_template, sample_job.id) + data = _notification_json(sample_template, job_id=sample_job.id) notification_1 = Notification(**data) notification_2 = Notification(**data) @@ -768,7 +762,7 @@ def test_get_template_stats_for_service_returns_stats_in_reverse_date_order(samp template_stats = dao_get_template_statistics_for_service(sample_template.service.id) assert len(template_stats) == 0 - data = _notification_json(sample_template, sample_job.id) + data = _notification_json(sample_template, job_id=sample_job.id) notification = Notification(**data) dao_create_notification(notification, sample_template.template_type, mmg_provider.identifier) @@ -901,7 +895,7 @@ def test_sms_fragment_count(char_count, expected_sms_fragment_count): assert get_sms_fragment_count(char_count) == expected_sms_fragment_count -def _notification_json(sample_template, job_id=None): +def _notification_json(sample_template, job_id=None, id=None): data = { 'to': '+44709123456', 'service': sample_template.service, @@ -914,4 +908,6 @@ def _notification_json(sample_template, job_id=None): } if job_id: data.update({'job_id': job_id}) + if id: + data.update({'id': id}) return data diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index cf5591606..8327ea3e4 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -1,11 +1,10 @@ from sqlalchemy.orm.exc import NoResultFound -from sqlalchemy.exc import IntegrityError from app.dao.templates_dao import ( dao_create_template, dao_get_template_by_id_and_service_id, dao_get_all_templates_for_service, - dao_update_template -) + dao_update_template, + dao_get_template_versions) from tests.app.conftest import sample_template as create_sample_template from app.models import Template import pytest @@ -230,3 +229,19 @@ def test_get_template_history_version(sample_user, sample_service, sample_templa '1' ) assert old_template.content == old_content + + +def test_get_template_versions(sample_template): + original_content = sample_template.content + sample_template.content = 'new version' + dao_update_template(sample_template) + versions = dao_get_template_versions(service_id=sample_template.service_id, template_id=sample_template.id) + assert versions.__len__() == 2 + for x in versions: + if x.version == 2: + assert x.content == 'new version' + else: + assert x.content == original_content + from app.schemas import (template_history_schema) + v = template_history_schema.load(versions, many=True) + assert v.__len__() == 2 diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 0f8567961..6b6c812fa 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -3,6 +3,7 @@ import uuid import random import string import app.celery.tasks +from app import encryption from tests import create_authorization_header from tests.app.conftest import sample_notification as create_sample_notification from tests.app.conftest import sample_job as create_sample_job @@ -516,11 +517,10 @@ def test_send_notification_with_placeholders_replaced(notify_api, sample_templat with notify_api.test_request_context(): with notify_api.test_client() as client: mocker.patch('app.celery.tasks.send_sms.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") data = { 'to': '+447700900855', - 'template': sample_template_with_placeholders.id, + 'template': str(sample_template_with_placeholders.id), 'personalisation': { 'name': 'Jo' } @@ -533,11 +533,13 @@ def test_send_notification_with_placeholders_replaced(notify_api, sample_templat headers=[('Content-Type', 'application/json'), auth_header]) notification_id = json.loads(response.data)['data']['notification']['id'] + data.update({"template_version": sample_template_with_placeholders.version}) + encrypted_notification = encryption.encrypt(data) app.celery.tasks.send_sms.apply_async.assert_called_once_with( (str(sample_template_with_placeholders.service.id), notification_id, - "something_encrypted", + encrypted_notification, "2016-01-01T11:09:00.061258"), queue="sms" ) @@ -705,6 +707,9 @@ def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker notification_id = json.loads(response.data)['data']['notification']['id'] assert app.encryption.encrypt.call_args[0][0]['to'] == '+447700900855' + assert app.encryption.encrypt.call_args[0][0]['template'] == str(sample_template.id) + assert app.encryption.encrypt.call_args[0][0]['template_version'] == sample_template.version + app.celery.tasks.send_sms.apply_async.assert_called_once_with( (str(sample_template.service_id), notification_id, @@ -895,6 +900,9 @@ def test_should_allow_valid_email_notification(notify_api, sample_email_template headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 201 notification_id = json.loads(response.get_data(as_text=True))['data']['notification']['id'] + assert app.encryption.encrypt.call_args[0][0]['to'] == 'ok@ok.com' + assert app.encryption.encrypt.call_args[0][0]['template'] == str(sample_email_template.id) + assert app.encryption.encrypt.call_args[0][0]['template_version'] == sample_email_template.version app.celery.tasks.send_email.apply_async.assert_called_once_with( (str(sample_email_template.service_id), notification_id, @@ -903,6 +911,7 @@ def test_should_allow_valid_email_notification(notify_api, sample_email_template "2016-01-01T11:09:00.061258"), queue="email" ) + assert response.status_code == 201 assert notification_id diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index ac47ad6d7..7b5865895 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -460,3 +460,27 @@ def test_update_400_for_over_limit_content(notify_api, sample_user, sample_templ assert ( 'Content has a character count greater than the limit of {}' ).format(limit) in json_resp['message']['content'] + + +def test_should_return_all_template_versions_for_service_and_template_id(notify_api, sample_template): + original_content = sample_template.content + from app.dao.templates_dao import dao_update_template + sample_template.content = original_content + '1' + dao_update_template(sample_template) + sample_template.content = original_content + '2' + dao_update_template(sample_template) + with notify_api.test_request_context(): + with notify_api.test_client() as client: + auth_header = create_authorization_header() + resp = client.get('/service/{}/template/{}/versions'.format(sample_template.service_id, sample_template.id), + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 200 + resp_json = json.loads(resp.get_data(as_text=True))['data'] + assert len(resp_json) == 3 + for x in resp_json: + if x['version'] == 1: + assert x['content'] == original_content + elif x['version'] == 2: + assert x['content'] == original_content + '1' + else: + assert x['content'] == original_content + '2'