From 4a7267be8bf59ba65f5472618eaecc032b379080 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 1 Sep 2016 14:31:01 +0100 Subject: [PATCH 01/20] Add an endpoint to cancel a job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you schedule a job you might change your mind or circumstances might change. So you need to be able to cancel it. This commit adds a `POST` endpoint for individual jobs which sets their status to `cancelled`. This also means adding a new status of `cancelled`, so there’s a migration… --- app/dao/jobs_dao.py | 7 ++++++ app/job/rest.py | 13 +++++++++- app/models.py | 1 + .../versions/0051_cancelled_job_status.py | 22 ++++++++++++++++ tests/app/conftest.py | 18 ++++++++++++- tests/app/dao/test_jobs_dao.py | 8 ++++++ tests/app/job/test_rest.py | 25 +++++++++++++++++++ 7 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 migrations/versions/0051_cancelled_job_status.py diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 3ad1f84fd..94bcc6210 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -44,6 +44,13 @@ def dao_get_scheduled_jobs(): .all() +def dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id): + return Job.query \ + .filter_by(service_id=service_id, id=job_id) \ + .filter(Job.job_status == 'scheduled', Job.scheduled_for > datetime.utcnow()) \ + .one() + + def dao_create_job(job): db.session.add(job) db.session.commit() diff --git a/app/job/rest.py b/app/job/rest.py index 8d490c333..9f9ef0047 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -7,8 +7,10 @@ from flask import ( from app.dao.jobs_dao import ( dao_create_job, + dao_update_job, dao_get_job_by_service_id_and_job_id, dao_get_jobs_by_service_id, + dao_get_future_scheduled_job_by_id_and_service_id, dao_get_notification_outcomes_for_job ) @@ -28,7 +30,7 @@ from app.schemas import ( from app.celery.tasks import process_job -from app.models import JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING +from app.models import JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING, JOB_STATUS_CANCELLED from app.utils import pagination_links @@ -53,6 +55,15 @@ def get_job_by_service_and_job_id(service_id, job_id): return jsonify(data=data) +@job.route('//cancel', methods=['POST']) +def cancel_job(service_id, job_id): + job = dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id) + job.job_status = JOB_STATUS_CANCELLED + dao_update_job(job) + + return get_job_by_service_and_job_id(service_id, job_id) + + @job.route('//notifications', methods=['GET']) def get_all_notifications_for_service_job(service_id, job_id): data = notifications_filter_schema.load(request.args).data diff --git a/app/models.py b/app/models.py index e3bb53f2c..6345071aa 100644 --- a/app/models.py +++ b/app/models.py @@ -308,6 +308,7 @@ JOB_STATUS_IN_PROGRESS = 'in progress' JOB_STATUS_FINISHED = 'finished' JOB_STATUS_SENDING_LIMITS_EXCEEDED = 'sending limits exceeded' JOB_STATUS_SCHEDULED = 'scheduled' +JOB_STATUS_CANCELLED = 'cancelled' class JobStatus(db.Model): diff --git a/migrations/versions/0051_cancelled_job_status.py b/migrations/versions/0051_cancelled_job_status.py new file mode 100644 index 000000000..902da29ac --- /dev/null +++ b/migrations/versions/0051_cancelled_job_status.py @@ -0,0 +1,22 @@ +"""empty message + +Revision ID: 0051_cancelled_job_status +Revises: 0050_index_for_stats +Create Date: 2016-09-01 14:34:06.839381 + +""" + +# revision identifiers, used by Alembic. +revision = '0051_cancelled_job_status' +down_revision = '0050_index_for_stats' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + op.execute("INSERT INTO job_status VALUES ('cancelled')") + +def downgrade(): + op.execute("UPDATE jobs SET job_status = 'finished' WHERE job_status = 'cancelled'") + op.execute("DELETE FROM job_status WHERE name = 'cancelled';") diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 76aba362a..1296d8bcd 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -1,7 +1,7 @@ import requests_mock import pytest import uuid -from datetime import (datetime, date) +from datetime import (datetime, date, timedelta) from flask import current_app @@ -288,6 +288,22 @@ def sample_job_with_placeholdered_template( ) +@pytest.fixture(scope='function') +def sample_scheduled_job( + notify_db, + notify_db_session, + service=None +): + return sample_job( + notify_db, + notify_db_session, + service=service, + template=sample_template_with_placeholders(notify_db, notify_db_session), + scheduled_for=(datetime.utcnow() + timedelta(minutes=60)).isoformat(), + job_status='scheduled' + ) + + @pytest.fixture(scope='function') def sample_email_job(notify_db, notify_db_session, diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 3674ef24c..0c23bad9d 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -7,6 +7,7 @@ from app.dao.jobs_dao import ( dao_update_job, dao_get_jobs_by_service_id, dao_get_scheduled_jobs, + dao_get_future_scheduled_job_by_id_and_service_id, dao_get_notification_outcomes_for_job ) @@ -251,3 +252,10 @@ def test_get_scheduled_jobs_gets_ignores_jobs_scheduled_in_the_future(notify_db, sample_job(notify_db, notify_db_session, scheduled_for=one_minute_in_the_future, job_status='scheduled') jobs = dao_get_scheduled_jobs() assert len(jobs) == 0 + + +def test_get_future_scheduled_job_gets_a_job_yet_to_send(notify_db, notify_db_session): + one_hour_from_now = datetime.utcnow() + timedelta(minutes=60) + job = sample_job(notify_db, notify_db_session, scheduled_for=one_hour_from_now, job_status='scheduled') + result = dao_get_future_scheduled_job_by_id_and_service_id(job.id, job.service_id) + assert result.id == job.id diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 488a75419..3fb3cd9a9 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -109,6 +109,31 @@ def test_get_job_by_id(notify_api, sample_job): assert resp_json['data']['created_by']['name'] == 'Test User' +def test_cancel_job(notify_api, sample_scheduled_job): + job_id = str(sample_scheduled_job.id) + service_id = sample_scheduled_job.service.id + with notify_api.test_request_context(), notify_api.test_client() as client: + path = '/service/{}/job/{}/cancel'.format(service_id, job_id) + auth_header = create_authorization_header(service_id=service_id) + response = client.post(path, headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['data']['id'] == job_id + assert resp_json['data']['job_status'] == 'cancelled' + + +def test_cant_cancel_normal_job(notify_api, sample_job, mocker): + job_id = str(sample_job.id) + service_id = sample_job.service.id + with notify_api.test_request_context(), notify_api.test_client() as client: + mock_update = mocker.patch('app.dao.jobs_dao.dao_update_job') + path = '/service/{}/job/{}/cancel'.format(service_id, job_id) + auth_header = create_authorization_header(service_id=service_id) + response = client.post(path, headers=[auth_header]) + assert response.status_code == 404 + assert mock_update.call_count == 0 + + def test_create_unscheduled_job(notify_api, sample_template, mocker, fake_uuid): with notify_api.test_request_context(): with notify_api.test_client() as client: From 11a4b1845181a97eea943d6651789bb5258a5bc7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Sep 2016 12:20:28 +0100 Subject: [PATCH 02/20] Combine query filters when getting scheduled job > filter_by and filter are just aliases for each other so can be > combined together - filter is probably the better one (and then use > == instead of keyword args) --- app/dao/jobs_dao.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 94bcc6210..74d9a4f90 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -46,8 +46,12 @@ def dao_get_scheduled_jobs(): def dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id): return Job.query \ - .filter_by(service_id=service_id, id=job_id) \ - .filter(Job.job_status == 'scheduled', Job.scheduled_for > datetime.utcnow()) \ + .filter( + Job.service_id == service_id, + Job.id == job_id, + Job.job_status == 'scheduled', + Job.scheduled_for > datetime.utcnow() + ) \ .one() From 5d4e942fa15c3f4231948813e1f5f010bfbb7dd6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Sep 2016 12:24:14 +0100 Subject: [PATCH 03/20] Use fixture for testing scheduled jobs --- tests/app/dao/test_jobs_dao.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 0c23bad9d..69efcd08d 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -247,15 +247,15 @@ def test_get_scheduled_jobs_gets_ignores_jobs_not_scheduled(notify_db, notify_db assert jobs[0].id == job_scheduled.id -def test_get_scheduled_jobs_gets_ignores_jobs_scheduled_in_the_future(notify_db, notify_db_session): - one_minute_in_the_future = datetime.utcnow() + timedelta(minutes=1) - sample_job(notify_db, notify_db_session, scheduled_for=one_minute_in_the_future, job_status='scheduled') +def test_get_scheduled_jobs_gets_ignores_jobs_scheduled_in_the_future( + notify_db, notify_db_session, sample_scheduled_job +): jobs = dao_get_scheduled_jobs() assert len(jobs) == 0 -def test_get_future_scheduled_job_gets_a_job_yet_to_send(notify_db, notify_db_session): - one_hour_from_now = datetime.utcnow() + timedelta(minutes=60) - job = sample_job(notify_db, notify_db_session, scheduled_for=one_hour_from_now, job_status='scheduled') - result = dao_get_future_scheduled_job_by_id_and_service_id(job.id, job.service_id) - assert result.id == job.id +def test_get_future_scheduled_job_gets_a_job_yet_to_send( + notify_db, notify_db_session, sample_scheduled_job +): + result = dao_get_future_scheduled_job_by_id_and_service_id(sample_scheduled_job.id, sample_scheduled_job.service_id) + assert result.id == sample_scheduled_job.id From 2c17825c128d25d0031cb15f66840dea9320936b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Sep 2016 23:13:32 +0100 Subject: [PATCH 04/20] =?UTF-8?q?Don=E2=80=99t=20use=20magic=20string=20fo?= =?UTF-8?q?r=20job=20status?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/dao/jobs_dao.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 74d9a4f90..0d4634d56 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -2,7 +2,7 @@ from datetime import date, timedelta, datetime from sqlalchemy import desc, asc, cast, Date as sql_date from app import db from app.dao import days_ago -from app.models import Job, NotificationHistory +from app.models import Job, NotificationHistory, JOB_STATUS_SCHEDULED from app.statsd_decorators import statsd from sqlalchemy import func, asc @@ -39,7 +39,7 @@ def dao_get_job_by_id(job_id): def dao_get_scheduled_jobs(): return Job.query \ - .filter(Job.job_status == 'scheduled', Job.scheduled_for < datetime.utcnow()) \ + .filter(Job.job_status == JOB_STATUS_SCHEDULED, Job.scheduled_for < datetime.utcnow()) \ .order_by(asc(Job.scheduled_for)) \ .all() @@ -49,7 +49,7 @@ def dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id): .filter( Job.service_id == service_id, Job.id == job_id, - Job.job_status == 'scheduled', + Job.job_status == JOB_STATUS_SCHEDULED, Job.scheduled_for > datetime.utcnow() ) \ .one() From 2aa8baec630985669aa8e75bbf0fb77fb2b017d2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Sep 2016 23:14:03 +0100 Subject: [PATCH 05/20] Make indentation consistent --- app/dao/jobs_dao.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 0d4634d56..790b9cb18 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -39,7 +39,10 @@ def dao_get_job_by_id(job_id): def dao_get_scheduled_jobs(): return Job.query \ - .filter(Job.job_status == JOB_STATUS_SCHEDULED, Job.scheduled_for < datetime.utcnow()) \ + .filter( + Job.job_status == JOB_STATUS_SCHEDULED, + Job.scheduled_for < datetime.utcnow() + ) \ .order_by(asc(Job.scheduled_for)) \ .all() From e28ce50d179e1c85470d9730a2d24b19365b2503 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Sep 2016 23:18:55 +0100 Subject: [PATCH 06/20] =?UTF-8?q?Don=E2=80=99t=20pass=20notify=5Fdb=20or?= =?UTF-8?q?=20notify=5Fdb=5Fsession=20to=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They get set up by the `sample_scheduled_job` fixture. --- tests/app/dao/test_jobs_dao.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 69efcd08d..0c3153e95 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -247,15 +247,11 @@ def test_get_scheduled_jobs_gets_ignores_jobs_not_scheduled(notify_db, notify_db assert jobs[0].id == job_scheduled.id -def test_get_scheduled_jobs_gets_ignores_jobs_scheduled_in_the_future( - notify_db, notify_db_session, sample_scheduled_job -): +def test_get_scheduled_jobs_gets_ignores_jobs_scheduled_in_the_future(sample_scheduled_job): jobs = dao_get_scheduled_jobs() assert len(jobs) == 0 -def test_get_future_scheduled_job_gets_a_job_yet_to_send( - notify_db, notify_db_session, sample_scheduled_job -): +def test_get_future_scheduled_job_gets_a_job_yet_to_send(sample_scheduled_job): result = dao_get_future_scheduled_job_by_id_and_service_id(sample_scheduled_job.id, sample_scheduled_job.service_id) assert result.id == sample_scheduled_job.id From 4d1c34fccea41e0f119fc5bc54c0b1f17c887917 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 5 Sep 2016 16:45:54 +0100 Subject: [PATCH 07/20] A little bit of clean up. - Remove the deploy to staging | live links in the README - Update most of the outdated requirements. Left Flask update out for now. --- README.md | 2 -- requirements.txt | 18 +++++++++--------- tests/app/celery/test_scheduled_tasks.py | 2 +- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 44bfd2822..a6917c43c 100644 --- a/README.md +++ b/README.md @@ -1,8 +1,6 @@ ![](https://travis-ci.org/alphagov/notifications-api.svg) [![Requirements Status](https://requires.io/github/alphagov/notifications-api/requirements.svg?branch=master)](https://requires.io/github/alphagov/notifications-api/requirements/?branch=master) -[![Deploy to staging](https://notify-build-monitor.herokuapp.com/deploys/notifications-api/master...staging.svg?prefix=Deploy%20to)](https://github.com/alphagov/notifications-api/compare/staging...master?expand=1&title=Deploy%20to%20staging) [![Deploy to live](https://notify-build-monitor.herokuapp.com/deploys/notifications-api/staging...live.svg?prefix=Deploy%20to)](https://github.com/alphagov/notifications-api/compare/live...staging?expand=1&title=Deploy%20to%20live) - # notifications-api Notifications api Application for the notification api. diff --git a/requirements.txt b/requirements.txt index d377b6184..1da28d23d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,25 +1,25 @@ -apispec==0.12.0 +apispec==0.14.0 bleach==1.4.3 Flask==0.10.1 Flask-Script==2.0.5 Flask-Migrate==1.3.1 Flask-SQLAlchemy==2.0 -psycopg2==2.6.1 -SQLAlchemy==1.0.5 -SQLAlchemy-Utils==0.30.5 -PyJWT==1.4.0 +psycopg2==2.6.2 +SQLAlchemy==1.0.15 +SQLAlchemy-Utils==0.32.9 +PyJWT==1.4.2 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.2.3 -boto==2.39.0 +boto3==1.4.0 +boto==2.42.0 celery==3.1.20 -monotonic==0.3 +monotonic==1.2 statsd==3.2.1 -git+https://github.com/alphagov/notifications-python-client.git@1.0.0#egg=notifications-python-client==1.0.0 +git+https://github.com/alphagov/notifications-python-client.git@1.2.0#egg=notifications-python-client==1.2.0 git+https://github.com/alphagov/notifications-utils.git@9.0.0#egg=notifications-utils==9.0.0 diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 92478d7da..893988490 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -11,7 +11,7 @@ from app.celery.scheduled_tasks import (delete_verify_codes, run_scheduled_jobs) from app.dao.jobs_dao import dao_get_job_by_id from tests.app.conftest import sample_notification, sample_job -from mock import call +from unittest.mock import call def test_should_have_decorated_tasks_functions(): From 96a1d1a135058cc754b09346a601cac7ffe49406 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 5 Sep 2016 17:00:16 +0100 Subject: [PATCH 08/20] Update celery requirement --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 1da28d23d..4821e5a16 100644 --- a/requirements.txt +++ b/requirements.txt @@ -15,7 +15,7 @@ Flask-Bcrypt==0.6.2 credstash==1.8.0 boto3==1.4.0 boto==2.42.0 -celery==3.1.20 +celery==3.1.23 monotonic==1.2 statsd==3.2.1 From d8a8a69aff5f079aeab051bf02b3fdc23f4af817 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 6 Sep 2016 10:30:57 +0100 Subject: [PATCH 09/20] Update README to include additonal instructions --- README.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 44bfd2822..d9f7e41c3 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,11 @@ NOTE: The SECRET_KEY and DANGEROUS_SALT should match those in the [notifications NOTE: Also note the unique prefix for the queue names. This prevents clashing with others queues in shared amazon environment and using a prefix enables filtering by queue name in the SQS interface. +Install Postgresql +```shell + brew install postgres +``` ## To run the application @@ -75,7 +79,6 @@ scripts/run_celery_beat.sh ``` - ## To test the application First, ensure that `scripts/boostrap.sh` has been run, as it creates the test database. From 3b6905b98d64c627aad47d24a5fe7ccbb6652bc0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 6 Sep 2016 10:53:34 +0100 Subject: [PATCH 10/20] Update requirements Brings in: - [x] https://github.com/alphagov/notifications-utils/pull/67 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index d377b6184..f1902beb0 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,4 +22,4 @@ statsd==3.2.1 git+https://github.com/alphagov/notifications-python-client.git@1.0.0#egg=notifications-python-client==1.0.0 -git+https://github.com/alphagov/notifications-utils.git@9.0.0#egg=notifications-utils==9.0.0 +git+https://github.com/alphagov/notifications-utils.git@9.0.1#egg=notifications-utils==9.0.1 From d6a87805152624c963c47820ad30dd49a5dc5ed6 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 6 Sep 2016 11:39:22 +0100 Subject: [PATCH 11/20] Remove send notification test --- app/notifications/rest.py | 9 -- tests/app/celery/test_scheduled_tasks.py | 2 +- .../rest/test_send_notification.py | 92 ------------------- .../test_POST_notification.py | 2 +- 4 files changed, 2 insertions(+), 103 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index f6abd950d..22e616f69 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -192,15 +192,6 @@ def send_notification(notification_type): service_id = str(api_user.service_id) service = services_dao.dao_fetch_service_by_id(service_id) - service_stats = sum(row.count for row in dao_fetch_todays_stats_for_service(service.id)) - - if all(( - api_user.key_type != KEY_TYPE_TEST, - service_stats >= service.message_limit - )): - error = 'Exceeded send limits ({}) for today'.format(service.message_limit) - raise InvalidRequest(error, status_code=429) - notification, errors = ( sms_template_notification_schema if notification_type == SMS_TYPE else email_notification_schema ).load(request.get_json()) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 92478d7da..893988490 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -11,7 +11,7 @@ from app.celery.scheduled_tasks import (delete_verify_codes, run_scheduled_jobs) from app.dao.jobs_dao import dao_get_job_by_id from tests.app.conftest import sample_notification, sample_job -from mock import call +from unittest.mock import call def test_should_have_decorated_tasks_functions(): diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index b0492e8bb..0a456b6de 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -568,98 +568,6 @@ def test_should_allow_valid_email_notification(notify_api, sample_email_template assert response_data['template_version'] == sample_email_template.version -@pytest.mark.parametrize('restricted', [True, False]) -@freeze_time("2016-01-01 12:00:00.061258") -def test_should_block_api_call_if_over_day_limit_for_restricted_and_live_service(notify_db, - notify_db_session, - notify_api, - mocker, - restricted): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_email.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - - service = create_sample_service(notify_db, notify_db_session, limit=1, restricted=restricted) - email_template = create_sample_email_template(notify_db, notify_db_session, service=service) - create_sample_notification( - notify_db, notify_db_session, template=email_template, service=service, created_at=datetime.utcnow() - ) - - data = { - 'to': 'ok@ok.com', - 'template': str(email_template.id) - } - - auth_header = create_authorization_header(service_id=service.id) - - response = client.post( - path='/notifications/email', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - json_resp = json.loads(response.get_data(as_text=True)) - - assert response.status_code == 429 - assert 'Exceeded send limits (1) for today' in json_resp['message'] - - -@freeze_time("2016-01-01 12:00:00.061258") -def test_should_block_api_call_if_over_day_limit_regardless_of_type(notify_db, notify_db_session, notify_api, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_sms.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - - service = create_sample_service(notify_db, notify_db_session, limit=1, restricted=True) - email_template = create_sample_email_template(notify_db, notify_db_session, service=service) - sms_template = create_sample_template(notify_db, notify_db_session, service=service) - create_sample_notification( - notify_db, notify_db_session, template=email_template, service=service, created_at=datetime.utcnow() - ) - - data = { - 'to': '+447234123123', - 'template': str(sms_template.id) - } - - auth_header = create_authorization_header(service_id=service.id) - - response = client.post( - path='/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 429 - assert 'Exceeded send limits (1) for today' in json_resp['message'] - - -@freeze_time("2016-01-01 12:00:00.061258") -def test_should_allow_api_call_if_under_day_limit_regardless_of_type(notify_db, notify_db_session, notify_api, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_sms.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - - service = create_sample_service(notify_db, notify_db_session, limit=2) - email_template = create_sample_email_template(notify_db, notify_db_session, service=service) - sms_template = create_sample_template(notify_db, notify_db_session, service=service) - create_sample_notification(notify_db, notify_db_session, template=email_template, service=service) - - data = { - 'to': '+447634123123', - 'template': str(sms_template.id) - } - - auth_header = create_authorization_header(service_id=service.id) - - response = client.post( - path='/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - assert response.status_code == 201 - - def test_should_not_return_html_in_body(notify_api, notify_db, notify_db_session, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: diff --git a/tests/app/public_contracts/test_POST_notification.py b/tests/app/public_contracts/test_POST_notification.py index ffb91210c..ce333cef3 100644 --- a/tests/app/public_contracts/test_POST_notification.py +++ b/tests/app/public_contracts/test_POST_notification.py @@ -25,7 +25,7 @@ def test_post_sms_contract(client, mocker, sample_template): def test_post_email_contract(client, mocker, sample_email_template): - mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('app.celery.tasks.send_email.apply_async') mocker.patch('app.encryption.encrypt', return_value="something_encrypted") data = { From 1bf37888dd245c85e203b2e11c27269fa3dfd7ef Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 6 Sep 2016 12:53:57 +0100 Subject: [PATCH 12/20] Fix mis-matched database migrations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cancelled job migration originally went in as version `0051`. But `0051_set_job_status` went in first in e3fa8bffd7dd436a9d3da68058528d3ad1cd2c09 This meant that Alembic couldn’t work out what order to run the migrations in, and blew up. This commit puts the scheduled job migration after all others. --- ...ncelled_job_status.py => 0053_cancelled_job_status.py} | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) rename migrations/versions/{0051_cancelled_job_status.py => 0053_cancelled_job_status.py} (75%) diff --git a/migrations/versions/0051_cancelled_job_status.py b/migrations/versions/0053_cancelled_job_status.py similarity index 75% rename from migrations/versions/0051_cancelled_job_status.py rename to migrations/versions/0053_cancelled_job_status.py index 902da29ac..6cc5365df 100644 --- a/migrations/versions/0051_cancelled_job_status.py +++ b/migrations/versions/0053_cancelled_job_status.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 0051_cancelled_job_status -Revises: 0050_index_for_stats +Revision ID: 0053_cancelled_job_status +Revises: 0052_drop_jobs_status Create Date: 2016-09-01 14:34:06.839381 """ # revision identifiers, used by Alembic. -revision = '0051_cancelled_job_status' -down_revision = '0050_index_for_stats' +revision = '0053_cancelled_job_status' +down_revision = '0052_drop_jobs_status' from alembic import op import sqlalchemy as sa From 259fb0cb6236afddeba6002c3f6f4a6f05ca3020 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 6 Sep 2016 14:21:36 +0100 Subject: [PATCH 13/20] Slightly changed to only unlimited the live services. Logic: - live services don't check days limit for now - restricted services check limits (caveat) simulate keys aren't checking day limit even in restricted mode. --- app/notifications/rest.py | 10 +- .../rest/test_send_notification.py | 100 ++++++++++++++++++ 2 files changed, 108 insertions(+), 2 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 22e616f69..246ee5efc 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -196,6 +196,12 @@ def send_notification(notification_type): sms_template_notification_schema if notification_type == SMS_TYPE else email_notification_schema ).load(request.get_json()) + if all((api_user.key_type != KEY_TYPE_TEST, service.restricted)): + service_stats = sum(row.count for row in dao_fetch_todays_stats_for_service(service.id)) + if service_stats >= service.message_limit: + error = 'Exceeded send limits ({}) for today'.format(service.message_limit) + raise InvalidRequest(error, status_code=429) + if errors: raise InvalidRequest(errors, status_code=400) @@ -224,8 +230,8 @@ def send_notification(notification_type): raise InvalidRequest(errors, status_code=400) if ( - template_object.template_type == SMS_TYPE and - template_object.replaced_content_count > current_app.config.get('SMS_CHAR_COUNT_LIMIT') + template_object.template_type == SMS_TYPE and + template_object.replaced_content_count > current_app.config.get('SMS_CHAR_COUNT_LIMIT') ): char_count = current_app.config.get('SMS_CHAR_COUNT_LIMIT') message = 'Content has a character count greater than the limit of {}'.format(char_count) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 0a456b6de..d4e649d95 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -568,6 +568,106 @@ def test_should_allow_valid_email_notification(notify_api, sample_email_template assert response_data['template_version'] == sample_email_template.version +@freeze_time("2016-01-01 12:00:00.061258") +def test_should_not_block_api_call_if_over_day_limit_for_live_service( + notify_db, + notify_db_session, + notify_api, + mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_email.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + + service = create_sample_service(notify_db, notify_db_session, limit=1, restricted=False) + email_template = create_sample_email_template(notify_db, notify_db_session, service=service) + create_sample_notification( + notify_db, notify_db_session, template=email_template, service=service, created_at=datetime.utcnow() + ) + + data = { + 'to': 'ok@ok.com', + 'template': str(email_template.id) + } + + auth_header = create_authorization_header(service_id=service.id) + + response = client.post( + path='/notifications/email', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + json.loads(response.get_data(as_text=True)) + + assert response.status_code == 201 + + +@freeze_time("2016-01-01 12:00:00.061258") +def test_should_block_api_call_if_over_day_limit_for_restricted_service( + notify_db, + notify_db_session, + notify_api, + mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_email.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + + service = create_sample_service(notify_db, notify_db_session, limit=1, restricted=True) + email_template = create_sample_email_template(notify_db, notify_db_session, service=service) + create_sample_notification( + notify_db, notify_db_session, template=email_template, service=service, created_at=datetime.utcnow() + ) + + data = { + 'to': 'ok@ok.com', + 'template': str(email_template.id) + } + + auth_header = create_authorization_header(service_id=service.id) + + response = client.post( + path='/notifications/email', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + json.loads(response.get_data(as_text=True)) + + assert response.status_code == 429 + + +@pytest.mark.parametrize('restricted', [True, False]) +@freeze_time("2016-01-01 12:00:00.061258") +def test_should_allow_api_call_if_under_day_limit_regardless_of_type( + notify_db, + notify_db_session, + notify_api, + sample_user, + mocker, + restricted): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + + service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=restricted) + email_template = create_sample_email_template(notify_db, notify_db_session, service=service) + sms_template = create_sample_template(notify_db, notify_db_session, service=service) + create_sample_notification(notify_db, notify_db_session, template=email_template, service=service) + + data = { + 'to': sample_user.mobile_number, + 'template': str(sms_template.id) + } + + auth_header = create_authorization_header(service_id=service.id) + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 201 + + def test_should_not_return_html_in_body(notify_api, notify_db, notify_db_session, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: From c1b00a5f0cb880008028dd72eee94c10c20c72ba Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 7 Sep 2016 09:35:31 +0100 Subject: [PATCH 14/20] Simplified the config. Aim is to get the actual secrets in credstash to be env specific, and not the random collection of things we have at the moment. Secret definition also includes env specific things such as URLs / Queue prefixes / URLs for providers and so on. --- README.md | 36 +++++--------- app/__init__.py | 1 - config.py | 94 +++++++++++++++++++++++++---------- environment_test.sh | 29 ++++------- tests/app/clients/test_mmg.py | 2 +- 5 files changed, 91 insertions(+), 71 deletions(-) diff --git a/README.md b/README.md index a6917c43c..b8b992150 100644 --- a/README.md +++ b/README.md @@ -20,31 +20,19 @@ Create a local environment.sh file containing the following: ``` echo " -export NOTIFY_ENVIRONMENT='development' -export ADMIN_BASE_URL='http://localhost:6012' -export ADMIN_CLIENT_USER_NAME='dev-notify-admin' -export ADMIN_CLIENT_SECRET='dev-notify-secret-key' -export API_HOST_NAME='http://localhost:6011' - -export AWS_REGION='eu-west-1' -export AWS_ACCESS_KEY_ID=[MY ACCESS KEY] -export AWS_SECRET_ACCESS_KEY=[MY SECRET] - -export DANGEROUS_SALT='dev-notify-salt' -export FIRETEXT_API_KEY=[contact team member for api key] -export FROM_NUMBER='40605' -export INVITATION_EMAIL_FROM='invites' -export INVITATION_EXPIRATION_DAYS=2 -export MMG_API_KEY=mmg=secret-key -export MMG_URL="https://api.mmg.co.uk/json/api.php" -export NOTIFICATION_QUEUE_PREFIX='[unique-to-environment]' # -export NOTIFY_EMAIL_DOMAIN='notify.tools' -export SECRET_KEY='dev-notify-secret-key' export SQLALCHEMY_DATABASE_URI='postgresql://localhost/notification_api' -export STATSD_ENABLED=True -export STATSD_HOST="localhost" -export STATSD_PORT=1000 -export STATSD_PREFIX="stats-prefix" +export SECRET_KEY='secret-key' +export DANGEROUS_SALT='dangerous-salt' +export NOTIFY_ENVIRONMENT="development" +export ADMIN_CLIENT_SECRET='notify-secret-key' +export ADMIN_BASE_URL='http://localhost:6012' +export FROM_NUMBER='development' +export MMG_URL="https://api.mmg.co.uk/json/api.php" +export MMG_API_KEY='MMG_API_KEY' +export LOADTESTING_API_KEY="FIRETEXT_SIMULATION_KEY" +export FIRETEXT_API_KEY="FIRETEXT_ACTUAL_KEY" +export STATSD_PREFIX="FAKE_PREFIX" +export NOTIFICATION_QUEUE_PREFIX="PREFIX-TO-IDENTIFY-SQS-QUEUE" "> environment.sh ``` diff --git a/app/__init__.py b/app/__init__.py index 464157f60..cf47632f9 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -41,7 +41,6 @@ def create_app(app_name=None): from config import configs application.config.from_object(configs[os.environ['NOTIFY_ENVIRONMENT']]) - if app_name: application.config['NOTIFY_APP_NAME'] = app_name diff --git a/config.py b/config.py index 44a0b88d1..32395c5d7 100644 --- a/config.py +++ b/config.py @@ -5,27 +5,61 @@ import os class Config(object): - DEBUG = False + ######################################## + # Secrets that are held in credstash ### + ######################################## + + # URL of admin app ADMIN_BASE_URL = os.environ['ADMIN_BASE_URL'] - ADMIN_CLIENT_USER_NAME = os.environ['ADMIN_CLIENT_USER_NAME'] + + # admin app api key ADMIN_CLIENT_SECRET = os.environ['ADMIN_CLIENT_SECRET'] - AWS_REGION = os.environ['AWS_REGION'] + + # encyption secret/salt + SECRET_KEY = os.environ['SECRET_KEY'] DANGEROUS_SALT = os.environ['DANGEROUS_SALT'] - INVITATION_EXPIRATION_DAYS = int(os.environ['INVITATION_EXPIRATION_DAYS']) - INVITATION_EMAIL_FROM = os.environ['INVITATION_EMAIL_FROM'] + + # DB conection string + SQLALCHEMY_DATABASE_URI = os.environ['SQLALCHEMY_DATABASE_URI'] + + # MMG API Url + MMG_URL = os.environ['MMG_URL'] + + # MMG API Key + MMG_API_KEY = os.environ['MMG_API_KEY'] + + # Firetext API Key + FIRETEXT_API_KEY = os.getenv("FIRETEXT_API_KEY") + + # Firetext simluation key + LOADTESTING_API_KEY = os.getenv("LOADTESTING_API_KEY") + + # Hosted graphite statsd prefix + STATSD_PREFIX = os.getenv('STATSD_PREFIX') + + # Prefix to identify queues in SQS + NOTIFICATION_QUEUE_PREFIX = os.getenv('NOTIFICATION_QUEUE_PREFIX') + + ########################### + # Default config values ### + ########################### + + DEBUG = False + NOTIFY_ENVIRONMENT = 'development' + ADMIN_CLIENT_USER_NAME = 'notify-admin' + AWS_REGION = 'eu-west-1' + INVITATION_EXPIRATION_DAYS = 2 + INVITATION_EMAIL_FROM = 'no-reply' NOTIFY_APP_NAME = 'api' NOTIFY_LOG_PATH = '/var/log/notify/application.log' # Notification Queue names are a combination of a prefix plus a name - NOTIFICATION_QUEUE_PREFIX = os.environ['NOTIFICATION_QUEUE_PREFIX'] - SECRET_KEY = os.environ['SECRET_KEY'] + NOTIFICATION_QUEUE_PREFIX = 'development' SQLALCHEMY_COMMIT_ON_TEARDOWN = False - SQLALCHEMY_DATABASE_URI = os.environ['SQLALCHEMY_DATABASE_URI'] SQLALCHEMY_RECORD_QUERIES = True SQLALCHEMY_TRACK_MODIFICATIONS = True - NOTIFY_EMAIL_DOMAIN = os.environ['NOTIFY_EMAIL_DOMAIN'] + NOTIFY_EMAIL_DOMAIN = 'notify.tools' PAGE_SIZE = 50 SMS_CHAR_COUNT_LIMIT = 495 - MMG_URL = os.environ['MMG_URL'] BRANDING_PATH = '/static/images/email-template/crests/' NOTIFY_SERVICE_ID = 'd6aa2c68-a2d9-4437-ab19-3ae8eb202553' @@ -37,10 +71,10 @@ class Config(object): BROKER_URL = 'sqs://' BROKER_TRANSPORT_OPTIONS = { - 'region': 'eu-west-1', + 'region': AWS_REGION, 'polling_interval': 1, # 1 second 'visibility_timeout': 14410, # 4 hours 10 seconds. 10 seconds longer than max retry - 'queue_name_prefix': os.environ['NOTIFICATION_QUEUE_PREFIX'] + '-' + 'queue_name_prefix': NOTIFICATION_QUEUE_PREFIX } CELERY_ENABLE_UTC = True, CELERY_TIMEZONE = 'Europe/London' @@ -96,26 +130,26 @@ class Config(object): Queue('retry', Exchange('default'), routing_key='retry'), Queue('email-already-registered', Exchange('default'), routing_key='email-already-registered') ] - API_HOST_NAME = os.environ['API_HOST_NAME'] - MMG_API_KEY = os.environ['MMG_API_KEY'] - FIRETEXT_API_KEY = os.getenv("FIRETEXT_API_KEY") - LOADTESTING_NUMBER = os.getenv('LOADTESTING_NUMBER') - LOADTESTING_API_KEY = os.getenv("LOADTESTING_API_KEY") - CSV_UPLOAD_BUCKET_NAME = os.getenv("CSV_UPLOAD_BUCKET_NAME") + API_HOST_NAME = "http://localhost:6011" + + CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' NOTIFICATIONS_ALERT = 5 # five mins - FROM_NUMBER = os.getenv('FROM_NUMBER') + FROM_NUMBER = 'development' STATSD_ENABLED = False STATSD_HOST = "statsd.hostedgraphite.com" STATSD_PORT = 8125 - STATSD_PREFIX = None SENDING_NOTIFICATIONS_TIMEOUT_PERIOD = 259200 +###################### +# Config overrides ### +###################### + class Development(Config): NOTIFY_ENVIRONMENT = 'development' - CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' + NOTIFICATION_QUEUE_PREFIX = 'development' DEBUG = True SQLALCHEMY_ECHO = False CELERY_QUEUES = Config.CELERY_QUEUES + [ @@ -127,10 +161,14 @@ class Development(Config): class Test(Config): + NOTIFY_EMAIL_DOMAIN = 'test.notify.com' + FROM_NUMBER = 'testing' NOTIFY_ENVIRONMENT = 'test' DEBUG = True CSV_UPLOAD_BUCKET_NAME = 'test-notifications-csv-upload' - STATSD_PREFIX = "test" + STATSD_ENABLED = True + STATSD_HOST = "localhost" + STATSD_PORT = 1000 CELERY_QUEUES = Config.CELERY_QUEUES + [ Queue('db-sms', Exchange('default'), routing_key='db-sms'), Queue('send-sms', Exchange('default'), routing_key='send-sms'), @@ -140,23 +178,29 @@ class Test(Config): class Preview(Config): + NOTIFY_EMAIL_DOMAIN = 'notify.works' NOTIFY_ENVIRONMENT = 'preview' CSV_UPLOAD_BUCKET_NAME = 'preview-notifications-csv-upload' - STATSD_PREFIX = "preview" + API_HOST_NAME = 'http://admin-api.internal' + FROM_NUMBER = 'NotifyPreview' class Staging(Config): + NOTIFY_EMAIL_DOMAIN = 'staging-notify.works' NOTIFY_ENVIRONMENT = 'staging' CSV_UPLOAD_BUCKET_NAME = 'staging-notify-csv-upload' - STATSD_PREFIX = os.getenv('STATSD_PREFIX') STATSD_ENABLED = True + API_HOST_NAME = 'http://admin-api.internal' + FROM_NUMBER = 'NotifyStage' class Live(Config): + NOTIFY_EMAIL_DOMAIN = 'notifications.service.gov.uk' NOTIFY_ENVIRONMENT = 'live' CSV_UPLOAD_BUCKET_NAME = 'live-notifications-csv-upload' - STATSD_PREFIX = os.getenv('STATSD_PREFIX') STATSD_ENABLED = True + API_HOST_NAME = 'http://admin-api.internal' + FROM_NUMBER = '40604' configs = { diff --git a/environment_test.sh b/environment_test.sh index d0c536072..3f8a2fbe5 100644 --- a/environment_test.sh +++ b/environment_test.sh @@ -1,24 +1,13 @@ #!/bin/bash -export NOTIFY_ENVIRONMENT='test' -export ADMIN_BASE_URL='http://localhost:6012' -export ADMIN_CLIENT_USER_NAME='dev-notify-admin' -export ADMIN_CLIENT_SECRET='dev-notify-secret-key' -export AWS_REGION='eu-west-1' -export DANGEROUS_SALT='dangerous-salt' -export INVITATION_EMAIL_FROM='invites' -export INVITATION_EXPIRATION_DAYS=2 -export NOTIFICATION_QUEUE_PREFIX='test-env-not-used' -export SECRET_KEY='secret-key' export SQLALCHEMY_DATABASE_URI=${TEST_DATABASE:='postgresql://localhost/test_notification_api'} -export FIRETEXT_API_KEY="Firetext" -export NOTIFY_EMAIL_DOMAIN="test.notify.com" -export MMG_API_KEY='mmg-secret-key' -export LOADTESTING_API_KEY="loadtesting" -export LOADTESTING_NUMBER="loadtesting" -export STATSD_ENABLED=True -export STATSD_HOST="localhost" -export STATSD_PORT=1000 -export STATSD_PREFIX="stats-prefix" -export API_HOST_NAME="http://localhost:6011" +export SECRET_KEY='secret-key' +export DANGEROUS_SALT='dangerous-salt' +export NOTIFY_ENVIRONMENT='test' +export ADMIN_CLIENT_SECRET='dev-notify-secret-key' +export ADMIN_BASE_URL='http://localhost:6012' export FROM_NUMBER='from_number' export MMG_URL="https://api.mmg.co.uk/json/api.php" +export MMG_API_KEY='mmg-secret-key' +export LOADTESTING_API_KEY="loadtesting" +export FIRETEXT_API_KEY="Firetext" +export STATSD_PREFIX="stats-prefix" diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index bc4428193..649764d89 100644 --- a/tests/app/clients/test_mmg.py +++ b/tests/app/clients/test_mmg.py @@ -60,7 +60,7 @@ def test_send_sms_calls_mmg_correctly(notify_api, mocker): assert request_args['reqType'] == 'BULK' assert request_args['MSISDN'] == to assert request_args['msg'] == content - assert request_args['sender'] == 'from_number' + assert request_args['sender'] == 'testing' assert request_args['cid'] == reference assert request_args['multi'] is True From d8bfad1ce7e5f116af5f11010f60e84750ddec5c Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 7 Sep 2016 09:37:45 +0100 Subject: [PATCH 15/20] Added prefix to test env file for completeness --- environment_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/environment_test.sh b/environment_test.sh index 3f8a2fbe5..a74a81a86 100644 --- a/environment_test.sh +++ b/environment_test.sh @@ -1,4 +1,3 @@ -#!/bin/bash export SQLALCHEMY_DATABASE_URI=${TEST_DATABASE:='postgresql://localhost/test_notification_api'} export SECRET_KEY='secret-key' export DANGEROUS_SALT='dangerous-salt' @@ -11,3 +10,4 @@ export MMG_API_KEY='mmg-secret-key' export LOADTESTING_API_KEY="loadtesting" export FIRETEXT_API_KEY="Firetext" export STATSD_PREFIX="stats-prefix" +export NOTIFICATION_QUEUE_PREFIX='testing' From 840b99b277b28b28b70d5565980ff583e3be69a0 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 7 Sep 2016 09:57:20 +0100 Subject: [PATCH 16/20] Fix issue where test key cannot send messages to external numbers --- app/celery/tasks.py | 13 ++++--- tests/app/celery/test_tasks.py | 62 +++++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a03df1878..0726bb220 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -28,7 +28,8 @@ from app.models import ( Notification, EMAIL_TYPE, SMS_TYPE, - KEY_TYPE_NORMAL + KEY_TYPE_NORMAL, + KEY_TYPE_TEST ) from app.statsd_decorators import statsd @@ -126,7 +127,7 @@ def send_sms(self, notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) - if not service_allowed_to_send_to(notification['to'], service): + if not service_allowed_to_send_to(notification['to'], service, key_type): current_app.logger.info( "SMS {} failed as restricted service".format(notification_id) ) @@ -160,10 +161,12 @@ def send_email(self, service_id, created_at, api_key_id=None, key_type=KEY_TYPE_NORMAL): + + notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) - if not service_allowed_to_send_to(notification['to'], service): + if not service_allowed_to_send_to(notification['to'], service, key_type): current_app.logger.info("Email {} failed as restricted service".format(notification_id)) return @@ -203,8 +206,8 @@ def _save_notification(created_at, notification, notification_id, service_id, no dao_create_notification(notification_db_object) -def service_allowed_to_send_to(recipient, service): - if not service.restricted: +def service_allowed_to_send_to(recipient, service, key_type): + if not service.restricted or key_type == KEY_TYPE_TEST: return True return allowed_to_send_to( diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 3b2159c38..8426947a1 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -17,7 +17,7 @@ from app.celery.tasks import ( send_email ) from app.dao import jobs_dao -from app.models import Notification, KEY_TYPE_TEAM +from app.models import Notification, KEY_TYPE_TEAM, KEY_TYPE_TEST from tests.app import load_example_csv from tests.app.conftest import ( sample_service, @@ -350,6 +350,66 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif assert persisted_notification.notification_type == 'sms' +def test_should_not_send_sms_if_restricted_service_and_invalid_number_with_test_key(notify_db, + notify_db_session, + mocker): + user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900205") + service = sample_service(notify_db, notify_db_session, user=user, restricted=True) + template = sample_template(notify_db, notify_db_session, service=service) + + notification = _notification_json(template, "07700 900849") + mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') + + notification_id = uuid.uuid4() + send_sms( + service.id, + notification_id, + encryption.encrypt(notification), + datetime.utcnow().strftime(DATETIME_FORMAT), + key_type=KEY_TYPE_TEST + ) + + provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( + (service.id, + notification_id), + queue="send-sms" + ) + + persisted_notification = Notification.query.filter_by(id=notification_id).one() + assert persisted_notification.id == notification_id + + +def test_should_not_send_email_if_restricted_service_and_invalid_email_address_with_test_key(notify_db, + notify_db_session, + mocker): + user = sample_user(notify_db, notify_db_session) + service = sample_service(notify_db, notify_db_session, user=user, restricted=True) + template = sample_template( + notify_db, notify_db_session, service=service, template_type='email', subject_line='Hello' + ) + + notification = _notification_json(template, to="test@example.com") + mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') + + notification_id = uuid.uuid4() + send_email( + service.id, + notification_id, + encryption.encrypt(notification), + datetime.utcnow().strftime(DATETIME_FORMAT), + key_type=KEY_TYPE_TEST + ) + + provider_tasks.send_email_to_provider.apply_async.assert_called_once_with( + (service.id, + notification_id), + queue="send-email" + ) + + persisted_notification = Notification.query.filter_by(id=notification_id).one() + assert persisted_notification.id == notification_id + + def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900205") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) From 94708bdee8b03f2a07739ea4d062a4dbd547f2f6 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 7 Sep 2016 11:03:16 +0100 Subject: [PATCH 17/20] Fix indentation issue --- app/celery/tasks.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 0726bb220..d655c28ce 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -161,8 +161,6 @@ def send_email(self, service_id, created_at, api_key_id=None, key_type=KEY_TYPE_NORMAL): - - notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) From 41681ac001b4f93902c3c894bf2d6862e8d435e5 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 7 Sep 2016 11:17:27 +0100 Subject: [PATCH 18/20] Fix mocker syntax issue --- tests/app/public_contracts/test_POST_notification.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/public_contracts/test_POST_notification.py b/tests/app/public_contracts/test_POST_notification.py index ffb91210c..ce333cef3 100644 --- a/tests/app/public_contracts/test_POST_notification.py +++ b/tests/app/public_contracts/test_POST_notification.py @@ -25,7 +25,7 @@ def test_post_sms_contract(client, mocker, sample_template): def test_post_email_contract(client, mocker, sample_email_template): - mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('app.celery.tasks.send_email.apply_async') mocker.patch('app.encryption.encrypt', return_value="something_encrypted") data = { From bdc1d464bf59c5259f727de61db149e158a029c0 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 8 Sep 2016 09:07:43 +0100 Subject: [PATCH 19/20] Fixed up a few things as per pull request review: - Moved email domain and css bucket into dev config, not default - deleted duplicated property - removed unused property --- config.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/config.py b/config.py index 32395c5d7..d4d43363a 100644 --- a/config.py +++ b/config.py @@ -49,15 +49,11 @@ class Config(object): ADMIN_CLIENT_USER_NAME = 'notify-admin' AWS_REGION = 'eu-west-1' INVITATION_EXPIRATION_DAYS = 2 - INVITATION_EMAIL_FROM = 'no-reply' NOTIFY_APP_NAME = 'api' NOTIFY_LOG_PATH = '/var/log/notify/application.log' - # Notification Queue names are a combination of a prefix plus a name - NOTIFICATION_QUEUE_PREFIX = 'development' SQLALCHEMY_COMMIT_ON_TEARDOWN = False SQLALCHEMY_RECORD_QUERIES = True SQLALCHEMY_TRACK_MODIFICATIONS = True - NOTIFY_EMAIL_DOMAIN = 'notify.tools' PAGE_SIZE = 50 SMS_CHAR_COUNT_LIMIT = 495 BRANDING_PATH = '/static/images/email-template/crests/' @@ -132,7 +128,6 @@ class Config(object): ] API_HOST_NAME = "http://localhost:6011" - CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' NOTIFICATIONS_ALERT = 5 # five mins FROM_NUMBER = 'development' @@ -148,6 +143,8 @@ class Config(object): ###################### class Development(Config): + NOTIFY_EMAIL_DOMAIN = 'notify.tools' + CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' NOTIFY_ENVIRONMENT = 'development' NOTIFICATION_QUEUE_PREFIX = 'development' DEBUG = True From a3f3f3e2796e9e651c4ac6e8d5769ea860a00dd6 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 8 Sep 2016 10:44:14 +0100 Subject: [PATCH 20/20] Changed the FROM_NUMBER on preview and stage to be shorter --- config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config.py b/config.py index d4d43363a..415487d7f 100644 --- a/config.py +++ b/config.py @@ -179,7 +179,7 @@ class Preview(Config): NOTIFY_ENVIRONMENT = 'preview' CSV_UPLOAD_BUCKET_NAME = 'preview-notifications-csv-upload' API_HOST_NAME = 'http://admin-api.internal' - FROM_NUMBER = 'NotifyPreview' + FROM_NUMBER = 'preview' class Staging(Config): @@ -188,7 +188,7 @@ class Staging(Config): CSV_UPLOAD_BUCKET_NAME = 'staging-notify-csv-upload' STATSD_ENABLED = True API_HOST_NAME = 'http://admin-api.internal' - FROM_NUMBER = 'NotifyStage' + FROM_NUMBER = 'stage' class Live(Config):