diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 3e8be9a89..2700775ec 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -120,13 +120,17 @@ 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)) + # 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,35 +138,43 @@ 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): if notification_type == SMS_TYPE: formatted_simulated_numbers = [ diff --git a/app/v2/notifications/create_response.py b/app/v2/notifications/create_response.py index b2852b2d5..36de9c724 100644 --- a/app/v2/notifications/create_response.py +++ b/app/v2/notifications/create_response.py @@ -1,45 +1,70 @@ -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'] = { +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( + notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for + ) + resp['content'] = { 'from_number': from_number, 'body': content } - return noti + return resp -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'] = { +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'] = { "from_email": email_from, "body": content, "subject": subject } - return noti + return resp -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'] = { +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( + notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for + ) + resp['content'] = { "body": content, "subject": subject } - return noti + return resp -def __create_notification_response(notification, url_root, scheduled_for): +def __create_notification_response( + notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for +): return { - "id": notification.id, - "reference": notification.client_reference, - "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)), + "id": notification_id, + "reference": client_reference, + "uri": "{}v2/notifications/{}".format(url_root, str(notification_id)), 'template': { - "id": notification.template_id, - "version": notification.template_version, + "id": template_id, + "version": template_version, "uri": "{}services/{}/templates/{}".format( url_root, - str(notification.service_id), - str(notification.template_id) + 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..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,9 +44,8 @@ from app.notifications.process_letter_notifications import ( 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, @@ -62,10 +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, 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, @@ -108,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']) @@ -157,6 +148,7 @@ def post_notification(notification_type): letter_data=form, api_key=api_user, template=template, + template_with_content=template_with_content, reply_to_text=reply_to ) else: @@ -164,41 +156,19 @@ 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 - - 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=notification, - url_root=request.url_root, - scheduled_for=scheduled_for, - content=template_with_content.content_with_placeholders_filled_in, - ) - return jsonify(resp), 201 + return jsonify(notification), 201 -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, @@ -214,6 +184,21 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ 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.research_mode + 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=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 \ and notification_type == EMAIL_TYPE: @@ -222,8 +207,7 @@ 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( + save_email_to_queue( form=form, notification_id=str(notification_id), notification_type=notification_type, @@ -234,7 +218,7 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ 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 @@ -242,16 +226,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 +244,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=service_in_research_mode, # research_mode is deprecated 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( @@ -290,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, @@ -341,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) @@ -354,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, @@ -395,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): @@ -420,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)) @@ -438,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): @@ -463,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 53e31f13d..7fe8acc0a 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -49,6 +49,7 @@ def test_post_sms_notification_returns_201(client, sample_template_with_placehol path='/v2/notifications/sms', data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 201 resp_json = json.loads(response.get_data(as_text=True)) assert validate(resp_json, post_sms_response) == resp_json @@ -424,7 +425,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"))