From 6b2c2962c92d72039049cc3ef363d82617901cdf Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 14 Sep 2017 17:54:38 +0100 Subject: [PATCH 01/22] New endpoint to insert new service reply to email address and update existing one. --- app/dao/service_email_reply_to_dao.py | 56 ++++++++- app/models.py | 4 +- app/service/rest.py | 23 +++- app/service/service_senders_schema.py | 11 ++ .../dao/test_service_email_reply_to_dao.py | 117 +++++++++++++++++- tests/app/service/test_rest.py | 69 +++++++++++ 6 files changed, 274 insertions(+), 6 deletions(-) create mode 100644 app/service/service_senders_schema.py diff --git a/app/dao/service_email_reply_to_dao.py b/app/dao/service_email_reply_to_dao.py index 3066c229d..b31549bca 100644 --- a/app/dao/service_email_reply_to_dao.py +++ b/app/dao/service_email_reply_to_dao.py @@ -9,7 +9,7 @@ def dao_get_reply_to_by_service_id(service_id): ServiceEmailReplyTo ).filter( ServiceEmailReplyTo.service_id == service_id - ).all() + ).order_by(ServiceEmailReplyTo.created_at).all() return reply_to @@ -37,3 +37,57 @@ def dao_create_reply_to_email_address(reply_to_email): @transactional def dao_update_reply_to_email(reply_to): db.session.add(reply_to) + + +@transactional +def add_reply_to_email_address_for_service(service_id, email_address, is_default=True): + old_default = _get_existing_default(service_id) + if is_default: + _reset_old_default_to_false(old_default) + else: + _raise_when_no_default(old_default) + + new_reply_to = ServiceEmailReplyTo(service_id=service_id, email_address=email_address, is_default=is_default) + db.session.add(new_reply_to) + return new_reply_to + + +@transactional +def update_reply_to_email_address(service_id, reply_to_id, email_address, is_default=True): + old_default = _get_existing_default(service_id) + if is_default: + _reset_old_default_to_false(old_default) + else: + if old_default.id == reply_to_id: + raise InvalidRequest("You must have at least one reply to email address as the default.", 400) + + reply_to_update = ServiceEmailReplyTo.query.get(reply_to_id) + reply_to_update.email_address = email_address + reply_to_update.is_default = is_default + db.session.add(reply_to_update) + return reply_to_update + + +def _get_existing_default(service_id): + existing_reply_to = dao_get_reply_to_by_service_id(service_id=service_id) + if existing_reply_to: + old_default = [x for x in existing_reply_to if x.is_default] + if len(old_default) == 1: + return old_default[0] + else: + # is this check necessary + raise InvalidRequest( + "There should only be one default reply to email for each service. Service {} has {}".format( + service_id, len(old_default))) + + +def _reset_old_default_to_false(old_default): + if old_default: + old_default.is_default = False + db.session.add(old_default) + + +def _raise_when_no_default(old_default): + # check that the update is not updating the only default to false + if not old_default: + raise InvalidRequest("You must have at least one reply to email address as the default.", 400) diff --git a/app/models.py b/app/models.py index f689cc3c4..0a30f544d 100644 --- a/app/models.py +++ b/app/models.py @@ -1348,6 +1348,6 @@ class ServiceEmailReplyTo(db.Model): return { 'email_address': self.email_address, 'is_default': self.is_default, - 'created_at': self.created_at, - 'updated_at': self.updated_at + 'created_at': self.created_at.strftime(DATETIME_FORMAT), + 'updated_at': self.updated_at.strftime(DATETIME_FORMAT) if self.updated_at else None } diff --git a/app/service/rest.py b/app/service/rest.py index e64678939..fdcb6f28d 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -46,7 +46,8 @@ from app.dao.service_whitelist_dao import ( dao_add_and_commit_whitelisted_contacts, dao_remove_service_whitelist ) -from app.dao.service_email_reply_to_dao import create_or_update_email_reply_to, dao_get_reply_to_by_service_id +from app.dao.service_email_reply_to_dao import create_or_update_email_reply_to, dao_get_reply_to_by_service_id, \ + add_reply_to_email_address_for_service, update_reply_to_email_address from app.dao.provider_statistics_dao import get_fragment_count from app.dao.users_dao import get_user_by_id from app.errors import ( @@ -57,6 +58,7 @@ from app.models import Service, ServiceInboundApi from app.schema_validation import validate from app.service import statistics from app.service.service_inbound_api_schema import service_inbound_api, update_service_inbound_api_schema +from app.service.service_senders_schema import add_service_email_reply_to_request from app.service.utils import get_whitelist_objects from app.service.sender import send_notification_to_service_users from app.service.send_notification import send_one_off_notification @@ -529,6 +531,25 @@ def get_email_reply_to_addresses(service_id): return jsonify([i.serialize() for i in result]), 200 +@service_blueprint.route('//email-reply-to', methods=['POST']) +def add_service_reply_to_email_address(service_id): + form = validate(request.get_json(), add_service_email_reply_to_request) + new_reply_to = add_reply_to_email_address_for_service(service_id=service_id, + email_address=form['email_address'], + is_default=form.get('is_default', True)) + return jsonify(data=new_reply_to.serialize()), 201 + + +@service_blueprint.route('//email-reply-to/', methods=['POST']) +def update_service_reply_to_email_address(service_id, id): + form = validate(request.get_json(), add_service_email_reply_to_request) + new_reply_to = update_reply_to_email_address(service_id=service_id, + reply_to_id=id, + email_address=form['email_address'], + is_default=form.get('is_default', True)) + return jsonify(data=new_reply_to.serialize()), 200 + + @service_blueprint.route('/unique', methods=["GET"]) def is_service_name_unique(): name, email_from = check_request_args(request) diff --git a/app/service/service_senders_schema.py b/app/service/service_senders_schema.py new file mode 100644 index 000000000..277820837 --- /dev/null +++ b/app/service/service_senders_schema.py @@ -0,0 +1,11 @@ +add_service_email_reply_to_request = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "POST service email reply to address", + "type": "object", + "title": "Add new email reply to address for service", + "properties": { + "email_address": {"type": "string", "format": "email_address"}, + "is_default": {"type": "boolean"} + }, + "required": ["email_address"] +} diff --git a/tests/app/dao/test_service_email_reply_to_dao.py b/tests/app/dao/test_service_email_reply_to_dao.py index 58ba416a8..f16dba808 100644 --- a/tests/app/dao/test_service_email_reply_to_dao.py +++ b/tests/app/dao/test_service_email_reply_to_dao.py @@ -2,8 +2,8 @@ import pytest from app.dao.service_email_reply_to_dao import ( create_or_update_email_reply_to, - dao_get_reply_to_by_service_id -) + dao_get_reply_to_by_service_id, + add_reply_to_email_address_for_service, update_reply_to_email_address) from app.errors import InvalidRequest from app.models import ServiceEmailReplyTo from tests.app.db import create_reply_to_email, create_service @@ -65,3 +65,116 @@ def test_dao_get_reply_to_by_service_id(notify_db_session): assert len(results) == 2 assert default_reply_to in results assert another_reply_to in results + + +def test_add_reply_to_email_address_for_service_creates_first_email_for_service(notify_db_session): + service = create_service() + add_reply_to_email_address_for_service(service_id=service.id, email_address='new@address.com') + + results = dao_get_reply_to_by_service_id(service_id=service.id) + assert len(results) == 1 + assert results[0].email_address == 'new@address.com' + assert results[0].is_default + + +def test_add_reply_to_email_address_for_service_creates_another_email_for_service(notify_db_session): + service = create_service() + first_reply_to = create_reply_to_email(service=service, email_address="first@address.com") + + add_reply_to_email_address_for_service(service_id=service.id, email_address='second@address.com', is_default=False) + + results = dao_get_reply_to_by_service_id(service_id=service.id) + assert len(results) == 2 + for x in results: + if x.email_address == 'first@address.com': + assert x.is_default + elif x.email_address == 'second@address.com': + assert not x.is_default + else: + assert False + + +def test_add_reply_to_email_address_new_reply_to_is_default_existing_reply_to_is_not(notify_db_session): + service = create_service() + first_reply_to = create_reply_to_email(service=service, email_address="first@address.com", is_default=True) + add_reply_to_email_address_for_service(service_id=service.id, email_address='second@address.com', is_default=True) + + results = dao_get_reply_to_by_service_id(service_id=service.id) + assert len(results) == 2 + for x in results: + if x.email_address == 'first@address.com': + assert not x.is_default + elif x.email_address == 'second@address.com': + assert x.is_default + else: + assert False + + +def test_add_reply_to_email_address_can_add_a_third_reply_to_address(sample_service): + add_reply_to_email_address_for_service(service_id=sample_service.id, + email_address="first@address.com") + add_reply_to_email_address_for_service(service_id=sample_service.id, email_address='second@address.com', + is_default=False) + add_reply_to_email_address_for_service(service_id=sample_service.id, email_address='third@address.com', + is_default=False) + + results = dao_get_reply_to_by_service_id(service_id=sample_service.id) + assert len(results) == 3 + + for x in results: + if x.email_address == 'first@address.com': + assert x.is_default + elif x.email_address == 'second@address.com': + assert not x.is_default + elif x.email_address == 'third@address.com': + assert not x.is_default + else: + assert False + + +def test_add_reply_to_email_address_ensures_first_reply_to_is_default(sample_service): + with pytest.raises(expected_exception=InvalidRequest): + add_reply_to_email_address_for_service(service_id=sample_service.id, + email_address="first@address.com", is_default=False) + + +def test_update_reply_to_email_address(sample_service): + first_reply_to = create_reply_to_email(service=sample_service, email_address="first@address.com") + update_reply_to_email_address(service_id=sample_service.id, reply_to_id=first_reply_to.id, + email_address='change_address@email.com') + updated_reply_to = ServiceEmailReplyTo.query.get(first_reply_to.id) + + assert updated_reply_to.email_address == 'change_address@email.com' + assert updated_reply_to.updated_at + assert updated_reply_to.is_default + + +def test_update_reply_to_email_address_set_updated_to_default(sample_service): + first_reply_to = create_reply_to_email(service=sample_service, email_address="first@address.com") + second_reply_to = create_reply_to_email(service=sample_service, + email_address="second@address.com", + is_default=False) + + update_reply_to_email_address(service_id=sample_service.id, + reply_to_id=second_reply_to.id, + email_address='change_address@email.com', + is_default=True) + + results = ServiceEmailReplyTo.query.all() + assert len(results) == 2 + for x in results: + if x.email_address == 'change_address@email.com': + assert x.is_default + elif x.email_address == 'first@address.com': + assert not x.is_default + else: + assert False + + +def test_update_reply_to_email_address_raises_exception_if_single_reply_to_and_setting_default_to_false(sample_service): + first_reply_to = create_reply_to_email(service=sample_service, email_address="first@address.com") + with pytest.raises(expected_exception=InvalidRequest): + update_reply_to_email_address(service_id=sample_service.id, + reply_to_id=first_reply_to.id, + email_address='should@fail.com', + is_default=False) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 39e5f776c..d5d01cd78 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2198,3 +2198,72 @@ def test_get_email_reply_to_addresses_with_multiple_email_addresses(client, noti assert not json_response[1]['is_default'] assert json_response[1]['created_at'] assert not json_response[1]['updated_at'] + + +def test_add_service_reply_to_email_address(client, sample_service): + data = json.dumps({"email_address": "new@reply.com"}) + response = client.post('/service/{}/email-reply-to'.format(sample_service.id), + data=data, + headers=[('Content-Type', 'application/json'), create_authorization_header()]) + + assert response.status_code == 201 + json_resp = json.loads(response.get_data(as_text=True)) + results = ServiceEmailReplyTo.query.all() + assert len(results) == 1 + assert json_resp['data'] == results[0].serialize() + + +def test_add_service_reply_to_email_address_can_add_multiple_addresses(client, sample_service): + data = json.dumps({"email_address": "first@reply.com"}) + client.post('/service/{}/email-reply-to'.format(sample_service.id), + data=data, + headers=[('Content-Type', 'application/json'), create_authorization_header()]) + + second = json.dumps({"email_address": "second@reply.com"}) + response = client.post('/service/{}/email-reply-to'.format(sample_service.id), + data=second, + headers=[('Content-Type', 'application/json'), create_authorization_header()]) + assert response.status_code == 201 + json_resp = json.loads(response.get_data(as_text=True)) + results = ServiceEmailReplyTo.query.all() + assert len(results) == 2 + default = [x for x in results if x.is_default] + assert json_resp['data'] == default[0].serialize() + first_reply_to_not_default = [x for x in results if not x.is_default] + assert first_reply_to_not_default[0].email_address == 'first@reply.com' + + +def test_add_service_reply_to_email_address_raise_exception_if_no_default(client, sample_service): + data = json.dumps({"email_address": "first@reply.com", "is_default": False}) + response = client.post('/service/{}/email-reply-to'.format(sample_service.id), + data=data, + headers=[('Content-Type', 'application/json'), create_authorization_header()]) + assert response.status_code == 400 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['message'] == 'You must have at least one reply to email address as the default.' + + +def test_update_service_reply_to_email_address(client, sample_service): + original_reply_to = create_reply_to_email(service=sample_service, email_address="some@email.com") + data = json.dumps({"email_address": "changed@reply.com"}) + response = client.post('/service/{}/email-reply-to/{}'.format(sample_service.id, original_reply_to.id), + data=data, + headers=[('Content-Type', 'application/json'), create_authorization_header()]) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + results = ServiceEmailReplyTo.query.all() + assert len(results) == 1 + assert json_resp['data'] == results[0].serialize() + + +def test_update_service_reply_to_email_address_returns_400_when_no_default(client, sample_service): + original_reply_to = create_reply_to_email(service=sample_service, email_address="some@email.com") + data = json.dumps({"email_address": "changed@reply.com", "is_default": False}) + response = client.post('/service/{}/email-reply-to/{}'.format(sample_service.id, original_reply_to.id), + data=data, + headers=[('Content-Type', 'application/json'), create_authorization_header()]) + + assert response.status_code == 400 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['message'] == 'You must have at least one reply to email address as the default.' From f49eca53246a742910633aa850209ba5d3f8dbd2 Mon Sep 17 00:00:00 2001 From: chrisw Date: Tue, 19 Sep 2017 13:18:59 +0100 Subject: [PATCH 02/22] Add a non-GOV.UK banner option for email branding Added an extra name, 'org_banner', for branding types into branding_type table Added org banner into user model in database Added checks for new branding type to ensure that the correct data is passed into the dict Tested new checks in html email options --- app/delivery/send_to_providers.py | 6 +++-- app/models.py | 1 + .../versions/0120_add_org_banner_branding.py | 22 +++++++++++++++++++ requirements.txt | 2 +- tests/app/delivery/test_send_to_providers.py | 13 ++++++++--- 5 files changed, 38 insertions(+), 6 deletions(-) create mode 100644 migrations/versions/0120_add_org_banner_branding.py diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 9aafe1a35..fe2d1fe76 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -19,6 +19,7 @@ from app.models import ( SMS_TYPE, KEY_TYPE_TEST, BRANDING_ORG, + BRANDING_ORG_BANNER, BRANDING_GOVUK, EMAIL_TYPE, NOTIFICATION_TECHNICAL_FAILURE, @@ -174,7 +175,8 @@ def get_logo_url(base_url, logo_file): def get_html_email_options(service): - govuk_banner = service.branding != BRANDING_ORG + govuk_banner = service.branding not in (BRANDING_ORG, BRANDING_ORG_BANNER) + brand_banner = service.branding == BRANDING_ORG_BANNER if service.organisation and service.branding != BRANDING_GOVUK: logo_url = get_logo_url( current_app.config['ADMIN_BASE_URL'], @@ -189,7 +191,7 @@ def get_html_email_options(service): else: branding = {} - return dict(govuk_banner=govuk_banner, **branding) + return dict(govuk_banner=govuk_banner, brand_banner=brand_banner, **branding) def technical_failure(notification): diff --git a/app/models.py b/app/models.py index 8d1437986..207e56e8c 100644 --- a/app/models.py +++ b/app/models.py @@ -123,6 +123,7 @@ user_to_service = db.Table( BRANDING_GOVUK = 'govuk' BRANDING_ORG = 'org' BRANDING_BOTH = 'both' +BRANDING_ORG_BANNER = 'org_banner' class BrandingTypes(db.Model): diff --git a/migrations/versions/0120_add_org_banner_branding.py b/migrations/versions/0120_add_org_banner_branding.py new file mode 100644 index 000000000..015b4e6f3 --- /dev/null +++ b/migrations/versions/0120_add_org_banner_branding.py @@ -0,0 +1,22 @@ +""" + +Revision ID: 0120_add_org_banner_branding +Revises: 0119_add_email_reply_to +Create Date: 2017-09-18 14:18:49.087143 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0120_add_org_banner_branding' +down_revision = '0119_add_email_reply_to' + + +def upgrade(): + op.execute("INSERT INTO branding_type VALUES ('org_banner')") + +def downgrade(): + op.execute("UPDATE services SET branding = 'org' WHERE branding = 'org_banner'") + op.execute("DELETE FROM branding_type WHERE name = 'org_banner'") + \ No newline at end of file diff --git a/requirements.txt b/requirements.txt index 573d446db..7d6fbf67a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -25,6 +25,6 @@ notifications-python-client==4.4.0 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@21.2.0#egg=notifications-utils==21.2.0 +git+https://github.com/alphagov/notifications-utils.git@21.3.0#egg=notifications-utils==21.3.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 5be786361..cad34eb8d 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -20,7 +20,8 @@ from app.models import ( KEY_TYPE_TEAM, BRANDING_ORG, BRANDING_GOVUK, - BRANDING_BOTH) + BRANDING_BOTH, + BRANDING_ORG_BANNER) from tests.app.db import create_service, create_template, create_notification, create_inbound_number @@ -411,7 +412,8 @@ def test_get_html_email_renderer_should_return_for_normal_service(sample_service @pytest.mark.parametrize('branding_type, govuk_banner', [ (BRANDING_ORG, False), - (BRANDING_BOTH, True) + (BRANDING_BOTH, True), + (BRANDING_ORG_BANNER, False) ]) def test_get_html_email_renderer_with_branding_details(branding_type, govuk_banner, notify_db, sample_service): sample_service.branding = branding_type @@ -426,6 +428,11 @@ def test_get_html_email_renderer_with_branding_details(branding_type, govuk_bann assert options['brand_colour'] == '#000000' assert options['brand_name'] == 'Justice League' + if sample_service.branding == BRANDING_ORG_BANNER: + assert options['brand_banner'] is True + else: + assert options['brand_banner'] is False + def test_get_html_email_renderer_with_branding_details_and_render_govuk_banner_only(notify_db, sample_service): sample_service.branding = BRANDING_GOVUK @@ -436,7 +443,7 @@ def test_get_html_email_renderer_with_branding_details_and_render_govuk_banner_o options = send_to_providers.get_html_email_options(sample_service) - assert options == {'govuk_banner': True} + assert options == {'govuk_banner': True, 'brand_banner': False} def test_get_html_email_renderer_prepends_logo_path(notify_api): From e3661440d6cefbe226e4598fff438c4688b4dd9f Mon Sep 17 00:00:00 2001 From: chrisw Date: Tue, 19 Sep 2017 16:13:20 +0100 Subject: [PATCH 03/22] bumped utils version --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 7d6fbf67a..645eafb0d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -25,6 +25,6 @@ notifications-python-client==4.4.0 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@21.3.0#egg=notifications-utils==21.3.0 +git+https://github.com/alphagov/notifications-utils.git@21.3.1#egg=notifications-utils==21.3.1 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From a53a0da414103e6b4f7f6daac858e6aa96255b1a Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 20 Sep 2017 10:45:35 +0100 Subject: [PATCH 04/22] [WIP] need to fix the tests --- app/delivery/send_to_providers.py | 3 +- app/models.py | 9 +++++- app/schemas.py | 4 +++ tests/app/delivery/test_send_to_providers.py | 7 +++-- tests/app/service/test_rest.py | 29 +++++++++----------- tests/app/test_model.py | 8 +++++- 6 files changed, 38 insertions(+), 22 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index fe2d1fe76..7d90b07d3 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -109,13 +109,14 @@ def send_email_to_provider(notification): else: from_address = '"{}" <{}@{}>'.format(service.name, service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']) + reference = provider.send_email( from_address, notification.to, plain_text_email.subject, body=str(plain_text_email), html_body=str(html_email), - reply_to_address=service.reply_to_email_address, + reply_to_address=service.get_default_reply_to_email_address(), ) notification.reference = reference update_notification(notification, provider) diff --git a/app/models.py b/app/models.py index 207e56e8c..e223bac0a 100644 --- a/app/models.py +++ b/app/models.py @@ -201,7 +201,7 @@ class Service(db.Model, Versioned): email_from = db.Column(db.Text, index=False, unique=True, nullable=False) created_by = db.relationship('User') created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) - reply_to_email_address = db.Column(db.Text, index=False, unique=False, nullable=True) + _reply_to_email_address = db.Column("reply_to_email_address", db.Text, index=False, unique=False, nullable=True) letter_contact_block = db.Column(db.Text, index=False, unique=False, nullable=True) sms_sender = db.Column(db.String(11), nullable=False, default=lambda: current_app.config['FROM_NUMBER']) organisation_id = db.Column(UUID(as_uuid=True), db.ForeignKey('organisation.id'), index=True, nullable=True) @@ -249,6 +249,13 @@ class Service(db.Model, Versioned): else: return self.sms_sender + def get_default_reply_to_email_address(self): + default_reply_to = [x for x in self.reply_to_email_addresses if x.is_default] + if len(default_reply_to) > 1: + raise Exception("There should only ever be one default") + else: + return default_reply_to[0].email_address if default_reply_to else None + class InboundNumber(db.Model): __tablename__ = "inbound_numbers" diff --git a/app/schemas.py b/app/schemas.py index 30bca95a0..60927b671 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -182,6 +182,7 @@ class ServiceSchema(BaseSchema): dvla_organisation = field_for(models.Service, 'dvla_organisation') permissions = fields.Method("service_permissions") override_flag = False + reply_to_email_address = fields.Method("get_reply_to_email_address") def get_free_sms_fragment_limit(selfs, service): return service.free_sms_fragment_limit() @@ -189,6 +190,9 @@ class ServiceSchema(BaseSchema): def service_permissions(self, service): return [p.permission for p in service.permissions] + def get_reply_to_email_address(self, service): + return service.get_default_reply_to_email_address() + class Meta: model = models.Service dump_only = ['free_sms_fragment_limit'] diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index cad34eb8d..fb52e8b98 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -23,7 +23,8 @@ from app.models import ( BRANDING_BOTH, BRANDING_ORG_BANNER) -from tests.app.db import create_service, create_template, create_notification, create_inbound_number +from tests.app.db import create_service, create_template, create_notification, create_inbound_number, \ + create_reply_to_email def test_should_return_highest_priority_active_provider(restore_provider_details): @@ -386,7 +387,7 @@ def test_send_email_should_use_service_reply_to_email( mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') db_notification = create_notification(template=sample_email_template) - sample_service.reply_to_email_address = 'foo@bar.com' + create_reply_to_email(service=sample_service, email_address='foo@bar.com') send_to_providers.send_email_to_provider( db_notification, @@ -398,7 +399,7 @@ def test_send_email_should_use_service_reply_to_email( ANY, body=ANY, html_body=ANY, - reply_to_address=sample_service.reply_to_email_address + reply_to_address=sample_service.get_default_reply_to_email_address() ) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 39e5f776c..9599f794b 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -213,19 +213,19 @@ def test_get_service_by_id_should_404_if_no_service(notify_api, notify_db): assert json_resp['message'] == 'No result found' -def test_get_service_by_id_and_user(notify_api, service_factory, sample_user): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - service = service_factory.get('new.service', sample_user) - auth_header = create_authorization_header() - resp = client.get( - '/service/{}?user_id={}'.format(service.id, sample_user.id), - headers=[auth_header] - ) - assert resp.status_code == 200 - json_resp = json.loads(resp.get_data(as_text=True)) - assert json_resp['data']['name'] == service.name - assert json_resp['data']['id'] == str(service.id) +def test_get_service_by_id_and_user(client, sample_service, sample_user): + sample_service.reply_to_email = 'something@service.com' + create_reply_to_email(service=sample_service, email_address='new@service.com') + auth_header = create_authorization_header() + resp = client.get( + '/service/{}?user_id={}'.format(sample_service.id, sample_user.id), + headers=[auth_header] + ) + assert resp.status_code == 200 + json_resp = json.loads(resp.get_data(as_text=True)) + assert json_resp['data']['name'] == sample_service.name + assert json_resp['data']['id'] == str(sample_service.id) + assert json_resp['data']['reply_to_email_address'] == 'new@service.com' def test_get_service_by_id_should_404_if_no_service_for_user(notify_api, sample_user): @@ -2161,7 +2161,6 @@ def test_get_email_reply_to_addresses_when_there_are_no_reply_to_email_addresses def test_get_email_reply_to_addresses_with_one_email_address(client, notify_db, notify_db_session): service = create_service(notify_db=notify_db, notify_db_session=notify_db_session) reply_to = create_reply_to_email(service, 'test@mail.com') - service.reply_to_email_address = 'test@mail.com' response = client.get('/service/{}/email-reply-to'.format(service.id), headers=[create_authorization_header()]) @@ -2180,8 +2179,6 @@ def test_get_email_reply_to_addresses_with_multiple_email_addresses(client, noti reply_to_a = create_reply_to_email(service, 'test_a@mail.com') reply_to_b = create_reply_to_email(service, 'test_b@mail.com', False) - service.reply_to_email_address = 'test_a@mail.com' - response = client.get('/service/{}/email-reply-to'.format(service.id), headers=[create_authorization_header()]) json_response = json.loads(response.get_data(as_text=True)) diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 56a121e01..14fd24df3 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -19,7 +19,7 @@ from tests.app.conftest import ( sample_template as create_sample_template, sample_notification_with_job as create_sample_notification_with_job ) -from tests.app.db import create_notification, create_service, create_inbound_number +from tests.app.db import create_notification, create_service, create_inbound_number, create_reply_to_email from tests.conftest import set_config @@ -248,3 +248,9 @@ def test_inbound_number_returns_from_number_config(client, notify_db_session): service = create_service(sms_sender=None) assert service.get_inbound_number() == 'test' + + +def test_service_get_default_reply_to_email_address(sample_service): + create_reply_to_email(service=sample_service, email_address="default@email.com") + + assert sample_service.get_default_reply_to_email_address() == 'default@email.com' From bde6a9e131c13234700cce10213cc70828a9cabd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 20 Sep 2017 11:03:48 +0100 Subject: [PATCH 05/22] Make organisation logo nullable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now we have the org banner branding, not all organisations need a logo. So it shouldn’t be an error to not provide one. --- app/models.py | 2 +- app/organisation/organisation_schema.py | 2 +- migrations/versions/0121_nullable_logos.py | 25 ++++++++++++++++++++++ tests/app/dao/test_organisations_dao.py | 9 +------- tests/app/organisation/test_rest.py | 5 ++--- 5 files changed, 30 insertions(+), 13 deletions(-) create mode 100644 migrations/versions/0121_nullable_logos.py diff --git a/app/models.py b/app/models.py index 207e56e8c..bad685309 100644 --- a/app/models.py +++ b/app/models.py @@ -135,7 +135,7 @@ class Organisation(db.Model): __tablename__ = 'organisation' id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) colour = db.Column(db.String(7), nullable=True) - logo = db.Column(db.String(255), nullable=False) + logo = db.Column(db.String(255), nullable=True) name = db.Column(db.String(255), nullable=True) def serialize(self): diff --git a/app/organisation/organisation_schema.py b/app/organisation/organisation_schema.py index 60b523057..44571d3fb 100644 --- a/app/organisation/organisation_schema.py +++ b/app/organisation/organisation_schema.py @@ -7,7 +7,7 @@ post_create_organisation_schema = { "name": {"type": ["string", "null"]}, "logo": {"type": ["string", "null"]} }, - "required": ["logo"] + "required": [] } post_update_organisation_schema = { diff --git a/migrations/versions/0121_nullable_logos.py b/migrations/versions/0121_nullable_logos.py new file mode 100644 index 000000000..c100a2f24 --- /dev/null +++ b/migrations/versions/0121_nullable_logos.py @@ -0,0 +1,25 @@ +""" + +Revision ID: 0121_nullable_logos +Revises: 0120_add_org_banner_branding +Create Date: 2017-09-20 11:00:20.415523 + +""" +from alembic import op +import sqlalchemy as sa + + +revision = '0121_nullable_logos' +down_revision = '0120_add_org_banner_branding' + + +def upgrade(): + op.alter_column( + 'organisation', 'logo', + existing_type=sa.VARCHAR(length=255), + nullable=True + ) + + +def downgrade(): + pass diff --git a/tests/app/dao/test_organisations_dao.py b/tests/app/dao/test_organisations_dao.py index bc5d8c0a6..e46c75878 100644 --- a/tests/app/dao/test_organisations_dao.py +++ b/tests/app/dao/test_organisations_dao.py @@ -21,20 +21,13 @@ def test_create_organisation(notify_db, notify_db_session): def test_create_organisation_without_name_or_colour_is_valid(notify_db, notify_db_session): - organisation = create_organisation(name=None, colour=None) + organisation = create_organisation(logo=None, name=None, colour=None) assert Organisation.query.count() == 1 organisation_from_db = Organisation.query.first() assert organisation == organisation_from_db -def test_create_organisation_without_logo_raises_error(notify_db, notify_db_session): - with pytest.raises(IntegrityError) as excinfo: - create_organisation(logo=None) - assert 'column "logo" violates not-null constraint' in str(excinfo.value) - assert Organisation.query.count() == 0 - - def test_get_organisations_gets_all_organisations(notify_db, notify_db_session): org_1 = create_organisation(name='test_org_1') org_2 = create_organisation(name='test_org_2') diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index ea5926f5d..688f6f74d 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -54,7 +54,7 @@ def test_post_create_organisation(admin_request, notify_db_session): assert data['logo'] == response['data']['logo'] -def test_post_create_organisation_without_logo_raises_error(admin_request, notify_db_session): +def test_post_create_organisation_without_logo_is_ok(admin_request, notify_db_session): data = { 'name': 'test organisation', 'colour': '#0000ff', @@ -62,9 +62,8 @@ def test_post_create_organisation_without_logo_raises_error(admin_request, notif response = admin_request.post( 'organisation.create_organisation', _data=data, - _expected_status=400 + _expected_status=201, ) - assert response['errors'][0]['message'] == "logo is a required property" def test_post_create_organisation_without_name_or_colour_is_valid(admin_request, notify_db_session): From 4936fa384cf8071bc681905116dc1a4d85312cd3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 8 Sep 2017 16:35:13 +0100 Subject: [PATCH 06/22] return fake "received_by_notify" status for letter notifications created and sending aren't quite as helpful for letters, since their journey through notify and our providers is so different to emails/sms. So instead, we should return estimated_dispatch_date (in a future PR) and the status should just read received_by_notify --- app/models.py | 31 +++++++++++++++++++++++-------- tests/app/test_model.py | 5 +++-- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/app/models.py b/app/models.py index 207e56e8c..3001e286d 100644 --- a/app/models.py +++ b/app/models.py @@ -797,6 +797,9 @@ NOTIFICATION_STATUS_TYPES_NON_BILLABLE = list(set(NOTIFICATION_STATUS_TYPES) - s NOTIFICATION_STATUS_TYPES_ENUM = db.Enum(*NOTIFICATION_STATUS_TYPES, name='notify_status_type') +NOTIFICATION_STATUS_LETTER_RECEIVED = 'received_by_notify' +NOTIFICATION_STATUS_LETTER_RECEIVED_PRETTY = 'Received by Notify' + class NotificationStatusTypes(db.Model): __tablename__ = 'notification_status_types' @@ -962,17 +965,29 @@ class Notification(db.Model): 'sent': 'Sent internationally' }, 'letter': { - 'failed': 'Failed', 'technical-failure': 'Technical failure', - 'temporary-failure': 'Temporary failure', - 'permanent-failure': 'Permanent failure', - 'delivered': 'Delivered', - 'sending': 'Sending', - 'created': 'Sending', - 'sent': 'Delivered' + 'sending': NOTIFICATION_STATUS_LETTER_RECEIVED_PRETTY, + 'created': NOTIFICATION_STATUS_LETTER_RECEIVED_PRETTY, } }[self.template.template_type].get(self.status, self.status) + def get_letter_status(self): + """ + Return the notification_status, as we should present for letters. The distinction between created and sending is + a bit more confusing for letters, not to mention that there's no concept of temporary or permanent failure yet. + + + """ + # this should only ever be called for letter notifications - it makes no sense otherwise and I'd rather not + # get the two code flows mixed up at all + assert self.notification_type == LETTER_TYPE + + if self.status == NOTIFICATION_CREATED or NOTIFICATION_SENDING: + return NOTIFICATION_STATUS_LETTER_RECEIVED + else: + # Currently can only be technical-failure + return status + def serialize_for_csv(self): created_at_in_bst = convert_utc_to_bst(self.created_at) serialized = { @@ -1007,7 +1022,7 @@ class Notification(db.Model): "line_6": None, "postcode": None, "type": self.notification_type, - "status": self.status, + "status": self.get_letter_status() if self.notification_type == LETTER_TYPE else self.status, "template": template_dict, "body": self.content, "subject": self.subject, diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 56a121e01..e1496163d 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -116,8 +116,9 @@ def test_notification_for_csv_returns_correct_job_row_number(notify_db, notify_d ('sms', 'temporary-failure', 'Phone not accepting messages right now'), ('sms', 'permanent-failure', 'Phone number doesn’t exist'), ('sms', 'sent', 'Sent internationally'), - ('letter', 'permanent-failure', 'Permanent failure'), - ('letter', 'delivered', 'Delivered') + ('letter', 'created', 'Received by Notify'), + ('letter', 'sending', 'Received by Notify'), + ('letter', 'technical-failure', 'Technical failure') ]) def test_notification_for_csv_returns_formatted_status( notify_db, From 10ceb0467eabed03c2b6bf53bb09aeb981940d0f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 8 Sep 2017 16:36:55 +0100 Subject: [PATCH 07/22] fix ordering to prevent random test failures --- tests/app/dao/test_monthly_billing.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/app/dao/test_monthly_billing.py b/tests/app/dao/test_monthly_billing.py index 23c445f92..e5813c9bf 100644 --- a/tests/app/dao/test_monthly_billing.py +++ b/tests/app/dao/test_monthly_billing.py @@ -198,7 +198,10 @@ def test_add_monthly_billing_for_multiple_months_populate_correctly( create_or_update_monthly_billing(service_id=sample_template.service_id, billing_month=FEB_2016_MONTH_START) create_or_update_monthly_billing(service_id=sample_template.service_id, billing_month=MAR_2016_MONTH_START) - monthly_billing = MonthlyBilling.query.order_by(MonthlyBilling.notification_type).all() + monthly_billing = MonthlyBilling.query.order_by( + MonthlyBilling.notification_type, + MonthlyBilling.start_date + ).all() assert len(monthly_billing) == 4 _assert_monthly_billing( From 18639354fa856fe59326b1fafc0303c5124844f2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 8 Sep 2017 16:38:36 +0100 Subject: [PATCH 08/22] changed conftest to put underscores in sample_letter_template this gets tests passing for now, but we'll need to make sure that notifications.serialize handles letters with personalisation that doesn't include underscores in future - for example, address_line_1 may also be addressline1 if someone uploaded a CSV --- .../notifications/test_get_notifications.py | 60 +++++++++++++++---- 1 file changed, 50 insertions(+), 10 deletions(-) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 814e6dd6c..626c01432 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -1,6 +1,6 @@ import datetime import pytest -from flask import json +from flask import json, url_for from app import DATETIME_FORMAT from tests import create_authorization_header @@ -23,13 +23,20 @@ from tests.app.conftest import ( def test_get_notification_by_id_returns_200( client, billable_units, provider, sample_template ): - sample_notification = create_notification(template=sample_template, billable_units=billable_units, sent_by=provider, - scheduled_for="2017-05-12 15:15" - ) + sample_notification = create_notification( + template=sample_template, + billable_units=billable_units, + sent_by=provider, + scheduled_for="2017-05-12 15:15" + ) + + another = create_notification( + template=sample_template, + billable_units=billable_units, + sent_by=provider, + scheduled_for="2017-06-12 15:15" + ) - another = create_notification(template=sample_template, billable_units=billable_units, sent_by=provider, - scheduled_for="2017-06-12 15:15" - ) auth_header = create_authorization_header(service_id=sample_notification.service_id) response = client.get( path='/v2/notifications/{}'.format(sample_notification.id), @@ -75,9 +82,10 @@ def test_get_notification_by_id_returns_200( def test_get_notification_by_id_with_placeholders_returns_200( client, sample_email_template_with_placeholders ): - sample_notification = create_notification(template=sample_email_template_with_placeholders, - personalisation={"name": "Bob"} - ) + sample_notification = create_notification( + template=sample_email_template_with_placeholders, + personalisation={"name": "Bob"} + ) auth_header = create_authorization_header(service_id=sample_notification.service_id) response = client.get( @@ -549,3 +557,35 @@ def test_get_all_notifications_filter_multiple_query_parameters(client, sample_e assert len(json_response['notifications']) == 1 assert json_response['notifications'][0]['id'] == str(older_notification.id) + + +def test_get_all_notifications_renames_letter_statuses(client, sample_letter_notification, sample_notification): + auth_header = create_authorization_header(service_id=sample_letter_notification.service_id) + response = client.get( + path=url_for('v2_notifications.get_notifications'), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + json_response = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + + for noti in json_response['notifications']: + if noti['type'] == 'sms': + assert noti['status'] == 'created' + elif noti['type'] == 'letter': + assert noti['status'] == 'received_by_notify' + else: + pytest.fail() + + +def test_get_notifications_renames_letter_statuses(client, sample_letter_notification): + auth_header = create_authorization_header(service_id=sample_letter_notification.service_id) + response = client.get( + path=url_for('v2_notifications.get_notification_by_id', id=sample_letter_notification.id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + json_response = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + + assert json_response['status'] == 'received_by_notify' From cb3379d0b62983747f0f721713002ccd1ab59eea Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 11 Sep 2017 11:57:33 +0100 Subject: [PATCH 09/22] rename received_by_notify to accepted accepted is nice since it both implies that things look good, while not being commital about next steps. --- app/models.py | 10 +++++----- tests/app/test_model.py | 4 ++-- tests/app/v2/notifications/test_get_notifications.py | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/models.py b/app/models.py index 3001e286d..e1a9f36fd 100644 --- a/app/models.py +++ b/app/models.py @@ -797,8 +797,8 @@ NOTIFICATION_STATUS_TYPES_NON_BILLABLE = list(set(NOTIFICATION_STATUS_TYPES) - s NOTIFICATION_STATUS_TYPES_ENUM = db.Enum(*NOTIFICATION_STATUS_TYPES, name='notify_status_type') -NOTIFICATION_STATUS_LETTER_RECEIVED = 'received_by_notify' -NOTIFICATION_STATUS_LETTER_RECEIVED_PRETTY = 'Received by Notify' +NOTIFICATION_STATUS_LETTER_ACCEPTED = 'accepted' +NOTIFICATION_STATUS_LETTER_ACCEPTED_PRETTY = 'Accepted' class NotificationStatusTypes(db.Model): @@ -966,8 +966,8 @@ class Notification(db.Model): }, 'letter': { 'technical-failure': 'Technical failure', - 'sending': NOTIFICATION_STATUS_LETTER_RECEIVED_PRETTY, - 'created': NOTIFICATION_STATUS_LETTER_RECEIVED_PRETTY, + 'sending': NOTIFICATION_STATUS_LETTER_ACCEPTED_PRETTY, + 'created': NOTIFICATION_STATUS_LETTER_ACCEPTED_PRETTY, } }[self.template.template_type].get(self.status, self.status) @@ -983,7 +983,7 @@ class Notification(db.Model): assert self.notification_type == LETTER_TYPE if self.status == NOTIFICATION_CREATED or NOTIFICATION_SENDING: - return NOTIFICATION_STATUS_LETTER_RECEIVED + return NOTIFICATION_STATUS_LETTER_ACCEPTED else: # Currently can only be technical-failure return status diff --git a/tests/app/test_model.py b/tests/app/test_model.py index e1496163d..0dd18eb56 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -116,8 +116,8 @@ def test_notification_for_csv_returns_correct_job_row_number(notify_db, notify_d ('sms', 'temporary-failure', 'Phone not accepting messages right now'), ('sms', 'permanent-failure', 'Phone number doesn’t exist'), ('sms', 'sent', 'Sent internationally'), - ('letter', 'created', 'Received by Notify'), - ('letter', 'sending', 'Received by Notify'), + ('letter', 'created', 'Accepted'), + ('letter', 'sending', 'Accepted'), ('letter', 'technical-failure', 'Technical failure') ]) def test_notification_for_csv_returns_formatted_status( diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 626c01432..651de4253 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -573,7 +573,7 @@ def test_get_all_notifications_renames_letter_statuses(client, sample_letter_not if noti['type'] == 'sms': assert noti['status'] == 'created' elif noti['type'] == 'letter': - assert noti['status'] == 'received_by_notify' + assert noti['status'] == 'accepted' else: pytest.fail() @@ -588,4 +588,4 @@ def test_get_notifications_renames_letter_statuses(client, sample_letter_notific json_response = json.loads(response.get_data(as_text=True)) assert response.status_code == 200 - assert json_response['status'] == 'received_by_notify' + assert json_response['status'] == 'accepted' From f3b4a06a07b45b5d71597386fa26a05e19471110 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 13 Sep 2017 15:28:23 +0100 Subject: [PATCH 10/22] check both sms and email --- tests/app/v2/notifications/test_get_notifications.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 651de4253..66133da75 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -559,7 +559,12 @@ def test_get_all_notifications_filter_multiple_query_parameters(client, sample_e assert json_response['notifications'][0]['id'] == str(older_notification.id) -def test_get_all_notifications_renames_letter_statuses(client, sample_letter_notification, sample_notification): +def test_get_all_notifications_renames_letter_statuses( + client, + sample_letter_notification, + sample_notification, + sample_email_notification, +): auth_header = create_authorization_header(service_id=sample_letter_notification.service_id) response = client.get( path=url_for('v2_notifications.get_notifications'), @@ -570,7 +575,7 @@ def test_get_all_notifications_renames_letter_statuses(client, sample_letter_not assert response.status_code == 200 for noti in json_response['notifications']: - if noti['type'] == 'sms': + if noti['type'] == 'sms' or noti['type'] == 'email': assert noti['status'] == 'created' elif noti['type'] == 'letter': assert noti['status'] == 'accepted' From c8ff45be2dbc1098c3be80895c5c2d99a138590c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 20 Sep 2017 11:21:27 +0100 Subject: [PATCH 11/22] don't create test db anymore, thats done in pytest --- scripts/bootstrap.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/scripts/bootstrap.sh b/scripts/bootstrap.sh index 15c1d8ef0..9ee35d24b 100755 --- a/scripts/bootstrap.sh +++ b/scripts/bootstrap.sh @@ -33,7 +33,6 @@ pip3 install -r requirements_for_test.txt # Create Postgres databases createdb notification_api -createdb test_notification_api # Upgrade databases source environment.sh From c6bf38e8693fbda4da3d6ec755b35eda7f2baa64 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 20 Sep 2017 11:58:18 +0100 Subject: [PATCH 12/22] - is_default is required on the add_service_email_reply_to_request schema - Added check that the service exists for the POST reply-to methods. - Added tests --- app/dao/service_email_reply_to_dao.py | 4 +-- app/service/rest.py | 10 ++++-- app/service/service_senders_schema.py | 2 +- .../dao/test_service_email_reply_to_dao.py | 9 ++++++ tests/app/service/test_rest.py | 32 +++++++++++++++---- 5 files changed, 45 insertions(+), 12 deletions(-) diff --git a/app/dao/service_email_reply_to_dao.py b/app/dao/service_email_reply_to_dao.py index b31549bca..70e07e02d 100644 --- a/app/dao/service_email_reply_to_dao.py +++ b/app/dao/service_email_reply_to_dao.py @@ -75,10 +75,10 @@ def _get_existing_default(service_id): if len(old_default) == 1: return old_default[0] else: - # is this check necessary - raise InvalidRequest( + raise Exception( "There should only be one default reply to email for each service. Service {} has {}".format( service_id, len(old_default))) + return None def _reset_old_default_to_false(old_default): diff --git a/app/service/rest.py b/app/service/rest.py index fdcb6f28d..61527ce1c 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -533,6 +533,8 @@ def get_email_reply_to_addresses(service_id): @service_blueprint.route('//email-reply-to', methods=['POST']) def add_service_reply_to_email_address(service_id): + # validate the service exists, throws ResultNotFound exception. + dao_fetch_service_by_id(service_id) form = validate(request.get_json(), add_service_email_reply_to_request) new_reply_to = add_reply_to_email_address_for_service(service_id=service_id, email_address=form['email_address'], @@ -540,11 +542,13 @@ def add_service_reply_to_email_address(service_id): return jsonify(data=new_reply_to.serialize()), 201 -@service_blueprint.route('//email-reply-to/', methods=['POST']) -def update_service_reply_to_email_address(service_id, id): +@service_blueprint.route('//email-reply-to/', methods=['POST']) +def update_service_reply_to_email_address(service_id, reply_to_email_id): + # validate the service exists, throws ResultNotFound exception. + dao_fetch_service_by_id(service_id) form = validate(request.get_json(), add_service_email_reply_to_request) new_reply_to = update_reply_to_email_address(service_id=service_id, - reply_to_id=id, + reply_to_id=reply_to_email_id, email_address=form['email_address'], is_default=form.get('is_default', True)) return jsonify(data=new_reply_to.serialize()), 200 diff --git a/app/service/service_senders_schema.py b/app/service/service_senders_schema.py index 277820837..57b689099 100644 --- a/app/service/service_senders_schema.py +++ b/app/service/service_senders_schema.py @@ -7,5 +7,5 @@ add_service_email_reply_to_request = { "email_address": {"type": "string", "format": "email_address"}, "is_default": {"type": "boolean"} }, - "required": ["email_address"] + "required": ["email_address", "is_default"] } diff --git a/tests/app/dao/test_service_email_reply_to_dao.py b/tests/app/dao/test_service_email_reply_to_dao.py index f16dba808..5bfe190f1 100644 --- a/tests/app/dao/test_service_email_reply_to_dao.py +++ b/tests/app/dao/test_service_email_reply_to_dao.py @@ -138,6 +138,15 @@ def test_add_reply_to_email_address_ensures_first_reply_to_is_default(sample_ser email_address="first@address.com", is_default=False) +def test_add_reply_to_email_address_ensure_there_is_not_more_than_one_default(sample_service): + create_reply_to_email(service=sample_service, email_address='first@email.com', is_default=True) + create_reply_to_email(service=sample_service, email_address='second@email.com', is_default=True) + with pytest.raises(Exception): + add_reply_to_email_address_for_service(service_id=sample_service.id, + email_address='third_email@address.com', + is_default=False) + + def test_update_reply_to_email_address(sample_service): first_reply_to = create_reply_to_email(service=sample_service, email_address="first@address.com") update_reply_to_email_address(service_id=sample_service.id, reply_to_id=first_reply_to.id, diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index d5d01cd78..e0c126074 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2180,8 +2180,6 @@ def test_get_email_reply_to_addresses_with_multiple_email_addresses(client, noti reply_to_a = create_reply_to_email(service, 'test_a@mail.com') reply_to_b = create_reply_to_email(service, 'test_b@mail.com', False) - service.reply_to_email_address = 'test_a@mail.com' - response = client.get('/service/{}/email-reply-to'.format(service.id), headers=[create_authorization_header()]) json_response = json.loads(response.get_data(as_text=True)) @@ -2201,7 +2199,7 @@ def test_get_email_reply_to_addresses_with_multiple_email_addresses(client, noti def test_add_service_reply_to_email_address(client, sample_service): - data = json.dumps({"email_address": "new@reply.com"}) + data = json.dumps({"email_address": "new@reply.com", "is_default": True}) response = client.post('/service/{}/email-reply-to'.format(sample_service.id), data=data, headers=[('Content-Type', 'application/json'), create_authorization_header()]) @@ -2214,12 +2212,12 @@ def test_add_service_reply_to_email_address(client, sample_service): def test_add_service_reply_to_email_address_can_add_multiple_addresses(client, sample_service): - data = json.dumps({"email_address": "first@reply.com"}) + data = json.dumps({"email_address": "first@reply.com", "is_default": True}) client.post('/service/{}/email-reply-to'.format(sample_service.id), data=data, headers=[('Content-Type', 'application/json'), create_authorization_header()]) - second = json.dumps({"email_address": "second@reply.com"}) + second = json.dumps({"email_address": "second@reply.com", "is_default": True}) response = client.post('/service/{}/email-reply-to'.format(sample_service.id), data=second, headers=[('Content-Type', 'application/json'), create_authorization_header()]) @@ -2243,9 +2241,20 @@ def test_add_service_reply_to_email_address_raise_exception_if_no_default(client assert json_resp['message'] == 'You must have at least one reply to email address as the default.' +def test_add_service_reply_to_email_address_404s_when_invalid_service_id(client, notify_db, notify_db_session): + response = client.post('/service/{}/email-reply-to'.format(uuid.uuid4()), + data={}, + headers=[('Content-Type', 'application/json'), create_authorization_header()]) + + assert response.status_code == 404 + result = json.loads(response.get_data(as_text=True)) + assert result['result'] == 'error' + assert result['message'] == 'No result found' + + def test_update_service_reply_to_email_address(client, sample_service): original_reply_to = create_reply_to_email(service=sample_service, email_address="some@email.com") - data = json.dumps({"email_address": "changed@reply.com"}) + data = json.dumps({"email_address": "changed@reply.com", "is_default": True}) response = client.post('/service/{}/email-reply-to/{}'.format(sample_service.id, original_reply_to.id), data=data, headers=[('Content-Type', 'application/json'), create_authorization_header()]) @@ -2267,3 +2276,14 @@ def test_update_service_reply_to_email_address_returns_400_when_no_default(clien assert response.status_code == 400 json_resp = json.loads(response.get_data(as_text=True)) assert json_resp['message'] == 'You must have at least one reply to email address as the default.' + + +def test_update_service_reply_to_email_address_404s_when_invalid_service_id(client, notify_db, notify_db_session): + response = client.post('/service/{}/email-reply-to/{}'.format(uuid.uuid4(), uuid.uuid4()), + data={}, + headers=[('Content-Type', 'application/json'), create_authorization_header()]) + + assert response.status_code == 404 + result = json.loads(response.get_data(as_text=True)) + assert result['result'] == 'error' + assert result['message'] == 'No result found' From 877d8a2877370b82d85d18f3d7396530db2cbdd8 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 20 Sep 2017 12:20:12 +0100 Subject: [PATCH 13/22] Remove the default value - expect it to always be set. --- app/dao/service_email_reply_to_dao.py | 4 ++-- tests/app/dao/test_service_email_reply_to_dao.py | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/dao/service_email_reply_to_dao.py b/app/dao/service_email_reply_to_dao.py index 70e07e02d..ef94a7130 100644 --- a/app/dao/service_email_reply_to_dao.py +++ b/app/dao/service_email_reply_to_dao.py @@ -40,7 +40,7 @@ def dao_update_reply_to_email(reply_to): @transactional -def add_reply_to_email_address_for_service(service_id, email_address, is_default=True): +def add_reply_to_email_address_for_service(service_id, email_address, is_default): old_default = _get_existing_default(service_id) if is_default: _reset_old_default_to_false(old_default) @@ -53,7 +53,7 @@ def add_reply_to_email_address_for_service(service_id, email_address, is_default @transactional -def update_reply_to_email_address(service_id, reply_to_id, email_address, is_default=True): +def update_reply_to_email_address(service_id, reply_to_id, email_address, is_default): old_default = _get_existing_default(service_id) if is_default: _reset_old_default_to_false(old_default) diff --git a/tests/app/dao/test_service_email_reply_to_dao.py b/tests/app/dao/test_service_email_reply_to_dao.py index 5bfe190f1..1761b44eb 100644 --- a/tests/app/dao/test_service_email_reply_to_dao.py +++ b/tests/app/dao/test_service_email_reply_to_dao.py @@ -69,7 +69,9 @@ def test_dao_get_reply_to_by_service_id(notify_db_session): def test_add_reply_to_email_address_for_service_creates_first_email_for_service(notify_db_session): service = create_service() - add_reply_to_email_address_for_service(service_id=service.id, email_address='new@address.com') + add_reply_to_email_address_for_service(service_id=service.id, + email_address='new@address.com', + is_default=True) results = dao_get_reply_to_by_service_id(service_id=service.id) assert len(results) == 1 @@ -112,7 +114,8 @@ def test_add_reply_to_email_address_new_reply_to_is_default_existing_reply_to_is def test_add_reply_to_email_address_can_add_a_third_reply_to_address(sample_service): add_reply_to_email_address_for_service(service_id=sample_service.id, - email_address="first@address.com") + email_address="first@address.com", + is_default=True) add_reply_to_email_address_for_service(service_id=sample_service.id, email_address='second@address.com', is_default=False) add_reply_to_email_address_for_service(service_id=sample_service.id, email_address='third@address.com', @@ -150,7 +153,8 @@ def test_add_reply_to_email_address_ensure_there_is_not_more_than_one_default(sa def test_update_reply_to_email_address(sample_service): first_reply_to = create_reply_to_email(service=sample_service, email_address="first@address.com") update_reply_to_email_address(service_id=sample_service.id, reply_to_id=first_reply_to.id, - email_address='change_address@email.com') + email_address='change_address@email.com', + is_default=True) updated_reply_to = ServiceEmailReplyTo.query.get(first_reply_to.id) assert updated_reply_to.email_address == 'change_address@email.com' From 4174c72f218fe305de18af58f3c203f0462907bf Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 20 Sep 2017 15:21:05 +0100 Subject: [PATCH 14/22] allow 'accepted' as a proxy for created + sending as well as 'failed' for the three failure types when querying the api --- app/models.py | 18 +++++++++--------- app/v2/notifications/notification_schemas.py | 4 ++-- tests/app/test_model.py | 15 +++++++++++---- .../v2/notifications/test_get_notifications.py | 2 +- .../notifications/test_notification_schemas.py | 4 ++-- 5 files changed, 25 insertions(+), 18 deletions(-) diff --git a/app/models.py b/app/models.py index e1a9f36fd..5dede96e9 100644 --- a/app/models.py +++ b/app/models.py @@ -1,3 +1,4 @@ +import itertools import time import uuid import datetime @@ -903,10 +904,10 @@ class Notification(db.Model): - > IN - ['failed', 'created'] + ['failed', 'created', 'accepted'] < OUT - ['technical-failure', 'temporary-failure', 'permanent-failure', 'created'] + ['technical-failure', 'temporary-failure', 'permanent-failure', 'created', 'sending'] :param status_or_statuses: a single status or list of statuses @@ -914,18 +915,17 @@ class Notification(db.Model): """ def _substitute_status_str(_status): - return NOTIFICATION_STATUS_TYPES_FAILED if _status == NOTIFICATION_FAILED else _status + return ( + NOTIFICATION_STATUS_TYPES_FAILED if _status == NOTIFICATION_FAILED else + [NOTIFICATION_CREATED, NOTIFICATION_SENDING] if _status == NOTIFICATION_STATUS_LETTER_ACCEPTED else + [_status] + ) def _substitute_status_seq(_statuses): - if NOTIFICATION_FAILED in _statuses: - _statuses = list(set( - NOTIFICATION_STATUS_TYPES_FAILED + [_s for _s in _statuses if _s != NOTIFICATION_FAILED] - )) - return _statuses + return list(set(itertools.chain.from_iterable(_substitute_status_str(status) for status in _statuses))) if isinstance(status_or_statuses, str): return _substitute_status_str(status_or_statuses) - return _substitute_status_seq(status_or_statuses) @property diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index a22ad863b..9ff028d1f 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -1,4 +1,4 @@ -from app.models import NOTIFICATION_STATUS_TYPES, TEMPLATE_TYPES +from app.models import NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_LETTER_ACCEPTED, TEMPLATE_TYPES from app.schema_validation.definitions import (uuid, personalisation, letter_personalisation) @@ -59,7 +59,7 @@ get_notifications_request = { "status": { "type": "array", "items": { - "enum": NOTIFICATION_STATUS_TYPES + "enum": NOTIFICATION_STATUS_TYPES + [NOTIFICATION_STATUS_LETTER_ACCEPTED] } }, "template_type": { diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 0dd18eb56..a20e83001 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -10,10 +10,12 @@ from app.models import ( MOBILE_TYPE, EMAIL_TYPE, NOTIFICATION_CREATED, + NOTIFICATION_SENDING, NOTIFICATION_PENDING, NOTIFICATION_FAILED, NOTIFICATION_TECHNICAL_FAILURE, - NOTIFICATION_STATUS_TYPES_FAILED + NOTIFICATION_STATUS_TYPES_FAILED, + NOTIFICATION_STATUS_LETTER_ACCEPTED ) from tests.app.conftest import ( sample_template as create_sample_template, @@ -55,8 +57,9 @@ def test_should_not_build_service_whitelist_from_invalid_contact(recipient_type, @pytest.mark.parametrize('initial_statuses, expected_statuses', [ # passing in single statuses as strings (NOTIFICATION_FAILED, NOTIFICATION_STATUS_TYPES_FAILED), - (NOTIFICATION_CREATED, NOTIFICATION_CREATED), - (NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_TECHNICAL_FAILURE), + (NOTIFICATION_STATUS_LETTER_ACCEPTED, [NOTIFICATION_SENDING, NOTIFICATION_CREATED]), + (NOTIFICATION_CREATED, [NOTIFICATION_CREATED]), + (NOTIFICATION_TECHNICAL_FAILURE, [NOTIFICATION_TECHNICAL_FAILURE]), # passing in lists containing single statuses ([NOTIFICATION_FAILED], NOTIFICATION_STATUS_TYPES_FAILED), ([NOTIFICATION_CREATED], [NOTIFICATION_CREATED]), @@ -65,13 +68,17 @@ def test_should_not_build_service_whitelist_from_invalid_contact(recipient_type, ([NOTIFICATION_FAILED, NOTIFICATION_CREATED], NOTIFICATION_STATUS_TYPES_FAILED + [NOTIFICATION_CREATED]), ([NOTIFICATION_CREATED, NOTIFICATION_PENDING], [NOTIFICATION_CREATED, NOTIFICATION_PENDING]), ([NOTIFICATION_CREATED, NOTIFICATION_TECHNICAL_FAILURE], [NOTIFICATION_CREATED, NOTIFICATION_TECHNICAL_FAILURE]), + ( + [NOTIFICATION_FAILED, NOTIFICATION_STATUS_LETTER_ACCEPTED], + NOTIFICATION_STATUS_TYPES_FAILED + [NOTIFICATION_SENDING, NOTIFICATION_CREATED] + ), # checking we don't end up with duplicates ( [NOTIFICATION_FAILED, NOTIFICATION_CREATED, NOTIFICATION_TECHNICAL_FAILURE], NOTIFICATION_STATUS_TYPES_FAILED + [NOTIFICATION_CREATED] ), ]) -def test_status_conversion_handles_failed_statuses(initial_statuses, expected_statuses): +def test_status_conversion(initial_statuses, expected_statuses): converted_statuses = Notification.substitute_status(initial_statuses) assert len(converted_statuses) == len(expected_statuses) assert set(converted_statuses) == set(expected_statuses) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 66133da75..926c8c1d8 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -393,7 +393,7 @@ def test_get_all_notifications_filter_by_status_invalid_status(client, sample_no assert json_response['status_code'] == 400 assert len(json_response['errors']) == 1 assert json_response['errors'][0]['message'] == "status elephant is not one of [created, sending, sent, " \ - "delivered, pending, failed, technical-failure, temporary-failure, permanent-failure]" + "delivered, pending, failed, technical-failure, temporary-failure, permanent-failure, accepted]" def test_get_all_notifications_filter_by_multiple_statuses(client, sample_template): diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 9e3040194..c667def11 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -26,7 +26,7 @@ def test_get_notifications_request_invalid_statuses( ): partial_error_status = "is not one of " \ "[created, sending, sent, delivered, pending, failed, " \ - "technical-failure, temporary-failure, permanent-failure]" + "technical-failure, temporary-failure, permanent-failure, accepted]" with pytest.raises(ValidationError) as e: validate({'status': invalid_statuses + valid_statuses}, get_notifications_request) @@ -73,7 +73,7 @@ def test_get_notifications_request_invalid_statuses_and_template_types(): error_messages = [error['message'] for error in errors] for invalid_status in ["elephant", "giraffe"]: assert "status {} is not one of [created, sending, sent, delivered, " \ - "pending, failed, technical-failure, temporary-failure, permanent-failure]".format( + "pending, failed, technical-failure, temporary-failure, permanent-failure, accepted]".format( invalid_status ) in error_messages From 9e818a558eaa931a23a18d245dfe7726d775e8b1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 20 Sep 2017 15:47:29 +0100 Subject: [PATCH 15/22] Fix the ServiceSchema to only dump the reply_to_email_address --- app/schemas.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 60927b671..f2fe4ab82 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -182,7 +182,7 @@ class ServiceSchema(BaseSchema): dvla_organisation = field_for(models.Service, 'dvla_organisation') permissions = fields.Method("service_permissions") override_flag = False - reply_to_email_address = fields.Method("get_reply_to_email_address") + reply_to_email_address = fields.Method(method_name="get_reply_to_email_address") def get_free_sms_fragment_limit(selfs, service): return service.free_sms_fragment_limit() @@ -195,7 +195,7 @@ class ServiceSchema(BaseSchema): class Meta: model = models.Service - dump_only = ['free_sms_fragment_limit'] + dump_only = ['free_sms_fragment_limit', 'reply_to_email_address'] exclude = ( 'updated_at', 'created_at', From 0c9f16a8d1931b860dfd60f93311a2afe7fb33cf Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 21 Sep 2017 10:21:32 +0100 Subject: [PATCH 16/22] Add id and service_id to the serialize method on ServiceEmailReplyTo --- app/models.py | 2 ++ tests/app/service/test_rest.py | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/app/models.py b/app/models.py index 38f23deff..abbf49204 100644 --- a/app/models.py +++ b/app/models.py @@ -1374,6 +1374,8 @@ class ServiceEmailReplyTo(db.Model): def serialize(self): return { + 'id': str(self.id), + 'service_id': str(self.service_id), 'email_address': self.email_address, 'is_default': self.is_default, 'created_at': self.created_at.strftime(DATETIME_FORMAT), diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index dd1e5fbb3..5b267be2c 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2186,11 +2186,15 @@ def test_get_email_reply_to_addresses_with_multiple_email_addresses(client, noti assert len(json_response) == 2 assert response.status_code == 200 + assert json_response[0]['id'] == str(reply_to_a.id) + assert json_response[0]['service_id'] == str(reply_to_a.service_id) assert json_response[0]['email_address'] == 'test_a@mail.com' assert json_response[0]['is_default'] assert json_response[0]['created_at'] assert not json_response[0]['updated_at'] + assert json_response[1]['id'] == str(reply_to_b.id) + assert json_response[1]['service_id'] == str(reply_to_b.service_id) assert json_response[1]['email_address'] == 'test_b@mail.com' assert not json_response[1]['is_default'] assert json_response[1]['created_at'] From e18e78180e23a2f38bd62b76ef17b76b0e861646 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 21 Sep 2017 11:50:49 +0100 Subject: [PATCH 17/22] update letter tests to use correct service previously they were using sample_service fixture under the hood, but with full permissions added - this works fine, **unless** there's already a service with the name "sample service" in the database. This can happen for two reasons: * A previous test didn't tear down correctly * This test already invoked the sample_service fixture somehow If this happens, we just return the existing service, without modifying its values - values that we might change in tests, such as research mode or letters permissions. In the future, we'll have to be vigilant! and aware! and careful! to not use sample_service if we're doing tests involving letters, since they create a service with a different name now --- scripts/run_tests.sh | 2 +- tests/app/celery/test_tasks.py | 86 +++++++++++++++++----------------- tests/app/conftest.py | 8 +++- tests/app/job/test_rest.py | 4 +- 4 files changed, 53 insertions(+), 47 deletions(-) diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh index b2a0ee32e..ece4a6283 100755 --- a/scripts/run_tests.sh +++ b/scripts/run_tests.sh @@ -31,5 +31,5 @@ pycodestyle . display_result $? 1 "Code style check" # run with four concurrent threads -py.test --cov=app --cov-report=term-missing tests/ --junitxml=test_results.xml -n 4 +py.test --cov=app --cov-report=term-missing tests/ --junitxml=test_results.xml -n 4 -v display_result $? 2 "Unit tests" diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index a12e6c050..d82249953 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -45,11 +45,11 @@ from app.models import ( from tests.app import load_example_csv from tests.conftest import set_config from tests.app.conftest import ( - sample_service, - sample_template, - sample_job, - sample_email_template, - sample_notification + sample_service as create_sample_service, + sample_template as create_sample_template, + sample_job as create_sample_job, + sample_email_template as create_sample_email_template, + sample_notification as create_sample_notification ) from tests.app.db import create_user, create_notification, create_job, create_service_inbound_api, create_inbound_sms @@ -82,7 +82,7 @@ def test_should_have_decorated_tasks_functions(): @pytest.fixture def email_job_with_placeholders(notify_db, notify_db_session, sample_email_template_with_placeholders): - return sample_job(notify_db, notify_db_session, template=sample_email_template_with_placeholders) + return create_sample_job(notify_db, notify_db_session, template=sample_email_template_with_placeholders) # -------------- process_job tests -------------- # @@ -123,8 +123,8 @@ def test_should_process_sms_job(sample_job, mocker): def test_should_not_process_sms_job_if_would_exceed_send_limits(notify_db, notify_db_session, mocker): - service = sample_service(notify_db, notify_db_session, limit=9) - job = sample_job(notify_db, notify_db_session, service=service, notification_count=10) + service = create_sample_service(notify_db, notify_db_session, limit=9) + job = create_sample_job(notify_db, notify_db_session, service=service, notification_count=10) mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) mocker.patch('app.celery.tasks.process_row') @@ -142,10 +142,10 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits(notify_db, def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(notify_db, notify_db_session, mocker): - service = sample_service(notify_db, notify_db_session, limit=1) - job = sample_job(notify_db, notify_db_session, service=service) + service = create_sample_service(notify_db, notify_db_session, limit=1) + job = create_sample_job(notify_db, notify_db_session, service=service) - sample_notification(notify_db, notify_db_session, service=service, job=job) + create_sample_notification(notify_db, notify_db_session, service=service, job=job) mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms')) mocker.patch('app.celery.tasks.process_row') @@ -161,11 +161,11 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(notify def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(notify_db, notify_db_session, mocker): - service = sample_service(notify_db, notify_db_session, limit=1) - template = sample_email_template(notify_db, notify_db_session, service=service) - job = sample_job(notify_db, notify_db_session, service=service, template=template) + service = create_sample_service(notify_db, notify_db_session, limit=1) + template = create_sample_email_template(notify_db, notify_db_session, service=service) + job = create_sample_job(notify_db, notify_db_session, service=service, template=template) - sample_notification(notify_db, notify_db_session, service=service, job=job) + create_sample_notification(notify_db, notify_db_session, service=service, job=job) mocker.patch('app.celery.tasks.s3.get_job_from_s3') mocker.patch('app.celery.tasks.process_row') @@ -182,9 +182,9 @@ def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(noti @freeze_time("2016-01-01 11:09:00.061258") def test_should_not_process_email_job_if_would_exceed_send_limits(notify_db, notify_db_session, mocker): - service = sample_service(notify_db, notify_db_session, limit=0) - template = sample_email_template(notify_db, notify_db_session, service=service) - job = sample_job(notify_db, notify_db_session, service=service, template=template) + service = create_sample_service(notify_db, notify_db_session, limit=0) + template = create_sample_email_template(notify_db, notify_db_session, service=service) + job = create_sample_job(notify_db, notify_db_session, service=service, template=template) mocker.patch('app.celery.tasks.s3.get_job_from_s3') mocker.patch('app.celery.tasks.process_row') @@ -200,7 +200,7 @@ def test_should_not_process_email_job_if_would_exceed_send_limits(notify_db, not def test_should_not_process_job_if_already_pending(notify_db, notify_db_session, mocker): - job = sample_job(notify_db, notify_db_session, job_status='scheduled') + job = create_sample_job(notify_db, notify_db_session, job_status='scheduled') mocker.patch('app.celery.tasks.s3.get_job_from_s3') mocker.patch('app.celery.tasks.process_row') @@ -217,9 +217,9 @@ def test_should_not_process_job_if_already_pending(notify_db, notify_db_session, def test_should_process_email_job_if_exactly_on_send_limits(notify_db, notify_db_session, mocker): - service = sample_service(notify_db, notify_db_session, limit=10) - template = sample_email_template(notify_db, notify_db_session, service=service) - job = sample_job(notify_db, notify_db_session, service=service, template=template, notification_count=10) + service = create_sample_service(notify_db, notify_db_session, limit=10) + template = create_sample_email_template(notify_db, notify_db_session, service=service) + job = create_sample_job(notify_db, notify_db_session, service=service, template=template, notification_count=10) mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_email')) mocker.patch('app.celery.tasks.send_email.apply_async') @@ -429,11 +429,11 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_service(notify_db, notify_db_session, mocker): - service = sample_service(notify_db, notify_db_session) + service = create_sample_service(notify_db, notify_db_session) service.research_mode = True services_dao.dao_update_service(service) - template = sample_template(notify_db, notify_db_session, service=service) + template = create_sample_template(notify_db, notify_db_session, service=service) notification = _notification_json(template, to="+447234123123") @@ -457,8 +457,8 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notify_db_session, mocker): user = create_user(mobile_number="07700 900890") - service = sample_service(notify_db, notify_db_session, user=user, restricted=True) - template = sample_template(notify_db, notify_db_session, service=service) + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + template = create_sample_template(notify_db, notify_db_session, service=service) notification = _notification_json(template, "+447700900890") # The user’s own number, but in a different format mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') @@ -493,8 +493,8 @@ def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key notify_db_session, mocker): user = create_user(mobile_number="07700 900205") - service = sample_service(notify_db, notify_db_session, user=user, restricted=True) - template = sample_template(notify_db, notify_db_session, service=service) + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + template = create_sample_template(notify_db, notify_db_session, service=service) notification = _notification_json(template, "07700 900849") mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') @@ -519,8 +519,8 @@ def test_should_send_email_if_restricted_service_and_non_team_email_address_with notify_db_session, mocker): user = create_user() - service = sample_service(notify_db, notify_db_session, user=user, restricted=True) - template = sample_template( + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + template = create_sample_template( notify_db, notify_db_session, service=service, template_type='email', subject_line='Hello' ) @@ -545,8 +545,8 @@ def test_should_send_email_if_restricted_service_and_non_team_email_address_with def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): user = create_user(mobile_number="07700 900205") - service = sample_service(notify_db, notify_db_session, user=user, restricted=True) - template = sample_template(notify_db, notify_db_session, service=service) + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + template = create_sample_template(notify_db, notify_db_session, service=service) notification = _notification_json(template, "07700 900849") mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') @@ -564,8 +564,8 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, def test_should_not_send_email_if_restricted_service_and_invalid_email_address(notify_db, notify_db_session, mocker): user = create_user() - service = sample_service(notify_db, notify_db_session, user=user, restricted=True) - template = sample_template( + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + template = create_sample_template( notify_db, notify_db_session, service=service, template_type='email', subject_line='Hello' ) notification = _notification_json(template, to="test@example.com") @@ -584,11 +584,11 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address(n def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_service( notify_db, notify_db_session, mocker ): - service = sample_service(notify_db, notify_db_session) + service = create_sample_service(notify_db, notify_db_session) service.research_mode = True services_dao.dao_update_service(service) - template = sample_email_template(notify_db, notify_db_session, service=service) + template = create_sample_email_template(notify_db, notify_db_session, service=service) notification = _notification_json(template, to="test@test.com") @@ -611,10 +611,10 @@ def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_serv def test_should_not_build_dvla_file_in_research_mode_for_letter_job( - mocker, sample_service, sample_letter_job, fake_uuid + mocker, sample_letter_job, fake_uuid ): test_encrypted_data = 'some encrypted data' - sample_service.research_mode = True + sample_letter_job.service.research_mode = True csv = """address_line_1,address_line_2,address_line_3,address_line_4,postcode,name A1,A2,A3,A4,A_POST,Alice @@ -633,10 +633,10 @@ def test_should_not_build_dvla_file_in_research_mode_for_letter_job( @freeze_time("2017-08-29 17:30:00") def test_should_update_job_to_sent_to_dvla_in_research_mode_for_letter_job( - mocker, sample_service, sample_letter_job, fake_uuid + mocker, sample_letter_job, fake_uuid ): test_encrypted_data = 'some encrypted data' - sample_service.research_mode = True + sample_letter_job.service.research_mode = True csv = """address_line_1,address_line_2,address_line_3,address_line_4,postcode,name A1,A2,A3,A4,A_POST,Alice @@ -654,7 +654,7 @@ def test_should_update_job_to_sent_to_dvla_in_research_mode_for_letter_job( persist_letter.apply_async.assert_called_once_with( ( - str(sample_service.id), + str(sample_letter_job.service_id), fake_uuid, test_encrypted_data, datetime.utcnow().strftime(DATETIME_FORMAT) @@ -737,8 +737,8 @@ def test_should_not_send_email_if_team_key_and_recipient_not_in_team(sample_emai def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, notify_db_session, mocker): assert Notification.query.count() == 0 user = create_user(mobile_number="07700 900205") - service = sample_service(notify_db, notify_db_session, user=user, restricted=True) - template = sample_template(notify_db, notify_db_session, service=service) + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + template = create_sample_template(notify_db, notify_db_session, service=service) team_members = [user.mobile_number for user in service.users] assert "07890 300000" not in team_members diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 0acfb3c39..c3a9d4b9f 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -172,7 +172,13 @@ def sample_service( @pytest.fixture(scope='function') def sample_service_full_permissions(notify_db, notify_db_session): - return sample_service(notify_db, notify_db_session, permissions=SERVICE_PERMISSION_TYPES) + return sample_service( + notify_db, + notify_db_session, + # ensure name doesn't clash with regular sample service + service_name="sample service full permissions", + permissions=SERVICE_PERMISSION_TYPES + ) @pytest.fixture(scope='function') diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 920bc3397..f9f0c2929 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -187,7 +187,7 @@ def test_create_job_returns_403_if_service_is_not_active(client, fake_uuid, samp def test_create_job_returns_403_if_letter_template_type_and_service_in_trial( - client, fake_uuid, sample_service, sample_trial_letter_template, mocker): + client, fake_uuid, sample_trial_letter_template, mocker): data = { 'id': fake_uuid, 'service': str(sample_trial_letter_template.service.id), @@ -198,7 +198,7 @@ def test_create_job_returns_403_if_letter_template_type_and_service_in_trial( } mock_job_dao = mocker.patch("app.dao.jobs_dao.dao_create_job") auth_header = create_authorization_header() - response = client.post('/service/{}/job'.format(sample_service.id), + response = client.post('/service/{}/job'.format(sample_trial_letter_template.service.id), data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) From ee5888f07f811065adcb98d01f4668de9cb5ea57 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 21 Sep 2017 13:37:57 +0100 Subject: [PATCH 18/22] Fix sending emails with no logo Code was not expecting logo to be `None`, thereby causing the task to throw an exception, and retry until eventually putting the email in technical error (for services with org branding but no logo). --- app/delivery/send_to_providers.py | 3 ++- tests/app/delivery/test_send_to_providers.py | 12 ++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 7d90b07d3..6cbd00e2d 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -179,10 +179,11 @@ def get_html_email_options(service): govuk_banner = service.branding not in (BRANDING_ORG, BRANDING_ORG_BANNER) brand_banner = service.branding == BRANDING_ORG_BANNER if service.organisation and service.branding != BRANDING_GOVUK: + logo_url = get_logo_url( current_app.config['ADMIN_BASE_URL'], service.organisation.logo - ) + ) if service.organisation.logo else None branding = { 'brand_colour': service.organisation.colour, diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index fb52e8b98..c88b7f380 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -459,6 +459,18 @@ def test_get_html_email_renderer_prepends_logo_path(notify_api): assert renderer['brand_logo'] == 'http://static-logos.notify.tools/justice-league.png' +def test_get_html_email_renderer_handles_org_without_logo(notify_api): + Service = namedtuple('Service', ['branding', 'organisation']) + Organisation = namedtuple('Organisation', ['colour', 'name', 'logo']) + + org = Organisation(colour='#000000', logo=None, name='Justice League') + service = Service(branding=BRANDING_ORG, organisation=org) + + renderer = send_to_providers.get_html_email_options(service) + + assert renderer['brand_logo'] is None + + @pytest.mark.parametrize('base_url, expected_url', [ # don't change localhost to prevent errors when testing locally ('http://localhost:6012', 'http://static-logos.notify.tools/filename.png'), From 366d07dbbed9d0f753479abfb15da54eb16c5133 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 21 Sep 2017 16:08:49 +0100 Subject: [PATCH 19/22] Add ServiceLetterContact data model and script --- app/models.py | 30 +++++++++++++++++ .../0122_add_service_letter_contact.py | 32 +++++++++++++++++++ tests/app/db.py | 21 +++++++++++- tests/app/test_model.py | 19 ++++++++++- 4 files changed, 100 insertions(+), 2 deletions(-) create mode 100644 migrations/versions/0122_add_service_letter_contact.py diff --git a/app/models.py b/app/models.py index 4b3c24b94..5739f1659 100644 --- a/app/models.py +++ b/app/models.py @@ -257,6 +257,14 @@ class Service(db.Model, Versioned): else: return default_reply_to[0].email_address if default_reply_to else None + def get_default_letter_contact(self): + default_letter_contact = [x for x in self.letter_contacts if x.is_default] + if len(default_letter_contact) > 1: + raise Exception("There should only ever be one default") + else: + return default_letter_contact[0].contact_block if default_letter_contact else \ + self.letter_contact_block # need to update this to None after dropping the letter_contact_block column + class InboundNumber(db.Model): __tablename__ = "inbound_numbers" @@ -1381,3 +1389,25 @@ class ServiceEmailReplyTo(db.Model): 'created_at': self.created_at.strftime(DATETIME_FORMAT), 'updated_at': self.updated_at.strftime(DATETIME_FORMAT) if self.updated_at else None } + + +class ServiceLetterContact(db.Model): + __tablename__ = "service_letter_contacts" + + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), unique=False, index=True, nullable=False) + service = db.relationship(Service, backref=db.backref("letter_contacts")) + + contact_block = db.Column(db.Text, nullable=False, index=False, unique=False) + is_default = db.Column(db.Boolean, nullable=False, default=True) + created_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) + updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + + def serialize(self): + return { + 'contact_block': self.contact_block, + 'is_default': self.is_default, + 'created_at': self.created_at.strftime(DATETIME_FORMAT), + 'updated_at': self.updated_at.strftime(DATETIME_FORMAT) if self.updated_at else None + } diff --git a/migrations/versions/0122_add_service_letter_contact.py b/migrations/versions/0122_add_service_letter_contact.py new file mode 100644 index 000000000..2fbe904c2 --- /dev/null +++ b/migrations/versions/0122_add_service_letter_contact.py @@ -0,0 +1,32 @@ +""" + +Revision ID: 0122_add_service_letter_contact +Revises: 0121_nullable_logos +Create Date: 2017-09-21 12:16:02.975120 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0122_add_service_letter_contact' +down_revision = '0121_nullable_logos' + + +def upgrade(): + op.create_table('service_letter_contacts', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('contact_block', sa.Text(), nullable=False), + sa.Column('is_default', sa.Boolean(), nullable=False), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=True), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_index(op.f('ix_service_letter_contact_service_id'), 'service_letter_contacts', ['service_id'], unique=False) + + +def downgrade(): + op.drop_index(op.f('ix_service_letter_contact_service_id'), table_name='service_letter_contacts') + op.drop_table('service_letter_contacts') diff --git a/tests/app/db.py b/tests/app/db.py index 50c04bbde..f271d70e9 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -26,7 +26,8 @@ from app.models import ( INBOUND_SMS_TYPE, KEY_TYPE_NORMAL, ServiceInboundApi, - ServiceEmailReplyTo + ServiceEmailReplyTo, + ServiceLetterContact ) from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification, dao_created_scheduled_notification @@ -347,3 +348,21 @@ def create_reply_to_email( db.session.commit() return reply_to + + +def create_letter_contact( + service, + contact_block, + is_default=True +): + data = { + 'service': service, + 'contact_block': contact_block, + 'is_default': is_default, + } + letter_content = ServiceLetterContact(**data) + + db.session.add(letter_content) + db.session.commit() + + return letter_content diff --git a/tests/app/test_model.py b/tests/app/test_model.py index b0bf01a4e..0303782ff 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -21,7 +21,13 @@ from tests.app.conftest import ( sample_template as create_sample_template, sample_notification_with_job as create_sample_notification_with_job ) -from tests.app.db import create_notification, create_service, create_inbound_number, create_reply_to_email +from tests.app.db import ( + create_notification, + create_service, + create_inbound_number, + create_reply_to_email, + create_letter_contact +) from tests.conftest import set_config @@ -262,3 +268,14 @@ def test_service_get_default_reply_to_email_address(sample_service): create_reply_to_email(service=sample_service, email_address="default@email.com") assert sample_service.get_default_reply_to_email_address() == 'default@email.com' + + +def test_service_get_default_contact_letter(sample_service): + create_letter_contact(service=sample_service, contact_block='London,\nNW1A 1AA') + + assert sample_service.get_default_letter_contact() == 'London,\nNW1A 1AA' + + +# this test will need to be removed after letter_contact_block is dropped +def test_service_get_default_letter_contact_block_from_service(sample_service): + assert sample_service.get_default_letter_contact() == sample_service.letter_contact_block From 91a618531dd1cb0dd069f96666792b63b0db8afa Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 21 Sep 2017 16:38:24 +0100 Subject: [PATCH 20/22] Update serialization and service schema - added id and service_id in serialization - added 'letter_contacts' to the exluded list for marshmallow service schema --- app/models.py | 2 ++ app/schemas.py | 1 + 2 files changed, 3 insertions(+) diff --git a/app/models.py b/app/models.py index 5739f1659..639d619d3 100644 --- a/app/models.py +++ b/app/models.py @@ -1406,6 +1406,8 @@ class ServiceLetterContact(db.Model): def serialize(self): return { + 'id': str(self.id), + 'service_id': str(self.service_id), 'contact_block': self.contact_block, 'is_default': self.is_default, 'created_at': self.created_at.strftime(DATETIME_FORMAT), diff --git a/app/schemas.py b/app/schemas.py index f2fe4ab82..96f64a24c 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -209,6 +209,7 @@ class ServiceSchema(BaseSchema): 'service_sms_senders', 'monthly_billing', 'reply_to_email_addresses', + 'letter_contacts', ) strict = True From 795bd4271ce66d1cdd9e38979f7ba506a23135cb Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 21 Sep 2017 17:02:58 +0100 Subject: [PATCH 21/22] New endpoint to fetch a single reply-to email address by id --- app/dao/service_email_reply_to_dao.py | 10 +++++++++ app/service/rest.py | 8 ++++++- .../dao/test_service_email_reply_to_dao.py | 22 ++++++++++++++++++- tests/app/service/test_rest.py | 11 ++++++++++ 4 files changed, 49 insertions(+), 2 deletions(-) diff --git a/app/dao/service_email_reply_to_dao.py b/app/dao/service_email_reply_to_dao.py index ef94a7130..d63395ab8 100644 --- a/app/dao/service_email_reply_to_dao.py +++ b/app/dao/service_email_reply_to_dao.py @@ -13,6 +13,16 @@ def dao_get_reply_to_by_service_id(service_id): return reply_to +def dao_get_reply_to_by_id(service_id, reply_to_id): + reply_to = db.session.query( + ServiceEmailReplyTo + ).filter( + ServiceEmailReplyTo.service_id == service_id, + ServiceEmailReplyTo.id == reply_to_id + ).order_by(ServiceEmailReplyTo.created_at).one() + return reply_to + + def create_or_update_email_reply_to(service_id, email_address): reply_to = dao_get_reply_to_by_service_id(service_id) if len(reply_to) == 0: diff --git a/app/service/rest.py b/app/service/rest.py index 61527ce1c..48f1e6f44 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -47,7 +47,7 @@ from app.dao.service_whitelist_dao import ( dao_remove_service_whitelist ) from app.dao.service_email_reply_to_dao import create_or_update_email_reply_to, dao_get_reply_to_by_service_id, \ - add_reply_to_email_address_for_service, update_reply_to_email_address + add_reply_to_email_address_for_service, update_reply_to_email_address, dao_get_reply_to_by_id from app.dao.provider_statistics_dao import get_fragment_count from app.dao.users_dao import get_user_by_id from app.errors import ( @@ -531,6 +531,12 @@ def get_email_reply_to_addresses(service_id): return jsonify([i.serialize() for i in result]), 200 +@service_blueprint.route('//email-reply-to/', methods=["GET"]) +def get_email_reply_to_address(service_id, reply_to_id): + result = dao_get_reply_to_by_id(service_id=service_id, reply_to_id=reply_to_id) + return jsonify(result.serialize()), 200 + + @service_blueprint.route('//email-reply-to', methods=['POST']) def add_service_reply_to_email_address(service_id): # validate the service exists, throws ResultNotFound exception. diff --git a/tests/app/dao/test_service_email_reply_to_dao.py b/tests/app/dao/test_service_email_reply_to_dao.py index 1761b44eb..f95badf80 100644 --- a/tests/app/dao/test_service_email_reply_to_dao.py +++ b/tests/app/dao/test_service_email_reply_to_dao.py @@ -1,9 +1,12 @@ +import uuid + import pytest +from sqlalchemy.exc import SQLAlchemyError from app.dao.service_email_reply_to_dao import ( create_or_update_email_reply_to, dao_get_reply_to_by_service_id, - add_reply_to_email_address_for_service, update_reply_to_email_address) + add_reply_to_email_address_for_service, update_reply_to_email_address, dao_get_reply_to_by_id) from app.errors import InvalidRequest from app.models import ServiceEmailReplyTo from tests.app.db import create_reply_to_email, create_service @@ -191,3 +194,20 @@ def test_update_reply_to_email_address_raises_exception_if_single_reply_to_and_s reply_to_id=first_reply_to.id, email_address='should@fail.com', is_default=False) + + +def test_dao_get_reply_to_by_id(sample_service): + reply_to = create_reply_to_email(service=sample_service, email_address='email@address.com') + result = dao_get_reply_to_by_id(service_id=sample_service.id, reply_to_id=reply_to.id) + assert result == reply_to + + +def test_dao_get_reply_to_by_id_raises_sqlalchemy_error_when_reply_to_does_not_exist(sample_service): + with pytest.raises(SQLAlchemyError): + dao_get_reply_to_by_id(service_id=sample_service.id, reply_to_id=uuid.uuid4()) + + +def test_dao_get_reply_to_by_id_raises_sqlalchemy_error_when_service_does_not_exist(sample_service): + reply_to = create_reply_to_email(service=sample_service, email_address='email@address.com') + with pytest.raises(SQLAlchemyError): + dao_get_reply_to_by_id(service_id=uuid.uuid4(), reply_to_id=reply_to.id) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 5b267be2c..0551ec7c5 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2290,3 +2290,14 @@ def test_update_service_reply_to_email_address_404s_when_invalid_service_id(clie result = json.loads(response.get_data(as_text=True)) assert result['result'] == 'error' assert result['message'] == 'No result found' + + +def test_get_email_reply_to_address(client, notify_db, notify_db_session): + service = create_service(notify_db=notify_db, notify_db_session=notify_db_session) + reply_to = create_reply_to_email(service, 'test_a@mail.com') + + response = client.get('/service/{}/email-reply-to/{}'.format(service.id, reply_to.id), + headers=[('Content-Type', 'application/json'), create_authorization_header()]) + + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True)) == reply_to.serialize() From 03ea09fd6ae5938cd516440da9089fa5af155679 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 22 Sep 2017 10:02:59 +0100 Subject: [PATCH 22/22] Add order by in the dao_get_reply_to_by_service_id() --- app/dao/service_email_reply_to_dao.py | 4 +++- tests/app/dao/test_service_email_reply_to_dao.py | 8 +++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/dao/service_email_reply_to_dao.py b/app/dao/service_email_reply_to_dao.py index d63395ab8..a3364ce4d 100644 --- a/app/dao/service_email_reply_to_dao.py +++ b/app/dao/service_email_reply_to_dao.py @@ -1,3 +1,5 @@ +from sqlalchemy import desc + from app import db from app.dao.dao_utils import transactional from app.errors import InvalidRequest @@ -9,7 +11,7 @@ def dao_get_reply_to_by_service_id(service_id): ServiceEmailReplyTo ).filter( ServiceEmailReplyTo.service_id == service_id - ).order_by(ServiceEmailReplyTo.created_at).all() + ).order_by(desc(ServiceEmailReplyTo.is_default), desc(ServiceEmailReplyTo.created_at)).all() return reply_to diff --git a/tests/app/dao/test_service_email_reply_to_dao.py b/tests/app/dao/test_service_email_reply_to_dao.py index f95badf80..66e5c261c 100644 --- a/tests/app/dao/test_service_email_reply_to_dao.py +++ b/tests/app/dao/test_service_email_reply_to_dao.py @@ -61,13 +61,15 @@ def test_create_or_update_email_reply_to_raises_exception_if_multilple_email_add def test_dao_get_reply_to_by_service_id(notify_db_session): service = create_service() default_reply_to = create_reply_to_email(service=service, email_address='something@email.com') + second_reply_to = create_reply_to_email(service=service, email_address='second@email.com', is_default=False) another_reply_to = create_reply_to_email(service=service, email_address='another@email.com', is_default=False) results = dao_get_reply_to_by_service_id(service_id=service.id) - assert len(results) == 2 - assert default_reply_to in results - assert another_reply_to in results + assert len(results) == 3 + assert default_reply_to == results[0] + assert another_reply_to == results[1] + assert second_reply_to == results[2] def test_add_reply_to_email_address_for_service_creates_first_email_for_service(notify_db_session):