From f71dbe9c0ff28f8513e2e50e38ffaa882511339d Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Fri, 29 Apr 2016 10:36:59 +0100 Subject: [PATCH] Message limit added and all tests passing. --- app/celery/tasks.py | 5 +-- app/dao/notifications_dao.py | 14 ++---- app/notifications/rest.py | 6 +++ app/template/rest.py | 30 ++++++++++--- config.py | 1 + requirements.txt | 2 +- tests/app/dao/test_notification_dao.py | 18 ++------ tests/app/notifications/test_rest.py | 35 ++++++++++++++- tests/app/template/test_rest.py | 60 +++++++++++++++++++++++++- 9 files changed, 135 insertions(+), 36 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index f8b274145..7c07b8715 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -38,8 +38,7 @@ from app.dao.notifications_dao import ( dao_update_notification, delete_notifications_created_more_than_a_week_ago, dao_get_notification_statistics_for_service_and_day, - update_notification_reference_by_id, - get_character_count_of_content + update_notification_reference_by_id ) from app.dao.jobs_dao import ( @@ -233,7 +232,7 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): created_at=datetime.strptime(created_at, DATETIME_FORMAT), sent_at=sent_at, sent_by=client.get_name(), - content_char_count=get_character_count_of_content(template.replaced) + content_char_count=template.replaced_content_count ) dao_create_notification(notification_db_object, TEMPLATE_TYPE_SMS, client.get_name()) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 5959c26cd..b7a22fe74 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -22,6 +22,8 @@ from app.models import ( ProviderStatistics ) +from notifications_utils.template import get_sms_fragment_count + from app.clients import ( STATISTICS_FAILURE, STATISTICS_DELIVERED, @@ -31,14 +33,6 @@ from app.clients import ( from app.dao.dao_utils import transactional -def get_character_count_of_content(content, encoding='utf-8'): - return len(content.encode(encoding)) - - -def get_sms_message_count(char_count): - return 1 if char_count <= 160 else math.ceil(float(char_count) / 153) - - def dao_get_notification_statistics_for_service(service_id, limit_days=None): filter = [NotificationStatistics.service_id == service_id] if limit_days is not None: @@ -105,14 +99,14 @@ def dao_create_notification(notification, notification_type, provider): service_id=notification.service_id, provider=provider ).update({'unit_count': ProviderStatistics.unit_count + ( - 1 if notification_type == TEMPLATE_TYPE_EMAIL else get_sms_message_count(notification.content_char_count))}) + 1 if notification_type == TEMPLATE_TYPE_EMAIL else get_sms_fragment_count(notification.content_char_count))}) if update_count == 0: provider_stats = ProviderStatistics( day=notification.created_at.date(), service_id=notification.service_id, provider=provider, - unit_count=1 if notification_type == TEMPLATE_TYPE_EMAIL else get_sms_message_count( + unit_count=1 if notification_type == TEMPLATE_TYPE_EMAIL else get_sms_fragment_count( notification.content_char_count)) db.session.add(provider_stats) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 4bfd85db2..4749203c9 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -338,6 +338,12 @@ def send_notification(notification_type): } ), 400 + if template_object.replaced_content_count > current_app.config.get('SMS_CHAR_COUNT_LIMIT'): + return jsonify( + result="error", + message={'content': ['Content has a character count greater than the limit of {}'.format( + current_app.config.get('SMS_CHAR_COUNT_LIMIT'))]}), 400 + if service.restricted and not allowed_to_send_to( notification['to'], itertools.chain.from_iterable( diff --git a/app/template/rest.py b/app/template/rest.py index 26fd09bcf..b04513356 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -13,9 +13,8 @@ from app.dao.templates_dao import ( dao_get_template_by_id_and_service_id, dao_get_all_templates_for_service ) -from app.dao.services_dao import ( - dao_fetch_service_by_id -) +from notifications_utils.template import Template +from app.dao.services_dao import dao_fetch_service_by_id from app.schemas import template_schema template = Blueprint('template', __name__, url_prefix='/service//template') @@ -25,6 +24,18 @@ from app.errors import register_errors register_errors(template) +def _content_count_greater_than_limit(content, template_type): + template = Template({'content': content, 'template_type': template_type}) + if template_type == 'sms' and \ + template.content_count > current_app.config.get('SMS_CHAR_COUNT_LIMIT'): + return True, jsonify( + result="error", + message={'content': ['Content has a character count greater than the limit of {}'.format( + current_app.config.get('SMS_CHAR_COUNT_LIMIT'))]} + ) + return False, '' + + @template.route('', methods=['POST']) def create_template(service_id): fetched_service = dao_fetch_service_by_id(service_id=service_id) @@ -33,6 +44,11 @@ def create_template(service_id): return jsonify(result="error", message=errors), 400 new_template.service = fetched_service new_template.content = _strip_html(new_template.content) + over_limit, json_resp = _content_count_greater_than_limit( + new_template.content, + new_template.template_type) + if over_limit: + return json_resp, 400 try: dao_create_template(new_template) except IntegrityError as ex: @@ -40,7 +56,7 @@ def create_template(service_id): message = "Failed to create template" if "templates_subject_key" in str(ex): message = 'Duplicate template subject' - return jsonify(result="error", message=[{'subject': message}]), 400 + return jsonify(result="error", message={'subject': [message]}), 400 return jsonify(result="error", message=message), 500 return jsonify(data=template_schema.dump(new_template).data), 201 @@ -57,7 +73,11 @@ def update_template(service_id, template_id): update_dict, errors = template_schema.load(current_data) if errors: return jsonify(result="error", message=errors), 400 - + over_limit, json_resp = _content_count_greater_than_limit( + current_data['content'], + fetched_template.template_type) + if over_limit: + return json_resp, 400 dao_update_template(update_dict) return jsonify(data=template_schema.dump(update_dict).data), 200 diff --git a/config.py b/config.py index a93592aee..13ac1130e 100644 --- a/config.py +++ b/config.py @@ -26,6 +26,7 @@ class Config(object): VERIFY_CODE_FROM_EMAIL_ADDRESS = os.environ['VERIFY_CODE_FROM_EMAIL_ADDRESS'] NOTIFY_EMAIL_DOMAIN = os.environ['NOTIFY_EMAIL_DOMAIN'] PAGE_SIZE = 50 + SMS_CHAR_COUNT_LIMIT = 495 BROKER_URL = 'sqs://' BROKER_TRANSPORT_OPTIONS = { diff --git a/requirements.txt b/requirements.txt index 9d26ec931..c38e4f8b2 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,4 +22,4 @@ monotonic==0.3 git+https://github.com/alphagov/notifications-python-client.git@0.5.0#egg=notifications-python-client==0.5.0 -git+https://github.com/alphagov/notifications-utils.git@4.2.0#egg=notifications-utils==4.2.0 +git+https://github.com/alphagov/notifications-utils.git@5.1.0#egg=notifications-utils==5.1.0 diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index b4e8db8ce..083947d1e 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -28,11 +28,11 @@ from app.dao.notifications_dao import ( update_notification_reference_by_id, update_notification_status_by_reference, dao_get_template_statistics_for_service, - get_character_count_of_content, - get_sms_message_count, get_notifications_for_service ) +from notifications_utils.template import get_sms_fragment_count + from tests.app.conftest import sample_job from tests.app.conftest import sample_notification @@ -1089,18 +1089,6 @@ def test_should_limit_notifications_return_by_day_limit_plus_one(notify_db, noti assert len(all_notifications) == 2 -@pytest.mark.parametrize( - "content,encoding,expected_length", - [ - ("The quick brown fox jumped over the lazy dog", "utf-8", 44), - ("深", "utf-8", 3), - ("'First line.\n", 'utf-8', 13), - ("\t\n\r", 'utf-8', 3) - ]) -def test_get_character_count_of_content(content, encoding, expected_length): - assert get_character_count_of_content(content, encoding=encoding) == expected_length - - @pytest.mark.parametrize( "char_count, expected_sms_fragment_count", [ @@ -1114,4 +1102,4 @@ def test_get_character_count_of_content(content, encoding, expected_length): (461, 4) ]) def test_sms_fragment_count(char_count, expected_sms_fragment_count): - assert get_sms_message_count(char_count) == expected_sms_fragment_count + assert get_sms_fragment_count(char_count) == expected_sms_fragment_count diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index b748fa622..25f257d90 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -1,5 +1,7 @@ from datetime import datetime import uuid +import random +import string import app.celery.tasks from tests import create_authorization_header from tests.app.conftest import sample_notification as create_sample_notification @@ -7,7 +9,7 @@ from tests.app.conftest import sample_job as create_sample_job from tests.app.conftest import sample_service as create_sample_service from tests.app.conftest import sample_email_template as create_sample_email_template from tests.app.conftest import sample_template as create_sample_template -from flask import json +from flask import (json, current_app, url_for) from app.models import Service from app.dao.templates_dao import dao_get_all_templates_for_service from app.dao.services_dao import dao_update_service @@ -764,6 +766,37 @@ def test_should_not_allow_template_from_another_service(notify_api, service_fact assert test_string in json_resp['message'] +def test_should_not_allow_template_content_too_large(notify_api, notify_db, notify_db_session, sample_user): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + template = create_sample_template(notify_db, notify_db_session, content="((long_text))") + limit = current_app.config.get('SMS_CHAR_COUNT_LIMIT') + json_data = json.dumps({ + 'to': sample_user.mobile_number, + 'template': template.id, + 'personalisation': { + 'long_text': ''.join( + random.choice(string.ascii_uppercase + string.digits) for _ in range(limit + 1)) + } + }) + endpoint = url_for('notifications.send_notification', notification_type='sms') + auth_header = create_authorization_header( + service_id=template.service.id, + request_body=json_data, + path=endpoint, + method='POST') + + resp = client.post( + path=endpoint, + data=json_data, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 400 + json_resp = json.loads(resp.get_data(as_text=True)) + assert json_resp['message']['content'][0] == ( + 'Content has a character count greater' + ' than the limit of {}').format(limit) + + @freeze_time("2016-01-01 11:09:00.061258") def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker): with notify_api.test_request_context(): diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 5e43f1775..04dfb6709 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -1,5 +1,7 @@ import json import uuid +import random +import string from app.models import Template from tests import create_authorization_header @@ -224,7 +226,7 @@ def test_must_have_a_uniqe_subject_on_an_email_template(notify_api, sample_user, assert response.status_code == 400 json_resp = json.loads(response.get_data(as_text=True)) assert json_resp['result'] == 'error' - assert json_resp['message'][0]['subject'] == 'Duplicate template subject' + assert json_resp['message']['subject'][0] == 'Duplicate template subject' def test_should_be_able_to_update_a_template(notify_api, sample_user, sample_service): @@ -479,3 +481,59 @@ def test_should_return_404_if_no_templates_for_service_with_id(notify_api, sampl json_resp = json.loads(response.get_data(as_text=True)) assert json_resp['result'] == 'error' assert json_resp['message'] == 'No result found' + + +def test_create_400_for_over_limit_content(notify_api, sample_user, sample_service, fake_uuid): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + limit = notify_api.config.get('SMS_CHAR_COUNT_LIMIT') + content = ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(limit + 1)) + data = { + 'name': 'too big template', + 'template_type': 'sms', + 'content': content, + 'service': str(sample_service.id), + 'created_by': str(sample_user.id) + } + data = json.dumps(data) + auth_header = create_authorization_header( + path='/service/{}/template'.format(sample_service.id), + method='POST', + request_body=data + ) + + response = client.post( + '/service/{}/template'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + assert response.status_code == 400 + json_resp = json.loads(response.get_data(as_text=True)) + assert ( + 'Content has a character count greater than the limit of {}' + ).format(limit) in json_resp['message']['content'] + + +def test_update_400_for_over_limit_content(notify_api, sample_user, sample_template): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + limit = notify_api.config.get('SMS_CHAR_COUNT_LIMIT') + json_data = json.dumps({ + 'content': ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(limit + 1)), + 'created_by': str(sample_user.id) + }) + auth_header = create_authorization_header( + path='/service/{}/template/{}'.format(sample_template.service.id, sample_template.id), + method='POST', + request_body=json_data + ) + resp = client.post( + '/service/{}/template/{}'.format(sample_template.service.id, sample_template.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=json_data + ) + assert resp.status_code == 400 + json_resp = json.loads(resp.get_data(as_text=True)) + assert ( + 'Content has a character count greater than the limit of {}' + ).format(limit) in json_resp['message']['content']