From 39198ed67edb5e37192e429baa9f88c457b9af5e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 2 Nov 2018 16:00:22 +0000 Subject: [PATCH] Using jsonschema for create_template. Updated jsonschema to Draft7, this allowed a conditional validation on subject, if template_type == 'email' or 'letter' then subject is required. This version is backward compatible with Draft4. When creating TempalteRedacted, I've built the dict depending on if the created_by or created_by_id exists. --- app/dao/templates_dao.py | 15 ++++++++++----- app/models.py | 14 ++++++++++++++ app/schema_validation/__init__.py | 4 ++-- app/template/rest.py | 9 ++++++--- app/template/template_schemas.py | 28 ++++++++++++++++++++++++++++ requirements-app.txt | 2 +- requirements.txt | 4 +++- tests/app/template/test_rest.py | 10 +++++----- 8 files changed, 69 insertions(+), 17 deletions(-) create mode 100644 app/template/template_schemas.py diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index 3d3dd9bc1..2a9164429 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -21,11 +21,16 @@ def dao_create_template(template): template.id = uuid.uuid4() # must be set now so version history model can use same id template.archived = False - template.template_redacted = TemplateRedacted( - template=template, - redact_personalisation=False, - updated_by=template.created_by - ) + redacted_dict = { + "template": template, + "redact_personalisation": False, + } + if template.created_by: + redacted_dict.update({"updated_by": template.created_by}) + else: + redacted_dict.update({"updated_by_id": template.created_by_id}) + + template.template_redacted = TemplateRedacted(**redacted_dict) db.session.add(template) diff --git a/app/models.py b/app/models.py index 3f2877dff..bf548bd32 100644 --- a/app/models.py +++ b/app/models.py @@ -884,6 +884,20 @@ class Template(TemplateBase): _external=True ) + @classmethod + def from_json(cls, data): + """ + Assumption: data has been validated appropriately. + + Returns a Template object based on the provided data. + """ + fields = data.copy() + + fields['created_by_id'] = fields.pop('created_by') + fields['service_id'] = fields.pop('service') + + return cls(**fields) + class TemplateRedacted(db.Model): __tablename__ = 'template_redacted' diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 44cb0f9ce..382d9229c 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -3,7 +3,7 @@ from datetime import datetime, timedelta from uuid import UUID from iso8601 import iso8601, ParseError -from jsonschema import (Draft4Validator, ValidationError, FormatChecker) +from jsonschema import (Draft7Validator, ValidationError, FormatChecker) from notifications_utils.recipients import (validate_phone_number, validate_email_address, InvalidPhoneError, InvalidEmailError) @@ -43,7 +43,7 @@ def validate(json_to_validate, schema): "https://en.wikipedia.org/wiki/ISO_8601") return True - validator = Draft4Validator(schema, format_checker=format_checker) + validator = Draft7Validator(schema, format_checker=format_checker) errors = list(validator.iter_errors(json_to_validate)) if errors.__len__() > 0: raise ValidationError(build_error_message(errors)) diff --git a/app/template/rest.py b/app/template/rest.py index 64a353c0a..52a3497fb 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -29,9 +29,11 @@ from app.errors import ( InvalidRequest ) from app.letters.utils import get_letter_pdf -from app.models import SMS_TYPE +from app.models import SMS_TYPE, Template from app.notifications.validators import service_has_permission, check_reply_to +from app.schema_validation import validate from app.schemas import (template_schema, template_history_schema) +from app.template.template_schemas import post_create_template_schema from app.utils import get_template_instance, get_public_notify_type_text template_blueprint = Blueprint('template', __name__, url_prefix='/service//template') @@ -49,9 +51,9 @@ def _content_count_greater_than_limit(content, template_type): @template_blueprint.route('', methods=['POST']) def create_template(service_id): fetched_service = dao_fetch_service_by_id(service_id=service_id) - # permissions needs to be placed here otherwise marshmallow will intefere with versioning + # permissions needs to be placed here otherwise marshmallow will interfere with versioning permissions = fetched_service.permissions - new_template = template_schema.load(request.get_json()).data + new_template = Template.from_json(validate(request.get_json(), post_create_template_schema)) if not service_has_permission(new_template.template_type, permissions): message = "Creating {} templates is not allowed".format( @@ -60,6 +62,7 @@ def create_template(service_id): raise InvalidRequest(errors, 403) new_template.service = fetched_service + over_limit = _content_count_greater_than_limit(new_template.content, new_template.template_type) if over_limit: message = 'Content has a character count greater than the limit of {}'.format(SMS_CHAR_COUNT_LIMIT) diff --git a/app/template/template_schemas.py b/app/template/template_schemas.py new file mode 100644 index 000000000..4f13c8ced --- /dev/null +++ b/app/template/template_schemas.py @@ -0,0 +1,28 @@ +from app.models import ( + TEMPLATE_PROCESS_TYPE, + TEMPLATE_TYPES, +) +from app.schema_validation.definitions import uuid + +post_create_template_schema = { + "$schema": "http://json-schema.org/draft-07/schema#", + "description": "POST create new template", + "type": "object", + "title": "payload for POST /service//template", + "properties": { + "name": {"type": "string"}, + "template_type": {"enum": TEMPLATE_TYPES}, + "service": uuid, + "process_type": {"emun": TEMPLATE_PROCESS_TYPE}, + "content": {"type": "string"}, + "subject": {"type": "string"}, + "created_by": uuid + }, + "if": { + "properties": { + "template_type": {"enum": ["email", "letter"]} + } + }, + "then": {"required": ["subject"]}, + "required": ["name", "template_type", "content", "service", "created_by"] +} diff --git a/requirements-app.txt b/requirements-app.txt index e75740910..99535815d 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -13,7 +13,7 @@ click-datetime==0.2 eventlet==0.23.0 gunicorn==19.7.1 iso8601==0.1.12 -jsonschema==2.6.0 +jsonschema==3.0.0a3 marshmallow-sqlalchemy==0.14.1 marshmallow==2.16.0 psycopg2-binary==2.7.5 diff --git a/requirements.txt b/requirements.txt index 37613bb91..c68365b0a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -15,7 +15,7 @@ click-datetime==0.2 eventlet==0.23.0 gunicorn==19.7.1 iso8601==0.1.12 -jsonschema==2.6.0 +jsonschema==3.0.0a3 marshmallow-sqlalchemy==0.14.1 marshmallow==2.16.0 psycopg2-binary==2.7.5 @@ -39,6 +39,7 @@ git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 alembic==1.0.2 amqp==1.4.9 anyjson==0.3.3 +attrs==18.2.0 awscli==1.16.49 bcrypt==3.1.4 billiard==3.3.0.23 @@ -67,6 +68,7 @@ phonenumbers==8.9.4 pyasn1==0.4.4 pycparser==2.19 PyPDF2==1.26.0 +pyrsistent==0.14.5 python-dateutil==2.7.5 python-editor==1.0.3 python-json-logger==0.1.8 diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 50c5d7f37..55e265f8b 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -172,7 +172,8 @@ def test_should_error_if_created_by_missing(client, sample_user, sample_service) ) json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 400 - assert json_resp['result'] == 'error' + assert json_resp["errors"][0]["error"] == 'ValidationError' + assert json_resp["errors"][0]["message"] == 'created_by is a required property' def test_should_be_error_if_service_does_not_exist_on_update(client, fake_uuid): @@ -211,15 +212,14 @@ def test_must_have_a_subject_on_an_email_or_letter_template(client, sample_user, data=data ) json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert json_resp['message'] == {'subject': ['Invalid template subject']} + assert json_resp['errors'][0]['error'] == "ValidationError" + assert json_resp['errors'][0]["message"] == 'subject is a required property' def test_update_should_update_a_template(client, sample_user, sample_template): data = { 'content': 'my template has new content ', - 'created_by': str(sample_user.id) + 'created_by_id': str(sample_user.id) } data = json.dumps(data) auth_header = create_authorization_header()