From 07bb0f0332e1ff271f7db889a801bb8eaa01f919 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 9 Jul 2019 11:59:33 +0100 Subject: [PATCH 1/3] send emails when MOU is signed we build up one personalisation dict, and then pass it in to all the different templates - so be careful editing things. also of note, we check if the agreement_signed_on_behalf_of is set, and send a different template with slightly different wording to the person who clicked the confirm button. --- app/config.py | 4 +++ app/models.py | 1 + app/organisation/rest.py | 73 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 77 insertions(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index 2c5ee54ae..0b074e8c6 100644 --- a/app/config.py +++ b/app/config.py @@ -151,6 +151,10 @@ class Config(object): TEAM_MEMBER_EDIT_EMAIL_TEMPLATE_ID = 'c73f1d71-4049-46d5-a647-d013bdeca3f0' TEAM_MEMBER_EDIT_MOBILE_TEMPLATE_ID = '8a31520f-4751-4789-8ea1-fe54496725eb' REPLY_TO_EMAIL_ADDRESS_VERIFICATION_TEMPLATE_ID = 'a42f1d17-9404-46d5-a647-d013bdfca3e1' + MOU_SIGNER_RECEIPT_TEMPLATE_ID = '4fd2e43c-309b-4e50-8fb8-1955852d9d71' + MOU_SIGNED_ON_BEHALF_SIGNER_RECEIPT_TEMPLATE_ID = 'c20206d5-bf03-4002-9a90-37d5032d9e84' + MOU_SIGNED_ON_BEHALF_ON_BEHALF_RECEIPT_TEMPLATE_ID = '522b6657-5ca5-4368-a294-6b527703bd0b' + MOU_NOTIFY_TEAM_ALERT_TEMPLATE_ID = 'd0e66c4c-0c50-43f0-94f5-f85b613202d4' BROKER_URL = 'sqs://' BROKER_TRANSPORT_OPTIONS = { diff --git a/app/models.py b/app/models.py index a46c51980..3db41ff6d 100644 --- a/app/models.py +++ b/app/models.py @@ -346,6 +346,7 @@ class Organisation(db.Model): db.ForeignKey('users.id'), nullable=True, ) + agreement_signed_by = db.relationship('User') agreement_signed_on_behalf_of_name = db.Column(db.String(255), nullable=True) agreement_signed_on_behalf_of_email_address = db.Column(db.String(255), nullable=True) agreement_signed_version = db.Column(db.Float, nullable=True) diff --git a/app/organisation/rest.py b/app/organisation/rest.py index 6296e63dd..fd63698cc 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -1,6 +1,7 @@ from flask import abort, Blueprint, jsonify, request, current_app from sqlalchemy.exc import IntegrityError +from app.config import QueueNames from app.dao.organisation_dao import ( dao_create_organisation, dao_get_organisations, @@ -12,9 +13,11 @@ from app.dao.organisation_dao import ( dao_get_users_for_organisation, dao_add_user_to_organisation ) +from app.dao.templates_dao import dao_get_template_by_id from app.dao.services_dao import dao_fetch_service_by_id from app.errors import register_errors, InvalidRequest -from app.models import Organisation +from app.models import Organisation, KEY_TYPE_NORMAL +from app.notifications.process_notifications import persist_notification, send_notification_to_queue from app.organisation.organisation_schema import ( post_create_organisation_schema, post_update_organisation_schema, @@ -91,6 +94,10 @@ def update_organisation(organisation_id): data = request.get_json() validate(data, post_update_organisation_schema) result = dao_update_organisation(organisation_id, **data) + + if data.get('agreement_signed') is True: + send_notifications_on_mou_signed(organisation_id) + if result: return '', 204 else: @@ -149,3 +156,67 @@ def check_request_args(request): if errors: raise InvalidRequest(errors, status_code=400) return org_id, name + + +def send_notifications_on_mou_signed(organisation_id): + notify_service = dao_fetch_service_by_id(current_app.config['NOTIFY_SERVICE_ID']) + + def _send_notification(template_id, recipient, personalisation): + template = dao_get_template_by_id(template_id) + + saved_notification = persist_notification( + template_id=template.id, + template_version=template.version, + recipient=recipient, + service=notify_service, + personalisation=personalisation, + notification_type=template.template_type, + api_key_id=None, + key_type=KEY_TYPE_NORMAL, + reply_to_text=notify_service.get_default_reply_to_email_address() + ) + + send_notification_to_queue(saved_notification, research_mode=False, queue=QueueNames.NOTIFY) + + organisation = dao_get_organisation_by_id(organisation_id) + personalisation = { + 'mou_link': '{}/agreement/{}.pdf'.format( + current_app.config['ADMIN_BASE_URL'], + 'crown' if organisation.crown else 'non-crown' + ), + 'org_name': organisation.name, + 'org_dashboard_link': '{}/organisations/{}'.format( + current_app.config['ADMIN_BASE_URL'], + organisation.id + ), + 'signed_by_name': organisation.agreement_signed_by.name, + 'on_behalf_of_name': organisation.agreement_signed_on_behalf_of_name + } + + # let notify team know something's happened + _send_notification( + current_app.config['MOU_NOTIFY_TEAM_ALERT_TEMPLATE_ID'], + + # TODO: decide should we do this to distinguish emails from different envs? + 'notify-support+{}@digital.cabinet-office.gov.uk'.format(current_app.config['NOTIFY_ENVIRONMENT']), + personalisation + ) + + if not organisation.agreement_signed_on_behalf_of_email_address: + signer_template_id = 'MOU_SIGNER_RECEIPT_TEMPLATE_ID' + else: + signer_template_id = 'MOU_SIGNED_ON_BEHALF_SIGNER_RECEIPT_TEMPLATE_ID' + + # let the person who has been signed on behalf of know. + _send_notification( + current_app.config['MOU_SIGNED_ON_BEHALF_ON_BEHALF_RECEIPT_TEMPLATE_ID'], + organisation.agreement_signed_on_behalf_of_email_address, + personalisation + ) + + # let the person who signed know - the template is different depending on if they signed on behalf of someone + _send_notification( + current_app.config[signer_template_id], + organisation.agreement_signed_by.email_address, + personalisation + ) From 8e32995c8d38e964dfca4cc1fdcd6a9fdfe010e7 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 4 Jul 2019 15:12:08 +0100 Subject: [PATCH 2/3] only send emails if a user signed the MOU the agreement_signed field can also be edited by a platform admin - if that happened we might not have details for who signed it, and even if we did they shouldn't find out about, and we don't need an email since we were the ones who clicked the button. the `agreement_signed_by` field is only set when a user confirms that they are signing the MOU on the admin page - not if a platform admin modifies the page from the platform admin page --- app/dao/organisation_dao.py | 4 ++-- app/organisation/rest.py | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py index a667661c2..3f17823c0 100644 --- a/app/dao/organisation_dao.py +++ b/app/dao/organisation_dao.py @@ -64,7 +64,7 @@ def dao_update_organisation(organisation_id, **kwargs): domains = kwargs.pop('domains', None) - organisation = Organisation.query.filter_by(id=organisation_id).update( + num_updated = Organisation.query.filter_by(id=organisation_id).update( kwargs ) @@ -79,7 +79,7 @@ def dao_update_organisation(organisation_id, **kwargs): db.session.commit() - return organisation + return num_updated @transactional diff --git a/app/organisation/rest.py b/app/organisation/rest.py index fd63698cc..1d3da458b 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -96,7 +96,9 @@ def update_organisation(organisation_id): result = dao_update_organisation(organisation_id, **data) if data.get('agreement_signed') is True: - send_notifications_on_mou_signed(organisation_id) + # if a platform admin has manually adjusted the organisation, don't tell people + if data.get('agreement_signed_by_id'): + send_notifications_on_mou_signed(organisation_id) if result: return '', 204 @@ -159,6 +161,7 @@ def check_request_args(request): def send_notifications_on_mou_signed(organisation_id): + organisation = dao_get_organisation_by_id(organisation_id) notify_service = dao_fetch_service_by_id(current_app.config['NOTIFY_SERVICE_ID']) def _send_notification(template_id, recipient, personalisation): @@ -178,7 +181,6 @@ def send_notifications_on_mou_signed(organisation_id): send_notification_to_queue(saved_notification, research_mode=False, queue=QueueNames.NOTIFY) - organisation = dao_get_organisation_by_id(organisation_id) personalisation = { 'mou_link': '{}/agreement/{}.pdf'.format( current_app.config['ADMIN_BASE_URL'], From a0c0dec42924ff2478dca275f499adaaf0914ce8 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 8 Jul 2019 12:33:53 +0100 Subject: [PATCH 3/3] add tests include dynamically loading the template contents from the alembic upgrade file itself --- app/organisation/rest.py | 3 - tests/app/conftest.py | 29 ++++++++++ tests/app/organisation/test_rest.py | 86 +++++++++++++++++++++++++---- 3 files changed, 103 insertions(+), 15 deletions(-) diff --git a/app/organisation/rest.py b/app/organisation/rest.py index 1d3da458b..cbc405025 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -178,7 +178,6 @@ def send_notifications_on_mou_signed(organisation_id): key_type=KEY_TYPE_NORMAL, reply_to_text=notify_service.get_default_reply_to_email_address() ) - send_notification_to_queue(saved_notification, research_mode=False, queue=QueueNames.NOTIFY) personalisation = { @@ -198,8 +197,6 @@ def send_notifications_on_mou_signed(organisation_id): # let notify team know something's happened _send_notification( current_app.config['MOU_NOTIFY_TEAM_ALERT_TEMPLATE_ID'], - - # TODO: decide should we do this to distinguish emails from different envs? 'notify-support+{}@digital.cabinet-office.gov.uk'.format(current_app.config['NOTIFY_ENVIRONMENT']), personalisation ) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 56ba4e099..2ed53996b 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -1001,6 +1001,35 @@ def change_email_confirmation_template(notify_db, return template +@pytest.fixture(scope='function') +def mou_signed_templates(notify_db, notify_db_session): + service, user = notify_service(notify_db, notify_db_session) + import importlib + alembic_script = importlib.import_module('migrations.versions.0298_add_mou_signed_receipt') + + return { + config_name: create_custom_template( + service, + user, + config_name, + 'email', + content='\n'.join( + next( + x + for x in alembic_script.templates + if x['id'] == current_app.config[config_name] + )['content_lines'] + ), + ) + for config_name in [ + 'MOU_SIGNER_RECEIPT_TEMPLATE_ID', + 'MOU_SIGNED_ON_BEHALF_SIGNER_RECEIPT_TEMPLATE_ID', + 'MOU_SIGNED_ON_BEHALF_ON_BEHALF_RECEIPT_TEMPLATE_ID', + 'MOU_NOTIFY_TEAM_ALERT_TEMPLATE_ID', + ] + } + + def create_custom_template(service, user, template_config_name, template_type, content='', subject=None): template = Template.query.get(current_app.config[template_config_name]) if not template: diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index e3bef936c..d016a67f7 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -222,29 +222,21 @@ def test_post_create_organisation_with_missing_name_gives_validation_error( assert response['errors'][0]['message'] == expected_error -@pytest.mark.parametrize('agreement_signed', ( - None, True, False -)) @pytest.mark.parametrize('crown', ( None, True, False )) def test_post_update_organisation_updates_fields( admin_request, notify_db_session, - agreement_signed, crown, ): org = create_organisation() data = { 'name': 'new organisation name', 'active': False, - 'agreement_signed': agreement_signed, 'crown': crown, - 'agreement_signed_on_behalf_of_name': 'Firstname Lastname', - 'agreement_signed_on_behalf_of_email_address': 'test@example.com', 'organisation_type': 'central', } - assert org.agreement_signed is None assert org.crown is None admin_request.post( @@ -260,11 +252,8 @@ def test_post_update_organisation_updates_fields( assert organisation[0].id == org.id assert organisation[0].name == data['name'] assert organisation[0].active == data['active'] - assert organisation[0].agreement_signed == agreement_signed assert organisation[0].crown == crown assert organisation[0].domains == [] - assert organisation[0].agreement_signed_on_behalf_of_name == 'Firstname Lastname' - assert organisation[0].agreement_signed_on_behalf_of_email_address == 'test@example.com' assert organisation[0].organisation_type == 'central' @@ -308,7 +297,7 @@ def test_update_other_organisation_attributes_doesnt_clear_domains( admin_request.post( 'organisation.update_organisation', _data={ - 'agreement_signed': True, + 'crown': True, }, organisation_id=org.id, _expected_status=204 @@ -398,6 +387,79 @@ def test_post_update_organisation_returns_400_if_domain_is_duplicate(admin_reque assert response['message'] == 'Domain already exists' +def test_post_update_organisation_set_mou_doesnt_email_if_no_signed_by( + sample_organisation, + admin_request, + mocker +): + queue_mock = mocker.patch('app.organisation.rest.send_notification_to_queue') + + data = {'agreement_signed': True} + + admin_request.post( + 'organisation.update_organisation', + _data=data, + organisation_id=sample_organisation.id, + _expected_status=204 + ) + + assert queue_mock.called is False + + +@pytest.mark.parametrize('on_behalf_of_name, on_behalf_of_email_address, templates_and_recipients', [ + ( + None, + None, + { + 'MOU_NOTIFY_TEAM_ALERT_TEMPLATE_ID': 'notify-support+test@digital.cabinet-office.gov.uk', + 'MOU_SIGNER_RECEIPT_TEMPLATE_ID': 'notify@digital.cabinet-office.gov.uk', + } + ), + ( + 'Important Person', + 'important@person.com', + { + 'MOU_NOTIFY_TEAM_ALERT_TEMPLATE_ID': 'notify-support+test@digital.cabinet-office.gov.uk', + 'MOU_SIGNED_ON_BEHALF_ON_BEHALF_RECEIPT_TEMPLATE_ID': 'important@person.com', + 'MOU_SIGNED_ON_BEHALF_SIGNER_RECEIPT_TEMPLATE_ID': 'notify@digital.cabinet-office.gov.uk', + } + ), +]) +def test_post_update_organisation_set_mou_emails_signed_by( + sample_organisation, + admin_request, + mou_signed_templates, + mocker, + sample_user, + on_behalf_of_name, + on_behalf_of_email_address, + templates_and_recipients +): + queue_mock = mocker.patch('app.organisation.rest.send_notification_to_queue') + sample_organisation.agreement_signed_on_behalf_of_name = on_behalf_of_name + sample_organisation.agreement_signed_on_behalf_of_email_address = on_behalf_of_email_address + + admin_request.post( + 'organisation.update_organisation', + _data={'agreement_signed': True, 'agreement_signed_by_id': str(sample_user.id)}, + organisation_id=sample_organisation.id, + _expected_status=204 + ) + + notifications = [x[0][0] for x in queue_mock.call_args_list] + assert {n.template.name: n.to for n in notifications} == templates_and_recipients + + for n in notifications: + # we pass in the same personalisation for all templates (though some templates don't use all fields) + assert n.personalisation == { + 'mou_link': 'http://localhost:6012/agreement/non-crown.pdf', + 'org_name': 'sample organisation', + 'org_dashboard_link': 'http://localhost:6012/organisations/{}'.format(sample_organisation.id), + 'signed_by_name': 'Test User', + 'on_behalf_of_name': on_behalf_of_name + } + + def test_post_link_service_to_organisation(admin_request, sample_service, sample_organisation): data = { 'service_id': str(sample_service.id)