Merge branch 'master' into async-job-stats

app/dao/services_dao.py

@@ -127,7 +127,8 @@ def dao_update_service(service):
     db.session.add(service)
 
 
-def dao_add_user_to_service(service, user, permissions=[]):
+def dao_add_user_to_service(service, user, permissions=None):
+    permissions = permissions or []
     try:
         from app.dao.permissions_dao import permission_dao
         service.users.append(user)
@@ -214,7 +215,8 @@ def fetch_todays_total_message_count(service_id):
 def _stats_for_service_query(service_id):
     return db.session.query(
         Notification.notification_type,
-        Notification.status,
+        # see dao_fetch_todays_stats_for_all_services for why we have this label
+        Notification.status.label('status'),
         func.count(Notification.id).label('count')
     ).filter(
         Notification.service_id == service_id,
@@ -232,13 +234,13 @@ def dao_fetch_monthly_historical_stats_by_template_for_service(service_id, year)
     start_date, end_date = get_financial_year(year)
     sq = db.session.query(
         NotificationHistory.template_id,
-        NotificationHistory.status,
+        # see dao_fetch_todays_stats_for_all_services for why we have this label
+        NotificationHistory.status.label('status'),
         month.label('month'),
         func.count().label('count')
     ).filter(
         NotificationHistory.service_id == service_id,
         NotificationHistory.created_at.between(start_date, end_date)
-
     ).group_by(
         month,
         NotificationHistory.template_id,
@@ -249,7 +251,7 @@ def dao_fetch_monthly_historical_stats_by_template_for_service(service_id, year)
         Template.id.label('template_id'),
         Template.name,
         Template.template_type,
-        sq.c.status,
+        sq.c.status.label('status'),
         sq.c.count.label('count'),
         sq.c.month
     ).join(
@@ -267,7 +269,8 @@ def dao_fetch_monthly_historical_stats_for_service(service_id, year):
     start_date, end_date = get_financial_year(year)
     rows = db.session.query(
         NotificationHistory.notification_type,
-        NotificationHistory.status,
+        # see dao_fetch_todays_stats_for_all_services for why we have this label
+        NotificationHistory.status.label('status'),
         month,
         func.count(NotificationHistory.id).label('count')
     ).filter(
@@ -306,7 +309,9 @@ def dao_fetch_monthly_historical_stats_for_service(service_id, year):
 def dao_fetch_todays_stats_for_all_services(include_from_test_key=True):
     query = db.session.query(
         Notification.notification_type,
-        Notification.status,
+        # this label is necessary as the column has a different name under the hood (_status_enum / _status_fkey),
+        # if we query the Notification object there is a hybrid property to translate, but here there isn't anything.
+        Notification.status.label('status'),
         Notification.service_id,
         func.count(Notification.id).label('count')
     ).filter(
@@ -336,7 +341,8 @@ def fetch_stats_by_date_range_for_all_services(start_date, end_date, include_fro
 
     query = db.session.query(
         table.notification_type,
-        table.status,
+        # see dao_fetch_todays_stats_for_all_services for why we have this label
+        table.status.label('status'),
         table.service_id,
         func.count(table.id).label('count')
     ).filter(

app/errors.py

@@ -92,7 +92,7 @@ def register_errors(blueprint):
     @blueprint.errorhandler(SQLAlchemyError)
     def db_error(e):
         current_app.logger.exception(e)
-        if e.orig.pgerror and \
+        if hasattr(e, 'orig') and hasattr(e.orig, 'pgerror') and e.orig.pgerror and \
             ('duplicate key value violates unique constraint "services_name_key"' in e.orig.pgerror or
                 'duplicate key value violates unique constraint "services_email_from_key"' in e.orig.pgerror):
             return jsonify(

app/models.py

@@ -1,8 +1,9 @@
 import time
 import uuid
 import datetime
-from flask import url_for
+from flask import url_for, current_app
 
+from sqlalchemy.ext.hybrid import hybrid_property
 from sqlalchemy.dialects.postgresql import (
     UUID,
     JSON
@@ -46,7 +47,12 @@ class HistoryModel:
 
     def update_from_original(self, original):
         for c in self.__table__.columns:
-            setattr(self, c.name, getattr(original, c.name))
+            # in some cases, columns may have different names to their underlying db column -  so only copy those
+            # that we can, and leave it up to subclasses to deal with any oddities/properties etc.
+            if hasattr(original, c.name):
+                setattr(self, c.name, getattr(original, c.name))
+            else:
+                current_app.logger.debug('{} has no column {} to copy from'.format(original, c.name))
 
 
 class User(db.Model):
@@ -620,6 +626,12 @@ NOTIFICATION_STATUS_TYPES = [
 NOTIFICATION_STATUS_TYPES_ENUM = db.Enum(*NOTIFICATION_STATUS_TYPES, name='notify_status_type')
 
 
+class NotificationStatusTypes(db.Model):
+    __tablename__ = 'notification_status_types'
+
+    name = db.Column(db.String(255), primary_key=True)
+
+
 class Notification(db.Model):
     __tablename__ = 'notifications'
 
@@ -655,7 +667,15 @@ class Notification(db.Model):
         unique=False,
         nullable=True,
         onupdate=datetime.datetime.utcnow)
-    status = db.Column(NOTIFICATION_STATUS_TYPES_ENUM, index=True, nullable=False, default='created')
+    _status_enum = db.Column('status', NOTIFICATION_STATUS_TYPES_ENUM, index=True, nullable=False, default='created')
+    _status_fkey = db.Column(
+        'notification_status',
+        db.String,
+        db.ForeignKey('notification_status_types.name'),
+        index=True,
+        nullable=True,
+        default='created'
+    )
     reference = db.Column(db.String, nullable=True, index=True)
     client_reference = db.Column(db.String, index=True, nullable=True)
     _personalisation = db.Column(db.String, nullable=True)
@@ -671,6 +691,15 @@ class Notification(db.Model):
     phone_prefix = db.Column(db.String, nullable=True)
     rate_multiplier = db.Column(db.Float(asdecimal=False), nullable=True)
 
+    @hybrid_property
+    def status(self):
+        return self._status_enum
+
+    @status.setter
+    def status(self, status):
+        self._status_fkey = status
+        self._status_enum = status
+
     @property
     def personalisation(self):
         if self._personalisation:
@@ -843,7 +872,15 @@ class NotificationHistory(db.Model, HistoryModel):
     sent_at = db.Column(db.DateTime, index=False, unique=False, nullable=True)
     sent_by = db.Column(db.String, nullable=True)
     updated_at = db.Column(db.DateTime, index=False, unique=False, nullable=True)
-    status = db.Column(NOTIFICATION_STATUS_TYPES_ENUM, index=True, nullable=False, default='created')
+    _status_enum = db.Column('status', NOTIFICATION_STATUS_TYPES_ENUM, index=True, nullable=False, default='created')
+    _status_fkey = db.Column(
+        'notification_status',
+        db.String,
+        db.ForeignKey('notification_status_types.name'),
+        index=True,
+        nullable=True,
+        default='created'
+    )
     reference = db.Column(db.String, nullable=True, index=True)
     client_reference = db.Column(db.String, nullable=True)
 
@@ -854,8 +891,22 @@ class NotificationHistory(db.Model, HistoryModel):
     @classmethod
     def from_original(cls, notification):
         history = super().from_original(notification)
+        history.status = notification.status
         return history
 
+    def update_from_original(self, original):
+        super().update_from_original(original)
+        self.status = original.status
+
+    @hybrid_property
+    def status(self):
+        return self._status_enum
+
+    @status.setter
+    def status(self, status):
+        self._status_fkey = status
+        self._status_enum = status
+
 
 INVITED_USER_STATUS_TYPES = ['pending', 'accepted', 'cancelled']
 

app/schema_validation/__init__.py

@@ -10,13 +10,13 @@ def validate(json_to_validate, schema):
 
     @format_checker.checks('phone_number', raises=InvalidPhoneError)
     def validate_schema_phone_number(instance):
-        if instance is not None:
+        if isinstance(instance, str):
             validate_phone_number(instance, international=True)
         return True
 
     @format_checker.checks('email_address', raises=InvalidEmailError)
     def validate_schema_email_address(instance):
-        if instance is not None:
+        if isinstance(instance, str):
             validate_email_address(instance)
         return True
 

app/schemas.py

@@ -220,7 +220,9 @@ class NotificationModelSchema(BaseSchema):
     class Meta:
         model = models.Notification
         strict = True
-        exclude = ('_personalisation', 'job', 'service', 'template', 'api_key', '')
+        exclude = ('_personalisation', 'job', 'service', 'template', 'api_key', '_status_enum', '_status_fkey')
+
+    status = fields.String(required=False)
 
 
 class BaseTemplateSchema(BaseSchema):
@@ -315,6 +317,7 @@ class NotificationSchema(ma.Schema):
     class Meta:
         strict = True
 
+    status = fields.String(required=False)
     personalisation = fields.Dict(required=False)
 
 
@@ -369,7 +372,7 @@ class NotificationWithTemplateSchema(BaseSchema):
     class Meta:
         model = models.Notification
         strict = True
-        exclude = ('_personalisation',)
+        exclude = ('_personalisation', '_status_enum', '_status_fkey')
 
     template = fields.Nested(
         TemplateSchema,
@@ -377,6 +380,7 @@ class NotificationWithTemplateSchema(BaseSchema):
         dump_only=True
     )
     job = fields.Nested(JobSchema, only=["id", "original_file_name"], dump_only=True)
+    status = fields.String(required=False)
     personalisation = fields.Dict(required=False)
     key_type = field_for(models.Notification, 'key_type', required=True)
     key_name = fields.String()

migrations/versions/0081_noti_status_as_enum.py

@@ -0,0 +1,63 @@
+"""empty message
+
+Revision ID: 0081_noti_status_as_enum
+Revises: 0080_fix_rate_start_date
+Create Date: 2017-05-02 14:50:04.070874
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '0081_noti_status_as_enum'
+down_revision = '0080_fix_rate_start_date'
+
+from alembic import op
+import sqlalchemy as sa
+
+
+def upgrade():
+    status_table = op.create_table('notification_status_types',
+        sa.Column('name', sa.String(), nullable=False),
+        sa.PrimaryKeyConstraint('name')
+    )
+    op.bulk_insert(status_table,
+        [
+            {'name': x} for x in {
+                'created',
+                'sending',
+                'delivered',
+                'pending',
+                'failed',
+                'technical-failure',
+                'temporary-failure',
+                'permanent-failure',
+                'sent',
+            }
+        ]
+    )
+
+    op.execute('ALTER TABLE notifications ADD COLUMN notification_status text')
+    op.execute('ALTER TABLE notification_history ADD COLUMN notification_status text')
+
+    op.create_index(op.f('ix_notifications_notification_status'), 'notifications', ['notification_status'])
+    op.create_index(op.f('ix_notification_history_notification_status'), 'notification_history', ['notification_status'])
+    op.create_foreign_key(
+        'fk_notifications_notification_status',
+        'notifications',
+        'notification_status_types',
+        ['notification_status'],
+        ['name'],
+    )
+    op.create_foreign_key(
+        'fk_notification_history_notification_status',
+        'notification_history',
+        'notification_status_types',
+        ['notification_status'],
+        ['name'],
+    )
+
+
+
+def downgrade():
+    op.execute('ALTER TABLE notifications DROP COLUMN notification_status')
+    op.execute('ALTER TABLE notification_history DROP COLUMN notification_status')
+    op.execute('DROP TABLE notification_status_types')

migrations/versions/0082_add_golive_template.py

@@ -0,0 +1,119 @@
+"""empty message
+
+Revision ID: 0082_add_go_live_template
+Revises: 0081_noti_status_as_enum
+Create Date: 2017-05-10 16:06:04.070874
+
+"""
+
+# revision identifiers, used by Alembic.
+from datetime import datetime
+
+from flask import current_app
+
+from alembic import op
+import sqlalchemy as sa
+
+revision = '0082_add_go_live_template'
+down_revision = '0081_noti_status_as_enum'
+
+template_id = '618185c6-3636-49cd-b7d2-6f6f5eb3bdde'
+
+
+def upgrade():
+    template_insert = """
+        INSERT INTO templates (id, name, template_type, created_at, content, archived, service_id, subject, created_by_id, version, process_type)
+        VALUES ('{}', '{}', '{}', '{}', '{}', False, '{}', '{}', '{}', 1, '{}')
+    """
+    template_history_insert = """
+        INSERT INTO templates_history (id, name, template_type, created_at, content, archived, service_id, subject, created_by_id, version, process_type)
+        VALUES ('{}', '{}', '{}', '{}', '{}', False, '{}', '{}', '{}', 1, '{}')
+    """
+
+    template_content = """Hi ((name)),
+
+((service name)) is now live on GOV.UK Notify.
+
+You can send up to ((message limit)) messages per day.
+
+As a live service, you’ll need to know who to contact if you have a question, or something goes wrong.
+
+^To get email updates whenever there is a problem with Notify, it’s important that you subscribe to our system status page:
+https://status.notifications.service.gov.uk
+
+If our system status page shows a problem, then we’ve been alerted and are working on it – you don’t need to contact us.
+#Problems or questions during office hours
+
+Our office hours are 9.30am to 5.30pm, Monday to Friday.
+
+To report a problem or ask a question, go to the support page:
+https://www.notifications.service.gov.uk/support
+
+We’ll reply within 30 minutes whether you’re reporting a problem or just asking a question.
+
+The team are also available to answer questions on the cross-government Slack channel:
+https://ukgovernmentdigital.slack.com/messages/govuk-notify
+
+#Problems or questions out of hours
+
+We offer out of hours support for emergencies.
+
+It’s only an emergency if:
+*   no one in your team can log in
+*   a ‘technical difficulties’ error appears when you try to upload a file
+*   a 500 response code appears when you try to send messages using the API
+
+If you have one of these emergencies, email details to:
+ooh-gov-uk-notify-support@digital.cabinet-office.gov.uk
+
+^Only use this email address for out of hours emergencies. Don’t share this address with people outside of your team.
+
+We’ll get back to you within 30 minutes and give you hourly updates until the problem’s fixed.
+
+For non-emergency problems or questions, use our support page and we’ll reply in office hours:
+https://www.notifications.service.gov.uk/support
+#Escalation for emergency problems
+
+If we haven’t acknowledged an emergency problem you’ve reported within 30 minutes and you need to know what’s happening, you can escalate to:
+
+or
+
+Thanks
+GOV.UK Notify team
+"""
+
+    template_name = "Automated \"You''re now live\" message"
+    template_subject = '((service name)) is now live on GOV.UK Notify'
+
+    op.execute(
+        template_history_insert.format(
+            template_id,
+            template_name,
+            'email',
+            datetime.utcnow(),
+            template_content,
+            current_app.config['NOTIFY_SERVICE_ID'],
+            template_subject,
+            current_app.config['NOTIFY_USER_ID'],
+            'normal'
+        )
+    )
+
+    op.execute(
+        template_insert.format(
+            template_id,
+            template_name,
+            'email',
+            datetime.utcnow(),
+            template_content,
+            current_app.config['NOTIFY_SERVICE_ID'],
+            template_subject,
+            current_app.config['NOTIFY_USER_ID'],
+            'normal'
+        )
+    )
+
+
+def downgrade():
+   op.execute("DELETE FROM templates_history WHERE id = '{}'".format(template_id))
+   op.execute("DELETE FROM templates WHERE id = '{}'".format(template_id))

tests/app/dao/test_notification_dao.py

@@ -311,6 +311,8 @@ def test_should_by_able_to_update_status_by_id(sample_template, sample_job, mmg_
         data = _notification_json(sample_template, job_id=sample_job.id, status='sending')
         notification = Notification(**data)
         dao_create_notification(notification)
+        assert notification._status_enum == 'sending'
+        assert notification._status_fkey == 'sending'
 
     assert Notification.query.get(notification.id).status == 'sending'
 
@@ -321,6 +323,8 @@ def test_should_by_able_to_update_status_by_id(sample_template, sample_job, mmg_
     assert updated.updated_at == datetime(2000, 1, 2, 12, 0, 0)
     assert Notification.query.get(notification.id).status == 'delivered'
     assert notification.updated_at == datetime(2000, 1, 2, 12, 0, 0)
+    assert notification._status_enum == 'delivered'
+    assert notification._status_fkey == 'delivered'
 
 
 def test_should_not_update_status_by_id_if_not_sending_and_does_not_update_job(notify_db, notify_db_session):
@@ -825,7 +829,7 @@ def test_get_notification_billable_unit_count_per_month(notify_db, notify_db_ses
         ) == months
 
 
-def test_update_notification(sample_notification, sample_template):
+def test_update_notification(sample_notification):
     assert sample_notification.status == 'created'
     sample_notification.status = 'failed'
     dao_update_notification(sample_notification)
@@ -833,6 +837,37 @@ def test_update_notification(sample_notification, sample_template):
     assert notification_from_db.status == 'failed'
 
 
+def test_update_notification_with_no_notification_status(sample_notification):
+    # specifically, it has an old enum status, but not a new status (because the upgrade script has just run)
+    update_dict = {'_status_enum': 'created', '_status_fkey': None}
+    Notification.query.filter(Notification.id == sample_notification.id).update(update_dict)
+
+    # now lets update the status to failed - both columns should now be populated
+    sample_notification.status = 'failed'
+    dao_update_notification(sample_notification)
+
+    notification_from_db = Notification.query.get(sample_notification.id)
+    assert notification_from_db.status == 'failed'
+    assert notification_from_db._status_enum == 'failed'
+    assert notification_from_db._status_fkey == 'failed'
+
+
+def test_updating_notification_with_no_notification_status_updates_notification_history(sample_notification):
+    # same as above, but with notification history
+    update_dict = {'_status_enum': 'created', '_status_fkey': None}
+    Notification.query.filter(Notification.id == sample_notification.id).update(update_dict)
+    NotificationHistory.query.filter(NotificationHistory.id == sample_notification.id).update(update_dict)
+
+    # now lets update the notification's status to failed - both columns should now be populated on the history object
+    sample_notification.status = 'failed'
+    dao_update_notification(sample_notification)
+
+    hist_from_db = NotificationHistory.query.get(sample_notification.id)
+    assert hist_from_db.status == 'failed'
+    assert hist_from_db._status_enum == 'failed'
+    assert hist_from_db._status_fkey == 'failed'
+
+
 @freeze_time("2016-01-10 12:00:00.000000")
 def test_should_delete_notifications_after_seven_days(notify_db, notify_db_session):
     assert len(Notification.query.all()) == 0

tests/app/job/test_rest.py

@@ -447,6 +447,25 @@ def test_get_all_notifications_for_job_filtered_by_status(
         assert response.status_code == 200
 
 
+def test_get_all_notifications_for_job_returns_correct_format(
+    client,
+    sample_notification_with_job
+):
+    service_id = sample_notification_with_job.service_id
+    job_id = sample_notification_with_job.job_id
+    response = client.get(
+        path='/service/{}/job/{}/notifications'.format(service_id, job_id),
+        headers=[create_authorization_header()]
+    )
+    assert response.status_code == 200
+    resp = json.loads(response.get_data(as_text=True))
+    assert len(resp['notifications']) == 1
+    assert resp['notifications'][0]['id'] == str(sample_notification_with_job.id)
+    assert resp['notifications'][0]['status'] == sample_notification_with_job.status
+    assert '_status_fkey' not in resp['notifications'][0]
+    assert '_status_enum' not in resp['notifications'][0]
+
+
 def test_get_job_by_id(notify_api, sample_job):
     job_id = str(sample_job.id)
     service_id = sample_job.service.id

tests/app/service/test_rest.py

@@ -1240,17 +1240,16 @@ def test_get_monthly_notification_stats(mocker, client, sample_service, url, exp
     assert json.loads(response.get_data(as_text=True)) == expected_json
 
 
-def test_get_services_with_detailed_flag(notify_api, notify_db, notify_db_session):
+def test_get_services_with_detailed_flag(client, notify_db, notify_db_session):
     notifications = [
         create_sample_notification(notify_db, notify_db_session),
         create_sample_notification(notify_db, notify_db_session),
         create_sample_notification(notify_db, notify_db_session, key_type=KEY_TYPE_TEST)
     ]
-    with notify_api.test_request_context(), notify_api.test_client() as client:
+    resp = client.get(
-        resp = client.get(
+        '/service?detailed=True',
-            '/service?detailed=True',
+        headers=[create_authorization_header()]
-            headers=[create_authorization_header()]
+    )
-        )
 
     assert resp.status_code == 200
     data = json.loads(resp.get_data(as_text=True))['data']

tests/app/test_schemas.py

@@ -1,5 +1,4 @@
 import pytest
-
 from marshmallow import ValidationError
 from sqlalchemy import desc
 
@@ -33,6 +32,22 @@ def test_notification_schema_adds_api_key_name(sample_notification_with_api_key)
     assert data['key_name'] == 'Test key'
 
 
+@pytest.mark.parametrize('schema_name', [
+    'notification_with_template_schema',
+    'notification_schema',
+    'notification_with_template_schema',
+    'notification_with_personalisation_schema',
+])
+def test_notification_schema_has_correct_status(sample_notification, schema_name):
+    from app import schemas
+
+    data = getattr(schemas, schema_name).dump(sample_notification).data
+
+    assert data['status'] == sample_notification.status
+    assert '_status_enum' not in data
+    assert '_status_fkey' not in data
+
+
 @pytest.mark.parametrize('user_attribute, user_value', [
     ('name', 'New User'),
     ('email_address', 'newuser@mail.com'),

tests/app/v2/notifications/test_notification_schemas.py

@@ -131,10 +131,15 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict():
     assert len(error.keys()) == 2
 
 
-@pytest.mark.parametrize('invalid_phone_number, err_msg',
+@pytest.mark.parametrize('invalid_phone_number, err_msg', [
-                         [('08515111111', 'phone_number Not a UK mobile number'),
+    ('08515111111', 'phone_number Not a UK mobile number'),
-                          ('07515111*11', 'phone_number Must not contain letters or symbols'),
+    ('07515111*11', 'phone_number Must not contain letters or symbols'),
-                          ('notaphoneumber', 'phone_number Must not contain letters or symbols')])
+    ('notaphoneumber', 'phone_number Must not contain letters or symbols'),
+    (7700900001, 'phone_number 7700900001 is not of type string'),
+    (None, 'phone_number None is not of type string'),
+    ([], 'phone_number [] is not of type string'),
+    ({}, 'phone_number {} is not of type string'),
+])
 def test_post_sms_request_schema_invalid_phone_number(invalid_phone_number, err_msg):
     j = {"phone_number": invalid_phone_number,
          "template_id": str(uuid.uuid4())
@@ -213,12 +218,22 @@ def test_post_email_schema_bad_uuid_and_missing_email_address():
         validate(j, post_email_request_schema)
 
 
-def test_post_email_schema_invalid_email_address():
+@pytest.mark.parametrize('email_address, err_msg', [
-    j = {"template_id": str(uuid.uuid4()),
+    ('example', 'email_address Not a valid email address'),
-         "email_address": "notavalidemail@address"}
+    (12345, 'email_address 12345 is not of type string'),
-    with pytest.raises(ValidationError):
+    (None, 'email_address None is not of type string'),
+    ([], 'email_address [] is not of type string'),
+    ({}, 'email_address {} is not of type string'),
+])
+def test_post_email_schema_invalid_email_address(email_address, err_msg):
+    j = {"template_id": str(uuid.uuid4()), "email_address": email_address}
+    with pytest.raises(ValidationError) as e:
         validate(j, post_email_request_schema)
 
+    errors = json.loads(str(e.value)).get('errors')
+    assert len(errors) == 1
+    assert {"error": "ValidationError", "message": err_msg} == errors[0]
+
 
 def valid_email_response():
     return {

tests/conftest.py

@@ -76,7 +76,8 @@ def notify_db_session(notify_db):
                             "job_status",
                             "provider_details_history",
                             "template_process_type",
-                            "dvla_organisation"]:
+                            "dvla_organisation",
+                            "notification_status_types"]:
             notify_db.engine.execute(tbl.delete())
     notify_db.session.commit()