Merge pull request #986 from alphagov/create-service-bug

separate service deserialization from validation

app/models.py

@@ -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'])
+    # validate json with marshmallow
-    data.pop('user_id', None)
+    service_schema.load(request.get_json())
-    valid_service = service_schema.load(request.get_json()).data
+
+    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(
@@ -1731,3 +1735,19 @@ def test_update_service_does_not_call_send_notification_when_restricted_not_chan
 
     assert resp.status_code == 200
     assert not send_notification_mock.called
+
+
+def test_update_service_works_when_sms_sender_is_null(sample_service, client, mocker):
+    sample_service.sms_sender = None
+    data = {'name': 'new name'}
+
+    resp = client.post(
+        'service/{}'.format(sample_service.id),
+        data=json.dumps(data),
+        headers=[create_authorization_header()],
+        content_type='application/json'
+    )
+
+    assert resp.status_code == 200
+    # make sure it wasn't changed to not-null under the hood
+    assert sample_service.sms_sender is None