Merge branch 'master' of https://github.com/alphagov/notifications-api into vb-platform-admin-api

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)

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):

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
         }

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 = {

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
 

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('/<uuid:service_id>/email-reply-to/<uuid:reply_to_id>', 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('/<uuid:service_id>/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('/<uuid:service_id>/email-reply-to/<uuid:reply_to_email_id>', 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)

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"]
+}

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": {

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'")
+   

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

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')

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

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

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"

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

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')

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(

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')

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)

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

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'),

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])
 

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):

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()

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

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'

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