From 048861b9680ced6b2fac235d3882eb769edc35fb Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 10 May 2017 15:58:44 +0100 Subject: [PATCH 01/24] Add dao to get active users for service --- app/dao/services_dao.py | 9 +++++++++ tests/app/dao/test_services_dao.py | 12 +++++++++++- tests/app/db.py | 5 +++-- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index c5679289c..b0891c490 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -384,3 +384,12 @@ def dao_suspend_service(service_id): def dao_resume_service(service_id): service = Service.query.get(service_id) service.active = True + + +def dao_fetch_active_users_for_service(service_id): + query = User.query.filter( + User.user_to_service.any(id=service_id), + User.state == 'active' + ) + + return query.all() diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index d37f8341b..435c4935b 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -24,7 +24,8 @@ from app.dao.services_dao import ( dao_fetch_todays_stats_for_all_services, fetch_stats_by_date_range_for_all_services, dao_suspend_service, - dao_resume_service + dao_resume_service, + dao_fetch_active_users_for_service ) from app.dao.users_dao import save_model_user from app.models import ( @@ -49,6 +50,7 @@ from app.models import ( KEY_TYPE_TEST ) +from tests.app.db import create_user from tests.app.conftest import ( sample_notification as create_notification, sample_notification_history as create_notification_history, @@ -783,3 +785,11 @@ def test_fetch_monthly_historical_template_stats_for_service_separates_templates assert len(result.get('2016-04').keys()) == 2 assert str(template_one.id) in result.get('2016-04').keys() assert str(template_two.id) in result.get('2016-04').keys() + + +def test_dao_fetch_active_users_for_service_returns_active_only(sample_service): + pending_user = create_user(email='foo@bar.com', state='pending') + dao_add_user_to_service(sample_service, pending_user) + users = dao_fetch_active_users_for_service(sample_service.id) + + assert len(users) == 1 diff --git a/tests/app/db.py b/tests/app/db.py index b5838d693..8bbe2bd3b 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -9,13 +9,14 @@ from app.dao.templates_dao import dao_create_template from app.dao.services_dao import dao_create_service -def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk"): +def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk", state='active'): data = { + 'id': uuid.uuid4(), 'name': 'Test User', 'email_address': email, 'password': 'password', 'mobile_number': mobile_number, - 'state': 'active' + 'state': state } user = User.query.filter_by(email_address=email).first() if not user: From 570d0ec9dba64520a706fc58730d1e8d4b94a6c9 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 12 May 2017 14:06:29 +0100 Subject: [PATCH 02/24] Send notification to active service users with user fields (optional) --- app/config.py | 1 + app/service/sender.py | 33 ++++++++++ tests/app/service/test_sender.py | 110 +++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+) create mode 100644 app/service/sender.py create mode 100644 tests/app/service/test_sender.py diff --git a/app/config.py b/app/config.py index 4b0f170fb..b87f4d031 100644 --- a/app/config.py +++ b/app/config.py @@ -89,6 +89,7 @@ class Config(object): PASSWORD_RESET_TEMPLATE_ID = '474e9242-823b-4f99-813d-ed392e7f1201' ALREADY_REGISTERED_EMAIL_TEMPLATE_ID = '0880fbb1-a0c6-46f0-9a8e-36c986381ceb' CHANGE_EMAIL_CONFIRMATION_TEMPLATE_ID = 'eb4d9930-87ab-4aef-9bce-786762687884' + SERVICE_NOW_LIVE_TEMPLATE_ID = '618185c6-3636-49cd-b7d2-6f6f5eb3bdde' BROKER_URL = 'sqs://' BROKER_TRANSPORT_OPTIONS = { diff --git a/app/service/sender.py b/app/service/sender.py new file mode 100644 index 000000000..3c6a6a03e --- /dev/null +++ b/app/service/sender.py @@ -0,0 +1,33 @@ +from flask import current_app + +from app.dao.services_dao import dao_fetch_service_by_id, dao_fetch_active_users_for_service +from app.dao.templates_dao import dao_get_template_by_id +from app.models import EMAIL_TYPE, KEY_TYPE_NORMAL +from app.notifications.process_notifications import persist_notification, send_notification_to_queue + + +def send_notification_to_service_users(service_id, template_id, personalisation={}, include_user_fields=[]): + template = dao_get_template_by_id(template_id) + service = dao_fetch_service_by_id(service_id) + active_users = dao_fetch_active_users_for_service(service.id) + notify_service = dao_fetch_service_by_id(current_app.config['NOTIFY_SERVICE_ID']) + + for user in active_users: + personalisation = _add_user_fields(user, personalisation, include_user_fields) + notification = persist_notification( + template_id=template.id, + template_version=template.version, + recipient=user.email_address if template.template_type == EMAIL_TYPE else user.mobile_number, + service=notify_service, + personalisation=personalisation, + notification_type=template.template_type, + api_key_id=None, + key_type=KEY_TYPE_NORMAL + ) + send_notification_to_queue(notification, False, queue='notify') + + +def _add_user_fields(user, personalisation, fields): + for field in fields: + personalisation[field] = getattr(user, field) + return personalisation diff --git a/tests/app/service/test_sender.py b/tests/app/service/test_sender.py new file mode 100644 index 000000000..8f2d7ac07 --- /dev/null +++ b/tests/app/service/test_sender.py @@ -0,0 +1,110 @@ +import pytest + +from flask import current_app + +from app.dao.services_dao import dao_add_user_to_service +from app.models import Notification, EMAIL_TYPE, SMS_TYPE +from app.service.sender import send_notification_to_service_users + +from tests.app.conftest import ( + notify_service as create_notify_service, + sample_service as create_sample_service +) +from tests.app.db import create_template, create_user + + +@pytest.mark.parametrize('notification_type', [ + EMAIL_TYPE, + SMS_TYPE +]) +def test_send_notification_to_service_users_persists_notifications_correctly( + notify_db, + notify_db_session, + notification_type, + sample_user, + mocker +): + mocker.patch('app.service.sender.send_notification_to_queue') + + create_notify_service(notify_db, notify_db_session) + service = create_sample_service(notify_db, notify_db_session, user=sample_user) + template = create_template(service, template_type=notification_type) + send_notification_to_service_users(service_id=service.id, template_id=template.id) + to = sample_user.email_address if notification_type == EMAIL_TYPE else sample_user.mobile_number + + notification = Notification.query.one() + + assert Notification.query.count() == 1 + assert notification.to == to + assert str(notification.service_id) == current_app.config['NOTIFY_SERVICE_ID'] + assert notification.template.id == template.id + assert notification.template.template_type == notification_type + assert notification.notification_type == notification_type + + +def test_send_notification_to_service_users_sends_to_queue( + notify_db, + notify_db_session, + sample_user, + mocker +): + send_mock = mocker.patch('app.service.sender.send_notification_to_queue') + + create_notify_service(notify_db, notify_db_session) + service = create_sample_service(notify_db, notify_db_session, user=sample_user) + template = create_template(service, template_type=EMAIL_TYPE) + send_notification_to_service_users(service_id=service.id, template_id=template.id) + + assert send_mock.called + assert send_mock.call_count == 1 + + +def test_send_notification_to_service_users_includes_user_fields_in_personalisation( + notify_db, + notify_db_session, + sample_user, + mocker +): + persist_mock = mocker.patch('app.service.sender.persist_notification') + mocker.patch('app.service.sender.send_notification_to_queue') + + create_notify_service(notify_db, notify_db_session) + service = create_sample_service(notify_db, notify_db_session, user=sample_user) + template = create_template(service, template_type=EMAIL_TYPE) + send_notification_to_service_users( + service_id=service.id, + template_id=template.id, + include_user_fields=['name', 'email_address', 'state'] + ) + + assert persist_mock.mock_calls[0][2]['personalisation'] == { + 'name': sample_user.name, + 'email_address': sample_user.email_address, + 'state': sample_user.state, + } + + +def test_send_notification_to_service_users_sends_to_active_users_only( + notify_db, + notify_db_session, + mocker +): + mocker.patch('app.service.sender.send_notification_to_queue') + + create_notify_service(notify_db, notify_db_session) + + first_active_user = create_user(email='foo@bar.com', state='active') + second_active_user = create_user(email='foo1@bar.com', state='active') + pending_user = create_user(email='foo2@bar.com', state='pending') + service = create_sample_service(notify_db, notify_db_session, user=first_active_user) + dao_add_user_to_service(service, second_active_user) + dao_add_user_to_service(service, pending_user) + template = create_template(service, template_type=EMAIL_TYPE) + + send_notification_to_service_users(service_id=service.id, template_id=template.id) + notifications = Notification.query.all() + + assert Notification.query.count() == 2 + + assert notifications[0].to == first_active_user.email_address + assert notifications[1].to == second_active_user.email_address From 5eb02a45a56e0b94189fe629523fb21468164f3b Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 12 May 2017 14:07:06 +0100 Subject: [PATCH 03/24] Send go live email when service goes live --- app/service/rest.py | 16 +++++++++++++ tests/app/db.py | 4 ++-- tests/app/service/test_rest.py | 44 ++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 2 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 4122db82a..6624c8979 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -46,6 +46,7 @@ from app.errors import ( InvalidRequest, register_errors) from app.service import statistics from app.service.utils import get_whitelist_objects +from app.service.sender import send_notification_to_service_users from app.schemas import ( service_schema, api_key_schema, @@ -117,10 +118,25 @@ def create_service(): @service_blueprint.route('/', methods=['POST']) def update_service(service_id): fetched_service = dao_fetch_service_by_id(service_id) + # Capture the status change here as Marshmallow changes this later + service_going_live = fetched_service.restricted and not request.get_json().get('restricted') + current_data = dict(service_schema.dump(fetched_service).data.items()) current_data.update(request.get_json()) update_dict = service_schema.load(current_data).data dao_update_service(update_dict) + + if service_going_live: + send_notification_to_service_users( + service_id=service_id, + template_id=current_app.config['SERVICE_NOW_LIVE_TEMPLATE_ID'], + personalisation={ + 'service_name': current_data['name'], + 'message_limit': current_data['message_limit'] + }, + include_user_fields=['name'] + ) + return jsonify(data=service_schema.dump(fetched_service).data), 200 diff --git a/tests/app/db.py b/tests/app/db.py index 8bbe2bd3b..f1dba8ffa 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -25,11 +25,11 @@ def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-off return user -def create_service(user=None, service_name="Sample service", service_id=None): +def create_service(user=None, service_name="Sample service", service_id=None, restricted=False): service = Service( name=service_name, message_limit=1000, - restricted=False, + restricted=restricted, email_from=service_name.lower().replace(' ', '.'), created_by=user or create_user() ) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 91c928751..8fb5e3fc7 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1658,3 +1658,47 @@ def test_search_for_notification_by_to_field_return_multiple_matches( assert str(notification1.id) in [n["id"] for n in result["notifications"]] assert str(notification2.id) in [n["id"] for n in result["notifications"]] assert str(notification3.id) in [n["id"] for n in result["notifications"]] + + +def test_update_service_calls_send_notification_as_service_becomes_live(notify_db, notify_db_session, client, mocker): + send_notification_mock = mocker.patch('app.service.rest.send_notification_to_service_users') + + restricted_service = create_service( + notify_db, + notify_db_session, + restricted=True + ) + + data = { + "restricted": False + } + + auth_header = create_authorization_header() + resp = client.post( + 'service/{}'.format(restricted_service.id), + data=json.dumps(data), + headers=[auth_header], + content_type='application/json' + ) + + assert resp.status_code == 200 + assert send_notification_mock.called + + +def test_update_service_does_not_call_send_notification_for_live_service(sample_service, client, mocker): + send_notification_mock = mocker.patch('app.service.rest.send_notification_to_service_users') + + data = { + "restricted": True + } + + auth_header = create_authorization_header() + resp = client.post( + 'service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[auth_header], + content_type='application/json' + ) + + assert resp.status_code == 200 + assert not send_notification_mock.called From f766f90207cc7a6c3cef52935b324a859dff239f Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 4 May 2017 10:31:18 +0100 Subject: [PATCH 04/24] Add task to process a DVLA response file: * Currently we do nothing with the parsed response. We will * update the status of the notifications in a separate PR --- app/aws/s3.py | 20 ++++++++++++-------- app/celery/tasks.py | 32 +++++++++++++++++++++++++++++++- app/dao/notifications_dao.py | 3 ++- tests/app/celery/test_tasks.py | 34 ++++++++++++++++++++++++++++++++-- 4 files changed, 77 insertions(+), 12 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 2aa7aac39..e901416d4 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -4,20 +4,24 @@ from flask import current_app FILE_LOCATION_STRUCTURE = 'service-{}-notify/{}.csv' -def get_s3_job_object(bucket_name, file_location): +def get_s3_object(bucket_name, file_location): s3 = resource('s3') - return s3.Object(bucket_name, file_location) + s3_object = s3.Object(bucket_name, file_location) + return s3_object.get()['Body'].read() def get_job_from_s3(service_id, job_id): - bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME'] - file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id) - obj = get_s3_job_object(bucket_name, file_location) - return obj.get()['Body'].read().decode('utf-8') + job = _job_from_s3(service_id, job_id) + return job def remove_job_from_s3(service_id, job_id): + job = _job_from_s3(service_id, job_id) + return job.delete() + + +def _job_from_s3(): bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME'] file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id) - obj = get_s3_job_object(bucket_name, file_location) - return obj.delete() + obj = get_s3_object(bucket_name, file_location).decode('utf-8') + return obj diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 464e3e1be..4ebb73a42 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,6 +1,7 @@ import random from datetime import (datetime) +from collections import namedtuple from flask import current_app from notifications_utils.recipients import ( @@ -23,7 +24,10 @@ from app.dao.jobs_dao import ( all_notifications_are_created_for_job, dao_get_all_notifications_for_job, dao_update_job_status) -from app.dao.notifications_dao import get_notification_by_id, dao_update_notifications_sent_to_dvla +from app.dao.notifications_dao import ( + get_notification_by_id, + dao_update_notifications_sent_to_dvla +) from app.dao.provider_details_dao import get_current_provider from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count from app.dao.templates_dao import dao_get_template_by_id @@ -354,3 +358,29 @@ def get_template_class(template_type): # since we don't need rendering capabilities (we only need to extract placeholders) both email and letter can # use the same base template return WithSubjectTemplate + + +@notify_celery.task(bind=True, name='update-letter-notifications-statuses') +@statsd(namespace="tasks") +def update_letter_notifications_statuses(self, filename): + response_file = s3.get_s3_object('development-notifications-csv-upload', filename).decode('utf-8') + lines = response_file.splitlines() + notification_updates = [] + + try: + NotificationUpdate = namedtuple('NotificationUpdate', ['reference', 'status', 'page_count', 'cost_threshold']) + for line in lines: + notification_updates.append(NotificationUpdate(*line.split('|'))) + + except TypeError: + current_app.logger.exception('DVLA response file has an invalid format') + raise + + else: + if notification_updates: + for update in notification_updates: + current_app.logger.error(str(update)) + # TODO: Update notifications with desired status + return notification_updates + else: + current_app.logger.exception('DVLA response file contained no updates') diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index da81c348b..66cf031d6 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -27,7 +27,8 @@ from app.models import ( NOTIFICATION_PERMANENT_FAILURE, KEY_TYPE_NORMAL, KEY_TYPE_TEST, LETTER_TYPE, - NOTIFICATION_SENT) + NOTIFICATION_SENT +) from app.dao.dao_utils import transactional from app.statsd_decorators import statsd diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 8b9631b4f..4eb74da47 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -12,15 +12,19 @@ from celery.exceptions import Retry from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks from app.celery import tasks -from app.celery.tasks import s3, build_dvla_file, create_dvla_file_contents, update_dvla_job_to_error from app.celery.tasks import ( + s3, + build_dvla_file, + create_dvla_file_contents, + update_dvla_job_to_error, process_job, process_row, send_sms, send_email, persist_letter, get_template_class, - update_job_to_sent_to_dvla + update_job_to_sent_to_dvla, + update_letter_notifications_statuses ) from app.dao import jobs_dao, services_dao from app.models import ( @@ -1071,3 +1075,29 @@ def test_update_dvla_job_to_error(sample_letter_template, sample_letter_job): assert not n.sent_by assert 'error' == Job.query.filter_by(id=sample_letter_job.id).one().job_status + + +def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_api, mocker): + invalid_file = b'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2' + mocker.patch('app.celery.tasks.s3.get_s3_object', return_value=invalid_file) + + with pytest.raises(TypeError): + update_letter_notifications_statuses(filename='foo.txt') + + +def test_update_letter_notifications_statuses_builds_updates_list(notify_api, mocker): + valid_file = b'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted' + mocker.patch('app.celery.tasks.s3.get_s3_object', return_value=valid_file) + updates = update_letter_notifications_statuses(filename='foo.txt') + + assert len(updates) == 2 + + assert updates[0].reference == 'ref-foo' + assert updates[0].status == 'Sent' + assert updates[0].page_count == '1' + assert updates[0].cost_threshold == 'Unsorted' + + assert updates[1].reference == 'ref-bar' + assert updates[1].status == 'Sent' + assert updates[1].page_count == '2' + assert updates[1].cost_threshold == 'Sorted' From 20bb91bfdde8a1ffb591973e576bae155ec02772 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 4 May 2017 10:33:44 +0100 Subject: [PATCH 05/24] Update DVLA callback to process request and call task (if it can) --- .../notifications_letter_callback.py | 18 ++-- .../app/notifications/rest/test_callbacks.py | 82 ++++++++++++++++++- 2 files changed, 92 insertions(+), 8 deletions(-) diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index f7c357055..3b4a0fbb3 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/app/notifications/notifications_letter_callback.py @@ -9,6 +9,7 @@ from flask import ( ) from app import statsd_client +from app.celery.tasks import update_letter_notifications_statuses from app.clients.email.aws_ses import get_aws_responses from app.dao import ( notifications_dao @@ -29,11 +30,18 @@ register_errors(letter_callback_blueprint) @letter_callback_blueprint.route('/notifications/letter/dvla', methods=['POST']) def process_letter_response(): try: - dvla_request = json.loads(request.data) - current_app.logger.info(dvla_request) + req_json = json.loads(request.data) + # The callback should have one record for an S3 Put Event. + filename = req_json['Message']['Records'][0]['s3']['object']['key'] + + except (ValueError, KeyError): + error = "DVLA callback failed: Invalid JSON" + raise InvalidRequest(error, status_code=400) + + else: + current_app.logger.info('DVLA callback: Calling task to update letter notifications') + update_letter_notifications_statuses.apply_async([filename], queue='notify') + return jsonify( result="success", message="DVLA callback succeeded" ), 200 - except ValueError: - error = "DVLA callback failed: invalid json" - raise InvalidRequest(error, status_code=400) diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 780076b34..0dd070271 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -1,3 +1,4 @@ +import pytest import uuid from datetime import datetime @@ -6,6 +7,7 @@ from flask import json from freezegun import freeze_time import app.celery.tasks +from app.errors import InvalidRequest from app.dao.notifications_dao import ( get_notification_by_id ) @@ -13,16 +15,47 @@ from app.models import NotificationStatistics from tests.app.conftest import sample_notification as create_sample_notification -def test_dvla_callback_should_not_need_auth(client): - data = json.dumps({"somekey": "somevalue"}) +def test_dvla_callback_returns_400_with_invalid_request(client): + data = json.dumps({"foo": "bar"}) response = client.post( path='/notifications/letter/dvla', data=data, - headers=[('Content-Type', 'application/json')]) + headers=[('Content-Type', 'application/json')] + ) + json_resp = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'DVLA callback failed: Invalid JSON' + + +def test_dvla_callback_returns_200_with_valid_request(client): + data = _sample_sns_s3_callback() + response = client.post( + path='/notifications/letter/dvla', + data=data, + headers=[('Content-Type', 'application/json')] + ) + json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 200 +def test_dvla_callback_calls_update_letter_notifications_task(client, mocker): + update_notifications_mock = \ + mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses') + data = _sample_sns_s3_callback() + response = client.post( + path='/notifications/letter/dvla', + data=data, + headers=[('Content-Type', 'application/json')] + ) + json_resp = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 200 + assert update_notifications_mock.apply_async.called is True + + def test_firetext_callback_should_not_need_auth(client, mocker): mocker.patch('app.statsd_client.incr') response = client.post( @@ -458,3 +491,46 @@ def test_firetext_callback_should_record_statsd(client, notify_db, notify_db_ses def get_notification_stats(service_id): return NotificationStatistics.query.filter_by(service_id=service_id).one() + + +def _sample_sns_s3_callback(): + return json.dumps({ + "SigningCertURL": "foo.pem", + "UnsubscribeURL": "bar", + "Signature": "some-signature", + "Type": "Notification", + "Timestamp": "2016-05-03T08:35:12.884Z", + "SignatureVersion": "1", + "MessageId": "6adbfe0a-d610-509a-9c47-af894e90d32d", + "Subject": "Amazon S3 Notification", + "TopicArn": "sample-topic-arn", + "Message": { + "Records": [{ + "eventVersion": "2.0", + "eventSource": "aws:s3", + "awsRegion": "eu-west-1", + "eventTime": "2017-05-03T08:35:12.826Z", + "eventName": "ObjectCreated:Put", + "userIdentity": {"principalId": "some-p-id"}, + "requestParameters": {"sourceIPAddress": "8.8.8.8"}, + "responseElements": {"x-amz-request-id": "some-req-id", "x-amz-id-2": "some-amz-id"}, + "s3": { + "s3SchemaVersion": "1.0", + "configurationId": "some-config-id", + "bucket": { + "name": "some-bucket", + "ownerIdentity": {"principalId": "some-p-id"}, + "arn": "some-bucket-arn" + }, + "object": { + "key": "bar.txt", + "size": 200, + "eTag": "some-etag", + "versionId": "some-v-id", + "sequencer": "some-seq" + } + } + } + ] + } + }) From 8a5e82904e4ffe58bbd6ffcb3696e44db905ec66 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 4 May 2017 11:42:59 +0100 Subject: [PATCH 06/24] Update to pull from correct bucket and fix tests not mocking out correctly --- app/celery/tasks.py | 3 ++- tests/app/celery/test_tasks.py | 10 ++++++++++ tests/app/notifications/rest/test_callbacks.py | 10 ++++++---- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 4ebb73a42..8498ff05a 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -363,7 +363,8 @@ def get_template_class(template_type): @notify_celery.task(bind=True, name='update-letter-notifications-statuses') @statsd(namespace="tasks") def update_letter_notifications_statuses(self, filename): - response_file = s3.get_s3_object('development-notifications-csv-upload', filename).decode('utf-8') + bucket_location = '{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']) + response_file = s3.get_s3_object(bucket_location, filename).decode('utf-8') lines = response_file.splitlines() notification_updates = [] diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 4eb74da47..12f558f7a 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -38,6 +38,7 @@ from app.models import ( Job) from tests.app import load_example_csv +from tests.conftest import set_config from tests.app.conftest import ( sample_service, sample_template, @@ -1085,6 +1086,15 @@ def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_a update_letter_notifications_statuses(filename='foo.txt') +def test_update_letter_notifications_statuses_calls_with_correct_bucket_location(notify_api, mocker): + invalid_file = b'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2' + s3_mock = mocker.patch('app.celery.tasks.s3.get_s3_object') + + with set_config(notify_api, 'NOTIFY_EMAIL_DOMAIN', 'foo.bar'): + update_letter_notifications_statuses(filename='foo.txt') + s3_mock.assert_called_with('{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']), 'foo.txt') + + def test_update_letter_notifications_statuses_builds_updates_list(notify_api, mocker): valid_file = b'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted' mocker.patch('app.celery.tasks.s3.get_s3_object', return_value=valid_file) diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 0dd070271..095384836 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -29,8 +29,9 @@ def test_dvla_callback_returns_400_with_invalid_request(client): assert json_resp['message'] == 'DVLA callback failed: Invalid JSON' -def test_dvla_callback_returns_200_with_valid_request(client): +def test_dvla_callback_returns_200_with_valid_request(client, mocker): data = _sample_sns_s3_callback() + mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') response = client.post( path='/notifications/letter/dvla', data=data, @@ -42,8 +43,8 @@ def test_dvla_callback_returns_200_with_valid_request(client): def test_dvla_callback_calls_update_letter_notifications_task(client, mocker): - update_notifications_mock = \ - mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses') + update_task = \ + mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') data = _sample_sns_s3_callback() response = client.post( path='/notifications/letter/dvla', @@ -53,7 +54,8 @@ def test_dvla_callback_calls_update_letter_notifications_task(client, mocker): json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 200 - assert update_notifications_mock.apply_async.called is True + assert update_task.called is True + update_task.assert_called_with(['bar.txt'], queue='notify') def test_firetext_callback_should_not_need_auth(client, mocker): From 37165e5b6a16d6ac0d1b3f22380a650a1277eb95 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 9 May 2017 14:20:56 +0100 Subject: [PATCH 07/24] Add autoconfirm sns in dvla callback --- .../notifications_letter_callback.py | 58 ++++++++++++------- app/notifications/utils.py | 7 +++ .../app/notifications/rest/test_callbacks.py | 32 +++++++--- 3 files changed, 69 insertions(+), 28 deletions(-) diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index 3b4a0fbb3..c7bc80d9e 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/app/notifications/notifications_letter_callback.py @@ -1,4 +1,5 @@ from datetime import datetime +from functools import wraps from flask import ( Blueprint, @@ -11,37 +12,52 @@ from flask import ( from app import statsd_client from app.celery.tasks import update_letter_notifications_statuses from app.clients.email.aws_ses import get_aws_responses -from app.dao import ( - notifications_dao -) - +from app.dao import notifications_dao +from app.v2.errors import register_errors from app.notifications.process_client_response import validate_callback_data +from app.notifications.utils import autoconfirm_subscription +from app.schema_validation import validate + letter_callback_blueprint = Blueprint('notifications_letter_callback', __name__) - -from app.errors import ( - register_errors, - InvalidRequest -) - register_errors(letter_callback_blueprint) +dvla_sns_callback_schema = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "sns callback received on s3 update", + "type": "object", + "title": "dvla internal sns callback", + "properties": { + "Type": {"enum": ["Notification", "SubscriptionConfirmation"]}, + "MessageId": {"type": "string"}, + "Message": {"type": ["string", "object"]} + }, + "required": ["Type", "MessageId", "Message"] +} + + +def validate_schema(schema): + def decorator(f): + @wraps(f) + def wrapper(*args, **kw): + validate(request.json, schema) + return f(*args, **kw) + return wrapper + return decorator + + @letter_callback_blueprint.route('/notifications/letter/dvla', methods=['POST']) +@validate_schema(dvla_sns_callback_schema) def process_letter_response(): - try: - req_json = json.loads(request.data) + req_json = request.json + if not autoconfirm_subscription(req_json): # The callback should have one record for an S3 Put Event. filename = req_json['Message']['Records'][0]['s3']['object']['key'] - - except (ValueError, KeyError): - error = "DVLA callback failed: Invalid JSON" - raise InvalidRequest(error, status_code=400) - - else: + current_app.logger.info('Received file from DVLA: {}'.format(filename)) current_app.logger.info('DVLA callback: Calling task to update letter notifications') update_letter_notifications_statuses.apply_async([filename], queue='notify') - return jsonify( - result="success", message="DVLA callback succeeded" - ), 200 + return jsonify( + result="success", message="DVLA callback succeeded" + ), 200 diff --git a/app/notifications/utils.py b/app/notifications/utils.py index 80346c475..5f1443f09 100644 --- a/app/notifications/utils.py +++ b/app/notifications/utils.py @@ -16,3 +16,10 @@ def confirm_subscription(confirmation_request): raise e return confirmation_request['TopicArn'] + + +def autoconfirm_subscription(req_json): + if req_json.get('Type') == 'SubscriptionConfirmation': + current_app.logger.info("SNS subscription confirmation url: {}".format(req_json['SubscribeURL'])) + subscribed_topic = confirm_subscription(req_json) + return subscribed_topic diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 095384836..242f18a46 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -12,6 +12,7 @@ from app.dao.notifications_dao import ( get_notification_by_id ) from app.models import NotificationStatistics +from tests.app.notifications.test_notifications_ses_callback import ses_confirmation_callback from tests.app.conftest import sample_notification as create_sample_notification @@ -22,24 +23,41 @@ def test_dvla_callback_returns_400_with_invalid_request(client): data=data, headers=[('Content-Type', 'application/json')] ) + json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'DVLA callback failed: Invalid JSON' -def test_dvla_callback_returns_200_with_valid_request(client, mocker): - data = _sample_sns_s3_callback() - mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') +def test_dvla_callback_autoconfirms_subscription(client, mocker): + autoconfirm_mock = mocker.patch('app.notifications.notifications_letter_callback.autoconfirm_subscription') + + data = ses_confirmation_callback() response = client.post( path='/notifications/letter/dvla', data=data, headers=[('Content-Type', 'application/json')] ) - json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 200 + assert autoconfirm_mock.called + + +def test_dvla_callback_autoconfirm_does_not_call_update_letter_notifications_task(client, mocker): + autoconfirm_mock = mocker.patch('app.notifications.notifications_letter_callback.autoconfirm_subscription') + update_task = \ + mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') + + data = ses_confirmation_callback() + response = client.post( + path='/notifications/letter/dvla', + data=data, + headers=[('Content-Type', 'application/json')] + ) + + assert response.status_code == 200 + assert autoconfirm_mock.called + assert not update_task.called def test_dvla_callback_calls_update_letter_notifications_task(client, mocker): @@ -54,7 +72,7 @@ def test_dvla_callback_calls_update_letter_notifications_task(client, mocker): json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 200 - assert update_task.called is True + assert update_task.called update_task.assert_called_with(['bar.txt'], queue='notify') From 4d82512ec65bc369e69b52d4fb4aecce74e4ce7c Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 9 May 2017 14:21:57 +0100 Subject: [PATCH 08/24] Update SES callback to use autconfirm method --- app/notifications/notifications_ses_callback.py | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index ce8dec9ba..e78fb9394 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -13,9 +13,8 @@ from app.clients.email.aws_ses import get_aws_responses from app.dao import ( notifications_dao ) - from app.notifications.process_client_response import validate_callback_data -from app.notifications.utils import confirm_subscription +from app.notifications.utils import autoconfirm_subscription ses_callback_blueprint = Blueprint('notifications_ses_callback', __name__) @@ -32,14 +31,12 @@ def process_ses_response(): try: ses_request = json.loads(request.data) - if ses_request.get('Type') == 'SubscriptionConfirmation': - current_app.logger.info("SNS subscription confirmation url: {}".format(ses_request['SubscribeURL'])) - subscribed_topic = confirm_subscription(ses_request) - if subscribed_topic: - current_app.logger.info("Automatically subscribed to topic: {}".format(subscribed_topic)) - return jsonify( - result="success", message="SES callback succeeded" - ), 200 + subscribed_topic = autoconfirm_subscription(ses_request) + if subscribed_topic: + current_app.logger.info("Automatically subscribed to topic: {}".format(subscribed_topic)) + return jsonify( + result="success", message="SES callback succeeded" + ), 200 errors = validate_callback_data(data=ses_request, fields=['Message'], client_name=client_name) if errors: From 95aa5680f97228791bfbbf5579483173c3e5673e Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 9 May 2017 14:22:10 +0100 Subject: [PATCH 09/24] Add more logging in update letter notifications task --- app/celery/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 8498ff05a..988977fcd 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -380,7 +380,7 @@ def update_letter_notifications_statuses(self, filename): else: if notification_updates: for update in notification_updates: - current_app.logger.error(str(update)) + current_app.logger.info('DVLA update: {}'.format(str(update))) # TODO: Update notifications with desired status return notification_updates else: From 0f7093fc386f23ed450864e9c384b8b8058d2d03 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 12 May 2017 14:24:17 +0100 Subject: [PATCH 10/24] Refactor and add filename in logging --- app/celery/tasks.py | 7 ++----- tests/app/celery/test_tasks.py | 1 - 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 988977fcd..da90ba6f0 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -365,16 +365,13 @@ def get_template_class(template_type): def update_letter_notifications_statuses(self, filename): bucket_location = '{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']) response_file = s3.get_s3_object(bucket_location, filename).decode('utf-8') - lines = response_file.splitlines() - notification_updates = [] try: NotificationUpdate = namedtuple('NotificationUpdate', ['reference', 'status', 'page_count', 'cost_threshold']) - for line in lines: - notification_updates.append(NotificationUpdate(*line.split('|'))) + notification_updates = [NotificationUpdate(*line.split('|')) for line in response_file.splitlines()] except TypeError: - current_app.logger.exception('DVLA response file has an invalid format') + current_app.logger.exception('DVLA response file: {} has an invalid format'.format(filename)) raise else: diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 12f558f7a..e48932302 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1087,7 +1087,6 @@ def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_a def test_update_letter_notifications_statuses_calls_with_correct_bucket_location(notify_api, mocker): - invalid_file = b'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2' s3_mock = mocker.patch('app.celery.tasks.s3.get_s3_object') with set_config(notify_api, 'NOTIFY_EMAIL_DOMAIN', 'foo.bar'): From 7a10a91262738e84e5677238f6ae815216384bb1 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 12 May 2017 17:21:07 +0100 Subject: [PATCH 11/24] Revert "Process SNS request triggered by a DVLA S3 update" --- app/aws/s3.py | 20 ++-- app/celery/tasks.py | 30 +----- app/dao/notifications_dao.py | 3 +- .../notifications_letter_callback.py | 62 ++++------- .../notifications_ses_callback.py | 17 +-- app/notifications/utils.py | 7 -- tests/app/celery/test_tasks.py | 43 +------- .../app/notifications/rest/test_callbacks.py | 102 +----------------- 8 files changed, 44 insertions(+), 240 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index e901416d4..2aa7aac39 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -4,24 +4,20 @@ from flask import current_app FILE_LOCATION_STRUCTURE = 'service-{}-notify/{}.csv' -def get_s3_object(bucket_name, file_location): +def get_s3_job_object(bucket_name, file_location): s3 = resource('s3') - s3_object = s3.Object(bucket_name, file_location) - return s3_object.get()['Body'].read() + return s3.Object(bucket_name, file_location) def get_job_from_s3(service_id, job_id): - job = _job_from_s3(service_id, job_id) - return job + bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME'] + file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id) + obj = get_s3_job_object(bucket_name, file_location) + return obj.get()['Body'].read().decode('utf-8') def remove_job_from_s3(service_id, job_id): - job = _job_from_s3(service_id, job_id) - return job.delete() - - -def _job_from_s3(): bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME'] file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id) - obj = get_s3_object(bucket_name, file_location).decode('utf-8') - return obj + obj = get_s3_job_object(bucket_name, file_location) + return obj.delete() diff --git a/app/celery/tasks.py b/app/celery/tasks.py index da90ba6f0..464e3e1be 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,7 +1,6 @@ import random from datetime import (datetime) -from collections import namedtuple from flask import current_app from notifications_utils.recipients import ( @@ -24,10 +23,7 @@ from app.dao.jobs_dao import ( all_notifications_are_created_for_job, dao_get_all_notifications_for_job, dao_update_job_status) -from app.dao.notifications_dao import ( - get_notification_by_id, - dao_update_notifications_sent_to_dvla -) +from app.dao.notifications_dao import get_notification_by_id, dao_update_notifications_sent_to_dvla from app.dao.provider_details_dao import get_current_provider from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count from app.dao.templates_dao import dao_get_template_by_id @@ -358,27 +354,3 @@ def get_template_class(template_type): # since we don't need rendering capabilities (we only need to extract placeholders) both email and letter can # use the same base template return WithSubjectTemplate - - -@notify_celery.task(bind=True, name='update-letter-notifications-statuses') -@statsd(namespace="tasks") -def update_letter_notifications_statuses(self, filename): - bucket_location = '{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']) - response_file = s3.get_s3_object(bucket_location, filename).decode('utf-8') - - try: - NotificationUpdate = namedtuple('NotificationUpdate', ['reference', 'status', 'page_count', 'cost_threshold']) - notification_updates = [NotificationUpdate(*line.split('|')) for line in response_file.splitlines()] - - except TypeError: - current_app.logger.exception('DVLA response file: {} has an invalid format'.format(filename)) - raise - - else: - if notification_updates: - for update in notification_updates: - current_app.logger.info('DVLA update: {}'.format(str(update))) - # TODO: Update notifications with desired status - return notification_updates - else: - current_app.logger.exception('DVLA response file contained no updates') diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 66cf031d6..da81c348b 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -27,8 +27,7 @@ from app.models import ( NOTIFICATION_PERMANENT_FAILURE, KEY_TYPE_NORMAL, KEY_TYPE_TEST, LETTER_TYPE, - NOTIFICATION_SENT -) + NOTIFICATION_SENT) from app.dao.dao_utils import transactional from app.statsd_decorators import statsd diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index c7bc80d9e..f7c357055 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/app/notifications/notifications_letter_callback.py @@ -1,5 +1,4 @@ from datetime import datetime -from functools import wraps from flask import ( Blueprint, @@ -10,54 +9,31 @@ from flask import ( ) from app import statsd_client -from app.celery.tasks import update_letter_notifications_statuses from app.clients.email.aws_ses import get_aws_responses -from app.dao import notifications_dao -from app.v2.errors import register_errors -from app.notifications.process_client_response import validate_callback_data -from app.notifications.utils import autoconfirm_subscription -from app.schema_validation import validate +from app.dao import ( + notifications_dao +) +from app.notifications.process_client_response import validate_callback_data letter_callback_blueprint = Blueprint('notifications_letter_callback', __name__) + +from app.errors import ( + register_errors, + InvalidRequest +) + register_errors(letter_callback_blueprint) -dvla_sns_callback_schema = { - "$schema": "http://json-schema.org/draft-04/schema#", - "description": "sns callback received on s3 update", - "type": "object", - "title": "dvla internal sns callback", - "properties": { - "Type": {"enum": ["Notification", "SubscriptionConfirmation"]}, - "MessageId": {"type": "string"}, - "Message": {"type": ["string", "object"]} - }, - "required": ["Type", "MessageId", "Message"] -} - - -def validate_schema(schema): - def decorator(f): - @wraps(f) - def wrapper(*args, **kw): - validate(request.json, schema) - return f(*args, **kw) - return wrapper - return decorator - - @letter_callback_blueprint.route('/notifications/letter/dvla', methods=['POST']) -@validate_schema(dvla_sns_callback_schema) def process_letter_response(): - req_json = request.json - if not autoconfirm_subscription(req_json): - # The callback should have one record for an S3 Put Event. - filename = req_json['Message']['Records'][0]['s3']['object']['key'] - current_app.logger.info('Received file from DVLA: {}'.format(filename)) - current_app.logger.info('DVLA callback: Calling task to update letter notifications') - update_letter_notifications_statuses.apply_async([filename], queue='notify') - - return jsonify( - result="success", message="DVLA callback succeeded" - ), 200 + try: + dvla_request = json.loads(request.data) + current_app.logger.info(dvla_request) + return jsonify( + result="success", message="DVLA callback succeeded" + ), 200 + except ValueError: + error = "DVLA callback failed: invalid json" + raise InvalidRequest(error, status_code=400) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index e78fb9394..ce8dec9ba 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -13,8 +13,9 @@ from app.clients.email.aws_ses import get_aws_responses from app.dao import ( notifications_dao ) + from app.notifications.process_client_response import validate_callback_data -from app.notifications.utils import autoconfirm_subscription +from app.notifications.utils import confirm_subscription ses_callback_blueprint = Blueprint('notifications_ses_callback', __name__) @@ -31,12 +32,14 @@ def process_ses_response(): try: ses_request = json.loads(request.data) - subscribed_topic = autoconfirm_subscription(ses_request) - if subscribed_topic: - current_app.logger.info("Automatically subscribed to topic: {}".format(subscribed_topic)) - return jsonify( - result="success", message="SES callback succeeded" - ), 200 + if ses_request.get('Type') == 'SubscriptionConfirmation': + current_app.logger.info("SNS subscription confirmation url: {}".format(ses_request['SubscribeURL'])) + subscribed_topic = confirm_subscription(ses_request) + if subscribed_topic: + current_app.logger.info("Automatically subscribed to topic: {}".format(subscribed_topic)) + return jsonify( + result="success", message="SES callback succeeded" + ), 200 errors = validate_callback_data(data=ses_request, fields=['Message'], client_name=client_name) if errors: diff --git a/app/notifications/utils.py b/app/notifications/utils.py index 5f1443f09..80346c475 100644 --- a/app/notifications/utils.py +++ b/app/notifications/utils.py @@ -16,10 +16,3 @@ def confirm_subscription(confirmation_request): raise e return confirmation_request['TopicArn'] - - -def autoconfirm_subscription(req_json): - if req_json.get('Type') == 'SubscriptionConfirmation': - current_app.logger.info("SNS subscription confirmation url: {}".format(req_json['SubscribeURL'])) - subscribed_topic = confirm_subscription(req_json) - return subscribed_topic diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index e48932302..8b9631b4f 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -12,19 +12,15 @@ from celery.exceptions import Retry from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks from app.celery import tasks +from app.celery.tasks import s3, build_dvla_file, create_dvla_file_contents, update_dvla_job_to_error from app.celery.tasks import ( - s3, - build_dvla_file, - create_dvla_file_contents, - update_dvla_job_to_error, process_job, process_row, send_sms, send_email, persist_letter, get_template_class, - update_job_to_sent_to_dvla, - update_letter_notifications_statuses + update_job_to_sent_to_dvla ) from app.dao import jobs_dao, services_dao from app.models import ( @@ -38,7 +34,6 @@ from app.models import ( Job) from tests.app import load_example_csv -from tests.conftest import set_config from tests.app.conftest import ( sample_service, sample_template, @@ -1076,37 +1071,3 @@ def test_update_dvla_job_to_error(sample_letter_template, sample_letter_job): assert not n.sent_by assert 'error' == Job.query.filter_by(id=sample_letter_job.id).one().job_status - - -def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_api, mocker): - invalid_file = b'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2' - mocker.patch('app.celery.tasks.s3.get_s3_object', return_value=invalid_file) - - with pytest.raises(TypeError): - update_letter_notifications_statuses(filename='foo.txt') - - -def test_update_letter_notifications_statuses_calls_with_correct_bucket_location(notify_api, mocker): - s3_mock = mocker.patch('app.celery.tasks.s3.get_s3_object') - - with set_config(notify_api, 'NOTIFY_EMAIL_DOMAIN', 'foo.bar'): - update_letter_notifications_statuses(filename='foo.txt') - s3_mock.assert_called_with('{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']), 'foo.txt') - - -def test_update_letter_notifications_statuses_builds_updates_list(notify_api, mocker): - valid_file = b'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted' - mocker.patch('app.celery.tasks.s3.get_s3_object', return_value=valid_file) - updates = update_letter_notifications_statuses(filename='foo.txt') - - assert len(updates) == 2 - - assert updates[0].reference == 'ref-foo' - assert updates[0].status == 'Sent' - assert updates[0].page_count == '1' - assert updates[0].cost_threshold == 'Unsorted' - - assert updates[1].reference == 'ref-bar' - assert updates[1].status == 'Sent' - assert updates[1].page_count == '2' - assert updates[1].cost_threshold == 'Sorted' diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 242f18a46..780076b34 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -1,4 +1,3 @@ -import pytest import uuid from datetime import datetime @@ -7,73 +6,21 @@ from flask import json from freezegun import freeze_time import app.celery.tasks -from app.errors import InvalidRequest from app.dao.notifications_dao import ( get_notification_by_id ) from app.models import NotificationStatistics -from tests.app.notifications.test_notifications_ses_callback import ses_confirmation_callback from tests.app.conftest import sample_notification as create_sample_notification -def test_dvla_callback_returns_400_with_invalid_request(client): - data = json.dumps({"foo": "bar"}) +def test_dvla_callback_should_not_need_auth(client): + data = json.dumps({"somekey": "somevalue"}) response = client.post( path='/notifications/letter/dvla', data=data, - headers=[('Content-Type', 'application/json')] - ) - - json_resp = json.loads(response.get_data(as_text=True)) - - assert response.status_code == 400 - - -def test_dvla_callback_autoconfirms_subscription(client, mocker): - autoconfirm_mock = mocker.patch('app.notifications.notifications_letter_callback.autoconfirm_subscription') - - data = ses_confirmation_callback() - response = client.post( - path='/notifications/letter/dvla', - data=data, - headers=[('Content-Type', 'application/json')] - ) + headers=[('Content-Type', 'application/json')]) assert response.status_code == 200 - assert autoconfirm_mock.called - - -def test_dvla_callback_autoconfirm_does_not_call_update_letter_notifications_task(client, mocker): - autoconfirm_mock = mocker.patch('app.notifications.notifications_letter_callback.autoconfirm_subscription') - update_task = \ - mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') - - data = ses_confirmation_callback() - response = client.post( - path='/notifications/letter/dvla', - data=data, - headers=[('Content-Type', 'application/json')] - ) - - assert response.status_code == 200 - assert autoconfirm_mock.called - assert not update_task.called - - -def test_dvla_callback_calls_update_letter_notifications_task(client, mocker): - update_task = \ - mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') - data = _sample_sns_s3_callback() - response = client.post( - path='/notifications/letter/dvla', - data=data, - headers=[('Content-Type', 'application/json')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - - assert response.status_code == 200 - assert update_task.called - update_task.assert_called_with(['bar.txt'], queue='notify') def test_firetext_callback_should_not_need_auth(client, mocker): @@ -511,46 +458,3 @@ def test_firetext_callback_should_record_statsd(client, notify_db, notify_db_ses def get_notification_stats(service_id): return NotificationStatistics.query.filter_by(service_id=service_id).one() - - -def _sample_sns_s3_callback(): - return json.dumps({ - "SigningCertURL": "foo.pem", - "UnsubscribeURL": "bar", - "Signature": "some-signature", - "Type": "Notification", - "Timestamp": "2016-05-03T08:35:12.884Z", - "SignatureVersion": "1", - "MessageId": "6adbfe0a-d610-509a-9c47-af894e90d32d", - "Subject": "Amazon S3 Notification", - "TopicArn": "sample-topic-arn", - "Message": { - "Records": [{ - "eventVersion": "2.0", - "eventSource": "aws:s3", - "awsRegion": "eu-west-1", - "eventTime": "2017-05-03T08:35:12.826Z", - "eventName": "ObjectCreated:Put", - "userIdentity": {"principalId": "some-p-id"}, - "requestParameters": {"sourceIPAddress": "8.8.8.8"}, - "responseElements": {"x-amz-request-id": "some-req-id", "x-amz-id-2": "some-amz-id"}, - "s3": { - "s3SchemaVersion": "1.0", - "configurationId": "some-config-id", - "bucket": { - "name": "some-bucket", - "ownerIdentity": {"principalId": "some-p-id"}, - "arn": "some-bucket-arn" - }, - "object": { - "key": "bar.txt", - "size": 200, - "eTag": "some-etag", - "versionId": "some-v-id", - "sequencer": "some-seq" - } - } - } - ] - } - }) From ee484ec368a68205e4cfc6c32a27634491244878 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 12 May 2017 17:39:15 +0100 Subject: [PATCH 12/24] Add get_s3_file method for use in DVLA processing --- app/aws/s3.py | 11 ++++++++--- app/celery/tasks.py | 25 +++++++++++++++++++++++++ tests/app/aws/test_s3.py | 11 +++++++++++ tests/app/celery/test_tasks.py | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 tests/app/aws/test_s3.py diff --git a/app/aws/s3.py b/app/aws/s3.py index 2aa7aac39..ed206b3a4 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -4,7 +4,12 @@ from flask import current_app FILE_LOCATION_STRUCTURE = 'service-{}-notify/{}.csv' -def get_s3_job_object(bucket_name, file_location): +def get_s3_file(bucket_name, file_location): + s3_file = get_s3_object(bucket_name, file_location) + return s3_file.get()['Body'].read().decode('utf-8') + + +def get_s3_object(bucket_name, file_location): s3 = resource('s3') return s3.Object(bucket_name, file_location) @@ -12,12 +17,12 @@ def get_s3_job_object(bucket_name, file_location): def get_job_from_s3(service_id, job_id): bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME'] file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id) - obj = get_s3_job_object(bucket_name, file_location) + obj = get_s3_object(bucket_name, file_location) return obj.get()['Body'].read().decode('utf-8') def remove_job_from_s3(service_id, job_id): bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME'] file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id) - obj = get_s3_job_object(bucket_name, file_location) + obj = get_s3_object(bucket_name, file_location) return obj.delete() diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 464e3e1be..48923d228 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,6 +1,7 @@ import random from datetime import (datetime) +from collections import namedtuple from flask import current_app from notifications_utils.recipients import ( @@ -354,3 +355,27 @@ def get_template_class(template_type): # since we don't need rendering capabilities (we only need to extract placeholders) both email and letter can # use the same base template return WithSubjectTemplate + + +@notify_celery.task(bind=True, name='update-letter-notifications-statuses') +@statsd(namespace="tasks") +def update_letter_notifications_statuses(self, filename): + bucket_location = '{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']) + response_file = s3.get_s3_file(bucket_location, filename) + + try: + NotificationUpdate = namedtuple('NotificationUpdate', ['reference', 'status', 'page_count', 'cost_threshold']) + notification_updates = [NotificationUpdate(*line.split('|')) for line in response_file.splitlines()] + + except TypeError: + current_app.logger.exception('DVLA response file: {} has an invalid format'.format(filename)) + raise + + else: + if notification_updates: + for update in notification_updates: + current_app.logger.info('DVLA update: {}'.format(str(update))) + # TODO: Update notifications with desired status + return notification_updates + else: + current_app.logger.exception('DVLA response file contained no updates') diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py new file mode 100644 index 000000000..38c9966dd --- /dev/null +++ b/tests/app/aws/test_s3.py @@ -0,0 +1,11 @@ +from app.aws.s3 import get_s3_file + + +def test_get_s3_file_makes_correct_call(sample_service, sample_job, mocker): + get_s3_mock = mocker.patch('app.aws.s3.get_s3_object') + get_s3_file('foo-bucket', 'bar-file.txt') + + get_s3_mock.assert_called_with( + 'foo-bucket', + 'bar-file.txt' + ) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 8b9631b4f..882ac1fcb 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1071,3 +1071,37 @@ def test_update_dvla_job_to_error(sample_letter_template, sample_letter_job): assert not n.sent_by assert 'error' == Job.query.filter_by(id=sample_letter_job.id).one().job_status + + +def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_api, mocker): + invalid_file = 'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2' + mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=invalid_file) + + with pytest.raises(TypeError): + update_letter_notifications_statuses(filename='foo.txt') + + +def test_update_letter_notifications_statuses_calls_with_correct_bucket_location(notify_api, mocker): + s3_mock = mocker.patch('app.celery.tasks.s3.get_s3_object') + + with set_config(notify_api, 'NOTIFY_EMAIL_DOMAIN', 'foo.bar'): + update_letter_notifications_statuses(filename='foo.txt') + s3_mock.assert_called_with('{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']), 'foo.txt') + + +def test_update_letter_notifications_statuses_builds_updates_list(notify_api, mocker): + valid_file = 'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted' + mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) + updates = update_letter_notifications_statuses(filename='foo.txt') + + assert len(updates) == 2 + + assert updates[0].reference == 'ref-foo' + assert updates[0].status == 'Sent' + assert updates[0].page_count == '1' + assert updates[0].cost_threshold == 'Unsorted' + + assert updates[1].reference == 'ref-bar' + assert updates[1].status == 'Sent' + assert updates[1].page_count == '2' + assert updates[1].cost_threshold == 'Sorted' From 4003edfa67ae7ba73fcabc684c664f04642ad5af Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 15 May 2017 11:12:31 +0100 Subject: [PATCH 13/24] Add DVLA callback: * Process SNS callback, trigger the update notifications celery task * Put autoconfirm into its own method and use in callbacks --- .../notifications_letter_callback.py | 66 ++++++++----- .../notifications_ses_callback.py | 17 ++-- app/notifications/utils.py | 7 ++ tests/app/celery/test_tasks.py | 9 +- .../app/notifications/rest/test_callbacks.py | 97 ++++++++++++++++++- 5 files changed, 157 insertions(+), 39 deletions(-) diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index f7c357055..2f758b7ac 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/app/notifications/notifications_letter_callback.py @@ -1,39 +1,57 @@ -from datetime import datetime +from functools import wraps from flask import ( Blueprint, jsonify, request, - current_app, - json + current_app ) -from app import statsd_client -from app.clients.email.aws_ses import get_aws_responses -from app.dao import ( - notifications_dao -) +from app.celery.tasks import update_letter_notifications_statuses +from app.v2.errors import register_errors +from app.notifications.utils import autoconfirm_subscription +from app.schema_validation import validate -from app.notifications.process_client_response import validate_callback_data letter_callback_blueprint = Blueprint('notifications_letter_callback', __name__) - -from app.errors import ( - register_errors, - InvalidRequest -) - register_errors(letter_callback_blueprint) +dvla_sns_callback_schema = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "sns callback received on s3 update", + "type": "object", + "title": "dvla internal sns callback", + "properties": { + "Type": {"enum": ["Notification", "SubscriptionConfirmation"]}, + "MessageId": {"type": "string"}, + "Message": {"type": ["string", "object"]} + }, + "required": ["Type", "MessageId", "Message"] +} + + +def validate_schema(schema): + def decorator(f): + @wraps(f) + def wrapper(*args, **kw): + validate(request.json, schema) + return f(*args, **kw) + return wrapper + return decorator + + @letter_callback_blueprint.route('/notifications/letter/dvla', methods=['POST']) +@validate_schema(dvla_sns_callback_schema) def process_letter_response(): - try: - dvla_request = json.loads(request.data) - current_app.logger.info(dvla_request) - return jsonify( - result="success", message="DVLA callback succeeded" - ), 200 - except ValueError: - error = "DVLA callback failed: invalid json" - raise InvalidRequest(error, status_code=400) + req_json = request.json + if not autoconfirm_subscription(req_json): + # The callback should have one record for an S3 Put Event. + filename = req_json['Message']['Records'][0]['s3']['object']['key'] + current_app.logger.info('Received file from DVLA: {}'.format(filename)) + current_app.logger.info('DVLA callback: Calling task to update letter notifications') + update_letter_notifications_statuses.apply_async([filename], queue='notify') + + return jsonify( + result="success", message="DVLA callback succeeded" + ), 200 diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index ce8dec9ba..e78fb9394 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -13,9 +13,8 @@ from app.clients.email.aws_ses import get_aws_responses from app.dao import ( notifications_dao ) - from app.notifications.process_client_response import validate_callback_data -from app.notifications.utils import confirm_subscription +from app.notifications.utils import autoconfirm_subscription ses_callback_blueprint = Blueprint('notifications_ses_callback', __name__) @@ -32,14 +31,12 @@ def process_ses_response(): try: ses_request = json.loads(request.data) - if ses_request.get('Type') == 'SubscriptionConfirmation': - current_app.logger.info("SNS subscription confirmation url: {}".format(ses_request['SubscribeURL'])) - subscribed_topic = confirm_subscription(ses_request) - if subscribed_topic: - current_app.logger.info("Automatically subscribed to topic: {}".format(subscribed_topic)) - return jsonify( - result="success", message="SES callback succeeded" - ), 200 + subscribed_topic = autoconfirm_subscription(ses_request) + if subscribed_topic: + current_app.logger.info("Automatically subscribed to topic: {}".format(subscribed_topic)) + return jsonify( + result="success", message="SES callback succeeded" + ), 200 errors = validate_callback_data(data=ses_request, fields=['Message'], client_name=client_name) if errors: diff --git a/app/notifications/utils.py b/app/notifications/utils.py index 80346c475..5f1443f09 100644 --- a/app/notifications/utils.py +++ b/app/notifications/utils.py @@ -16,3 +16,10 @@ def confirm_subscription(confirmation_request): raise e return confirmation_request['TopicArn'] + + +def autoconfirm_subscription(req_json): + if req_json.get('Type') == 'SubscriptionConfirmation': + current_app.logger.info("SNS subscription confirmation url: {}".format(req_json['SubscribeURL'])) + subscribed_topic = confirm_subscription(req_json) + return subscribed_topic diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 882ac1fcb..560f68c27 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -12,15 +12,19 @@ from celery.exceptions import Retry from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks from app.celery import tasks -from app.celery.tasks import s3, build_dvla_file, create_dvla_file_contents, update_dvla_job_to_error from app.celery.tasks import ( + s3, + build_dvla_file, + create_dvla_file_contents, + update_dvla_job_to_error, process_job, process_row, send_sms, send_email, persist_letter, get_template_class, - update_job_to_sent_to_dvla + update_job_to_sent_to_dvla, + update_letter_notifications_statuses ) from app.dao import jobs_dao, services_dao from app.models import ( @@ -34,6 +38,7 @@ from app.models import ( Job) from tests.app import load_example_csv +from tests.conftest import set_config from tests.app.conftest import ( sample_service, sample_template, diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 780076b34..a15a607df 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -10,17 +10,65 @@ from app.dao.notifications_dao import ( get_notification_by_id ) from app.models import NotificationStatistics +from tests.app.notifications.test_notifications_ses_callback import ses_confirmation_callback from tests.app.conftest import sample_notification as create_sample_notification -def test_dvla_callback_should_not_need_auth(client): - data = json.dumps({"somekey": "somevalue"}) +def test_dvla_callback_returns_400_with_invalid_request(client): + data = json.dumps({"foo": "bar"}) response = client.post( path='/notifications/letter/dvla', data=data, - headers=[('Content-Type', 'application/json')]) + headers=[('Content-Type', 'application/json')] + ) + + assert response.status_code == 400 + + +def test_dvla_callback_autoconfirms_subscription(client, mocker): + autoconfirm_mock = mocker.patch('app.notifications.notifications_letter_callback.autoconfirm_subscription') + + data = ses_confirmation_callback() + response = client.post( + path='/notifications/letter/dvla', + data=data, + headers=[('Content-Type', 'application/json')] + ) assert response.status_code == 200 + assert autoconfirm_mock.called + + +def test_dvla_callback_autoconfirm_does_not_call_update_letter_notifications_task(client, mocker): + autoconfirm_mock = mocker.patch('app.notifications.notifications_letter_callback.autoconfirm_subscription') + update_task = \ + mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') + + data = ses_confirmation_callback() + response = client.post( + path='/notifications/letter/dvla', + data=data, + headers=[('Content-Type', 'application/json')] + ) + + assert response.status_code == 200 + assert autoconfirm_mock.called + assert not update_task.called + + +def test_dvla_callback_calls_update_letter_notifications_task(client, mocker): + update_task = \ + mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') + data = _sample_sns_s3_callback() + response = client.post( + path='/notifications/letter/dvla', + data=data, + headers=[('Content-Type', 'application/json')] + ) + + assert response.status_code == 200 + assert update_task.called + update_task.assert_called_with(['bar.txt'], queue='notify') def test_firetext_callback_should_not_need_auth(client, mocker): @@ -458,3 +506,46 @@ def test_firetext_callback_should_record_statsd(client, notify_db, notify_db_ses def get_notification_stats(service_id): return NotificationStatistics.query.filter_by(service_id=service_id).one() + + +def _sample_sns_s3_callback(): + return json.dumps({ + "SigningCertURL": "foo.pem", + "UnsubscribeURL": "bar", + "Signature": "some-signature", + "Type": "Notification", + "Timestamp": "2016-05-03T08:35:12.884Z", + "SignatureVersion": "1", + "MessageId": "6adbfe0a-d610-509a-9c47-af894e90d32d", + "Subject": "Amazon S3 Notification", + "TopicArn": "sample-topic-arn", + "Message": { + "Records": [{ + "eventVersion": "2.0", + "eventSource": "aws:s3", + "awsRegion": "eu-west-1", + "eventTime": "2017-05-03T08:35:12.826Z", + "eventName": "ObjectCreated:Put", + "userIdentity": {"principalId": "some-p-id"}, + "requestParameters": {"sourceIPAddress": "8.8.8.8"}, + "responseElements": {"x-amz-request-id": "some-req-id", "x-amz-id-2": "some-amz-id"}, + "s3": { + "s3SchemaVersion": "1.0", + "configurationId": "some-config-id", + "bucket": { + "name": "some-bucket", + "ownerIdentity": {"principalId": "some-p-id"}, + "arn": "some-bucket-arn" + }, + "object": { + "key": "bar.txt", + "size": 200, + "eTag": "some-etag", + "versionId": "some-v-id", + "sequencer": "some-seq" + } + } + } + ] + } + }) From 359c4d2138c4634f24c64441a865a81a6ed36314 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 15 May 2017 12:49:46 +0100 Subject: [PATCH 14/24] add service permissions model + migration script --- app/models.py | 48 ++++++++++++----- .../0083_add_perm_types_and_svc_perm.py | 53 +++++++++++++++++++ 2 files changed, 88 insertions(+), 13 deletions(-) create mode 100644 migrations/versions/0083_add_perm_types_and_svc_perm.py diff --git a/app/models.py b/app/models.py index 1b07a6fc5..aec86d388 100644 --- a/app/models.py +++ b/app/models.py @@ -31,6 +31,18 @@ from app import ( from app.history_meta import Versioned from app.utils import get_utc_time_in_bst +SMS_TYPE = 'sms' +EMAIL_TYPE = 'email' +LETTER_TYPE = 'letter' + +TEMPLATE_TYPES = [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE] + +template_types = db.Enum(*TEMPLATE_TYPES, name='template_type') + +NORMAL = 'normal' +PRIORITY = 'priority' +TEMPLATE_PROCESS_TYPE = [NORMAL, PRIORITY] + def filter_null_value_fields(obj): return dict( @@ -183,6 +195,29 @@ class Service(db.Model, Versioned): ) +INTERNATIONAL_SMS_TYPE = 'international_sms' +INCOMING_SMS_TYPE = 'incoming_sms' + +SERVICE_PERMISSION_TYPES = [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE] + + +class ServicePermissionTypes(db.Model): + __tablename__ = 'service_permission_types' + + name = db.Column(db.String(255), primary_key=True) + + +class ServicePermission(db.Model): + __tablename__ = "service_permissions" + + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), + primary_key=True, index=True, nullable=False) + permission = db.Column(db.String(255), db.ForeignKey('service_permission_types.name'), + index=True, primary_key=True, nullable=False) + created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) + updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + + MOBILE_TYPE = 'mobile' EMAIL_TYPE = 'email' @@ -293,19 +328,6 @@ class TemplateProcessTypes(db.Model): name = db.Column(db.String(255), primary_key=True) -SMS_TYPE = 'sms' -EMAIL_TYPE = 'email' -LETTER_TYPE = 'letter' - -TEMPLATE_TYPES = [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE] - -template_types = db.Enum(*TEMPLATE_TYPES, name='template_type') - -NORMAL = 'normal' -PRIORITY = 'priority' -TEMPLATE_PROCESS_TYPE = [NORMAL, PRIORITY] - - class Template(db.Model): __tablename__ = 'templates' diff --git a/migrations/versions/0083_add_perm_types_and_svc_perm.py b/migrations/versions/0083_add_perm_types_and_svc_perm.py new file mode 100644 index 000000000..485ddef5d --- /dev/null +++ b/migrations/versions/0083_add_perm_types_and_svc_perm.py @@ -0,0 +1,53 @@ +"""empty message + +Revision ID: 0083_add_perm_types_and_svc_perm +Revises: 0082_add_go_live_template +Create Date: 2017-05-12 11:29:32.664811 + +""" + +# revision identifiers, used by Alembic. +revision = '0083_add_perm_types_and_svc_perm' +down_revision = '0082_add_go_live_template' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + service_permission_types = op.create_table('service_permission_types', + sa.Column('name', sa.String(length=255), nullable=False), + sa.PrimaryKeyConstraint('name')) + + op.bulk_insert(service_permission_types, + [ + {'name': x} for x in { + 'letter', + 'email', + 'sms', + 'international_sms', + 'incoming_sms' + } + ]) + + op.create_table('service_permissions', + sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('permission', sa.String(length=255), nullable=False), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=True), + sa.ForeignKeyConstraint(['permission'], ['service_permission_types.name'], ), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), + sa.PrimaryKeyConstraint('service_id', 'permission')) + op.create_index(op.f('ix_service_permissions_permission'), 'service_permissions', ['permission'], unique=False) + op.create_index(op.f('ix_service_permissions_service_id'), 'service_permissions', ['service_id'], unique=False) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_index(op.f('ix_service_permissions_service_id'), table_name='service_permissions') + op.drop_index(op.f('ix_service_permissions_permission'), table_name='service_permissions') + op.drop_table('service_permissions') + op.drop_table('service_permission_types') + # ### end Alembic commands ### From 380bc22f22378c84c1d5336a8de3f58b5b3db0f7 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 15 May 2017 13:44:52 +0100 Subject: [PATCH 15/24] Add relationship for service in ServicePermission --- app/models.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models.py b/app/models.py index aec86d388..ecbc37f28 100644 --- a/app/models.py +++ b/app/models.py @@ -212,6 +212,7 @@ class ServicePermission(db.Model): service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), primary_key=True, index=True, nullable=False) + service = db.relationship('Service') permission = db.Column(db.String(255), db.ForeignKey('service_permission_types.name'), index=True, primary_key=True, nullable=False) created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) From d993e1a43e6920bf56b695179aab714ddbf719c0 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 15 May 2017 15:02:01 +0100 Subject: [PATCH 16/24] Don't send notification if other service attrs changed --- app/service/rest.py | 2 +- tests/app/service/test_rest.py | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/app/service/rest.py b/app/service/rest.py index 6624c8979..db8c1aefa 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -119,7 +119,7 @@ def create_service(): def update_service(service_id): fetched_service = dao_fetch_service_by_id(service_id) # Capture the status change here as Marshmallow changes this later - service_going_live = fetched_service.restricted and not request.get_json().get('restricted') + service_going_live = fetched_service.restricted and not request.get_json().get('restricted', True) current_data = dict(service_schema.dump(fetched_service).data.items()) current_data.update(request.get_json()) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 8fb5e3fc7..1681444b7 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1702,3 +1702,22 @@ def test_update_service_does_not_call_send_notification_for_live_service(sample_ assert resp.status_code == 200 assert not send_notification_mock.called + + +def test_update_service_does_not_call_send_notification_when_restricted_not_changed(sample_service, client, mocker): + send_notification_mock = mocker.patch('app.service.rest.send_notification_to_service_users') + + data = { + "name": 'Name of service' + } + + auth_header = create_authorization_header() + resp = client.post( + 'service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[auth_header], + content_type='application/json' + ) + + assert resp.status_code == 200 + assert not send_notification_mock.called From aa5f8ba443eb8aab644035de86e931938fb94e7e Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 15 May 2017 15:02:16 +0100 Subject: [PATCH 17/24] Refactor tests for clarity --- tests/app/dao/test_services_dao.py | 12 +++++++----- tests/app/service/test_sender.py | 5 ++++- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 435c4935b..89b909431 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -50,7 +50,7 @@ from app.models import ( KEY_TYPE_TEST ) -from tests.app.db import create_user +from tests.app.db import create_user, create_service from tests.app.conftest import ( sample_notification as create_notification, sample_notification_history as create_notification_history, @@ -787,9 +787,11 @@ def test_fetch_monthly_historical_template_stats_for_service_separates_templates assert str(template_two.id) in result.get('2016-04').keys() -def test_dao_fetch_active_users_for_service_returns_active_only(sample_service): - pending_user = create_user(email='foo@bar.com', state='pending') - dao_add_user_to_service(sample_service, pending_user) - users = dao_fetch_active_users_for_service(sample_service.id) +def test_dao_fetch_active_users_for_service_returns_active_only(notify_db, notify_db_session): + active_user = create_user(email='active@foo.com', state='active') + pending_user = create_user(email='pending@foo.com', state='pending') + service = create_service(user=active_user) + dao_add_user_to_service(service, pending_user) + users = dao_fetch_active_users_for_service(service.id) assert len(users) == 1 diff --git a/tests/app/service/test_sender.py b/tests/app/service/test_sender.py index 8f2d7ac07..3223e9e71 100644 --- a/tests/app/service/test_sender.py +++ b/tests/app/service/test_sender.py @@ -77,7 +77,10 @@ def test_send_notification_to_service_users_includes_user_fields_in_personalisat include_user_fields=['name', 'email_address', 'state'] ) - assert persist_mock.mock_calls[0][2]['personalisation'] == { + persist_call = persist_mock.call_args_list[0][1] + + assert len(persist_mock.call_args_list) == 1 + assert persist_call['personalisation'] == { 'name': sample_user.name, 'email_address': sample_user.email_address, 'state': sample_user.state, From 114d4d84d497c1b057b875f019efc831ab7aba6c Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 11 May 2017 15:22:58 +0100 Subject: [PATCH 18/24] Add service permissions DAO and refactor user service permission mock --- app/dao/service_permissions_dao.py | 41 ++++++++++ migrations/README | 8 +- .../0083_add_perm_types_and_svc_perm.py | 8 +- tests/app/conftest.py | 10 +-- tests/app/dao/test_service_permissions_dao.py | 82 +++++++++++++++++++ tests/app/db.py | 14 +++- tests/app/service/test_rest.py | 16 ++-- tests/app/user/test_rest.py | 6 +- tests/conftest.py | 3 +- 9 files changed, 165 insertions(+), 23 deletions(-) create mode 100644 app/dao/service_permissions_dao.py create mode 100644 tests/app/dao/test_service_permissions_dao.py diff --git a/app/dao/service_permissions_dao.py b/app/dao/service_permissions_dao.py new file mode 100644 index 000000000..7ea9eb0d3 --- /dev/null +++ b/app/dao/service_permissions_dao.py @@ -0,0 +1,41 @@ +from sqlalchemy import exc + +from app import db +from app.dao.dao_utils import transactional +from app.models import Service, ServicePermission, SERVICE_PERMISSION_TYPES + + +def dao_fetch_service_permissions(service_id): + return ServicePermission.query.filter( + ServicePermission.service_id == service_id).all() + + +def make_service_permissions_list(service_id, permissions): + arr = [] + for permission in permissions: + if permission not in SERVICE_PERMISSION_TYPES: + raise ValueError("'{}' not of service permission type: {}".format(permission, SERVICE_PERMISSION_TYPES)) + + service_permission = ServicePermission(service_id=service_id, permission=permission) + arr.append(service_permission) + + return arr + + +@transactional +def dao_add_and_commit_service_permissions(service_id, permissions): + service_permissions = make_service_permissions_list(service_id, permissions) + + try: + db.session.add_all(service_permissions) + db.session.commit() + except exc.IntegrityError as e: + if "duplicate key value violates unique constraint" in str(e.orig): + raise ValueError(e.orig) + raise + + +def dao_remove_service_permission(service_id, permission=None): + return ServicePermission.query.filter( + ServicePermission.service_id == service_id, + ServicePermission.permission == permission if permission else None).delete() diff --git a/migrations/README b/migrations/README index 98e4f9c44..6c36a3e0e 100644 --- a/migrations/README +++ b/migrations/README @@ -1 +1,7 @@ -Generic single-database configuration. \ No newline at end of file +Generic single-database configuration. + +python application.py db migration to generate migration script. + +python application.py db upgrade to upgrade db with script. + +python application.py db downgrade to rollback db changes. diff --git a/migrations/versions/0083_add_perm_types_and_svc_perm.py b/migrations/versions/0083_add_perm_types_and_svc_perm.py index 485ddef5d..2bebb273e 100644 --- a/migrations/versions/0083_add_perm_types_and_svc_perm.py +++ b/migrations/versions/0083_add_perm_types_and_svc_perm.py @@ -15,10 +15,10 @@ import sqlalchemy as sa from sqlalchemy.dialects import postgresql def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### - service_permission_types = op.create_table('service_permission_types', - sa.Column('name', sa.String(length=255), nullable=False), - sa.PrimaryKeyConstraint('name')) + ### commands auto generated by Alembic - please adjust! ### + service_permission_types=op.create_table('service_permission_types', + sa.Column('name', sa.String(length=255), nullable=False), + sa.PrimaryKeyConstraint('name')) op.bulk_insert(service_permission_types, [ diff --git a/tests/app/conftest.py b/tests/app/conftest.py index fc9748870..00b4604b1 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -666,11 +666,11 @@ def sample_permission(notify_db, @pytest.fixture(scope='function') -def sample_service_permission(notify_db, - notify_db_session, - service=None, - user=None, - permission="manage_settings"): +def sample_user_service_permission(notify_db, + notify_db_session, + service=None, + user=None, + permission="manage_settings"): if user is None: user = create_user() if service is None: diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py new file mode 100644 index 000000000..b6130f0da --- /dev/null +++ b/tests/app/dao/test_service_permissions_dao.py @@ -0,0 +1,82 @@ +import pytest + +from app.dao.service_permissions_dao import ( + dao_fetch_service_permissions, dao_remove_service_permission) +from app.models import ( + EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE, SERVICE_PERMISSION_TYPES) + +from tests.app.db import create_service_permissions, create_service + + +def test_create_service_permissions(sample_service): + service_permission_types = [SMS_TYPE, INTERNATIONAL_SMS_TYPE] + + service_permissions = create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types) + + assert len(service_permissions) == len(service_permission_types) + assert all(sp.service_id == sample_service.id for sp in service_permissions) + assert all(sp.permission in service_permission_types for sp in service_permissions) + + +def test_fetch_service_permissions_gets_service_permissions(sample_service): + service_permission_types = [LETTER_TYPE, EMAIL_TYPE, SMS_TYPE] + create_service_permissions(service_id=sample_service.id, permissions=service_permission_types) + service_permissions = dao_fetch_service_permissions(sample_service.id) + + assert len(service_permission_types) == len(service_permission_types) + assert all(sp.service_id == sample_service.id for sp in service_permissions) + assert all(sp.permission in service_permission_types for sp in service_permissions) + + +def test_add_service_permissions_to_existing_permissions(sample_service): + service_permission_types_1 = [EMAIL_TYPE, INCOMING_SMS_TYPE] + service_permission_types_2 = [LETTER_TYPE, INTERNATIONAL_SMS_TYPE, SMS_TYPE] + + create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types_1) + create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types_2) + + permissions = dao_fetch_service_permissions(sample_service.id) + + assert len(permissions) == len(service_permission_types_1 + service_permission_types_2) + + +def test_create_invalid_service_permissions_raises_error(sample_service): + service_permission_types = ['invalid'] + + with pytest.raises(ValueError) as e: + service_permissions = create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types) + + assert "'invalid' not of service permission type: " + str(SERVICE_PERMISSION_TYPES) in str(e.value) + + +def test_remove_service_permission(sample_service): + service_permission_types = [EMAIL_TYPE, INCOMING_SMS_TYPE] + service_permission_type_to_remove = EMAIL_TYPE + service_permission_type_remaining = INCOMING_SMS_TYPE + + service_permissions = create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types) + + dao_remove_service_permission(sample_service.id, service_permission_type_to_remove) + + permissions = dao_fetch_service_permissions(sample_service.id) + assert len(permissions) == 1 + assert permissions[0].permission == service_permission_type_remaining + assert permissions[0].service_id == sample_service.id + + +def test_adding_duplicate_service_id_permission_raises_value_error(sample_service): + service_permission_types = [EMAIL_TYPE, INCOMING_SMS_TYPE] + service_permission_types_with_duplicate_email_type = [LETTER_TYPE, EMAIL_TYPE] + + with pytest.raises(ValueError) as e: + create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types) + create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types_with_duplicate_email_type) + + assert "duplicate key value violates unique constraint \"service_permissions_pkey\"" in str(e.value) diff --git a/tests/app/db.py b/tests/app/db.py index b5838d693..c193fd4a9 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -2,11 +2,14 @@ from datetime import datetime import uuid from app.dao.jobs_dao import dao_create_job -from app.models import Service, User, Template, Notification, SMS_TYPE, KEY_TYPE_NORMAL, Job +from app.models import (Service, User, Template, Notification, EMAIL_TYPE, LETTER_TYPE, + SMS_TYPE, KEY_TYPE_NORMAL, Job, ServicePermission) from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification from app.dao.templates_dao import dao_create_template from app.dao.services_dao import dao_create_service +from app.dao.service_permissions_dao import ( + dao_add_and_commit_service_permissions, make_service_permissions_list) def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk"): @@ -141,3 +144,12 @@ def create_job(template, job = Job(**data) dao_create_job(job) return job + + +def create_service_permissions(service_id, permissions=[EMAIL_TYPE, LETTER_TYPE]): + dao_add_and_commit_service_permissions( + service_id if service_id else create_service().id, permissions) + + service_permissions = ServicePermission.query.all() + + return service_permissions diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 91c928751..2f4732473 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -15,7 +15,7 @@ from tests import create_authorization_header from tests.app.db import create_template from tests.app.conftest import ( sample_service as create_service, - sample_service_permission as create_service_permission, + sample_user_service_permission as create_user_service_permission, sample_notification as create_sample_notification, sample_notification_history as create_notification_history, sample_notification_with_job @@ -941,12 +941,12 @@ def test_add_unknown_user_to_service_returns404(notify_api, notify_db, notify_db assert result['message'] == expected_message -def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_service_permission): +def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_user_service_permission): with notify_api.test_request_context(): with notify_api.test_client() as client: second_user = create_user(email="new@digital.cabinet-office.gov.uk") # Simulates successfully adding a user to the service - second_permission = create_service_permission( + second_permission = create_user_service_permission( notify_db, notify_db_session, user=second_user) @@ -961,13 +961,13 @@ def test_remove_user_from_service(notify_api, notify_db, notify_db_session, samp assert resp.status_code == 204 -def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_service_permission): +def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_user_service_permission): with notify_api.test_request_context(): with notify_api.test_client() as client: second_user = create_user(email="new@digital.cabinet-office.gov.uk") endpoint = url_for( 'service.remove_user_from_service', - service_id=str(sample_service_permission.service.id), + service_id=str(sample_user_service_permission.service.id), user_id=str(second_user.id)) auth_header = create_authorization_header() resp = client.delete( @@ -979,13 +979,13 @@ def test_remove_user_from_service(notify_api, notify_db, notify_db_session, samp def test_cannot_remove_only_user_from_service(notify_api, notify_db, notify_db_session, - sample_service_permission): + sample_user_service_permission): with notify_api.test_request_context(): with notify_api.test_client() as client: endpoint = url_for( 'service.remove_user_from_service', - service_id=str(sample_service_permission.service.id), - user_id=str(sample_service_permission.user.id)) + service_id=str(sample_user_service_permission.service.id), + user_id=str(sample_user_service_permission.user.id)) auth_header = create_authorization_header() resp = client.delete( endpoint, diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index e476b2ebb..ddbb3eaae 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -290,13 +290,13 @@ def test_get_user_by_email_bad_url_returns_404(client, sample_user): assert json_resp['message'] == 'Invalid request. Email query string param required' -def test_get_user_with_permissions(client, sample_service_permission): +def test_get_user_with_permissions(client, sample_user_service_permission): header = create_authorization_header() - response = client.get(url_for('user.get_user', user_id=str(sample_service_permission.user.id)), + response = client.get(url_for('user.get_user', user_id=str(sample_user_service_permission.user.id)), headers=[header]) assert response.status_code == 200 permissions = json.loads(response.get_data(as_text=True))['data']['permissions'] - assert sample_service_permission.permission in permissions[str(sample_service_permission.service.id)] + assert sample_user_service_permission.permission in permissions[str(sample_user_service_permission.service.id)] def test_set_user_permissions(client, sample_user, sample_service): diff --git a/tests/conftest.py b/tests/conftest.py index 61cd17ce9..bf9823331 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -77,7 +77,8 @@ def notify_db_session(notify_db): "provider_details_history", "template_process_type", "dvla_organisation", - "notification_status_types"]: + "notification_status_types", + "service_permission_types"]: notify_db.engine.execute(tbl.delete()) notify_db.session.commit() From e6db9ffc1b2057a4c21f034dce2284ed8cb7cb04 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 16 May 2017 10:29:27 +0100 Subject: [PATCH 19/24] Force parse JSON received from SNS: * An SNS callback containing JSON has a plaintext header set. Using * request.get_json() will return None if the header is not * application/json unless the force parameter is set to True --- app/notifications/notifications_letter_callback.py | 5 +++-- tests/app/notifications/rest/test_callbacks.py | 12 ++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index 2f758b7ac..1de6c0009 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/app/notifications/notifications_letter_callback.py @@ -35,7 +35,7 @@ def validate_schema(schema): def decorator(f): @wraps(f) def wrapper(*args, **kw): - validate(request.json, schema) + validate(request.get_json(force=True), schema) return f(*args, **kw) return wrapper return decorator @@ -44,7 +44,8 @@ def validate_schema(schema): @letter_callback_blueprint.route('/notifications/letter/dvla', methods=['POST']) @validate_schema(dvla_sns_callback_schema) def process_letter_response(): - req_json = request.json + req_json = request.get_json(force=True) + current_app.logger.info('Received SNS callback: {}'.format(req_json)) if not autoconfirm_subscription(req_json): # The callback should have one record for an S3 Put Event. filename = req_json['Message']['Records'][0]['s3']['object']['key'] diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index a15a607df..5c8f96baa 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -71,6 +71,18 @@ def test_dvla_callback_calls_update_letter_notifications_task(client, mocker): update_task.assert_called_with(['bar.txt'], queue='notify') +def test_dvla_callback_does_not_raise_error_parsing_json_for_plaintext_header(client, mocker): + mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') + data = _sample_sns_s3_callback() + response = client.post( + path='/notifications/letter/dvla', + data=data, + headers=[('Content-Type', 'text/plain')] + ) + + assert response.status_code == 200 + + def test_firetext_callback_should_not_need_auth(client, mocker): mocker.patch('app.statsd_client.incr') response = client.post( From 54d801979caf0bb0c25711b2b29e4df3e586b416 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 10:57:57 +0100 Subject: [PATCH 20/24] Refactored to handle single service permission --- app/dao/service_permissions_dao.py | 27 ++----- tests/app/dao/test_service_permissions_dao.py | 71 ++++++------------- tests/app/db.py | 9 ++- 3 files changed, 32 insertions(+), 75 deletions(-) diff --git a/app/dao/service_permissions_dao.py b/app/dao/service_permissions_dao.py index 7ea9eb0d3..26b1f6f9e 100644 --- a/app/dao/service_permissions_dao.py +++ b/app/dao/service_permissions_dao.py @@ -10,29 +10,14 @@ def dao_fetch_service_permissions(service_id): ServicePermission.service_id == service_id).all() -def make_service_permissions_list(service_id, permissions): - arr = [] - for permission in permissions: - if permission not in SERVICE_PERMISSION_TYPES: - raise ValueError("'{}' not of service permission type: {}".format(permission, SERVICE_PERMISSION_TYPES)) - - service_permission = ServicePermission(service_id=service_id, permission=permission) - arr.append(service_permission) - - return arr - - @transactional -def dao_add_and_commit_service_permissions(service_id, permissions): - service_permissions = make_service_permissions_list(service_id, permissions) +def dao_add_and_commit_service_permission(service_id, permission): + if permission not in SERVICE_PERMISSION_TYPES: + raise ValueError("'{}' not of service permission type: {}".format(permission, SERVICE_PERMISSION_TYPES)) - try: - db.session.add_all(service_permissions) - db.session.commit() - except exc.IntegrityError as e: - if "duplicate key value violates unique constraint" in str(e.orig): - raise ValueError(e.orig) - raise + service_permission = ServicePermission(service_id=service_id, permission=permission) + + db.session.add(service_permission) def dao_remove_service_permission(service_id, permission=None): diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index b6130f0da..cf3768306 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -5,61 +5,47 @@ from app.dao.service_permissions_dao import ( from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE, SERVICE_PERMISSION_TYPES) -from tests.app.db import create_service_permissions, create_service +from tests.app.db import create_service_permission, create_service -def test_create_service_permissions(sample_service): - service_permission_types = [SMS_TYPE, INTERNATIONAL_SMS_TYPE] +def test_create_service_permission(sample_service): + service_permission_type = SMS_TYPE - service_permissions = create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types) + service_permission = create_service_permission( + service_id=sample_service.id, permission=service_permission_type) + + assert len(service_permission) == 1 + assert all(sp.service_id == sample_service.id for sp in service_permission) + assert all(sp.permission in service_permission_type for sp in service_permission) + + +def test_fetch_service_permissions_gets_service_permissions(sample_service): + service_permission_types = [LETTER_TYPE, EMAIL_TYPE, SMS_TYPE] + for spt in service_permission_types: + create_service_permission(service_id=sample_service.id, permission=spt) + service_permissions = dao_fetch_service_permissions(sample_service.id) assert len(service_permissions) == len(service_permission_types) assert all(sp.service_id == sample_service.id for sp in service_permissions) assert all(sp.permission in service_permission_types for sp in service_permissions) -def test_fetch_service_permissions_gets_service_permissions(sample_service): - service_permission_types = [LETTER_TYPE, EMAIL_TYPE, SMS_TYPE] - create_service_permissions(service_id=sample_service.id, permissions=service_permission_types) - service_permissions = dao_fetch_service_permissions(sample_service.id) - - assert len(service_permission_types) == len(service_permission_types) - assert all(sp.service_id == sample_service.id for sp in service_permissions) - assert all(sp.permission in service_permission_types for sp in service_permissions) - - -def test_add_service_permissions_to_existing_permissions(sample_service): - service_permission_types_1 = [EMAIL_TYPE, INCOMING_SMS_TYPE] - service_permission_types_2 = [LETTER_TYPE, INTERNATIONAL_SMS_TYPE, SMS_TYPE] - - create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types_1) - create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types_2) - - permissions = dao_fetch_service_permissions(sample_service.id) - - assert len(permissions) == len(service_permission_types_1 + service_permission_types_2) - - def test_create_invalid_service_permissions_raises_error(sample_service): - service_permission_types = ['invalid'] + service_permission_type = 'invalid' with pytest.raises(ValueError) as e: - service_permissions = create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types) + create_service_permission(service_id=sample_service.id, permission=service_permission_type) - assert "'invalid' not of service permission type: " + str(SERVICE_PERMISSION_TYPES) in str(e.value) + assert "'invalid' not of service permission type: {}".format(str(SERVICE_PERMISSION_TYPES)) in str(e.value) def test_remove_service_permission(sample_service): - service_permission_types = [EMAIL_TYPE, INCOMING_SMS_TYPE] + service_permission_types_to_create = [EMAIL_TYPE, INCOMING_SMS_TYPE] service_permission_type_to_remove = EMAIL_TYPE service_permission_type_remaining = INCOMING_SMS_TYPE - service_permissions = create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types) + for spt in service_permission_types_to_create: + create_service_permission(service_id=sample_service.id, permission=spt) dao_remove_service_permission(sample_service.id, service_permission_type_to_remove) @@ -67,16 +53,3 @@ def test_remove_service_permission(sample_service): assert len(permissions) == 1 assert permissions[0].permission == service_permission_type_remaining assert permissions[0].service_id == sample_service.id - - -def test_adding_duplicate_service_id_permission_raises_value_error(sample_service): - service_permission_types = [EMAIL_TYPE, INCOMING_SMS_TYPE] - service_permission_types_with_duplicate_email_type = [LETTER_TYPE, EMAIL_TYPE] - - with pytest.raises(ValueError) as e: - create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types) - create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types_with_duplicate_email_type) - - assert "duplicate key value violates unique constraint \"service_permissions_pkey\"" in str(e.value) diff --git a/tests/app/db.py b/tests/app/db.py index c193fd4a9..e4cded3cc 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -8,8 +8,7 @@ from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification from app.dao.templates_dao import dao_create_template from app.dao.services_dao import dao_create_service -from app.dao.service_permissions_dao import ( - dao_add_and_commit_service_permissions, make_service_permissions_list) +from app.dao.service_permissions_dao import dao_add_and_commit_service_permission def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk"): @@ -146,9 +145,9 @@ def create_job(template, return job -def create_service_permissions(service_id, permissions=[EMAIL_TYPE, LETTER_TYPE]): - dao_add_and_commit_service_permissions( - service_id if service_id else create_service().id, permissions) +def create_service_permission(service_id, permission=EMAIL_TYPE): + dao_add_and_commit_service_permission( + service_id if service_id else create_service().id, permission) service_permissions = ServicePermission.query.all() From 733c16b2bbf20247947565c11594e685326da1f5 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 12:33:27 +0100 Subject: [PATCH 21/24] Update to strip down DAO and clarify tests --- app/dao/service_permissions_dao.py | 13 ++---- tests/app/conftest.py | 8 ++-- tests/app/dao/test_service_permissions_dao.py | 42 +++++++------------ tests/app/db.py | 4 +- 4 files changed, 23 insertions(+), 44 deletions(-) diff --git a/app/dao/service_permissions_dao.py b/app/dao/service_permissions_dao.py index 26b1f6f9e..5c38ad6a2 100644 --- a/app/dao/service_permissions_dao.py +++ b/app/dao/service_permissions_dao.py @@ -1,8 +1,6 @@ -from sqlalchemy import exc - from app import db from app.dao.dao_utils import transactional -from app.models import Service, ServicePermission, SERVICE_PERMISSION_TYPES +from app.models import ServicePermission, SERVICE_PERMISSION_TYPES def dao_fetch_service_permissions(service_id): @@ -11,16 +9,13 @@ def dao_fetch_service_permissions(service_id): @transactional -def dao_add_and_commit_service_permission(service_id, permission): - if permission not in SERVICE_PERMISSION_TYPES: - raise ValueError("'{}' not of service permission type: {}".format(permission, SERVICE_PERMISSION_TYPES)) - +def dao_create_service_permission(service_id, permission): service_permission = ServicePermission(service_id=service_id, permission=permission) db.session.add(service_permission) -def dao_remove_service_permission(service_id, permission=None): +def dao_remove_service_permission(service_id, permission): return ServicePermission.query.filter( ServicePermission.service_id == service_id, - ServicePermission.permission == permission if permission else None).delete() + ServicePermission.permission == permission).delete() diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 00b4604b1..c10c053cb 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -666,11 +666,9 @@ def sample_permission(notify_db, @pytest.fixture(scope='function') -def sample_user_service_permission(notify_db, - notify_db_session, - service=None, - user=None, - permission="manage_settings"): +def sample_user_service_permission( + notify_db, notify_db_session, service=None, user=None, permission="manage_settings" +): if user is None: user = create_user() if service is None: diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index cf3768306..248dd6ae6 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -5,51 +5,37 @@ from app.dao.service_permissions_dao import ( from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE, SERVICE_PERMISSION_TYPES) -from tests.app.db import create_service_permission, create_service +from tests.app.db import create_service_permission def test_create_service_permission(sample_service): - service_permission_type = SMS_TYPE - service_permission = create_service_permission( - service_id=sample_service.id, permission=service_permission_type) + service_id=sample_service.id, permission=SMS_TYPE) assert len(service_permission) == 1 - assert all(sp.service_id == sample_service.id for sp in service_permission) - assert all(sp.permission in service_permission_type for sp in service_permission) + assert service_permission[0].service_id == sample_service.id + assert service_permission[0].permission == SMS_TYPE def test_fetch_service_permissions_gets_service_permissions(sample_service): - service_permission_types = [LETTER_TYPE, EMAIL_TYPE, SMS_TYPE] - for spt in service_permission_types: - create_service_permission(service_id=sample_service.id, permission=spt) + create_service_permission(service_id=sample_service.id, permission=LETTER_TYPE) + create_service_permission(service_id=sample_service.id, permission=INTERNATIONAL_SMS_TYPE) + create_service_permission(service_id=sample_service.id, permission=SMS_TYPE) + service_permissions = dao_fetch_service_permissions(sample_service.id) - assert len(service_permissions) == len(service_permission_types) + assert len(service_permissions) == 3 assert all(sp.service_id == sample_service.id for sp in service_permissions) - assert all(sp.permission in service_permission_types for sp in service_permissions) - - -def test_create_invalid_service_permissions_raises_error(sample_service): - service_permission_type = 'invalid' - - with pytest.raises(ValueError) as e: - create_service_permission(service_id=sample_service.id, permission=service_permission_type) - - assert "'invalid' not of service permission type: {}".format(str(SERVICE_PERMISSION_TYPES)) in str(e.value) + assert all(sp.permission in [LETTER_TYPE, INTERNATIONAL_SMS_TYPE, SMS_TYPE] for sp in service_permissions) def test_remove_service_permission(sample_service): - service_permission_types_to_create = [EMAIL_TYPE, INCOMING_SMS_TYPE] - service_permission_type_to_remove = EMAIL_TYPE - service_permission_type_remaining = INCOMING_SMS_TYPE + create_service_permission(service_id=sample_service.id, permission=EMAIL_TYPE) + create_service_permission(service_id=sample_service.id, permission=INCOMING_SMS_TYPE) - for spt in service_permission_types_to_create: - create_service_permission(service_id=sample_service.id, permission=spt) - - dao_remove_service_permission(sample_service.id, service_permission_type_to_remove) + dao_remove_service_permission(sample_service.id, EMAIL_TYPE) permissions = dao_fetch_service_permissions(sample_service.id) assert len(permissions) == 1 - assert permissions[0].permission == service_permission_type_remaining + assert permissions[0].permission == INCOMING_SMS_TYPE assert permissions[0].service_id == sample_service.id diff --git a/tests/app/db.py b/tests/app/db.py index e4cded3cc..228d49ec4 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -8,7 +8,7 @@ from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification from app.dao.templates_dao import dao_create_template from app.dao.services_dao import dao_create_service -from app.dao.service_permissions_dao import dao_add_and_commit_service_permission +from app.dao.service_permissions_dao import dao_create_service_permission def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk"): @@ -146,7 +146,7 @@ def create_job(template, def create_service_permission(service_id, permission=EMAIL_TYPE): - dao_add_and_commit_service_permission( + dao_create_service_permission( service_id if service_id else create_service().id, permission) service_permissions = ServicePermission.query.all() From b233ae46f3899680fbfb0b641353e27076acde64 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 12:53:46 +0100 Subject: [PATCH 22/24] Tidy up test code for service permissions --- tests/app/dao/test_service_permissions_dao.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index 248dd6ae6..8005e2af2 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -1,20 +1,17 @@ import pytest -from app.dao.service_permissions_dao import ( - dao_fetch_service_permissions, dao_remove_service_permission) -from app.models import ( - EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE, SERVICE_PERMISSION_TYPES) +from app.dao.service_permissions_dao import dao_fetch_service_permissions, dao_remove_service_permission +from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE from tests.app.db import create_service_permission def test_create_service_permission(sample_service): - service_permission = create_service_permission( - service_id=sample_service.id, permission=SMS_TYPE) + service_permissions = create_service_permission(service_id=sample_service.id, permission=SMS_TYPE) - assert len(service_permission) == 1 - assert service_permission[0].service_id == sample_service.id - assert service_permission[0].permission == SMS_TYPE + assert len(service_permissions) == 1 + assert service_permissions[0].service_id == sample_service.id + assert service_permissions[0].permission == SMS_TYPE def test_fetch_service_permissions_gets_service_permissions(sample_service): From 2a488910255848eb0559fad7986b1faac29dd807 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 12:54:32 +0100 Subject: [PATCH 23/24] Removed unused pytest from test --- tests/app/dao/test_service_permissions_dao.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index 8005e2af2..a098d1b0f 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -1,5 +1,3 @@ -import pytest - from app.dao.service_permissions_dao import dao_fetch_service_permissions, dao_remove_service_permission from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE From 3602431c2a7212c8c1b89cfe5d2c5c598fa743fe Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 13:41:54 +0100 Subject: [PATCH 24/24] Renamed test and refactored fixtures --- tests/app/service/test_rest.py | 62 +++++++++++++++++----------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 2f4732473..93621668f 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -941,39 +941,39 @@ def test_add_unknown_user_to_service_returns404(notify_api, notify_db, notify_db assert result['message'] == expected_message -def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_user_service_permission): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - second_user = create_user(email="new@digital.cabinet-office.gov.uk") - # Simulates successfully adding a user to the service - second_permission = create_user_service_permission( - notify_db, - notify_db_session, - user=second_user) - endpoint = url_for( - 'service.remove_user_from_service', - service_id=str(second_permission.service.id), - user_id=str(second_permission.user.id)) - auth_header = create_authorization_header() - resp = client.delete( - endpoint, - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 204 +def test_remove_user_from_service( + notify_db, notify_db_session, client, sample_user_service_permission +): + second_user = create_user(email="new@digital.cabinet-office.gov.uk") + # Simulates successfully adding a user to the service + second_permission = create_user_service_permission( + notify_db, + notify_db_session, + user=second_user) + endpoint = url_for( + 'service.remove_user_from_service', + service_id=str(second_permission.service.id), + user_id=str(second_permission.user.id)) + auth_header = create_authorization_header() + resp = client.delete( + endpoint, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 204 -def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_user_service_permission): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - second_user = create_user(email="new@digital.cabinet-office.gov.uk") - endpoint = url_for( - 'service.remove_user_from_service', - service_id=str(sample_user_service_permission.service.id), - user_id=str(second_user.id)) - auth_header = create_authorization_header() - resp = client.delete( - endpoint, - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 404 +def test_remove_non_existant_user_from_service( + client, sample_user_service_permission +): + second_user = create_user(email="new@digital.cabinet-office.gov.uk") + endpoint = url_for( + 'service.remove_user_from_service', + service_id=str(sample_user_service_permission.service.id), + user_id=str(second_user.id)) + auth_header = create_authorization_header() + resp = client.delete( + endpoint, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 404 def test_cannot_remove_only_user_from_service(notify_api,