From c6199cb2badb6ab21a8089a9b1a75c6bf02d23b0 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Mon, 24 Jul 2017 17:54:24 +0100 Subject: [PATCH 01/39] Update pyjwt from 1.4.2 to 1.5.2 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 875e27a94..c5eb0c719 100644 --- a/requirements.txt +++ b/requirements.txt @@ -6,7 +6,7 @@ Flask-SQLAlchemy==2.0 psycopg2==2.6.2 SQLAlchemy==1.0.15 SQLAlchemy-Utils==0.32.9 -PyJWT==1.4.2 +PyJWT==1.5.2 marshmallow==2.4.2 marshmallow-sqlalchemy==0.8.0 flask-marshmallow==0.6.2 From b62952d82e7afd8cb979ea5f6bcac2c130d6b3e7 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Mon, 24 Jul 2017 17:54:50 +0100 Subject: [PATCH 02/39] Update monotonic from 1.2 to 1.3 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 875e27a94..5ff3e989b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -14,7 +14,7 @@ Flask-Bcrypt==0.6.2 credstash==1.8.0 boto3==1.4.4 celery==3.1.25 -monotonic==1.2 +monotonic==1.3 statsd==3.2.1 jsonschema==2.5.1 gunicorn==19.6.0 From 3ce66ce0ab23ce720e1b85f7e38ab78761921270 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Mon, 24 Jul 2017 17:54:53 +0100 Subject: [PATCH 03/39] Update jsonschema from 2.5.1 to 2.6.0 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 875e27a94..72ec1db43 100644 --- a/requirements.txt +++ b/requirements.txt @@ -16,7 +16,7 @@ boto3==1.4.4 celery==3.1.25 monotonic==1.2 statsd==3.2.1 -jsonschema==2.5.1 +jsonschema==2.6.0 gunicorn==19.6.0 docopt==0.6.2 six==1.10.0 From 96e005a7d2ea6f64c75aa6d6536c50c9571154dc Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Mon, 24 Jul 2017 17:55:00 +0100 Subject: [PATCH 04/39] Pin notifications-python-client to latest version 4.3.1 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 875e27a94..684bc8ff0 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,7 +23,7 @@ six==1.10.0 iso8601==0.1.11 # pin to minor version 3.1.x -notifications-python-client>=3.1,<3.2 +notifications-python-client==4.3.1 # PaaS awscli>=1.11,<1.12 From 7db1bfbb774ca222855cbaf47366477d7134a675 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 25 Jul 2017 14:45:59 +0100 Subject: [PATCH 05/39] remove credstash --- aws_run_celery.py | 8 +------- db.py | 8 ++------ requirements.txt | 1 - server_commands.py | 5 ----- wsgi.py | 7 ------- 5 files changed, 3 insertions(+), 26 deletions(-) diff --git a/aws_run_celery.py b/aws_run_celery.py index 36a1f480a..8194528db 100644 --- a/aws_run_celery.py +++ b/aws_run_celery.py @@ -1,11 +1,5 @@ #!/usr/bin/env python -from app import notify_celery, create_app -from credstash import getAllSecrets -import os - -# On AWS get secrets and export to env, skip this on Cloud Foundry -if os.getenv('VCAP_SERVICES') is None: - os.environ.update(getAllSecrets(region="eu-west-1")) +from app import create_app application = create_app("delivery") application.app_context().push() diff --git a/db.py b/db.py index 3a8da01d4..bc558cbd2 100644 --- a/db.py +++ b/db.py @@ -1,12 +1,8 @@ from flask.ext.script import Manager, Server from flask_migrate import Migrate, MigrateCommand -from app import create_app, db -from credstash import getAllSecrets -import os -# On AWS get secrets and export to env, skip this on Cloud Foundry -if os.getenv('VCAP_SERVICES') is None: - os.environ.update(getAllSecrets(region="eu-west-1")) +from app import create_app, db + application = create_app() diff --git a/requirements.txt b/requirements.txt index 0c9b3f1e0..61390b6c4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -11,7 +11,6 @@ marshmallow==2.4.2 marshmallow-sqlalchemy==0.8.0 flask-marshmallow==0.6.2 Flask-Bcrypt==0.6.2 -credstash==1.8.0 boto3==1.4.4 celery==3.1.25 # pyup: <4 monotonic==1.2 diff --git a/server_commands.py b/server_commands.py index 85bf13137..af071db47 100644 --- a/server_commands.py +++ b/server_commands.py @@ -1,7 +1,6 @@ from flask.ext.script import Manager, Server from flask_migrate import Migrate, MigrateCommand from app import (create_app, db, commands) -from credstash import getAllSecrets import os default_env_file = '/home/ubuntu/environment' @@ -11,10 +10,6 @@ if os.path.isfile(default_env_file): with open(default_env_file, 'r') as environment_file: environment = environment_file.readline().strip() -# On AWS get secrets and export to env, skip this on Cloud Foundry -if os.getenv('VCAP_SERVICES') is None: - os.environ.update(getAllSecrets(region="eu-west-1")) - from app.config import configs os.environ['NOTIFY_API_ENVIRONMENT'] = configs[environment] diff --git a/wsgi.py b/wsgi.py index 2df2c3976..9fbeb28ac 100644 --- a/wsgi.py +++ b/wsgi.py @@ -1,13 +1,6 @@ -import os - from app import create_app -from credstash import getAllSecrets -# On AWS get secrets and export to env, skip this on Cloud Foundry -if os.getenv('VCAP_SERVICES') is None: - os.environ.update(getAllSecrets(region="eu-west-1")) - application = create_app() if __name__ == "__main__": From 2746bf0318cfc45ee16c13b33e47f39536b81c33 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 26 Jul 2017 15:57:30 +0100 Subject: [PATCH 06/39] process letters from api traffic there are three steps to this 1. Create a job * Starts in status 'ready to send' * Created by None, since it's from the API * original file name 'letter submitted via api' 2. Create a single notification for that job * job_row_number 0 * client reference if provided * address line 1 as recipient 3. Trigger the build_dvla_file task we know that all the notifications have been created for this job (since we just created them ourselves synchronously), so this will just create the dvla-format file for the job, and upload it to s3. --- app/models.py | 2 +- .../process_letter_notifications.py | 41 +++++++++++++++++++ app/notifications/process_notifications.py | 2 +- app/v2/notifications/post_notifications.py | 24 +++++++---- 4 files changed, 58 insertions(+), 11 deletions(-) create mode 100644 app/notifications/process_letter_notifications.py diff --git a/app/models.py b/app/models.py index 5ab742b4f..db5bb1a1e 100644 --- a/app/models.py +++ b/app/models.py @@ -619,7 +619,7 @@ class JobStatus(db.Model): class Job(db.Model): __tablename__ = 'jobs' - id = db.Column(UUID(as_uuid=True), primary_key=True) + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) original_file_name = db.Column(db.String, nullable=False) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False, nullable=False) service = db.relationship('Service', backref=db.backref('jobs', lazy='dynamic')) diff --git a/app/notifications/process_letter_notifications.py b/app/notifications/process_letter_notifications.py new file mode 100644 index 000000000..358df8771 --- /dev/null +++ b/app/notifications/process_letter_notifications.py @@ -0,0 +1,41 @@ +from app import create_random_identifier +from app.models import LETTER_TYPE, JOB_STATUS_READY_TO_SEND +from app.notifications.process_notifications import persist_notification + + +def create_letter_api_job(template): + service = template.service + if not service.active: + raise InvalidRequest('Create job is not allowed: service is inactive', 403) + if template.archived: + raise InvalidRequest('Create job is not allowed: template is deleted', 400) + + + job = Job( + original_file_name='letter submitted via api', + service=service, + template=template, + template_version=template.version, + notification_count=1, + job_status=JOB_STATUS_READY_TO_SEND, + created_by=None + ) + dao_create_job(job) + + +def create_letter_notification(letter_data, job, api_key): + notification = persist_notification( + template_id=job.template.id, + template_version=job.template.version, + recipient=letter_data['personalisation']['address line 1'], # or addressline1 or address_line_1? + service=job.service, + personalisation=letter_data['personalisation'], + notification_type=LETTER_TYPE, + api_key_id=api_key.id, + key_type=api_key.key_type, + job_id=job.id, + job_row_number=0, + reference=create_random_identifier(), + client_reference=letter_data.get('reference') + ) + return notification diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 19430c386..e7e8779f2 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -55,7 +55,7 @@ def persist_notification( created_by_id=None ): notification_created_at = created_at or datetime.utcnow() - if not notification_id and simulated: + if not notification_id: notification_id = uuid.uuid4() notification = Notification( id=notification_id, diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index f39f54345..502f2aa4d 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -5,11 +5,16 @@ from flask import request, jsonify, current_app, abort from app import api_user, authenticated_service from app.config import QueueNames from app.models import SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, PRIORITY +from app.celery.tasks import build_dvla_file from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue, simulated_recipient, persist_scheduled_notification) +from app.notifications.process_letter_notifications import ( + create_letter_api_job, + create_letter_notification +) from app.notifications.validators import ( validate_and_format_recipient, check_rate_limiting, @@ -58,10 +63,9 @@ def post_notification(notification_type): if notification_type == LETTER_TYPE: notification = process_letter_notification( - form=form, + letter_data=form, api_key=api_user, template=template, - service=authenticated_service, ) else: notification = process_sms_or_email_notification( @@ -140,10 +144,12 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ return notification -def process_letter_notification(*, form, api_key, template, service): - # create job - - # create notification - - # trigger build_dvla_file task - raise NotImplementedError +def process_letter_notification(*, letter_data, api_key, template, service): + job = create_letter_api_job(template) + notification = create_letter_notification(letter_data, job, api_key) + build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) + current_app.logger.info("send job {} for api notification {} to build-dvla-file in the process-job queue".format( + job.id, + notification.id + )) + return notification From 11458c421b4572f31585d4dd2786367fe6a1978b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 27 Jul 2017 11:10:22 +0100 Subject: [PATCH 07/39] ensure permissions are correct in sample letter fixtures sample_letter_* should always include a service that has letter permissions. Also, print out the JSON response in the admin_request fixture if the response code doesn't match --- tests/app/conftest.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 4fb05d422..ed1516337 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -255,8 +255,8 @@ def sample_template_without_email_permission(notify_db, notify_db_session): @pytest.fixture -def sample_letter_template(sample_service): - return create_template(sample_service, template_type=LETTER_TYPE) +def sample_letter_template(sample_service_full_permissions): + return create_template(sample_service_full_permissions, template_type=LETTER_TYPE) @pytest.fixture(scope='function') @@ -397,17 +397,18 @@ def sample_email_job(notify_db, @pytest.fixture -def sample_letter_job(sample_service, sample_letter_template): +def sample_letter_job(sample_letter_template): + service = sample_letter_template.service data = { 'id': uuid.uuid4(), - 'service_id': sample_service.id, - 'service': sample_service, + 'service_id': service.id, + 'service': service, 'template_id': sample_letter_template.id, 'template_version': sample_letter_template.version, 'original_file_name': 'some.csv', 'notification_count': 1, 'created_at': datetime.utcnow(), - 'created_by': sample_service.created_by, + 'created_by': service.created_by, } job = Job(**data) dao_create_job(job) @@ -1026,7 +1027,7 @@ def admin_request(client): headers=[('Content-Type', 'application/json'), create_authorization_header()] ) json_resp = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == _expected_status + assert resp.status_code == _expected_status, json_resp return json_resp @staticmethod @@ -1036,7 +1037,7 @@ def admin_request(client): headers=[create_authorization_header()] ) json_resp = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == _expected_status + assert resp.status_code == _expected_status, json_resp return json_resp return AdminRequest From f528236eda42e1c55823a90722cc53e1d5e59d2a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 27 Jul 2017 12:58:13 +0100 Subject: [PATCH 08/39] make job.created_by nullable Since letter jobs from the API aren't created by any single individual, lets make created_by nullable. Note: We'll have to make sure that we update the admin app to handle these jobs nicely --- app/models.py | 2 +- app/v2/notifications/post_notifications.py | 2 +- .../versions/0113_job_created_by_nullable.py | 24 +++++++++++++++++++ .../test_post_letter_notifications.py | 6 +---- 4 files changed, 27 insertions(+), 7 deletions(-) create mode 100644 migrations/versions/0113_job_created_by_nullable.py diff --git a/app/models.py b/app/models.py index db5bb1a1e..3a42a5319 100644 --- a/app/models.py +++ b/app/models.py @@ -654,7 +654,7 @@ class Job(db.Model): unique=False, nullable=True) created_by = db.relationship('User') - created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) + created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=True) scheduled_for = db.Column( db.DateTime, index=True, diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 502f2aa4d..e36ac6c9b 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -144,7 +144,7 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ return notification -def process_letter_notification(*, letter_data, api_key, template, service): +def process_letter_notification(*, letter_data, api_key, template): job = create_letter_api_job(template) notification = create_letter_notification(letter_data, job, api_key) build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) diff --git a/migrations/versions/0113_job_created_by_nullable.py b/migrations/versions/0113_job_created_by_nullable.py new file mode 100644 index 000000000..8596dac7b --- /dev/null +++ b/migrations/versions/0113_job_created_by_nullable.py @@ -0,0 +1,24 @@ +"""empty message + +Revision ID: 0113_job_created_by_nullable +Revises: 0112_add_start_end_dates +Create Date: 2017-07-27 11:12:34.938086 + +""" + +# revision identifiers, used by Alembic. +revision = '0113_job_created_by_nullable' +down_revision = '0112_add_start_end_dates' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + op.alter_column('job', 'created_by', nullable=True) + + +def downgrade(): + # This will error if there are any jobs with no created_by - we'll have to decide how to handle those as and when + # we downgrade + op.alter_column('job', 'created_by', nullable=False) diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index d65c6c5d8..e794520f2 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -1,4 +1,3 @@ - import uuid from flask import url_for, json @@ -11,9 +10,6 @@ from tests import create_authorization_header from tests.app.db import create_service, create_template -pytestmark = pytest.mark.skip('Leters not currently implemented') - - def letter_request(client, data, service_id, _expected_status=201): resp = client.post( url_for('v2_notifications.post_notification', notification_type='letter'), @@ -160,7 +156,7 @@ def test_post_letter_notification_returns_403_if_not_allowed_to_send_notificatio } error_json = letter_request(client, data, service_id=service.id, _expected_status=400) - assert error_json['status_code'] == 403 + assert error_json['status_code'] == 400 assert error_json['errors'] == [ {'error': 'BadRequestError', 'message': 'Cannot send letters'} ] From 2ab105aaf44a33fce050edb46cfb44640e222424 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 27 Jul 2017 11:10:22 +0100 Subject: [PATCH 09/39] add tests for letter api notifications --- app/dao/jobs_dao.py | 3 + .../process_letter_notifications.py | 13 +-- app/utils.py | 2 +- .../versions/0113_job_created_by_nullable.py | 4 +- .../test_process_letter_notifications.py | 83 +++++++++++++++++++ .../test_post_letter_notifications.py | 44 ++++------ .../notifications/test_post_notifications.py | 10 ++- 7 files changed, 120 insertions(+), 39 deletions(-) create mode 100644 tests/app/notifications/test_process_letter_notifications.py diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index ec7bbe2a6..81c00633c 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -1,3 +1,4 @@ +import uuid from datetime import datetime, timedelta from flask import current_app @@ -108,6 +109,8 @@ def dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id): def dao_create_job(job): + if not job.id: + job.id = uuid.uuid4() job_stats = JobStatistics( job_id=job.id, updated_at=datetime.utcnow() diff --git a/app/notifications/process_letter_notifications.py b/app/notifications/process_letter_notifications.py index 358df8771..0cfb35442 100644 --- a/app/notifications/process_letter_notifications.py +++ b/app/notifications/process_letter_notifications.py @@ -1,15 +1,16 @@ from app import create_random_identifier -from app.models import LETTER_TYPE, JOB_STATUS_READY_TO_SEND +from app.models import LETTER_TYPE, JOB_STATUS_READY_TO_SEND, Job +from app.dao.jobs_dao import dao_create_job from app.notifications.process_notifications import persist_notification +from app.v2.errors import InvalidRequest def create_letter_api_job(template): service = template.service if not service.active: - raise InvalidRequest('Create job is not allowed: service is inactive', 403) + raise InvalidRequest('Service {} is inactive'.format(service.id), 403) if template.archived: - raise InvalidRequest('Create job is not allowed: template is deleted', 400) - + raise InvalidRequest('Template {} is deleted'.format(template.id), 400) job = Job( original_file_name='letter submitted via api', @@ -21,13 +22,15 @@ def create_letter_api_job(template): created_by=None ) dao_create_job(job) + return job def create_letter_notification(letter_data, job, api_key): notification = persist_notification( template_id=job.template.id, template_version=job.template.version, - recipient=letter_data['personalisation']['address line 1'], # or addressline1 or address_line_1? + # we only accept addresses_with_underscores from the API (from CSV we also accept dashes, spaces etc) + recipient=letter_data['personalisation']['address_line_1'], service=job.service, personalisation=letter_data['personalisation'], notification_type=LETTER_TYPE, diff --git a/app/utils.py b/app/utils.py index ea30a6df2..3cb69294e 100644 --- a/app/utils.py +++ b/app/utils.py @@ -30,7 +30,7 @@ def url_with_token(data, url, config): def get_template_instance(template, values): from app.models import SMS_TYPE, EMAIL_TYPE, LETTER_TYPE return { - SMS_TYPE: SMSMessageTemplate, EMAIL_TYPE: PlainTextEmailTemplate, LETTER_TYPE: LetterPreviewTemplate + SMS_TYPE: SMSMessageTemplate, EMAIL_TYPE: PlainTextEmailTemplate, LETTER_TYPE: PlainTextEmailTemplate }[template['template_type']](template, values) diff --git a/migrations/versions/0113_job_created_by_nullable.py b/migrations/versions/0113_job_created_by_nullable.py index 8596dac7b..c6a391523 100644 --- a/migrations/versions/0113_job_created_by_nullable.py +++ b/migrations/versions/0113_job_created_by_nullable.py @@ -15,10 +15,10 @@ import sqlalchemy as sa from sqlalchemy.dialects import postgresql def upgrade(): - op.alter_column('job', 'created_by', nullable=True) + op.alter_column('jobs', 'created_by_id', nullable=True) def downgrade(): # This will error if there are any jobs with no created_by - we'll have to decide how to handle those as and when # we downgrade - op.alter_column('job', 'created_by', nullable=False) + op.alter_column('jobs', 'created_by_id', nullable=False) diff --git a/tests/app/notifications/test_process_letter_notifications.py b/tests/app/notifications/test_process_letter_notifications.py new file mode 100644 index 000000000..4a65a487e --- /dev/null +++ b/tests/app/notifications/test_process_letter_notifications.py @@ -0,0 +1,83 @@ +import pytest + +from app.dao.services_dao import dao_archive_service +from app.models import Job +from app.models import JOB_STATUS_READY_TO_SEND +from app.models import LETTER_TYPE +from app.models import Notification +from app.notifications.process_letter_notifications import create_letter_api_job +from app.notifications.process_letter_notifications import create_letter_notification +from app.v2.errors import InvalidRequest + +from tests.app.db import create_service +from tests.app.db import create_template + + +def test_create_job_rejects_inactive_service(notify_db_session): + service = create_service() + template = create_template(service, template_type=LETTER_TYPE) + dao_archive_service(service.id) + + with pytest.raises(InvalidRequest) as exc_info: + create_letter_api_job(template) + + assert exc_info.value.message == 'Service {} is inactive'.format(service.id) + + +def test_create_job_rejects_archived_template(sample_letter_template): + sample_letter_template.archived = True + + with pytest.raises(InvalidRequest) as exc_info: + create_letter_api_job(sample_letter_template) + + assert exc_info.value.message == 'Template {} is deleted'.format(sample_letter_template.id) + + +def test_create_job_creates_job(sample_letter_template): + job = create_letter_api_job(sample_letter_template) + + assert job == Job.query.one() + assert job.original_file_name == 'letter submitted via api' + assert job.service == sample_letter_template.service + assert job.template_id == sample_letter_template.id + assert job.template_version == sample_letter_template.version + assert job.notification_count == 1 + assert job.job_status == JOB_STATUS_READY_TO_SEND + assert job.created_by is None + + +def test_create_letter_notification_creates_notification(sample_letter_job, sample_api_key): + data = { + 'personalisation': { + 'address_line_1': 'The Queen', + 'address_line_2': 'Buckingham Palace', + 'postcode': 'SW1 1AA', + } + } + + notification = create_letter_notification(data, sample_letter_job, sample_api_key) + + assert notification == Notification.query.one() + assert notification.job == sample_letter_job + assert notification.template == sample_letter_job.template + assert notification.api_key == sample_api_key + assert notification.notification_type == LETTER_TYPE + assert notification.key_type == sample_api_key.key_type + assert notification.job_row_number == 0 + assert notification.reference is not None + assert notification.client_reference is None + + +def test_create_letter_notification_sets_reference(sample_letter_job, sample_api_key): + data = { + 'personalisation': { + 'address_line_1': 'The Queen', + 'address_line_2': 'Buckingham Palace', + 'postcode': 'SW1 1AA', + }, + 'reference': 'foo' + } + + notification = create_letter_notification(data, sample_letter_job, sample_api_key) + + assert notification.client_reference == 'foo' diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index e794520f2..66a9004c7 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -1,13 +1,20 @@ import uuid -from flask import url_for, json +from flask import json +from flask import url_for import pytest -from app.models import Job, Notification, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE +from app.models import EMAIL_TYPE +from app.models import Job +from app.models import LETTER_TYPE +from app.models import Notification +from app.models import SMS_TYPE from app.v2.errors import RateLimitError +from app.v2.notifications.post_notifications import process_letter_notification from tests import create_authorization_header -from tests.app.db import create_service, create_template +from tests.app.db import create_service +from tests.app.db import create_template def letter_request(client, data, service_id, _expected_status=201): @@ -41,7 +48,7 @@ def test_post_letter_notification_returns_201(client, sample_letter_template, mo resp_json = letter_request(client, data, service_id=sample_letter_template.service_id) job = Job.query.one() - notification = Notification.query.all() + notification = Notification.query.one() notification_id = notification.id assert resp_json['id'] == str(notification_id) assert resp_json['reference'] == reference @@ -58,51 +65,33 @@ def test_post_letter_notification_returns_201(client, sample_letter_template, mo ) assert not resp_json['scheduled_for'] - mocked.assert_called_once_with((str(job.id), ), queue='job-tasks') + mocked.assert_called_once_with([str(job.id)], queue='job-tasks') def test_post_letter_notification_returns_400_and_missing_template( client, - sample_service + sample_service_full_permissions ): data = { 'template_id': str(uuid.uuid4()), 'personalisation': {'address_line_1': '', 'postcode': ''} } - error_json = letter_request(client, data, service_id=sample_service.id, _expected_status=400) + error_json = letter_request(client, data, service_id=sample_service_full_permissions.id, _expected_status=400) assert error_json['status_code'] == 400 assert error_json['errors'] == [{'error': 'BadRequestError', 'message': 'Template not found'}] -def test_post_notification_returns_403_and_well_formed_auth_error( - client, - sample_letter_template -): - data = { - 'template_id': str(sample_letter_template.id), - 'personalisation': {'address_line_1': '', 'postcode': ''} - } - - error_json = letter_request(client, data, service_id=sample_letter_template.service_id, _expected_status=401) - - assert error_json['status_code'] == 401 - assert error_json['errors'] == [{ - 'error': 'AuthError', - 'message': 'Unauthorized, authentication token must be provided' - }] - - def test_notification_returns_400_for_schema_problems( client, - sample_service + sample_service_full_permissions ): data = { 'personalisation': {'address_line_1': '', 'postcode': ''} } - error_json = letter_request(client, data, service_id=sample_service.id, _expected_status=400) + error_json = letter_request(client, data, service_id=sample_service_full_permissions.id, _expected_status=400) assert error_json['status_code'] == 400 assert error_json['errors'] == [{ @@ -111,6 +100,7 @@ def test_notification_returns_400_for_schema_problems( }] + def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( client, sample_letter_template, diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 71dc87fce..c108faacf 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -80,10 +80,12 @@ def test_post_notification_returns_400_and_missing_template(client, sample_servi "message": 'Template not found'}] -@pytest.mark.parametrize("notification_type, key_send_to, send_to", - [("sms", "phone_number", "+447700900855"), - ("email", "email_address", "sample@email.com")]) -def test_post_notification_returns_403_and_well_formed_auth_error(client, sample_template, +@pytest.mark.parametrize("notification_type, key_send_to, send_to", [ + ("sms", "phone_number", "+447700900855"), + ("email", "email_address", "sample@email.com"), + ("letter", "personalisation", {"address_line_1": "The queen", "postcode": "SW1 1AA"}) +]) +def test_post_notification_returns_401_and_well_formed_auth_error(client, sample_template, notification_type, key_send_to, send_to): data = { key_send_to: send_to, From 11f860331949875bc278c4c5a2852a50d978c254 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 27 Jul 2017 16:49:37 +0100 Subject: [PATCH 10/39] Remove custom error message from personalisation validation There's no longer a single err msg that fits all problems with personalisation - since letters expect specific fields there --- app/schema_validation/definitions.py | 1 - .../test_notification_schemas.py | 2 +- .../test_post_letter_notifications.py | 20 +++++++++++++++++++ .../app/v2/template/test_template_schemas.py | 17 +++++++++++----- 4 files changed, 33 insertions(+), 7 deletions(-) diff --git a/app/schema_validation/definitions.py b/app/schema_validation/definitions.py index e779c5794..0bf163999 100644 --- a/app/schema_validation/definitions.py +++ b/app/schema_validation/definitions.py @@ -14,7 +14,6 @@ uuid = { personalisation = { "type": "object", - "validationMessage": "should contain key value pairs", "code": "1001", # yet to be implemented "link": "link to our error documentation not yet implemented" } diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 0aafb38cc..83ad8c5f7 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -127,7 +127,7 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): error = json.loads(str(e.value)) assert len(error.get('errors')) == 1 assert error['errors'] == [{'error': 'ValidationError', - 'message': "personalisation should contain key value pairs"}] + 'message': "personalisation not_a_dict is not of type object"}] assert error.get('status_code') == 400 assert len(error.keys()) == 2 diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 66a9004c7..18f36d421 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -100,6 +100,26 @@ def test_notification_returns_400_for_schema_problems( }] +def test_notification_returns_400_if_address_doesnt_have_underscores( + client, + sample_letter_template +): + data = { + 'template_id': str(sample_letter_template.id), + 'personalisation': { + 'address line 1': 'Her Royal Highness Queen Elizabeth II', + 'postcode': 'SW1 1AA', + } + } + + error_json = letter_request(client, data, service_id=sample_letter_template.service_id, _expected_status=400) + + assert error_json['status_code'] == 400 + assert error_json['errors'] == [{ + 'error': 'ValidationError', + 'message': 'personalisation address_line_1 is a required property' + }] + def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( client, diff --git a/tests/app/v2/template/test_template_schemas.py b/tests/app/v2/template/test_template_schemas.py index 4517ad7e8..1e5a91e11 100644 --- a/tests/app/v2/template/test_template_schemas.py +++ b/tests/app/v2/template/test_template_schemas.py @@ -51,11 +51,18 @@ valid_json_post_args = { } invalid_json_post_args = [ - ({"id": "invalid_uuid", "personalisation": {"key": "value"}}, ["id is not a valid UUID"]), - ({"id": str(uuid.uuid4()), "personalisation": "invalid_personalisation"}, - ["personalisation should contain key value pairs"]), - ({"personalisation": "invalid_personalisation"}, - ["id is a required property", "personalisation should contain key value pairs"]) + ( + {"id": "invalid_uuid", "personalisation": {"key": "value"}}, + ["id is not a valid UUID"] + ), + ( + {"id": str(uuid.uuid4()), "personalisation": ['a', 'b']}, + ["personalisation [a, b] is not of type object"] + ), + ( + {"personalisation": "invalid_personalisation"}, + ["id is a required property", "personalisation invalid_personalisation is not of type object"] + ) ] valid_json_post_response = { From c87511959ce4f5ae7b54954bb4af7a197281ea00 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Thu, 27 Jul 2017 23:15:02 +0100 Subject: [PATCH 11/39] Update iso8601 from 0.1.11 to 0.1.12 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 0c9b3f1e0..60547def7 100644 --- a/requirements.txt +++ b/requirements.txt @@ -20,7 +20,7 @@ jsonschema==2.5.1 gunicorn==19.6.0 docopt==0.6.2 six==1.10.0 -iso8601==0.1.11 +iso8601==0.1.12 # pin to minor version 3.1.x notifications-python-client>=3.1,<3.2 From de41fde0d37e3f52b316bcda74445c07d04eb28b Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Fri, 28 Jul 2017 16:09:49 +0100 Subject: [PATCH 12/39] Revert "remove credstash" This reverts commit 7db1bfbb774ca222855cbaf47366477d7134a675. --- aws_run_celery.py | 8 +++++++- db.py | 6 +++++- requirements.txt | 1 + server_commands.py | 5 +++++ wsgi.py | 9 ++++++++- 5 files changed, 26 insertions(+), 3 deletions(-) diff --git a/aws_run_celery.py b/aws_run_celery.py index 8194528db..36a1f480a 100644 --- a/aws_run_celery.py +++ b/aws_run_celery.py @@ -1,5 +1,11 @@ #!/usr/bin/env python -from app import create_app +from app import notify_celery, create_app +from credstash import getAllSecrets +import os + +# On AWS get secrets and export to env, skip this on Cloud Foundry +if os.getenv('VCAP_SERVICES') is None: + os.environ.update(getAllSecrets(region="eu-west-1")) application = create_app("delivery") application.app_context().push() diff --git a/db.py b/db.py index bc558cbd2..3a8da01d4 100644 --- a/db.py +++ b/db.py @@ -1,8 +1,12 @@ from flask.ext.script import Manager, Server from flask_migrate import Migrate, MigrateCommand - from app import create_app, db +from credstash import getAllSecrets +import os +# On AWS get secrets and export to env, skip this on Cloud Foundry +if os.getenv('VCAP_SERVICES') is None: + os.environ.update(getAllSecrets(region="eu-west-1")) application = create_app() diff --git a/requirements.txt b/requirements.txt index 61390b6c4..0c9b3f1e0 100644 --- a/requirements.txt +++ b/requirements.txt @@ -11,6 +11,7 @@ marshmallow==2.4.2 marshmallow-sqlalchemy==0.8.0 flask-marshmallow==0.6.2 Flask-Bcrypt==0.6.2 +credstash==1.8.0 boto3==1.4.4 celery==3.1.25 # pyup: <4 monotonic==1.2 diff --git a/server_commands.py b/server_commands.py index af071db47..85bf13137 100644 --- a/server_commands.py +++ b/server_commands.py @@ -1,6 +1,7 @@ from flask.ext.script import Manager, Server from flask_migrate import Migrate, MigrateCommand from app import (create_app, db, commands) +from credstash import getAllSecrets import os default_env_file = '/home/ubuntu/environment' @@ -10,6 +11,10 @@ if os.path.isfile(default_env_file): with open(default_env_file, 'r') as environment_file: environment = environment_file.readline().strip() +# On AWS get secrets and export to env, skip this on Cloud Foundry +if os.getenv('VCAP_SERVICES') is None: + os.environ.update(getAllSecrets(region="eu-west-1")) + from app.config import configs os.environ['NOTIFY_API_ENVIRONMENT'] = configs[environment] diff --git a/wsgi.py b/wsgi.py index 9fbeb28ac..2df2c3976 100644 --- a/wsgi.py +++ b/wsgi.py @@ -1,5 +1,12 @@ -from app import create_app +import os +from app import create_app +from credstash import getAllSecrets + + +# On AWS get secrets and export to env, skip this on Cloud Foundry +if os.getenv('VCAP_SERVICES') is None: + os.environ.update(getAllSecrets(region="eu-west-1")) application = create_app() From 8e738b783e6f93fdf36c91ffdc65ef5751756440 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 27 Jul 2017 17:07:14 +0100 Subject: [PATCH 13/39] update test_send_notification to account for new uuid mock --- app/v2/notifications/notification_schemas.py | 3 +- tests/app/job/test_rest.py | 1 - .../rest/test_send_notification.py | 196 +++++++++--------- 3 files changed, 99 insertions(+), 101 deletions(-) diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 7f61f0ee0..fefbf5634 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -228,7 +228,8 @@ post_letter_response = { "content": letter_content, "uri": {"type": "string", "format": "uri"}, "template": template, - "scheduled_for": {"type": ["string", "null"]} + # letters cannot be scheduled + "scheduled_for": {"type": "null"} }, "required": ["id", "content", "uri", "template"] } diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index ca25e0769..7ce45b51e 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -276,7 +276,6 @@ def test_create_job_returns_400_if_missing_data(notify_api, sample_template, moc assert resp_json['result'] == 'error' assert 'Missing data for required field.' in resp_json['message']['original_file_name'] assert 'Missing data for required field.' in resp_json['message']['notification_count'] - assert 'Missing data for required field.' in resp_json['message']['id'] def test_create_job_returns_404_if_template_does_not_exist(notify_api, sample_service, mocker): diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 3822395cd..bc723341e 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -572,128 +572,124 @@ def test_should_not_send_sms_if_team_api_key_and_not_a_service_user(notify_api, ] == json_resp['message']['to'] -def test_should_send_email_if_team_api_key_and_a_service_user(notify_api, sample_email_template, fake_uuid, mocker): - with notify_api.test_request_context(), notify_api.test_client() as client: - mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) +def test_should_send_email_if_team_api_key_and_a_service_user(client, sample_email_template, fake_uuid, mocker): + mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + mocker.patch('app.notifications.process_notifications.uuid.uuid4', return_value=fake_uuid) - data = { - 'to': sample_email_template.service.created_by.email_address, - 'template': sample_email_template.id - } - api_key = ApiKey(service=sample_email_template.service, - name='team_key', - created_by=sample_email_template.created_by, - key_type=KEY_TYPE_TEAM) - save_model_api_key(api_key) - auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) + data = { + 'to': sample_email_template.service.created_by.email_address, + 'template': sample_email_template.id + } + api_key = ApiKey(service=sample_email_template.service, + name='team_key', + created_by=sample_email_template.created_by, + key_type=KEY_TYPE_TEAM) + save_model_api_key(api_key) + auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) - response = client.post( - path='/notifications/email', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + response = client.post( + path='/notifications/email', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with( - [fake_uuid], - queue='send-email-tasks' - ) - assert response.status_code == 201 + app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with( + [fake_uuid], + queue='send-email-tasks' + ) + assert response.status_code == 201 @pytest.mark.parametrize('restricted', [True, False]) @pytest.mark.parametrize('limit', [0, 1]) def test_should_send_sms_to_anyone_with_test_key( - notify_api, sample_template, mocker, restricted, limit, fake_uuid + client, sample_template, mocker, restricted, limit, fake_uuid ): - with notify_api.test_request_context(), notify_api.test_client() as client: - mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.notifications.process_notifications.uuid.uuid4', return_value=fake_uuid) - data = { - 'to': '07811111111', - 'template': sample_template.id - } - sample_template.service.restricted = restricted - sample_template.service.message_limit = limit - api_key = ApiKey( - service=sample_template.service, - name='test_key', - created_by=sample_template.created_by, - key_type=KEY_TYPE_TEST - ) - save_model_api_key(api_key) - auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) + data = { + 'to': '07811111111', + 'template': sample_template.id + } + sample_template.service.restricted = restricted + sample_template.service.message_limit = limit + api_key = ApiKey( + service=sample_template.service, + name='test_key', + created_by=sample_template.created_by, + key_type=KEY_TYPE_TEST + ) + save_model_api_key(api_key) + auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) - response = client.post( - path='/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))] - ) - app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [fake_uuid], queue='research-mode-tasks' - ) - assert response.status_code == 201 + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))] + ) + app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with( + [fake_uuid], queue='research-mode-tasks' + ) + assert response.status_code == 201 @pytest.mark.parametrize('restricted', [True, False]) @pytest.mark.parametrize('limit', [0, 1]) def test_should_send_email_to_anyone_with_test_key( - notify_api, sample_email_template, mocker, restricted, limit, fake_uuid + client, sample_email_template, mocker, restricted, limit, fake_uuid ): - with notify_api.test_request_context(), notify_api.test_client() as client: - mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) + mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + mocker.patch('app.notifications.process_notifications.uuid.uuid4', return_value=fake_uuid) - data = { - 'to': 'anyone123@example.com', - 'template': sample_email_template.id - } - sample_email_template.service.restricted = restricted - sample_email_template.service.message_limit = limit - api_key = ApiKey( - service=sample_email_template.service, - name='test_key', - created_by=sample_email_template.created_by, - key_type=KEY_TYPE_TEST - ) - save_model_api_key(api_key) - auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) + data = { + 'to': 'anyone123@example.com', + 'template': sample_email_template.id + } + sample_email_template.service.restricted = restricted + sample_email_template.service.message_limit = limit + api_key = ApiKey( + service=sample_email_template.service, + name='test_key', + created_by=sample_email_template.created_by, + key_type=KEY_TYPE_TEST + ) + save_model_api_key(api_key) + auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) - response = client.post( - path='/notifications/email', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))] - ) + response = client.post( + path='/notifications/email', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))] + ) - app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with( - [fake_uuid], queue='research-mode-tasks' - ) - assert response.status_code == 201 + app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with( + [fake_uuid], queue='research-mode-tasks' + ) + assert response.status_code == 201 -def test_should_send_sms_if_team_api_key_and_a_service_user(notify_api, sample_template, fake_uuid, mocker): - with notify_api.test_request_context(), notify_api.test_client() as client: - mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) +def test_should_send_sms_if_team_api_key_and_a_service_user(client, sample_template, fake_uuid, mocker): + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.notifications.process_notifications.uuid.uuid4', return_value=fake_uuid) - data = { - 'to': sample_template.service.created_by.mobile_number, - 'template': sample_template.id - } - api_key = ApiKey(service=sample_template.service, - name='team_key', - created_by=sample_template.created_by, - key_type=KEY_TYPE_TEAM) - save_model_api_key(api_key) - auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) + data = { + 'to': sample_template.service.created_by.mobile_number, + 'template': sample_template.id + } + api_key = ApiKey(service=sample_template.service, + name='team_key', + created_by=sample_template.created_by, + key_type=KEY_TYPE_TEAM) + save_model_api_key(api_key) + auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) - response = client.post( - path='/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with([fake_uuid], queue='send-sms-tasks') - assert response.status_code == 201 + app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with([fake_uuid], queue='send-sms-tasks') + assert response.status_code == 201 @pytest.mark.parametrize('template_type,queue_name', [ @@ -710,7 +706,8 @@ def test_should_persist_notification( queue_name ): mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type)) - mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) + mocker.patch('app.notifications.process_notifications.uuid.uuid4', return_value=fake_uuid) + template = sample_template if template_type == SMS_TYPE else sample_email_template to = sample_template.service.created_by.mobile_number if template_type == SMS_TYPE \ else sample_email_template.service.created_by.email_address @@ -757,7 +754,8 @@ def test_should_delete_notification_and_return_error_if_sqs_fails( 'app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type), side_effect=Exception("failed to talk to SQS") ) - mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) + mocker.patch('app.notifications.process_notifications.uuid.uuid4', return_value=fake_uuid) + template = sample_template if template_type == SMS_TYPE else sample_email_template to = sample_template.service.created_by.mobile_number if template_type == SMS_TYPE \ else sample_email_template.service.created_by.email_address From 9c212e78af2b0e2c17b4375581226660b93f9837 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 31 Jul 2017 11:11:00 +0100 Subject: [PATCH 14/39] Revert "Revert "remove credstash"" This reverts commit de41fde0d37e3f52b316bcda74445c07d04eb28b. --- aws_run_celery.py | 8 +------- db.py | 8 ++------ requirements.txt | 1 - server_commands.py | 5 ----- wsgi.py | 7 ------- 5 files changed, 3 insertions(+), 26 deletions(-) diff --git a/aws_run_celery.py b/aws_run_celery.py index 36a1f480a..8194528db 100644 --- a/aws_run_celery.py +++ b/aws_run_celery.py @@ -1,11 +1,5 @@ #!/usr/bin/env python -from app import notify_celery, create_app -from credstash import getAllSecrets -import os - -# On AWS get secrets and export to env, skip this on Cloud Foundry -if os.getenv('VCAP_SERVICES') is None: - os.environ.update(getAllSecrets(region="eu-west-1")) +from app import create_app application = create_app("delivery") application.app_context().push() diff --git a/db.py b/db.py index 3a8da01d4..bc558cbd2 100644 --- a/db.py +++ b/db.py @@ -1,12 +1,8 @@ from flask.ext.script import Manager, Server from flask_migrate import Migrate, MigrateCommand -from app import create_app, db -from credstash import getAllSecrets -import os -# On AWS get secrets and export to env, skip this on Cloud Foundry -if os.getenv('VCAP_SERVICES') is None: - os.environ.update(getAllSecrets(region="eu-west-1")) +from app import create_app, db + application = create_app() diff --git a/requirements.txt b/requirements.txt index 0c9b3f1e0..61390b6c4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -11,7 +11,6 @@ marshmallow==2.4.2 marshmallow-sqlalchemy==0.8.0 flask-marshmallow==0.6.2 Flask-Bcrypt==0.6.2 -credstash==1.8.0 boto3==1.4.4 celery==3.1.25 # pyup: <4 monotonic==1.2 diff --git a/server_commands.py b/server_commands.py index 85bf13137..af071db47 100644 --- a/server_commands.py +++ b/server_commands.py @@ -1,7 +1,6 @@ from flask.ext.script import Manager, Server from flask_migrate import Migrate, MigrateCommand from app import (create_app, db, commands) -from credstash import getAllSecrets import os default_env_file = '/home/ubuntu/environment' @@ -11,10 +10,6 @@ if os.path.isfile(default_env_file): with open(default_env_file, 'r') as environment_file: environment = environment_file.readline().strip() -# On AWS get secrets and export to env, skip this on Cloud Foundry -if os.getenv('VCAP_SERVICES') is None: - os.environ.update(getAllSecrets(region="eu-west-1")) - from app.config import configs os.environ['NOTIFY_API_ENVIRONMENT'] = configs[environment] diff --git a/wsgi.py b/wsgi.py index 2df2c3976..9fbeb28ac 100644 --- a/wsgi.py +++ b/wsgi.py @@ -1,13 +1,6 @@ -import os - from app import create_app -from credstash import getAllSecrets -# On AWS get secrets and export to env, skip this on Cloud Foundry -if os.getenv('VCAP_SERVICES') is None: - os.environ.update(getAllSecrets(region="eu-west-1")) - application = create_app() if __name__ == "__main__": From b5dc7642aa574b60e172142ad88fd0e42d56528d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 31 Jul 2017 11:12:43 +0100 Subject: [PATCH 15/39] remove aws_run_celery file it's no longer relevant since the switch to PaaS --- aws_run_celery.py | 5 ----- manifest-delivery-base.yml | 14 +++++++------- run_celery.py | 1 + 3 files changed, 8 insertions(+), 12 deletions(-) delete mode 100644 aws_run_celery.py diff --git a/aws_run_celery.py b/aws_run_celery.py deleted file mode 100644 index 8194528db..000000000 --- a/aws_run_celery.py +++ /dev/null @@ -1,5 +0,0 @@ -#!/usr/bin/env python -from app import create_app - -application = create_app("delivery") -application.app_context().push() diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 597df9f3f..6fba774ea 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -16,39 +16,39 @@ memory: 1G applications: - name: notify-delivery-celery-beat - command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery beat --loglevel=INFO + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery beat --loglevel=INFO instances: 1 memory: 128M env: NOTIFY_APP_NAME: delivery-celery-beat - name: notify-delivery-worker-database - command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q database-tasks + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q database-tasks env: NOTIFY_APP_NAME: delivery-worker-database - name: notify-delivery-worker-research - command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=5 -Q research-mode-tasks + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=5 -Q research-mode-tasks env: NOTIFY_APP_NAME: delivery-worker-research - name: notify-delivery-worker-sender - command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q send-tasks,send-sms-tasks,send-email-tasks + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q send-tasks,send-sms-tasks,send-email-tasks env: NOTIFY_APP_NAME: delivery-worker-sender - name: notify-delivery-worker-periodic - command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=2 -Q periodic-tasks,statistics-tasks + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=2 -Q periodic-tasks,statistics-tasks instances: 1 env: NOTIFY_APP_NAME: delivery-worker-periodic - name: notify-delivery-worker-priority - command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=5 -Q priority-tasks + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=5 -Q priority-tasks env: NOTIFY_APP_NAME: delivery-worker-priority - name: notify-delivery-worker - command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q job-tasks,retry-tasks,notify-internal-tasks + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q job-tasks,retry-tasks,notify-internal-tasks env: NOTIFY_APP_NAME: delivery-worker diff --git a/run_celery.py b/run_celery.py index 066735e63..013499615 100644 --- a/run_celery.py +++ b/run_celery.py @@ -1,4 +1,5 @@ #!/usr/bin/env python +# notify_celery is referenced from manifest_delivery_base.yml, and cannot be removed from app import notify_celery, create_app application = create_app('delivery') From 3cb3cf438ec905d7050572d8804310b6b67db3f9 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 31 Jul 2017 13:29:30 +0100 Subject: [PATCH 16/39] remove SEND_COMBINED --- app/config.py | 2 -- manifest-delivery-base.yml | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/config.py b/app/config.py index 5d71c0f41..cfd37fa33 100644 --- a/app/config.py +++ b/app/config.py @@ -22,7 +22,6 @@ class QueueNames(object): PERIODIC = 'periodic-tasks' PRIORITY = 'priority-tasks' DATABASE = 'database-tasks' - SEND_COMBINED = 'send-tasks' SEND_SMS = 'send-sms-tasks' SEND_EMAIL = 'send-email-tasks' RESEARCH_MODE = 'research-mode-tasks' @@ -38,7 +37,6 @@ class QueueNames(object): QueueNames.PRIORITY, QueueNames.PERIODIC, QueueNames.DATABASE, - QueueNames.SEND_COMBINED, QueueNames.SEND_SMS, QueueNames.SEND_EMAIL, QueueNames.RESEARCH_MODE, diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 6fba774ea..78053a729 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -33,7 +33,7 @@ applications: NOTIFY_APP_NAME: delivery-worker-research - name: notify-delivery-worker-sender - command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q send-tasks,send-sms-tasks,send-email-tasks + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q send-sms-tasks,send-email-tasks env: NOTIFY_APP_NAME: delivery-worker-sender From d01d875f7ee2811757f797be483414c7bfb7d7a8 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 31 Jul 2017 17:47:53 +0100 Subject: [PATCH 17/39] Fix issue where monthly billing data was not being updated: --- app/dao/monthly_billing_dao.py | 27 ++++++++---- tests/app/dao/test_monthly_billing.py | 60 +++++++++++++++++++++++++-- 2 files changed, 77 insertions(+), 10 deletions(-) diff --git a/app/dao/monthly_billing_dao.py b/app/dao/monthly_billing_dao.py index 4f4e254c8..4ddacddfa 100644 --- a/app/dao/monthly_billing_dao.py +++ b/app/dao/monthly_billing_dao.py @@ -25,20 +25,33 @@ def create_or_update_monthly_billing_sms(service_id, billing_month): monthly = get_billing_data_for_month(service_id=service_id, start_date=start_date, end_date=end_date) # update monthly monthly_totals = _monthly_billing_data_to_json(monthly) - row = MonthlyBilling.query.filter_by(start_date=start_date, - notification_type='sms').first() + row = get_monthly_billing_entry(service_id, start_date, SMS_TYPE) + if row: row.monthly_totals = monthly_totals row.updated_at = datetime.utcnow() else: - row = MonthlyBilling(service_id=service_id, - notification_type=SMS_TYPE, - monthly_totals=monthly_totals, - start_date=start_date, - end_date=end_date) + row = MonthlyBilling( + service_id=service_id, + notification_type=SMS_TYPE, + monthly_totals=monthly_totals, + start_date=start_date, + end_date=end_date + ) + db.session.add(row) +def get_monthly_billing_entry(service_id, start_date, notification_type): + entry = MonthlyBilling.query.filter_by( + service_id=service_id, + start_date=start_date, + notification_type=notification_type + ).first() + + return entry + + @statsd(namespace="dao") def get_monthly_billing_sms(service_id, billing_month): start_date, end_date = get_month_start_end_date(billing_month) diff --git a/tests/app/dao/test_monthly_billing.py b/tests/app/dao/test_monthly_billing.py index e07588374..e622be569 100644 --- a/tests/app/dao/test_monthly_billing.py +++ b/tests/app/dao/test_monthly_billing.py @@ -1,17 +1,40 @@ -from datetime import datetime - +from datetime import datetime, timedelta from freezegun import freeze_time from freezegun.api import FakeDatetime +from app import db from app.dao.monthly_billing_dao import ( create_or_update_monthly_billing_sms, + get_monthly_billing_entry, get_monthly_billing_sms, get_service_ids_that_need_sms_billing_populated ) -from app.models import MonthlyBilling +from app.models import MonthlyBilling, SMS_TYPE from tests.app.db import create_notification, create_rate, create_service, create_template +def create_sample_monthly_billing_entry( + notify_db, + notify_db_session, + service_id, + notification_type, + monthly_totals, + start_date, + end_date +): + entry = MonthlyBilling( + service_id=service_id, + notification_type=notification_type, + monthly_totals=monthly_totals, + start_date=start_date, + end_date=end_date + ) + db.session.add(entry) + db.session.commit() + + return entry + + def test_add_monthly_billing(sample_template): jan = datetime(2017, 1, 1) feb = datetime(2017, 2, 15) @@ -136,3 +159,34 @@ def test_get_service_id(notify_db_session): end_date=datetime(2017, 7, 16)) expected_services = [service_1.id, service_2.id] assert sorted([x.service_id for x in services]) == sorted(expected_services) + + +def test_get_monthly_billing_entry_filters_by_service(notify_db, notify_db_session): + service_1 = create_service(service_name="Service One") + service_2 = create_service(service_name="Service Two") + now = datetime.utcnow() + + create_sample_monthly_billing_entry( + notify_db, + notify_db_session, + service_1.id, + 'sms', + [], + now, + now + timedelta(days=30) + ) + + create_sample_monthly_billing_entry( + notify_db, + notify_db_session, + service_2.id, + 'sms', + [], + now, + now + timedelta(days=30) + ) + + entry = get_monthly_billing_entry(service_2.id, now, SMS_TYPE) + + assert entry.start_date == now + assert entry.service_id == service_2.id From a08de0939bc55b467620178a3c5762bd86b7a007 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 1 Aug 2017 11:36:30 +0100 Subject: [PATCH 18/39] Adjust command to backfill (less granular) Rates began from 05-2016 This adjusts the command to backfill by year. If 2016, let's backfill from May. If 2017, let's backfill from the beginning of the year. --- app/commands.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/app/commands.py b/app/commands.py index 14e26b887..99eb95608 100644 --- a/app/commands.py +++ b/app/commands.py @@ -153,12 +153,19 @@ class PopulateMonthlyBilling(Command): option_list = ( Option('-s', '-service-id', dest='service_id', help="Service id to populate monthly billing for"), - Option('-m', '-month', dest="month", help="Use for integer value for month, e.g. 7 for July"), Option('-y', '-year', dest="year", help="Use for integer value for year, e.g. 2017") ) - def run(self, service_id, month, year): - print('Starting populating monthly billing') + def run(self, service_id, year): + start, end = 1, 13 + if year == '2016': + start = 6 + + print('Starting populating monthly billing for {}'.format(year)) + for i in range(start, end): + self.populate(service_id, year, i) + + def populate(self, service_id, year, month): create_or_update_monthly_billing_sms(service_id, datetime(int(year), int(month), 1)) results = get_monthly_billing_sms(service_id, datetime(int(year), int(month), 1)) print("Finished populating data for {} for service id {}".format(month, service_id)) From dc7a1051a64ae4a944587895ac866cab7a839288 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 1 Aug 2017 14:04:17 +0100 Subject: [PATCH 19/39] Refactor --- tests/app/dao/test_monthly_billing.py | 38 +++++++++++---------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/tests/app/dao/test_monthly_billing.py b/tests/app/dao/test_monthly_billing.py index e622be569..bb40b7f9f 100644 --- a/tests/app/dao/test_monthly_billing.py +++ b/tests/app/dao/test_monthly_billing.py @@ -14,13 +14,11 @@ from tests.app.db import create_notification, create_rate, create_service, creat def create_sample_monthly_billing_entry( - notify_db, - notify_db_session, service_id, - notification_type, monthly_totals, start_date, - end_date + end_date, + notification_type=SMS_TYPE ): entry = MonthlyBilling( service_id=service_id, @@ -38,8 +36,8 @@ def create_sample_monthly_billing_entry( def test_add_monthly_billing(sample_template): jan = datetime(2017, 1, 1) feb = datetime(2017, 2, 15) - create_rate(start_date=jan, value=0.0158, notification_type='sms') - create_rate(start_date=datetime(2017, 3, 31, 23, 00, 00), value=0.123, notification_type='sms') + create_rate(start_date=jan, value=0.0158, notification_type=SMS_TYPE) + create_rate(start_date=datetime(2017, 3, 31, 23, 00, 00), value=0.123, notification_type=SMS_TYPE) create_notification(template=sample_template, created_at=jan, billable_units=1, status='delivered') create_notification(template=sample_template, created_at=feb, billable_units=2, status='delivered') @@ -74,8 +72,8 @@ def test_add_monthly_billing(sample_template): def test_add_monthly_billing_multiple_rates_in_a_month(sample_template): rate_1 = datetime(2016, 12, 1) rate_2 = datetime(2017, 1, 15) - create_rate(start_date=rate_1, value=0.0158, notification_type='sms') - create_rate(start_date=rate_2, value=0.0124, notification_type='sms') + create_rate(start_date=rate_1, value=0.0158, notification_type=SMS_TYPE) + create_rate(start_date=rate_2, value=0.0124, notification_type=SMS_TYPE) create_notification(template=sample_template, created_at=datetime(2017, 1, 1), billable_units=1, status='delivered') create_notification(template=sample_template, created_at=datetime(2017, 1, 14, 23, 59), billable_units=1, @@ -110,7 +108,7 @@ def test_add_monthly_billing_multiple_rates_in_a_month(sample_template): def test_update_monthly_billing_overwrites_old_totals(sample_template): july = datetime(2017, 7, 1) - create_rate(july, 0.123, 'sms') + create_rate(july, 0.123, SMS_TYPE) create_notification(template=sample_template, created_at=datetime(2017, 7, 2), billable_units=1, status='delivered') with freeze_time('2017-07-20 02:30:00'): create_or_update_monthly_billing_sms(sample_template.service_id, july) @@ -167,23 +165,17 @@ def test_get_monthly_billing_entry_filters_by_service(notify_db, notify_db_sessi now = datetime.utcnow() create_sample_monthly_billing_entry( - notify_db, - notify_db_session, - service_1.id, - 'sms', - [], - now, - now + timedelta(days=30) + service_id=service_1.id, + monthly_totals=[], + start_date=now, + end_date=now + timedelta(days=30) ) create_sample_monthly_billing_entry( - notify_db, - notify_db_session, - service_2.id, - 'sms', - [], - now, - now + timedelta(days=30) + service_id=service_2.id, + monthly_totals=[], + start_date=now, + end_date=now + timedelta(days=30) ) entry = get_monthly_billing_entry(service_2.id, now, SMS_TYPE) From 075d2a334679211c6fb1f91f4b0474754eb7bc7b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 31 Jul 2017 18:28:00 +0100 Subject: [PATCH 20/39] tighten key_type validation on letters api when in research mode or test key, dont send letters via api - instead, just put them straight to success state when using a team key, flat out reject the request (403) --- app/v2/errors.py | 4 +- app/v2/notifications/post_notifications.py | 14 ++++- tests/app/db.py | 10 +++- .../rest/test_send_notification.py | 9 +-- .../test_post_letter_notifications.py | 58 ++++++++++++++++++- 5 files changed, 78 insertions(+), 17 deletions(-) diff --git a/app/v2/errors.py b/app/v2/errors.py index d38477474..0fa6bddac 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -29,10 +29,10 @@ class RateLimitError(InvalidRequest): class BadRequestError(InvalidRequest): - status_code = 400 message = "An error occurred" - def __init__(self, fields=[], message=None): + def __init__(self, fields=[], message=None, status_code=400): + self.status_code = status_code self.fields = fields self.message = message if message else self.message diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index e36ac6c9b..efeca36a9 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -4,8 +4,8 @@ from flask import request, jsonify, current_app, abort from app import api_user, authenticated_service from app.config import QueueNames -from app.models import SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, PRIORITY -from app.celery.tasks import build_dvla_file +from app.models import SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, PRIORITY, KEY_TYPE_TEST, KEY_TYPE_TEAM +from app.celery.tasks import build_dvla_file, update_job_to_sent_to_dvla from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue, @@ -23,6 +23,7 @@ from app.notifications.validators import ( validate_template ) from app.schema_validation import validate +from app.v2.errors import BadRequestError from app.v2.notifications import v2_notification_blueprint from app.v2.notifications.notification_schemas import ( post_sms_request, @@ -145,9 +146,16 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ def process_letter_notification(*, letter_data, api_key, template): + if api_key.key_type == KEY_TYPE_TEAM: + raise BadRequestError(message='Cannot send letters with a team api key', status_code=403) + job = create_letter_api_job(template) notification = create_letter_notification(letter_data, job, api_key) - build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) + if api_key.service.research_mode or api_key.key_type == KEY_TYPE_TEST: + update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) + else: + build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) + current_app.logger.info("send job {} for api notification {} to build-dvla-file in the process-job queue".format( job.id, notification.id diff --git a/tests/app/db.py b/tests/app/db.py index ace7b7c1c..bdb1dfd3f 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -50,7 +50,9 @@ def create_service( service_id=None, restricted=False, service_permissions=[EMAIL_TYPE, SMS_TYPE], - sms_sender='testing' + sms_sender='testing', + research_mode=False, + active=True, ): service = Service( name=service_name, @@ -58,9 +60,13 @@ def create_service( restricted=restricted, email_from=service_name.lower().replace(' ', '.'), created_by=user or create_user(), - sms_sender=sms_sender + sms_sender=sms_sender, ) dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) + + service.active = active + service.research_mode = research_mode + return service diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index bc723341e..73eab744a 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -580,17 +580,12 @@ def test_should_send_email_if_team_api_key_and_a_service_user(client, sample_ema 'to': sample_email_template.service.created_by.email_address, 'template': sample_email_template.id } - api_key = ApiKey(service=sample_email_template.service, - name='team_key', - created_by=sample_email_template.created_by, - key_type=KEY_TYPE_TEAM) - save_model_api_key(api_key) - auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) + auth_header = create_authorization_header(service_id=sample_email_template.service_id, key_type=KEY_TYPE_TEAM) response = client.post( path='/notifications/email', data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + headers=[('Content-Type', 'application/json'), auth_header]) app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with( [fake_uuid], diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 18f36d421..556ef4765 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -6,22 +6,27 @@ import pytest from app.models import EMAIL_TYPE from app.models import Job +from app.models import KEY_TYPE_NORMAL +from app.models import KEY_TYPE_TEAM +from app.models import KEY_TYPE_TEST from app.models import LETTER_TYPE from app.models import Notification from app.models import SMS_TYPE from app.v2.errors import RateLimitError -from app.v2.notifications.post_notifications import process_letter_notification from tests import create_authorization_header from tests.app.db import create_service from tests.app.db import create_template -def letter_request(client, data, service_id, _expected_status=201): +def letter_request(client, data, service_id, key_type=KEY_TYPE_NORMAL, _expected_status=201): resp = client.post( url_for('v2_notifications.post_notification', notification_type='letter'), data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header(service_id=service_id)] + headers=[ + ('Content-Type', 'application/json'), + create_authorization_header(service_id=service_id, key_type=key_type) + ] ) json_resp = json.loads(resp.get_data(as_text=True)) assert resp.status_code == _expected_status, json_resp @@ -170,3 +175,50 @@ def test_post_letter_notification_returns_403_if_not_allowed_to_send_notificatio assert error_json['errors'] == [ {'error': 'BadRequestError', 'message': 'Cannot send letters'} ] + + +@pytest.mark.parametrize('research_mode, key_type', [ + (True, KEY_TYPE_NORMAL), + (False, KEY_TYPE_TEST) +]) +def test_post_letter_notification_doesnt_queue_task( + client, + notify_db_session, + mocker, + research_mode, + key_type +): + real_task = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') + fake_task = mocker.patch('app.celery.tasks.update_job_to_sent_to_dvla.apply_async') + + service = create_service(research_mode=research_mode, service_permissions=[LETTER_TYPE]) + template = create_template(service, template_type=LETTER_TYPE) + + data = { + 'template_id': str(template.id), + 'personalisation': {'address_line_1': 'Foo', 'postcode': 'Bar'} + } + + letter_request(client, data, service_id=service.id, key_type=key_type) + + job = Job.query.one() + assert not real_task.called + fake_task.assert_called_once_with([str(job.id)], queue='research-mode-tasks') + + +def test_post_letter_notification_doesnt_accept_team_key(client, sample_letter_template): + data = { + 'template_id': str(sample_letter_template.id), + 'personalisation': {'address_line_1': 'Foo', 'postcode': 'Bar'} + } + + error_json = letter_request( + client, + data, + sample_letter_template.service_id, + key_type=KEY_TYPE_TEAM, + _expected_status=403 + ) + + assert error_json['status_code'] == 403 + assert error_json['errors'] == [{'error': 'BadRequestError', 'message': 'Cannot send letters with a team api key'}] From 588410e8345a6f69d9c667c54d78ff7e145ba8b0 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 1 Aug 2017 16:06:03 +0100 Subject: [PATCH 21/39] Add celery --- requirements.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements.txt b/requirements.txt index baf0855e8..733413b8c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -19,6 +19,7 @@ gunicorn==19.6.0 docopt==0.6.2 six==1.10.0 iso8601==0.1.12 +celery==3.1.25 # pin to minor version 3.1.x notifications-python-client>=3.1,<3.2 From 1278f15289d46dbd4e1ea91533737c2923951ba8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 1 Aug 2017 16:18:56 +0100 Subject: [PATCH 22/39] Bump utils to 17.7.0 Changes: https://github.com/alphagov/notifications-utils/compare/17.5.7...17.7.0 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 733413b8c..47fa77580 100644 --- a/requirements.txt +++ b/requirements.txt @@ -28,6 +28,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@17.5.7#egg=notifications-utils==17.5.7 +git+https://github.com/alphagov/notifications-utils.git@17.7.0#egg=notifications-utils==17.7.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From 13917c9c578ffd473eaf8d15fd4cf99409c0f954 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 1 Aug 2017 18:23:29 +0100 Subject: [PATCH 23/39] give test letter api notifications a different filename so they can be distinguished on the frontend. Also, some related cleanup: * don't show test api letters on the frontpage * make sure the subject is returned from the API for letters * make sure the letter's address is returned for letters --- app/dao/jobs_dao.py | 16 ++++++++++++--- app/dao/notifications_dao.py | 3 ++- app/models.py | 20 ++++++++++++++++--- .../process_letter_notifications.py | 3 ++- app/v2/notifications/post_notifications.py | 8 ++++++++ app/variables.py | 3 +++ 6 files changed, 45 insertions(+), 8 deletions(-) create mode 100644 app/variables.py diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 81c00633c..fc4ca2ae8 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -11,6 +11,7 @@ from app.models import ( JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING, LETTER_TYPE ) +from app.variables import LETTER_TEST_API_FILENAME from app.statsd_decorators import statsd @@ -142,9 +143,18 @@ def dao_get_jobs_older_than_limited_by(job_types, older_than=7, limit_days=2): def dao_get_all_letter_jobs(): - return db.session.query(Job).join(Job.template).filter( - Template.template_type == LETTER_TYPE - ).order_by(desc(Job.created_at)).all() + return db.session.query( + Job + ).join( + Job.template + ).filter( + Template.template_type == LETTER_TYPE, + # test letter jobs (or from research mode services) are created with a different filename, + # exclude them so we don't see them on the send to CSV + Job.original_file_name != LETTER_TEST_API_FILENAME + ).order_by( + desc(Job.created_at) + ).all() @statsd(namespace="dao") diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 432224653..231ceba59 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -338,7 +338,8 @@ def get_notifications_for_service( filters.append(Notification.created_at < older_than_created_at) if not include_jobs or (key_type and key_type != KEY_TYPE_NORMAL): - filters.append(Notification.job_id.is_(None)) + # we can't say "job_id == None" here, because letters sent via the API still have a job_id :( + filters.append(Notification.api_key_id != None) # noqa if key_type is not None: filters.append(Notification.key_type == key_type) diff --git a/app/models.py b/app/models.py index 3a42a5319..1592557b6 100644 --- a/app/models.py +++ b/app/models.py @@ -891,7 +891,7 @@ class Notification(db.Model): @property def subject(self): from app.utils import get_template_instance - if self.notification_type == EMAIL_TYPE: + if self.notification_type != SMS_TYPE: template_object = get_template_instance(self.template.__dict__, self.personalisation) return template_object.subject @@ -971,10 +971,24 @@ class Notification(db.Model): "created_at": self.created_at.strftime(DATETIME_FORMAT), "sent_at": self.sent_at.strftime(DATETIME_FORMAT) if self.sent_at else None, "completed_at": self.completed_at(), - "scheduled_for": convert_bst_to_utc(self.scheduled_notification.scheduled_for - ).strftime(DATETIME_FORMAT) if self.scheduled_notification else None + "scheduled_for": ( + convert_bst_to_utc( + self.scheduled_notification.scheduled_for + ).strftime(DATETIME_FORMAT) + if self.scheduled_notification + else None + ) } + if self.notification_type == LETTER_TYPE: + serialized['line_1'] = self.personalisation['address_line_1'] + serialized['line_2'] = self.personalisation.get('address_line_2') + serialized['line_3'] = self.personalisation.get('address_line_3') + serialized['line_4'] = self.personalisation.get('address_line_4') + serialized['line_5'] = self.personalisation.get('address_line_5') + serialized['line_6'] = self.personalisation.get('address_line_6') + serialized['postcode'] = self.personalisation['postcode'] + return serialized diff --git a/app/notifications/process_letter_notifications.py b/app/notifications/process_letter_notifications.py index 0cfb35442..332f6ed32 100644 --- a/app/notifications/process_letter_notifications.py +++ b/app/notifications/process_letter_notifications.py @@ -3,6 +3,7 @@ from app.models import LETTER_TYPE, JOB_STATUS_READY_TO_SEND, Job from app.dao.jobs_dao import dao_create_job from app.notifications.process_notifications import persist_notification from app.v2.errors import InvalidRequest +from app.variables import LETTER_API_FILENAME def create_letter_api_job(template): @@ -13,7 +14,7 @@ def create_letter_api_job(template): raise InvalidRequest('Template {} is deleted'.format(template.id), 400) job = Job( - original_file_name='letter submitted via api', + original_file_name=LETTER_API_FILENAME, service=service, template=template, template_version=template.version, diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index efeca36a9..d18bb3b4d 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -4,6 +4,7 @@ from flask import request, jsonify, current_app, abort from app import api_user, authenticated_service from app.config import QueueNames +from app.dao.jobs_dao import dao_update_job from app.models import SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, PRIORITY, KEY_TYPE_TEST, KEY_TYPE_TEAM from app.celery.tasks import build_dvla_file, update_job_to_sent_to_dvla from app.notifications.process_notifications import ( @@ -35,6 +36,7 @@ from app.v2.notifications.create_response import ( create_post_email_response_from_notification, create_post_letter_response_from_notification ) +from app.variables import LETTER_TEST_API_FILENAME @v2_notification_blueprint.route('/', methods=['POST']) @@ -151,7 +153,13 @@ def process_letter_notification(*, letter_data, api_key, template): job = create_letter_api_job(template) notification = create_letter_notification(letter_data, job, api_key) + if api_key.service.research_mode or api_key.key_type == KEY_TYPE_TEST: + + # distinguish real API jobs from test jobs by giving the test jobs a different filename + job.original_file_name = LETTER_TEST_API_FILENAME + dao_update_job(job) + update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) else: build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) diff --git a/app/variables.py b/app/variables.py new file mode 100644 index 000000000..513e30b96 --- /dev/null +++ b/app/variables.py @@ -0,0 +1,3 @@ +# all jobs for letters created via the api must have this filename +LETTER_API_FILENAME = 'letter submitted via api' +LETTER_TEST_API_FILENAME = 'test letter submitted via api' From d16650c849042d0b688aa998ecc88ed8ad144f1c Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Tue, 1 Aug 2017 23:40:13 +0100 Subject: [PATCH 24/39] Update pytest from 3.1.3 to 3.2.0 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 78d2d58ae..501387acd 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,6 +1,6 @@ -r requirements.txt pycodestyle==2.3.1 -pytest==3.1.3 +pytest==3.2.0 pytest-mock==1.6.2 pytest-cov==2.5.1 coveralls==1.1 From eafeeed4f7401276aebcea009978899d88ae21d5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 2 Aug 2017 11:04:20 +0100 Subject: [PATCH 25/39] Stop trying to upgrade us to Celery Pyup --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 47fa77580..92e8b0b0f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -19,7 +19,7 @@ gunicorn==19.6.0 docopt==0.6.2 six==1.10.0 iso8601==0.1.12 -celery==3.1.25 +celery==3.1.25 # pyup: <4 # pin to minor version 3.1.x notifications-python-client>=3.1,<3.2 From 5d61b3644ce881fad2d778e30a54687dda1e90bd Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 2 Aug 2017 11:14:05 +0100 Subject: [PATCH 26/39] add tests for new test-key handling --- app/dao/jobs_dao.py | 2 +- tests/app/dao/test_notification_dao.py | 65 +++++++------------ tests/app/db.py | 23 +++++++ tests/app/letters/test_send_letter_jobs.py | 12 +++- .../test_process_letter_notifications.py | 3 +- tests/app/test_model.py | 55 +++++++++++++--- .../test_post_letter_notifications.py | 4 ++ 7 files changed, 110 insertions(+), 54 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index fc4ca2ae8..e6a80832e 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -150,7 +150,7 @@ def dao_get_all_letter_jobs(): ).filter( Template.template_type == LETTER_TYPE, # test letter jobs (or from research mode services) are created with a different filename, - # exclude them so we don't see them on the send to CSV + # exclude them so we don't see them on the send to CSV Job.original_file_name != LETTER_TEST_API_FILENAME ).order_by( desc(Job.created_at) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 6e50c8718..1eca24544 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -836,13 +836,16 @@ def test_get_notification_by_id(notify_db, notify_db_session, sample_template): assert notification_from_db.scheduled_notification.scheduled_for == datetime(2017, 5, 5, 14, 15) -def test_get_notifications_by_reference(notify_db, notify_db_session, sample_service): +def test_get_notifications_by_reference(sample_template): client_reference = 'some-client-ref' assert len(Notification.query.all()) == 0 - sample_notification(notify_db, notify_db_session, client_reference=client_reference) - sample_notification(notify_db, notify_db_session, client_reference=client_reference) - sample_notification(notify_db, notify_db_session, client_reference='other-ref') - all_notifications = get_notifications_for_service(sample_service.id, client_reference=client_reference).items + create_notification(sample_template, client_reference=client_reference) + create_notification(sample_template, client_reference=client_reference) + create_notification(sample_template, client_reference='other-ref') + all_notifications = get_notifications_for_service( + sample_template.service_id, + client_reference=client_reference + ).items assert len(all_notifications) == 2 @@ -1066,22 +1069,22 @@ def test_should_not_delete_notification_history(notify_db, notify_db_session, sa @freeze_time("2016-01-10") -def test_should_limit_notifications_return_by_day_limit_plus_one(notify_db, notify_db_session, sample_service): +def test_should_limit_notifications_return_by_day_limit_plus_one(sample_template): assert len(Notification.query.all()) == 0 # create one notification a day between 1st and 9th for i in range(1, 11): past_date = '2016-01-{0:02d}'.format(i) with freeze_time(past_date): - sample_notification(notify_db, notify_db_session, created_at=datetime.utcnow(), status="failed") + create_notification(sample_template, created_at=datetime.utcnow(), status="failed") all_notifications = Notification.query.all() assert len(all_notifications) == 10 - all_notifications = get_notifications_for_service(sample_service.id, limit_days=10).items + all_notifications = get_notifications_for_service(sample_template.service_id, limit_days=10).items assert len(all_notifications) == 10 - all_notifications = get_notifications_for_service(sample_service.id, limit_days=1).items + all_notifications = get_notifications_for_service(sample_template.service_id, limit_days=1).items assert len(all_notifications) == 2 @@ -1302,42 +1305,20 @@ def test_dao_timeout_notifications_doesnt_affect_letters(sample_letter_template) assert updated == 0 -def test_should_return_notifications_excluding_jobs_by_default(notify_db, notify_db_session, sample_service): - assert len(Notification.query.all()) == 0 +def test_should_return_notifications_excluding_jobs_by_default(sample_template, sample_job, sample_api_key): + with_job = create_notification(sample_template, job=sample_job) + without_job = create_notification(sample_template, api_key_id=sample_api_key.id) - job = sample_job(notify_db, notify_db_session) - with_job = sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), status="delivered", job=job - ) - without_job = sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), status="delivered" - ) + include_jobs = get_notifications_for_service(sample_template.service_id, include_jobs=True).items + assert len(include_jobs) == 2 - all_notifications = Notification.query.all() - assert len(all_notifications) == 2 + exclude_jobs_by_default = get_notifications_for_service(sample_template.service_id).items + assert len(exclude_jobs_by_default) == 1 + assert exclude_jobs_by_default[0].id == without_job.id - all_notifications = get_notifications_for_service(sample_service.id).items - assert len(all_notifications) == 1 - assert all_notifications[0].id == without_job.id - - -def test_should_return_notifications_including_jobs(notify_db, notify_db_session, sample_service): - assert len(Notification.query.all()) == 0 - - job = sample_job(notify_db, notify_db_session) - with_job = sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), status="delivered", job=job - ) - - all_notifications = Notification.query.all() - assert len(all_notifications) == 1 - - all_notifications = get_notifications_for_service(sample_service.id).items - assert len(all_notifications) == 0 - - all_notifications = get_notifications_for_service(sample_service.id, limit_days=1, include_jobs=True).items - assert len(all_notifications) == 1 - assert all_notifications[0].id == with_job.id + exclude_jobs_manually = get_notifications_for_service(sample_template.service_id, include_jobs=False).items + assert len(exclude_jobs_manually) == 1 + assert exclude_jobs_manually[0].id == without_job.id def test_get_notifications_created_by_api_or_csv_are_returned_correctly_excluding_test_key_notifications( diff --git a/tests/app/db.py b/tests/app/db.py index bdb1dfd3f..19062c4c7 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -5,6 +5,7 @@ from app import db from app.dao.jobs_dao import dao_create_job from app.dao.service_inbound_api_dao import save_service_inbound_api from app.models import ( + ApiKey, Service, User, Template, @@ -120,6 +121,14 @@ def create_notification( sent_at = sent_at or datetime.utcnow() updated_at = updated_at or datetime.utcnow() + if not job and not api_key_id: + # we didn't specify in test - lets create it + existing_api_key = ApiKey.query.filter(ApiKey.service == template.service, ApiKey.key_type == key_type).first() + if existing_api_key: + api_key_id = existing_api_key.id + else: + api_key_id = create_api_key(template.service, key_type=key_type).id + data = { 'id': uuid.uuid4(), 'to': to_field, @@ -253,3 +262,17 @@ def create_rate(start_date, value, notification_type): db.session.add(rate) db.session.commit() return rate + + +def create_api_key(service, key_type=KEY_TYPE_NORMAL): + api_key = ApiKey( + service=service, + name='live api key', + created_by=service.created_by, + key_type=key_type, + id=uuid.uuid4(), + secret=uuid.uuid4() + ) + db.session.add(api_key) + db.session.commit() + return api_key diff --git a/tests/app/letters/test_send_letter_jobs.py b/tests/app/letters/test_send_letter_jobs.py index 1d32baf12..b0d34b987 100644 --- a/tests/app/letters/test_send_letter_jobs.py +++ b/tests/app/letters/test_send_letter_jobs.py @@ -2,6 +2,8 @@ import uuid from flask import json +from app.variables import LETTER_TEST_API_FILENAME + from tests import create_authorization_header @@ -41,7 +43,7 @@ def test_send_letter_jobs_throws_validation_error(client, mocker): assert not mock_celery.called -def test_send_letter_jobs_throws_validation_error(client, sample_letter_job): +def test_get_letter_jobs_excludes_non_letter_jobs(client, sample_letter_job, sample_job): auth_header = create_authorization_header() response = client.get( path='/letter-jobs', @@ -53,3 +55,11 @@ def test_send_letter_jobs_throws_validation_error(client, sample_letter_job): assert json_resp['data'][0]['id'] == str(sample_letter_job.id) assert json_resp['data'][0]['service_name']['name'] == sample_letter_job.service.name assert json_resp['data'][0]['job_status'] == 'pending' + + +def test_get_letter_jobs_excludes_test_jobs(admin_request, sample_letter_job): + sample_letter_job.original_file_name = LETTER_TEST_API_FILENAME + + json_resp = admin_request.get('letter-job.get_letter_jobs') + + assert len(json_resp['data']) == 0 diff --git a/tests/app/notifications/test_process_letter_notifications.py b/tests/app/notifications/test_process_letter_notifications.py index 4a65a487e..8d964b706 100644 --- a/tests/app/notifications/test_process_letter_notifications.py +++ b/tests/app/notifications/test_process_letter_notifications.py @@ -8,6 +8,7 @@ from app.models import Notification from app.notifications.process_letter_notifications import create_letter_api_job from app.notifications.process_letter_notifications import create_letter_notification from app.v2.errors import InvalidRequest +from app.variables import LETTER_API_FILENAME from tests.app.db import create_service from tests.app.db import create_template @@ -37,7 +38,7 @@ def test_create_job_creates_job(sample_letter_template): job = create_letter_api_job(sample_letter_template) assert job == Job.query.one() - assert job.original_file_name == 'letter submitted via api' + assert job.original_file_name == LETTER_API_FILENAME assert job.service == sample_letter_template.service assert job.template_id == sample_letter_template.id assert job.template_version == sample_letter_template.version diff --git a/tests/app/test_model.py b/tests/app/test_model.py index d662c6670..9cefb66df 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -6,6 +6,7 @@ from app import encryption from app.models import ( ServiceWhitelist, Notification, + SMS_TYPE, MOBILE_TYPE, EMAIL_TYPE, NOTIFICATION_CREATED, @@ -18,6 +19,7 @@ from tests.app.conftest import ( sample_template as create_sample_template, sample_notification_with_job as create_sample_notification_with_job ) +from tests.app.db import create_notification @pytest.mark.parametrize('mobile_number', [ @@ -148,21 +150,56 @@ def test_notification_for_csv_returns_bst_correctly(notify_db, notify_db_session assert serialized['created_at'] == 'Monday 27 March 2017 at 00:01' -def test_notification_personalisation_getter_returns_empty_dict_from_None(sample_notification): - sample_notification._personalisation = None - assert sample_notification.personalisation == {} +def test_notification_personalisation_getter_returns_empty_dict_from_None(): + noti = Notification() + noti._personalisation = None + assert noti.personalisation == {} -def test_notification_personalisation_getter_always_returns_empty_dict(sample_notification): - sample_notification._personalisation = encryption.encrypt({}) - assert sample_notification.personalisation == {} +def test_notification_personalisation_getter_always_returns_empty_dict(): + noti = Notification() + noti._personalisation = encryption.encrypt({}) + assert noti.personalisation == {} @pytest.mark.parametrize('input_value', [ None, {} ]) -def test_notification_personalisation_setter_always_sets_empty_dict(sample_notification, input_value): - sample_notification.personalisation = input_value +def test_notification_personalisation_setter_always_sets_empty_dict(input_value): + noti = Notification() + noti.personalisation = input_value - assert sample_notification._personalisation == encryption.encrypt({}) + assert noti._personalisation == encryption.encrypt({}) + + +def test_notification_subject_is_none_for_sms(): + assert Notification(notification_type=SMS_TYPE).subject is None + + +def test_notification_subject_fills_in_placeholders_for_email(sample_email_template_with_placeholders): + noti = create_notification(sample_email_template_with_placeholders, personalisation={'name': 'hello'}) + assert noti.subject == 'hello' + + +def test_notification_subject_fills_in_placeholders_for_letter(sample_letter_template): + sample_letter_template.subject = '((name))' + noti = create_notification(sample_letter_template, personalisation={'name': 'hello'}) + assert noti.subject == 'hello' + + +def test_letter_notification_serializes_with_address(client, sample_letter_notification): + sample_letter_notification.personalisation = { + 'address_line_1': 'foo', + 'address_line_3': 'bar', + 'address_line_5': None, + 'postcode': 'SW1 1AA' + } + res = sample_letter_notification.serialize() + assert res['line_1'] == 'foo' + assert res['line_2'] is None + assert res['line_3'] == 'bar' + assert res['line_4'] is None + assert res['line_5'] is None + assert res['line_6'] is None + assert res['postcode'] == 'SW1 1AA' diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 556ef4765..4751defd0 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -13,6 +13,8 @@ from app.models import LETTER_TYPE from app.models import Notification from app.models import SMS_TYPE from app.v2.errors import RateLimitError +from app.variables import LETTER_TEST_API_FILENAME +from app.variables import LETTER_API_FILENAME from tests import create_authorization_header from tests.app.db import create_service @@ -53,6 +55,7 @@ def test_post_letter_notification_returns_201(client, sample_letter_template, mo resp_json = letter_request(client, data, service_id=sample_letter_template.service_id) job = Job.query.one() + assert job.original_file_name == LETTER_API_FILENAME notification = Notification.query.one() notification_id = notification.id assert resp_json['id'] == str(notification_id) @@ -202,6 +205,7 @@ def test_post_letter_notification_doesnt_queue_task( letter_request(client, data, service_id=service.id, key_type=key_type) job = Job.query.one() + assert job.original_file_name == LETTER_TEST_API_FILENAME assert not real_task.called fake_task.assert_called_once_with([str(job.id)], queue='research-mode-tasks') From 5b9377c69740db8328afe3ef176a1b033c03dd7a Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 1 Aug 2017 11:38:25 +0100 Subject: [PATCH 27/39] Start populating monthly billing on a schedule --- app/commands.py | 2 +- app/config.py | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/app/commands.py b/app/commands.py index 99eb95608..a2e692ebd 100644 --- a/app/commands.py +++ b/app/commands.py @@ -159,7 +159,7 @@ class PopulateMonthlyBilling(Command): def run(self, service_id, year): start, end = 1, 13 if year == '2016': - start = 6 + start = 4 print('Starting populating monthly billing for {}'.format(year)) for i in range(start, end): diff --git a/app/config.py b/app/config.py index cfd37fa33..12d52df9b 100644 --- a/app/config.py +++ b/app/config.py @@ -221,6 +221,11 @@ class Config(object): 'task': 'timeout-job-statistics', 'schedule': crontab(minute=0, hour=5), 'options': {'queue': QueueNames.PERIODIC} + }, + 'populate_monthly_billing': { + 'task': 'populate_monthly_billing', + 'schedule': crontab(minute=10, hour=5), + 'options': {'queue': QueueNames.PERIODIC} } } CELERY_QUEUES = [] From 824063ddb8d20aa75b4e8c62ece7087d1171d731 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 2 Aug 2017 15:24:14 +0100 Subject: [PATCH 28/39] Fix to return billing data before a rate begins --- app/dao/notification_usage_dao.py | 28 ++++++++- tests/app/dao/test_notification_usage_dao.py | 62 ++++++++++++++++++-- 2 files changed, 82 insertions(+), 8 deletions(-) diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index 79f786dd0..cf90fe499 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -22,14 +22,31 @@ def get_yearly_billing_data(service_id, year): start_date, end_date = get_financial_year(year) rates = get_rates_for_daterange(start_date, end_date, SMS_TYPE) + if not rates: + return [] + def get_valid_from(valid_from): return start_date if valid_from < start_date else valid_from result = [] for r, n in zip(rates, rates[1:]): - result.append(sms_yearly_billing_data_query(r.rate, service_id, get_valid_from(r.valid_from), n.valid_from)) + result.append( + sms_yearly_billing_data_query( + r.rate, + service_id, + get_valid_from(r.valid_from), + n.valid_from + ) + ) result.append( - sms_yearly_billing_data_query(rates[-1].rate, service_id, get_valid_from(rates[-1].valid_from), end_date)) + sms_yearly_billing_data_query( + rates[-1].rate, + service_id, + get_valid_from(rates[-1].valid_from), + end_date + ) + ) + result.append(email_yearly_billing_data_query(service_id, start_date, end_date)) return sum(result, []) @@ -38,6 +55,10 @@ def get_yearly_billing_data(service_id, year): @statsd(namespace="dao") def get_billing_data_for_month(service_id, start_date, end_date): rates = get_rates_for_daterange(start_date, end_date, SMS_TYPE) + + if not rates: + return [] + result = [] # so the start end date in the query are the valid from the rate, not the month - this is going to take some thought for r, n in zip(rates, rates[1:]): @@ -54,6 +75,9 @@ def get_monthly_billing_data(service_id, year): start_date, end_date = get_financial_year(year) rates = get_rates_for_daterange(start_date, end_date, SMS_TYPE) + if not rates: + return [] + result = [] for r, n in zip(rates, rates[1:]): result.extend(sms_billing_data_per_month_query(r.rate, service_id, r.valid_from, n.valid_from)) diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py index 9eab4f089..394c71540 100644 --- a/tests/app/dao/test_notification_usage_dao.py +++ b/tests/app/dao/test_notification_usage_dao.py @@ -1,13 +1,15 @@ +import pytest import uuid from datetime import datetime, timedelta +from freezegun import freeze_time -import pytest from flask import current_app from app.dao.date_util import get_financial_year from app.dao.notification_usage_dao import ( get_rates_for_daterange, get_yearly_billing_data, + get_billing_data_for_month, get_monthly_billing_data, get_total_billable_units_for_sent_sms_notifications_in_date_range, discover_rate_bounds_for_billing_query @@ -16,11 +18,16 @@ from app.models import ( Rate, NOTIFICATION_DELIVERED, NOTIFICATION_STATUS_TYPES_BILLABLE, - NOTIFICATION_STATUS_TYPES_NON_BILLABLE) -from tests.app.conftest import sample_notification, sample_email_template, sample_letter_template, sample_service -from tests.app.db import create_notification -from freezegun import freeze_time - + NOTIFICATION_STATUS_TYPES_NON_BILLABLE, + SMS_TYPE, +) +from tests.app.conftest import ( + sample_notification, + sample_email_template, + sample_letter_template, + sample_service +) +from tests.app.db import create_notification, create_rate from tests.conftest import set_config @@ -722,3 +729,46 @@ def test_deducts_free_tier_from_bill_across_rate_boundaries( )[1] == expected_cost finally: current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] = start_value + + +def test_get_yearly_billing_data_for_start_date_before_rate_returns_empty( + sample_template +): + create_rate(datetime(2016, 4, 1), 0.014, SMS_TYPE) + + results = get_yearly_billing_data( + service_id=sample_template.service_id, + year=2015 + ) + + assert not results + + +@freeze_time("2016-05-01") +def test_get_billing_data_for_month_where_start_date_before_rate_returns_empty( + sample_template +): + create_rate(datetime(2016, 4, 1), 0.014, SMS_TYPE) + + results = get_monthly_billing_data( + service_id=sample_template.service_id, + year=2015 + ) + + assert not results + + +@freeze_time("2016-05-01") +def test_get_monthly_billing_data_where_start_date_before_rate_returns_empty( + sample_template +): + now = datetime.utcnow() + create_rate(now, 0.014, SMS_TYPE) + + results = get_billing_data_for_month( + service_id=sample_template.service_id, + start_date=now - timedelta(days=2), + end_date=now - timedelta(days=1) + ) + + assert not results From 372b10f19cfd4d0a9240cb50126e969e2f370d42 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 2 Aug 2017 15:35:56 +0100 Subject: [PATCH 29/39] fix up tests to be internally consistent notifications should always have at least one of job and api key, and the key type should match the api key's key type (or be 'normal') --- app/dao/services_dao.py | 4 +- tests/app/conftest.py | 24 ++- tests/app/dao/test_notification_dao.py | 58 +++--- tests/app/dao/test_services_dao.py | 29 ++- tests/app/db.py | 26 +-- tests/app/notifications/test_rest.py | 233 ++++++++----------------- tests/app/service/test_rest.py | 24 ++- 7 files changed, 151 insertions(+), 247 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 310bd8f32..20099c009 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -207,14 +207,14 @@ def delete_service_and_all_associated_db_objects(service): _delete_commit(ProviderStatistics.query.filter_by(service=service)) _delete_commit(InvitedUser.query.filter_by(service=service)) _delete_commit(Permission.query.filter_by(service=service)) - _delete_commit(ApiKey.query.filter_by(service=service)) - _delete_commit(ApiKey.get_history_model().query.filter_by(service_id=service.id)) _delete_commit(NotificationHistory.query.filter_by(service=service)) _delete_commit(Notification.query.filter_by(service=service)) _delete_commit(Job.query.filter_by(service=service)) _delete_commit(Template.query.filter_by(service=service)) _delete_commit(TemplateHistory.query.filter_by(service_id=service.id)) _delete_commit(ServicePermission.query.filter_by(service_id=service.id)) + _delete_commit(ApiKey.query.filter_by(service=service)) + _delete_commit(ApiKey.get_history_model().query.filter_by(service_id=service.id)) verify_codes = VerifyCode.query.join(User).filter(User.id.in_([x.id for x in service.users])) list(map(db.session.delete, verify_codes)) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index ed1516337..30a9d4210 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -39,7 +39,7 @@ from app.dao.invited_user_dao import save_invited_user from app.dao.provider_rates_dao import create_provider_rates from app.clients.sms.firetext import FiretextClient from tests import create_authorization_header -from tests.app.db import create_user, create_template, create_notification +from tests.app.db import create_user, create_template, create_notification, create_api_key @pytest.yield_fixture @@ -430,7 +430,7 @@ def sample_notification_with_job( sent_at=None, billable_units=1, personalisation=None, - api_key_id=None, + api_key=None, key_type=KEY_TYPE_NORMAL ): if job is None: @@ -449,7 +449,7 @@ def sample_notification_with_job( sent_at=sent_at, billable_units=billable_units, personalisation=personalisation, - api_key_id=api_key_id, + api_key=api_key, key_type=key_type ) @@ -469,7 +469,7 @@ def sample_notification( sent_at=None, billable_units=1, personalisation=None, - api_key_id=None, + api_key=None, key_type=KEY_TYPE_NORMAL, sent_by=None, client_reference=None, @@ -484,6 +484,12 @@ def sample_notification( if template is None: template = sample_template(notify_db, notify_db_session, service=service) + if job is None and api_key is None: + # we didn't specify in test - lets create it + api_key = ApiKey.query.filter(ApiKey.service == template.service, ApiKey.key_type == key_type).first() + if not api_key: + api_key = create_api_key(template.service, key_type=key_type) + notification_id = uuid.uuid4() if to_field: @@ -508,8 +514,9 @@ def sample_notification( 'billable_units': billable_units, 'personalisation': personalisation, 'notification_type': template.template_type, - 'api_key_id': api_key_id, - 'key_type': key_type, + 'api_key': api_key, + 'api_key_id': api_key and api_key.id, + 'key_type': api_key.key_type if api_key else key_type, 'sent_by': sent_by, 'updated_at': created_at if status in NOTIFICATION_STATUS_TYPES_COMPLETED else None, 'client_reference': client_reference, @@ -550,11 +557,12 @@ def sample_letter_notification(sample_letter_template): @pytest.fixture(scope='function') def sample_notification_with_api_key(notify_db, notify_db_session): notification = sample_notification(notify_db, notify_db_session) - notification.api_key_id = sample_api_key( + notification.api_key = sample_api_key( notify_db, notify_db_session, name='Test key' - ).id + ) + notification.api_key_id = notification.api_key.id return notification diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 1eca24544..58680f39d 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -46,7 +46,7 @@ from app.dao.notifications_dao import ( dao_created_scheduled_notification, dao_get_scheduled_notifications, set_scheduled_notification_to_processed) from app.dao.services_dao import dao_update_service -from tests.app.db import create_notification +from tests.app.db import create_notification, create_api_key from tests.app.conftest import ( sample_notification, sample_template, @@ -117,14 +117,14 @@ def test_template_usage_should_ignore_test_keys( notify_db_session, created_at=two_minutes_ago, template=sms, - api_key_id=sample_team_api_key.id, + api_key=sample_team_api_key, key_type=KEY_TYPE_TEAM) sample_notification( notify_db, notify_db_session, created_at=one_minute_ago, template=sms, - api_key_id=sample_test_api_key.id, + api_key=sample_test_api_key, key_type=KEY_TYPE_TEST) results = dao_get_last_template_usage(sms.id) @@ -169,11 +169,11 @@ def test_template_history_should_ignore_test_keys( sms = sample_template(notify_db, notify_db_session) sample_notification( - notify_db, notify_db_session, template=sms, api_key_id=sample_api_key.id, key_type=KEY_TYPE_NORMAL) + notify_db, notify_db_session, template=sms, api_key=sample_api_key, key_type=KEY_TYPE_NORMAL) sample_notification( - notify_db, notify_db_session, template=sms, api_key_id=sample_team_api_key.id, key_type=KEY_TYPE_TEAM) + notify_db, notify_db_session, template=sms, api_key=sample_team_api_key, key_type=KEY_TYPE_TEAM) sample_notification( - notify_db, notify_db_session, template=sms, api_key_id=sample_test_api_key.id, key_type=KEY_TYPE_TEST) + notify_db, notify_db_session, template=sms, api_key=sample_test_api_key, key_type=KEY_TYPE_TEST) sample_notification( notify_db, notify_db_session, template=sms) @@ -1307,7 +1307,7 @@ def test_dao_timeout_notifications_doesnt_affect_letters(sample_letter_template) def test_should_return_notifications_excluding_jobs_by_default(sample_template, sample_job, sample_api_key): with_job = create_notification(sample_template, job=sample_job) - without_job = create_notification(sample_template, api_key_id=sample_api_key.id) + without_job = create_notification(sample_template, api_key=sample_api_key) include_jobs = get_notifications_for_service(sample_template.service_id, include_jobs=True).items assert len(include_jobs) == 2 @@ -1334,15 +1334,15 @@ def test_get_notifications_created_by_api_or_csv_are_returned_correctly_excludin notify_db, notify_db_session, created_at=datetime.utcnow(), job=sample_job ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_api_key, key_type=sample_api_key.key_type ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_team_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_team_api_key, key_type=sample_team_api_key.key_type ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_test_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_test_api_key, key_type=sample_test_api_key.key_type ) @@ -1375,15 +1375,15 @@ def test_get_notifications_with_a_live_api_key_type( notify_db, notify_db_session, created_at=datetime.utcnow(), job=sample_job ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_api_key, key_type=sample_api_key.key_type ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_team_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_team_api_key, key_type=sample_team_api_key.key_type ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_test_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_test_api_key, key_type=sample_test_api_key.key_type ) @@ -1413,15 +1413,15 @@ def test_get_notifications_with_a_test_api_key_type( notify_db, notify_db_session, created_at=datetime.utcnow(), job=sample_job ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_api_key, key_type=sample_api_key.key_type ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_team_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_team_api_key, key_type=sample_team_api_key.key_type ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_test_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_test_api_key, key_type=sample_test_api_key.key_type ) @@ -1448,15 +1448,15 @@ def test_get_notifications_with_a_team_api_key_type( notify_db, notify_db_session, created_at=datetime.utcnow(), job=sample_job ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_api_key, key_type=sample_api_key.key_type ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_team_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_team_api_key, key_type=sample_team_api_key.key_type ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_test_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_test_api_key, key_type=sample_test_api_key.key_type ) @@ -1484,15 +1484,15 @@ def test_should_exclude_test_key_notifications_by_default( ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_api_key, key_type=sample_api_key.key_type ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_team_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_team_api_key, key_type=sample_team_api_key.key_type ) sample_notification( - notify_db, notify_db_session, created_at=datetime.utcnow(), api_key_id=sample_test_api_key.id, + notify_db, notify_db_session, created_at=datetime.utcnow(), api_key=sample_test_api_key, key_type=sample_test_api_key.key_type ) @@ -1765,13 +1765,15 @@ def test_dao_update_notifications_sent_to_dvla(notify_db, notify_db_session, sam assert history.updated_at -def test_dao_update_notifications_sent_to_dvla_does_update_history_if_test_key( - notify_db, notify_db_session, sample_letter_template, sample_api_key): - job = sample_job(notify_db=notify_db, notify_db_session=notify_db_session, template=sample_letter_template) +def test_dao_update_notifications_sent_to_dvla_does_update_history_if_test_key(sample_letter_job): + api_key = create_api_key(sample_letter_job.service, key_type=KEY_TYPE_TEST) notification = create_notification( - template=sample_letter_template, job=job, api_key_id=sample_api_key.id, key_type='test') + sample_letter_job.template, + job=sample_letter_job, + api_key=api_key + ) - updated_count = dao_update_notifications_sent_to_dvla(job_id=job.id, provider='some provider') + updated_count = dao_update_notifications_sent_to_dvla(job_id=sample_letter_job.id, provider='some provider') assert updated_count == 1 updated_notification = Notification.query.get(notification.id) @@ -1779,7 +1781,7 @@ def test_dao_update_notifications_sent_to_dvla_does_update_history_if_test_key( assert updated_notification.sent_by == 'some provider' assert updated_notification.sent_at assert updated_notification.updated_at - assert not NotificationHistory.query.get(notification.id) + assert NotificationHistory.query.count() == 0 def test_dao_get_notifications_by_to_field(sample_template): diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 38ef273cd..ee4f4f912 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -552,11 +552,11 @@ def test_fetch_stats_counts_should_ignore_team_key( sample_team_api_key ): # two created email, one failed email, and one created sms - create_notification(notify_db, notify_db_session, api_key_id=sample_api_key.id, key_type=sample_api_key.key_type) + create_notification(notify_db, notify_db_session, api_key=sample_api_key, key_type=sample_api_key.key_type) create_notification( - notify_db, notify_db_session, api_key_id=sample_test_api_key.id, key_type=sample_test_api_key.key_type) + notify_db, notify_db_session, api_key=sample_test_api_key, key_type=sample_test_api_key.key_type) create_notification( - notify_db, notify_db_session, api_key_id=sample_team_api_key.id, key_type=sample_team_api_key.key_type) + notify_db, notify_db_session, api_key=sample_team_api_key, key_type=sample_team_api_key.key_type) create_notification( notify_db, notify_db_session) @@ -757,24 +757,17 @@ def test_dao_suspend_service_marks_service_as_inactive_and_expires_api_keys(samp ("8", "4", "2")]) # a date range that starts more than 7 days ago def test_fetch_stats_by_date_range_for_all_services_returns_test_notifications(notify_db, notify_db_session, - sample_api_key, start_delta, end_delta, expected): - result_one = create_notification(notify_db, notify_db_session, created_at=datetime.now(), - api_key_id=sample_api_key.id, key_type='test') - create_notification(notify_db, notify_db_session, created_at=datetime.now() - timedelta(days=2), - api_key_id=sample_api_key.id, key_type='test') - create_notification(notify_db, notify_db_session, created_at=datetime.now() - timedelta(days=3), - api_key_id=sample_api_key.id, key_type='test') - create_notification(notify_db, notify_db_session, created_at=datetime.now() - timedelta(days=4), - api_key_id=sample_api_key.id, key_type='normal') - create_notification(notify_db, notify_db_session, created_at=datetime.now() - timedelta(days=4), - api_key_id=sample_api_key.id, key_type='test') - create_notification(notify_db, notify_db_session, created_at=datetime.now() - timedelta(days=8), - api_key_id=sample_api_key.id, key_type='test') - create_notification(notify_db, notify_db_session, created_at=datetime.now() - timedelta(days=8), - api_key_id=sample_api_key.id, key_type='normal') + create_noti = functools.partial(create_notification, notify_db, notify_db_session) + result_one = create_noti(created_at=datetime.now(), key_type='test') + create_noti(created_at=datetime.now() - timedelta(days=2), key_type='test') + create_noti(created_at=datetime.now() - timedelta(days=3), key_type='test') + create_noti(created_at=datetime.now() - timedelta(days=4), key_type='normal') + create_noti(created_at=datetime.now() - timedelta(days=4), key_type='test') + create_noti(created_at=datetime.now() - timedelta(days=8), key_type='test') + create_noti(created_at=datetime.now() - timedelta(days=8), key_type='normal') start_date = (datetime.utcnow() - timedelta(days=int(start_delta))).date() end_date = (datetime.utcnow() - timedelta(days=int(end_delta))).date() diff --git a/tests/app/db.py b/tests/app/db.py index 19062c4c7..71bf29bcb 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -104,7 +104,7 @@ def create_notification( updated_at=None, billable_units=1, personalisation=None, - api_key_id=None, + api_key=None, key_type=KEY_TYPE_NORMAL, sent_by=None, client_reference=None, @@ -121,22 +121,20 @@ def create_notification( sent_at = sent_at or datetime.utcnow() updated_at = updated_at or datetime.utcnow() - if not job and not api_key_id: + if job is None and api_key is None: # we didn't specify in test - lets create it - existing_api_key = ApiKey.query.filter(ApiKey.service == template.service, ApiKey.key_type == key_type).first() - if existing_api_key: - api_key_id = existing_api_key.id - else: - api_key_id = create_api_key(template.service, key_type=key_type).id + api_key = ApiKey.query.filter(ApiKey.service == template.service, ApiKey.key_type == key_type).first() + if not api_key: + api_key = create_api_key(template.service, key_type=key_type) data = { 'id': uuid.uuid4(), 'to': to_field, - 'job_id': job.id if job else None, + 'job_id': job and job.id, 'job': job, 'service_id': template.service.id, 'service': template.service, - 'template_id': template.id if template else None, + 'template_id': template and template.id, 'template': template, 'template_version': template.version, 'status': status, @@ -146,8 +144,9 @@ def create_notification( 'billable_units': billable_units, 'personalisation': personalisation, 'notification_type': template.template_type, - 'api_key_id': api_key_id, - 'key_type': key_type, + 'api_key': api_key, + 'api_key_id': api_key and api_key.id, + 'key_type': api_key.key_type if api_key else key_type, 'sent_by': sent_by, 'updated_at': updated_at, 'client_reference': client_reference, @@ -265,12 +264,13 @@ def create_rate(start_date, value, notification_type): def create_api_key(service, key_type=KEY_TYPE_NORMAL): + id_ = uuid.uuid4() api_key = ApiKey( service=service, - name='live api key', + name='{} api key {}'.format(key_type, id_), created_by=service.created_by, key_type=key_type, - id=uuid.uuid4(), + id=id_, secret=uuid.uuid4() ) db.session.add(api_key) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 811fc1290..6a0b53925 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -9,51 +9,53 @@ from app.dao.notifications_dao import dao_update_notification from app.dao.api_key_dao import save_model_api_key from app.dao.templates_dao import dao_update_template from app.models import ApiKey, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST + from tests import create_authorization_header from tests.app.conftest import sample_notification as create_sample_notification +from tests.app.db import create_notification, create_api_key @pytest.mark.parametrize('type', ('email', 'sms')) def test_get_notification_by_id(client, sample_notification, sample_email_notification, type): - if type == 'email': - notification_to_get = sample_email_notification - if type == 'sms': - notification_to_get = sample_notification + if type == 'email': + notification_to_get = sample_email_notification + if type == 'sms': + notification_to_get = sample_notification - auth_header = create_authorization_header(service_id=notification_to_get.service_id) - response = client.get( - '/notifications/{}'.format(notification_to_get.id), - headers=[auth_header]) + auth_header = create_authorization_header(service_id=notification_to_get.service_id) + response = client.get( + '/notifications/{}'.format(notification_to_get.id), + headers=[auth_header]) - assert response.status_code == 200 - notification = json.loads(response.get_data(as_text=True))['data']['notification'] - assert notification['status'] == 'created' - assert notification['template'] == { - 'id': str(notification_to_get.template.id), - 'name': notification_to_get.template.name, - 'template_type': notification_to_get.template.template_type, - 'version': 1 - } - assert notification['to'] == notification_to_get.to - assert notification['service'] == str(notification_to_get.service_id) - assert notification['body'] == notification_to_get.template.content - assert notification.get('subject', None) == notification_to_get.subject + assert response.status_code == 200 + notification = json.loads(response.get_data(as_text=True))['data']['notification'] + assert notification['status'] == 'created' + assert notification['template'] == { + 'id': str(notification_to_get.template.id), + 'name': notification_to_get.template.name, + 'template_type': notification_to_get.template.template_type, + 'version': 1 + } + assert notification['to'] == notification_to_get.to + assert notification['service'] == str(notification_to_get.service_id) + assert notification['body'] == notification_to_get.template.content + assert notification.get('subject', None) == notification_to_get.subject @pytest.mark.parametrize("id", ["1234-badly-formatted-id-7890", "0"]) @pytest.mark.parametrize('type', ('email', 'sms')) def test_get_notification_by_invalid_id(client, sample_notification, sample_email_notification, id, type): - if type == 'email': - notification_to_get = sample_email_notification - if type == 'sms': - notification_to_get = sample_notification - auth_header = create_authorization_header(service_id=notification_to_get.service_id) + if type == 'email': + notification_to_get = sample_email_notification + if type == 'sms': + notification_to_get = sample_notification + auth_header = create_authorization_header(service_id=notification_to_get.service_id) - response = client.get( - '/notifications/{}'.format(id), - headers=[auth_header]) + response = client.get( + '/notifications/{}'.format(id), + headers=[auth_header]) - assert response.status_code == 405 + assert response.status_code == 405 def test_get_notifications_empty_result(client, sample_api_key): @@ -156,7 +158,7 @@ def test_normal_api_key_returns_notifications_created_from_jobs_and_from_api( api_notification = create_sample_notification( notify_db, notify_db_session, - api_key_id=sample_api_key.id + api_key=sample_api_key ) api_notification.job = None @@ -200,19 +202,19 @@ def test_get_all_notifications_only_returns_notifications_of_matching_type( normal_notification = create_sample_notification( notify_db, notify_db_session, - api_key_id=normal_api_key.id, + api_key=normal_api_key, key_type=KEY_TYPE_NORMAL ) team_notification = create_sample_notification( notify_db, notify_db_session, - api_key_id=team_api_key.id, + api_key=team_api_key, key_type=KEY_TYPE_TEAM ) test_notification = create_sample_notification( notify_db, notify_db_session, - api_key_id=test_api_key.id, + api_key=test_api_key, key_type=KEY_TYPE_TEST ) @@ -236,54 +238,18 @@ def test_get_all_notifications_only_returns_notifications_of_matching_type( @pytest.mark.parametrize('key_type', [KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST]) def test_no_api_keys_return_job_notifications_by_default( client, - notify_db, - notify_db_session, - sample_service, + sample_template, sample_job, key_type ): - team_api_key = ApiKey(service=sample_service, - name='team_api_key', - created_by=sample_service.created_by, - key_type=KEY_TYPE_TEAM) - save_model_api_key(team_api_key) + team_api_key = create_api_key(sample_template.service, KEY_TYPE_TEAM) + normal_api_key = create_api_key(sample_template.service, KEY_TYPE_NORMAL) + test_api_key = create_api_key(sample_template.service, KEY_TYPE_TEST) - normal_api_key = ApiKey(service=sample_service, - name='normal_api_key', - created_by=sample_service.created_by, - key_type=KEY_TYPE_NORMAL) - save_model_api_key(normal_api_key) - - test_api_key = ApiKey(service=sample_service, - name='test_api_key', - created_by=sample_service.created_by, - key_type=KEY_TYPE_TEST) - save_model_api_key(test_api_key) - - job_notification = create_sample_notification( - notify_db, - notify_db_session, - api_key_id=normal_api_key.id, - job=sample_job - ) - normal_notification = create_sample_notification( - notify_db, - notify_db_session, - api_key_id=normal_api_key.id, - key_type=KEY_TYPE_NORMAL - ) - team_notification = create_sample_notification( - notify_db, - notify_db_session, - api_key_id=team_api_key.id, - key_type=KEY_TYPE_TEAM - ) - test_notification = create_sample_notification( - notify_db, - notify_db_session, - api_key_id=test_api_key.id, - key_type=KEY_TYPE_TEST - ) + job_notification = create_notification(sample_template, job=sample_job) + normal_notification = create_notification(sample_template, api_key=normal_api_key) + team_notification = create_notification(sample_template, api_key=team_api_key) + test_notification = create_notification(sample_template, api_key=test_api_key) notification_objs = { KEY_TYPE_NORMAL: normal_notification, @@ -336,25 +302,24 @@ def test_only_normal_api_keys_can_return_job_notifications( job_notification = create_sample_notification( notify_db, notify_db_session, - api_key_id=normal_api_key.id, job=sample_job ) normal_notification = create_sample_notification( notify_db, notify_db_session, - api_key_id=normal_api_key.id, + api_key=normal_api_key, key_type=KEY_TYPE_NORMAL ) team_notification = create_sample_notification( notify_db, notify_db_session, - api_key_id=team_api_key.id, + api_key=team_api_key, key_type=KEY_TYPE_TEAM ) test_notification = create_sample_notification( notify_db, notify_db_session, - api_key_id=test_api_key.id, + api_key=test_api_key, key_type=KEY_TYPE_TEST ) @@ -505,20 +470,10 @@ def test_filter_by_template_type(client, notify_db, notify_db_session, sample_te def test_filter_by_multiple_template_types(client, - notify_db, - notify_db_session, sample_template, sample_email_template): - notification_1 = create_sample_notification( - notify_db, - notify_db_session, - service=sample_email_template.service, - template=sample_template) - notification_2 = create_sample_notification( - notify_db, - notify_db_session, - service=sample_email_template.service, - template=sample_email_template) + notification_1 = create_notification(sample_template) + notification_2 = create_notification(sample_email_template) auth_header = create_authorization_header(service_id=sample_email_template.service_id) @@ -529,23 +484,12 @@ def test_filter_by_multiple_template_types(client, assert response.status_code == 200 notifications = json.loads(response.get_data(as_text=True)) assert len(notifications['notifications']) == 2 - set(['sms', 'email']) == set( - [x['template']['template_type'] for x in notifications['notifications']]) + assert {'sms', 'email'} == set(x['template']['template_type'] for x in notifications['notifications']) -def test_filter_by_status(client, notify_db, notify_db_session, sample_email_template): - notification_1 = create_sample_notification( - notify_db, - notify_db_session, - service=sample_email_template.service, - template=sample_email_template, - status="delivered") - - notification_2 = create_sample_notification( - notify_db, - notify_db_session, - service=sample_email_template.service, - template=sample_email_template) +def test_filter_by_status(client, sample_email_template): + notification_1 = create_notification(sample_email_template, status="delivered") + notification_2 = create_notification(sample_email_template) auth_header = create_authorization_header(service_id=sample_email_template.service_id) @@ -559,58 +503,27 @@ def test_filter_by_status(client, notify_db, notify_db_session, sample_email_tem assert response.status_code == 200 -def test_filter_by_multiple_statuss(client, - notify_db, - notify_db_session, - sample_email_template): - notification_1 = create_sample_notification( - notify_db, - notify_db_session, - service=sample_email_template.service, - template=sample_email_template, - status="delivered") - - notification_2 = create_sample_notification( - notify_db, - notify_db_session, - service=sample_email_template.service, - template=sample_email_template, - status='sending') +def test_filter_by_multiple_statuses(client, sample_email_template): + notification_1 = create_notification(sample_email_template, status="delivered") + notification_2 = create_notification(sample_email_template, status='sending') auth_header = create_authorization_header(service_id=sample_email_template.service_id) response = client.get( '/notifications?status=delivered&status=sending', - headers=[auth_header]) + headers=[auth_header] + ) assert response.status_code == 200 notifications = json.loads(response.get_data(as_text=True)) assert len(notifications['notifications']) == 2 - set(['delivered', 'sending']) == set( - [x['status'] for x in notifications['notifications']]) + assert {'delivered', 'sending'} == set(x['status'] for x in notifications['notifications']) -def test_filter_by_status_and_template_type(client, - notify_db, - notify_db_session, - sample_template, - sample_email_template): - notification_1 = create_sample_notification( - notify_db, - notify_db_session, - service=sample_email_template.service, - template=sample_template) - notification_2 = create_sample_notification( - notify_db, - notify_db_session, - service=sample_email_template.service, - template=sample_email_template) - notification_3 = create_sample_notification( - notify_db, - notify_db_session, - service=sample_email_template.service, - template=sample_email_template, - status="delivered") +def test_filter_by_status_and_template_type(client, sample_template, sample_email_template): + notification_1 = create_notification(sample_template) + notification_2 = create_notification(sample_email_template) + notification_3 = create_notification(sample_email_template, status="delivered") auth_header = create_authorization_header(service_id=sample_email_template.service_id) @@ -625,15 +538,9 @@ def test_filter_by_status_and_template_type(client, assert notifications['notifications'][0]['status'] == 'delivered' -def test_get_notification_by_id_returns_merged_template_content(notify_db, - notify_db_session, - client, - sample_template_with_placeholders): +def test_get_notification_by_id_returns_merged_template_content(client, sample_template_with_placeholders): - sample_notification = create_sample_notification(notify_db, - notify_db_session, - template=sample_template_with_placeholders, - personalisation={"name": "world"}) + sample_notification = create_notification(sample_template_with_placeholders, personalisation={"name": "world"}) auth_header = create_authorization_header(service_id=sample_notification.service_id) @@ -649,15 +556,13 @@ def test_get_notification_by_id_returns_merged_template_content(notify_db, def test_get_notification_by_id_returns_merged_template_content_for_email( - notify_db, - notify_db_session, client, sample_email_template_with_placeholders ): - sample_notification = create_sample_notification(notify_db, - notify_db_session, - template=sample_email_template_with_placeholders, - personalisation={"name": "world"}) + sample_notification = create_notification( + sample_email_template_with_placeholders, + personalisation={"name": "world"} + ) auth_header = create_authorization_header(service_id=sample_notification.service_id) response = client.get( diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 7b0478916..9df09cfa1 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1362,24 +1362,20 @@ def test_get_all_notifications_for_service_including_ones_made_by_jobs( def test_get_only_api_created_notifications_for_service( - client, - notify_db, - notify_db_session, - sample_service + admin_request, + sample_job, + sample_template ): - with_job = sample_notification_with_job(notify_db, notify_db_session, service=sample_service) - without_job = create_sample_notification(notify_db, notify_db_session, service=sample_service) + with_job = create_notification(sample_template, job=sample_job) + without_job = create_notification(sample_template) - auth_header = create_authorization_header() - - response = client.get( - path='/service/{}/notifications?include_jobs=false'.format(sample_service.id), - headers=[auth_header]) - - resp = json.loads(response.get_data(as_text=True)) + resp = admin_request.get( + 'service.get_all_notifications_for_service', + service_id=sample_template.service_id, + include_jobs=False + ) assert len(resp['notifications']) == 1 assert resp['notifications'][0]['id'] == str(without_job.id) - assert response.status_code == 200 def test_set_sms_sender_for_service(client, sample_service): From 3c050af19037d58f7da7889b3c5bbb8323e2b5f9 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 3 Aug 2017 14:52:13 +0100 Subject: [PATCH 30/39] Bump version to pick up fix for dashes --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 47fa77580..5d7dbdce3 100644 --- a/requirements.txt +++ b/requirements.txt @@ -28,6 +28,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@17.7.0#egg=notifications-utils==17.7.0 +git+https://github.com/alphagov/notifications-utils.git@17.8.0#egg=notifications-utils==17.8.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From f5e38a896c665f0dd822f130647e934d5eb32d21 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 26 Jul 2017 12:09:13 +0100 Subject: [PATCH 31/39] Update the last template usage query to check Notification table: * Don't check the NotificationHistory table (this can cause a timeout) * Check template actually exists first --- app/dao/notifications_dao.py | 8 ++++---- app/template_statistics/rest.py | 19 +++++++++++++----- tests/app/template_statistics/test_rest.py | 23 +++++++++++++++++++--- 3 files changed, 38 insertions(+), 12 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 231ceba59..c3333fc5a 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -149,11 +149,11 @@ def dao_get_template_usage(service_id, limit_days=None): @statsd(namespace="dao") def dao_get_last_template_usage(template_id): - return NotificationHistory.query.filter( - NotificationHistory.template_id == template_id, - NotificationHistory.key_type != KEY_TYPE_TEST + return Notification.query.filter( + Notification.template_id == template_id, + Notification.key_type != KEY_TYPE_TEST ).order_by( - desc(NotificationHistory.created_at) + desc(Notification.created_at) ).first() diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 16cd17672..78ae711e4 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -7,8 +7,12 @@ from flask import ( from app import redis_store from app.dao.notifications_dao import ( dao_get_template_usage, - dao_get_last_template_usage) -from app.dao.templates_dao import dao_get_templates_for_cache + dao_get_last_template_usage +) +from app.dao.templates_dao import ( + dao_get_templates_for_cache, + dao_get_template_by_id_and_service_id +) from app.schemas import notification_with_template_schema from app.utils import cache_key_for_service_template_counter @@ -52,12 +56,17 @@ def get_template_statistics_for_service_by_day(service_id): @template_statistics.route('/') def get_template_statistics_for_template_id(service_id, template_id): - notification = dao_get_last_template_usage(template_id) - if not notification: + template = dao_get_template_by_id_and_service_id(template_id, service_id) + if not template: message = 'No template found for id {}'.format(template_id) errors = {'template_id': [message]} raise InvalidRequest(errors, status_code=404) - data = notification_with_template_schema.dump(notification).data + + data = None + notification = dao_get_last_template_usage(template_id) + if notification: + data = notification_with_template_schema.dump(notification).data + return jsonify(data=data) diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index 4f79541ec..f1d2ef078 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -211,8 +211,8 @@ def test_get_template_statistics_by_id_returns_last_notification( def test_get_template_statistics_for_template_returns_empty_if_no_statistics( - client, - sample_template, + client, + sample_template, ): auth_header = create_authorization_header() @@ -221,7 +221,24 @@ def test_get_template_statistics_for_template_returns_empty_if_no_statistics( headers=[('Content-Type', 'application/json'), auth_header], ) + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['data'] == {} + + +def test_get_template_statistics_raises_error_for_nonexistent_template( + client, + sample_service, + fake_uuid +): + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/{}'.format(sample_service.id, fake_uuid), + headers=[('Content-Type', 'application/json'), auth_header], + ) + assert response.status_code == 404 json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['message'] == 'No result found' assert json_resp['result'] == 'error' - assert json_resp['message']['template_id'] == ['No template found for id {}'.format(sample_template.id)] From 864e356163041731a8e41215d2e7ff2a6a712b61 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 7 Aug 2017 10:23:20 +0100 Subject: [PATCH 32/39] Add test to check with notificationhistory but no notification --- tests/app/template_statistics/test_rest.py | 29 ++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index f1d2ef078..a13993768 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -5,7 +5,12 @@ import pytest from freezegun import freeze_time from tests import create_authorization_header -from tests.app.conftest import (sample_template as create_sample_template, sample_notification, sample_email_template) +from tests.app.conftest import ( + sample_template as create_sample_template, + sample_notification, + sample_notification_history, + sample_email_template +) def test_get_all_template_statistics_with_bad_arg_returns_400(client, sample_service): @@ -223,7 +228,7 @@ def test_get_template_statistics_for_template_returns_empty_if_no_statistics( assert response.status_code == 200 json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['data'] == {} + assert not json_resp['data'] def test_get_template_statistics_raises_error_for_nonexistent_template( @@ -242,3 +247,23 @@ def test_get_template_statistics_raises_error_for_nonexistent_template( json_resp = json.loads(response.get_data(as_text=True)) assert json_resp['message'] == 'No result found' assert json_resp['result'] == 'error' + + +def test_get_template_statistics_by_id_returns_empty_for_old_notification( + notify_db, + notify_db_session, + client, + sample_template +): + sample_notification_history(notify_db, notify_db_session, sample_template) + + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/{}'.format(sample_template.service.id, sample_template.id), + headers=[('Content-Type', 'application/json'), auth_header], + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True))['data'] + assert not json_resp From 5491adcabd113b100a79dff6d4c5ad8e54daacb3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 7 Aug 2017 14:17:01 +0100 Subject: [PATCH 33/39] address_line_2 is a required field --- app/schema_validation/definitions.py | 2 +- .../test_post_letter_notifications.py | 24 ++++++++++++------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/app/schema_validation/definitions.py b/app/schema_validation/definitions.py index 0bf163999..f0dbb0a58 100644 --- a/app/schema_validation/definitions.py +++ b/app/schema_validation/definitions.py @@ -19,7 +19,7 @@ personalisation = { } -letter_personalisation = dict(personalisation, required=["address_line_1", "postcode"]) +letter_personalisation = dict(personalisation, required=["address_line_1", "address_line_2", "postcode"]) https_url = { diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 4751defd0..429e53eb8 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -82,7 +82,7 @@ def test_post_letter_notification_returns_400_and_missing_template( ): data = { 'template_id': str(uuid.uuid4()), - 'personalisation': {'address_line_1': '', 'postcode': ''} + 'personalisation': {'address_line_1': '', 'address_line_2': '', 'postcode': ''} } error_json = letter_request(client, data, service_id=sample_service_full_permissions.id, _expected_status=400) @@ -91,12 +91,12 @@ def test_post_letter_notification_returns_400_and_missing_template( assert error_json['errors'] == [{'error': 'BadRequestError', 'message': 'Template not found'}] -def test_notification_returns_400_for_schema_problems( +def test_notification_returns_400_for_missing_template_field( client, sample_service_full_permissions ): data = { - 'personalisation': {'address_line_1': '', 'postcode': ''} + 'personalisation': {'address_line_1': '', 'address_line_2': '', 'postcode': ''} } error_json = letter_request(client, data, service_id=sample_service_full_permissions.id, _expected_status=400) @@ -116,6 +116,7 @@ def test_notification_returns_400_if_address_doesnt_have_underscores( 'template_id': str(sample_letter_template.id), 'personalisation': { 'address line 1': 'Her Royal Highness Queen Elizabeth II', + 'address-line-2': 'Buckingham Palace', 'postcode': 'SW1 1AA', } } @@ -123,10 +124,15 @@ def test_notification_returns_400_if_address_doesnt_have_underscores( error_json = letter_request(client, data, service_id=sample_letter_template.service_id, _expected_status=400) assert error_json['status_code'] == 400 - assert error_json['errors'] == [{ + assert len(error_json['errors']) == 2 + assert { 'error': 'ValidationError', 'message': 'personalisation address_line_1 is a required property' - }] + } in error_json['errors'] + assert { + 'error': 'ValidationError', + 'message': 'personalisation address_line_2 is a required property' + } in error_json['errors'] def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( @@ -142,7 +148,7 @@ def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( data = { 'template_id': str(sample_letter_template.id), - 'personalisation': {'address_line_1': '', 'postcode': ''} + 'personalisation': {'address_line_1': '', 'address_line_2': '', 'postcode': ''} } error_json = letter_request(client, data, service_id=sample_letter_template.service_id, _expected_status=429) @@ -170,7 +176,7 @@ def test_post_letter_notification_returns_403_if_not_allowed_to_send_notificatio data = { 'template_id': str(template.id), - 'personalisation': {'address_line_1': '', 'postcode': ''} + 'personalisation': {'address_line_1': '', 'address_line_2': '', 'postcode': ''} } error_json = letter_request(client, data, service_id=service.id, _expected_status=400) @@ -199,7 +205,7 @@ def test_post_letter_notification_doesnt_queue_task( data = { 'template_id': str(template.id), - 'personalisation': {'address_line_1': 'Foo', 'postcode': 'Bar'} + 'personalisation': {'address_line_1': 'Foo', 'address_line_2': 'Bar', 'postcode': 'Baz'} } letter_request(client, data, service_id=service.id, key_type=key_type) @@ -213,7 +219,7 @@ def test_post_letter_notification_doesnt_queue_task( def test_post_letter_notification_doesnt_accept_team_key(client, sample_letter_template): data = { 'template_id': str(sample_letter_template.id), - 'personalisation': {'address_line_1': 'Foo', 'postcode': 'Bar'} + 'personalisation': {'address_line_1': 'Foo', 'address_line_2': 'Bar', 'postcode': 'Baz'} } error_json = letter_request( From c9762f75a18499da813e6e9480c6b60bb28464a1 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 9 Aug 2017 11:56:52 +0100 Subject: [PATCH 34/39] make sure template serialises letter subjects --- app/models.py | 2 +- tests/app/test_model.py | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index 1592557b6..4effde3c6 100644 --- a/app/models.py +++ b/app/models.py @@ -464,7 +464,7 @@ class Template(db.Model): "created_by": self.created_by.email_address, "version": self.version, "body": self.content, - "subject": self.subject if self.template_type == EMAIL_TYPE else None + "subject": self.subject if self.template_type != SMS_TYPE else None } return serialized diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 9cefb66df..d38e641ab 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -203,3 +203,18 @@ def test_letter_notification_serializes_with_address(client, sample_letter_notif assert res['line_5'] is None assert res['line_6'] is None assert res['postcode'] == 'SW1 1AA' + + +def test_sms_notification_serializes_without_subject(client, sample_template): + res = sample_template.serialize() + assert res['subject'] is None + + +def test_email_notification_serializes_with_subject(client, sample_email_template): + res = sample_email_template.serialize() + assert res['subject'] == 'Email Subject' + + +def test_letter_notification_serializes_with_subject(client, sample_letter_template): + res = sample_letter_template.serialize() + assert res['subject'] == 'Template Subject' From da02ffa32fe8b7f5deb4172ef9e99a067830c1df Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 9 Aug 2017 12:24:35 +0100 Subject: [PATCH 35/39] ensure template history serializes using template serialize fn --- app/models.py | 13 +------------ app/v2/template/get_template.py | 1 - tests/app/test_model.py | 2 +- tests/app/v2/template/test_get_template.py | 12 ++++++++---- 4 files changed, 10 insertions(+), 18 deletions(-) diff --git a/app/models.py b/app/models.py index 4effde3c6..4ab486276 100644 --- a/app/models.py +++ b/app/models.py @@ -506,18 +506,7 @@ class TemplateHistory(db.Model): default=NORMAL) def serialize(self): - serialized = { - "id": self.id, - "type": self.template_type, - "created_at": self.created_at.strftime(DATETIME_FORMAT), - "updated_at": self.updated_at.strftime(DATETIME_FORMAT) if self.updated_at else None, - "created_by": self.created_by.email_address, - "version": self.version, - "body": self.content, - "subject": self.subject if self.template_type == EMAIL_TYPE else None - } - - return serialized + return Template.serialize(self) MMG_PROVIDER = "mmg" diff --git a/app/v2/template/get_template.py b/app/v2/template/get_template.py index 4054e161a..8440b231d 100644 --- a/app/v2/template/get_template.py +++ b/app/v2/template/get_template.py @@ -18,5 +18,4 @@ def get_template_by_id(template_id, version=None): template = templates_dao.dao_get_template_by_id_and_service_id( template_id, authenticated_service.id, data.get('version')) - return jsonify(template.serialize()), 200 diff --git a/tests/app/test_model.py b/tests/app/test_model.py index d38e641ab..66b96ba4e 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -217,4 +217,4 @@ def test_email_notification_serializes_with_subject(client, sample_email_templat def test_letter_notification_serializes_with_subject(client, sample_letter_template): res = sample_letter_template.serialize() - assert res['subject'] == 'Template Subject' + assert res['subject'] == 'Template subject' diff --git a/tests/app/v2/template/test_get_template.py b/tests/app/v2/template/test_get_template.py index 0f028a967..5de2b2814 100644 --- a/tests/app/v2/template/test_get_template.py +++ b/tests/app/v2/template/test_get_template.py @@ -3,16 +3,20 @@ import pytest from flask import json from app import DATETIME_FORMAT -from app.models import EMAIL_TYPE, TEMPLATE_TYPES +from app.models import TEMPLATE_TYPES, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE from tests import create_authorization_header from tests.app.db import create_template valid_version_params = [None, 1] -@pytest.mark.parametrize("tmp_type", TEMPLATE_TYPES) +@pytest.mark.parametrize("tmp_type, expected_subject", [ + (SMS_TYPE, None), + (EMAIL_TYPE, 'Template subject'), + (LETTER_TYPE, 'Template subject') +]) @pytest.mark.parametrize("version", valid_version_params) -def test_get_template_by_id_returns_200(client, sample_service, tmp_type, version): +def test_get_template_by_id_returns_200(client, sample_service, tmp_type, expected_subject, version): template = create_template(sample_service, template_type=tmp_type) auth_header = create_authorization_header(service_id=sample_service.id) @@ -34,7 +38,7 @@ def test_get_template_by_id_returns_200(client, sample_service, tmp_type, versio 'version': template.version, 'created_by': template.created_by.email_address, 'body': template.content, - "subject": template.subject if tmp_type == EMAIL_TYPE else None + "subject": expected_subject } assert json_response == expected_response From 1ec6a3b73d21e8a3d4977fe4106050eaf6b66b63 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 9 Aug 2017 15:12:52 +0100 Subject: [PATCH 36/39] Created a new service endpoint that checks if the service name or email_from is unique. Why is this needed? When a user updates a service name they enter the new name in a form, are then asked to confirm the change by entering their password. Then the API call to update_service is called. If we let the update serivce API call fail with the integrity constraint it will be ackward for the user. --- app/service/rest.py | 24 ++++++++++++++++++++ tests/app/service/test_rest.py | 40 ++++++++++++++++++++++++++++------ 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index deb37f525..6af9a1f0e 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -596,3 +596,27 @@ def handle_sql_errror(e): def create_one_off_notification(service_id): resp = send_one_off_notification(service_id, request.get_json()) return jsonify(resp), 201 + + +@service_blueprint.route('/unique', methods=["GET"]) +def is_service_name_unique(): + name, email_from = check_request_args(request) + + name_exists = Service.query.filter_by(name=name).first() + email_from_exists = Service.query.filter_by(email_from=email_from).first() + if name_exists or email_from_exists: + return jsonify(result=False), 200 + return jsonify(result=True), 200 + + +def check_request_args(request): + name = request.args.get('name', None) + email_from = request.args.get('email_from', None) + errors = [] + if not name: + errors.append({'name': ["Can't be empty"]}) + if not email_from: + errors.append({'email_from': ["Can't be empty"]}) + if errors: + raise InvalidRequest(errors, status_code=400) + return name, email_from diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 9df09cfa1..77a3a9d40 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1,26 +1,23 @@ -from datetime import datetime, timedelta, date -from functools import partial import json import uuid +from datetime import datetime, timedelta, date +from functools import partial from unittest.mock import ANY import pytest from flask import url_for, current_app from freezegun import freeze_time -from app import encryption -from app.dao.users_dao import save_model_user 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, Rate, Service, ServicePermission, Notification, 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, ) - from tests import create_authorization_header -from tests.app.db import create_template, create_service_inbound_api, create_user, create_notification from tests.app.conftest import ( sample_service as create_service, sample_user_service_permission as create_user_service_permission, @@ -28,8 +25,8 @@ from tests.app.conftest import ( sample_notification_history as create_notification_history, sample_notification_with_job ) +from tests.app.db import create_template, create_service_inbound_api, create_notification from tests.app.db import create_user -from tests.conftest import set_config_values def test_get_service_list(client, service_factory): @@ -2311,3 +2308,32 @@ def test_search_for_notification_by_to_field_returns_personlisation( assert len(notifications) == 1 assert 'personalisation' in notifications[0].keys() assert notifications[0]['personalisation']['name'] == 'Foo' + + +def test_is_service_name_unique_returns_200_if_unique(client): + response = client.get('/service/unique?name=something&email_from=something', + headers=[create_authorization_header()]) + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True)) == {"result": True} + + +@pytest.mark.parametrize('name, email_from', + [("something unique", "something"), + ("unique", "something.unique"), + ("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') + response = client.get('/service/unique?name={}&email_from={}'.format(name, email_from), + headers=[create_authorization_header()]) + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True)) == {"result": False} + + +def test_is_service_name_unique_returns_400_when_name_does_not_exist(client): + response = client.get('/service/unique', headers=[create_authorization_header()]) + assert response.status_code == 400 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp["message"][0]["name"] == ["Can't be empty"] + assert json_resp["message"][1]["email_from"] == ["Can't be empty"] From e7b13e727a976e3a3cc4bbc30164fb1247cb1407 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 31 Jul 2017 13:28:34 +0100 Subject: [PATCH 37/39] don't capture logs directly from stdout previously in run_app_paas.sh, we captured stdout from the app and piped that into the log file. However, this came up with a bunch of problems, mainly: * exceptions with stack traces often weren't formatted properly, and kibana could not parse them * celery logs were duplicated - we'd collect both the json logs and the human readable stdout logs. instead, with the updated utils library, we can use that to log json straight to the appropriate directory directly. --- app/cloudfoundry_config.py | 2 +- app/config.py | 4 ++-- requirements.txt | 2 +- scripts/run_app_paas.sh | 9 +++------ tests/app/test_cloudfoundry_config.py | 2 +- tests/app/test_config.py | 21 --------------------- 6 files changed, 8 insertions(+), 32 deletions(-) diff --git a/app/cloudfoundry_config.py b/app/cloudfoundry_config.py index 293f0cd2c..c890a8333 100644 --- a/app/cloudfoundry_config.py +++ b/app/cloudfoundry_config.py @@ -18,7 +18,7 @@ def set_config_env_vars(vcap_services): vcap_application = json.loads(os.environ['VCAP_APPLICATION']) os.environ['NOTIFY_ENVIRONMENT'] = vcap_application['space_name'] - os.environ['LOGGING_STDOUT_JSON'] = '1' + os.environ['NOTIFY_LOG_PATH'] = '/home/vcap/logs/app.log' # Notify common config for s in vcap_services['user-provided']: diff --git a/app/config.py b/app/config.py index 12d52df9b..c05b96660 100644 --- a/app/config.py +++ b/app/config.py @@ -95,7 +95,6 @@ class Config(object): # Logging DEBUG = False - LOGGING_STDOUT_JSON = os.getenv('LOGGING_STDOUT_JSON') == '1' ########################### # Default config values ### @@ -106,7 +105,7 @@ class Config(object): AWS_REGION = 'eu-west-1' INVITATION_EXPIRATION_DAYS = 2 NOTIFY_APP_NAME = 'api' - NOTIFY_LOG_PATH = '/var/log/notify/application.log' + NOTIFY_LOG_PATH = None SQLALCHEMY_COMMIT_ON_TEARDOWN = False SQLALCHEMY_RECORD_QUERIES = True SQLALCHEMY_TRACK_MODIFICATIONS = True @@ -277,6 +276,7 @@ class Config(object): ###################### class Development(Config): + NOTIFY_LOG_PATH = 'application.log' SQLALCHEMY_ECHO = False NOTIFY_EMAIL_DOMAIN = 'notify.tools' CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' diff --git a/requirements.txt b/requirements.txt index 5b011f57c..6c2d839b5 100644 --- a/requirements.txt +++ b/requirements.txt @@ -28,6 +28,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@17.8.0#egg=notifications-utils==17.8.0 +git+https://github.com/alphagov/notifications-utils.git@18.0.0#egg=notifications-utils==18.0.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/scripts/run_app_paas.sh b/scripts/run_app_paas.sh index 2ac52c388..2112e4789 100755 --- a/scripts/run_app_paas.sh +++ b/scripts/run_app_paas.sh @@ -52,11 +52,9 @@ function on_exit { kill 0 } -function start_appplication { - exec "$@" 2>&1 | while read line; do echo $line; echo $line >> /home/vcap/logs/app.log.`date +%Y-%m-%d`; done & - LOGGER_PID=$! +function start_application { + exec "$@" & APP_PID=`jobs -p` - echo "Logger process pid: ${LOGGER_PID}" echo "Application process pid: ${APP_PID}" } @@ -69,7 +67,6 @@ function start_aws_logs_agent { function run { while true; do kill -0 ${APP_PID} 2&>/dev/null || break - kill -0 ${LOGGER_PID} 2&>/dev/null || break kill -0 ${AWSLOGS_AGENT_PID} 2&>/dev/null || start_aws_logs_agent sleep 1 done @@ -84,7 +81,7 @@ trap "on_exit" EXIT configure_aws_logs # The application has to start first! -start_appplication "$@" +start_application "$@" start_aws_logs_agent diff --git a/tests/app/test_cloudfoundry_config.py b/tests/app/test_cloudfoundry_config.py index 15ff3dd55..a393fc8bb 100644 --- a/tests/app/test_cloudfoundry_config.py +++ b/tests/app/test_cloudfoundry_config.py @@ -122,8 +122,8 @@ def test_extract_cloudfoundry_config_populates_other_vars(): extract_cloudfoundry_config() assert os.environ['SQLALCHEMY_DATABASE_URI'] == 'postgres uri' - assert os.environ['LOGGING_STDOUT_JSON'] == '1' assert os.environ['NOTIFY_ENVIRONMENT'] == '🚀🌌' + assert os.environ['NOTIFY_LOG_PATH'] == '/home/vcap/logs/app.log' @pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') diff --git a/tests/app/test_config.py b/tests/app/test_config.py index 63a9bff9a..c1e1cbeee 100644 --- a/tests/app/test_config.py +++ b/tests/app/test_config.py @@ -57,24 +57,3 @@ def test_load_config_if_cloudfoundry_not_available(monkeypatch, reload_config): def test_cloudfoundry_config_has_different_defaults(): # these should always be set on Sandbox assert config.Sandbox.REDIS_ENABLED is False - - -def test_logging_stdout_json_defaults_to_off(reload_config): - os.environ.pop('LOGGING_STDOUT_JSON', None) - assert config.Config.LOGGING_STDOUT_JSON is False - - -def test_logging_stdout_json_sets_to_off_if_not_recognised(reload_config): - os.environ['LOGGING_STDOUT_JSON'] = 'foo' - - importlib.reload(config) - - assert config.Config.LOGGING_STDOUT_JSON is False - - -def test_logging_stdout_json_sets_to_on_if_set_to_1(reload_config): - os.environ['LOGGING_STDOUT_JSON'] = '1' - - importlib.reload(config) - - assert config.Config.LOGGING_STDOUT_JSON is True From b227ed450a3ae9eb016a0e16c6f346130b2fdd88 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 9 Aug 2017 15:42:40 +0100 Subject: [PATCH 38/39] Got rid of an if --- app/service/rest.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 6af9a1f0e..743fa5d25 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -604,9 +604,8 @@ def is_service_name_unique(): name_exists = Service.query.filter_by(name=name).first() email_from_exists = Service.query.filter_by(email_from=email_from).first() - if name_exists or email_from_exists: - return jsonify(result=False), 200 - return jsonify(result=True), 200 + result = not (name_exists or email_from_exists) + return jsonify(result=result), 200 def check_request_args(request): From 83fc529cc540dd6da6f32bbd3ec3556319fad2ad Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 9 Aug 2017 16:07:35 +0100 Subject: [PATCH 39/39] fix broken cloudfoundry logs wasn't setting log path properly --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index c05b96660..940c08622 100644 --- a/app/config.py +++ b/app/config.py @@ -95,6 +95,7 @@ class Config(object): # Logging DEBUG = False + NOTIFY_LOG_PATH = os.getenv('NOTIFY_LOG_PATH') ########################### # Default config values ### @@ -105,7 +106,6 @@ class Config(object): AWS_REGION = 'eu-west-1' INVITATION_EXPIRATION_DAYS = 2 NOTIFY_APP_NAME = 'api' - NOTIFY_LOG_PATH = None SQLALCHEMY_COMMIT_ON_TEARDOWN = False SQLALCHEMY_RECORD_QUERIES = True SQLALCHEMY_TRACK_MODIFICATIONS = True