diff --git a/app/dao/service_email_reply_to_dao.py b/app/dao/service_email_reply_to_dao.py index 3066c229d..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,17 @@ def dao_get_reply_to_by_service_id(service_id): ServiceEmailReplyTo ).filter( ServiceEmailReplyTo.service_id == service_id - ).all() + ).order_by(desc(ServiceEmailReplyTo.is_default), desc(ServiceEmailReplyTo.created_at)).all() + 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 @@ -37,3 +49,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): + 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): + 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: + 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): + 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/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 9aafe1a35..6cbd00e2d 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, @@ -108,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) @@ -174,12 +176,14 @@ 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'], service.organisation.logo - ) + ) if service.organisation.logo else None branding = { 'brand_colour': service.organisation.colour, @@ -189,7 +193,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..639d619d3 100644 --- a/app/models.py +++ b/app/models.py @@ -1,3 +1,4 @@ +import itertools import time import uuid import datetime @@ -123,6 +124,7 @@ user_to_service = db.Table( BRANDING_GOVUK = 'govuk' BRANDING_ORG = 'org' BRANDING_BOTH = 'both' +BRANDING_ORG_BANNER = 'org_banner' class BrandingTypes(db.Model): @@ -134,7 +136,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): @@ -200,7 +202,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) @@ -248,6 +250,21 @@ 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 + + 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" @@ -796,6 +813,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_ACCEPTED = 'accepted' +NOTIFICATION_STATUS_LETTER_ACCEPTED_PRETTY = 'Accepted' + class NotificationStatusTypes(db.Model): __tablename__ = 'notification_status_types' @@ -899,10 +919,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 @@ -910,18 +930,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 @@ -961,17 +980,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_ACCEPTED_PRETTY, + 'created': NOTIFICATION_STATUS_LETTER_ACCEPTED_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_ACCEPTED + 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 = { @@ -1006,7 +1037,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, @@ -1351,8 +1382,34 @@ 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, - '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 + } + + +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 { + '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), + 'updated_at': self.updated_at.strftime(DATETIME_FORMAT) if self.updated_at else None } 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/app/schemas.py b/app/schemas.py index c5de11a60..0aca1ab92 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(method_name="get_reply_to_email_address") def get_free_sms_fragment_limit(selfs, service): return service.free_sms_fragment_limit() @@ -189,9 +190,12 @@ 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'] + dump_only = ['free_sms_fragment_limit', 'reply_to_email_address'] exclude = ( 'updated_at', 'created_at', @@ -205,6 +209,7 @@ class ServiceSchema(BaseSchema): 'service_sms_senders', 'monthly_billing', 'reply_to_email_addresses', + 'letter_contacts', ) strict = True diff --git a/app/service/rest.py b/app/service/rest.py index c211fa1dd..aa8dc6347 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, 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 ( @@ -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 @@ -543,6 +545,35 @@ 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. + 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'], + 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, 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=reply_to_email_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..57b689099 --- /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", "is_default"] +} 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/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/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/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/requirements.txt b/requirements.txt index 573d446db..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.2.0#egg=notifications-utils==21.2.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 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 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/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( 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/dao/test_service_email_reply_to_dao.py b/tests/app/dao/test_service_email_reply_to_dao.py index 58ba416a8..66e5c261c 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 -) + dao_get_reply_to_by_service_id, + 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 @@ -58,10 +61,155 @@ 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) == 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): + service = create_service() + 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 + 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 - assert default_reply_to in results - assert another_reply_to in results + 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", + 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', + 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_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, + 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' + 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) + + +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/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/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 5be786361..c88b7f380 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -20,9 +20,11 @@ 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 +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): @@ -385,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, @@ -397,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() ) @@ -411,7 +413,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 +429,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 +444,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): @@ -451,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'), 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]) 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): diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 39e5f776c..0551ec7c5 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)) @@ -2189,12 +2186,118 @@ 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'] 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", "is_default": True}) + 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", "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", "is_default": True}) + 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_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", "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()]) + + 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.' + + +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' + + +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() diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 56a121e01..0303782ff 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -10,16 +10,24 @@ 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, 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, + create_letter_contact +) from tests.conftest import set_config @@ -55,8 +63,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 +74,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) @@ -116,8 +129,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', 'Accepted'), + ('letter', 'sending', 'Accepted'), + ('letter', 'technical-failure', 'Technical failure') ]) def test_notification_for_csv_returns_formatted_status( notify_db, @@ -248,3 +262,20 @@ 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' + + +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 diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 814e6dd6c..926c8c1d8 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( @@ -385,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): @@ -549,3 +557,40 @@ 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, + 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'), + 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' or noti['type'] == 'email': + assert noti['status'] == 'created' + elif noti['type'] == 'letter': + assert noti['status'] == 'accepted' + 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'] == 'accepted' 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