From 048861b9680ced6b2fac235d3882eb769edc35fb Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 10 May 2017 15:58:44 +0100 Subject: [PATCH 1/5] 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 2/5] 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 3/5] 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 d993e1a43e6920bf56b695179aab714ddbf719c0 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 15 May 2017 15:02:01 +0100 Subject: [PATCH 4/5] 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 5/5] 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,