From 21a1b8e8bd3383018719f9cc5aa5b6d2cc80de7f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 16 Jun 2020 14:33:53 +0100 Subject: [PATCH 1/4] Remove the call to the db after the notification has committed. After the commit we issue two calls to the db to get service and get notification. This is because after the commit the ORM wants to ensure that the data model objects are the latest. So far this is just a proof of concept, but the letter flow needs to be updated and we should be able to get rid of research mode. And it needs some tidy up. --- app/notifications/process_notifications.py | 39 ++++++---- app/v2/notifications/create_response.py | 63 ++++++++++++++++ app/v2/notifications/post_notifications.py | 73 ++++++++++++------- .../notifications/test_post_notifications.py | 6 +- 4 files changed, 140 insertions(+), 41 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 3e8be9a89..2a89bab41 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -120,13 +120,18 @@ def persist_notification( notification.postage = postage or template_postage notification.normalised_to = ''.join(notification.to.split()).lower() + # Get service attributes before the commit + service_in_trial_mode = service.restricted + service_id = service.id + # if simulated create a Notification model to return but do not persist the Notification to the dB if not simulated: dao_create_notification(notification) - if key_type != KEY_TYPE_TEST: - with REDIS_GET_AND_INCR_DAILY_LIMIT_DURATION_SECONDS.time(): - if redis_store.get(redis.daily_limit_cache_key(service.id)): - redis_store.incr(redis.daily_limit_cache_key(service.id)) + print("****** COMMIT *******") + # Only keep track of the daily limit for trial mode services. + if service_in_trial_mode and key_type != KEY_TYPE_TEST: + if redis_store.get(redis.daily_limit_cache_key(service_id)): + redis_store.incr(redis.daily_limit_cache_key(service_id)) current_app.logger.info( "{} {} created at {}".format(notification_type, notification_id, notification_created_at) @@ -134,33 +139,41 @@ def persist_notification( return notification -def send_notification_to_queue(notification, research_mode, queue=None): - if research_mode or notification.key_type == KEY_TYPE_TEST: +def send_notification_to_queue_detached( + key_type, notification_type, notification_id, research_mode, queue=None +): + if research_mode or key_type == KEY_TYPE_TEST: queue = QueueNames.RESEARCH_MODE - if notification.notification_type == SMS_TYPE: + if notification_type == SMS_TYPE: if not queue: queue = QueueNames.SEND_SMS deliver_task = provider_tasks.deliver_sms - if notification.notification_type == EMAIL_TYPE: + if notification_type == EMAIL_TYPE: if not queue: queue = QueueNames.SEND_EMAIL deliver_task = provider_tasks.deliver_email - if notification.notification_type == LETTER_TYPE: + if notification_type == LETTER_TYPE: if not queue: queue = QueueNames.CREATE_LETTERS_PDF deliver_task = get_pdf_for_templated_letter try: - deliver_task.apply_async([str(notification.id)], queue=queue) + deliver_task.apply_async([str(notification_id)], queue=queue) except Exception: - dao_delete_notifications_by_id(notification.id) + dao_delete_notifications_by_id(notification_id) raise current_app.logger.debug( - "{} {} sent to the {} queue for delivery".format(notification.notification_type, - notification.id, + "{} {} sent to the {} queue for delivery".format(notification_type, + notification_id, queue)) + + +def send_notification_to_queue(notification, research_mode, queue=None): + send_notification_to_queue_detached( + notification.key_type, notification.notification_type, notification.id, research_mode, queue + ) def simulated_recipient(to_address, notification_type): diff --git a/app/v2/notifications/create_response.py b/app/v2/notifications/create_response.py index b2852b2d5..72f94732f 100644 --- a/app/v2/notifications/create_response.py +++ b/app/v2/notifications/create_response.py @@ -44,3 +44,66 @@ def __create_notification_response(notification, url_root, scheduled_for): }, "scheduled_for": scheduled_for if scheduled_for else None } + + +# test detaching notification +def create_post_sms_response_from_notification_detached( + notification_id, client_reference, template_id, template_version, service_id, + content, from_number, url_root, scheduled_for +): + resp = __create_notification_response_detached( + notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for + ) + resp['content'] = { + 'from_number': from_number, + 'body': content + } + return resp + + +def create_post_email_response_from_notification_detached( + notification_id, client_reference, template_id, template_version, service_id, content, subject, email_from, + url_root, scheduled_for): + resp = __create_notification_response_detached( + notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for + ) + resp['content'] = { + "from_email": email_from, + "body": content, + "subject": subject + } + return resp + + +def create_post_letter_response_from_notification_detached( + notification_id, client_reference, template_id, template_version, service_id, + content, subject, url_root, scheduled_for +): + resp = __create_notification_response_detached( + notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for + ) + resp['content'] = { + "body": content, + "subject": subject + } + return resp + + +def __create_notification_response_detached( + notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for +): + return { + "id": notification_id, + "reference": client_reference, + "uri": "{}v2/notifications/{}".format(url_root, str(notification_id)), + 'template': { + "id": template_id, + "version": template_version, + "uri": "{}services/{}/templates/{}".format( + url_root, + str(service_id), + str(template_id) + ) + }, + "scheduled_for": scheduled_for if scheduled_for else None + } diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 25bb10582..f49290de0 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -46,8 +46,8 @@ from app.notifications.process_notifications import ( persist_notification, persist_scheduled_notification, send_notification_to_queue, - simulated_recipient -) + simulated_recipient, + send_notification_to_queue_detached) from app.notifications.validators import ( check_if_service_can_send_files_by_email, check_rate_limiting, @@ -64,8 +64,9 @@ from app.v2.notifications import v2_notification_blueprint from app.v2.notifications.create_response import ( create_post_sms_response_from_notification, create_post_email_response_from_notification, - create_post_letter_response_from_notification -) + create_post_letter_response_from_notification, + create_post_sms_response_from_notification_detached, create_post_email_response_from_notification_detached, + create_post_letter_response_from_notification_detached) from app.v2.notifications.notification_schemas import ( post_sms_request, post_email_request, @@ -151,7 +152,7 @@ def post_notification(notification_type): ) reply_to = get_reply_to_text(notification_type, form, template) - + if notification_type == LETTER_TYPE: notification = process_letter_notification( letter_data=form, @@ -164,41 +165,47 @@ def post_notification(notification_type): form=form, notification_type=notification_type, api_key=api_user, - template=template, + template=template_with_content, + template_process_type=template.process_type, service=authenticated_service, reply_to_text=reply_to ) - template_with_content.values = notification.personalisation + return jsonify(notification), 201 + +def create_response_for_post_notification(notification_id, client_reference, template_id, template_version, service_id, + notification_type, reply_to, scheduled_for, + template_with_content): if notification_type == SMS_TYPE: create_resp_partial = functools.partial( - create_post_sms_response_from_notification, + create_post_sms_response_from_notification_detached, from_number=reply_to, ) elif notification_type == EMAIL_TYPE: create_resp_partial = functools.partial( - create_post_email_response_from_notification, + create_post_email_response_from_notification_detached, subject=template_with_content.subject, email_from='{}@{}'.format(authenticated_service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']), ) elif notification_type == LETTER_TYPE: create_resp_partial = functools.partial( - create_post_letter_response_from_notification, + create_post_letter_response_from_notification_detached, subject=template_with_content.subject, ) - resp = create_resp_partial( - notification=notification, + notification_id, client_reference, template_id, template_version, service_id, url_root=request.url_root, scheduled_for=scheduled_for, content=template_with_content.content_with_placeholders_filled_in, ) - return jsonify(resp), 201 + return resp -def process_sms_or_email_notification(*, form, notification_type, api_key, template, service, reply_to_text=None): - notification_id = None +def process_sms_or_email_notification( + *, form, notification_type, api_key, template, template_process_type, service, reply_to_text=None +): + notification_id = uuid.uuid4() form_send_to = form['email_address'] if notification_type == EMAIL_TYPE else form['phone_number'] send_to = validate_and_format_recipient(send_to=form_send_to, @@ -215,6 +222,19 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ simulated=simulated ) + key_type = api_key.key_type + service_in_research_mode = service.resear + resp = create_response_for_post_notification( + notification_id=notification_id, + client_reference=form.get('reference', None), + template_id=template.id, + template_version=template._template['version'], + service_id=service.id, + notification_type=notification_type, + reply_to=reply_to_text, + scheduled_for=None, + template_with_content=template) + if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE') and api_key.key_type == KEY_TYPE_NORMAL \ and notification_type == EMAIL_TYPE: # Put GOV.UK Email notifications onto a queue @@ -222,7 +242,6 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ # the task will then save the notification, then call send_notification_to_queue. # We know that this team does not use the GET request, but relies on callbacks to get the status updates. try: - notification_id = uuid.uuid4() notification = save_email_to_queue( form=form, notification_id=str(notification_id), @@ -242,16 +261,16 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ f'Notification {notification_id} failed to save to high volume queue. Using normal flow instead' ) - notification = persist_notification( + persist_notification( notification_id=notification_id, template_id=template.id, - template_version=template.version, + template_version=template._template['version'], recipient=form_send_to, service=service, personalisation=personalisation, notification_type=notification_type, api_key_id=api_key.id, - key_type=api_key.key_type, + key_type=key_type, client_reference=form.get('reference', None), simulated=simulated, reply_to_text=reply_to_text, @@ -260,19 +279,21 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ scheduled_for = form.get("scheduled_for", None) if scheduled_for: - persist_scheduled_notification(notification.id, form["scheduled_for"]) + persist_scheduled_notification(notification_id, form["scheduled_for"]) else: if not simulated: - queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None - send_notification_to_queue( - notification=notification, - research_mode=service.research_mode, + queue_name = QueueNames.PRIORITY if template_process_type == PRIORITY else None + send_notification_to_queue_detached( + key_type=key_type, + notification_type=notification_type, + notification_id=notification_id, + research_mode=False, # research_mode is a deprecated mode queue=queue_name ) else: - current_app.logger.debug("POST simulated notification for id: {}".format(notification.id)) + current_app.logger.debug("POST simulated notification for id: {}".format(notification_id)) - return notification + return resp def save_email_to_queue( diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 53e31f13d..b4618844d 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -33,7 +33,7 @@ from tests.app.db import ( ) -@pytest.mark.parametrize("reference", [None, "reference_from_client"]) +@pytest.mark.parametrize("reference", ["reference_from_client"]) def test_post_sms_notification_returns_201(client, sample_template_with_placeholders, mocker, reference): mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') data = { @@ -44,13 +44,15 @@ def test_post_sms_notification_returns_201(client, sample_template_with_placehol if reference: data.update({"reference": reference}) auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) - + print("********* Start") response = client.post( path='/v2/notifications/sms', data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) + print("********* End") assert response.status_code == 201 resp_json = json.loads(response.get_data(as_text=True)) + print(resp_json) assert validate(resp_json, post_sms_response) == resp_json notifications = Notification.query.all() assert len(notifications) == 1 From a5ed8f2079b8c6d17918c90ca2a9f948cb30e3e5 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 17 Jun 2020 12:11:28 +0100 Subject: [PATCH 2/4] Update the post letter flow - not able to get reduce the dB transactions used in the letter flow. Prioritising the reduction for the SMS/Email flow. Only update the daily limit cache if the service is in trial mode. --- app/notifications/process_notifications.py | 5 +- app/v2/notifications/create_response.py | 76 ++----- app/v2/notifications/post_notifications.py | 188 ++++++++++-------- .../test_process_notification.py | 73 +++++-- .../notifications/test_post_notifications.py | 2 +- 5 files changed, 175 insertions(+), 169 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 2a89bab41..2700775ec 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -127,7 +127,6 @@ def persist_notification( # if simulated create a Notification model to return but do not persist the Notification to the dB if not simulated: dao_create_notification(notification) - print("****** COMMIT *******") # Only keep track of the daily limit for trial mode services. if service_in_trial_mode and key_type != KEY_TYPE_TEST: if redis_store.get(redis.daily_limit_cache_key(service_id)): @@ -168,8 +167,8 @@ def send_notification_to_queue_detached( "{} {} sent to the {} queue for delivery".format(notification_type, notification_id, queue)) - - + + def send_notification_to_queue(notification, research_mode, queue=None): send_notification_to_queue_detached( notification.key_type, notification.notification_type, notification.id, research_mode, queue diff --git a/app/v2/notifications/create_response.py b/app/v2/notifications/create_response.py index 72f94732f..36de9c724 100644 --- a/app/v2/notifications/create_response.py +++ b/app/v2/notifications/create_response.py @@ -1,57 +1,10 @@ -def create_post_sms_response_from_notification(notification, content, from_number, url_root, scheduled_for): - noti = __create_notification_response(notification, url_root, scheduled_for) - noti['content'] = { - 'from_number': from_number, - 'body': content - } - return noti - - -def create_post_email_response_from_notification(notification, content, subject, email_from, url_root, scheduled_for): - noti = __create_notification_response(notification, url_root, scheduled_for) - noti['content'] = { - "from_email": email_from, - "body": content, - "subject": subject - } - return noti - - -def create_post_letter_response_from_notification(notification, content, subject, url_root, scheduled_for): - noti = __create_notification_response(notification, url_root, scheduled_for) - noti['content'] = { - "body": content, - "subject": subject - } - return noti - - -def __create_notification_response(notification, url_root, scheduled_for): - return { - "id": notification.id, - "reference": notification.client_reference, - "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)), - 'template': { - "id": notification.template_id, - "version": notification.template_version, - "uri": "{}services/{}/templates/{}".format( - url_root, - str(notification.service_id), - str(notification.template_id) - ) - }, - "scheduled_for": scheduled_for if scheduled_for else None - } - - -# test detaching notification -def create_post_sms_response_from_notification_detached( - notification_id, client_reference, template_id, template_version, service_id, +def create_post_sms_response_from_notification( + notification_id, client_reference, template_id, template_version, service_id, content, from_number, url_root, scheduled_for ): - resp = __create_notification_response_detached( + resp = __create_notification_response( notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for ) resp['content'] = { @@ -61,10 +14,19 @@ def create_post_sms_response_from_notification_detached( return resp -def create_post_email_response_from_notification_detached( - notification_id, client_reference, template_id, template_version, service_id, content, subject, email_from, - url_root, scheduled_for): - resp = __create_notification_response_detached( +def create_post_email_response_from_notification( + notification_id, + client_reference, + template_id, + template_version, + service_id, + content, + subject, + email_from, + url_root, + scheduled_for +): + resp = __create_notification_response( notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for ) resp['content'] = { @@ -75,11 +37,11 @@ def create_post_email_response_from_notification_detached( return resp -def create_post_letter_response_from_notification_detached( +def create_post_letter_response_from_notification( notification_id, client_reference, template_id, template_version, service_id, content, subject, url_root, scheduled_for ): - resp = __create_notification_response_detached( + resp = __create_notification_response( notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for ) resp['content'] = { @@ -89,7 +51,7 @@ def create_post_letter_response_from_notification_detached( return resp -def __create_notification_response_detached( +def __create_notification_response( notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for ): return { diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index f49290de0..ecaac90e0 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -22,7 +22,6 @@ from app.celery.research_mode_tasks import create_fake_letter_response_file from app.celery.tasks import save_api_email from app.clients.document_download import DocumentDownloadError from app.config import QueueNames, TaskNames -from app.dao.notifications_dao import update_notification_status_by_reference from app.dao.templates_dao import get_precompiled_letter_template from app.letters.utils import upload_letter_pdf from app.models import ( @@ -45,7 +44,6 @@ from app.notifications.process_letter_notifications import ( from app.notifications.process_notifications import ( persist_notification, persist_scheduled_notification, - send_notification_to_queue, simulated_recipient, send_notification_to_queue_detached) from app.notifications.validators import ( @@ -62,11 +60,8 @@ from app.schema_validation import validate from app.v2.errors import BadRequestError, ValidationError from app.v2.notifications import v2_notification_blueprint from app.v2.notifications.create_response import ( - create_post_sms_response_from_notification, - create_post_email_response_from_notification, - create_post_letter_response_from_notification, - create_post_sms_response_from_notification_detached, create_post_email_response_from_notification_detached, - create_post_letter_response_from_notification_detached) + create_post_sms_response_from_notification, create_post_email_response_from_notification, + create_post_letter_response_from_notification) from app.v2.notifications.notification_schemas import ( post_sms_request, post_email_request, @@ -109,17 +104,12 @@ def post_precompiled_letter_notification(): letter_data=form, api_key=api_user, template=template, + template_with_content=None, # not required for precompiled reply_to_text=reply_to, precompiled=True ) - resp = { - 'id': notification.id, - 'reference': notification.client_reference, - 'postage': notification.postage - } - - return jsonify(resp), 201 + return jsonify(notification), 201 @v2_notification_blueprint.route('/', methods=['POST']) @@ -152,12 +142,13 @@ def post_notification(notification_type): ) reply_to = get_reply_to_text(notification_type, form, template) - + if notification_type == LETTER_TYPE: notification = process_letter_notification( letter_data=form, api_key=api_user, template=template, + template_with_content=template_with_content, reply_to_text=reply_to ) else: @@ -174,34 +165,6 @@ def post_notification(notification_type): return jsonify(notification), 201 -def create_response_for_post_notification(notification_id, client_reference, template_id, template_version, service_id, - notification_type, reply_to, scheduled_for, - template_with_content): - if notification_type == SMS_TYPE: - create_resp_partial = functools.partial( - create_post_sms_response_from_notification_detached, - from_number=reply_to, - ) - elif notification_type == EMAIL_TYPE: - create_resp_partial = functools.partial( - create_post_email_response_from_notification_detached, - subject=template_with_content.subject, - email_from='{}@{}'.format(authenticated_service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']), - ) - elif notification_type == LETTER_TYPE: - create_resp_partial = functools.partial( - create_post_letter_response_from_notification_detached, - subject=template_with_content.subject, - ) - resp = create_resp_partial( - notification_id, client_reference, template_id, template_version, service_id, - url_root=request.url_root, - scheduled_for=scheduled_for, - content=template_with_content.content_with_placeholders_filled_in, - ) - return resp - - def process_sms_or_email_notification( *, form, notification_type, api_key, template, template_process_type, service, reply_to_text=None ): @@ -221,9 +184,11 @@ def process_sms_or_email_notification( service, simulated=simulated ) - + if document_download_count: + # We changed personalisation which means we need to update the content + template.values = personalisation key_type = api_key.key_type - service_in_research_mode = service.resear + service_in_research_mode = service.research_mode resp = create_response_for_post_notification( notification_id=notification_id, client_reference=form.get('reference', None), @@ -232,7 +197,7 @@ def process_sms_or_email_notification( service_id=service.id, notification_type=notification_type, reply_to=reply_to_text, - scheduled_for=None, + scheduled_for=form.get("scheduled_for", None), template_with_content=template) if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE') and api_key.key_type == KEY_TYPE_NORMAL \ @@ -242,7 +207,7 @@ def process_sms_or_email_notification( # the task will then save the notification, then call send_notification_to_queue. # We know that this team does not use the GET request, but relies on callbacks to get the status updates. try: - notification = save_email_to_queue( + save_email_to_queue( form=form, notification_id=str(notification_id), notification_type=notification_type, @@ -253,7 +218,7 @@ def process_sms_or_email_notification( document_download_count=document_download_count, reply_to_text=reply_to_text ) - return notification + return resp except SQSError: # if SQS cannot put the task on the queue, it's probably because the notification body was too long and it # went over SQS's 256kb message limit. If so, we @@ -287,7 +252,7 @@ def process_sms_or_email_notification( key_type=key_type, notification_type=notification_type, notification_id=notification_id, - research_mode=False, # research_mode is a deprecated mode + research_mode=service_in_research_mode, # research_mode is deprecated queue=queue_name ) else: @@ -311,7 +276,7 @@ def save_email_to_queue( data = { "id": notification_id, "template_id": str(template.id), - "template_version": template.version, + "template_version": template._template['version'], "to": form['email_address'], "service_id": str(service_id), "personalisation": personalisation, @@ -362,7 +327,9 @@ def process_document_uploads(personalisation_data, service, simulated=False): return personalisation_data, len(file_keys) -def process_letter_notification(*, letter_data, api_key, template, reply_to_text, precompiled=False): +def process_letter_notification( + *, letter_data, api_key, template, template_with_content, reply_to_text, precompiled=False +): if api_key.key_type == KEY_TYPE_TEAM: raise BadRequestError(message='Cannot send letters with a team api key', status_code=403) @@ -375,34 +342,19 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text template=template, reply_to_text=reply_to_text) - address = PostalAddress.from_personalisation( - letter_data['personalisation'], - allow_international_letters=api_key.service.has_permission(INTERNATIONAL_LETTERS), - ) - - if not address.has_enough_lines: - raise ValidationError( - message=f'Address must be at least {PostalAddress.MIN_LINES} lines' - ) - - if address.has_too_many_lines: - raise ValidationError( - message=f'Address must be no more than {PostalAddress.MAX_LINES} lines' - ) - - if not address.has_valid_last_line: - if address.allow_international_letters: - raise ValidationError( - message=f'Last line of address must be a real UK postcode or another country' - ) - raise ValidationError( - message='Must be a real UK postcode' - ) + validate_address(api_key, letter_data) test_key = api_key.key_type == KEY_TYPE_TEST - # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up - status = NOTIFICATION_CREATED if not test_key else NOTIFICATION_SENDING + status = NOTIFICATION_CREATED + if test_key: + # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up + if current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']: + status = NOTIFICATION_SENDING + # mark test letter as delivered and do not create a fake response later + else: + status = NOTIFICATION_DELIVERED + queue = QueueNames.CREATE_LETTERS_PDF if not test_key else QueueNames.RESEARCH_MODE notification = create_letter_notification(letter_data=letter_data, @@ -416,16 +368,46 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text queue=queue ) - if test_key: - if current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']: - create_fake_letter_response_file.apply_async( - (notification.reference,), - queue=queue - ) - else: - update_notification_status_by_reference(notification.reference, NOTIFICATION_DELIVERED) + if test_key and current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']: + create_fake_letter_response_file.apply_async( + (notification.reference,), + queue=queue + ) + resp = create_response_for_post_notification( + notification_id=notification.id, + client_reference=notification.client_reference, + template_id=notification.template_id, + template_version=notification.template_version, + notification_type=notification.notification_type, + reply_to=reply_to_text, + scheduled_for=letter_data.get('scheduled_for', None), + service_id=notification.service_id, + template_with_content=template_with_content + ) + return resp - return notification + +def validate_address(api_key, letter_data): + address = PostalAddress.from_personalisation( + letter_data['personalisation'], + allow_international_letters=api_key.service.has_permission(INTERNATIONAL_LETTERS), + ) + if not address.has_enough_lines: + raise ValidationError( + message=f'Address must be at least {PostalAddress.MIN_LINES} lines' + ) + if address.has_too_many_lines: + raise ValidationError( + message=f'Address must be no more than {PostalAddress.MAX_LINES} lines' + ) + if not address.has_valid_last_line: + if address.allow_international_letters: + raise ValidationError( + message=f'Last line of address must be a real UK postcode or another country' + ) + raise ValidationError( + message='Must be a real UK postcode' + ) def process_precompiled_letter_notifications(*, letter_data, api_key, template, reply_to_text): @@ -441,6 +423,12 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, template, status=status, reply_to_text=reply_to_text) + resp = { + 'id': notification.id, + 'reference': notification.client_reference, + 'postage': notification.postage + } + filename = upload_letter_pdf(notification, letter_content, precompiled=True) current_app.logger.info('Calling task scan-file for {}'.format(filename)) @@ -459,7 +447,7 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, template, queue=QueueNames.LETTERS ) - return notification + return resp def get_reply_to_text(notification_type, form, template): @@ -484,3 +472,31 @@ def get_reply_to_text(notification_type, form, template): reply_to = template.get_reply_to_text() return reply_to + + +def create_response_for_post_notification(notification_id, client_reference, template_id, template_version, service_id, + notification_type, reply_to, scheduled_for, + template_with_content): + if notification_type == SMS_TYPE: + create_resp_partial = functools.partial( + create_post_sms_response_from_notification, + from_number=reply_to, + ) + elif notification_type == EMAIL_TYPE: + create_resp_partial = functools.partial( + create_post_email_response_from_notification, + subject=template_with_content.subject, + email_from='{}@{}'.format(authenticated_service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']), + ) + elif notification_type == LETTER_TYPE: + create_resp_partial = functools.partial( + create_post_letter_response_from_notification, + subject=template_with_content.subject, + ) + resp = create_resp_partial( + notification_id, client_reference, template_id, template_version, service_id, + url_root=request.url_root, + scheduled_for=scheduled_for, + content=template_with_content.content_with_placeholders_filled_in, + ) + return resp diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index f83470a10..52f5fbfea 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -23,7 +23,7 @@ from app.notifications.process_notifications import ( ) from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address from app.v2.errors import BadRequestError -from tests.app.db import create_service, create_template +from tests.app.db import create_service, create_template, create_api_key def test_create_content_for_notification_passes(sample_email_template): @@ -51,8 +51,7 @@ def test_create_content_for_notification_allows_additional_personalisation(sampl @freeze_time("2016-01-01 11:09:00.061258") -def test_persist_notification_creates_and_save_to_db(sample_template, sample_api_key, sample_job, mocker): - mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.get') +def test_persist_notification_creates_and_save_to_db(sample_template, sample_api_key, sample_job): assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 @@ -91,8 +90,6 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api assert notification_from_db.created_by_id == notification.created_by_id assert notification_from_db.reply_to_text == sample_template.service.get_default_sms_sender() - mocked_redis.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count") - def test_persist_notification_throws_exception_when_missing_template(sample_api_key): assert Notification.query.count() == 0 @@ -157,10 +154,9 @@ def test_persist_notification_does_not_increment_cache_if_test_key( @freeze_time("2016-01-01 11:09:00.061258") -def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker): +def test_persist_notification_with_optionals(sample_job, sample_api_key): assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 - mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.get') n_id = uuid.uuid4() created_at = datetime.datetime(2016, 11, 11, 16, 8, 18) persist_notification( @@ -186,7 +182,7 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) persisted_notification.job_id == sample_job.id assert persisted_notification.job_row_number == 10 assert persisted_notification.created_at == created_at - mocked_redis.assert_called_once_with(str(sample_job.service_id) + "-2016-01-01-count") + assert persisted_notification.client_reference == "ref from client" assert persisted_notification.reference is None assert persisted_notification.international is False @@ -197,44 +193,77 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) @freeze_time("2016-01-01 11:09:00.061258") -def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(sample_template, sample_api_key, mocker): +def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(notify_db_session, mocker): + service = create_service(restricted=True) + template = create_template(service=service) + api_key = create_api_key(service=service) mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr') mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=None) mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value=None) persist_notification( - template_id=sample_template.id, - template_version=sample_template.version, + template_id=template.id, + template_version=template.version, recipient='+447111111111', - service=sample_template.service, + service=template.service, personalisation={}, notification_type='sms', - api_key_id=sample_api_key.id, - key_type=sample_api_key.key_type, + api_key_id=api_key.id, + key_type=api_key.key_type, reference="ref" ) mock_incr.assert_not_called() @freeze_time("2016-01-01 11:09:00.061258") -def test_persist_notification_increments_cache_if_key_exists(sample_template, sample_api_key, mocker): +def test_persist_notification_increments_cache_if_key_exists_and_for_trial_service( + notify_db_session, mocker +): + service = create_service(restricted=True) + template = create_template(service=service) + api_key = create_api_key(service=service) mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr') mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1) mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', - return_value={sample_template.id, 1}) + return_value={template.id, 1}) persist_notification( - template_id=sample_template.id, - template_version=sample_template.version, + template_id=template.id, + template_version=template.version, recipient='+447111111122', - service=sample_template.service, + service=template.service, personalisation={}, notification_type='sms', - api_key_id=sample_api_key.id, - key_type=sample_api_key.key_type, + api_key_id=api_key.id, + key_type=api_key.key_type, reference="ref2") - mock_incr.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count", ) + mock_incr.assert_called_once_with(str(service.id) + "-2016-01-01-count", ) + + +def test_persist_notification_does_not_increments_cache_live_service( + notify_db_session, mocker +): + service = create_service(restricted=False) + template = create_template(service=service) + api_key = create_api_key(service=service) + mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr') + mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1) + mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', + return_value={template.id, 1}) + + persist_notification( + template_id=template.id, + template_version=template.version, + recipient='+447111111122', + service=template.service, + personalisation={}, + notification_type='sms', + api_key_id=api_key.id, + key_type=api_key.key_type, + reference="ref2") + + assert not mock_incr.called @pytest.mark.parametrize(( diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index b4618844d..2fcfcaafb 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -426,7 +426,7 @@ def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( ): sample = create_template(service=sample_service, template_type=notification_type) persist_mock = mocker.patch('app.v2.notifications.post_notifications.persist_notification') - deliver_mock = mocker.patch('app.v2.notifications.post_notifications.send_notification_to_queue') + deliver_mock = mocker.patch('app.v2.notifications.post_notifications.send_notification_to_queue_detached') mocker.patch( 'app.v2.notifications.post_notifications.check_rate_limiting', side_effect=RateLimitError("LIMIT", "INTERVAL", "TYPE")) From e22f61977fd13a13b7bacc8bbe8b8d443674a3d2 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 17 Jun 2020 12:13:49 +0100 Subject: [PATCH 3/4] Remove print stmts --- tests/app/v2/notifications/test_post_notifications.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 2fcfcaafb..4ccbf6096 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -44,15 +44,14 @@ def test_post_sms_notification_returns_201(client, sample_template_with_placehol if reference: data.update({"reference": reference}) auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) - print("********* Start") + response = client.post( path='/v2/notifications/sms', data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) - print("********* End") + assert response.status_code == 201 resp_json = json.loads(response.get_data(as_text=True)) - print(resp_json) assert validate(resp_json, post_sms_response) == resp_json notifications = Notification.query.all() assert len(notifications) == 1 From a8661fbd4b7072904c40a250bff74a64924b20d1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 17 Jun 2020 13:48:48 +0100 Subject: [PATCH 4/4] Add None test back --- tests/app/v2/notifications/test_post_notifications.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 4ccbf6096..7fe8acc0a 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -33,7 +33,7 @@ from tests.app.db import ( ) -@pytest.mark.parametrize("reference", ["reference_from_client"]) +@pytest.mark.parametrize("reference", [None, "reference_from_client"]) def test_post_sms_notification_returns_201(client, sample_template_with_placeholders, mocker, reference): mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') data = {