From 809c5fc055738ba8c51fe130bcaa892490d92e2e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 17 Oct 2017 16:49:50 +0100 Subject: [PATCH 01/18] Remove unique constraint for ServiceSmsSenders. This will allow a service to have multiple sms senders. --- app/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index 69bce7fd5..c5938d2dd 100644 --- a/app/models.py +++ b/app/models.py @@ -300,7 +300,7 @@ class ServiceSmsSender(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) sms_sender = db.Column(db.String(11), nullable=False) - service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), unique=True, index=True, nullable=False) + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False) service = db.relationship(Service, backref=db.backref("service_sms_senders", uselist=True)) is_default = db.Column(db.Boolean, nullable=False, default=True) inbound_number_id = db.Column(UUID(as_uuid=True), db.ForeignKey('inbound_numbers.id'), From edba490c7274bf1afb3f13dbb9f82351aab03495 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 17 Oct 2017 16:53:13 +0100 Subject: [PATCH 02/18] Remove unique constraint for ServiceSmsSenders. This will allow a service to have multiple sms senders. --- .../versions/0125_remove_unique_constraint.py | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 migrations/versions/0125_remove_unique_constraint.py diff --git a/migrations/versions/0125_remove_unique_constraint.py b/migrations/versions/0125_remove_unique_constraint.py new file mode 100644 index 000000000..0039bb931 --- /dev/null +++ b/migrations/versions/0125_remove_unique_constraint.py @@ -0,0 +1,25 @@ +""" + +Revision ID: 0125_remove_unique_constraint +Revises: 0124_add_free_sms_fragment_limit +Create Date: 2017-10-17 16:47:37.826333 + +""" +from alembic import op +import sqlalchemy as sa + +revision = '0125_remove_unique_constraint' +down_revision = '0124_add_free_sms_fragment_limit' + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_index('ix_service_sms_senders_service_id', table_name='service_sms_senders') + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_index(op.f('ix_service_sms_senders_service_id'), table_name='service_sms_senders') + op.create_index('ix_service_sms_senders_service_id', 'service_sms_senders', ['service_id'], unique=True) + # ### end Alembic commands ### From 4ca6fbc72466c3e0400343fd4c12b0e01fc9a347 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 18 Oct 2017 13:13:23 +0100 Subject: [PATCH 03/18] Added dao methods needed to add or update multiple sms senders for a service. Remove the unique constraint for service on the ServiceSmsSender model. --- app/dao/service_sms_sender_dao.py | 77 +++++++++++ ...nt.py => 0126_remove_unique_constraint.py} | 9 +- tests/app/dao/test_service_sms_sender_dao.py | 120 +++++++++++++++++- 3 files changed, 200 insertions(+), 6 deletions(-) rename migrations/versions/{0125_remove_unique_constraint.py => 0126_remove_unique_constraint.py} (66%) diff --git a/app/dao/service_sms_sender_dao.py b/app/dao/service_sms_sender_dao.py index 18760cd29..6a084e644 100644 --- a/app/dao/service_sms_sender_dao.py +++ b/app/dao/service_sms_sender_dao.py @@ -32,3 +32,80 @@ def insert_service_sms_sender(service, sms_sender): is_default=True ) db.session.add(new_sms_sender) + + +def dao_get_service_sms_senders_by_id(service_id, service_sms_sender_id): + return ServiceSmsSender.query.filter_by( + id=service_sms_sender_id, + service_id=service_id + ).one() + + +def dao_get_sms_senders_by_service_id(service_id): + return ServiceSmsSender.query.filter_by(service_id=service_id).all() + + +@transactional +def dao_add_sms_sender_for_service(service_id, sms_sender, is_default, inbound_number_id=None): + old_default = _get_existing_default(service_id=service_id) + if is_default: + _reset_old_default_to_false(old_default) + else: + _raise_when_no_default(old_default) + + new_sms_sender = ServiceSmsSender( + service_id=service_id, + sms_sender=sms_sender, + is_default=is_default, + inbound_number_id=inbound_number_id + ) + + db.session.add(new_sms_sender) + return new_sms_sender + + +@transactional +def dao_update_service_sms_sender(service_id, service_sms_sender_id, is_default, sms_sender=None): + old_default = _get_existing_default(service_id) + if is_default: + _reset_old_default_to_false(old_default) + else: + if old_default.id == service_sms_sender_id: + raise Exception("You must have at least one SMS sender as the default") + + sms_sender_to_update = ServiceSmsSender.query.get(service_sms_sender_id) + sms_sender_to_update.is_default = is_default + if not sms_sender_to_update.inbound_number_id and sms_sender: + sms_sender_to_update.sms_sender = sms_sender + db.session.add(sms_sender_to_update) + return sms_sender_to_update + + +def _get_existing_default(service_id): + sms_senders = dao_get_sms_senders_by_service_id(service_id=service_id) + if sms_senders: + old_default = [x for x in sms_senders if x.is_default] + if len(old_default) == 1: + return old_default[0] + else: + raise Exception( + "There should only be one default sms sender for each service. Service {} has {}".format( + service_id, + len(old_default) + ) + ) + return None + + +def _reset_old_default_to_false(old_default): + if old_default: + old_default.is_default = False + db.session.add(old_default) + + +def _raise_when_no_default(old_default): + # check that the update is not updating the only default to false + if not old_default: + raise Exception("You must have at least one letter contact as the default.", 400) + + diff --git a/migrations/versions/0125_remove_unique_constraint.py b/migrations/versions/0126_remove_unique_constraint.py similarity index 66% rename from migrations/versions/0125_remove_unique_constraint.py rename to migrations/versions/0126_remove_unique_constraint.py index 0039bb931..ca7ec8ca2 100644 --- a/migrations/versions/0125_remove_unique_constraint.py +++ b/migrations/versions/0126_remove_unique_constraint.py @@ -1,15 +1,15 @@ """ -Revision ID: 0125_remove_unique_constraint -Revises: 0124_add_free_sms_fragment_limit +Revision ID: 0126_remove_unique_constraint +Revises: 0125_add_organisation_type Create Date: 2017-10-17 16:47:37.826333 """ from alembic import op import sqlalchemy as sa -revision = '0125_remove_unique_constraint' -down_revision = '0124_add_free_sms_fragment_limit' +revision = '0126_remove_unique_constraint' +down_revision = '0125_add_organisation_type' def upgrade(): @@ -20,6 +20,5 @@ def upgrade(): def downgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.drop_index(op.f('ix_service_sms_senders_service_id'), table_name='service_sms_senders') op.create_index('ix_service_sms_senders_service_id', 'service_sms_senders', ['service_id'], unique=True) # ### end Alembic commands ### diff --git a/tests/app/dao/test_service_sms_sender_dao.py b/tests/app/dao/test_service_sms_sender_dao.py index 5c04629c6..ebe641786 100644 --- a/tests/app/dao/test_service_sms_sender_dao.py +++ b/tests/app/dao/test_service_sms_sender_dao.py @@ -1,4 +1,12 @@ -from app.dao.service_sms_sender_dao import insert_or_update_service_sms_sender +import uuid + +import pytest +from sqlalchemy.exc import SQLAlchemyError + +from app.dao.service_sms_sender_dao import ( + insert_or_update_service_sms_sender, + dao_add_sms_sender_for_service, + dao_update_service_sms_sender, dao_get_service_sms_senders_by_id, dao_get_sms_senders_by_service_id) from app.models import ServiceSmsSender from tests.app.db import create_service @@ -27,3 +35,113 @@ def test_create_service_inserts_new_service_sms_sender(notify_db_session): assert len(service_sms_senders) == 1 assert service_sms_senders[0].sms_sender == 'new_sms' assert service_sms_senders[0].is_default + + +def test_dao_get_service_sms_senders_id(notify_db_session): + service = create_service(sms_sender='first_sms') + second_sender = dao_add_sms_sender_for_service(service_id=service.id, + sms_sender='second', + is_default=False, + inbound_number_id=None) + result = dao_get_service_sms_senders_by_id(service_id=service.id, + service_sms_sender_id=second_sender.id) + assert result.sms_sender == "second" + assert not result.is_default + + +def test_dao_get_service_sms_senders_id_raise_exception_when_not_found(notify_db_session): + service = create_service() + with pytest.raises(expected_exception=SQLAlchemyError): + dao_get_service_sms_senders_by_id(service_id=service.id, + service_sms_sender_id=uuid.uuid4()) + + +def test_dao_get_sms_senders_by_service_id(notify_db_session): + service = create_service(sms_sender='first_sms') + second_sender = dao_add_sms_sender_for_service(service_id=service.id, + sms_sender='second', + is_default=False, + inbound_number_id=None) + results = dao_get_sms_senders_by_service_id(service_id=service.id) + assert len(results) == 2 + for x in results: + if x.is_default: + x.sms_sender = 'first_sms' + else: + x == second_sender + + +def test_dao_add_sms_sender_for_service(notify_db_session): + service = create_service(sms_sender="first_sms") + new_sms_sender = dao_add_sms_sender_for_service(service_id=service.id, + sms_sender='new_sms', + is_default=False, + inbound_number_id=None) + + service_sms_senders = ServiceSmsSender.query.order_by(ServiceSmsSender.created_at).all() + assert len(service_sms_senders) == 2 + assert service_sms_senders[0].sms_sender == 'first_sms' + assert service_sms_senders[0].is_default + assert service_sms_senders[1] == new_sms_sender + + +def test_dao_add_sms_sender_for_service_switches_default(notify_db_session): + service = create_service(sms_sender="first_sms") + new_sms_sender = dao_add_sms_sender_for_service(service_id=service.id, + sms_sender='new_sms', + is_default=True, + inbound_number_id=None) + + service_sms_senders = ServiceSmsSender.query.order_by(ServiceSmsSender.created_at).all() + assert len(service_sms_senders) == 2 + assert service_sms_senders[0].sms_sender == 'first_sms' + assert not service_sms_senders[0].is_default + assert service_sms_senders[1] == new_sms_sender + + +def test_dao_update_service_sms_sender(notify_db_session): + service = create_service(sms_sender='first_sms') + service_sms_senders = ServiceSmsSender.query.filter_by(service_id =service.id).all() + assert len(service_sms_senders) == 1 + sms_sender_to_update = service_sms_senders[0] + + dao_update_service_sms_sender(service_id=service.id, + service_sms_sender_id=sms_sender_to_update.id, + is_default=True, + sms_sender="updated") + sms_senders = ServiceSmsSender.query.filter_by(service_id=service.id).all() + assert len(sms_senders) == 1 + assert sms_senders[0].is_default + assert sms_senders[0].sms_sender == 'updated' + assert not sms_senders[0].inbound_number_id + + +def test_dao_update_service_sms_sender_switches_default(notify_db_session): + service = create_service(sms_sender='first_sms') + sms_sender = dao_add_sms_sender_for_service(service_id=service.id, + sms_sender='new_sms', + is_default=False, + inbound_number_id=None) + dao_update_service_sms_sender(service_id=service.id, + service_sms_sender_id=sms_sender.id, + is_default=True, + sms_sender="updated") + sms_senders = ServiceSmsSender.query.filter_by(service_id=service.id).order_by(ServiceSmsSender.created_at).all() + assert len(sms_senders) == 2 + assert sms_senders[0].sms_sender == 'first_sms' + assert not sms_senders[0].is_default + assert sms_senders[1].sms_sender == 'updated' + assert sms_senders[1].is_default + + +def test_dao_update_service_sms_sender_raises_exception_when_no_default_after_update(notify_db_session): + service = create_service(sms_sender='first_sms') + sms_sender = dao_add_sms_sender_for_service(service_id=service.id, + sms_sender='new_sms', + is_default=True, + inbound_number_id=None) + with pytest.raises(expected_exception=Exception) as e: + dao_update_service_sms_sender(service_id=service.id, + service_sms_sender_id=sms_sender.id, + is_default=False, + sms_sender="updated") From 4ac2dc36064b824b20459cd95676843d8034832d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 18 Oct 2017 13:17:52 +0100 Subject: [PATCH 04/18] Fix code style --- app/dao/service_sms_sender_dao.py | 2 -- tests/app/dao/test_service_sms_sender_dao.py | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/dao/service_sms_sender_dao.py b/app/dao/service_sms_sender_dao.py index 6a084e644..a2c4fa9ea 100644 --- a/app/dao/service_sms_sender_dao.py +++ b/app/dao/service_sms_sender_dao.py @@ -107,5 +107,3 @@ def _raise_when_no_default(old_default): # check that the update is not updating the only default to false if not old_default: raise Exception("You must have at least one letter contact as the default.", 400) - - diff --git a/tests/app/dao/test_service_sms_sender_dao.py b/tests/app/dao/test_service_sms_sender_dao.py index ebe641786..a3d05e021 100644 --- a/tests/app/dao/test_service_sms_sender_dao.py +++ b/tests/app/dao/test_service_sms_sender_dao.py @@ -101,7 +101,7 @@ def test_dao_add_sms_sender_for_service_switches_default(notify_db_session): def test_dao_update_service_sms_sender(notify_db_session): service = create_service(sms_sender='first_sms') - service_sms_senders = ServiceSmsSender.query.filter_by(service_id =service.id).all() + service_sms_senders = ServiceSmsSender.query.filter_by(service_id=service.id).all() assert len(service_sms_senders) == 1 sms_sender_to_update = service_sms_senders[0] From 0c9a4bce595f4e186c6e0fc4d29eb1439eec4103 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 18 Oct 2017 17:00:37 +0100 Subject: [PATCH 05/18] Add celery tasks- save_sms, save_email, save_letter Created three new celery tasks: * save_sms (will replace send_sms) * save_email (will replace send_email) * save_letter (will replace persist_letter) The difference between the new tasks and the tasks they are replacing is that we no longer pass in the datetime as a parameter. The code has been changed to use the new tasks, and the tests now run against the new tasks too. The old tasks will need be removed in a separate commit. --- app/celery/tasks.py | 133 +++++++++++++++++++- tests/app/celery/test_tasks.py | 223 ++++++++++++++------------------- 2 files changed, 222 insertions(+), 134 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index f3244edb5..46a145ad7 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -148,9 +148,9 @@ def process_row(row_number, recipient, personalisation, template, job, service): }) send_fns = { - SMS_TYPE: send_sms, - EMAIL_TYPE: send_email, - LETTER_TYPE: persist_letter + SMS_TYPE: save_sms, + EMAIL_TYPE: save_email, + LETTER_TYPE: save_letter } send_fn = send_fns[template_type] @@ -160,7 +160,6 @@ def process_row(row_number, recipient, personalisation, template, job, service): str(service.id), create_uuid(), encrypted, - datetime.utcnow().strftime(DATETIME_FORMAT) ), queue=QueueNames.DATABASE if not service.research_mode else QueueNames.RESEARCH_MODE ) @@ -228,6 +227,55 @@ def send_sms(self, handle_exception(self, notification, notification_id, e) +@notify_celery.task(bind=True, name="save-sms", max_retries=5, default_retry_delay=300) +@statsd(namespace="tasks") +def save_sms(self, + service_id, + notification_id, + encrypted_notification, + api_key_id=None, + key_type=KEY_TYPE_NORMAL): + notification = encryption.decrypt(encrypted_notification) + service = dao_fetch_service_by_id(service_id) + + if not service_allowed_to_send_to(notification['to'], service, key_type): + current_app.logger.info( + "SMS {} failed as restricted service".format(notification_id) + ) + return + + try: + saved_notification = persist_notification( + template_id=notification['template'], + template_version=notification['template_version'], + recipient=notification['to'], + service=service, + personalisation=notification.get('personalisation'), + notification_type=SMS_TYPE, + api_key_id=api_key_id, + key_type=key_type, + created_at=datetime.utcnow(), + job_id=notification.get('job', None), + job_row_number=notification.get('row_number', None), + notification_id=notification_id + ) + + provider_tasks.deliver_sms.apply_async( + [str(saved_notification.id)], + queue=QueueNames.SEND_SMS if not service.research_mode else QueueNames.RESEARCH_MODE + ) + + current_app.logger.info( + "SMS {} created at {} for job {}".format( + saved_notification.id, + saved_notification.created_at, + notification.get('job', None)) + ) + + except SQLAlchemyError as e: + handle_exception(self, notification, notification_id, e) + + @notify_celery.task(bind=True, name="send-email", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") def send_email(self, @@ -270,6 +318,47 @@ def send_email(self, handle_exception(self, notification, notification_id, e) +@notify_celery.task(bind=True, name="save-email", max_retries=5, default_retry_delay=300) +@statsd(namespace="tasks") +def save_email(self, + service_id, + notification_id, + encrypted_notification, + api_key_id=None, + key_type=KEY_TYPE_NORMAL): + notification = encryption.decrypt(encrypted_notification) + service = dao_fetch_service_by_id(service_id) + + if not service_allowed_to_send_to(notification['to'], service, key_type): + current_app.logger.info("Email {} failed as restricted service".format(notification_id)) + return + + try: + saved_notification = persist_notification( + template_id=notification['template'], + template_version=notification['template_version'], + recipient=notification['to'], + service=service, + personalisation=notification.get('personalisation'), + notification_type=EMAIL_TYPE, + api_key_id=api_key_id, + key_type=key_type, + created_at=datetime.utcnow(), + job_id=notification.get('job', None), + job_row_number=notification.get('row_number', None), + notification_id=notification_id + ) + + provider_tasks.deliver_email.apply_async( + [str(saved_notification.id)], + queue=QueueNames.SEND_EMAIL if not service.research_mode else QueueNames.RESEARCH_MODE + ) + + current_app.logger.info("Email {} created at {}".format(saved_notification.id, saved_notification.created_at)) + except SQLAlchemyError as e: + handle_exception(self, notification, notification_id, e) + + @notify_celery.task(bind=True, name="persist-letter", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") def persist_letter( @@ -307,6 +396,42 @@ def persist_letter( handle_exception(self, notification, notification_id, e) +@notify_celery.task(bind=True, name="save-letter", max_retries=5, default_retry_delay=300) +@statsd(namespace="tasks") +def save_letter( + self, + service_id, + notification_id, + encrypted_notification, +): + notification = encryption.decrypt(encrypted_notification) + + # we store the recipient as just the first item of the person's address + recipient = notification['personalisation']['addressline1'] + + service = dao_fetch_service_by_id(service_id) + try: + saved_notification = persist_notification( + template_id=notification['template'], + template_version=notification['template_version'], + recipient=recipient, + service=service, + personalisation=notification['personalisation'], + notification_type=LETTER_TYPE, + api_key_id=None, + key_type=KEY_TYPE_NORMAL, + created_at=datetime.utcnow(), + job_id=notification['job'], + job_row_number=notification['row_number'], + notification_id=notification_id, + reference=create_random_identifier() + ) + + current_app.logger.info("Letter {} created at {}".format(saved_notification.id, saved_notification.created_at)) + except SQLAlchemyError as e: + handle_exception(self, notification, notification_id, e) + + @notify_celery.task(bind=True, name="build-dvla-file", countdown=60, max_retries=15, default_retry_delay=300) @statsd(namespace="tasks") def build_dvla_file(self, job_id): diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 6841f69e7..e018a4f32 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -19,9 +19,9 @@ from app.celery.tasks import ( create_dvla_file_contents_for_job, process_job, process_row, - send_sms, - send_email, - persist_letter, + save_sms, + save_email, + save_letter, process_incomplete_job, process_incomplete_jobs, get_template_class, @@ -86,8 +86,9 @@ def _notification_json(template, to, personalisation=None, job_id=None, row_numb def test_should_have_decorated_tasks_functions(): assert process_job.__wrapped__.__name__ == 'process_job' - assert send_sms.__wrapped__.__name__ == 'send_sms' - assert send_email.__wrapped__.__name__ == 'send_email' + assert save_sms.__wrapped__.__name__ == 'save_sms' + assert save_email.__wrapped__.__name__ == 'save_email' + assert save_letter.__wrapped__.__name__ == 'save_letter' @pytest.fixture @@ -98,10 +99,9 @@ def email_job_with_placeholders(notify_db, notify_db_session, sample_email_templ # -------------- process_job tests -------------- # -@freeze_time("2016-01-01 11:09:00.061258") def test_should_process_sms_job(sample_job, mocker): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms')) - mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('app.celery.tasks.save_sms.apply_async') mocker.patch('app.encryption.encrypt', return_value="something_encrypted") mocker.patch('app.celery.tasks.build_dvla_file') mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") @@ -117,11 +117,10 @@ def test_should_process_sms_job(sample_job, mocker): assert encryption.encrypt.call_args[0][0]['template_version'] == sample_job.template.version assert encryption.encrypt.call_args[0][0]['personalisation'] == {'phonenumber': '+441234123123'} assert encryption.encrypt.call_args[0][0]['row_number'] == 0 - tasks.send_sms.apply_async.assert_called_once_with( + tasks.save_sms.apply_async.assert_called_once_with( (str(sample_job.service_id), "uuid", - "something_encrypted", - "2016-01-01T11:09:00.061258Z"), + "something_encrypted"), queue="database-tasks" ) job = jobs_dao.dao_get_job_by_id(sample_job.id) @@ -223,7 +222,6 @@ def test_should_not_process_job_if_already_pending(notify_db, notify_db_session, tasks.build_dvla_file.assert_not_called() -@freeze_time("2016-01-01 11:09:00.061258") def test_should_process_email_job_if_exactly_on_send_limits(notify_db, notify_db_session, mocker): @@ -232,7 +230,7 @@ def test_should_process_email_job_if_exactly_on_send_limits(notify_db, job = create_sample_job(notify_db, notify_db_session, service=service, template=template, notification_count=10) mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_email')) - mocker.patch('app.celery.tasks.send_email.apply_async') + mocker.patch('app.celery.tasks.save_email.apply_async') mocker.patch('app.encryption.encrypt', return_value="something_encrypted") mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") @@ -244,20 +242,19 @@ def test_should_process_email_job_if_exactly_on_send_limits(notify_db, ) job = jobs_dao.dao_get_job_by_id(job.id) assert job.job_status == 'finished' - tasks.send_email.apply_async.assert_called_with( + tasks.save_email.apply_async.assert_called_with( ( str(job.service_id), "uuid", "something_encrypted", - "2016-01-01T11:09:00.061258Z" ), queue="database-tasks" ) -def test_should_not_create_send_task_for_empty_file(sample_job, mocker): +def test_should_not_create_save_task_for_empty_file(sample_job, mocker): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('empty')) - mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('app.celery.tasks.save_sms.apply_async') process_job(sample_job.id) @@ -267,16 +264,15 @@ def test_should_not_create_send_task_for_empty_file(sample_job, mocker): ) job = jobs_dao.dao_get_job_by_id(sample_job.id) assert job.job_status == 'finished' - assert tasks.send_sms.apply_async.called is False + assert tasks.save_sms.apply_async.called is False -@freeze_time("2016-01-01 11:09:00.061258") def test_should_process_email_job(email_job_with_placeholders, mocker): email_csv = """email_address,name test@test.com,foo """ mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=email_csv) - mocker.patch('app.celery.tasks.send_email.apply_async') + mocker.patch('app.celery.tasks.save_email.apply_async') mocker.patch('app.encryption.encrypt', return_value="something_encrypted") mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") @@ -290,12 +286,11 @@ def test_should_process_email_job(email_job_with_placeholders, mocker): assert encryption.encrypt.call_args[0][0]['template'] == str(email_job_with_placeholders.template.id) assert encryption.encrypt.call_args[0][0]['template_version'] == email_job_with_placeholders.template.version assert encryption.encrypt.call_args[0][0]['personalisation'] == {'emailaddress': 'test@test.com', 'name': 'foo'} - tasks.send_email.apply_async.assert_called_once_with( + tasks.save_email.apply_async.assert_called_once_with( ( str(email_job_with_placeholders.service_id), "uuid", "something_encrypted", - "2016-01-01T11:09:00.061258Z" ), queue="database-tasks" ) @@ -309,7 +304,6 @@ def test_should_process_letter_job(sample_letter_job, mocker): A1,A2,A3,A4,A_POST,Alice """ s3_mock = mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=csv) - mocker.patch('app.celery.tasks.send_email.apply_async') process_row_mock = mocker.patch('app.celery.tasks.process_row') mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") mocker.patch('app.celery.tasks.build_dvla_file') @@ -344,7 +338,7 @@ def test_should_process_letter_job(sample_letter_job, mocker): def test_should_process_all_sms_job(sample_job_with_placeholdered_template, mocker): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) - mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('app.celery.tasks.save_sms.apply_async') mocker.patch('app.encryption.encrypt', return_value="something_encrypted") mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") @@ -359,7 +353,7 @@ def test_should_process_all_sms_job(sample_job_with_placeholdered_template, assert encryption.encrypt.call_args[0][0][ 'template_version'] == sample_job_with_placeholdered_template.template.version # noqa assert encryption.encrypt.call_args[0][0]['personalisation'] == {'phonenumber': '+441234123120', 'name': 'chris'} - assert tasks.send_sms.apply_async.call_count == 10 + assert tasks.save_sms.apply_async.call_count == 10 job = jobs_dao.dao_get_job_by_id(sample_job_with_placeholdered_template.id) assert job.job_status == 'finished' @@ -367,14 +361,13 @@ def test_should_process_all_sms_job(sample_job_with_placeholdered_template, # -------------- process_row tests -------------- # -@freeze_time('2001-01-01T12:00:00') @pytest.mark.parametrize('template_type, research_mode, expected_function, expected_queue', [ - (SMS_TYPE, False, 'send_sms', 'database-tasks'), - (SMS_TYPE, True, 'send_sms', 'research-mode-tasks'), - (EMAIL_TYPE, False, 'send_email', 'database-tasks'), - (EMAIL_TYPE, True, 'send_email', 'research-mode-tasks'), - (LETTER_TYPE, False, 'persist_letter', 'database-tasks'), - (LETTER_TYPE, True, 'persist_letter', 'research-mode-tasks'), + (SMS_TYPE, False, 'save_sms', 'database-tasks'), + (SMS_TYPE, True, 'save_sms', 'research-mode-tasks'), + (EMAIL_TYPE, False, 'save_email', 'database-tasks'), + (EMAIL_TYPE, True, 'save_email', 'research-mode-tasks'), + (LETTER_TYPE, False, 'save_letter', 'database-tasks'), + (LETTER_TYPE, True, 'save_letter', 'research-mode-tasks'), ]) def test_process_row_sends_letter_task(template_type, research_mode, expected_function, expected_queue, mocker): mocker.patch('app.celery.tasks.create_uuid', return_value='noti_uuid') @@ -400,11 +393,10 @@ def test_process_row_sends_letter_task(template_type, research_mode, expected_fu 'noti_uuid', # encrypted data encrypt_mock.return_value, - '2001-01-01T12:00:00.000000Z' ), queue=expected_queue ) -# -------- send_sms and send_email tests -------- # +# -------- save_sms and save_email tests -------- # def test_should_send_template_to_correct_sms_task_and_persist(sample_template_with_placeholders, mocker): @@ -413,11 +405,10 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - send_sms( + save_sms( sample_template_with_placeholders.service_id, uuid.uuid4(), encryption.encrypt(notification), - datetime.utcnow().strftime(DATETIME_FORMAT) ) persisted_notification = Notification.query.one() @@ -438,7 +429,7 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi ) -def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_service(notify_db, notify_db_session, mocker): +def test_should_put_save_sms_task_in_research_mode_queue_if_research_mode_service(notify_db, notify_db_session, mocker): service = create_sample_service(notify_db, notify_db_session) service.research_mode = True services_dao.dao_update_service(service) @@ -451,11 +442,10 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic notification_id = uuid.uuid4() - send_sms( + save_sms( template.service_id, notification_id, encryption.encrypt(notification), - datetime.utcnow().strftime(DATETIME_FORMAT) ) persisted_notification = Notification.query.one() provider_tasks.deliver_sms.apply_async.assert_called_once_with( @@ -465,7 +455,7 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic assert mocked_deliver_sms.called -def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notify_db_session, mocker): +def test_should_save_sms_if_restricted_service_and_valid_number(notify_db, notify_db_session, mocker): user = create_user(mobile_number="07700 900890") service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) template = create_sample_template(notify_db, notify_db_session, service=service) @@ -475,11 +465,10 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif notification_id = uuid.uuid4() encrypt_notification = encryption.encrypt(notification) - send_sms( + save_sms( service.id, notification_id, encrypt_notification, - datetime.utcnow().strftime(DATETIME_FORMAT) ) persisted_notification = Notification.query.one() @@ -499,7 +488,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif ) -def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key(notify_db, +def test_should_save_sms_if_restricted_service_and_non_team_number_with_test_key(notify_db, notify_db_session, mocker): user = create_user(mobile_number="07700 900205") @@ -510,11 +499,10 @@ def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() - send_sms( + save_sms( service.id, notification_id, encryption.encrypt(notification), - datetime.utcnow().strftime(DATETIME_FORMAT), key_type=KEY_TYPE_TEST ) @@ -525,7 +513,7 @@ def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key ) -def test_should_send_email_if_restricted_service_and_non_team_email_address_with_test_key(notify_db, +def test_should_save_email_if_restricted_service_and_non_team_email_address_with_test_key(notify_db, notify_db_session, mocker): user = create_user() @@ -538,11 +526,10 @@ def test_should_send_email_if_restricted_service_and_non_team_email_address_with mocked_deliver_email = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') notification_id = uuid.uuid4() - send_email( + save_email( service.id, notification_id, encryption.encrypt(notification), - datetime.utcnow().strftime(DATETIME_FORMAT), key_type=KEY_TYPE_TEST ) @@ -553,7 +540,7 @@ def test_should_send_email_if_restricted_service_and_non_team_email_address_with ) -def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): +def test_should_not_save_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): user = create_user(mobile_number="07700 900205") service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) template = create_sample_template(notify_db, notify_db_session, service=service) @@ -562,17 +549,16 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() - send_sms( + save_sms( service.id, notification_id, encryption.encrypt(notification), - datetime.utcnow().strftime(DATETIME_FORMAT) ) assert provider_tasks.deliver_sms.apply_async.called is False assert Notification.query.count() == 0 -def test_should_not_send_email_if_restricted_service_and_invalid_email_address(notify_db, notify_db_session, mocker): +def test_should_not_save_email_if_restricted_service_and_invalid_email_address(notify_db, notify_db_session, mocker): user = create_user() service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) template = create_sample_template( @@ -581,17 +567,16 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address(n notification = _notification_json(template, to="test@example.com") notification_id = uuid.uuid4() - send_email( + save_email( service.id, notification_id, encryption.encrypt(notification), - datetime.utcnow().strftime(DATETIME_FORMAT) ) assert Notification.query.count() == 0 -def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_service( +def test_should_put_save_email_task_in_research_mode_queue_if_research_mode_service( notify_db, notify_db_session, mocker ): service = create_sample_service(notify_db, notify_db_session) @@ -606,11 +591,10 @@ def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_serv notification_id = uuid.uuid4() - send_email( + save_email( template.service_id, notification_id, encryption.encrypt(notification), - datetime.utcnow().strftime(DATETIME_FORMAT) ) persisted_notification = Notification.query.one() @@ -631,7 +615,7 @@ def test_should_not_build_dvla_file_in_research_mode_for_letter_job( """ mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=csv) mocker.patch('app.celery.tasks.update_job_to_sent_to_dvla.apply_async') - mocker.patch('app.celery.tasks.persist_letter.apply_async') + mocker.patch('app.celery.tasks.save_letter.apply_async') mocker.patch('app.celery.tasks.create_uuid', return_value=fake_uuid) mocker.patch('app.celery.tasks.encryption.encrypt', return_value=test_encrypted_data) mock_dvla_file_task = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') @@ -641,7 +625,6 @@ def test_should_not_build_dvla_file_in_research_mode_for_letter_job( assert not mock_dvla_file_task.called -@freeze_time("2017-08-29 17:30:00") def test_should_update_job_to_sent_to_dvla_in_research_mode_for_letter_job( mocker, sample_letter_job, fake_uuid ): @@ -653,7 +636,7 @@ def test_should_update_job_to_sent_to_dvla_in_research_mode_for_letter_job( """ mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=csv) mock_update_job_task = mocker.patch('app.celery.tasks.update_job_to_sent_to_dvla.apply_async') - mocker.patch('app.celery.tasks.persist_letter.apply_async') + mocker.patch('app.celery.tasks.save_letter.apply_async') mocker.patch('app.celery.tasks.create_uuid', return_value=fake_uuid) mocker.patch('app.celery.tasks.encryption.encrypt', return_value=test_encrypted_data) mock_dvla_file_task = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') @@ -662,12 +645,11 @@ def test_should_update_job_to_sent_to_dvla_in_research_mode_for_letter_job( job = jobs_dao.dao_get_job_by_id(sample_letter_job.id) - persist_letter.apply_async.assert_called_once_with( + save_letter.apply_async.assert_called_once_with( ( str(sample_letter_job.service_id), fake_uuid, test_encrypted_data, - datetime.utcnow().strftime(DATETIME_FORMAT) ), queue=QueueNames.RESEARCH_MODE ) @@ -676,7 +658,7 @@ def test_should_update_job_to_sent_to_dvla_in_research_mode_for_letter_job( [str(job.id)], queue=QueueNames.RESEARCH_MODE) -def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_api_key, mocker): +def test_should_save_sms_template_to_and_persist_with_job_id(sample_job, sample_api_key, mocker): notification = _notification_json( sample_job.template, to="+447234123123", @@ -686,11 +668,10 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ notification_id = uuid.uuid4() now = datetime.utcnow() - send_sms( + save_sms( sample_job.service.id, notification_id, encryption.encrypt(notification), - now.strftime(DATETIME_FORMAT), api_key_id=str(sample_api_key.id), key_type=KEY_TYPE_NORMAL ) @@ -713,7 +694,7 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ ) -def test_should_not_send_email_if_team_key_and_recipient_not_in_team(sample_email_template_with_placeholders, +def test_should_not_save_email_if_team_key_and_recipient_not_in_team(sample_email_template_with_placeholders, sample_team_api_key, mocker): notification = _notification_json( @@ -727,24 +708,20 @@ def test_should_not_send_email_if_team_key_and_recipient_not_in_team(sample_emai team_members = [user.email_address for user in sample_email_template_with_placeholders.service.users] assert "my_email@my_email.com" not in team_members - with freeze_time("2016-01-01 11:09:00.00000"): - now = datetime.utcnow() + save_email( + sample_email_template_with_placeholders.service_id, + notification_id, + encryption.encrypt(notification), + api_key_id=str(sample_team_api_key.id), + key_type=KEY_TYPE_TEAM + ) - send_email( - sample_email_template_with_placeholders.service_id, - notification_id, - encryption.encrypt(notification), - now.strftime(DATETIME_FORMAT), - api_key_id=str(sample_team_api_key.id), - key_type=KEY_TYPE_TEAM - ) - - assert Notification.query.count() == 0 + assert Notification.query.count() == 0 apply_async.not_called() -def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, notify_db_session, mocker): +def test_should_not_save_sms_if_team_key_and_recipient_not_in_team(notify_db, notify_db_session, mocker): assert Notification.query.count() == 0 user = create_user(mobile_number="07700 900205") service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) @@ -757,11 +734,10 @@ def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, no mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() - send_sms( + save_sms( service.id, notification_id, encryption.encrypt(notification), - datetime.utcnow().strftime(DATETIME_FORMAT) ) assert provider_tasks.deliver_sms.apply_async.called is False assert Notification.query.count() == 0 @@ -781,11 +757,10 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh row_number=1) with freeze_time("2016-01-01 11:10:00.00000"): - send_email( + save_email( sample_email_template_with_placeholders.service_id, notification_id, encryption.encrypt(notification), - now.strftime(DATETIME_FORMAT), api_key_id=str(sample_api_key.id), key_type=sample_api_key.key_type ) @@ -809,7 +784,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh [str(persisted_notification.id)], queue='send-email-tasks') -def test_send_email_should_use_template_version_from_job_not_latest(sample_email_template, mocker): +def test_save_email_should_use_template_version_from_job_not_latest(sample_email_template, mocker): notification = _notification_json(sample_email_template, 'my_email@my_email.com') version_on_notification = sample_email_template.version # Change the template @@ -820,11 +795,10 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email t = dao_get_template_by_id(sample_email_template.id) assert t.version > version_on_notification now = datetime.utcnow() - send_email( + save_email( sample_email_template.service_id, uuid.uuid4(), encryption.encrypt(notification), - now.strftime(DATETIME_FORMAT) ) persisted_notification = Notification.query.one() @@ -847,11 +821,10 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi notification_id = uuid.uuid4() now = datetime.utcnow() - send_email( + save_email( sample_email_template_with_placeholders.service_id, notification_id, encryption.encrypt(notification), - now.strftime(DATETIME_FORMAT) ) persisted_notification = Notification.query.one() assert persisted_notification.to == 'my_email@my_email.com' @@ -874,11 +847,10 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em notification_id = uuid.uuid4() now = datetime.utcnow() - send_email( + save_email( sample_email_template.service_id, notification_id, encryption.encrypt(notification), - now.strftime(DATETIME_FORMAT) ) persisted_notification = Notification.query.one() assert persisted_notification.to == 'my_email@my_email.com' @@ -894,95 +866,87 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em queue='send-email-tasks') -def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, mocker): +def test_save_sms_should_go_to_retry_queue_if_database_errors(sample_template, mocker): notification = _notification_json(sample_template, "+447234123123") expected_exception = SQLAlchemyError() mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - mocker.patch('app.celery.tasks.send_sms.retry', side_effect=Retry) + mocker.patch('app.celery.tasks.save_sms.retry', side_effect=Retry) mocker.patch('app.notifications.process_notifications.dao_create_notification', side_effect=expected_exception) - now = datetime.utcnow() notification_id = uuid.uuid4() with pytest.raises(Retry): - send_sms( + save_sms( sample_template.service_id, notification_id, encryption.encrypt(notification), - now.strftime(DATETIME_FORMAT) ) assert provider_tasks.deliver_sms.apply_async.called is False - tasks.send_sms.retry.assert_called_with(exc=expected_exception, queue="retry-tasks") + tasks.save_sms.retry.assert_called_with(exc=expected_exception, queue="retry-tasks") assert Notification.query.count() == 0 -def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_template, mocker): +def test_save_email_should_go_to_retry_queue_if_database_errors(sample_email_template, mocker): notification = _notification_json(sample_email_template, "test@example.gov.uk") expected_exception = SQLAlchemyError() mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - mocker.patch('app.celery.tasks.send_email.retry', side_effect=Retry) + mocker.patch('app.celery.tasks.save_email.retry', side_effect=Retry) mocker.patch('app.notifications.process_notifications.dao_create_notification', side_effect=expected_exception) - now = datetime.utcnow() notification_id = uuid.uuid4() with pytest.raises(Retry): - send_email( + save_email( sample_email_template.service_id, notification_id, encryption.encrypt(notification), - now.strftime(DATETIME_FORMAT) ) assert not provider_tasks.deliver_email.apply_async.called - tasks.send_email.retry.assert_called_with(exc=expected_exception, queue="retry-tasks") + tasks.save_email.retry.assert_called_with(exc=expected_exception, queue="retry-tasks") assert Notification.query.count() == 0 -def test_send_email_does_not_send_duplicate_and_does_not_put_in_retry_queue(sample_notification, mocker): +def test_save_email_does_not_send_duplicate_and_does_not_put_in_retry_queue(sample_notification, mocker): json = _notification_json(sample_notification.template, sample_notification.to, job_id=uuid.uuid4(), row_number=1) deliver_email = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - retry = mocker.patch('app.celery.tasks.send_email.retry', side_effect=Exception()) - now = datetime.utcnow() + retry = mocker.patch('app.celery.tasks.save_email.retry', side_effect=Exception()) notification_id = sample_notification.id - send_email( + save_email( sample_notification.service_id, notification_id, encryption.encrypt(json), - now.strftime(DATETIME_FORMAT) ) assert Notification.query.count() == 1 assert not deliver_email.called assert not retry.called -def test_send_sms_does_not_send_duplicate_and_does_not_put_in_retry_queue(sample_notification, mocker): +def test_save_sms_does_not_send_duplicate_and_does_not_put_in_retry_queue(sample_notification, mocker): json = _notification_json(sample_notification.template, sample_notification.to, job_id=uuid.uuid4(), row_number=1) deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - retry = mocker.patch('app.celery.tasks.send_sms.retry', side_effect=Exception()) - now = datetime.utcnow() + retry = mocker.patch('app.celery.tasks.save_sms.retry', side_effect=Exception()) notification_id = sample_notification.id - send_sms( + save_sms( sample_notification.service_id, notification_id, encryption.encrypt(json), - now.strftime(DATETIME_FORMAT) ) assert Notification.query.count() == 1 assert not deliver_sms.called assert not retry.called -def test_persist_letter_saves_letter_to_database(sample_letter_job, mocker): +def test_save_letter_saves_letter_to_database(sample_letter_job, mocker): mocker.patch('app.celery.tasks.create_random_identifier', return_value="this-is-random-in-real-life") @@ -1005,11 +969,10 @@ def test_persist_letter_saves_letter_to_database(sample_letter_job, mocker): notification_id = uuid.uuid4() created_at = datetime.utcnow() - persist_letter( + save_letter( sample_letter_job.service_id, notification_id, encryption.encrypt(notification_json), - created_at ) notification_db = Notification.query.one() @@ -1229,7 +1192,7 @@ def test_check_job_status_task_does_not_raise_error(sample_template): def test_process_incomplete_job_sms(mocker, sample_template): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) - send_sms = mocker.patch('app.celery.tasks.send_sms.apply_async') + save_sms = mocker.patch('app.celery.tasks.save_sms.apply_async') job = create_job(template=sample_template, notification_count=10, created_at=datetime.utcnow() - timedelta(hours=2), @@ -1248,13 +1211,13 @@ def test_process_incomplete_job_sms(mocker, sample_template): assert completed_job.job_status == JOB_STATUS_FINISHED - assert send_sms.call_count == 8 # There are 10 in the file and we've added two already + assert save_sms.call_count == 8 # There are 10 in the file and we've added two already def test_process_incomplete_job_with_notifications_all_sent(mocker, sample_template): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) - mock_send_sms = mocker.patch('app.celery.tasks.send_sms.apply_async') + mock_save_sms = mocker.patch('app.celery.tasks.save_sms.apply_async') job = create_job(template=sample_template, notification_count=10, created_at=datetime.utcnow() - timedelta(hours=2), @@ -1281,13 +1244,13 @@ def test_process_incomplete_job_with_notifications_all_sent(mocker, sample_templ assert completed_job.job_status == JOB_STATUS_FINISHED - assert mock_send_sms.call_count == 0 # There are 10 in the file and we've added 10 it should not have been called + assert mock_save_sms.call_count == 0 # There are 10 in the file and we've added 10 it should not have been called def test_process_incomplete_jobs_sms(mocker, sample_template): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) - mock_send_sms = mocker.patch('app.celery.tasks.send_sms.apply_async') + mock_save_sms = mocker.patch('app.celery.tasks.save_sms.apply_async') job = create_job(template=sample_template, notification_count=10, created_at=datetime.utcnow() - timedelta(hours=2), @@ -1324,12 +1287,12 @@ def test_process_incomplete_jobs_sms(mocker, sample_template): assert completed_job2.job_status == JOB_STATUS_FINISHED - assert mock_send_sms.call_count == 12 # There are 20 in total over 2 jobs we've added 8 already + assert mock_save_sms.call_count == 12 # There are 20 in total over 2 jobs we've added 8 already def test_process_incomplete_jobs_no_notifications_added(mocker, sample_template): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) - mock_send_sms = mocker.patch('app.celery.tasks.send_sms.apply_async') + mock_save_sms = mocker.patch('app.celery.tasks.save_sms.apply_async') job = create_job(template=sample_template, notification_count=10, created_at=datetime.utcnow() - timedelta(hours=2), @@ -1345,35 +1308,35 @@ def test_process_incomplete_jobs_no_notifications_added(mocker, sample_template) assert completed_job.job_status == JOB_STATUS_FINISHED - assert mock_send_sms.call_count == 10 # There are 10 in the csv file + assert mock_save_sms.call_count == 10 # There are 10 in the csv file def test_process_incomplete_jobs(mocker): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) - mock_send_sms = mocker.patch('app.celery.tasks.send_sms.apply_async') + mock_save_sms = mocker.patch('app.celery.tasks.save_sms.apply_async') jobs = [] process_incomplete_jobs(jobs) - assert mock_send_sms.call_count == 0 # There are no jobs to process so it will not have been called + assert mock_save_sms.call_count == 0 # There are no jobs to process so it will not have been called def test_process_incomplete_job_no_job_in_database(mocker, fake_uuid): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) - mock_send_sms = mocker.patch('app.celery.tasks.send_sms.apply_async') + mock_save_sms = mocker.patch('app.celery.tasks.save_sms.apply_async') with pytest.raises(expected_exception=Exception) as e: process_incomplete_job(fake_uuid) - assert mock_send_sms.call_count == 0 # There is no job in the db it will not have been called + assert mock_save_sms.call_count == 0 # There is no job in the db it will not have been called def test_process_incomplete_job_email(mocker, sample_email_template): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_email')) - mock_email_sender = mocker.patch('app.celery.tasks.send_email.apply_async') + mock_email_saver = mocker.patch('app.celery.tasks.save_email.apply_async') job = create_job(template=sample_email_template, notification_count=10, created_at=datetime.utcnow() - timedelta(hours=2), @@ -1392,13 +1355,13 @@ def test_process_incomplete_job_email(mocker, sample_email_template): assert completed_job.job_status == JOB_STATUS_FINISHED - assert mock_email_sender.call_count == 8 # There are 10 in the file and we've added two already + assert mock_email_saver.call_count == 8 # There are 10 in the file and we've added two already def test_process_incomplete_job_letter(mocker, sample_letter_template): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_letter')) - mock_letter_sender = mocker.patch('app.celery.tasks.persist_letter.apply_async') + mock_letter_saver = mocker.patch('app.celery.tasks.save_letter.apply_async') job = create_job(template=sample_letter_template, notification_count=10, created_at=datetime.utcnow() - timedelta(hours=2), @@ -1417,4 +1380,4 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template): assert completed_job.job_status == JOB_STATUS_FINISHED - assert mock_letter_sender.call_count == 8 + assert mock_letter_saver.call_count == 8 From 6dc41c3b4774d0b52b33fefb2f8d284c74671c6a Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 19 Oct 2017 09:58:23 +0100 Subject: [PATCH 06/18] New endpoints to add and update multiple SMS sender for a service. --- app/models.py | 11 ++ app/service/rest.py | 40 ++++- app/service/service_senders_schema.py | 16 ++ tests/app/db.py | 7 +- tests/app/service/test_rest.py | 207 +++++++++++++++++++++----- 5 files changed, 243 insertions(+), 38 deletions(-) diff --git a/app/models.py b/app/models.py index 834dda1de..74bcd7cfc 100644 --- a/app/models.py +++ b/app/models.py @@ -313,6 +313,17 @@ class ServiceSmsSender(db.Model): created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + def serialize(self): + return { + "id": str(self.id), + "sms_sender": self.sms_sender, + "service_id": self.service_id, + "is_default": self.is_default, + "inbound_number_id": self.inbound_number_id, + "created_at": self.created_at.strftime(DATETIME_FORMAT), + "updated_at": self.updated_at.strftime(DATETIME_FORMAT) if self.updated_at else None, + } + class ServicePermission(db.Model): __tablename__ = "service_permissions" diff --git a/app/service/rest.py b/app/service/rest.py index 66cd90317..7abc645d9 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -17,12 +17,14 @@ from app.dao.api_key_dao import ( get_model_api_keys, get_unsigned_secret, expire_api_key) +from app.dao.inbound_numbers_dao import dao_allocate_number_for_service from app.dao.service_inbound_api_dao import ( save_service_inbound_api, reset_service_inbound_api, get_service_inbound_api ) -from app.dao.service_sms_sender_dao import insert_or_update_service_sms_sender +from app.dao.service_sms_sender_dao import insert_or_update_service_sms_sender, dao_add_sms_sender_for_service, \ + dao_update_service_sms_sender, dao_get_service_sms_senders_by_id from app.dao.services_dao import ( dao_fetch_service_by_id, dao_fetch_all_services, @@ -73,7 +75,7 @@ from app.service.service_inbound_api_schema import service_inbound_api, update_s from app.service.service_senders_schema import ( add_service_email_reply_to_request, add_service_letter_contact_block_request, -) + add_service_sms_sender_request) from app.service.utils import get_whitelist_objects from app.service.sender import send_notification_to_service_users from app.service.send_notification import send_one_off_notification @@ -619,6 +621,40 @@ def update_service_letter_contact(service_id, letter_contact_id): return jsonify(data=new_reply_to.serialize()), 200 +@service_blueprint.route('//sms-sender', methods=['POST']) +def add_service_sms_sender(service_id): + dao_fetch_service_by_id(service_id) + form = validate(request.get_json(), add_service_sms_sender_request) + inbound_number_id = form.get('inbound_number_id', None) + if inbound_number_id: + dao_allocate_number_for_service(service_id=service_id, inbound_number_id=inbound_number_id) + new_sms_sender = dao_add_sms_sender_for_service(service_id=service_id, + sms_sender=form['sms_sender'], + is_default=form['is_default'], + inbound_number_id=inbound_number_id + ) + return jsonify(data=new_sms_sender.serialize()), 201 + + +@service_blueprint.route('//sms-sender/', methods=['POST']) +def update_service_sms_sender(service_id, sms_sender_id): + dao_fetch_service_by_id(service_id) + form = validate(request.get_json(), add_service_sms_sender_request) + + sms_sender_to_update = dao_get_service_sms_senders_by_id(service_id=service_id, + service_sms_sender_id=sms_sender_id) + if sms_sender_to_update.inbound_number_id and form['sms_sender'] != sms_sender_to_update.sms_sender: + raise InvalidRequest("You can not change the inbound number for service {}".format(service_id), + status_code=400) + + new_sms_sender = dao_update_service_sms_sender(service_id=service_id, + service_sms_sender_id=sms_sender_id, + is_default=form['is_default'], + sms_sender=form['sms_sender'] + ) + return jsonify(data=new_sms_sender.serialize()), 200 + + @service_blueprint.route('/unique', methods=["GET"]) def is_service_name_unique(): name, email_from = check_request_args(request) diff --git a/app/service/service_senders_schema.py b/app/service/service_senders_schema.py index e0a07b0a4..ef30b0bd3 100644 --- a/app/service/service_senders_schema.py +++ b/app/service/service_senders_schema.py @@ -1,3 +1,5 @@ +from app.schema_validation.definitions import uuid + add_service_email_reply_to_request = { "$schema": "http://json-schema.org/draft-04/schema#", "description": "POST service email reply to address", @@ -22,3 +24,17 @@ add_service_letter_contact_block_request = { }, "required": ["contact_block", "is_default"] } + + +add_service_sms_sender_request = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "POST add service SMS sender", + "type": "object", + "title": "Add new SMS sender for service", + "properties": { + "sms_sender": {"type": "string"}, + "is_default": {"type": "boolean"}, + "inbound_number_id": uuid + }, + "required": ["sms_sender", "is_default"] +} \ No newline at end of file diff --git a/tests/app/db.py b/tests/app/db.py index 518f0cbdd..0cdf7cfee 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -63,12 +63,13 @@ def create_service( research_mode=False, active=True, do_create_inbound_number=True, + email_from=None ): service = Service( name=service_name, message_limit=1000, restricted=restricted, - email_from=service_name.lower().replace(' ', '.'), + email_from=email_from if email_from else service_name.lower().replace(' ', '.'), created_by=user or create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())), sms_sender=sms_sender, ) @@ -355,12 +356,14 @@ def create_reply_to_email( def create_service_sms_sender( service, sms_sender, - is_default=True + is_default=True, + inbound_number_id=None ): data = { 'service_id': service.id, 'sms_sender': sms_sender, 'is_default': is_default, + 'inbound_number_id': inbound_number_id } service_sms_sender = ServiceSmsSender(**data) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 21046702d..edf38e7ff 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -12,25 +12,29 @@ from app.dao.services_dao import dao_remove_user_from_service from app.dao.templates_dao import dao_redact_template from app.dao.users_dao import save_model_user from app.models import ( - User, Organisation, Service, ServicePermission, Notification, ServiceEmailReplyTo, ServiceLetterContact, + User, Organisation, Service, ServicePermission, Notification, + ServiceEmailReplyTo, ServiceLetterContact, + ServiceSmsSender, InboundNumber, DVLA_ORG_LAND_REGISTRY, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, - EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE, - ServiceSmsSender) + EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE +) from tests import create_authorization_header from tests.app.conftest import ( - sample_service as create_service, sample_user_service_permission as create_user_service_permission, sample_notification as create_sample_notification, sample_notification_history as create_notification_history, sample_notification_with_job ) from tests.app.db import ( + create_service, create_template, create_service_inbound_api, create_notification, create_reply_to_email, create_letter_contact, + create_inbound_number, + create_service_sms_sender ) from tests.app.db import create_user @@ -563,7 +567,7 @@ def test_update_service_flags(client, sample_service): @pytest.fixture(scope='function') def service_with_no_permissions(notify_db, notify_db_session): - return create_service(notify_db, notify_db_session, permissions=[]) + return create_service(service_permissions=[]) def test_update_service_flags_with_service_without_default_service_permissions(client, service_with_no_permissions): @@ -586,8 +590,7 @@ def test_update_service_flags_with_service_without_default_service_permissions(c def test_update_service_flags_will_remove_service_permissions(client, notify_db, notify_db_session): auth_header = create_authorization_header() - service = create_service( - notify_db, notify_db_session, permissions=[SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE]) + service = create_service(service_permissions=[SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE]) assert INTERNATIONAL_SMS_TYPE in [p.permission for p in service.permissions] @@ -781,8 +784,6 @@ def test_should_not_update_service_with_duplicate_name(notify_api, with notify_api.test_client() as client: service_name = "another name" service = create_service( - notify_db, - notify_db_session, service_name=service_name, user=sample_user, email_from='another.name') @@ -814,8 +815,6 @@ def test_should_not_update_service_with_duplicate_email_from(notify_api, email_from = "duplicate.name" service_name = "duplicate name" service = create_service( - notify_db, - notify_db_session, service_name=service_name, user=sample_user, email_from=email_from) @@ -1338,8 +1337,8 @@ def test_set_reply_to_email_for_service(notify_api, sample_service): def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notify_db_session): with notify_api.test_request_context(), notify_api.test_client() as client: - service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1') - service_2 = create_service(notify_db, notify_db_session, service_name="2", email_from='2') + service_1 = create_service(service_name="1", email_from='1') + service_2 = create_service(service_name="2", email_from='2') create_sample_notification(notify_db, notify_db_session, service=service_2) @@ -1362,7 +1361,7 @@ def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notif def test_get_notification_for_service_without_uuid(client, notify_db, notify_db_session): - service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1') + service_1 = create_service(service_name="1", email_from='1') response = client.get( path='/service/{}/notifications/{}'.format(service_1.id, 'foo'), headers=[create_authorization_header()] @@ -1372,8 +1371,8 @@ def test_get_notification_for_service_without_uuid(client, notify_db, notify_db_ def test_get_notification_for_service(client, notify_db, notify_db_session): - service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1') - service_2 = create_service(notify_db, notify_db_session, service_name="2", email_from='2') + service_1 = create_service(service_name="1", email_from='1') + service_2 = create_service(service_name="2", email_from='2') service_1_notifications = [ create_sample_notification(notify_db, notify_db_session, service=service_1), @@ -1669,8 +1668,8 @@ def test_get_services_with_detailed_flag_defaults_to_today(client, mocker): def test_get_detailed_services_groups_by_service(notify_db, notify_db_session): from app.service.rest import get_detailed_services - service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1') - service_2 = create_service(notify_db, notify_db_session, service_name="2", email_from='2') + service_1 = create_service(service_name="1", email_from='1') + service_2 = create_service(service_name="2", email_from='2') create_sample_notification(notify_db, notify_db_session, service=service_1, status='created') create_sample_notification(notify_db, notify_db_session, service=service_2, status='created') @@ -1698,8 +1697,8 @@ def test_get_detailed_services_groups_by_service(notify_db, notify_db_session): def test_get_detailed_services_includes_services_with_no_notifications(notify_db, notify_db_session): from app.service.rest import get_detailed_services - service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1') - service_2 = create_service(notify_db, notify_db_session, service_name="2", email_from='2') + service_1 = create_service(service_name="1", email_from='1') + service_2 = create_service(service_name="2", email_from='2') create_sample_notification(notify_db, notify_db_session, service=service_1) @@ -1892,11 +1891,7 @@ def test_search_for_notification_by_to_field_return_multiple_matches(client, not def test_update_service_calls_send_notification_as_service_becomes_live(notify_db, notify_db_session, client, mocker): send_notification_mock = mocker.patch('app.service.rest.send_notification_to_service_users') - restricted_service = create_service( - notify_db, - notify_db_session, - restricted=True - ) + restricted_service = create_service(restricted=True) data = { "restricted": False @@ -2213,8 +2208,7 @@ def test_is_service_name_unique_returns_200_if_unique(client): ("something unique", "something.unique") ]) def test_is_service_name_unique_returns_200_and_false(client, notify_db, notify_db_session, name, email_from): - create_service(notify_db=notify_db, notify_db_session=notify_db_session, - service_name='something unique', email_from='something.unique') + create_service(service_name='something unique', email_from='something.unique') response = client.get('/service/unique?name={}&email_from={}'.format(name, email_from), headers=[create_authorization_header()]) assert response.status_code == 200 @@ -2255,7 +2249,7 @@ def test_get_email_reply_to_addresses_when_there_are_no_reply_to_email_addresses def test_get_email_reply_to_addresses_with_one_email_address(client, notify_db, notify_db_session): - service = create_service(notify_db=notify_db, notify_db_session=notify_db_session) + service = create_service() reply_to = create_reply_to_email(service, 'test@mail.com') response = client.get('/service/{}/email-reply-to'.format(service.id), @@ -2271,7 +2265,7 @@ def test_get_email_reply_to_addresses_with_one_email_address(client, notify_db, def test_get_email_reply_to_addresses_with_multiple_email_addresses(client, notify_db, notify_db_session): - service = create_service(notify_db=notify_db, notify_db_session=notify_db_session) + service = create_service() reply_to_a = create_reply_to_email(service, 'test_a@mail.com') reply_to_b = create_reply_to_email(service, 'test_b@mail.com', False) @@ -2389,7 +2383,7 @@ def test_update_service_reply_to_email_address_404s_when_invalid_service_id(clie def test_get_email_reply_to_address(client, notify_db, notify_db_session): - service = create_service(notify_db=notify_db, notify_db_session=notify_db_session) + service = create_service() reply_to = create_reply_to_email(service, 'test_a@mail.com') response = client.get('/service/{}/email-reply-to/{}'.format(service.id, reply_to.id), @@ -2426,7 +2420,7 @@ def test_get_letter_contacts_when_there_are_no_letter_contacts(client, sample_se def test_get_letter_contacts_with_one_letter_contact(client, notify_db, notify_db_session): - service = create_service(notify_db=notify_db, notify_db_session=notify_db_session) + service = create_service() letter_contact = create_letter_contact(service, 'Aberdeen, AB23 1XH') response = client.get('/service/{}/letter-contact'.format(service.id), @@ -2442,7 +2436,7 @@ def test_get_letter_contacts_with_one_letter_contact(client, notify_db, notify_d def test_get_letter_contacts_with_multiple_letter_contacts(client, notify_db, notify_db_session): - service = create_service(notify_db=notify_db, notify_db_session=notify_db_session) + service = create_service() letter_contact_a = create_letter_contact(service, 'Aberdeen, AB23 1XH') letter_contact_b = create_letter_contact(service, 'London, E1 8QS', False) @@ -2469,7 +2463,7 @@ def test_get_letter_contacts_with_multiple_letter_contacts(client, notify_db, no def test_get_letter_contact_by_id(client, notify_db, notify_db_session): - service = create_service(notify_db=notify_db, notify_db_session=notify_db_session) + service = create_service() letter_contact = create_letter_contact(service, 'London, E1 8QS') response = client.get('/service/{}/letter-contact/{}'.format(service.id, letter_contact.id), @@ -2480,7 +2474,7 @@ def test_get_letter_contact_by_id(client, notify_db, notify_db_session): def test_get_letter_contact_return_404_when_invalid_contact_id(client, notify_db, notify_db_session): - service = create_service(notify_db=notify_db, notify_db_session=notify_db_session) + service = create_service() response = client.get('/service/{}/letter-contact/{}'.format(service.id, '93d59f88-4aa1-453c-9900-f61e2fc8a2de'), headers=[('Content-Type', 'application/json'), create_authorization_header()]) @@ -2577,3 +2571,148 @@ def test_update_service_letter_contact_returns_404_when_invalid_service_id(clien result = json.loads(response.get_data(as_text=True)) assert result['result'] == 'error' assert result['message'] == 'No result found' + + +def test_add_service_sms_sender_can_add_multiple_senders(client, notify_db_session): + service = create_service() + data = { + "sms_sender": 'second', + "is_default": False, + } + response = client.post('/service/{}/sms-sender'.format(service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True))['data'] + assert resp_json['sms_sender'] == 'second' + assert not resp_json['is_default'] + senders = ServiceSmsSender.query.all() + assert len(senders) == 2 + + +def test_add_service_sms_sender_when_it_is_an_inbound_number(client, notify_db_session): + service = create_service() + inbound_number = create_inbound_number(number='12345') + data = { + "sms_sender": inbound_number.number, + "is_default": False, + "inbound_number_id": str(inbound_number.id) + } + response = client.post('/service/{}/sms-sender'.format(service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 201 + updated_number = InboundNumber.query.get(inbound_number.id) + assert updated_number.service_id == service.id + resp_json = json.loads(response.get_data(as_text=True))['data'] + assert resp_json['sms_sender'] == inbound_number.number + assert resp_json['inbound_number_id'] == str(inbound_number.id) + assert not resp_json['is_default'] + + +def test_add_service_sms_sender_switches_default(client, notify_db_session): + service = create_service(sms_sender='first') + data = { + "sms_sender": 'second', + "is_default": True, + } + response = client.post('/service/{}/sms-sender'.format(service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True))['data'] + assert resp_json['sms_sender'] == 'second' + assert not resp_json['inbound_number_id'] + assert resp_json['is_default'] + sms_senders = ServiceSmsSender.query.filter_by(sms_sender='first').first() + assert not sms_senders.is_default + + +def test_add_service_sms_sender_return_404_when_service_does_not_exist(client): + data = { + "sms_sender": '12345', + "is_default": False + } + response = client.post('/service/{}/sms-sender'.format(uuid.uuid4()), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 404 + result = json.loads(response.get_data(as_text=True)) + assert result['result'] == 'error' + assert result['message'] == 'No result found' + + +def test_update_service_sms_sender(client, notify_db_session): + service = create_service() + service_sms_sender = create_service_sms_sender(service=service, sms_sender='1235', is_default=False) + data = { + "sms_sender": 'second', + "is_default": False, + } + response = client.post('/service/{}/sms-sender/{}'.format(service.id, service_sms_sender.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True))['data'] + assert resp_json['sms_sender'] == 'second' + assert not resp_json['inbound_number_id'] + assert not resp_json['is_default'] + + +def test_update_service_sms_sender_switches_default(client, notify_db_session): + service = create_service(sms_sender='first') + service_sms_sender = create_service_sms_sender(service=service, sms_sender='1235', is_default=False) + data = { + "sms_sender": 'second', + "is_default": True, + } + response = client.post('/service/{}/sms-sender/{}'.format(service.id, service_sms_sender.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True))['data'] + assert resp_json['sms_sender'] == 'second' + assert not resp_json['inbound_number_id'] + assert resp_json['is_default'] + sms_senders = ServiceSmsSender.query.filter_by(sms_sender='first').first() + assert not sms_senders.is_default + + +def test_update_service_sms_sender_does_not_allow_sender_update_for_inbound_number(client, notify_db_session): + service = create_service() + inbound_number = create_inbound_number('12345', service_id=service.id) + service_sms_sender = create_service_sms_sender(service=service, + sms_sender='1235', + is_default=False, + inbound_number_id=inbound_number.id) + data = { + "sms_sender": 'second', + "is_default": True, + "inbound_number_id": str(inbound_number.id) + } + response = client.post('/service/{}/sms-sender/{}'.format(service.id, service_sms_sender.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 400 + + +def test_update_service_sms_sender_return_404_when_service_does_not_exist(client): + data = { + "sms_sender": '12345', + "is_default": False + } + response = client.post('/service/{}/sms-sender/{}'.format(uuid.uuid4(), uuid.uuid4()), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 404 + result = json.loads(response.get_data(as_text=True)) + assert result['result'] == 'error' + assert result['message'] == 'No result found' From 709e24e26711763a8afc40eb6ebe1d91ed31937a Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 19 Oct 2017 10:43:49 +0100 Subject: [PATCH 07/18] Added endpoints to get sms senders for a service --- app/dao/service_sms_sender_dao.py | 4 +- app/models.py | 4 +- app/service/rest.py | 15 ++++++- app/service/service_senders_schema.py | 2 +- tests/app/service/test_rest.py | 56 +++++++++++++++++++++++++++ 5 files changed, 76 insertions(+), 5 deletions(-) diff --git a/app/dao/service_sms_sender_dao.py b/app/dao/service_sms_sender_dao.py index a2c4fa9ea..bf718fee8 100644 --- a/app/dao/service_sms_sender_dao.py +++ b/app/dao/service_sms_sender_dao.py @@ -1,3 +1,5 @@ +from sqlalchemy import desc + from app import db from app.dao.dao_utils import transactional from app.models import ServiceSmsSender @@ -42,7 +44,7 @@ def dao_get_service_sms_senders_by_id(service_id, service_sms_sender_id): def dao_get_sms_senders_by_service_id(service_id): - return ServiceSmsSender.query.filter_by(service_id=service_id).all() + return ServiceSmsSender.query.filter_by(service_id=service_id).order_by(desc(ServiceSmsSender.is_default)).all() @transactional diff --git a/app/models.py b/app/models.py index 74bcd7cfc..36fb8c949 100644 --- a/app/models.py +++ b/app/models.py @@ -317,9 +317,9 @@ class ServiceSmsSender(db.Model): return { "id": str(self.id), "sms_sender": self.sms_sender, - "service_id": self.service_id, + "service_id": str(self.service_id), "is_default": self.is_default, - "inbound_number_id": self.inbound_number_id, + "inbound_number_id": str(self.inbound_number_id) if self.inbound_number_id else None, "created_at": self.created_at.strftime(DATETIME_FORMAT), "updated_at": self.updated_at.strftime(DATETIME_FORMAT) if self.updated_at else None, } diff --git a/app/service/rest.py b/app/service/rest.py index 7abc645d9..d4da620f2 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -24,7 +24,7 @@ from app.dao.service_inbound_api_dao import ( get_service_inbound_api ) from app.dao.service_sms_sender_dao import insert_or_update_service_sms_sender, dao_add_sms_sender_for_service, \ - dao_update_service_sms_sender, dao_get_service_sms_senders_by_id + dao_update_service_sms_sender, dao_get_service_sms_senders_by_id, dao_get_sms_senders_by_service_id from app.dao.services_dao import ( dao_fetch_service_by_id, dao_fetch_all_services, @@ -655,6 +655,19 @@ def update_service_sms_sender(service_id, sms_sender_id): return jsonify(data=new_sms_sender.serialize()), 200 +@service_blueprint.route('//sms-sender/', methods=['GET']) +def get_service_sms_sender_by_id(service_id, sms_sender_id): + sms_sender = dao_get_service_sms_senders_by_id(service_id=service_id, + service_sms_sender_id=sms_sender_id) + return jsonify(data=sms_sender.serialize()), 200 + + +@service_blueprint.route('//sms-sender', methods=['GET']) +def get_service_sms_senders_for_service(service_id): + sms_senders = dao_get_sms_senders_by_service_id(service_id=service_id) + return jsonify(data=[sms_sender.serialize() for sms_sender in sms_senders]), 200 + + @service_blueprint.route('/unique', methods=["GET"]) def is_service_name_unique(): name, email_from = check_request_args(request) diff --git a/app/service/service_senders_schema.py b/app/service/service_senders_schema.py index ef30b0bd3..9d168cb8d 100644 --- a/app/service/service_senders_schema.py +++ b/app/service/service_senders_schema.py @@ -37,4 +37,4 @@ add_service_sms_sender_request = { "inbound_number_id": uuid }, "required": ["sms_sender", "is_default"] -} \ No newline at end of file +} diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index edf38e7ff..9b9e0ef07 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2678,6 +2678,7 @@ def test_update_service_sms_sender_switches_default(client, notify_db_session): assert response.status_code == 200 resp_json = json.loads(response.get_data(as_text=True))['data'] assert resp_json['sms_sender'] == 'second' + print(resp_json) assert not resp_json['inbound_number_id'] assert resp_json['is_default'] sms_senders = ServiceSmsSender.query.filter_by(sms_sender='first').first() @@ -2716,3 +2717,58 @@ def test_update_service_sms_sender_return_404_when_service_does_not_exist(client result = json.loads(response.get_data(as_text=True)) assert result['result'] == 'error' assert result['message'] == 'No result found' + + +def test_get_service_sms_sender_by_id(client, notify_db_session): + service_sms_sender = create_service_sms_sender(service=create_service(), + sms_sender='1235', + is_default=False) + response = client.get('/service/{}/sms-sender/{}'.format(service_sms_sender.service_id, service_sms_sender.id), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True))['data'] == service_sms_sender.serialize() + + +def test_get_service_sms_sender_by_id_returns_404_when_service_does_not_exist(client, notify_db_session): + service_sms_sender = create_service_sms_sender(service=create_service(), + sms_sender='1235', + is_default=False) + response = client.get('/service/{}/sms-sender/{}'.format(uuid.uuid4(), service_sms_sender.id), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 404 + + +def test_get_service_sms_sender_by_id_returns_404_when_sms_sender_does_not_exist(client, notify_db_session): + service_sms_sender = create_service_sms_sender(service=create_service(), + sms_sender='1235', + is_default=False) + response = client.get('/service/{}/sms-sender/{}'.format(service_sms_sender.service_id, uuid.uuid4()), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 404 + + +def test_get_service_sms_senders_for_service(client, notify_db_session): + service_sms_sender = create_service_sms_sender(service=create_service(sms_sender='first'), + sms_sender='second', + is_default=False) + response = client.get('/service/{}/sms-sender'.format(service_sms_sender.service_id), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True))['data'] + assert len(json_resp) == 2 + assert json_resp[0]['is_default'] + assert json_resp[0]['sms_sender'] == 'first' + assert not json_resp[1]['is_default'] + assert json_resp[1]['sms_sender'] == 'second' + + +def test_get_service_sms_senders_for_service_returns_empty_list_when_service_does_not_exist(client): + response = client.get('/service/{}/sms-sender'.format(uuid.uuid4()), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True))['data'] == [] From 46d45d859545cf1c4e1a72d0dc51cdaafe6cf5c1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 19 Oct 2017 11:06:28 +0100 Subject: [PATCH 08/18] Make international SMS on for new services MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit International SMS is a mature, documented feature now. There’s no reason it shouldn’t be available to everyone. If it’s turned off by default then we’re relying on people finding it in the settings page to know that it exists (which we found in research the other week that users, who would have benefitted from having international SMS, were failing to do). This also fixes the problem whereby users signing up for Notify with an international phone number (eg those working abroad for the Foreign and Commonwealth Office) couldn’t get through the tour because they weren’t able to send themselves the example text message (see https://www.pivotaltracker.com/story/show/150705515). --- app/dao/services_dao.py | 15 +++++- tests/app/conftest.py | 4 +- tests/app/dao/test_services_dao.py | 48 ++++++++++++++----- tests/app/notifications/test_validators.py | 9 +++- tests/app/service/test_rest.py | 15 +++++- .../notifications/test_post_notifications.py | 31 ++++++++---- 6 files changed, 91 insertions(+), 31 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 5ae25c0e6..31b9e3300 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -35,11 +35,19 @@ from app.models import ( JobStatistics, SMS_TYPE, EMAIL_TYPE, - ServiceSmsSender) + INTERNATIONAL_SMS_TYPE, + ServiceSmsSender, +) from app.service.statistics import format_monthly_template_notification_stats from app.statsd_decorators import statsd from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc +DEFAULT_SERVICE_PERMISSIONS = [ + SMS_TYPE, + EMAIL_TYPE, + INTERNATIONAL_SMS_TYPE, +] + def dao_fetch_all_services(only_active=False): query = Service.query.order_by( @@ -146,13 +154,16 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): @transactional @version_class(Service) -def dao_create_service(service, user, service_id=None, service_permissions=[SMS_TYPE, EMAIL_TYPE]): +def dao_create_service(service, user, service_id=None, service_permissions=None): # the default property does not appear to work when there is a difference between the sqlalchemy schema and the # db schema (ie: during a migration), so we have to set sms_sender manually here. After the GOVUK sms_sender # migration is completed, this code should be able to be removed. if not service.sms_sender: service.sms_sender = current_app.config['FROM_NUMBER'] + if service_permissions is None: + service_permissions = DEFAULT_SERVICE_PERMISSIONS + from app.dao.permissions_dao import permission_dao service.users.append(user) permission_dao.add_default_service_permissions_for_user(user, service) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index d904c8ecf..7f3bd3008 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -137,7 +137,7 @@ def sample_service( restricted=False, limit=1000, email_from=None, - permissions=[SMS_TYPE, EMAIL_TYPE], + permissions=None, research_mode=None, free_sms_fragment_limit=250000 ): @@ -166,7 +166,7 @@ def sample_service( if user not in service.users: dao_add_user_to_service(service, user) - if INBOUND_SMS_TYPE in permissions: + if permissions and INBOUND_SMS_TYPE in permissions: create_inbound_number('12345', service_id=service.id) return service diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 1d01c4a3e..3396e52ca 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -57,6 +57,7 @@ from app.models import ( KEY_TYPE_TEST, EMAIL_TYPE, SMS_TYPE, + INTERNATIONAL_SMS_TYPE, LETTER_TYPE, SERVICE_PERMISSION_TYPES ) @@ -260,25 +261,38 @@ def test_create_service_returns_service_with_default_permissions(service_factory service = service_factory.get('testing', email_from='testing') service = dao_fetch_service_by_id(service.id) - assert len(service.permissions) == 2 - assert set([SMS_TYPE, EMAIL_TYPE]) == set(p.permission for p in service.permissions) + assert len(service.permissions) == 3 + assert set([ + SMS_TYPE, + EMAIL_TYPE, + INTERNATIONAL_SMS_TYPE, + ]) == set( + p.permission for p in service.permissions + ) -@pytest.mark.parametrize("permission_to_remove, permission_remaining", - [(SMS_TYPE, EMAIL_TYPE), - (EMAIL_TYPE, SMS_TYPE)]) +@pytest.mark.parametrize("permission_to_remove, permission_remaining", [ + (SMS_TYPE, EMAIL_TYPE), + (EMAIL_TYPE, SMS_TYPE), +]) def test_remove_permission_from_service_by_id_returns_service_with_correct_permissions( - sample_service, permission_to_remove, permission_remaining): + sample_service, permission_to_remove, permission_remaining +): dao_remove_service_permission(service_id=sample_service.id, permission=permission_to_remove) service = dao_fetch_service_by_id(sample_service.id) - assert len(service.permissions) == 1 - assert service.permissions[0].permission == permission_remaining + assert len(service.permissions) == 2 + assert set( + p.permission for p in service.permissions + ) == set(( + permission_remaining, INTERNATIONAL_SMS_TYPE + )) def test_removing_all_permission_returns_service_with_no_permissions(sample_service): dao_remove_service_permission(service_id=sample_service.id, permission=SMS_TYPE) dao_remove_service_permission(service_id=sample_service.id, permission=EMAIL_TYPE) + dao_remove_service_permission(service_id=sample_service.id, permission=INTERNATIONAL_SMS_TYPE) service = dao_fetch_service_by_id(sample_service.id) assert len(service.permissions) == 0 @@ -298,14 +312,22 @@ def test_create_service_by_id_adding_and_removing_letter_returns_service_without dao_add_service_permission(service_id=service.id, permission=LETTER_TYPE) service = dao_fetch_service_by_id(service.id) - assert len(service.permissions) == 3 - assert set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE]) == set([p.permission for p in service.permissions]) + assert len(service.permissions) == 4 + assert set([ + SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, + ]) == set([ + p.permission for p in service.permissions + ]) dao_remove_service_permission(service_id=service.id, permission=LETTER_TYPE) service = dao_fetch_service_by_id(service.id) - assert len(service.permissions) == 2 - assert set([SMS_TYPE, EMAIL_TYPE]) == set([p.permission for p in service.permissions]) + assert len(service.permissions) == 3 + assert set([ + SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE, + ]) == set([ + p.permission for p in service.permissions + ]) def test_create_service_creates_a_history_record_with_current_data(sample_user): @@ -431,7 +453,7 @@ def test_delete_service_and_associated_objects(notify_db, sample_permission, sample_provider_statistics): # Default service permissions of Email and SMS - assert ServicePermission.query.count() == 2 + assert ServicePermission.query.count() == 3 delete_service_and_all_associated_db_objects(sample_service) assert NotificationStatistics.query.count() == 0 diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 387da140e..fecfc4da5 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -310,9 +310,14 @@ def test_should_not_rate_limit_if_limiting_is_disabled( @pytest.mark.parametrize('key_type', ['test', 'normal']) -def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_int_sms(sample_service, key_type): +def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_int_sms( + key_type, + notify_db, + notify_db_session, +): + service = create_service(notify_db, notify_db_session, permissions=[SMS_TYPE]) with pytest.raises(BadRequestError) as e: - validate_and_format_recipient('20-12-1234-1234', key_type, sample_service, SMS_TYPE) + validate_and_format_recipient('20-12-1234-1234', key_type, service, SMS_TYPE) assert e.value.status_code == 400 assert e.value.message == 'Cannot send to international mobile numbers' assert e.value.fields == [] diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 21046702d..63381561c 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -203,7 +203,14 @@ def test_get_service_list_has_default_permissions(client, service_factory): assert response.status_code == 200 json_resp = json.loads(response.get_data(as_text=True)) assert len(json_resp['data']) == 3 - assert all([set(json['permissions']) == set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']]) + assert all( + set( + json['permissions'] + ) == set([ + EMAIL_TYPE, SMS_TYPE, INTERNATIONAL_SMS_TYPE, + ]) + for json in json_resp['data'] + ) def test_get_service_by_id_has_default_service_permissions(client, sample_service): @@ -214,7 +221,11 @@ def test_get_service_by_id_has_default_service_permissions(client, sample_servic ) json_resp = json.loads(resp.get_data(as_text=True)) - assert set(json_resp['data']['permissions']) == set([EMAIL_TYPE, SMS_TYPE]) + assert set( + json_resp['data']['permissions'] + ) == set([ + EMAIL_TYPE, SMS_TYPE, INTERNATIONAL_SMS_TYPE, + ]) def test_get_service_by_id_should_404_if_no_service(notify_api, notify_db): diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 390ccac08..94fea27e5 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -328,17 +328,26 @@ def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( assert not deliver_mock.called -def test_post_sms_notification_returns_400_if_not_allowed_to_send_int_sms(client, sample_service, sample_template): +def test_post_sms_notification_returns_400_if_not_allowed_to_send_int_sms( + client, + notify_db, + notify_db_session, +): + + service = sample_service(notify_db, notify_db_session, permissions=[SMS_TYPE]) + template = create_sample_template(notify_db, notify_db_session, service=service) + data = { 'phone_number': '20-12-1234-1234', - 'template_id': sample_template.id + 'template_id': template.id } - auth_header = create_authorization_header(service_id=sample_service.id) + auth_header = create_authorization_header(service_id=service.id) response = client.post( path='/v2/notifications/sms', data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) + headers=[('Content-Type', 'application/json'), auth_header] + ) assert response.status_code == 400 assert response.headers['Content-type'] == 'application/json' @@ -378,18 +387,20 @@ def test_post_sms_notification_returns_400_if_not_allowed_to_send_notification( ] -def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms(notify_db, notify_db_session, client, mocker): - - service = sample_service(notify_db, notify_db_session, permissions=[SMS_TYPE, INTERNATIONAL_SMS_TYPE]) - template = create_sample_template(notify_db, notify_db_session, service=service) +def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms( + sample_service, + sample_template, + client, + mocker, +): mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') data = { 'phone_number': '20-12-1234-1234', - 'template_id': template.id + 'template_id': sample_template.id } - auth_header = create_authorization_header(service_id=service.id) + auth_header = create_authorization_header(service_id=sample_service.id) response = client.post( path='/v2/notifications/sms', From 534a2ea7cd6ddcf1976f72487d2982ba59493498 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 19 Oct 2017 11:14:07 +0100 Subject: [PATCH 09/18] Factor permissions tests for code reuse We had these fiddly little generator expressions repeated throughout this file. Code is made easier to understand by refactoring them into a function that describes what it does. --- tests/app/dao/test_services_dao.py | 47 ++++++++++++++---------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 3396e52ca..a1538eebf 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -261,14 +261,9 @@ def test_create_service_returns_service_with_default_permissions(service_factory service = service_factory.get('testing', email_from='testing') service = dao_fetch_service_by_id(service.id) - assert len(service.permissions) == 3 - assert set([ - SMS_TYPE, - EMAIL_TYPE, - INTERNATIONAL_SMS_TYPE, - ]) == set( - p.permission for p in service.permissions - ) + _assert_service_permissions(service.permissions, ( + SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE, + )) @pytest.mark.parametrize("permission_to_remove, permission_remaining", [ @@ -281,11 +276,8 @@ def test_remove_permission_from_service_by_id_returns_service_with_correct_permi dao_remove_service_permission(service_id=sample_service.id, permission=permission_to_remove) service = dao_fetch_service_by_id(sample_service.id) - assert len(service.permissions) == 2 - assert set( - p.permission for p in service.permissions - ) == set(( - permission_remaining, INTERNATIONAL_SMS_TYPE + _assert_service_permissions(service.permissions, ( + permission_remaining, INTERNATIONAL_SMS_TYPE, )) @@ -312,22 +304,16 @@ def test_create_service_by_id_adding_and_removing_letter_returns_service_without dao_add_service_permission(service_id=service.id, permission=LETTER_TYPE) service = dao_fetch_service_by_id(service.id) - assert len(service.permissions) == 4 - assert set([ + _assert_service_permissions(service.permissions, ( SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, - ]) == set([ - p.permission for p in service.permissions - ]) + )) dao_remove_service_permission(service_id=service.id, permission=LETTER_TYPE) service = dao_fetch_service_by_id(service.id) - assert len(service.permissions) == 3 - assert set([ + _assert_service_permissions(service.permissions, ( SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE, - ]) == set([ - p.permission for p in service.permissions - ]) + )) def test_create_service_creates_a_history_record_with_current_data(sample_user): @@ -404,7 +390,10 @@ def test_update_service_permission_creates_a_history_record_with_current_data(sa service_from_db = Service.query.first() assert service_from_db.version == 2 - assert LETTER_TYPE in [p.permission for p in service_from_db.permissions] + + _assert_service_permissions(service.permissions, ( + SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE, LETTER_TYPE, + )) permission = [p for p in service.permissions if p.permission == 'sms'][0] service.permissions.remove(permission) @@ -415,7 +404,9 @@ def test_update_service_permission_creates_a_history_record_with_current_data(sa service_from_db = Service.query.first() assert service_from_db.version == 3 - assert SMS_TYPE not in [p.permission for p in service_from_db.permissions] + _assert_service_permissions(service.permissions, ( + EMAIL_TYPE, INTERNATIONAL_SMS_TYPE, LETTER_TYPE, + )) assert len(Service.get_history_model().query.filter_by(name='service_name').all()) == 3 assert Service.get_history_model().query.filter_by(name='service_name').all()[2].version == 3 @@ -971,3 +962,9 @@ def test_dao_allocating_inbound_number_shows_on_service(notify_db_session, sampl dao_set_inbound_number_to_service(service.id, inbound_numbers[0]) assert service.inbound_number.number == inbound_numbers[0].number + + +def _assert_service_permissions(service_permissions, expected): + + assert len(service_permissions) == len(expected) + assert set(expected) == set(p.permission for p in service_permissions) From cee916c68a6c466b6ab513a7915ce96cd5cb6ba5 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 19 Oct 2017 13:51:33 +0100 Subject: [PATCH 10/18] As per review commemnts: - Add the assert to a test - oops. - Fix typo in error message. --- app/dao/service_sms_sender_dao.py | 2 +- app/service/rest.py | 15 +++++++++++---- tests/app/dao/test_service_sms_sender_dao.py | 4 ++-- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/app/dao/service_sms_sender_dao.py b/app/dao/service_sms_sender_dao.py index bf718fee8..69397f441 100644 --- a/app/dao/service_sms_sender_dao.py +++ b/app/dao/service_sms_sender_dao.py @@ -108,4 +108,4 @@ def _reset_old_default_to_false(old_default): def _raise_when_no_default(old_default): # check that the update is not updating the only default to false if not old_default: - raise Exception("You must have at least one letter contact as the default.", 400) + raise Exception("You must have at least one SMS sender as the default.", 400) diff --git a/app/service/rest.py b/app/service/rest.py index d4da620f2..70c00b6bf 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -23,8 +23,13 @@ from app.dao.service_inbound_api_dao import ( reset_service_inbound_api, get_service_inbound_api ) -from app.dao.service_sms_sender_dao import insert_or_update_service_sms_sender, dao_add_sms_sender_for_service, \ - dao_update_service_sms_sender, dao_get_service_sms_senders_by_id, dao_get_sms_senders_by_service_id +from app.dao.service_sms_sender_dao import ( + insert_or_update_service_sms_sender, + dao_add_sms_sender_for_service, + dao_update_service_sms_sender, + dao_get_service_sms_senders_by_id, + dao_get_sms_senders_by_service_id +) from app.dao.services_dao import ( dao_fetch_service_by_id, dao_fetch_all_services, @@ -71,7 +76,10 @@ from app.errors import ( from app.models import Service, ServiceInboundApi from app.schema_validation import validate from app.service import statistics -from app.service.service_inbound_api_schema import service_inbound_api, update_service_inbound_api_schema +from app.service.service_inbound_api_schema import ( + service_inbound_api, + update_service_inbound_api_schema +) from app.service.service_senders_schema import ( add_service_email_reply_to_request, add_service_letter_contact_block_request, @@ -638,7 +646,6 @@ def add_service_sms_sender(service_id): @service_blueprint.route('//sms-sender/', methods=['POST']) def update_service_sms_sender(service_id, sms_sender_id): - dao_fetch_service_by_id(service_id) form = validate(request.get_json(), add_service_sms_sender_request) sms_sender_to_update = dao_get_service_sms_senders_by_id(service_id=service_id, diff --git a/tests/app/dao/test_service_sms_sender_dao.py b/tests/app/dao/test_service_sms_sender_dao.py index a3d05e021..3dcec1469 100644 --- a/tests/app/dao/test_service_sms_sender_dao.py +++ b/tests/app/dao/test_service_sms_sender_dao.py @@ -66,9 +66,9 @@ def test_dao_get_sms_senders_by_service_id(notify_db_session): assert len(results) == 2 for x in results: if x.is_default: - x.sms_sender = 'first_sms' + assert x.sms_sender == 'first_sms' else: - x == second_sender + assert x == second_sender def test_dao_add_sms_sender_for_service(notify_db_session): From 2193afe9a3e795efb8cfa009eaef347a98deae2b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 19 Oct 2017 13:59:22 +0100 Subject: [PATCH 11/18] Remove 'data' from the json being returned. --- app/service/rest.py | 8 ++++---- tests/app/service/test_rest.py | 17 ++++++++--------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 70c00b6bf..de0dff3b4 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -641,7 +641,7 @@ def add_service_sms_sender(service_id): is_default=form['is_default'], inbound_number_id=inbound_number_id ) - return jsonify(data=new_sms_sender.serialize()), 201 + return jsonify(new_sms_sender.serialize()), 201 @service_blueprint.route('//sms-sender/', methods=['POST']) @@ -659,20 +659,20 @@ def update_service_sms_sender(service_id, sms_sender_id): is_default=form['is_default'], sms_sender=form['sms_sender'] ) - return jsonify(data=new_sms_sender.serialize()), 200 + return jsonify(new_sms_sender.serialize()), 200 @service_blueprint.route('//sms-sender/', methods=['GET']) def get_service_sms_sender_by_id(service_id, sms_sender_id): sms_sender = dao_get_service_sms_senders_by_id(service_id=service_id, service_sms_sender_id=sms_sender_id) - return jsonify(data=sms_sender.serialize()), 200 + return jsonify(sms_sender.serialize()), 200 @service_blueprint.route('//sms-sender', methods=['GET']) def get_service_sms_senders_for_service(service_id): sms_senders = dao_get_sms_senders_by_service_id(service_id=service_id) - return jsonify(data=[sms_sender.serialize() for sms_sender in sms_senders]), 200 + return jsonify([sms_sender.serialize() for sms_sender in sms_senders]), 200 @service_blueprint.route('/unique', methods=["GET"]) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 9b9e0ef07..406f1a457 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2584,7 +2584,7 @@ def test_add_service_sms_sender_can_add_multiple_senders(client, notify_db_sessi headers=[('Content-Type', 'application/json'), create_authorization_header()] ) assert response.status_code == 201 - resp_json = json.loads(response.get_data(as_text=True))['data'] + resp_json = json.loads(response.get_data(as_text=True)) assert resp_json['sms_sender'] == 'second' assert not resp_json['is_default'] senders = ServiceSmsSender.query.all() @@ -2606,7 +2606,7 @@ def test_add_service_sms_sender_when_it_is_an_inbound_number(client, notify_db_s assert response.status_code == 201 updated_number = InboundNumber.query.get(inbound_number.id) assert updated_number.service_id == service.id - resp_json = json.loads(response.get_data(as_text=True))['data'] + resp_json = json.loads(response.get_data(as_text=True)) assert resp_json['sms_sender'] == inbound_number.number assert resp_json['inbound_number_id'] == str(inbound_number.id) assert not resp_json['is_default'] @@ -2623,7 +2623,7 @@ def test_add_service_sms_sender_switches_default(client, notify_db_session): headers=[('Content-Type', 'application/json'), create_authorization_header()] ) assert response.status_code == 201 - resp_json = json.loads(response.get_data(as_text=True))['data'] + resp_json = json.loads(response.get_data(as_text=True)) assert resp_json['sms_sender'] == 'second' assert not resp_json['inbound_number_id'] assert resp_json['is_default'] @@ -2658,7 +2658,7 @@ def test_update_service_sms_sender(client, notify_db_session): headers=[('Content-Type', 'application/json'), create_authorization_header()] ) assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True))['data'] + resp_json = json.loads(response.get_data(as_text=True)) assert resp_json['sms_sender'] == 'second' assert not resp_json['inbound_number_id'] assert not resp_json['is_default'] @@ -2676,9 +2676,8 @@ def test_update_service_sms_sender_switches_default(client, notify_db_session): headers=[('Content-Type', 'application/json'), create_authorization_header()] ) assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True))['data'] + resp_json = json.loads(response.get_data(as_text=True)) assert resp_json['sms_sender'] == 'second' - print(resp_json) assert not resp_json['inbound_number_id'] assert resp_json['is_default'] sms_senders = ServiceSmsSender.query.filter_by(sms_sender='first').first() @@ -2727,7 +2726,7 @@ def test_get_service_sms_sender_by_id(client, notify_db_session): headers=[('Content-Type', 'application/json'), create_authorization_header()] ) assert response.status_code == 200 - assert json.loads(response.get_data(as_text=True))['data'] == service_sms_sender.serialize() + assert json.loads(response.get_data(as_text=True)) == service_sms_sender.serialize() def test_get_service_sms_sender_by_id_returns_404_when_service_does_not_exist(client, notify_db_session): @@ -2758,7 +2757,7 @@ def test_get_service_sms_senders_for_service(client, notify_db_session): headers=[('Content-Type', 'application/json'), create_authorization_header()] ) assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True))['data'] + json_resp = json.loads(response.get_data(as_text=True)) assert len(json_resp) == 2 assert json_resp[0]['is_default'] assert json_resp[0]['sms_sender'] == 'first' @@ -2771,4 +2770,4 @@ def test_get_service_sms_senders_for_service_returns_empty_list_when_service_doe headers=[('Content-Type', 'application/json'), create_authorization_header()] ) assert response.status_code == 200 - assert json.loads(response.get_data(as_text=True))['data'] == [] + assert json.loads(response.get_data(as_text=True)) == [] From d975f30888f34ae87a8455a06f43e15de37a0093 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 19 Oct 2017 14:39:53 +0100 Subject: [PATCH 12/18] Fix merge conflict --- ...que_constraint.py => 0127_remove_unique_constraint.py} | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) rename migrations/versions/{0126_remove_unique_constraint.py => 0127_remove_unique_constraint.py} (76%) diff --git a/migrations/versions/0126_remove_unique_constraint.py b/migrations/versions/0127_remove_unique_constraint.py similarity index 76% rename from migrations/versions/0126_remove_unique_constraint.py rename to migrations/versions/0127_remove_unique_constraint.py index ca7ec8ca2..2db71b9ea 100644 --- a/migrations/versions/0126_remove_unique_constraint.py +++ b/migrations/versions/0127_remove_unique_constraint.py @@ -1,15 +1,15 @@ """ -Revision ID: 0126_remove_unique_constraint -Revises: 0125_add_organisation_type +Revision ID: 0127_remove_unique_constraint +Revises: 0126_add_annual_billing Create Date: 2017-10-17 16:47:37.826333 """ from alembic import op import sqlalchemy as sa -revision = '0126_remove_unique_constraint' -down_revision = '0125_add_organisation_type' +revision = '0127_remove_unique_constraint' +down_revision = '0126_add_annual_billing' def upgrade(): From ce28e84af6b31671769242af11bf16a4e8dbc97c Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 19 Oct 2017 16:17:25 +0100 Subject: [PATCH 13/18] Remove unused tasks Deleted three Celery tasks (send-email, send-sms and persist-letter). These are not being used anymore - we replaced them in commit 0c9a4bce595f4e186c6e0fc4d29eb1439eec4103. --- app/celery/tasks.py | 126 -------------------------------------------- 1 file changed, 126 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 46a145ad7..bece59f7c 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -180,53 +180,6 @@ def __sending_limits_for_job_exceeded(service, job, job_id): return False -@notify_celery.task(bind=True, name="send-sms", max_retries=5, default_retry_delay=300) -@statsd(namespace="tasks") -def send_sms(self, - service_id, - notification_id, - encrypted_notification, - created_at, - api_key_id=None, - key_type=KEY_TYPE_NORMAL): - notification = encryption.decrypt(encrypted_notification) - service = dao_fetch_service_by_id(service_id) - - if not service_allowed_to_send_to(notification['to'], service, key_type): - current_app.logger.info( - "SMS {} failed as restricted service".format(notification_id) - ) - return - - try: - saved_notification = persist_notification( - template_id=notification['template'], - template_version=notification['template_version'], - recipient=notification['to'], - service=service, - personalisation=notification.get('personalisation'), - notification_type=SMS_TYPE, - api_key_id=api_key_id, - key_type=key_type, - created_at=datetime.utcnow(), - job_id=notification.get('job', None), - job_row_number=notification.get('row_number', None), - notification_id=notification_id - ) - - provider_tasks.deliver_sms.apply_async( - [str(saved_notification.id)], - queue=QueueNames.SEND_SMS if not service.research_mode else QueueNames.RESEARCH_MODE - ) - - current_app.logger.info( - "SMS {} created at {} for job {}".format(saved_notification.id, created_at, notification.get('job', None)) - ) - - except SQLAlchemyError as e: - handle_exception(self, notification, notification_id, e) - - @notify_celery.task(bind=True, name="save-sms", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") def save_sms(self, @@ -276,48 +229,6 @@ def save_sms(self, handle_exception(self, notification, notification_id, e) -@notify_celery.task(bind=True, name="send-email", max_retries=5, default_retry_delay=300) -@statsd(namespace="tasks") -def send_email(self, - service_id, - notification_id, - encrypted_notification, - created_at, - api_key_id=None, - key_type=KEY_TYPE_NORMAL): - notification = encryption.decrypt(encrypted_notification) - service = dao_fetch_service_by_id(service_id) - - if not service_allowed_to_send_to(notification['to'], service, key_type): - current_app.logger.info("Email {} failed as restricted service".format(notification_id)) - return - - try: - saved_notification = persist_notification( - template_id=notification['template'], - template_version=notification['template_version'], - recipient=notification['to'], - service=service, - personalisation=notification.get('personalisation'), - notification_type=EMAIL_TYPE, - api_key_id=api_key_id, - key_type=key_type, - created_at=datetime.utcnow(), - job_id=notification.get('job', None), - job_row_number=notification.get('row_number', None), - notification_id=notification_id - ) - - provider_tasks.deliver_email.apply_async( - [str(saved_notification.id)], - queue=QueueNames.SEND_EMAIL if not service.research_mode else QueueNames.RESEARCH_MODE - ) - - current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at)) - except SQLAlchemyError as e: - handle_exception(self, notification, notification_id, e) - - @notify_celery.task(bind=True, name="save-email", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") def save_email(self, @@ -359,43 +270,6 @@ def save_email(self, handle_exception(self, notification, notification_id, e) -@notify_celery.task(bind=True, name="persist-letter", max_retries=5, default_retry_delay=300) -@statsd(namespace="tasks") -def persist_letter( - self, - service_id, - notification_id, - encrypted_notification, - created_at -): - notification = encryption.decrypt(encrypted_notification) - - # we store the recipient as just the first item of the person's address - recipient = notification['personalisation']['addressline1'] - - service = dao_fetch_service_by_id(service_id) - try: - saved_notification = persist_notification( - template_id=notification['template'], - template_version=notification['template_version'], - recipient=recipient, - service=service, - personalisation=notification['personalisation'], - notification_type=LETTER_TYPE, - api_key_id=None, - key_type=KEY_TYPE_NORMAL, - created_at=datetime.utcnow(), - job_id=notification['job'], - job_row_number=notification['row_number'], - notification_id=notification_id, - reference=create_random_identifier() - ) - - current_app.logger.info("Letter {} created at {}".format(saved_notification.id, created_at)) - except SQLAlchemyError as e: - handle_exception(self, notification, notification_id, e) - - @notify_celery.task(bind=True, name="save-letter", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") def save_letter( From d1c93353075e60d3e5d3bd9b6726829f8d07dbd7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 19 Oct 2017 16:29:54 +0100 Subject: [PATCH 14/18] If the add sms sender is for an inbound number use the number from the inbound number object, rather than the value passed in. --- app/service/rest.py | 7 +++++-- tests/app/service/test_rest.py | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index de0dff3b4..3122484f6 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -634,10 +634,13 @@ def add_service_sms_sender(service_id): dao_fetch_service_by_id(service_id) form = validate(request.get_json(), add_service_sms_sender_request) inbound_number_id = form.get('inbound_number_id', None) + sms_sender = form.get('sms_sender') if inbound_number_id: - dao_allocate_number_for_service(service_id=service_id, inbound_number_id=inbound_number_id) + updated_number = dao_allocate_number_for_service(service_id=service_id, inbound_number_id=inbound_number_id) + # the sms_sender in the form is the inbound_number_id from client, use number from table. + sms_sender = updated_number.number new_sms_sender = dao_add_sms_sender_for_service(service_id=service_id, - sms_sender=form['sms_sender'], + sms_sender=sms_sender, is_default=form['is_default'], inbound_number_id=inbound_number_id ) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 406f1a457..7db38221a 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2595,7 +2595,7 @@ def test_add_service_sms_sender_when_it_is_an_inbound_number(client, notify_db_s service = create_service() inbound_number = create_inbound_number(number='12345') data = { - "sms_sender": inbound_number.number, + "sms_sender": str(inbound_number.id), "is_default": False, "inbound_number_id": str(inbound_number.id) } From 202a0d008bde130e6f83bba5320933d7471be3c6 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 19 Oct 2017 17:08:56 +0100 Subject: [PATCH 15/18] Add syslog drain service to preview and staging This should and will be moved back to the approriate manifest-base-*.yml files when it is configured for production usage. --- manifest-api-preview.yml | 11 +++++++++++ manifest-api-staging.yml | 11 +++++++++++ manifest-delivery-preview.yml | 12 ++++++++++++ manifest-delivery-staging.yml | 11 +++++++++++ 4 files changed, 45 insertions(+) diff --git a/manifest-api-preview.yml b/manifest-api-preview.yml index 1eb88ef10..1e4174cb1 100644 --- a/manifest-api-preview.yml +++ b/manifest-api-preview.yml @@ -2,6 +2,17 @@ inherit: manifest-api-base.yml +services: + - notify-aws + - notify-config + - notify-db + - mmg + - firetext + - hosted-graphite + - redis + - performance-platform + - logit-ssl-syslog-drain + routes: - route: notify-api-preview.cloudapps.digital - route: api.notify.works diff --git a/manifest-api-staging.yml b/manifest-api-staging.yml index ec6602930..67044f918 100644 --- a/manifest-api-staging.yml +++ b/manifest-api-staging.yml @@ -2,6 +2,17 @@ inherit: manifest-api-base.yml +services: + - notify-aws + - notify-config + - notify-db + - mmg + - firetext + - hosted-graphite + - redis + - performance-platform + - logit-ssl-syslog-drain + routes: - route: notify-api-staging.cloudapps.digital - route: api.staging-notify.works diff --git a/manifest-delivery-preview.yml b/manifest-delivery-preview.yml index 492bc6c55..ec8203cac 100644 --- a/manifest-delivery-preview.yml +++ b/manifest-delivery-preview.yml @@ -1,4 +1,16 @@ --- inherit: manifest-delivery-base.yml + +services: + - notify-aws + - notify-config + - notify-db + - mmg + - firetext + - hosted-graphite + - redis + - performance-platform + - logit-ssl-syslog-drain + memory: 1G diff --git a/manifest-delivery-staging.yml b/manifest-delivery-staging.yml index 53c8d2f12..5c7c1dea7 100644 --- a/manifest-delivery-staging.yml +++ b/manifest-delivery-staging.yml @@ -2,5 +2,16 @@ inherit: manifest-delivery-base.yml +services: + - notify-aws + - notify-config + - notify-db + - mmg + - firetext + - hosted-graphite + - redis + - performance-platform + - logit-ssl-syslog-drain + instances: 2 memory: 1G From bfb8528ea907712da6c124e4ebc9b69725f5071e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 23 Oct 2017 10:58:06 +0100 Subject: [PATCH 16/18] The /platform-admin takes a long time, probably because the marshmallow schema used joins to the service table to return all the service data and is inefficient. The query itself has not been improved much at all but by not using a marshmallow schema I hope to get the performance gain I am looking for. --- app/dao/services_dao.py | 29 +++++++++++++++++++++++++++++ app/service/rest.py | 21 +++++++++++++++++++-- tests/app/service/test_rest.py | 27 +++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 2 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 5ae25c0e6..1082f4576 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -412,6 +412,35 @@ def fetch_stats_by_date_range_for_all_services(start_date, end_date, include_fro return query.all() +@statsd(namespace='dao') +def fetch_aggregate_stats_by_date_range_for_all_services(start_date, end_date, include_from_test_key=True): + start_date = get_london_midnight_in_utc(start_date) + end_date = get_london_midnight_in_utc(end_date + timedelta(days=1)) + table = NotificationHistory + + if start_date >= datetime.utcnow() - timedelta(days=7): + table = Notification + + query = db.session.query( + table.notification_type, + table.status, + func.count(table.id).label('count') + ).filter( + table.created_at >= start_date, + table.created_at < end_date + ).group_by( + table.notification_type, + table.status + ).order_by( + table.notification_type + ) + + if not include_from_test_key: + query = query.filter(table.key_type != KEY_TYPE_TEST) + + return query.all() + + @transactional @version_class(Service) @version_class(ApiKey) diff --git a/app/service/rest.py b/app/service/rest.py index 3122484f6..4790e0113 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -46,8 +46,8 @@ from app.dao.services_dao import ( dao_suspend_service, dao_resume_service, dao_fetch_monthly_historical_stats_for_service, - dao_fetch_monthly_historical_stats_by_template_for_service -) + dao_fetch_monthly_historical_stats_by_template_for_service, + fetch_aggregate_stats_by_date_range_for_all_services) from app.dao.service_whitelist_dao import ( dao_fetch_service_whitelist, dao_add_and_commit_whitelisted_contacts, @@ -103,6 +103,23 @@ service_blueprint = Blueprint('service', __name__) register_errors(service_blueprint) +@service_blueprint.route('/platform-stats', methods=['GET']) +def get_platform_stats(): + include_from_test_key = request.args.get('include_from_test_key', 'True') != 'False' + + # If start and end date are not set, we are expecting today's stats. + today = str(datetime.utcnow().date()) + + start_date = datetime.strptime(request.args.get('start_date', today), '%Y-%m-%d').date() + end_date = datetime.strptime(request.args.get('end_date', today), '%Y-%m-%d').date() + data = fetch_aggregate_stats_by_date_range_for_all_services(start_date=start_date, + end_date=end_date, + include_from_test_key=include_from_test_key + ) + result = jsonify(data) + return result + + @service_blueprint.route('', methods=['GET']) def get_services(): only_active = request.args.get('only_active') == 'True' diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 7db38221a..0ce87e341 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2771,3 +2771,30 @@ def test_get_service_sms_senders_for_service_returns_empty_list_when_service_doe ) assert response.status_code == 200 assert json.loads(response.get_data(as_text=True)) == [] + + +def test_get_platform_stats(client, notify_db_session): + service_1 = create_service(service_name='Service 1') + service_2 = create_service(service_name='Service 2') + sms_template = create_template(service=service_1) + email_template = create_template(service=service_2, template_type=EMAIL_TYPE) + letter_template = create_template(service=service_2, template_type=LETTER_TYPE) + create_notification(template=sms_template, status='sending') + create_notification(template=sms_template, status='delivered') + create_notification(template=sms_template, status='delivered') + create_notification(template=sms_template, status='delivered') + create_notification(template=email_template, status='temporary-failure') + create_notification(template=email_template, status='delivered') + create_notification(template=letter_template, status='sending') + create_notification(template=letter_template, status='sending') + + response = client.get('/service/platform-stats', + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp == [['email', 'delivered', 1], + ['email', 'temporary-failure', 1], + ['sms', 'delivered', 3], + ['sms', 'sending', 1], + ['letter', 'sending', 2]] From f1f2e5cd90b9af871d339ba1be8d4bf7d9af3efe Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 23 Oct 2017 15:06:11 +0100 Subject: [PATCH 17/18] Fix the results to be returned in the same format that the admin app expects. --- app/service/rest.py | 4 +++- tests/app/service/test_rest.py | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/app/service/rest.py b/app/service/rest.py index 4790e0113..3d2dc2827 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -116,7 +116,9 @@ def get_platform_stats(): end_date=end_date, include_from_test_key=include_from_test_key ) - result = jsonify(data) + stats = statistics.format_statistics(data) + + result = jsonify(stats) return result diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 0ce87e341..72cfb2cd8 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2798,3 +2798,28 @@ def test_get_platform_stats(client, notify_db_session): ['sms', 'delivered', 3], ['sms', 'sending', 1], ['letter', 'sending', 2]] + + +def test_get_platform_stats_creates_zero_stats(client, notify_db_session): + service_1 = create_service(service_name='Service 1') + service_2 = create_service(service_name='Service 2') + sms_template = create_template(service=service_1) + email_template = create_template(service=service_2, template_type=EMAIL_TYPE) + letter_template = create_template(service=service_2, template_type=LETTER_TYPE) + create_notification(template=sms_template, status='sending') + create_notification(template=sms_template, status='delivered') + create_notification(template=sms_template, status='delivered') + create_notification(template=sms_template, status='delivered') + create_notification(template=email_template, status='temporary-failure') + create_notification(template=email_template, status='delivered') + + response = client.get('/service/platform-stats', + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + print(json_resp) + assert json_resp == {'email': {'failed': 1, 'requested': 2, 'delivered': 1}, + 'sms': {'failed': 0, 'requested': 4, 'delivered': 3}, + 'letter': {'failed': 0, 'requested': 0, 'delivered': 0} + } From 5ce44c07f5e74536250612d0aad65d280e8130e1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 23 Oct 2017 16:07:10 +0100 Subject: [PATCH 18/18] Fix unit tests. --- tests/app/service/test_rest.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 72cfb2cd8..169fbe134 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2793,11 +2793,9 @@ def test_get_platform_stats(client, notify_db_session): ) assert response.status_code == 200 json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp == [['email', 'delivered', 1], - ['email', 'temporary-failure', 1], - ['sms', 'delivered', 3], - ['sms', 'sending', 1], - ['letter', 'sending', 2]] + assert json_resp['email'] == {'delivered': 1, 'requested': 2, 'failed': 1} + assert json_resp['letter'] == {'delivered': 0, 'requested': 2, 'failed': 0} + assert json_resp['sms'] == {'delivered': 3, 'requested': 4, 'failed': 0} def test_get_platform_stats_creates_zero_stats(client, notify_db_session): @@ -2805,7 +2803,6 @@ def test_get_platform_stats_creates_zero_stats(client, notify_db_session): service_2 = create_service(service_name='Service 2') sms_template = create_template(service=service_1) email_template = create_template(service=service_2, template_type=EMAIL_TYPE) - letter_template = create_template(service=service_2, template_type=LETTER_TYPE) create_notification(template=sms_template, status='sending') create_notification(template=sms_template, status='delivered') create_notification(template=sms_template, status='delivered') @@ -2818,8 +2815,6 @@ def test_get_platform_stats_creates_zero_stats(client, notify_db_session): ) assert response.status_code == 200 json_resp = json.loads(response.get_data(as_text=True)) - print(json_resp) - assert json_resp == {'email': {'failed': 1, 'requested': 2, 'delivered': 1}, - 'sms': {'failed': 0, 'requested': 4, 'delivered': 3}, - 'letter': {'failed': 0, 'requested': 0, 'delivered': 0} - } + assert json_resp['email'] == {'failed': 1, 'requested': 2, 'delivered': 1} + assert json_resp['letter'] == {'failed': 0, 'requested': 0, 'delivered': 0} + assert json_resp['sms'] == {'failed': 0, 'requested': 4, 'delivered': 3}