separate service deserialization from validation

Marshmallow validates and deserialises - BUT, when it deserialises,
it explicitly sets `sms_sender=None`, even when you haven't passed
sms_sender in. This is problematic, because we wanted to take advantage
of sqlalchemy's default value to set sms_sender to `GOVUK` when the
actual DB commit happens.

Instead, still use marshmallow for validating, but manually carry out
the json deserialisation in the model class.

This fixes a bug that only manifested when the database was upgraded,
but the code hadn't updated. 🎉

app/models.py

@@ -188,7 +188,7 @@ class Service(db.Model, Versioned):
     created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False)
     reply_to_email_address = db.Column(db.Text, index=False, unique=False, nullable=True)
     letter_contact_block = db.Column(db.Text, index=False, unique=False, nullable=True)
-    sms_sender = db.Column(db.String(11), nullable=True, default=lambda: current_app.config['FROM_NUMBER'])
+    sms_sender = db.Column(db.String(11), nullable=False, default=lambda: current_app.config['FROM_NUMBER'])
     organisation_id = db.Column(UUID(as_uuid=True), db.ForeignKey('organisation.id'), index=True, nullable=True)
     organisation = db.relationship('Organisation')
     dvla_organisation_id = db.Column(
@@ -215,6 +215,21 @@ class Service(db.Model, Versioned):
             self.can_send_letters = LETTER_TYPE in [p.permission for p in self.permissions]
             self.can_send_international_sms = INTERNATIONAL_SMS_TYPE in [p.permission for p in self.permissions]
 
+    @classmethod
+    def from_json(cls, data):
+        """
+        Assumption: data has been validated appropriately.
+
+        Returns a Service object based on the provided data. Deserialises created_by to created_by_id as marshmallow
+        would.
+        """
+        # validate json with marshmallow
+        fields = data.copy()
+
+        fields['created_by_id'] = fields.pop('created_by')
+
+        return cls(**fields)
+
 
 class ServicePermission(db.Model):
     __tablename__ = "service_permissions"

app/service/rest.py

@@ -5,11 +5,12 @@ from datetime import datetime
 from flask import (
     jsonify,
     request,
-    current_app
+    current_app,
+    Blueprint
 )
 from sqlalchemy.orm.exc import NoResultFound
 
-from app.dao import notification_usage_dao
+from app.dao import notification_usage_dao, notifications_dao
 from app.dao.dao_utils import dao_rollback
 from app.dao.api_key_dao import (
     save_model_api_key,
@@ -39,11 +40,13 @@ from app.dao.service_whitelist_dao import (
     dao_add_and_commit_whitelisted_contacts,
     dao_remove_service_whitelist
 )
-from app.dao import notifications_dao
 from app.dao.provider_statistics_dao import get_fragment_count
 from app.dao.users_dao import get_user_by_id
 from app.errors import (
-    InvalidRequest, register_errors)
+    InvalidRequest,
+    register_errors
+)
+from app.models import Service
 from app.service import statistics
 from app.service.utils import get_whitelist_objects
 from app.service.sender import send_notification_to_service_users
@@ -57,7 +60,6 @@ from app.schemas import (
     detailed_service_schema
 )
 from app.utils import pagination_links
-from flask import Blueprint
 
 service_blueprint = Blueprint('service', __name__)
 
@@ -108,9 +110,14 @@ def create_service():
         errors = {'user_id': ['Missing data for required field.']}
         raise InvalidRequest(errors, status_code=400)
 
-    user = get_user_by_id(data['user_id'])
-    data.pop('user_id', None)
-    valid_service = service_schema.load(request.get_json()).data
+    # validate json with marshmallow
+    service_schema.load(request.get_json())
+
+    user = get_user_by_id(data.pop('user_id', None))
+
+    # unpack valid json into service object
+    valid_service = Service.from_json(data)
+
     dao_create_service(valid_service, user)
     return jsonify(data=service_schema.dump(valid_service).data), 201
 

tests/app/service/test_rest.py

@@ -20,7 +20,7 @@ from tests.app.conftest import (
     sample_notification_history as create_notification_history,
     sample_notification_with_job
 )
-from app.models import KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST
+from app.models import Service, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST
 
 from tests.app.db import create_user
 
@@ -216,6 +216,10 @@ def test_create_service(client, sample_user):
     assert json_resp['data']['dvla_organisation'] == '001'
     assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER']
 
+    service_db = Service.query.get(json_resp['data']['id'])
+    assert service_db.name == 'created service'
+    assert service_db.sms_sender == current_app.config['FROM_NUMBER']
+
     auth_header_fetch = create_authorization_header()
 
     resp = client.get(