From db91a87fb238f7e35a88f356067f4b7d1b29a750 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Wed, 2 Nov 2016 14:48:15 +0000
Subject: [PATCH 01/29] Remove link from v2 error response

---
 app/errors.py                                           | 2 --
 app/schema_validation/__init__.py                       | 1 -
 app/v2/errors.py                                        | 3 ---
 tests/app/notifications/test_validators.py              | 5 -----
 tests/app/v2/notifications/test_notification_schemas.py | 3 ---
 tests/app/v2/notifications/test_post_notifications.py   | 3 ---
 6 files changed, 17 deletions(-)

diff --git a/app/errors.py b/app/errors.py
index 8b027ba60..2588bd54a 100644
--- a/app/errors.py
+++ b/app/errors.py
@@ -10,7 +10,6 @@ from app.authentication.auth import AuthError
 
 class InvalidRequest(Exception):
     code = None
-    link = None
     fields = []
 
     def __init__(self, message, status_code):
@@ -28,7 +27,6 @@ class InvalidRequest(Exception):
         return {
             "code": self.code,
             "message": self.message,
-            "link": self.link,
             "fields": self.fields
         }
 
diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py
index 38837b467..e6a2c9748 100644
--- a/app/schema_validation/__init__.py
+++ b/app/schema_validation/__init__.py
@@ -21,7 +21,6 @@ def build_error_message(errors, schema):
     message = {
         "code": "1001",
         "message": "Validation error occurred - {}".format(schema['title']),
-        "link": "link to error documentation (not yet implemented)",
         "fields": fields
     }
 
diff --git a/app/v2/errors.py b/app/v2/errors.py
index b7bd5f586..b7f7d229c 100644
--- a/app/v2/errors.py
+++ b/app/v2/errors.py
@@ -11,9 +11,7 @@ from app.errors import InvalidRequest
 
 class TooManyRequestsError(InvalidRequest):
     status_code = 429
-    # code and link will be in a static file
     code = "10429"
-    link = "link to docs"
     message_template = 'Exceeded send limits ({}) for today'
 
     def __init__(self, sending_limit):
@@ -23,7 +21,6 @@ class TooManyRequestsError(InvalidRequest):
 class BadRequestError(InvalidRequest):
     status_code = 400
     code = 10400
-    link = "link to documentation"
     message = "An error occurred"
 
     def __init__(self, fields=[], message=None):
diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py
index e41d5f81c..7855e95eb 100644
--- a/tests/app/notifications/test_validators.py
+++ b/tests/app/notifications/test_validators.py
@@ -52,7 +52,6 @@ def test_check_template_is_for_notification_type_fails_when_template_type_does_n
     assert e.value.code == 10400
     error_message = '{0} template is not suitable for {1} notification'.format(template_type, notification_type)
     assert e.value.message == error_message
-    assert e.value.link == 'link to documentation'
     assert e.value.fields == [{'template': error_message}]
 
 
@@ -69,7 +68,6 @@ def test_check_template_is_active_fails(sample_template):
     assert e.value.status_code == 400
     assert e.value.code == 10400
     assert e.value.message == 'Template has been deleted'
-    assert e.value.link == "link to documentation"
     assert e.value.fields == [{'template': 'Template has been deleted'}]
 
 
@@ -124,7 +122,6 @@ def test_service_can_send_to_recipient_fails_when_recipient_is_not_on_team(recip
     assert exec_info.value.status_code == 400
     assert exec_info.value.code == 10400
     assert exec_info.value.message == error_message
-    assert exec_info.value.link == 'link to documentation'
     assert exec_info.value.fields == []
 
 
@@ -137,7 +134,6 @@ def test_service_can_send_to_recipient_fails_when_mobile_number_is_not_on_team(n
     assert e.value.status_code == 400
     assert e.value.code == 10400
     assert e.value.message == 'Can’t send to this recipient using a team-only API key'
-    assert e.value.link == 'link to documentation'
     assert e.value.fields == []
 
 
@@ -154,5 +150,4 @@ def test_check_sms_content_char_count_fails(char_count, notify_api):
     assert e.value.code == 10400
     assert e.value.message == 'Content for template has a character count greater than the limit of {}'.format(
         notify_api.config['SMS_CHAR_COUNT_LIMIT'])
-    assert e.value.link == 'link to documentation'
     assert e.value.fields == []
diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py
index 96f9e1284..13ccc1124 100644
--- a/tests/app/v2/notifications/test_notification_schemas.py
+++ b/tests/app/v2/notifications/test_notification_schemas.py
@@ -33,7 +33,6 @@ def test_post_sms_json_schema_bad_uuid_and_missing_phone_number():
     assert {"phone_number": "is a required property"} in error['fields']
     assert {"template_id": "not a valid UUID"} in error['fields']
     assert error.get('code') == '1001'
-    assert error.get('link', None) is not None
 
 
 def test_post_sms_schema_with_personalisation_that_is_not_a_dict():
@@ -50,7 +49,6 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict():
     assert len(error.get('fields')) == 1
     assert error['fields'][0] == {"personalisation": "should contain key value pairs"}
     assert error.get('code') == '1001'
-    assert error.get('link', None) == 'link to error documentation (not yet implemented)'
 
 
 valid_response = {
@@ -87,6 +85,5 @@ def test_post_sms_response_schema_missing_uri():
         validate(j, post_sms_response)
     error = json.loads(e.value.message)
     assert '1001' == error['code']
-    assert 'link to error documentation (not yet implemented)' == error['link']
     assert 'Validation error occurred - response v2/notifications/sms' == error['message']
     assert [{"uri": "is a required property"}] == error['fields']
diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py
index bbc07e309..e8e500410 100644
--- a/tests/app/v2/notifications/test_post_notifications.py
+++ b/tests/app/v2/notifications/test_post_notifications.py
@@ -58,7 +58,6 @@ def test_post_sms_notification_returns_404_and_missing_template(notify_api, samp
             assert error_json['code'] == 10400
             assert error_json['message'] == 'Template not found'
             assert error_json['fields'] == [{'template': 'Template not found'}]
-            assert error_json['link'] == 'link to documentation'
 
 
 def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api, sample_template):
@@ -80,7 +79,6 @@ def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api
             assert error_resp['code'] == 401
             assert error_resp['message'] == 'Unauthorized, authentication token must be provided'
             assert error_resp['fields'] == {'token': ['Unauthorized, authentication token must be provided']}
-            assert error_resp['link'] == 'link to docs'
 
 
 def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, sample_template):
@@ -102,5 +100,4 @@ def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, s
             error_resp = json.loads(response.get_data(as_text=True))
             assert error_resp['code'] == '1001'
             assert error_resp['message'] == 'Validation error occurred - POST v2/notifications/sms'
-            assert error_resp['link'] == "link to error documentation (not yet implemented)"
             assert error_resp['fields'] == [{"template_id": "is a required property"}]

From 4cb38e2d12fd66d59af11976939158f851134054 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Wed, 2 Nov 2016 14:58:39 +0000
Subject: [PATCH 02/29] Use status_code in error response. Remove code.

---
 app/authentication/auth.py                              | 9 +++++----
 app/errors.py                                           | 2 +-
 app/schema_validation/__init__.py                       | 2 +-
 app/v2/errors.py                                        | 2 --
 tests/app/notifications/test_validators.py              | 7 +------
 tests/app/v2/notifications/test_notification_schemas.py | 6 +++---
 tests/app/v2/notifications/test_post_notifications.py   | 5 ++---
 7 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/app/authentication/auth.py b/app/authentication/auth.py
index 0ca700a62..0f18f2322 100644
--- a/app/authentication/auth.py
+++ b/app/authentication/auth.py
@@ -16,10 +16,11 @@ class AuthError(Exception):
         self.code = code
 
     def to_dict_v2(self):
-        return {'code': self.code,
-                'message': self.short_message,
-                'fields': self.message,
-                'link': 'link to docs'}
+        return {
+            'status_code': self.code,
+            'message': self.short_message,
+            'fields': self.message
+        }
 
 
 def get_auth_token(req):
diff --git a/app/errors.py b/app/errors.py
index 2588bd54a..db5826cbd 100644
--- a/app/errors.py
+++ b/app/errors.py
@@ -25,7 +25,7 @@ class InvalidRequest(Exception):
         Version 2 of the public api error response.
         '''
         return {
-            "code": self.code,
+            "status_code": self.code,
             "message": self.message,
             "fields": self.fields
         }
diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py
index e6a2c9748..51d43df7b 100644
--- a/app/schema_validation/__init__.py
+++ b/app/schema_validation/__init__.py
@@ -19,7 +19,7 @@ def build_error_message(errors, schema):
         field = {s[1]: s[2].strip()}
         fields.append(field)
     message = {
-        "code": "1001",
+        "status_code": 400,
         "message": "Validation error occurred - {}".format(schema['title']),
         "fields": fields
     }
diff --git a/app/v2/errors.py b/app/v2/errors.py
index b7f7d229c..8c98e48f8 100644
--- a/app/v2/errors.py
+++ b/app/v2/errors.py
@@ -11,7 +11,6 @@ from app.errors import InvalidRequest
 
 class TooManyRequestsError(InvalidRequest):
     status_code = 429
-    code = "10429"
     message_template = 'Exceeded send limits ({}) for today'
 
     def __init__(self, sending_limit):
@@ -20,7 +19,6 @@ class TooManyRequestsError(InvalidRequest):
 
 class BadRequestError(InvalidRequest):
     status_code = 400
-    code = 10400
     message = "An error occurred"
 
     def __init__(self, fields=[], message=None):
diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py
index 7855e95eb..2810e75e5 100644
--- a/tests/app/notifications/test_validators.py
+++ b/tests/app/notifications/test_validators.py
@@ -28,7 +28,6 @@ def test_check_service_message_limit_over_message_limit_fails(key_type, notify_d
     with pytest.raises(TooManyRequestsError) as e:
         check_service_message_limit(key_type, service)
     assert e.value.status_code == 429
-    assert e.value.code == '10429'
     assert e.value.message == 'Exceeded send limits (4) for today'
     assert e.value.fields == []
 
@@ -49,7 +48,7 @@ def test_check_template_is_for_notification_type_fails_when_template_type_does_n
     with pytest.raises(BadRequestError) as e:
         check_template_is_for_notification_type(notification_type=notification_type,
                                                 template_type=template_type)
-    assert e.value.code == 10400
+    e.value.status_code == 400
     error_message = '{0} template is not suitable for {1} notification'.format(template_type, notification_type)
     assert e.value.message == error_message
     assert e.value.fields == [{'template': error_message}]
@@ -66,7 +65,6 @@ def test_check_template_is_active_fails(sample_template):
     with pytest.raises(BadRequestError) as e:
         check_template_is_active(sample_template)
     assert e.value.status_code == 400
-    assert e.value.code == 10400
     assert e.value.message == 'Template has been deleted'
     assert e.value.fields == [{'template': 'Template has been deleted'}]
 
@@ -120,7 +118,6 @@ def test_service_can_send_to_recipient_fails_when_recipient_is_not_on_team(recip
                                       key_type,
                                       trial_mode_service)
     assert exec_info.value.status_code == 400
-    assert exec_info.value.code == 10400
     assert exec_info.value.message == error_message
     assert exec_info.value.fields == []
 
@@ -132,7 +129,6 @@ def test_service_can_send_to_recipient_fails_when_mobile_number_is_not_on_team(n
                                       'team',
                                       live_service)
     assert e.value.status_code == 400
-    assert e.value.code == 10400
     assert e.value.message == 'Can’t send to this recipient using a team-only API key'
     assert e.value.fields == []
 
@@ -147,7 +143,6 @@ def test_check_sms_content_char_count_fails(char_count, notify_api):
     with pytest.raises(BadRequestError) as e:
         check_sms_content_char_count(char_count)
     assert e.value.status_code == 400
-    assert e.value.code == 10400
     assert e.value.message == 'Content for template has a character count greater than the limit of {}'.format(
         notify_api.config['SMS_CHAR_COUNT_LIMIT'])
     assert e.value.fields == []
diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py
index 13ccc1124..ac63ec6d7 100644
--- a/tests/app/v2/notifications/test_notification_schemas.py
+++ b/tests/app/v2/notifications/test_notification_schemas.py
@@ -32,7 +32,7 @@ def test_post_sms_json_schema_bad_uuid_and_missing_phone_number():
     assert len(error.get('fields')) == 2
     assert {"phone_number": "is a required property"} in error['fields']
     assert {"template_id": "not a valid UUID"} in error['fields']
-    assert error.get('code') == '1001'
+    assert error.get('status_code') == 400
 
 
 def test_post_sms_schema_with_personalisation_that_is_not_a_dict():
@@ -48,7 +48,7 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict():
     assert "POST v2/notifications/sms" in error['message']
     assert len(error.get('fields')) == 1
     assert error['fields'][0] == {"personalisation": "should contain key value pairs"}
-    assert error.get('code') == '1001'
+    assert error.get('status_code') == 400
 
 
 valid_response = {
@@ -84,6 +84,6 @@ def test_post_sms_response_schema_missing_uri():
     with pytest.raises(ValidationError) as e:
         validate(j, post_sms_response)
     error = json.loads(e.value.message)
-    assert '1001' == error['code']
+    assert error['status_code'] == 400
     assert 'Validation error occurred - response v2/notifications/sms' == error['message']
     assert [{"uri": "is a required property"}] == error['fields']
diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py
index e8e500410..4b93a7aa5 100644
--- a/tests/app/v2/notifications/test_post_notifications.py
+++ b/tests/app/v2/notifications/test_post_notifications.py
@@ -55,7 +55,6 @@ def test_post_sms_notification_returns_404_and_missing_template(notify_api, samp
             assert response.headers['Content-type'] == 'application/json'
 
             error_json = json.loads(response.get_data(as_text=True))
-            assert error_json['code'] == 10400
             assert error_json['message'] == 'Template not found'
             assert error_json['fields'] == [{'template': 'Template not found'}]
 
@@ -76,7 +75,7 @@ def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api
             assert response.status_code == 401
             assert response.headers['Content-type'] == 'application/json'
             error_resp = json.loads(response.get_data(as_text=True))
-            assert error_resp['code'] == 401
+            assert error_resp['status_code'] == 401
             assert error_resp['message'] == 'Unauthorized, authentication token must be provided'
             assert error_resp['fields'] == {'token': ['Unauthorized, authentication token must be provided']}
 
@@ -98,6 +97,6 @@ def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, s
             assert response.status_code == 400
             assert response.headers['Content-type'] == 'application/json'
             error_resp = json.loads(response.get_data(as_text=True))
-            assert error_resp['code'] == '1001'
+            assert error_resp['status_code'] == 400
             assert error_resp['message'] == 'Validation error occurred - POST v2/notifications/sms'
             assert error_resp['fields'] == [{"template_id": "is a required property"}]

From 37c95b08b69e54fcd0cec91083a82814f23c652b Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Thu, 3 Nov 2016 11:11:51 +0000
Subject: [PATCH 03/29] Refactor saving user profile

---
 app/dao/users_dao.py |  9 ++++++---
 app/user/rest.py     | 10 +++-------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/app/dao/users_dao.py b/app/dao/users_dao.py
index 1a240fecc..264481bfe 100644
--- a/app/dao/users_dao.py
+++ b/app/dao/users_dao.py
@@ -7,6 +7,11 @@ from app import db
 from app.models import (User, VerifyCode)
 
 
+def _remove_values_for_keys_if_present(dict, keys):
+    for key in keys:
+        dict.pop(key, None)
+
+
 def create_secret_code():
     return ''.join(map(str, random.sample(range(9), 5)))
 
@@ -16,9 +21,7 @@ def save_model_user(usr, update_dict={}, pwd=None):
         usr.password = pwd
         usr.password_changed_at = datetime.utcnow()
     if update_dict:
-        if update_dict.get('id'):
-            del update_dict['id']
-            update_dict.pop('password_changed_at')
+        _remove_values_for_keys_if_present(update_dict, ['id', 'password', 'password_changed_at'])
         db.session.query(User).filter_by(id=usr.id).update(update_dict)
     else:
         db.session.add(usr)
diff --git a/app/user/rest.py b/app/user/rest.py
index 2e6e6b021..7aee5ac3f 100644
--- a/app/user/rest.py
+++ b/app/user/rest.py
@@ -55,14 +55,10 @@ def create_user():
 def update_user(user_id):
     user_to_update = get_model_users(user_id=user_id)
     req_json = request.get_json()
-    update_dct, errors = user_schema_load_json.load(req_json)
     pwd = req_json.get('password', None)
-    # TODO password validation, it is already done on the admin app
-    # but would be good to have the same validation here.
-    if pwd is not None and not pwd:
-        errors.update({'password': ['Invalid data for field']})
-        raise InvalidRequest(errors, status_code=400)
-    save_model_user(user_to_update, update_dict=update_dct, pwd=pwd)
+    if not pwd:
+        raise InvalidRequest('Invalid entry for password', status_code=400)
+    save_model_user(user_to_update, update_dict=req_json, pwd=pwd)
     return jsonify(data=user_schema.dump(user_to_update).data), 200
 
 

From a41e7d5a2f68cdd70438635bd44d34cb648f1caa Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Thu, 3 Nov 2016 11:46:58 +0000
Subject: [PATCH 04/29] Add the right password check

---
 app/user/rest.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/user/rest.py b/app/user/rest.py
index 7aee5ac3f..4fd21e202 100644
--- a/app/user/rest.py
+++ b/app/user/rest.py
@@ -56,7 +56,7 @@ def update_user(user_id):
     user_to_update = get_model_users(user_id=user_id)
     req_json = request.get_json()
     pwd = req_json.get('password', None)
-    if not pwd:
+    if pwd is not None and not pwd:
         raise InvalidRequest('Invalid entry for password', status_code=400)
     save_model_user(user_to_update, update_dict=req_json, pwd=pwd)
     return jsonify(data=user_schema.dump(user_to_update).data), 200

From 8a126c738770ce09c961e76db64ac903d223888e Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Mon, 7 Nov 2016 17:41:49 +0000
Subject: [PATCH 05/29] Add a schema to validate a single user attr 'strictly'

---
 app/schemas.py            | 36 ++++++++++++++++++++++++++++++++++++
 tests/app/test_schemas.py | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)

diff --git a/app/schemas.py b/app/schemas.py
index 6f5934114..b9f75cb2c 100644
--- a/app/schemas.py
+++ b/app/schemas.py
@@ -105,6 +105,41 @@ class UserSchema(BaseSchema):
         strict = True
 
 
+class UserUpdateAttributeSchema(BaseSchema):
+
+    class Meta:
+        model = models.User
+        exclude = (
+            "updated_at", "created_at", "user_to_service",
+            "_password", "verify_codes")
+        strict = True
+
+    @validates('name')
+    def validate_name(self, value):
+        if not value:
+            raise ValidationError('Invalid name')
+
+    @validates('email_address')
+    def validate_email_address(self, value):
+        try:
+            validate_email_address(value)
+        except InvalidEmailError as e:
+            raise ValidationError(e.message)
+
+    @validates('mobile_number')
+    def validate_mobile_number(self, value):
+        try:
+            validate_phone_number(value)
+        except InvalidPhoneError as error:
+            raise ValidationError('Invalid phone number: {}'.format(error))
+
+    @validates_schema(pass_original=True)
+    def check_unknown_fields(self, data, original_data):
+        for key in original_data:
+            if key not in self.fields:
+                raise ValidationError('Unknown field name {}'.format(key))
+
+
 class ProviderDetailsSchema(BaseSchema):
     class Meta:
         model = models.ProviderDetails
@@ -529,6 +564,7 @@ class UnarchivedTemplateSchema(BaseSchema):
 
 user_schema = UserSchema()
 user_schema_load_json = UserSchema(load_json=True)
+user_update_schema_load_json = UserUpdateAttributeSchema(load_json=True, partial=True)
 service_schema = ServiceSchema()
 service_schema_load_json = ServiceSchema(load_json=True)
 detailed_service_schema = DetailedServiceSchema()
diff --git a/tests/app/test_schemas.py b/tests/app/test_schemas.py
index 4ba1c98b5..5f2bc4371 100644
--- a/tests/app/test_schemas.py
+++ b/tests/app/test_schemas.py
@@ -1,3 +1,6 @@
+import pytest
+
+
 def test_job_schema_doesnt_return_notifications(sample_notification_with_job):
     from app.schemas import job_schema
 
@@ -22,3 +25,34 @@ def test_notification_schema_adds_api_key_name(sample_notification_with_api_key)
 
     data = notification_with_template_schema.dump(sample_notification_with_api_key).data
     assert data['key_name'] == 'Test key'
+
+
+@pytest.mark.parametrize('user_attribute, user_value', [
+    ('name', 'New User'),
+    ('email_address', 'newuser@mail.com'),
+    ('mobile_number', '+4407700900460')
+])
+def test_user_schema_accepts_valid_attributes(user_attribute, user_value):
+    update_dict = {
+        user_attribute: user_value
+    }
+    from app.schemas import user_update_schema_load_json
+
+    data, errors = user_update_schema_load_json.load(update_dict)
+    assert not errors
+
+
+@pytest.mark.parametrize('user_attribute, user_value', [
+    ('name', None),
+    ('name', ''),
+    ('email_address', 'bademail@...com'),
+    ('mobile_number', '+44077009')
+])
+def test_user_schema_rejects_invalid_attributes(user_attribute, user_value):
+    from app.schemas import user_update_schema_load_json
+    update_dict = {
+        user_attribute: user_value
+    }
+
+    with pytest.raises(Exception):
+        data, errors = user_update_schema_load_json.load(update_dict)

From 461d8a9b2c4850c6caa74936dfcf945f3df34a91 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Mon, 7 Nov 2016 17:42:23 +0000
Subject: [PATCH 06/29] Add separate endpoint to update a single user attr

---
 app/service/rest.py         |  8 ++++----
 app/user/rest.py            | 36 +++++++++++++++++++++++++-----------
 tests/app/user/test_rest.py | 24 ++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/app/service/rest.py b/app/service/rest.py
index 2cc7dab90..fb1ea29e1 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -34,7 +34,7 @@ from app.dao.service_whitelist_dao import (
 )
 from app.dao import notifications_dao
 from app.dao.provider_statistics_dao import get_fragment_count
-from app.dao.users_dao import get_model_users
+from app.dao.users_dao import get_user_by_id
 from app.errors import (
     register_errors,
     InvalidRequest
@@ -88,7 +88,7 @@ def create_service():
         errors = {'user_id': ['Missing data for required field.']}
         raise InvalidRequest(errors, status_code=400)
 
-    user = get_model_users(data['user_id'])
+    user = get_user_by_id(data['user_id'])
     data.pop('user_id', None)
     valid_service = service_schema.load(request.get_json()).data
     dao_create_service(valid_service, user)
@@ -148,7 +148,7 @@ def get_users_for_service(service_id):
 @service_blueprint.route('/<uuid:service_id>/users/<user_id>', methods=['POST'])
 def add_user_to_service(service_id, user_id):
     service = dao_fetch_service_by_id(service_id)
-    user = get_model_users(user_id=user_id)
+    user = get_user_by_id(user_id=user_id)
 
     if user in service.users:
         error = 'User id: {} already part of service id: {}'.format(user_id, service_id)
@@ -163,7 +163,7 @@ def add_user_to_service(service_id, user_id):
 @service_blueprint.route('/<uuid:service_id>/users/<user_id>', methods=['DELETE'])
 def remove_user_from_service(service_id, user_id):
     service = dao_fetch_service_by_id(service_id)
-    user = get_model_users(user_id=user_id)
+    user = get_user_by_id(user_id=user_id)
     if user not in service.users:
         error = 'User not found'
         raise InvalidRequest(error, status_code=404)
diff --git a/app/user/rest.py b/app/user/rest.py
index 2e6e6b021..e41e10877 100644
--- a/app/user/rest.py
+++ b/app/user/rest.py
@@ -4,7 +4,7 @@ from datetime import datetime
 from flask import (jsonify, request, Blueprint, current_app)
 from app import encryption, DATETIME_FORMAT
 from app.dao.users_dao import (
-    get_model_users,
+    get_user_by_id,
     save_model_user,
     create_user_code,
     get_user_code,
@@ -12,7 +12,8 @@ from app.dao.users_dao import (
     increment_failed_login_count,
     reset_failed_login_count,
     get_user_by_email,
-    create_secret_code
+    create_secret_code,
+    save_user_attribute
 )
 from app.dao.permissions_dao import permission_dao
 from app.dao.services_dao import dao_fetch_service_by_id
@@ -22,8 +23,10 @@ from app.schemas import (
     email_data_request_schema,
     user_schema,
     request_verify_code_schema,
+    permission_schema,
     user_schema_load_json,
-    permission_schema)
+    user_update_schema_load_json
+)
 
 from app.celery.tasks import (
     send_sms,
@@ -53,7 +56,7 @@ def create_user():
 
 @user.route('/<uuid:user_id>', methods=['PUT'])
 def update_user(user_id):
-    user_to_update = get_model_users(user_id=user_id)
+    user_to_update = get_user_by_id(user_id=user_id)
     req_json = request.get_json()
     update_dct, errors = user_schema_load_json.load(req_json)
     pwd = req_json.get('password', None)
@@ -66,9 +69,20 @@ def update_user(user_id):
     return jsonify(data=user_schema.dump(user_to_update).data), 200
 
 
+@user.route('/<uuid:user_id>/update-attribute', methods=['PUT'])
+def update_user_attribute(user_id):
+    user_to_update = get_user_by_id(user_id=user_id)
+    req_json = request.get_json()
+    update_dct, errors = user_update_schema_load_json.load(req_json)
+    if errors:
+        raise InvalidRequest(errors, status_code=400)
+    save_user_attribute(user_to_update, update_dict=update_dct)
+    return jsonify(data=user_schema.dump(user_to_update).data), 200
+
+
 @user.route('/<uuid:user_id>/verify/password', methods=['POST'])
 def verify_user_password(user_id):
-    user_to_verify = get_model_users(user_id=user_id)
+    user_to_verify = get_user_by_id(user_id=user_id)
 
     txt_pwd = None
     try:
@@ -92,7 +106,7 @@ def verify_user_password(user_id):
 
 @user.route('/<uuid:user_id>/verify/code', methods=['POST'])
 def verify_user_code(user_id):
-    user_to_verify = get_model_users(user_id=user_id)
+    user_to_verify = get_user_by_id(user_id=user_id)
 
     txt_code = None
     resp_json = request.get_json()
@@ -120,7 +134,7 @@ def verify_user_code(user_id):
 
 @user.route('/<uuid:user_id>/sms-code', methods=['POST'])
 def send_user_sms_code(user_id):
-    user_to_send_to = get_model_users(user_id=user_id)
+    user_to_send_to = get_user_by_id(user_id=user_id)
     verify_code, errors = request_verify_code_schema.load(request.get_json())
 
     secret_code = create_secret_code()
@@ -149,7 +163,7 @@ def send_user_sms_code(user_id):
 
 @user.route('/<uuid:user_id>/change-email-verification', methods=['POST'])
 def send_user_confirm_new_email(user_id):
-    user_to_send_to = get_model_users(user_id=user_id)
+    user_to_send_to = get_user_by_id(user_id=user_id)
     email, errors = email_data_request_schema.load(request.get_json())
     if errors:
         raise InvalidRequest(message=errors, status_code=400)
@@ -178,7 +192,7 @@ def send_user_confirm_new_email(user_id):
 
 @user.route('/<uuid:user_id>/email-verification', methods=['POST'])
 def send_user_email_verification(user_id):
-    user_to_send_to = get_model_users(user_id=user_id)
+    user_to_send_to = get_user_by_id(user_id=user_id)
     secret_code = create_secret_code()
     create_user_code(user_to_send_to, secret_code, 'email')
 
@@ -230,7 +244,7 @@ def send_already_registered_email(user_id):
 @user.route('/<uuid:user_id>', methods=['GET'])
 @user.route('', methods=['GET'])
 def get_user(user_id=None):
-    users = get_model_users(user_id=user_id)
+    users = get_user_by_id(user_id=user_id)
     result = user_schema.dump(users, many=True) if isinstance(users, list) else user_schema.dump(users)
     return jsonify(data=result.data)
 
@@ -239,7 +253,7 @@ def get_user(user_id=None):
 def set_permissions(user_id, service_id):
     # TODO fix security hole, how do we verify that the user
     # who is making this request has permission to make the request.
-    user = get_model_users(user_id=user_id)
+    user = get_user_by_id(user_id=user_id)
     service = dao_fetch_service_by_id(service_id=service_id)
     permissions, errors = permission_schema.load(request.get_json(), many=True)
 
diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py
index 14b26d387..d9261fbb8 100644
--- a/tests/app/user/test_rest.py
+++ b/tests/app/user/test_rest.py
@@ -1,4 +1,5 @@
 import json
+import pytest
 
 from flask import url_for, current_app
 from freezegun import freeze_time
@@ -180,6 +181,29 @@ def test_put_user(notify_api, notify_db, notify_db_session, sample_service):
             assert sorted(expected_permissions) == sorted(fetched['permissions'][str(sample_service.id)])
 
 
+@pytest.mark.parametrize('user_attribute, user_value', [
+    ('name', 'New User'),
+    ('email_address', 'newuser@mail.com'),
+    ('mobile_number', '+4407700900460')
+])
+def test_put_user_attribute(client, sample_user, user_attribute, user_value):
+    assert getattr(sample_user, user_attribute) != user_value
+    update_dict = {
+        user_attribute: user_value
+    }
+    auth_header = create_authorization_header()
+    headers = [('Content-Type', 'application/json'), auth_header]
+
+    resp = client.put(
+        url_for('user.update_user_attribute', user_id=sample_user.id),
+        data=json.dumps(update_dict),
+        headers=headers)
+
+    assert resp.status_code == 200
+    json_resp = json.loads(resp.get_data(as_text=True))
+    assert json_resp['data'][user_attribute] == user_value
+
+
 def test_put_user_update_password(notify_api,
                                   notify_db,
                                   notify_db_session,

From 3f10e59db3fd072ff28a774f22e13ff0196fef61 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Mon, 7 Nov 2016 17:42:39 +0000
Subject: [PATCH 07/29] Add user dao method to update a single user attr

---
 app/dao/users_dao.py            | 16 ++++++++++++----
 tests/app/dao/test_users_dao.py | 29 ++++++++++++++++++++++-------
 2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/app/dao/users_dao.py b/app/dao/users_dao.py
index 1a240fecc..6654234a5 100644
--- a/app/dao/users_dao.py
+++ b/app/dao/users_dao.py
@@ -7,18 +7,26 @@ from app import db
 from app.models import (User, VerifyCode)
 
 
+def _remove_values_for_keys_if_present(dict, keys):
+    for key in keys:
+        dict.pop(key, None)
+
+
 def create_secret_code():
     return ''.join(map(str, random.sample(range(9), 5)))
 
 
+def save_user_attribute(usr, update_dict={}):
+    db.session.query(User).filter_by(id=usr.id).update(update_dict)
+    db.session.commit()
+
+
 def save_model_user(usr, update_dict={}, pwd=None):
     if pwd:
         usr.password = pwd
         usr.password_changed_at = datetime.utcnow()
     if update_dict:
-        if update_dict.get('id'):
-            del update_dict['id']
-            update_dict.pop('password_changed_at')
+        _remove_values_for_keys_if_present(update_dict, ['id', 'password_changed_at'])
         db.session.query(User).filter_by(id=usr.id).update(update_dict)
     else:
         db.session.add(usr)
@@ -74,7 +82,7 @@ def delete_user_verify_codes(user):
     db.session.commit()
 
 
-def get_model_users(user_id=None):
+def get_user_by_id(user_id=None):
     if user_id:
         return User.query.filter_by(id=user_id).one()
     return User.query.filter_by().all()
diff --git a/tests/app/dao/test_users_dao.py b/tests/app/dao/test_users_dao.py
index 7afcf72ce..7d1901f0d 100644
--- a/tests/app/dao/test_users_dao.py
+++ b/tests/app/dao/test_users_dao.py
@@ -7,12 +7,13 @@ import pytest
 
 from app.dao.users_dao import (
     save_model_user,
-    get_model_users,
+    save_user_attribute,
+    get_user_by_id,
     delete_model_user,
     increment_failed_login_count,
     reset_failed_login_count,
     get_user_by_email,
-    delete_codes_older_created_more_than_a_day_ago
+    delete_codes_older_created_more_than_a_day_ago,
 )
 
 from tests.app.conftest import sample_user as create_sample_user
@@ -37,13 +38,13 @@ def test_create_user(notify_api, notify_db, notify_db_session):
 
 def test_get_all_users(notify_api, notify_db, notify_db_session, sample_user):
     assert User.query.count() == 1
-    assert len(get_model_users()) == 1
+    assert len(get_user_by_id()) == 1
     email = "another.notify@digital.cabinet-office.gov.uk"
     another_user = create_sample_user(notify_db,
                                       notify_db_session,
                                       email=email)
     assert User.query.count() == 2
-    assert len(get_model_users()) == 2
+    assert len(get_user_by_id()) == 2
 
 
 def test_get_user(notify_api, notify_db, notify_db_session):
@@ -51,12 +52,12 @@ def test_get_user(notify_api, notify_db, notify_db_session):
     another_user = create_sample_user(notify_db,
                                       notify_db_session,
                                       email=email)
-    assert get_model_users(user_id=another_user.id).email_address == email
+    assert get_user_by_id(user_id=another_user.id).email_address == email
 
 
 def test_get_user_not_exists(notify_api, notify_db, notify_db_session, fake_uuid):
     try:
-        get_model_users(user_id=fake_uuid)
+        get_user_by_id(user_id=fake_uuid)
         pytest.fail("NoResultFound exception not thrown.")
     except NoResultFound as e:
         pass
@@ -64,7 +65,7 @@ def test_get_user_not_exists(notify_api, notify_db, notify_db_session, fake_uuid
 
 def test_get_user_invalid_id(notify_api, notify_db, notify_db_session):
     try:
-        get_model_users(user_id="blah")
+        get_user_by_id(user_id="blah")
         pytest.fail("DataError exception not thrown.")
     except DataError:
         pass
@@ -131,3 +132,17 @@ def make_verify_code(user, age=timedelta(hours=0), code="12335"):
     )
     db.session.add(verify_code)
     db.session.commit()
+
+
+@pytest.mark.parametrize('user_attribute, user_value', [
+    ('name', 'New User'),
+    ('email_address', 'newuser@mail.com'),
+    ('mobile_number', '+4407700900460')
+])
+def test_update_user_attribute(client, sample_user, user_attribute, user_value):
+    assert getattr(sample_user, user_attribute) != user_value
+    update_dict = {
+        user_attribute: user_value
+    }
+    save_user_attribute(sample_user, update_dict)
+    assert getattr(sample_user, user_attribute) == user_value

From da2fa5b4bcf9787a9457711eea3c1b709fc2cf48 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Mon, 31 Oct 2016 18:14:29 +0000
Subject: [PATCH 08/29] move sqlalchemy defaults from booleans to SQL
 constructs

booleans aren't actually allowed, and quietly do nothing

also default Services.active to true
---
 app/models.py                                 | 18 +++++++--------
 .../versions/0059_set_services_to_active.py   | 23 +++++++++++++++++++
 tests/app/conftest.py                         |  2 +-
 tests/app/dao/test_services_dao.py            | 14 -----------
 tests/app/service/test_rest.py                |  1 -
 5 files changed, 33 insertions(+), 25 deletions(-)
 create mode 100644 migrations/versions/0059_set_services_to_active.py

diff --git a/app/models.py b/app/models.py
index 526288990..f5dc10ff9 100644
--- a/app/models.py
+++ b/app/models.py
@@ -5,7 +5,7 @@ from sqlalchemy.dialects.postgresql import (
     UUID,
     JSON
 )
-from sqlalchemy import UniqueConstraint, and_
+from sqlalchemy import UniqueConstraint, and_, false, true
 from sqlalchemy.orm import foreign, remote
 from notifications_utils.recipients import (
     validate_email_address,
@@ -58,7 +58,7 @@ class User(db.Model):
     logged_in_at = db.Column(db.DateTime, nullable=True)
     failed_login_count = db.Column(db.Integer, nullable=False, default=0)
     state = db.Column(db.String, nullable=False, default='pending')
-    platform_admin = db.Column(db.Boolean, nullable=False, default=False)
+    platform_admin = db.Column(db.Boolean, nullable=False, default=false())
 
     @property
     def password(self):
@@ -115,15 +115,15 @@ class Service(db.Model, Versioned):
         unique=False,
         nullable=True,
         onupdate=datetime.datetime.utcnow)
-    active = db.Column(db.Boolean, index=False, unique=False, nullable=False)
+    active = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=true())
     message_limit = db.Column(db.BigInteger, index=False, unique=False, nullable=False)
     users = db.relationship(
         'User',
         secondary=user_to_service,
         backref=db.backref('user_to_service', lazy='dynamic'))
     restricted = db.Column(db.Boolean, index=False, unique=False, nullable=False)
-    research_mode = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=False)
-    can_send_letters = db.Column(db.Boolean, nullable=False, default=False)
+    research_mode = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=false())
+    can_send_letters = db.Column(db.Boolean, nullable=False, default=false())
     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)
@@ -273,7 +273,7 @@ class Template(db.Model):
         nullable=True,
         onupdate=datetime.datetime.utcnow)
     content = db.Column(db.Text, index=False, unique=False, nullable=False)
-    archived = db.Column(db.Boolean, index=False, nullable=False, default=False)
+    archived = db.Column(db.Boolean, index=False, nullable=False, default=false())
     service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False, nullable=False)
     service = db.relationship('Service', backref=db.backref('templates', lazy='dynamic'))
     subject = db.Column(db.Text, index=False, unique=False, nullable=True)
@@ -291,7 +291,7 @@ class TemplateHistory(db.Model):
     created_at = db.Column(db.DateTime, nullable=False)
     updated_at = db.Column(db.DateTime)
     content = db.Column(db.Text, nullable=False)
-    archived = db.Column(db.Boolean, nullable=False, default=False)
+    archived = db.Column(db.Boolean, nullable=False, default=false())
     service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False)
     service = db.relationship('Service')
     subject = db.Column(db.Text)
@@ -344,7 +344,7 @@ class ProviderDetails(db.Model):
     identifier = db.Column(db.String, nullable=False)
     priority = db.Column(db.Integer, nullable=False)
     notification_type = db.Column(notification_types, nullable=False)
-    active = db.Column(db.Boolean, default=False)
+    active = db.Column(db.Boolean, default=false())
 
 
 JOB_STATUS_PENDING = 'pending'
@@ -431,7 +431,7 @@ class VerifyCode(db.Model):
     code_type = db.Column(db.Enum(*VERIFY_CODE_TYPES, name='verify_code_types'),
                           index=False, unique=False, nullable=False)
     expiry_datetime = db.Column(db.DateTime, nullable=False)
-    code_used = db.Column(db.Boolean, default=False)
+    code_used = db.Column(db.Boolean, default=false())
     created_at = db.Column(
         db.DateTime,
         index=False,
diff --git a/migrations/versions/0059_set_services_to_active.py b/migrations/versions/0059_set_services_to_active.py
new file mode 100644
index 000000000..f402d1a70
--- /dev/null
+++ b/migrations/versions/0059_set_services_to_active.py
@@ -0,0 +1,23 @@
+"""
+we weren't previously using the services.active column , and by default it was set to false. lets set all services to
+active, so that in the future we can turn it off to signify deactivating a service
+
+Revision ID: 0059_set_services_to_active
+Revises: 0058_add_letters_flag
+Create Date: 2016-10-31 15:17:16.716450
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '0059_set_services_to_active'
+down_revision = '0058_add_letters_flag'
+
+from alembic import op
+
+
+def upgrade():
+    op.execute('UPDATE services SET active = TRUE')
+
+
+def downgrade():
+    op.execute('UPDATE services SET active = FALSE')
diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index 002c7c13c..3d1af5359 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -139,7 +139,7 @@ def sample_service(notify_db,
     data = {
         'name': service_name,
         'message_limit': limit,
-        'active': False,
+        'active': True,
         'restricted': restricted,
         'email_from': email_from,
         'created_by': user
diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py
index a24f26c0b..ae7eeda06 100644
--- a/tests/app/dao/test_services_dao.py
+++ b/tests/app/dao/test_services_dao.py
@@ -58,7 +58,6 @@ def test_create_service(sample_user):
     service = Service(name="service_name",
                       email_from="email_from",
                       message_limit=1000,
-                      active=True,
                       restricted=False,
                       created_by=sample_user)
     dao_create_service(service, sample_user)
@@ -77,14 +76,12 @@ def test_cannot_create_two_services_with_same_name(sample_user):
     service1 = Service(name="service_name",
                        email_from="email_from1",
                        message_limit=1000,
-                       active=True,
                        restricted=False,
                        created_by=sample_user)
 
     service2 = Service(name="service_name",
                        email_from="email_from2",
                        message_limit=1000,
-                       active=True,
                        restricted=False,
                        created_by=sample_user)
     with pytest.raises(IntegrityError) as excinfo:
@@ -98,13 +95,11 @@ def test_cannot_create_two_services_with_same_email_from(sample_user):
     service1 = Service(name="service_name1",
                        email_from="email_from",
                        message_limit=1000,
-                       active=True,
                        restricted=False,
                        created_by=sample_user)
     service2 = Service(name="service_name2",
                        email_from="email_from",
                        message_limit=1000,
-                       active=True,
                        restricted=False,
                        created_by=sample_user)
     with pytest.raises(IntegrityError) as excinfo:
@@ -118,7 +113,6 @@ def test_cannot_create_service_with_no_user(notify_db_session, sample_user):
     service = Service(name="service_name",
                       email_from="email_from",
                       message_limit=1000,
-                      active=True,
                       restricted=False,
                       created_by=sample_user)
     with pytest.raises(FlushError) as excinfo:
@@ -130,7 +124,6 @@ def test_should_add_user_to_service(sample_user):
     service = Service(name="service_name",
                       email_from="email_from",
                       message_limit=1000,
-                      active=True,
                       restricted=False,
                       created_by=sample_user)
     dao_create_service(service, sample_user)
@@ -150,7 +143,6 @@ def test_should_remove_user_from_service(sample_user):
     service = Service(name="service_name",
                       email_from="email_from",
                       message_limit=1000,
-                      active=True,
                       restricted=False,
                       created_by=sample_user)
     dao_create_service(service, sample_user)
@@ -244,7 +236,6 @@ def test_create_service_creates_a_history_record_with_current_data(sample_user):
     service = Service(name="service_name",
                       email_from="email_from",
                       message_limit=1000,
-                      active=True,
                       restricted=False,
                       created_by=sample_user)
     dao_create_service(service, sample_user)
@@ -270,7 +261,6 @@ def test_update_service_creates_a_history_record_with_current_data(sample_user):
     service = Service(name="service_name",
                       email_from="email_from",
                       message_limit=1000,
-                      active=True,
                       restricted=False,
                       created_by=sample_user)
     dao_create_service(service, sample_user)
@@ -299,7 +289,6 @@ def test_create_service_and_history_is_transactional(sample_user):
     service = Service(name=None,
                       email_from="email_from",
                       message_limit=1000,
-                      active=True,
                       restricted=False,
                       created_by=sample_user)
 
@@ -348,7 +337,6 @@ def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sam
     service_one = Service(name="service_one",
                           email_from="service_one",
                           message_limit=1000,
-                          active=True,
                           restricted=False,
                           created_by=sample_user)
 
@@ -367,7 +355,6 @@ def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sam
     service_two = Service(name="service_two",
                           email_from="service_two",
                           message_limit=1000,
-                          active=True,
                           restricted=False,
                           created_by=other_user)
     dao_create_service(service_two, other_user)
@@ -397,7 +384,6 @@ def test_fetch_stats_filters_on_service(sample_notification):
     service_two = Service(name="service_two",
                           created_by=sample_notification.service.created_by,
                           email_from="hello",
-                          active=False,
                           restricted=False,
                           message_limit=1000)
     dao_create_service(service_two, sample_notification.service.created_by)
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 3959e401d..15c807266 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -285,7 +285,6 @@ def test_should_not_create_service_if_missing_data(notify_api, sample_user):
             assert resp.status_code == 400
             assert json_resp['result'] == 'error'
             assert 'Missing data for required field.' in json_resp['message']['name']
-            assert 'Missing data for required field.' in json_resp['message']['active']
             assert 'Missing data for required field.' in json_resp['message']['message_limit']
             assert 'Missing data for required field.' in json_resp['message']['restricted']
 

From 089ac099f3beae9a467bcb71de7cc04d77f175d0 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Tue, 1 Nov 2016 16:09:55 +0000
Subject: [PATCH 09/29] POST /service/{id}/deactivate deactivates a service:

* set active=False on the service
* renames service to "_archived_{old_name}"
* archives all templates for the service
* revokes all api keys for the service
---
 app/service/rest.py                  | 26 ++++++++++
 tests/app/service/test_deactivate.py | 75 ++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)
 create mode 100644 tests/app/service/test_deactivate.py

diff --git a/app/service/rest.py b/app/service/rest.py
index 2cc7dab90..4e6d94ba0 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -32,6 +32,9 @@ from app.dao.service_whitelist_dao import (
     dao_add_and_commit_whitelisted_contacts,
     dao_remove_service_whitelist
 )
+from app.dao.templates_dao import (
+    dao_update_template
+)
 from app.dao import notifications_dao
 from app.dao.provider_statistics_dao import get_fragment_count
 from app.dao.users_dao import get_model_users
@@ -312,6 +315,29 @@ def update_whitelist(service_id):
         return '', 204
 
 
+@service_blueprint.route('/<uuid:service_id>/deactivate', methods=['POST'])
+def deactivate_service(service_id):
+    service = dao_fetch_service_by_id(service_id)
+
+    if not service.active:
+        # assume already inactive, don't change service name
+        return '', 204
+
+    service.active = False
+    service.name = '_archived_' + service.name
+    service.email_from = '_archived_' + service.email_from
+    dao_update_service(service)
+
+    for template in service.templates:
+        template.archived = True
+        dao_update_template(template)
+
+    for api_key in service.api_keys:
+        expire_api_key(service.id, api_key.id)
+
+    return '', 204
+
+
 @service_blueprint.route('/<uuid:service_id>/billable-units')
 def get_billable_unit_count(service_id):
     try:
diff --git a/tests/app/service/test_deactivate.py b/tests/app/service/test_deactivate.py
new file mode 100644
index 000000000..50e552816
--- /dev/null
+++ b/tests/app/service/test_deactivate.py
@@ -0,0 +1,75 @@
+import uuid
+
+import pytest
+
+from app.models import Service
+from tests import create_authorization_header
+from tests.app.conftest import (
+    sample_template as create_template,
+    sample_api_key as create_api_key
+)
+
+
+def test_deactivate_only_allows_post(client, sample_service):
+    auth_header = create_authorization_header(service_id=str(sample_service.id))
+    response = client.get('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header])
+    assert response.status_code == 405
+
+
+def test_deactivate_service_errors_with_bad_service_id(client, sample_service):
+    auth_header = create_authorization_header(service_id=str(sample_service.id))
+    response = client.post('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header])
+    assert response.status_code == 404
+
+
+def test_deactivating_inactive_service_does_nothing(client, sample_service):
+    auth_header = create_authorization_header(service_id=str(sample_service.id))
+    sample_service.active = False
+    response = client.post('/service/{}/deactivate'.format(sample_service.id), headers=[auth_header])
+    assert response.status_code == 204
+    assert sample_service.name == 'Sample service'
+
+
+@pytest.fixture
+def deactivated_service(client, notify_db, notify_db_session, sample_service):
+    create_template(notify_db, notify_db_session, template_name='a')
+    create_template(notify_db, notify_db_session, template_name='b')
+    create_api_key(notify_db, notify_db_session)
+    create_api_key(notify_db, notify_db_session)
+
+    auth_header = create_authorization_header(service_id=str(sample_service.id))
+    response = client.post('/service/{}/deactivate'.format(sample_service.id), headers=[auth_header])
+    assert response.status_code == 204
+    assert response.data == b''
+    return sample_service
+
+
+def test_deactivating_service_changes_name_and_email(deactivated_service):
+    assert deactivated_service.name == '_archived_Sample service'
+    assert deactivated_service.email_from == '_archived_sample.service'
+
+
+def test_deactivating_service_revokes_api_keys(deactivated_service):
+    assert deactivated_service.api_keys.count() == 2
+    for key in deactivated_service.api_keys:
+        assert key.expiry_date is not None
+        assert key.version == 2
+
+
+def test_deactivating_service_archives_templates(deactivated_service):
+    assert deactivated_service.templates.count() == 2
+    for template in deactivated_service.templates:
+        assert template.archived is True
+        assert template.version == 2
+
+
+def test_deactivating_service_creates_history(deactivated_service):
+    ServiceHistory = Service.get_history_model()
+    history = ServiceHistory.query.filter_by(
+        id=deactivated_service.id
+    ).order_by(
+        ServiceHistory.version.desc()
+    ).first()
+
+    assert history.version == 2
+    assert history.active is False

From f6de17d6b73284dcf111673d919b14bdab4be67a Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Tue, 8 Nov 2016 13:48:14 +0000
Subject: [PATCH 10/29] dont let people accidentally delete their local
 database by forcing it to run against a different DB (ie
 test_notifications_api)

---
 tests/conftest.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/conftest.py b/tests/conftest.py
index 11f570de5..b8094852c 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -33,6 +33,8 @@ def client(notify_api):
 
 @pytest.fixture(scope='session')
 def notify_db(notify_api, request):
+    assert db.engine.url.database != 'notification_api', 'dont run tests against main db'
+
     Migrate(notify_api, db)
     Manager(db, MigrateCommand)
     BASE_DIR = os.path.dirname(os.path.dirname(__file__))

From efd2be9acb6c30a9599b7fe8b46e28450cc85643 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Tue, 8 Nov 2016 13:49:47 +0000
Subject: [PATCH 11/29] set service to active on creation

---
 app/dao/services_dao.py            | 1 +
 tests/app/dao/test_services_dao.py | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py
index c89886f1a..ceb1d1635 100644
--- a/app/dao/services_dao.py
+++ b/app/dao/services_dao.py
@@ -70,6 +70,7 @@ def dao_create_service(service, user):
     service.users.append(user)
     permission_dao.add_default_service_permissions_for_user(user, service)
     service.id = uuid.uuid4()  # must be set now so version history model can use same id
+    service.active = True
     service.research_mode = False
     db.session.add(service)
 
diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py
index ae7eeda06..6140231e4 100644
--- a/tests/app/dao/test_services_dao.py
+++ b/tests/app/dao/test_services_dao.py
@@ -67,7 +67,8 @@ def test_create_service(sample_user):
     assert service_db.name == "service_name"
     assert service_db.id == service.id
     assert service_db.branding == BRANDING_GOVUK
-    assert not service_db.research_mode
+    assert service_db.research_mode is False
+    assert service.active is True
     assert sample_user in service_db.users
 
 

From 3cbacecf1966d9c2da427d7db80729694ed3b402 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Wed, 9 Nov 2016 11:32:07 +0000
Subject: [PATCH 12/29] fix non-functional/overengineered existing tests

---
 tests/app/service/test_rest.py | 93 +++++++++++++++-------------------
 1 file changed, 42 insertions(+), 51 deletions(-)

diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 15c807266..c1983321e 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -11,8 +11,8 @@ from app.dao.services_dao import dao_remove_user_from_service
 from app.models import User, Organisation
 from tests import create_authorization_header
 from tests.app.conftest import (
-    sample_service as create_sample_service,
-    sample_service_permission as create_sample_service_permission,
+    sample_service as create_service,
+    sample_service_permission as create_service_permission,
     sample_user as create_sample_user,
     sample_notification as create_sample_notification,
     sample_notification_with_job)
@@ -38,50 +38,41 @@ def test_get_service_list(notify_api, service_factory):
             assert json_resp['data'][2]['name'] == 'three'
 
 
-def test_get_service_list_by_user(notify_api, sample_user, service_factory):
+def test_get_service_list_by_user(notify_db, notify_db_session, client, sample_user, service_factory):
+    other_user = create_sample_user(notify_db, notify_db_session, email='foo@bar.gov.uk')
+    service_factory.get('one', sample_user, email_from='one')
+    service_factory.get('two', sample_user, email_from='two')
+    service_factory.get('three', other_user, email_from='three')
 
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-            service_factory.get('one', sample_user, email_from='one')
-            service_factory.get('two', sample_user, email_from='two')
-            service_factory.get('three', sample_user, email_from='three')
-
-            auth_header = create_authorization_header()
-            response = client.get(
-                '/service?user_id='.format(sample_user.id),
-                headers=[auth_header]
-            )
-            json_resp = json.loads(response.get_data(as_text=True))
-            assert response.status_code == 200
-            assert len(json_resp['data']) == 3
-            assert json_resp['data'][0]['name'] == 'one'
-            assert json_resp['data'][1]['name'] == 'two'
-            assert json_resp['data'][2]['name'] == 'three'
+    auth_header = create_authorization_header()
+    response = client.get(
+        '/service?user_id={}'.format(sample_user.id),
+        headers=[auth_header]
+    )
+    json_resp = json.loads(response.get_data(as_text=True))
+    assert response.status_code == 200
+    assert len(json_resp['data']) == 2
+    assert json_resp['data'][0]['name'] == 'one'
+    assert json_resp['data'][1]['name'] == 'two'
 
 
-def test_get_service_list_by_user_should_return_empty_list_if_no_services(notify_api, service_factory, sample_user):
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-            new_user = User(
-                name='Test User',
-                email_address='new_user@digital.cabinet-office.gov.uk',
-                password='password',
-                mobile_number='+447700900986'
-            )
-            save_model_user(new_user)
+def test_get_service_list_by_user_should_return_empty_list_if_no_services(
+    notify_db,
+    notify_db_session,
+    client,
+    sample_service
+):
+    # service is already created by sample user
+    new_user = create_sample_user(notify_db, notify_db_session, email='foo@bar.gov.uk')
 
-            service_factory.get('one', sample_user, email_from='one')
-            service_factory.get('two', sample_user, email_from='two')
-            service_factory.get('three', sample_user, email_from='three')
-
-            auth_header = create_authorization_header()
-            response = client.get(
-                '/service?user_id={}'.format(new_user.id),
-                headers=[auth_header]
-            )
-            json_resp = json.loads(response.get_data(as_text=True))
-            assert response.status_code == 200
-            assert len(json_resp['data']) == 0
+    auth_header = create_authorization_header()
+    response = client.get(
+        '/service?user_id={}'.format(new_user.id),
+        headers=[auth_header]
+    )
+    json_resp = json.loads(response.get_data(as_text=True))
+    assert response.status_code == 200
+    assert len(json_resp['data']) == 0
 
 
 def test_get_service_list_should_return_empty_list_if_no_services(notify_api, notify_db, notify_db_session):
@@ -445,7 +436,7 @@ def test_should_not_update_service_with_duplicate_name(notify_api,
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
             service_name = "another name"
-            service = create_sample_service(
+            service = create_service(
                 notify_db,
                 notify_db_session,
                 service_name=service_name,
@@ -478,7 +469,7 @@ def test_should_not_update_service_with_duplicate_email_from(notify_api,
         with notify_api.test_client() as client:
             email_from = "duplicate.name"
             service_name = "duplicate name"
-            service = create_sample_service(
+            service = create_service(
                 notify_db,
                 notify_db_session,
                 service_name=service_name,
@@ -914,7 +905,7 @@ def test_remove_user_from_service(notify_api, notify_db, notify_db_session, samp
                 notify_db_session,
                 email="new@digital.cabinet-office.gov.uk")
             # Simulates successfully adding a user to the service
-            second_permission = create_sample_service_permission(
+            second_permission = create_service_permission(
                 notify_db,
                 notify_db_session,
                 user=second_user)
@@ -1018,8 +1009,8 @@ def test_set_reply_to_email_for_service(notify_api, sample_service):
 
 def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notify_db_session):
     with notify_api.test_request_context(), notify_api.test_client() as client:
-        service_1 = create_sample_service(notify_db, notify_db_session, service_name="1", email_from='1')
-        service_2 = create_sample_service(notify_db, notify_db_session, service_name="2", email_from='2')
+        service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1')
+        service_2 = create_service(notify_db, notify_db_session, service_name="2", email_from='2')
 
         create_sample_notification(notify_db, notify_db_session, service=service_2)
 
@@ -1239,8 +1230,8 @@ def test_get_services_with_detailed_flag(notify_api, notify_db, notify_db_sessio
 def test_get_detailed_services_groups_by_service(notify_db, notify_db_session):
     from app.service.rest import get_detailed_services
 
-    service_1 = create_sample_service(notify_db, notify_db_session, service_name="1", email_from='1')
-    service_2 = create_sample_service(notify_db, notify_db_session, service_name="2", email_from='2')
+    service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1')
+    service_2 = create_service(notify_db, notify_db_session, service_name="2", email_from='2')
 
     create_sample_notification(notify_db, notify_db_session, service=service_1, status='created')
     create_sample_notification(notify_db, notify_db_session, service=service_2, status='created')
@@ -1266,8 +1257,8 @@ def test_get_detailed_services_groups_by_service(notify_db, notify_db_session):
 def test_get_detailed_services_includes_services_with_no_notifications(notify_db, notify_db_session):
     from app.service.rest import get_detailed_services
 
-    service_1 = create_sample_service(notify_db, notify_db_session, service_name="1", email_from='1')
-    service_2 = create_sample_service(notify_db, notify_db_session, service_name="2", email_from='2')
+    service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1')
+    service_2 = create_service(notify_db, notify_db_session, service_name="2", email_from='2')
 
     create_sample_notification(notify_db, notify_db_session, service=service_1)
 

From d4a300ec7a7fd3af08a67c7e035d4aca253d72cd Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Wed, 9 Nov 2016 11:45:39 +0000
Subject: [PATCH 13/29] add only_active flag to GET /services/

does what it says on the tin
---
 app/dao/services_dao.py        | 11 ++++++++---
 app/service/rest.py            |  2 +-
 tests/app/conftest.py          |  3 ++-
 tests/app/service/test_rest.py | 17 +++++++++++++++++
 4 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py
index ceb1d1635..6a7be124e 100644
--- a/app/dao/services_dao.py
+++ b/app/dao/services_dao.py
@@ -28,12 +28,17 @@ from app.models import (
 from app.statsd_decorators import statsd
 
 
-def dao_fetch_all_services():
-    return Service.query.order_by(
+def dao_fetch_all_services(only_active=False):
+    query = Service.query.order_by(
         asc(Service.created_at)
     ).options(
         joinedload('users')
-    ).all()
+    )
+
+    if only_active:
+        query = query.filter(Service.active)
+
+    return query.all()
 
 
 def dao_fetch_service_by_id(service_id):
diff --git a/app/service/rest.py b/app/service/rest.py
index 4e6d94ba0..fbdbb04bf 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -67,7 +67,7 @@ def get_services():
     elif request.args.get('detailed') == 'True':
         return jsonify(data=get_detailed_services())
     else:
-        services = dao_fetch_all_services()
+        services = dao_fetch_all_services(only_active=request.args.get('only_active') == 'True')
     data = service_schema.dump(services, many=True).data
     return jsonify(data=data)
 
diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index 3d1af5359..aa4b7b5fd 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -129,6 +129,7 @@ def sample_service(notify_db,
                    notify_db_session,
                    service_name="Sample service",
                    user=None,
+                   active=True,
                    restricted=False,
                    limit=1000,
                    email_from=None):
@@ -139,7 +140,7 @@ def sample_service(notify_db,
     data = {
         'name': service_name,
         'message_limit': limit,
-        'active': True,
+        'active': active,
         'restricted': restricted,
         'email_from': email_from,
         'created_by': user
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index c1983321e..42184d6de 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -38,6 +38,23 @@ def test_get_service_list(notify_api, service_factory):
             assert json_resp['data'][2]['name'] == 'three'
 
 
+def test_get_service_list_with_only_active_flag(client, service_factory):
+    inactive = service_factory.get('one', email_from='one')
+    active = service_factory.get('two', email_from='two')
+
+    inactive.active = False
+
+    auth_header = create_authorization_header()
+    response = client.get(
+        '/service?only_active=True',
+        headers=[auth_header]
+    )
+    assert response.status_code == 200
+    json_resp = json.loads(response.get_data(as_text=True))
+    assert len(json_resp['data']) == 1
+    assert json_resp['data'][0]['id'] == str(active.id)
+
+
 def test_get_service_list_by_user(notify_db, notify_db_session, client, sample_user, service_factory):
     other_user = create_sample_user(notify_db, notify_db_session, email='foo@bar.gov.uk')
     service_factory.get('one', sample_user, email_from='one')

From 346d90e3199f0def6a0d75dae23ad8babe13a0b5 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Wed, 9 Nov 2016 14:56:54 +0000
Subject: [PATCH 14/29] update V2 error response to {status_code: 403,  errors:
 [error: AuthError, message: token has expired}] }

---
 app/authentication/auth.py                    |  8 +-
 app/errors.py                                 | 10 ++-
 app/schema_validation/__init__.py             |  7 +-
 .../test_notification_schemas.py              | 25 +++---
 .../notifications/test_post_notifications.py  | 14 +--
 tests/app/v2/test_errors.py                   | 89 +++++++++++++++++++
 6 files changed, 128 insertions(+), 25 deletions(-)
 create mode 100644 tests/app/v2/test_errors.py

diff --git a/app/authentication/auth.py b/app/authentication/auth.py
index f8ea32cd4..5b69c71c5 100644
--- a/app/authentication/auth.py
+++ b/app/authentication/auth.py
@@ -18,8 +18,12 @@ class AuthError(Exception):
     def to_dict_v2(self):
         return {
             'status_code': self.code,
-            'message': self.short_message,
-            'fields': self.message
+            "errors": [
+                {
+                    "error": "AuthError",
+                    "message": self.short_message
+                }
+            ]
         }
 
 
diff --git a/app/errors.py b/app/errors.py
index db5826cbd..546f75f0c 100644
--- a/app/errors.py
+++ b/app/errors.py
@@ -25,9 +25,13 @@ class InvalidRequest(Exception):
         Version 2 of the public api error response.
         '''
         return {
-            "status_code": self.code,
-            "message": self.message,
-            "fields": self.fields
+            "status_code": self.status_code,
+            "errors": [
+                {
+                    "error": self.__class__.__name__,
+                    "message": self.message
+                }
+            ]
         }
 
     def __str__(self):
diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py
index 51d43df7b..0f7099939 100644
--- a/app/schema_validation/__init__.py
+++ b/app/schema_validation/__init__.py
@@ -1,4 +1,6 @@
 import json
+from collections import OrderedDict
+
 from jsonschema import Draft4Validator, ValidationError
 
 
@@ -16,12 +18,11 @@ def build_error_message(errors, schema):
         field = "'{}' {}".format(e.path[0], e.schema.get('validationMessage')) if e.schema.get(
             'validationMessage') else e.message
         s = field.split("'")
-        field = {s[1]: s[2].strip()}
+        field = OrderedDict({"error": "ValidationError", "message": {s[1]: s[2].strip()}})
         fields.append(field)
     message = {
         "status_code": 400,
-        "message": "Validation error occurred - {}".format(schema['title']),
-        "fields": fields
+        "errors": fields
     }
 
     return json.dumps(message)
diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py
index ac63ec6d7..d917a8cf7 100644
--- a/tests/app/v2/notifications/test_notification_schemas.py
+++ b/tests/app/v2/notifications/test_notification_schemas.py
@@ -20,7 +20,7 @@ valid_json_with_optionals = {
 
 @pytest.mark.parametrize("input", [valid_json, valid_json_with_optionals])
 def test_post_sms_schema_is_valid(input):
-    validate(input, post_sms_request)
+    assert validate(input, post_sms_request) == input
 
 
 def test_post_sms_json_schema_bad_uuid_and_missing_phone_number():
@@ -28,11 +28,13 @@ def test_post_sms_json_schema_bad_uuid_and_missing_phone_number():
     with pytest.raises(ValidationError) as e:
         validate(j, post_sms_request)
     error = json.loads(e.value.message)
-    assert "POST v2/notifications/sms" in error['message']
-    assert len(error.get('fields')) == 2
-    assert {"phone_number": "is a required property"} in error['fields']
-    assert {"template_id": "not a valid UUID"} in error['fields']
+    assert len(error.keys()) == 2
     assert error.get('status_code') == 400
+    assert len(error.get('errors')) == 2
+    assert {'error': 'ValidationError',
+            'message': {"phone_number": "is a required property"}} in error['errors']
+    assert {'error': 'ValidationError',
+            'message': {"template_id": "not a valid UUID"}} in error['errors']
 
 
 def test_post_sms_schema_with_personalisation_that_is_not_a_dict():
@@ -45,10 +47,11 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict():
     with pytest.raises(ValidationError) as e:
         validate(j, post_sms_request)
     error = json.loads(e.value.message)
-    assert "POST v2/notifications/sms" in error['message']
-    assert len(error.get('fields')) == 1
-    assert error['fields'][0] == {"personalisation": "should contain key value pairs"}
+    assert len(error.get('errors')) == 1
+    assert error['errors'] == [{'error': 'ValidationError',
+                                'message': {"personalisation": "should contain key value pairs"}}]
     assert error.get('status_code') == 400
+    assert len(error.keys()) == 2
 
 
 valid_response = {
@@ -75,7 +78,7 @@ valid_response_with_optionals = {
 
 @pytest.mark.parametrize('input', [valid_response])
 def test_post_sms_response_schema_is_valid(input):
-    validate(input, post_sms_response)
+    assert validate(input, post_sms_response) == input
 
 
 def test_post_sms_response_schema_missing_uri():
@@ -85,5 +88,5 @@ def test_post_sms_response_schema_missing_uri():
         validate(j, post_sms_response)
     error = json.loads(e.value.message)
     assert error['status_code'] == 400
-    assert 'Validation error occurred - response v2/notifications/sms' == error['message']
-    assert [{"uri": "is a required property"}] == error['fields']
+    assert error['errors'] == [{'error': 'ValidationError',
+                               'message': {"uri": "is a required property"}}]
diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py
index 4b93a7aa5..6512eeff9 100644
--- a/tests/app/v2/notifications/test_post_notifications.py
+++ b/tests/app/v2/notifications/test_post_notifications.py
@@ -55,8 +55,8 @@ def test_post_sms_notification_returns_404_and_missing_template(notify_api, samp
             assert response.headers['Content-type'] == 'application/json'
 
             error_json = json.loads(response.get_data(as_text=True))
-            assert error_json['message'] == 'Template not found'
-            assert error_json['fields'] == [{'template': 'Template not found'}]
+            assert error_json['errors'] == [{"error": "BadRequestError",
+                                             "message": 'Template not found'}]
 
 
 def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api, sample_template):
@@ -76,8 +76,8 @@ def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api
             assert response.headers['Content-type'] == 'application/json'
             error_resp = json.loads(response.get_data(as_text=True))
             assert error_resp['status_code'] == 401
-            assert error_resp['message'] == 'Unauthorized, authentication token must be provided'
-            assert error_resp['fields'] == {'token': ['Unauthorized, authentication token must be provided']}
+            assert error_resp['errors'] == [{'error': "AuthError",
+                                             'message': 'Unauthorized, authentication token must be provided'}]
 
 
 def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, sample_template):
@@ -98,5 +98,7 @@ def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, s
             assert response.headers['Content-type'] == 'application/json'
             error_resp = json.loads(response.get_data(as_text=True))
             assert error_resp['status_code'] == 400
-            assert error_resp['message'] == 'Validation error occurred - POST v2/notifications/sms'
-            assert error_resp['fields'] == [{"template_id": "is a required property"}]
+            print(error_resp['errors'])
+            assert error_resp['errors'] == [{'error': 'ValidationError',
+                                             'message': {"template_id": "is a required property"}
+                                             }]
diff --git a/tests/app/v2/test_errors.py b/tests/app/v2/test_errors.py
new file mode 100644
index 000000000..15bd5af87
--- /dev/null
+++ b/tests/app/v2/test_errors.py
@@ -0,0 +1,89 @@
+import json
+
+import pytest
+from flask import url_for
+
+
+@pytest.fixture(scope='function')
+def app_for_test(mocker):
+    import flask
+    from flask import Blueprint
+    from app.authentication.auth import AuthError
+    from app.v2.errors import BadRequestError, TooManyRequestsError
+
+    app = flask.Flask(__name__)
+    app.config['TESTING'] = True
+
+    from app.v2.errors import register_errors
+    blue = Blueprint("v2_under_test", __name__, url_prefix='/v2/under_test')
+
+    @blue.route("/raise_auth_error", methods=["GET"])
+    def raising_auth_error():
+        raise AuthError("some message", 403)
+
+    @blue.route("/raise_bad_request", methods=["GET"])
+    def raising_bad_request():
+        raise BadRequestError(message="you forgot the thing")
+
+    @blue.route("/raise_too_many_requests", methods=["GET"])
+    def raising_too_many_requests():
+        raise TooManyRequestsError(sending_limit="452")
+
+    @blue.route("/raise_validation_error", methods=["GET"])
+    def raising_validation_error():
+        from app.schema_validation import validate
+        from app.v2.notifications.notification_schemas import post_sms_request
+        validate({"template_id": "bad_uuid"}, post_sms_request)
+
+    register_errors(blue)
+    app.register_blueprint(blue)
+
+    return app
+
+
+def test_auth_error(app_for_test):
+    with app_for_test.test_request_context():
+        with app_for_test.test_client() as client:
+            response = client.get(url_for('v2_under_test.raising_auth_error'))
+            assert response.status_code == 403
+            error = json.loads(response.get_data(as_text=True))
+            assert error == {"status_code": 403,
+                             "errors": [{"error": "AuthError",
+                                         "message": "some message"}]}
+
+
+def test_bad_request_error(app_for_test):
+    with app_for_test.test_request_context():
+        with app_for_test.test_client() as client:
+            response = client.get(url_for('v2_under_test.raising_bad_request'))
+            assert response.status_code == 400
+            error = json.loads(response.get_data(as_text=True))
+            assert error == {"status_code": 400,
+                             "errors": [{"error": "BadRequestError",
+                                         "message": "you forgot the thing"}]}
+
+
+def test_too_many_requests_error(app_for_test):
+    with app_for_test.test_request_context():
+        with app_for_test.test_client() as client:
+            response = client.get(url_for('v2_under_test.raising_too_many_requests'))
+            assert response.status_code == 429
+            error = json.loads(response.get_data(as_text=True))
+            assert error == {"status_code": 429,
+                             "errors": [{"error": "TooManyRequestsError",
+                                         "message": "Exceeded send limits (452) for today"}]}
+
+
+def test_validation_error(app_for_test):
+    with app_for_test.test_request_context():
+        with app_for_test.test_client() as client:
+            response = client.get(url_for('v2_under_test.raising_validation_error'))
+            assert response.status_code == 400
+            error = json.loads(response.get_data(as_text=True))
+            assert len(error.keys()) == 2
+            assert error['status_code'] == 400
+            assert len(error['errors']) == 2
+            assert {'error': 'ValidationError',
+                    'message': {'phone_number': 'is a required property'}} in error['errors']
+            assert {'error': 'ValidationError',
+                    'message': {'template_id': 'not a valid UUID'}} in error['errors']

From b0e240267a880c1543a3b2a6ac65b57ef3f802dd Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Wed, 9 Nov 2016 15:07:23 +0000
Subject: [PATCH 15/29] add only_active flag to get services functionality

---
 app/dao/services_dao.py        | 22 ++++++++++++-----
 app/service/rest.py            | 15 +++++++-----
 tests/app/service/test_rest.py | 44 +++++++++++++++++++++++++++-------
 3 files changed, 60 insertions(+), 21 deletions(-)

diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py
index 6a7be124e..dde38af32 100644
--- a/app/dao/services_dao.py
+++ b/app/dao/services_dao.py
@@ -41,22 +41,32 @@ def dao_fetch_all_services(only_active=False):
     return query.all()
 
 
-def dao_fetch_service_by_id(service_id):
-    return Service.query.filter_by(
+def dao_fetch_service_by_id(service_id, only_active=False):
+    query = Service.query.filter_by(
         id=service_id
     ).options(
         joinedload('users')
-    ).one()
+    )
+
+    if only_active:
+        query = query.filter(Service.active)
+
+    return query.one()
 
 
-def dao_fetch_all_services_by_user(user_id):
-    return Service.query.filter(
+def dao_fetch_all_services_by_user(user_id, only_active=False):
+    query = Service.query.filter(
         Service.users.any(id=user_id)
     ).order_by(
         asc(Service.created_at)
     ).options(
         joinedload('users')
-    ).all()
+    )
+
+    if only_active:
+        query = query.filter(Service.active)
+
+    return query.all()
 
 
 def dao_fetch_service_by_id_and_user(service_id, user_id):
diff --git a/app/service/rest.py b/app/service/rest.py
index fbdbb04bf..78138b434 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -61,13 +61,16 @@ register_errors(service_blueprint)
 
 @service_blueprint.route('', methods=['GET'])
 def get_services():
+    only_active = request.args.get('only_active') == 'True'
+    detailed = request.args.get('detailed') == 'True'
     user_id = request.args.get('user_id', None)
+
     if user_id:
-        services = dao_fetch_all_services_by_user(user_id)
-    elif request.args.get('detailed') == 'True':
-        return jsonify(data=get_detailed_services())
+        services = dao_fetch_all_services_by_user(user_id, only_active)
+    elif detailed:
+        return jsonify(data=get_detailed_services(only_active))
     else:
-        services = dao_fetch_all_services(only_active=request.args.get('only_active') == 'True')
+        services = dao_fetch_all_services(only_active)
     data = service_schema.dump(services, many=True).data
     return jsonify(data=data)
 
@@ -267,8 +270,8 @@ def get_detailed_service(service_id, today_only=False):
     return detailed_service_schema.dump(service).data
 
 
-def get_detailed_services():
-    services = {service.id: service for service in dao_fetch_all_services()}
+def get_detailed_services(only_active=False):
+    services = {service.id: service for service in dao_fetch_all_services(only_active)}
     stats = dao_fetch_todays_stats_for_all_services()
 
     for service_id, rows in itertools.groupby(stats, lambda x: x.service_id):
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 42184d6de..3b3bdc744 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -22,9 +22,9 @@ from app.models import KEY_TYPE_TEST
 def test_get_service_list(notify_api, service_factory):
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
-            service_factory.get('one', email_from='one')
-            service_factory.get('two', email_from='two')
-            service_factory.get('three', email_from='three')
+            service_factory.get('one')
+            service_factory.get('two')
+            service_factory.get('three')
             auth_header = create_authorization_header()
             response = client.get(
                 '/service',
@@ -39,8 +39,8 @@ def test_get_service_list(notify_api, service_factory):
 
 
 def test_get_service_list_with_only_active_flag(client, service_factory):
-    inactive = service_factory.get('one', email_from='one')
-    active = service_factory.get('two', email_from='two')
+    inactive = service_factory.get('one')
+    active = service_factory.get('two')
 
     inactive.active = False
 
@@ -55,11 +55,37 @@ def test_get_service_list_with_only_active_flag(client, service_factory):
     assert json_resp['data'][0]['id'] == str(active.id)
 
 
+def test_get_service_list_with_user_id_and_only_active_flag(
+    notify_db,
+    notify_db_session,
+    client,
+    sample_user,
+    service_factory
+):
+    other_user = create_sample_user(notify_db, notify_db_session, email='foo@bar.gov.uk')
+
+    inactive = service_factory.get('one', user=sample_user)
+    active = service_factory.get('two', user=sample_user)
+    from_other_user = service_factory.get('three', user=other_user)
+
+    inactive.active = False
+
+    auth_header = create_authorization_header()
+    response = client.get(
+        '/service?user_id={}&only_active=True'.format(sample_user.id),
+        headers=[auth_header]
+    )
+    assert response.status_code == 200
+    json_resp = json.loads(response.get_data(as_text=True))
+    assert len(json_resp['data']) == 1
+    assert json_resp['data'][0]['id'] == str(active.id)
+
+
 def test_get_service_list_by_user(notify_db, notify_db_session, client, sample_user, service_factory):
     other_user = create_sample_user(notify_db, notify_db_session, email='foo@bar.gov.uk')
-    service_factory.get('one', sample_user, email_from='one')
-    service_factory.get('two', sample_user, email_from='two')
-    service_factory.get('three', other_user, email_from='three')
+    service_factory.get('one', sample_user)
+    service_factory.get('two', sample_user)
+    service_factory.get('three', other_user)
 
     auth_header = create_authorization_header()
     response = client.get(
@@ -140,7 +166,7 @@ def test_get_service_by_id_should_404_if_no_service(notify_api, notify_db):
 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, email_from='new.service')
+            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),

From b2149bf02aaf81d38b215698e7d5d8eb7e6c837e Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Thu, 10 Nov 2016 10:38:36 +0000
Subject: [PATCH 16/29] undo unnecessary change to models

---
 app/models.py | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/app/models.py b/app/models.py
index f5dc10ff9..cf46530ca 100644
--- a/app/models.py
+++ b/app/models.py
@@ -58,7 +58,7 @@ class User(db.Model):
     logged_in_at = db.Column(db.DateTime, nullable=True)
     failed_login_count = db.Column(db.Integer, nullable=False, default=0)
     state = db.Column(db.String, nullable=False, default='pending')
-    platform_admin = db.Column(db.Boolean, nullable=False, default=false())
+    platform_admin = db.Column(db.Boolean, nullable=False, default=False)
 
     @property
     def password(self):
@@ -115,15 +115,15 @@ class Service(db.Model, Versioned):
         unique=False,
         nullable=True,
         onupdate=datetime.datetime.utcnow)
-    active = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=true())
+    active = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=True)
     message_limit = db.Column(db.BigInteger, index=False, unique=False, nullable=False)
     users = db.relationship(
         'User',
         secondary=user_to_service,
         backref=db.backref('user_to_service', lazy='dynamic'))
     restricted = db.Column(db.Boolean, index=False, unique=False, nullable=False)
-    research_mode = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=false())
-    can_send_letters = db.Column(db.Boolean, nullable=False, default=false())
+    research_mode = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=False)
+    can_send_letters = db.Column(db.Boolean, nullable=False, default=False)
     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)
@@ -273,7 +273,7 @@ class Template(db.Model):
         nullable=True,
         onupdate=datetime.datetime.utcnow)
     content = db.Column(db.Text, index=False, unique=False, nullable=False)
-    archived = db.Column(db.Boolean, index=False, nullable=False, default=false())
+    archived = db.Column(db.Boolean, index=False, nullable=False, default=False)
     service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False, nullable=False)
     service = db.relationship('Service', backref=db.backref('templates', lazy='dynamic'))
     subject = db.Column(db.Text, index=False, unique=False, nullable=True)
@@ -291,7 +291,7 @@ class TemplateHistory(db.Model):
     created_at = db.Column(db.DateTime, nullable=False)
     updated_at = db.Column(db.DateTime)
     content = db.Column(db.Text, nullable=False)
-    archived = db.Column(db.Boolean, nullable=False, default=false())
+    archived = db.Column(db.Boolean, nullable=False, default=False)
     service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False)
     service = db.relationship('Service')
     subject = db.Column(db.Text)
@@ -344,7 +344,7 @@ class ProviderDetails(db.Model):
     identifier = db.Column(db.String, nullable=False)
     priority = db.Column(db.Integer, nullable=False)
     notification_type = db.Column(notification_types, nullable=False)
-    active = db.Column(db.Boolean, default=false())
+    active = db.Column(db.Boolean, default=False)
 
 
 JOB_STATUS_PENDING = 'pending'
@@ -431,7 +431,7 @@ class VerifyCode(db.Model):
     code_type = db.Column(db.Enum(*VERIFY_CODE_TYPES, name='verify_code_types'),
                           index=False, unique=False, nullable=False)
     expiry_datetime = db.Column(db.DateTime, nullable=False)
-    code_used = db.Column(db.Boolean, default=false())
+    code_used = db.Column(db.Boolean, default=False)
     created_at = db.Column(
         db.DateTime,
         index=False,

From e8c3a5cddef6b9887b5aa771dbb121a6a1a0c3de Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Thu, 10 Nov 2016 11:07:12 +0000
Subject: [PATCH 17/29] add check for inactive services to auth handler

cleaned up some auth code to marginally improve efficiency of error checking
and hopefully make it easier to read

fixed some incorrect auth headers in the deactivate tests
---
 app/authentication/auth.py                    | 22 ++++++++++---------
 app/models.py                                 |  2 +-
 .../app/authentication/test_authentication.py | 11 ++++++++++
 tests/app/service/test_deactivate.py          | 10 ++++-----
 4 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/app/authentication/auth.py b/app/authentication/auth.py
index 841222279..68e7acf8b 100644
--- a/app/authentication/auth.py
+++ b/app/authentication/auth.py
@@ -46,10 +46,19 @@ def requires_auth():
         return handle_admin_key(auth_token, current_app.config.get('ADMIN_CLIENT_SECRET'))
 
     try:
-        api_keys = get_model_api_keys(client)
+        service = dao_fetch_service_by_id(client)
     except DataError:
         raise AuthError("Invalid token: service id is not the right data type", 403)
-    for api_key in api_keys:
+    except NoResultFound:
+        raise AuthError("Invalid token: service not found", 403)
+
+    if not service.api_keys:
+        raise AuthError("Invalid token: service has no API keys", 403)
+
+    if not service.active:
+        raise AuthError("Invalid token: service is archived", 403)
+
+    for api_key in service.api_keys:
         try:
             get_decode_errors(auth_token, api_key.unsigned_secret)
         except TokenDecodeError:
@@ -60,15 +69,8 @@ def requires_auth():
 
         _request_ctx_stack.top.api_user = api_key
         return
-
-    try:
-        dao_fetch_service_by_id(client)
-    except NoResultFound:
-        raise AuthError("Invalid token: service not found", 403)
-
-    if not api_keys:
-        raise AuthError("Invalid token: service has no API keys", 403)
     else:
+        # service has API keys, but none matching the one the user provided
         raise AuthError("Invalid token: signature, api token is not valid", 403)
 
 
diff --git a/app/models.py b/app/models.py
index cf46530ca..75b9615fc 100644
--- a/app/models.py
+++ b/app/models.py
@@ -188,7 +188,7 @@ class ApiKey(db.Model, Versioned):
     name = db.Column(db.String(255), nullable=False)
     secret = db.Column(db.String(255), unique=True, nullable=False)
     service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False)
-    service = db.relationship('Service', backref=db.backref('api_keys', lazy='dynamic'))
+    service = db.relationship('Service', backref='api_keys')
     key_type = db.Column(db.String(255), db.ForeignKey('key_types.name'), index=True, nullable=False)
     expiry_date = db.Column(db.DateTime)
     created_at = db.Column(
diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py
index 3a6d5b4d2..7c445687a 100644
--- a/tests/app/authentication/test_authentication.py
+++ b/tests/app/authentication/test_authentication.py
@@ -223,6 +223,17 @@ def test_authentication_returns_error_when_service_doesnt_exit(
         assert error_message['message'] == {'token': ['Invalid token: service not found']}
 
 
+def test_authentication_returns_error_when_service_inactive(client, sample_api_key):
+    sample_api_key.service.active = False
+    token = create_jwt_token(secret=str(sample_api_key.id), client_id=str(sample_api_key.service_id))
+
+    response = client.get('/service', headers={'Authorization': 'Bearer {}'.format(token)})
+
+    assert response.status_code == 403
+    error_message = json.loads(response.get_data())
+    assert error_message['message'] == {'token': ['Invalid token: service is archived']}
+
+
 def test_authentication_returns_error_when_service_has_no_secrets(notify_api,
                                                                   sample_service,
                                                                   fake_uuid):
diff --git a/tests/app/service/test_deactivate.py b/tests/app/service/test_deactivate.py
index 50e552816..d81de0e10 100644
--- a/tests/app/service/test_deactivate.py
+++ b/tests/app/service/test_deactivate.py
@@ -11,19 +11,19 @@ from tests.app.conftest import (
 
 
 def test_deactivate_only_allows_post(client, sample_service):
-    auth_header = create_authorization_header(service_id=str(sample_service.id))
+    auth_header = create_authorization_header()
     response = client.get('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header])
     assert response.status_code == 405
 
 
 def test_deactivate_service_errors_with_bad_service_id(client, sample_service):
-    auth_header = create_authorization_header(service_id=str(sample_service.id))
+    auth_header = create_authorization_header()
     response = client.post('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header])
     assert response.status_code == 404
 
 
 def test_deactivating_inactive_service_does_nothing(client, sample_service):
-    auth_header = create_authorization_header(service_id=str(sample_service.id))
+    auth_header = create_authorization_header()
     sample_service.active = False
     response = client.post('/service/{}/deactivate'.format(sample_service.id), headers=[auth_header])
     assert response.status_code == 204
@@ -37,7 +37,7 @@ def deactivated_service(client, notify_db, notify_db_session, sample_service):
     create_api_key(notify_db, notify_db_session)
     create_api_key(notify_db, notify_db_session)
 
-    auth_header = create_authorization_header(service_id=str(sample_service.id))
+    auth_header = create_authorization_header()
     response = client.post('/service/{}/deactivate'.format(sample_service.id), headers=[auth_header])
     assert response.status_code == 204
     assert response.data == b''
@@ -50,7 +50,7 @@ def test_deactivating_service_changes_name_and_email(deactivated_service):
 
 
 def test_deactivating_service_revokes_api_keys(deactivated_service):
-    assert deactivated_service.api_keys.count() == 2
+    assert len(deactivated_service.api_keys) == 2
     for key in deactivated_service.api_keys:
         assert key.expiry_date is not None
         assert key.version == 2

From 8474344c9a57a2b72bc37f63d3b27e8d45eaefee Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Tue, 1 Nov 2016 15:19:56 +0000
Subject: [PATCH 18/29] Ignore case and whitespace in personalisation keys

From a support ticket:

> it's possible to add a personalisation token with trailing whitespace
> (eg. "key " rather than "key"). Can this be trimmed in the UI to guard
> against this? (one of our devs copied and pasted it from a document
> and inadvertently included the space)

> Nothing major but caused a few hours of investigations!

Rather than trim the placeholder in the template, we should treat
placeholders in API calls the same way we do with CSV files, ie we
ignore case and spacing in the name of the placeholder. So
`(( First Name))` is equivalent to `((first_name))`, and both would be
populated with a dictionary like `{'firstName': 'Chris'}`.

Depends on:
- [x] https://github.com/alphagov/notifications-utils/pull/77
---
 app/celery/tasks.py   | 5 +----
 requirements.txt      | 2 +-
 tests/app/conftest.py | 3 ++-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/app/celery/tasks.py b/app/celery/tasks.py
index 9bf47c9d7..3ab7043a2 100644
--- a/app/celery/tasks.py
+++ b/app/celery/tasks.py
@@ -72,10 +72,7 @@ def process_job(job_id):
             'job': str(job.id),
             'to': recipient,
             'row_number': row_number,
-            'personalisation': {
-                key: personalisation.get(key)
-                for key in template.placeholders
-            }
+            'personalisation': dict(personalisation)
         })
 
         if template.template_type == SMS_TYPE:
diff --git a/requirements.txt b/requirements.txt
index a71037824..08834bd7b 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -21,6 +21,6 @@ jsonschema==2.5.1
 
 git+https://github.com/alphagov/notifications-python-client.git@1.3.0#egg=notifications-python-client==1.3.0
 
-git+https://github.com/alphagov/notifications-utils.git@9.1.1#egg=notifications-utils==9.1.1
+git+https://github.com/alphagov/notifications-utils.git@9.2.1#egg=notifications-utils==9.2.1
 
 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3
diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index 002c7c13c..2817e5dcc 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -190,7 +190,8 @@ def sample_template(notify_db,
 
 @pytest.fixture(scope='function')
 def sample_template_with_placeholders(notify_db, notify_db_session):
-    return sample_template(notify_db, notify_db_session, content="Hello ((name))\nYour thing is due soon")
+    # deliberate space and title case in placeholder
+    return sample_template(notify_db, notify_db_session, content="Hello (( Name))\nYour thing is due soon")
 
 
 @pytest.fixture(scope='function')

From 988bf061320988ebb2f6fa6977d83ac8ece5701a Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Tue, 1 Nov 2016 15:20:28 +0000
Subject: [PATCH 19/29] Update error message for invalid email addresses

It has a trailing full stop since:
https://github.com/alphagov/notifications-utils/pull/76
---
 tests/app/invite/test_invite_rest.py                   | 2 +-
 tests/app/notifications/rest/test_send_notification.py | 4 ++--
 tests/app/user/test_rest.py                            | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/app/invite/test_invite_rest.py b/tests/app/invite/test_invite_rest.py
index 301bf6e69..9cf9c2f13 100644
--- a/tests/app/invite/test_invite_rest.py
+++ b/tests/app/invite/test_invite_rest.py
@@ -85,7 +85,7 @@ def test_create_invited_user_invalid_email(notify_api, sample_service, mocker):
             assert response.status_code == 400
             json_resp = json.loads(response.get_data(as_text=True))
             assert json_resp['result'] == 'error'
-            assert json_resp['message'] == {'email_address': ['Not a valid email address']}
+            assert json_resp['message'] == {'email_address': ['Not a valid email address.']}
             app.celery.tasks.send_email.apply_async.assert_not_called()
 
 
diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py
index a28590c40..e87b6410e 100644
--- a/tests/app/notifications/rest/test_send_notification.py
+++ b/tests/app/notifications/rest/test_send_notification.py
@@ -306,7 +306,7 @@ def test_should_reject_email_notification_with_bad_email(notify_api, sample_emai
             mocked.apply_async.assert_not_called()
             assert response.status_code == 400
             assert data['result'] == 'error'
-            assert data['message']['to'][0] == 'Not a valid email address'
+            assert data['message']['to'][0] == 'Not a valid email address.'
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
@@ -913,7 +913,7 @@ def test_create_template_raises_invalid_request_exception_with_missing_personali
     from app.notifications.rest import create_template_object_for_notification
     with pytest.raises(InvalidRequest) as e:
         create_template_object_for_notification(template, {})
-    assert {'template': ['Missing personalisation: name']} == e.value.message
+    assert {'template': ['Missing personalisation:  Name']} == e.value.message
 
 
 def test_create_template_raises_invalid_request_exception_with_too_much_personalisation_data(
diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py
index 14b26d387..2de1f7c38 100644
--- a/tests/app/user/test_rest.py
+++ b/tests/app/user/test_rest.py
@@ -470,7 +470,7 @@ def test_send_user_reset_password_should_return_400_when_data_is_not_email_addre
             headers=[('Content-Type', 'application/json'), auth_header])
 
         assert resp.status_code == 400
-        assert json.loads(resp.get_data(as_text=True))['message'] == {'email': ['Not a valid email address']}
+        assert json.loads(resp.get_data(as_text=True))['message'] == {'email': ['Not a valid email address.']}
 
 
 @freeze_time("2016-01-01 11:09:00.061258")

From 8b64aa7e79f63a33aa4e9436f6ba1631f6b66575 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Thu, 10 Nov 2016 12:07:29 +0000
Subject: [PATCH 20/29] Use POST endpoint for updating a user attr

---
 app/user/rest.py            | 2 +-
 tests/app/user/test_rest.py | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/user/rest.py b/app/user/rest.py
index e41e10877..a188f162e 100644
--- a/app/user/rest.py
+++ b/app/user/rest.py
@@ -69,7 +69,7 @@ def update_user(user_id):
     return jsonify(data=user_schema.dump(user_to_update).data), 200
 
 
-@user.route('/<uuid:user_id>/update-attribute', methods=['PUT'])
+@user.route('/<uuid:user_id>', methods=['POST'])
 def update_user_attribute(user_id):
     user_to_update = get_user_by_id(user_id=user_id)
     req_json = request.get_json()
diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py
index d9261fbb8..951931e01 100644
--- a/tests/app/user/test_rest.py
+++ b/tests/app/user/test_rest.py
@@ -186,7 +186,7 @@ def test_put_user(notify_api, notify_db, notify_db_session, sample_service):
     ('email_address', 'newuser@mail.com'),
     ('mobile_number', '+4407700900460')
 ])
-def test_put_user_attribute(client, sample_user, user_attribute, user_value):
+def test_post_user_attribute(client, sample_user, user_attribute, user_value):
     assert getattr(sample_user, user_attribute) != user_value
     update_dict = {
         user_attribute: user_value
@@ -194,7 +194,7 @@ def test_put_user_attribute(client, sample_user, user_attribute, user_value):
     auth_header = create_authorization_header()
     headers = [('Content-Type', 'application/json'), auth_header]
 
-    resp = client.put(
+    resp = client.post(
         url_for('user.update_user_attribute', user_id=sample_user.id),
         data=json.dumps(update_dict),
         headers=headers)

From f85ee547075baa83f20431b6fa9a022827f5abff Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Thu, 10 Nov 2016 13:09:25 +0000
Subject: [PATCH 21/29] Refactor stuff + stricter validation for updating only
 ALLOWED user attrs

---
 app/schemas.py                  |  5 +++--
 tests/app/dao/test_users_dao.py |  5 +----
 tests/app/test_schemas.py       | 25 ++++++++++++++++++++++---
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/app/schemas.py b/app/schemas.py
index b9f75cb2c..7f52b9541 100644
--- a/app/schemas.py
+++ b/app/schemas.py
@@ -110,8 +110,9 @@ class UserUpdateAttributeSchema(BaseSchema):
     class Meta:
         model = models.User
         exclude = (
-            "updated_at", "created_at", "user_to_service",
-            "_password", "verify_codes")
+            'id', 'updated_at', 'created_at', 'user_to_service',
+            '_password', 'verify_codes', 'logged_in_at', 'password_changed_at',
+            'failed_login_count', 'state', 'platform_admin')
         strict = True
 
     @validates('name')
diff --git a/tests/app/dao/test_users_dao.py b/tests/app/dao/test_users_dao.py
index 7d1901f0d..cc814b160 100644
--- a/tests/app/dao/test_users_dao.py
+++ b/tests/app/dao/test_users_dao.py
@@ -64,11 +64,8 @@ def test_get_user_not_exists(notify_api, notify_db, notify_db_session, fake_uuid
 
 
 def test_get_user_invalid_id(notify_api, notify_db, notify_db_session):
-    try:
+    with pytest.raises(DataError):
         get_user_by_id(user_id="blah")
-        pytest.fail("DataError exception not thrown.")
-    except DataError:
-        pass
 
 
 def test_delete_users(notify_api, notify_db, notify_db_session, sample_user):
diff --git a/tests/app/test_schemas.py b/tests/app/test_schemas.py
index 5f2bc4371..1d66f3290 100644
--- a/tests/app/test_schemas.py
+++ b/tests/app/test_schemas.py
@@ -1,5 +1,7 @@
 import pytest
 
+from marshmallow import ValidationError
+
 
 def test_job_schema_doesnt_return_notifications(sample_notification_with_job):
     from app.schemas import job_schema
@@ -32,7 +34,7 @@ def test_notification_schema_adds_api_key_name(sample_notification_with_api_key)
     ('email_address', 'newuser@mail.com'),
     ('mobile_number', '+4407700900460')
 ])
-def test_user_schema_accepts_valid_attributes(user_attribute, user_value):
+def test_user_update_schema_accepts_valid_attribute_pairs(user_attribute, user_value):
     update_dict = {
         user_attribute: user_value
     }
@@ -48,11 +50,28 @@ def test_user_schema_accepts_valid_attributes(user_attribute, user_value):
     ('email_address', 'bademail@...com'),
     ('mobile_number', '+44077009')
 ])
-def test_user_schema_rejects_invalid_attributes(user_attribute, user_value):
+def test_user_update_schema_rejects_invalid_attribute_pairs(user_attribute, user_value):
     from app.schemas import user_update_schema_load_json
     update_dict = {
         user_attribute: user_value
     }
 
-    with pytest.raises(Exception):
+    with pytest.raises(ValidationError):
         data, errors = user_update_schema_load_json.load(update_dict)
+
+
+@pytest.mark.parametrize('user_attribute', [
+    'id', 'updated_at', 'created_at', 'user_to_service',
+    '_password', 'verify_codes', 'logged_in_at', 'password_changed_at',
+    'failed_login_count', 'state', 'platform_admin'
+])
+def test_user_update_schema_rejects_disallowed_attribute_keys(user_attribute):
+    update_dict = {
+        user_attribute: 'not important'
+    }
+    from app.schemas import user_update_schema_load_json
+
+    with pytest.raises(ValidationError) as excinfo:
+        data, errors = user_update_schema_load_json.load(update_dict)
+
+    assert excinfo.value.messages['_schema'][0] == 'Unknown field name {}'.format(user_attribute)

From 7d763260ce93b960a26a2fe1d9a34b62ab143b4d Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Thu, 10 Nov 2016 14:53:39 +0000
Subject: [PATCH 22/29] Update format of the errors for DataError and exception

---
 app/v2/errors.py            |  8 ++++----
 tests/app/v2/test_errors.py | 30 +++++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/app/v2/errors.py b/app/v2/errors.py
index 8c98e48f8..26cc3b36e 100644
--- a/app/v2/errors.py
+++ b/app/v2/errors.py
@@ -1,10 +1,8 @@
 import json
-
 from flask import jsonify, current_app
 from jsonschema import ValidationError
 from sqlalchemy.exc import DataError
 from sqlalchemy.orm.exc import NoResultFound
-
 from app.authentication.auth import AuthError
 from app.errors import InvalidRequest
 
@@ -42,7 +40,8 @@ def register_errors(blueprint):
     @blueprint.errorhandler(DataError)
     def no_result_found(e):
         current_app.logger.exception(e)
-        return jsonify(message="No result found"), 404
+        return jsonify(status_code=404,
+                       errors=[{"error": e.__class__.__name__, "message": "No result found"}]), 404
 
     @blueprint.errorhandler(AuthError)
     def auth_error(error):
@@ -51,4 +50,5 @@ def register_errors(blueprint):
     @blueprint.errorhandler(Exception)
     def internal_server_error(error):
         current_app.logger.exception(error)
-        return jsonify(message='Internal server error'), 500
+        return jsonify(status_code=500,
+                       errors=[{"error": error.__class__.__name__, "message": 'Internal server error'}]), 500
diff --git a/tests/app/v2/test_errors.py b/tests/app/v2/test_errors.py
index 15bd5af87..a95ddb680 100644
--- a/tests/app/v2/test_errors.py
+++ b/tests/app/v2/test_errors.py
@@ -1,7 +1,7 @@
 import json
-
 import pytest
 from flask import url_for
+from sqlalchemy.exc import DataError
 
 
 @pytest.fixture(scope='function')
@@ -35,6 +35,14 @@ def app_for_test(mocker):
         from app.v2.notifications.notification_schemas import post_sms_request
         validate({"template_id": "bad_uuid"}, post_sms_request)
 
+    @blue.route("raise_data_error", methods=["GET"])
+    def raising_data_error():
+        raise DataError("There was a db problem", "params", "orig")
+
+    @blue.route("raise_exception", methods=["GET"])
+    def raising_exception():
+        raise AssertionError("Raising any old exception")
+
     register_errors(blue)
     app.register_blueprint(blue)
 
@@ -87,3 +95,23 @@ def test_validation_error(app_for_test):
                     'message': {'phone_number': 'is a required property'}} in error['errors']
             assert {'error': 'ValidationError',
                     'message': {'template_id': 'not a valid UUID'}} in error['errors']
+
+
+def test_data_errors(app_for_test):
+    with app_for_test.test_request_context():
+        with app_for_test.test_client() as client:
+            response = client.get(url_for('v2_under_test.raising_data_error'))
+            assert response.status_code == 404
+            error = json.loads(response.get_data(as_text=True))
+            assert error == {"status_code": 404,
+                             "errors": [{"error": "DataError", "message": "No result found"}]}
+
+
+def test_internal_server_error_handler(app_for_test):
+    with app_for_test.test_request_context():
+        with app_for_test.test_client() as client:
+            response = client.get(url_for("v2_under_test.raising_exception"))
+            assert response.status_code == 500
+            error = json.loads(response.get_data(as_text=True))
+            assert error == {"status_code": 500,
+                             "errors": [{"error": "AssertionError", "message": "Internal server error"}]}

From c758694b988a348ee093e7c7446489b04c68b757 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Thu, 10 Nov 2016 16:30:51 +0000
Subject: [PATCH 23/29] Change validation error message to a string from a
 dict.

---
 app/schema_validation/__init__.py                       | 2 +-
 app/schema_validation/definitions.py                    | 2 +-
 tests/app/v2/notifications/test_notification_schemas.py | 8 ++++----
 tests/app/v2/notifications/test_post_notifications.py   | 4 ++--
 tests/app/v2/test_errors.py                             | 5 +++--
 5 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py
index 0f7099939..4cb27b12b 100644
--- a/app/schema_validation/__init__.py
+++ b/app/schema_validation/__init__.py
@@ -18,7 +18,7 @@ def build_error_message(errors, schema):
         field = "'{}' {}".format(e.path[0], e.schema.get('validationMessage')) if e.schema.get(
             'validationMessage') else e.message
         s = field.split("'")
-        field = OrderedDict({"error": "ValidationError", "message": {s[1]: s[2].strip()}})
+        field = OrderedDict({"error": "ValidationError", "message": "{}{}".format(s[1], s[2])})
         fields.append(field)
     message = {
         "status_code": 400,
diff --git a/app/schema_validation/definitions.py b/app/schema_validation/definitions.py
index e65566d7b..3841b64a8 100644
--- a/app/schema_validation/definitions.py
+++ b/app/schema_validation/definitions.py
@@ -6,7 +6,7 @@ If the definition is specific to a version put it in a definition file in the ve
 uuid = {
     "type": "string",
     "pattern": "^[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}$",
-    "validationMessage": "not a valid UUID",
+    "validationMessage": "is not a valid UUID",
     "code": "1001",  # yet to be implemented
     "link": "link to our error documentation not yet implemented"
 }
diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py
index d917a8cf7..74c734c2f 100644
--- a/tests/app/v2/notifications/test_notification_schemas.py
+++ b/tests/app/v2/notifications/test_notification_schemas.py
@@ -32,9 +32,9 @@ def test_post_sms_json_schema_bad_uuid_and_missing_phone_number():
     assert error.get('status_code') == 400
     assert len(error.get('errors')) == 2
     assert {'error': 'ValidationError',
-            'message': {"phone_number": "is a required property"}} in error['errors']
+            'message': "phone_number is a required property"} in error['errors']
     assert {'error': 'ValidationError',
-            'message': {"template_id": "not a valid UUID"}} in error['errors']
+            'message': "template_id is not a valid UUID"} in error['errors']
 
 
 def test_post_sms_schema_with_personalisation_that_is_not_a_dict():
@@ -49,7 +49,7 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict():
     error = json.loads(e.value.message)
     assert len(error.get('errors')) == 1
     assert error['errors'] == [{'error': 'ValidationError',
-                                'message': {"personalisation": "should contain key value pairs"}}]
+                                'message': "personalisation should contain key value pairs"}]
     assert error.get('status_code') == 400
     assert len(error.keys()) == 2
 
@@ -89,4 +89,4 @@ def test_post_sms_response_schema_missing_uri():
     error = json.loads(e.value.message)
     assert error['status_code'] == 400
     assert error['errors'] == [{'error': 'ValidationError',
-                               'message': {"uri": "is a required property"}}]
+                               'message': "uri is a required property"}]
diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py
index 6512eeff9..c02e2d8e9 100644
--- a/tests/app/v2/notifications/test_post_notifications.py
+++ b/tests/app/v2/notifications/test_post_notifications.py
@@ -55,6 +55,7 @@ def test_post_sms_notification_returns_404_and_missing_template(notify_api, samp
             assert response.headers['Content-type'] == 'application/json'
 
             error_json = json.loads(response.get_data(as_text=True))
+            assert error_json['status_code'] == 400
             assert error_json['errors'] == [{"error": "BadRequestError",
                                              "message": 'Template not found'}]
 
@@ -98,7 +99,6 @@ def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, s
             assert response.headers['Content-type'] == 'application/json'
             error_resp = json.loads(response.get_data(as_text=True))
             assert error_resp['status_code'] == 400
-            print(error_resp['errors'])
             assert error_resp['errors'] == [{'error': 'ValidationError',
-                                             'message': {"template_id": "is a required property"}
+                                             'message': "template_id is a required property"
                                              }]
diff --git a/tests/app/v2/test_errors.py b/tests/app/v2/test_errors.py
index a95ddb680..5f0ed0f81 100644
--- a/tests/app/v2/test_errors.py
+++ b/tests/app/v2/test_errors.py
@@ -88,13 +88,14 @@ def test_validation_error(app_for_test):
             response = client.get(url_for('v2_under_test.raising_validation_error'))
             assert response.status_code == 400
             error = json.loads(response.get_data(as_text=True))
+            print(error)
             assert len(error.keys()) == 2
             assert error['status_code'] == 400
             assert len(error['errors']) == 2
             assert {'error': 'ValidationError',
-                    'message': {'phone_number': 'is a required property'}} in error['errors']
+                    'message': "phone_number is a required property"} in error['errors']
             assert {'error': 'ValidationError',
-                    'message': {'template_id': 'not a valid UUID'}} in error['errors']
+                    'message': "template_id is not a valid UUID"} in error['errors']
 
 
 def test_data_errors(app_for_test):

From 6501a871bc26981e274b3780206175be9db5c027 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Mon, 7 Nov 2016 16:20:41 +0000
Subject: [PATCH 24/29] Add letter to possible template types in DB

A letter type was added to the `enum` in the `Template` model at the
same it was added to the `Notification` model. But the migration was
only done for the `notifications` table, not the `templates` table.

See: https://github.com/alphagov/notifications-api/commit/25db1bce#diff-516aab258e161fc65e7564dabd2c625aR19

This commit does the migration to add `letter` as a possible value for
the `template_type` column, which is a bit fiddly because `enum`s.

Before:
```
notification_api=# select enum_range(null::template_type);
 enum_range
-------------
 {sms,email}
(1 row)
```

After upgrade:
```
notification_api=# select enum_range(null::template_type);
     enum_range
--------------------
 {sms,email,letter}
(1 row)
```

After downgrade
```
notification_api=# select enum_range(null::template_type);
 enum_range
-------------
 {sms,email}
(1 row)
```
---
 .../versions/0059_add_letter_template_type.py | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 migrations/versions/0059_add_letter_template_type.py

diff --git a/migrations/versions/0059_add_letter_template_type.py b/migrations/versions/0059_add_letter_template_type.py
new file mode 100644
index 000000000..2b4cb6eea
--- /dev/null
+++ b/migrations/versions/0059_add_letter_template_type.py
@@ -0,0 +1,56 @@
+"""empty message
+
+Revision ID: f266fb67597a
+Revises: 0058_add_letters_flag
+Create Date: 2016-11-07 16:13:18.961527
+
+"""
+
+from alembic import op
+import sqlalchemy as sa
+
+# revision identifiers, used by Alembic.
+revision = '0059_add_letter_template_type'
+down_revision = '0058_add_letters_flag'
+
+
+name = 'template_type'
+tmp_name = 'tmp_' + name
+
+old_options = ('sms', 'email')
+new_options = old_options + ('letter',)
+
+new_type = sa.Enum(*new_options, name=name)
+old_type = sa.Enum(*old_options, name=name)
+
+tcr = sa.sql.table(
+    'templates',
+    sa.Column('template_type', new_type, nullable=False)
+)
+
+
+def upgrade():
+    op.execute('ALTER TYPE ' + name + ' RENAME TO ' + tmp_name)
+
+    new_type.create(op.get_bind())
+    op.execute(
+        'ALTER TABLE templates ALTER COLUMN template_type ' +
+        'TYPE ' + name + ' USING template_type::text::' + name
+    )
+    op.execute('DROP TYPE ' + tmp_name)
+
+
+def downgrade():
+    # Convert 'letter' template into 'email'
+    op.execute(
+        tcr.update().where(tcr.c.template_type=='letter').values(template_type='email')
+    )
+
+    op.execute('ALTER TYPE ' + name + ' RENAME TO ' + tmp_name)
+
+    old_type.create(op.get_bind())
+    op.execute(
+        'ALTER TABLE templates ALTER COLUMN template_type ' +
+        'TYPE ' + name + ' USING template_type::text::' + name
+    )
+    op.execute('DROP TYPE ' + tmp_name)

From b0d88e0888cb5da0f6f928d8cb70fa6c41a02cef Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Fri, 11 Nov 2016 10:50:38 +0000
Subject: [PATCH 25/29] Removed OrderedDict Added missing assert in test

---
 app/schema_validation/__init__.py          | 2 +-
 tests/app/notifications/test_validators.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py
index 4cb27b12b..88f0e2421 100644
--- a/app/schema_validation/__init__.py
+++ b/app/schema_validation/__init__.py
@@ -18,7 +18,7 @@ def build_error_message(errors, schema):
         field = "'{}' {}".format(e.path[0], e.schema.get('validationMessage')) if e.schema.get(
             'validationMessage') else e.message
         s = field.split("'")
-        field = OrderedDict({"error": "ValidationError", "message": "{}{}".format(s[1], s[2])})
+        field = {"error": "ValidationError", "message": "{}{}".format(s[1], s[2])}
         fields.append(field)
     message = {
         "status_code": 400,
diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py
index 2810e75e5..03781bcb8 100644
--- a/tests/app/notifications/test_validators.py
+++ b/tests/app/notifications/test_validators.py
@@ -48,7 +48,7 @@ def test_check_template_is_for_notification_type_fails_when_template_type_does_n
     with pytest.raises(BadRequestError) as e:
         check_template_is_for_notification_type(notification_type=notification_type,
                                                 template_type=template_type)
-    e.value.status_code == 400
+    assert e.value.status_code == 400
     error_message = '{0} template is not suitable for {1} notification'.format(template_type, notification_type)
     assert e.value.message == error_message
     assert e.value.fields == [{'template': error_message}]

From 488ba7a1ef35f5f6a3e28aa559bf16ee4ee90560 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Fri, 11 Nov 2016 10:56:21 +0000
Subject: [PATCH 26/29] Remove import

---
 app/schema_validation/__init__.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py
index 88f0e2421..71dd37c18 100644
--- a/app/schema_validation/__init__.py
+++ b/app/schema_validation/__init__.py
@@ -1,5 +1,4 @@
 import json
-from collections import OrderedDict
 
 from jsonschema import Draft4Validator, ValidationError
 

From 9ae6e14140aa90c1b15b1b47525fef402f2b85d8 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Thu, 10 Nov 2016 17:07:02 +0000
Subject: [PATCH 27/29] move deactivate functionality into one database
 transactions

this means that any errors will cause the entire thing to roll back

unfortunately, to do this we have to circumvent our regular code, which calls commit a lot, and lazily loads a lot of things, which will flush, and cause the version decorators to fail. so we have to write a lot of stuff by hand and re-select the service (even though it's already been queried) just to populate the api_keys and templates relationship on it
---
 app/dao/dao_utils.py                 |  2 --
 app/dao/services_dao.py              | 29 +++++++++++++++++++++++++++-
 app/models.py                        |  2 +-
 app/service/rest.py                  | 18 +++--------------
 tests/app/service/test_deactivate.py |  2 +-
 5 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/app/dao/dao_utils.py b/app/dao/dao_utils.py
index bdaafc074..96628be06 100644
--- a/app/dao/dao_utils.py
+++ b/app/dao/dao_utils.py
@@ -9,7 +9,6 @@ def transactional(func):
     @wraps(func)
     def commit_or_rollback(*args, **kwargs):
         from flask import current_app
-        from app import db
         try:
             res = func(*args, **kwargs)
             db.session.commit()
@@ -27,7 +26,6 @@ def version_class(model_class, history_cls=None):
     def versioned(func):
         @wraps(func)
         def record_version(*args, **kwargs):
-            from app import db
             func(*args, **kwargs)
             history_objects = [create_hist(obj) for obj in
                                itertools.chain(db.session.new, db.session.dirty)
diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py
index dde38af32..604f0dd25 100644
--- a/app/dao/services_dao.py
+++ b/app/dao/services_dao.py
@@ -1,5 +1,5 @@
 import uuid
-from datetime import date
+from datetime import date, datetime
 
 from sqlalchemy import asc, func
 from sqlalchemy.orm import joinedload
@@ -69,6 +69,33 @@ def dao_fetch_all_services_by_user(user_id, only_active=False):
     return query.all()
 
 
+@transactional
+@version_class(Service)
+@version_class(Template, TemplateHistory)
+@version_class(ApiKey)
+def dao_deactive_service(service_id):
+    # have to eager load templates and api keys so that we don't flush when we loop through them
+    service = Service.query.options(
+        joinedload('templates'),
+        joinedload('api_keys'),
+    ).filter(Service.id == service_id).one()
+
+    service.active = False
+    service.name = '_archived_' + service.name
+    service.email_from = '_archived_' + service.email_from
+
+
+    for template in service.templates:
+        template.archived = True
+
+    for api_key in service.api_keys:
+        api_key.expiry_date = datetime.utcnow()
+
+    db.session.add(service)
+    db.session.add_all(service.templates)
+    db.session.add_all(service.api_keys)
+
+
 def dao_fetch_service_by_id_and_user(service_id, user_id):
     return Service.query.filter(
         Service.users.any(id=user_id),
diff --git a/app/models.py b/app/models.py
index 75b9615fc..2f2c6163d 100644
--- a/app/models.py
+++ b/app/models.py
@@ -275,7 +275,7 @@ class Template(db.Model):
     content = db.Column(db.Text, index=False, unique=False, nullable=False)
     archived = db.Column(db.Boolean, index=False, nullable=False, default=False)
     service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False, nullable=False)
-    service = db.relationship('Service', backref=db.backref('templates', lazy='dynamic'))
+    service = db.relationship('Service', backref='templates')
     subject = db.Column(db.Text, index=False, unique=False, nullable=True)
     created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False)
     created_by = db.relationship('User')
diff --git a/app/service/rest.py b/app/service/rest.py
index 78138b434..312839435 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -25,16 +25,14 @@ from app.dao.services_dao import (
     dao_fetch_stats_for_service,
     dao_fetch_todays_stats_for_service,
     dao_fetch_weekly_historical_stats_for_service,
-    dao_fetch_todays_stats_for_all_services
+    dao_fetch_todays_stats_for_all_services,
+    dao_deactive_service
 )
 from app.dao.service_whitelist_dao import (
     dao_fetch_service_whitelist,
     dao_add_and_commit_whitelisted_contacts,
     dao_remove_service_whitelist
 )
-from app.dao.templates_dao import (
-    dao_update_template
-)
 from app.dao import notifications_dao
 from app.dao.provider_statistics_dao import get_fragment_count
 from app.dao.users_dao import get_model_users
@@ -326,17 +324,7 @@ def deactivate_service(service_id):
         # assume already inactive, don't change service name
         return '', 204
 
-    service.active = False
-    service.name = '_archived_' + service.name
-    service.email_from = '_archived_' + service.email_from
-    dao_update_service(service)
-
-    for template in service.templates:
-        template.archived = True
-        dao_update_template(template)
-
-    for api_key in service.api_keys:
-        expire_api_key(service.id, api_key.id)
+    dao_deactive_service(service.id)
 
     return '', 204
 
diff --git a/tests/app/service/test_deactivate.py b/tests/app/service/test_deactivate.py
index d81de0e10..df39fecac 100644
--- a/tests/app/service/test_deactivate.py
+++ b/tests/app/service/test_deactivate.py
@@ -57,7 +57,7 @@ def test_deactivating_service_revokes_api_keys(deactivated_service):
 
 
 def test_deactivating_service_archives_templates(deactivated_service):
-    assert deactivated_service.templates.count() == 2
+    assert len(deactivated_service.templates) == 2
     for template in deactivated_service.templates:
         assert template.archived is True
         assert template.version == 2

From 195f3615e648fa30bd5b0d2253227f266bdb9133 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Fri, 11 Nov 2016 12:43:51 +0000
Subject: [PATCH 28/29] add test that if we have an exception, nothing is
 committed

---
 app/dao/services_dao.py              |  6 +-----
 app/models.py                        |  2 +-
 tests/__init__.py                    |  9 +++++++++
 tests/app/service/test_deactivate.py | 22 ++++++++++++++++++----
 4 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py
index 604f0dd25..bead23a6e 100644
--- a/app/dao/services_dao.py
+++ b/app/dao/services_dao.py
@@ -75,6 +75,7 @@ def dao_fetch_all_services_by_user(user_id, only_active=False):
 @version_class(ApiKey)
 def dao_deactive_service(service_id):
     # have to eager load templates and api keys so that we don't flush when we loop through them
+    # to ensure that db.session still contains the models when it comes to creating history objects
     service = Service.query.options(
         joinedload('templates'),
         joinedload('api_keys'),
@@ -84,17 +85,12 @@ def dao_deactive_service(service_id):
     service.name = '_archived_' + service.name
     service.email_from = '_archived_' + service.email_from
 
-
     for template in service.templates:
         template.archived = True
 
     for api_key in service.api_keys:
         api_key.expiry_date = datetime.utcnow()
 
-    db.session.add(service)
-    db.session.add_all(service.templates)
-    db.session.add_all(service.api_keys)
-
 
 def dao_fetch_service_by_id_and_user(service_id, user_id):
     return Service.query.filter(
diff --git a/app/models.py b/app/models.py
index 2f2c6163d..699ae2275 100644
--- a/app/models.py
+++ b/app/models.py
@@ -5,7 +5,7 @@ from sqlalchemy.dialects.postgresql import (
     UUID,
     JSON
 )
-from sqlalchemy import UniqueConstraint, and_, false, true
+from sqlalchemy import UniqueConstraint, and_
 from sqlalchemy.orm import foreign, remote
 from notifications_utils.recipients import (
     validate_email_address,
diff --git a/tests/__init__.py b/tests/__init__.py
index 526e0efc1..a40d209cf 100644
--- a/tests/__init__.py
+++ b/tests/__init__.py
@@ -30,3 +30,12 @@ def create_authorization_header(service_id=None, key_type=KEY_TYPE_NORMAL):
 
     token = create_jwt_token(secret=secret, client_id=client_id)
     return 'Authorization', 'Bearer {}'.format(token)
+
+
+def unwrap_function(fn):
+    """
+    Given a function, returns its undecorated original.
+    """
+    while hasattr(fn, '__wrapped__'):
+        fn = fn.__wrapped__
+    return fn
diff --git a/tests/app/service/test_deactivate.py b/tests/app/service/test_deactivate.py
index df39fecac..0f9a3dcce 100644
--- a/tests/app/service/test_deactivate.py
+++ b/tests/app/service/test_deactivate.py
@@ -1,22 +1,26 @@
 import uuid
+from unittest import mock
 
 import pytest
 
-from app.models import Service
-from tests import create_authorization_header
+from app import db
+from app.models import Service, TemplateHistory, ApiKey
+from app.dao.services_dao import dao_deactive_service
+
+from tests import create_authorization_header, unwrap_function
 from tests.app.conftest import (
     sample_template as create_template,
     sample_api_key as create_api_key
 )
 
 
-def test_deactivate_only_allows_post(client, sample_service):
+def test_deactivate_only_allows_post(client):
     auth_header = create_authorization_header()
     response = client.get('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header])
     assert response.status_code == 405
 
 
-def test_deactivate_service_errors_with_bad_service_id(client, sample_service):
+def test_deactivate_service_errors_with_bad_service_id(client):
     auth_header = create_authorization_header()
     response = client.post('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header])
     assert response.status_code == 404
@@ -73,3 +77,13 @@ def test_deactivating_service_creates_history(deactivated_service):
 
     assert history.version == 2
     assert history.active is False
+
+
+def test_deactivating_service_rolls_back_everything_on_error(sample_service, sample_api_key, sample_template):
+    unwrapped_deactive_service = unwrap_function(dao_deactive_service)
+
+    unwrapped_deactive_service(sample_service.id)
+
+    assert sample_service in db.session.dirty
+    assert sample_api_key in db.session.dirty
+    assert sample_template in db.session.dirty

From 35e1e06db4fb987eb566ff7d0fdbc11e71fe5221 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 11 Nov 2016 16:30:05 +0000
Subject: [PATCH 29/29] Fix conflicting migration versions

Two different versions of 0059 were merged.

`0059_set_services_to_active` was merged first, so this renames my
migration to `0060`, so it goes in afterwards.
---
 ...er_template_type.py => 0060_add_letter_template_type.py} | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
 rename migrations/versions/{0059_add_letter_template_type.py => 0060_add_letter_template_type.py} (90%)

diff --git a/migrations/versions/0059_add_letter_template_type.py b/migrations/versions/0060_add_letter_template_type.py
similarity index 90%
rename from migrations/versions/0059_add_letter_template_type.py
rename to migrations/versions/0060_add_letter_template_type.py
index 2b4cb6eea..cf5057de4 100644
--- a/migrations/versions/0059_add_letter_template_type.py
+++ b/migrations/versions/0060_add_letter_template_type.py
@@ -1,7 +1,7 @@
 """empty message
 
 Revision ID: f266fb67597a
-Revises: 0058_add_letters_flag
+Revises: 0059_set_services_to_active
 Create Date: 2016-11-07 16:13:18.961527
 
 """
@@ -10,8 +10,8 @@ from alembic import op
 import sqlalchemy as sa
 
 # revision identifiers, used by Alembic.
-revision = '0059_add_letter_template_type'
-down_revision = '0058_add_letters_flag'
+revision = '0060_add_letter_template_type'
+down_revision = '0059_set_services_to_active'
 
 
 name = 'template_type'