diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index f27190ad3..c78582851 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -15,7 +15,7 @@ from notifications_utils.recipients import ( ) from notifications_utils.statsd_decorators import statsd from notifications_utils.timezones import convert_utc_to_bst -from sqlalchemy import (desc, func, or_, asc) +from sqlalchemy import (desc, func, asc) from sqlalchemy.orm import joinedload from sqlalchemy.sql import functions from sqlalchemy.sql.expression import case @@ -144,17 +144,23 @@ def _update_notification_status(notification, status): @statsd(namespace="dao") @transactional def update_notification_status_by_id(notification_id, status, sent_by=None): - notification = Notification.query.with_for_update().filter( - Notification.id == notification_id, - or_( - Notification.status == NOTIFICATION_CREATED, - Notification.status == NOTIFICATION_SENDING, - Notification.status == NOTIFICATION_PENDING, - Notification.status == NOTIFICATION_SENT, - Notification.status == NOTIFICATION_PENDING_VIRUS_CHECK - )).first() + notification = Notification.query.with_for_update().filter(Notification.id == notification_id).first() if not notification: + current_app.logger.error('notification not found for id {} (update to status {})'.format( + notification_id, + status + )) + return None + + if notification.status not in { + NOTIFICATION_CREATED, + NOTIFICATION_SENDING, + NOTIFICATION_PENDING, + NOTIFICATION_SENT, + NOTIFICATION_PENDING_VIRUS_CHECK + }: + _duplicate_update_warning(notification, status) return None if notification.international and not country_records_delivery(notification.phone_prefix): @@ -170,15 +176,18 @@ def update_notification_status_by_id(notification_id, status, sent_by=None): @statsd(namespace="dao") @transactional def update_notification_status_by_reference(reference, status): - notification = Notification.query.filter( - Notification.reference == reference, - or_( - Notification.status == NOTIFICATION_SENDING, - Notification.status == NOTIFICATION_PENDING, - Notification.status == NOTIFICATION_SENT - )).first() + # this is used to update letters and emails + notification = Notification.query.filter(Notification.reference == reference).first() - if not notification or notification.status == NOTIFICATION_SENT: + if not notification: + current_app.logger.error('notification not found for reference {} (update to {})'.format(reference, status)) + return None + + if notification.status not in { + NOTIFICATION_SENDING, + NOTIFICATION_PENDING + }: + _duplicate_update_warning(notification, status) return None return _update_notification_status( @@ -693,3 +702,19 @@ def guess_notification_type(search_term): return EMAIL_TYPE else: return SMS_TYPE + + +def _duplicate_update_warning(notification, status): + current_app.logger.info( + ( + 'Duplicate callback received. Notification id {id} received a status update to {new_status}' + '{time_diff} after being set to {old_status}. {type} sent by {sent_by}' + ).format( + id=notification.id, + old_status=notification.status, + new_status=status, + time_diff=datetime.utcnow() - (notification.updated_at or notification.created_at), + type=notification.notification_type, + sent_by=notification.sent_by + ) + ) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index e90cfeced..d75abcd4e 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -61,9 +61,6 @@ def process_ses_response(ses_request): notification_status ) if not notification: - warning = "SES callback failed: notification either not found or already updated " \ - "from sending. Status {} for notification reference {}".format(notification_status, reference) - current_app.logger.warning(warning) return if not aws_response_dict['success']: diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py index d45c3cc92..3de60c587 100644 --- a/app/notifications/process_client_response.py +++ b/app/notifications/process_client_response.py @@ -81,10 +81,6 @@ def _process_for_status(notification_status, client_name, provider_reference): sent_by=client_name.lower() ) if not notification: - current_app.logger.warning("{} callback failed: notification {} either not found or already updated " - "from sending. Status {}".format(client_name, - provider_reference, - notification_status)) return statsd_client.incr('callback.{}.{}'.format(client_name.lower(), notification_status)) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 6483ac766..8fc2f15f6 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -23,7 +23,8 @@ from app.models import ( LETTER_TYPE, NOTIFICATION_CREATED, Notification, - ScheduledNotification + ScheduledNotification, + CHOOSE_POSTAGE ) from app.dao.notifications_dao import ( dao_create_notification, @@ -31,6 +32,8 @@ from app.dao.notifications_dao import ( dao_created_scheduled_notification ) +from app.dao.templates_dao import dao_get_template_by_id + from app.v2.errors import BadRequestError from app.utils import ( cache_key_for_service_template_counter, @@ -109,7 +112,11 @@ def persist_notification( elif notification_type == EMAIL_TYPE: notification.normalised_to = format_email_address(notification.to) elif notification_type == LETTER_TYPE: - notification.postage = service.postage + template = dao_get_template_by_id(template_id, template_version) + if service.has_permission(CHOOSE_POSTAGE) and template.postage: + notification.postage = template.postage + else: + notification.postage = service.postage # if simulated create a Notification model to return but do not persist the Notification to the dB if not simulated: diff --git a/app/template/rest.py b/app/template/rest.py index 8cbb4125f..79b1d7d8b 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -31,7 +31,7 @@ from app.errors import ( InvalidRequest ) from app.letters.utils import get_letter_pdf -from app.models import SMS_TYPE, Template +from app.models import SMS_TYPE, Template, CHOOSE_POSTAGE 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) @@ -78,6 +78,12 @@ def create_template(service_id): errors = {'template_type': [message]} raise InvalidRequest(errors, 403) + if new_template.postage: + if not service_has_permission(CHOOSE_POSTAGE, fetched_service.permissions): + message = "Setting postage on templates is not enabled for this service." + errors = {'template_postage': [message]} + raise InvalidRequest(errors, 403) + new_template.service = fetched_service over_limit = _content_count_greater_than_limit(new_template.content, new_template.template_type) @@ -110,6 +116,12 @@ def update_template(service_id, template_id): if data.get('redact_personalisation') is True: return redact_template(fetched_template, data) + if data.get('postage'): + if not service_has_permission(CHOOSE_POSTAGE, fetched_template.service.permissions): + message = "Setting postage on templates is not enabled for this service." + errors = {'template_postage': [message]} + raise InvalidRequest(errors, 403) + if "reply_to" in data: check_reply_to(service_id, data.get("reply_to"), fetched_template.template_type) updated = dao_update_template_reply_to(template_id=template_id, reply_to=data.get("reply_to")) @@ -191,7 +203,7 @@ def get_template_versions(service_id, template_id): def _template_has_not_changed(current_data, updated_template): return all( current_data[key] == updated_template[key] - for key in ('name', 'content', 'subject', 'archived', 'process_type') + for key in ('name', 'content', 'subject', 'archived', 'process_type', 'postage') ) diff --git a/app/template/template_schemas.py b/app/template/template_schemas.py index f63ad4575..9f38262a5 100644 --- a/app/template/template_schemas.py +++ b/app/template/template_schemas.py @@ -17,7 +17,8 @@ post_create_template_schema = { "content": {"type": "string"}, "subject": {"type": "string"}, "created_by": uuid, - "parent_folder_id": uuid + "parent_folder_id": uuid, + "postage": {"type": "string"}, }, "if": { "properties": { diff --git a/app/v2/template/template_schemas.py b/app/v2/template/template_schemas.py index b1b1f4820..ebd0b8342 100644 --- a/app/v2/template/template_schemas.py +++ b/app/v2/template/template_schemas.py @@ -37,6 +37,7 @@ get_template_by_id_response = { "body": {"type": "string"}, "subject": {"type": ["string", "null"]}, "name": {"type": "string"}, + "postage": {"type": "string"} }, "required": ["id", "type", "created_at", "updated_at", "version", "created_by", "body", "name"], } @@ -63,7 +64,8 @@ post_template_preview_response = { "type": {"enum": TEMPLATE_TYPES}, "version": {"type": "integer"}, "body": {"type": "string"}, - "subject": {"type": ["string", "null"]} + "subject": {"type": ["string", "null"]}, + "postage": {"type": "string"} }, "required": ["id", "type", "version", "body"] } @@ -77,5 +79,6 @@ def create_post_template_preview_response(template, template_object): "type": template.template_type, "version": template.version, "body": str(template_object), - "subject": subject + "subject": subject, + "postage": template.postage } diff --git a/requirements-app.txt b/requirements-app.txt index b0fa41bf3..09c0ee350 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -29,6 +29,8 @@ awscli-cwlogs>=1.4,<1.5 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default itsdangerous==0.24 # pyup: <1.0.0 -git+https://github.com/alphagov/notifications-utils.git@30.7.1#egg=notifications-utils==30.7.1 +git+https://github.com/alphagov/notifications-utils.git@30.7.2#egg=notifications-utils==30.7.2 +# pinned until upgrade to redis-py 3 works +redis==2.10.6 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/requirements.txt b/requirements.txt index 1df4c529f..f711cac44 100644 --- a/requirements.txt +++ b/requirements.txt @@ -31,22 +31,24 @@ awscli-cwlogs>=1.4,<1.5 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default itsdangerous==0.24 # pyup: <1.0.0 -git+https://github.com/alphagov/notifications-utils.git@30.7.1#egg=notifications-utils==30.7.1 +git+https://github.com/alphagov/notifications-utils.git@30.7.2#egg=notifications-utils==30.7.2 +# pinned until upgrade to redis-py 3 works +redis==2.10.6 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 ## The following requirements were added by pip freeze: -alembic==1.0.3 +alembic==1.0.5 amqp==1.4.9 anyjson==0.3.3 attrs==18.2.0 -awscli==1.16.62 -bcrypt==3.1.4 +awscli==1.16.84 +bcrypt==3.1.5 billiard==3.3.0.23 bleach==2.1.3 boto3==1.6.16 -botocore==1.12.52 -certifi==2018.10.15 +botocore==1.12.74 +certifi==2018.11.29 chardet==3.0.4 Click==7.0 colorama==0.3.9 @@ -55,7 +57,7 @@ Flask-Redis==0.3.0 future==0.17.1 greenlet==0.4.15 html5lib==1.0.1 -idna==2.7 +idna==2.8 Jinja2==2.10 jmespath==0.9.3 kombu==3.0.37 @@ -65,20 +67,19 @@ mistune==0.8.3 monotonic==1.5 orderedset==2.0.1 phonenumbers==8.9.4 -pyasn1==0.4.4 +pyasn1==0.4.5 pycparser==2.19 PyPDF2==1.26.0 -pyrsistent==0.14.7 +pyrsistent==0.14.9 python-dateutil==2.7.5 python-editor==1.0.3 python-json-logger==0.1.8 -pytz==2018.7 +pytz==2018.9 PyYAML==3.12 -redis==2.10.6 -requests==2.20.1 +requests==2.21.0 rsa==3.4.2 s3transfer==0.1.13 -six==1.11.0 +six==1.12.0 smartypants==2.0.1 statsd==3.2.2 urllib3==1.24.1 diff --git a/scripts/run_multi_worker_app_paas.sh b/scripts/run_multi_worker_app_paas.sh index 3965ee577..5ddc933ec 100755 --- a/scripts/run_multi_worker_app_paas.sh +++ b/scripts/run_multi_worker_app_paas.sh @@ -39,13 +39,15 @@ log_stream_name = {hostname} EOF } -# For every PID, check if it's still running -# if it is, send the sigterm +# For every PID, check if it's still running. if it is, send the sigterm. then wait 9 seconds before sending sigkill function on_exit { + echo "multi worker app exiting" wait_time=0 - while true; do - # refresh pids to account for the case that - # some workers may have terminated but others not + + send_signal_to_celery_processes TERM + + # check if the apps are still running every second + while [[ "$wait_time" -le "$TERMINATE_TIMEOUT" ]]; do get_celery_pids # look here for explanation regarding this syntax: @@ -53,28 +55,14 @@ function on_exit { PROCESS_COUNT="${#APP_PIDS[@]}" if [[ "${PROCESS_COUNT}" -eq "0" ]]; then echo "No more .pid files found, exiting" - break + return 0 fi - echo "Terminating celery processes with pids "${APP_PIDS} - for APP_PID in ${APP_PIDS}; do - # if TERMINATE_TIMEOUT is reached, send SIGKILL - if [[ "$wait_time" -ge "$TERMINATE_TIMEOUT" ]]; then - echo "Timeout reached, killing process with pid ${APP_PID}" - kill -9 ${APP_PID} || true - continue - else - echo "Timeout not reached yet, checking " ${APP_PID} - # else, if process is still running send SIGTERM - if [[ $(kill -0 ${APP_PID} 2&>/dev/null) ]]; then - echo "Terminating celery process with pid ${APP_PID}" - kill ${APP_PID} || true - fi - fi - done let wait_time=wait_time+1 sleep 1 done + + send_signal_to_celery_processes KILL } function get_celery_pids { @@ -85,6 +73,16 @@ function get_celery_pids { fi } +function send_signal_to_celery_processes { + # refresh pids to account for the case that some workers may have terminated but others not + get_celery_pids + # send signal to all remaining apps + for APP_PID in ${APP_PIDS}; do + echo "Sending signal ${1} to process with pid ${APP_PID}" + kill -s ${1} ${APP_PID} || true + done +} + function start_application { eval "$@" get_celery_pids diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 15f76846f..5b6aa2234 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -76,20 +76,26 @@ def service_factory(notify_db, notify_db_session): user = create_user() if not email_from: email_from = service_name - service = sample_service(notify_db, notify_db_session, service_name, user, email_from=email_from) + + service = create_service( + email_from=email_from, + service_name=service_name, + service_permissions=None, + user=user, + check_if_service_exists=True, + ) if template_type == 'email': - sample_template( - notify_db, - notify_db_session, + create_template( + service, + template_name="Template Name", template_type=template_type, - subject_line=service.email_from, - service=service + subject=service.email_from, ) else: - sample_template( - notify_db, - notify_db_session, - service=service + create_template( + service, + template_name="Template Name", + template_type='sms', ) return service @@ -158,6 +164,7 @@ def sample_service( email_from=None, permissions=None, research_mode=None, + postage="second", ): if user is None: user = create_user() @@ -170,6 +177,7 @@ def sample_service( 'restricted': restricted, 'email_from': email_from, 'created_by': user, + "postage": postage, } service = Service.query.filter_by(name=service_name).first() if not service: @@ -193,7 +201,8 @@ def sample_service( def _sample_service_full_permissions(notify_db_session): service = create_service( service_name="sample service full permissions", - service_permissions=set(SERVICE_PERMISSION_TYPES) + service_permissions=set(SERVICE_PERMISSION_TYPES), + check_if_service_exists=True ) create_inbound_number('12345', service_id=service.id) return service @@ -226,7 +235,7 @@ def sample_template( if service is None: service = Service.query.filter_by(name='Sample service').first() if not service: - service = create_service(service_permissions=permissions) + service = create_service(service_permissions=permissions, check_if_service_exists=True) if created_by is None: created_by = create_user() @@ -288,7 +297,7 @@ def sample_email_template( if user is None: user = create_user() if service is None: - service = sample_service(notify_db, notify_db_session, permissions=permissions) + service = create_service(user=user, service_permissions=permissions, check_if_service_exists=True) data = { 'name': template_name, 'template_type': template_type, @@ -343,7 +352,7 @@ def sample_api_key(notify_db, key_type=KEY_TYPE_NORMAL, name=None): if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service(check_if_service_exists=True) data = {'service': service, 'name': name or uuid.uuid4(), 'created_by': service.created_by, 'key_type': key_type} api_key = ApiKey(**data) save_model_api_key(api_key) @@ -375,10 +384,9 @@ def sample_job( archived=False ): if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service(check_if_service_exists=True) if template is None: - template = sample_template(notify_db, notify_db_session, - service=service) + template = create_template(service=service) data = { 'id': uuid.uuid4(), 'service_id': service.id, @@ -435,7 +443,7 @@ def sample_email_job(notify_db, service=None, template=None): if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service(check_if_service_exists=True) if template is None: template = sample_email_template( notify_db, @@ -543,9 +551,9 @@ def sample_notification( if created_at is None: created_at = datetime.utcnow() if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service(check_if_service_exists=True) if template is None: - template = sample_template(notify_db, notify_db_session, service=service) + template = create_template(service=service) if job is None and api_key is None: # we didn't specify in test - lets create it @@ -632,7 +640,7 @@ def sample_notification_with_api_key(notify_db, notify_db_session): @pytest.fixture(scope='function') def sample_email_notification(notify_db, notify_db_session): created_at = datetime.utcnow() - service = sample_service(notify_db, notify_db_session) + service = create_service(check_if_service_exists=True) template = sample_email_template(notify_db, notify_db_session, service=service) job = sample_job(notify_db, notify_db_session, service=service, template=template) @@ -734,7 +742,7 @@ def sample_invited_user(notify_db, to_email_address=None): if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service(check_if_service_exists=True) if to_email_address is None: to_email_address = 'invited_user@digital.gov.uk' @@ -774,7 +782,7 @@ def sample_permission(notify_db, 'permission': permission } if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service(check_if_service_exists=True) if service: data['service'] = service p_model = Permission.query.filter_by( @@ -795,7 +803,7 @@ def sample_user_service_permission( if user is None: user = create_user() if service is None: - service = sample_service(notify_db, notify_db_session, user=user) + service = create_service(user=user, check_if_service_exists=True) data = { 'user': user, 'service': service, @@ -1029,7 +1037,7 @@ def notify_service(notify_db, notify_db_session): @pytest.fixture(scope='function') def sample_service_whitelist(notify_db, notify_db_session, service=None, email_address=None, mobile_number=None): if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service(check_if_service_exists=True) if email_address: whitelisted_user = ServiceWhitelist.from_string(service.id, EMAIL_TYPE, email_address) @@ -1054,7 +1062,7 @@ def sample_provider_rate(notify_db, notify_db_session, valid_from=None, rate=Non @pytest.fixture def sample_inbound_numbers(notify_db, notify_db_session, sample_service): - service = create_service(service_name='sample service 2') + service = create_service(service_name='sample service 2', check_if_service_exists=True) inbound_numbers = list() inbound_numbers.append(create_inbound_number(number='1', provider='mmg')) inbound_numbers.append(create_inbound_number(number='2', provider='mmg', active=False, service_id=service.id)) diff --git a/tests/app/dao/notification_dao/test_notification_dao_template_usage.py b/tests/app/dao/notification_dao/test_notification_dao_template_usage.py index 76800694f..88aaa783d 100644 --- a/tests/app/dao/notification_dao/test_notification_dao_template_usage.py +++ b/tests/app/dao/notification_dao/test_notification_dao_template_usage.py @@ -22,7 +22,7 @@ from tests.app.db import ( def test_last_template_usage_should_get_right_data(sample_notification): results = dao_get_last_template_usage(sample_notification.template_id, 'sms', sample_notification.service_id) - assert results.template.name == 'Template Name' + assert results.template.name == 'sms Template Name' assert results.template.template_type == 'sms' assert results.created_at == sample_notification.created_at assert results.template_id == sample_notification.template_id diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index d86cec7a0..cbe6c6b72 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -153,7 +153,7 @@ def test_dao_update_template_reply_to_none_to_some(sample_service, sample_user): assert template_history.updated_at == updated.updated_at -def test_dao_update_tempalte_reply_to_some_to_some(sample_service, sample_user): +def test_dao_update_template_reply_to_some_to_some(sample_service, sample_user): letter_contact = create_letter_contact(sample_service, 'Edinburgh, ED1 1AA') letter_contact_2 = create_letter_contact(sample_service, 'London, N1 1DE') diff --git a/tests/app/db.py b/tests/app/db.py index 94d6ac287..5c4889057 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -17,7 +17,7 @@ from app.dao.service_data_retention_dao import insert_service_data_retention from app.dao.service_inbound_api_dao import save_service_inbound_api from app.dao.service_permissions_dao import dao_add_service_permission from app.dao.service_sms_sender_dao import update_existing_sms_sender_with_inbound_number, dao_update_service_sms_sender -from app.dao.services_dao import dao_create_service +from app.dao.services_dao import dao_create_service, dao_add_user_to_service from app.dao.templates_dao import dao_create_template, dao_update_template from app.dao.users_dao import save_model_user from app.models import ( @@ -81,21 +81,29 @@ def create_service( prefix_sms=True, message_limit=1000, organisation_type='central', + postage='second', + check_if_service_exists=False ): - service = Service( - name=service_name, - message_limit=message_limit, - restricted=restricted, - email_from=email_from if email_from else service_name.lower().replace(' ', '.'), - created_by=user or create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())), - prefix_sms=prefix_sms, - organisation_type=organisation_type, - ) + if check_if_service_exists: + service = Service.query.filter_by(name=service_name).first() + if (not check_if_service_exists) or (check_if_service_exists and not service): + service = Service( + name=service_name, + message_limit=message_limit, + restricted=restricted, + email_from=email_from if email_from else service_name.lower().replace(' ', '.'), + created_by=user if user else create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())), + prefix_sms=prefix_sms, + organisation_type=organisation_type, + postage=postage + ) + dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) - dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) - - service.active = active - service.research_mode = research_mode + service.active = active + service.research_mode = research_mode + else: + if user and user not in service.users: + dao_add_user_to_service(service, user) return service @@ -140,6 +148,7 @@ def create_template( hidden=False, archived=False, folder=None, + postage=None, ): data = { 'name': template_name or '{} Template Name'.format(template_type), @@ -150,6 +159,7 @@ def create_template( 'reply_to': reply_to, 'hidden': hidden, 'folder': folder, + 'postage': postage, } if template_type != SMS_TYPE: data['subject'] = subject diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 609a863d8..192644bde 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -13,7 +13,9 @@ from app.models import ( Notification, NotificationHistory, ScheduledNotification, - Template + Template, + LETTER_TYPE, + CHOOSE_POSTAGE ) from app.notifications.process_notifications import ( create_content_for_notification, @@ -27,6 +29,8 @@ from app.utils import cache_key_for_service_template_counter from app.v2.errors import BadRequestError from tests.app.conftest import sample_api_key as create_api_key +from tests.app.db import create_service, create_template + def test_create_content_for_notification_passes(sample_email_template): template = Template.query.get(sample_email_template.id) @@ -477,6 +481,41 @@ def test_persist_email_notification_stores_normalised_email( assert persisted_notification.normalised_to == expected_recipient_normalised +@pytest.mark.parametrize( + "service_permissions, template_postage, expected_postage", + [ + ([LETTER_TYPE], "first", "second"), + ([LETTER_TYPE, CHOOSE_POSTAGE], "first", "first"), + ([LETTER_TYPE, CHOOSE_POSTAGE], None, "second"), + ] +) +def test_persist_letter_notification_finds_correct_postage( + mocker, + notify_db, + notify_db_session, + service_permissions, + template_postage, + expected_postage +): + service = create_service(service_permissions=service_permissions, postage="second") + api_key = create_api_key(notify_db, notify_db_session, service=service) + template = create_template(service, template_type=LETTER_TYPE, postage=template_postage) + mocker.patch('app.dao.templates_dao.dao_get_template_by_id', return_value=template) + persist_notification( + template_id=template.id, + template_version=template.version, + recipient="Jane Doe, 10 Downing Street, London", + service=service, + personalisation=None, + notification_type=LETTER_TYPE, + api_key_id=api_key.id, + key_type=api_key.key_type, + ) + persisted_notification = Notification.query.all()[0] + + assert persisted_notification.postage == expected_postage + + @pytest.mark.parametrize('utc_time, day_in_key', [ ('2016-01-01 23:00:00', '2016-01-01'), ('2016-06-01 22:59:00', '2016-06-01'), diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 0c9051d90..a035500cb 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -153,7 +153,7 @@ def test_get_all_notifications(client, sample_notification): assert notifications['notifications'][0]['to'] == '+447700900855' assert notifications['notifications'][0]['service'] == str(sample_notification.service_id) - assert notifications['notifications'][0]['body'] == "This is a template:\nwith a newline" + assert notifications['notifications'][0]['body'] == 'Dear Sir/Madam, Hello. Yours Truly, The Government.' def test_normal_api_key_returns_notifications_created_from_jobs_and_from_api( diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 7cf46aa80..64c51becc 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -18,7 +18,8 @@ from app.models import ( LETTER_TYPE, SMS_TYPE, Template, - TemplateHistory + TemplateHistory, + CHOOSE_POSTAGE ) from app.dao.templates_dao import dao_get_template_by_id, dao_redact_template @@ -43,7 +44,7 @@ from tests.conftest import set_config_values def test_should_create_a_new_template_for_a_service( client, sample_user, template_type, subject ): - service = create_service(service_permissions=[template_type]) + service = create_service(service_permissions=[template_type, CHOOSE_POSTAGE]) data = { 'name': 'my template', 'template_type': template_type, @@ -53,6 +54,8 @@ def test_should_create_a_new_template_for_a_service( } if subject: data.update({'subject': subject}) + if template_type == LETTER_TYPE: + data.update({'postage': 'first'}) data = json.dumps(data) auth_header = create_authorization_header() @@ -76,6 +79,11 @@ def test_should_create_a_new_template_for_a_service( else: assert not json_resp['data']['subject'] + if template_type == LETTER_TYPE: + assert json_resp['data']['postage'] == 'first' + else: + assert not json_resp['data']['postage'] + template = Template.query.get(json_resp['data']['id']) from app.schemas import template_schema assert sorted(json_resp['data']) == sorted(template_schema.dump(template).data) @@ -210,6 +218,34 @@ def test_should_raise_error_on_create_if_no_permission( assert json_resp['message'] == expected_error +def test_should_raise_error_on_create_if_no_choose_postage_permission(client, sample_user): + service = create_service(service_permissions=[LETTER_TYPE]) + data = { + 'name': 'my template', + 'template_type': LETTER_TYPE, + 'content': 'template content', + 'service': str(service.id), + 'created_by': str(sample_user.id), + 'subject': "Some letter", + 'postage': 'first', + } + + data = json.dumps(data) + auth_header = create_authorization_header() + + response = client.post( + '/service/{}/template'.format(service.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 403 + assert json_resp['result'] == 'error' + assert json_resp['message'] == { + "template_postage": ["Setting postage on templates is not enabled for this service."] + } + + @pytest.mark.parametrize('template_factory, expected_error', [ (sample_template_without_sms_permission, {'template_type': ['Updating text message templates is not allowed']}), (sample_template_without_email_permission, {'template_type': ['Updating email templates is not allowed']}), @@ -239,6 +275,33 @@ def test_should_be_error_on_update_if_no_permission( assert json_resp['message'] == expected_error +def test_should_be_error_on_update_if_no_choose_postage_permission(client, sample_user): + service = create_service(service_name='some_service', service_permissions=[LETTER_TYPE]) + template = create_template(service, template_type=LETTER_TYPE) + data = { + 'content': 'new template content', + 'created_by': str(sample_user.id), + 'postage': 'first' + } + + data = json.dumps(data) + auth_header = create_authorization_header() + + update_response = client.post( + '/service/{}/template/{}'.format( + template.service_id, template.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + + json_resp = json.loads(update_response.get_data(as_text=True)) + assert update_response.status_code == 403 + assert json_resp['result'] == 'error' + assert json_resp['message'] == { + "template_postage": ["Setting postage on templates is not enabled for this service."] + } + + def test_should_error_if_created_by_missing(client, sample_user, sample_service): service_id = str(sample_service.id) data = { @@ -301,16 +364,19 @@ def test_must_have_a_subject_on_an_email_or_letter_template(client, sample_user, assert json_resp['errors'][0]["message"] == 'subject is a required property' -def test_update_should_update_a_template(client, sample_user, sample_template): +def test_update_should_update_a_template(client, sample_user): + service = create_service(service_permissions=[LETTER_TYPE, CHOOSE_POSTAGE]) + template = create_template(service, template_type="letter", postage="second") data = { - 'content': 'my template has new content ', - 'created_by': str(sample_user.id) + 'content': 'my template has new content, swell!', + 'created_by': str(sample_user.id), + 'postage': 'first' } data = json.dumps(data) auth_header = create_authorization_header() update_response = client.post( - '/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), + '/service/{}/template/{}'.format(service.id, template.id), headers=[('Content-Type', 'application/json'), auth_header], data=data ) @@ -318,10 +384,11 @@ def test_update_should_update_a_template(client, sample_user, sample_template): assert update_response.status_code == 200 update_json_resp = json.loads(update_response.get_data(as_text=True)) assert update_json_resp['data']['content'] == ( - 'my template has new content ' + 'my template has new content, swell!' ) - assert update_json_resp['data']['name'] == sample_template.name - assert update_json_resp['data']['template_type'] == sample_template.template_type + assert update_json_resp['data']['postage'] == 'first' + assert update_json_resp['data']['name'] == template.name + assert update_json_resp['data']['template_type'] == template.template_type assert update_json_resp['data']['version'] == 2 diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index b9559e134..1a37cc6e7 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -59,7 +59,7 @@ def test_get_template_statistics_for_service_by_day_returns_template_info(admin_ assert json_resp['data'][0]['count'] == 1 assert json_resp['data'][0]['template_id'] == str(sample_notification.template_id) - assert json_resp['data'][0]['template_name'] == 'Template Name' + assert json_resp['data'][0]['template_name'] == 'sms Template Name' assert json_resp['data'][0]['template_type'] == 'sms' assert json_resp['data'][0]['is_precompiled_letter'] is False diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 4b42af6dc..734928088 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -20,6 +20,7 @@ from app.models import ( NOTIFICATION_DELIVERED, NOTIFICATION_PENDING_VIRUS_CHECK, SMS_TYPE, + CHOOSE_POSTAGE ) from app.schema_validation import validate from app.v2.errors import RateLimitError @@ -95,12 +96,23 @@ def test_post_letter_notification_returns_201(client, sample_letter_template, mo mock.assert_called_once_with([str(notification.id)], queue=QueueNames.CREATE_LETTERS_PDF) -@pytest.mark.parametrize('postage', ['first', 'second']) -def test_post_letter_notification_sets_postage(client, sample_letter_template, mocker, postage): - sample_letter_template.service.postage = postage +@pytest.mark.parametrize('service_permissions, service_postage, template_postage, expected_postage', [ + ([LETTER_TYPE], "second", "first", "second"), + ([LETTER_TYPE], "first", "second", "first"), + ([LETTER_TYPE], "first", None, "first"), + ([LETTER_TYPE, CHOOSE_POSTAGE], "second", "first", "first"), + ([LETTER_TYPE, CHOOSE_POSTAGE], "second", None, "second"), + ([LETTER_TYPE, CHOOSE_POSTAGE], "second", "second", "second"), + ([LETTER_TYPE, CHOOSE_POSTAGE], "first", "second", "second"), +]) +def test_post_letter_notification_sets_postage( + client, notify_db_session, mocker, service_permissions, service_postage, template_postage, expected_postage +): + service = create_service(service_permissions=service_permissions, postage=service_postage) + template = create_template(service, template_type="letter", postage=template_postage) mocker.patch('app.celery.tasks.letters_pdf_tasks.create_letters_pdf.apply_async') data = { - 'template_id': str(sample_letter_template.id), + 'template_id': str(template.id), 'personalisation': { 'address_line_1': 'Her Royal Highness Queen Elizabeth II', 'address_line_2': 'Buckingham Palace', @@ -110,11 +122,11 @@ def test_post_letter_notification_sets_postage(client, sample_letter_template, m } } - resp_json = letter_request(client, data, service_id=sample_letter_template.service_id) + resp_json = letter_request(client, data, service_id=service.id) assert validate(resp_json, post_letter_response) == resp_json notification = Notification.query.one() - assert notification.postage == postage + assert notification.postage == expected_postage @pytest.mark.parametrize('env', [