diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 04b38d847..d84424333 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -133,7 +133,7 @@ def send_sms(self, saved_notification = persist_notification(template_id=notification['template'], template_version=notification['template_version'], recipient=notification['to'], - service_id=service.id, + service=service, personalisation=notification.get('personalisation'), notification_type=SMS_TYPE, api_key_id=api_key_id, @@ -195,7 +195,7 @@ def send_email(self, service_id, template_id=notification['template'], template_version=notification['template_version'], recipient=notification['to'], - service_id=service.id, + service=service, personalisation=notification.get('personalisation'), notification_type=EMAIL_TYPE, api_key_id=api_key_id, diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d0fd17e8e..8ab6b0dd0 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -129,9 +129,17 @@ def dao_create_notification(notification): if not notification.status: notification.status = 'created' - notification_history = NotificationHistory.from_original(notification) db.session.add(notification) - db.session.add(notification_history) + if _should_record_notification_in_history_table(notification): + db.session.add(NotificationHistory.from_original(notification)) + + +def _should_record_notification_in_history_table(notification): + if notification.api_key_id and notification.key_type == KEY_TYPE_TEST: + return False + if notification.service.research_mode: + return False + return True def _decide_permanent_temporary_failure(current_status, status): @@ -188,9 +196,11 @@ def update_notification_status_by_reference(reference, status): @statsd(namespace="dao") def dao_update_notification(notification): notification.updated_at = datetime.utcnow() - notification_history = NotificationHistory.query.get(notification.id) - notification_history.update_from_original(notification) db.session.add(notification) + if _should_record_notification_in_history_table(notification): + notification_history = NotificationHistory.query.get(notification.id) + notification_history.update_from_original(notification) + db.session.add(notification_history) db.session.commit() diff --git a/app/invite/rest.py b/app/invite/rest.py index a35be3718..2361629d1 100644 --- a/app/invite/rest.py +++ b/app/invite/rest.py @@ -10,7 +10,7 @@ from app.dao.invited_user_dao import ( get_invited_users_for_service ) from app.dao.templates_dao import dao_get_template_by_id -from app.models import EMAIL_TYPE, KEY_TYPE_NORMAL +from app.models import EMAIL_TYPE, KEY_TYPE_NORMAL, Service from app.notifications.process_notifications import persist_notification, send_notification_to_queue from app.schemas import invited_user_schema @@ -27,12 +27,13 @@ def create_invited_user(service_id): save_invited_user(invited_user) template = dao_get_template_by_id(current_app.config['INVITATION_EMAIL_TEMPLATE_ID']) + service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) saved_notification = persist_notification( template_id=template.id, template_version=template.version, recipient=invited_user.email_address, - service_id=current_app.config['NOTIFY_SERVICE_ID'], + service=service, personalisation={ 'user_name': invited_user.from_user.name, 'service_name': invited_user.service.name, diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 8ba4995d4..81d37df2b 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -32,7 +32,7 @@ def check_placeholders(template_object): def persist_notification(template_id, template_version, recipient, - service_id, + service, personalisation, notification_type, api_key_id, @@ -47,7 +47,8 @@ def persist_notification(template_id, template_id=template_id, template_version=template_version, to=recipient, - service_id=service_id, + service_id=service.id, + service=service, personalisation=personalisation, notification_type=notification_type, api_key_id=api_key_id, @@ -58,7 +59,7 @@ def persist_notification(template_id, client_reference=reference ) dao_create_notification(notification) - redis_store.incr(redis.daily_limit_cache_key(service_id)) + redis_store.incr(redis.daily_limit_cache_key(service.id)) return notification diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 8ea7f61da..2af3cd981 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -238,7 +238,7 @@ def send_notification(notification_type): saved_notification = persist_notification(template_id=template.id, template_version=template.version, recipient=notification['to'], - service_id=service.id, + service=service, personalisation=notification.get('personalisation', None), notification_type=notification_type, api_key_id=api_user.id, diff --git a/app/user/rest.py b/app/user/rest.py index 63d34384b..cd7726ae2 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -16,7 +16,7 @@ from app.dao.users_dao import ( from app.dao.permissions_dao import permission_dao from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id -from app.models import SMS_TYPE, KEY_TYPE_NORMAL, EMAIL_TYPE +from app.models import SMS_TYPE, KEY_TYPE_NORMAL, EMAIL_TYPE, Service from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue @@ -139,13 +139,13 @@ def send_user_sms_code(user_id): mobile = user_to_send_to.mobile_number if verify_code.get('to', None) is None else verify_code.get('to') sms_code_template_id = current_app.config['SMS_CODE_TEMPLATE_ID'] sms_code_template = dao_get_template_by_id(sms_code_template_id) - notify_service_id = current_app.config['NOTIFY_SERVICE_ID'] + service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) saved_notification = persist_notification( template_id=sms_code_template_id, template_version=sms_code_template.version, recipient=mobile, - service_id=notify_service_id, + service=service, personalisation={'verify_code': secret_code}, notification_type=SMS_TYPE, api_key_id=None, @@ -167,13 +167,13 @@ def send_user_confirm_new_email(user_id): raise InvalidRequest(message=errors, status_code=400) template = dao_get_template_by_id(current_app.config['CHANGE_EMAIL_CONFIRMATION_TEMPLATE_ID']) - notify_service_id = current_app.config['NOTIFY_SERVICE_ID'] + service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) saved_notification = persist_notification( template_id=template.id, template_version=template.version, recipient=email['email'], - service_id=notify_service_id, + service=service, personalisation={ 'name': user_to_send_to.name, 'url': _create_confirmation_url(user=user_to_send_to, email_address=email['email']), @@ -195,12 +195,13 @@ def send_user_email_verification(user_id): create_user_code(user_to_send_to, secret_code, 'email') template = dao_get_template_by_id(current_app.config['EMAIL_VERIFY_CODE_TEMPLATE_ID']) + service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) saved_notification = persist_notification( template_id=template.id, template_version=template.version, recipient=user_to_send_to.email_address, - service_id=current_app.config['NOTIFY_SERVICE_ID'], + service=service, personalisation={ 'name': user_to_send_to.name, 'url': _create_verification_url(user_to_send_to, secret_code) @@ -219,12 +220,13 @@ def send_user_email_verification(user_id): def send_already_registered_email(user_id): to, errors = email_data_request_schema.load(request.get_json()) template = dao_get_template_by_id(current_app.config['ALREADY_REGISTERED_EMAIL_TEMPLATE_ID']) + service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) saved_notification = persist_notification( template_id=template.id, template_version=template.version, recipient=to['email'], - service_id=current_app.config['NOTIFY_SERVICE_ID'], + service=service, personalisation={ 'signin_url': current_app.config['ADMIN_BASE_URL'] + '/sign-in', 'forgot_password_url': current_app.config['ADMIN_BASE_URL'] + '/forgot-password', @@ -282,12 +284,12 @@ def send_user_reset_password(): user_to_send_to = get_user_by_email(email['email']) template = dao_get_template_by_id(current_app.config['PASSWORD_RESET_TEMPLATE_ID']) - + service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) saved_notification = persist_notification( template_id=template.id, template_version=template.version, recipient=email['email'], - service_id=current_app.config['NOTIFY_SERVICE_ID'], + service=service, personalisation={ 'user_name': user_to_send_to.name, 'url': _create_reset_password_url(user_to_send_to.email_address) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 524fd6a99..ed68f8e8b 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -33,7 +33,7 @@ def post_sms_notification(): notification = persist_notification(template_id=template.id, template_version=template.version, recipient=form['phone_number'], - service_id=service.id, + service=service, personalisation=form.get('personalisation', None), notification_type=SMS_TYPE, api_key_id=api_user.id, @@ -61,7 +61,7 @@ def post_email_notification(): notification = persist_notification(template_id=template.id, template_version=template.version, recipient=form['email_address'], - service_id=service.id, + service=service, personalisation=form.get('personalisation', None), notification_type=EMAIL_TYPE, api_key_id=api_user.id, diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 50777beb6..a6b75196f 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -42,6 +42,8 @@ from app.dao.notifications_dao import ( get_financial_year, get_april_fools) +from app.dao.services_dao import dao_update_service + from tests.app.conftest import (sample_notification, sample_template, sample_email_template, sample_service, sample_job, sample_api_key) @@ -519,19 +521,82 @@ def test_save_notification(sample_email_template, sample_job, ses_provider): assert Notification.query.count() == 2 -def test_save_notification(sample_template, sample_job, mmg_provider): +def test_save_notification_creates_history(sample_email_template, sample_job): assert Notification.query.count() == 0 - data = _notification_json(sample_template, sample_job.id) + data = _notification_json(sample_email_template, job_id=sample_job.id) notification_1 = Notification(**data) - notification_2 = Notification(**data) dao_create_notification(notification_1) assert Notification.query.count() == 1 + assert NotificationHistory.query.count() == 1 - dao_create_notification(notification_2) - assert Notification.query.count() == 2 +def test_save_notification_with_test_api_key_does_not_create_history(sample_email_template, sample_api_key): + assert Notification.query.count() == 0 + data = _notification_json(sample_email_template) + data['key_type'] = KEY_TYPE_TEST + data['api_key_id'] = sample_api_key.id + + notification_1 = Notification(**data) + dao_create_notification(notification_1) + + assert Notification.query.count() == 1 + assert NotificationHistory.query.count() == 0 + + +def test_save_notification_with_research_mode_service_does_not_create_history( + notify_db, + notify_db_session): + service = sample_service(notify_db, notify_db_session) + service.research_mode = True + dao_update_service(service) + template = sample_template(notify_db, notify_db_session, service=service) + + assert Notification.query.count() == 0 + data = _notification_json(template) + notification = Notification(**data) + dao_create_notification(notification) + assert Notification.query.count() == 1 + assert NotificationHistory.query.count() == 0 + + +def test_update_notification_with_test_api_key_does_not_update_or_create_history(sample_email_template, sample_api_key): + assert Notification.query.count() == 0 + data = _notification_json(sample_email_template) + data['key_type'] = KEY_TYPE_TEST + data['api_key_id'] = sample_api_key.id + + notification = Notification(**data) + dao_create_notification(notification) + + notification.status = 'delivered' + dao_update_notification(notification) + + assert Notification.query.one().status == 'delivered' + assert NotificationHistory.query.count() == 0 + + +def test_update_notification_with_research_mode_service_does_not_create_or_update_history( + notify_db, + notify_db_session): + service = sample_service(notify_db, notify_db_session) + service.research_mode = True + dao_update_service(service) + template = sample_template(notify_db, notify_db_session, service=service) + + data = _notification_json(template) + notification = Notification(**data) + dao_create_notification(notification) + + assert Notification.query.count() == 1 + assert NotificationHistory.query.count() == 0 + + notification.status = 'delivered' + dao_update_notification(notification) + + assert Notification.query.one().status == 'delivered' + assert NotificationHistory.query.count() == 0 def test_not_save_notification_and_not_create_stats_on_commit_error(sample_template, sample_job, mmg_provider): @@ -721,22 +786,10 @@ def test_get_notification_billable_unit_count_per_month(notify_db, notify_db_ses ) for financial_year, months in ( - ( - 2017, - [] - ), - ( - 2016, - [('April', 3), ('July', 2), ('October', 1), ('January', 1)] - ), - ( - 2015, - [('April', 1), ('November', 1), ('January', 1)] - ), - ( - 2014, - [] - ) + (2017, []), + (2016, [('April', 3), ('July', 2), ('October', 1), ('January', 1)]), + (2015, [('April', 1), ('November', 1), ('January', 1)]), + (2014, []) ): assert get_notification_billable_unit_count_per_month( sample_service.id, financial_year @@ -844,10 +897,12 @@ def test_updating_notification_updates_notification_history(sample_notification) sample_notification.status = 'sending' dao_update_notification(sample_notification) - - hist = NotificationHistory.query.one() - assert hist.id == sample_notification.id - assert hist.status == 'sending' + notification = Notification.query.one() + hist1 = NotificationHistory.query.one() + assert notification.id == sample_notification.id + assert notification.status == "sending" + assert hist1.id == sample_notification.id + assert hist1.status == 'sending' def test_should_delete_notification_and_notification_history_for_id(notify_db, notify_db_session, sample_template): @@ -865,6 +920,46 @@ def test_should_delete_notification_and_notification_history_for_id(notify_db, n assert NotificationHistory.query.count() == 0 +def test_should_delete_notification_and_ignore_history_for_test_api( + notify_db, + notify_db_session, + sample_email_template, + sample_api_key): + data = _notification_json(sample_email_template) + data['key_type'] = KEY_TYPE_TEST + data['api_key_id'] = sample_api_key.id + + notification = Notification(**data) + dao_create_notification(notification) + + assert Notification.query.count() == 1 + assert NotificationHistory.query.count() == 0 + + dao_delete_notifications_and_history_by_id(notification.id) + + assert Notification.query.count() == 0 + assert NotificationHistory.query.count() == 0 + + +def test_should_delete_notification_and_ignore_history_for_research_mode(notify_db, notify_db_session): + service = sample_service(notify_db, notify_db_session) + service.research_mode = True + dao_update_service(service) + template = sample_template(notify_db, notify_db_session, service=service) + + data = _notification_json(template) + notification = Notification(**data) + dao_create_notification(notification) + + assert Notification.query.count() == 1 + assert NotificationHistory.query.count() == 0 + + dao_delete_notifications_and_history_by_id(notification.id) + + assert Notification.query.count() == 0 + assert NotificationHistory.query.count() == 0 + + def test_should_delete_only_notification_and_notification_history_with_id(notify_db, notify_db_session, sample_template): id_1 = uuid.uuid4() @@ -1022,7 +1117,6 @@ def test_get_notifications_created_by_api_or_csv_are_returned_correctly_excludin sample_team_api_key, sample_test_api_key ): - sample_notification( notify_db, notify_db_session, created_at=datetime.utcnow(), job=sample_job ) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 95b0d2cb9..bb3b1d8b6 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -48,7 +48,7 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 notification = persist_notification(sample_template.id, sample_template.version, '+447111111111', - sample_template.service.id, {}, 'sms', sample_api_key.id, + sample_template.service, {}, 'sms', sample_api_key.id, sample_api_key.key_type, job_id=sample_job.id, job_row_number=100, reference="ref") @@ -84,7 +84,7 @@ def test_persist_notification_throws_exception_when_missing_template(sample_api_ persist_notification(template_id=None, template_version=None, recipient='+447111111111', - service_id=sample_api_key.service_id, + service=sample_api_key.service, personalisation=None, notification_type='sms', api_key_id=sample_api_key.id, @@ -102,7 +102,7 @@ def test_exception_thown_by_redis_store_get_should_not_be_fatal(sample_template, sample_template.id, sample_template.version, '+447111111111', - sample_template.service.id, + sample_template.service, {}, 'sms', sample_api_key.id, @@ -118,7 +118,7 @@ def test_cache_is_not_incremented_on_failure_to_persist_notification(sample_api_ persist_notification(template_id=None, template_version=None, recipient='+447111111111', - service_id=sample_api_key.service_id, + service=sample_api_key.service, personalisation=None, notification_type='sms', api_key_id=sample_api_key.id, @@ -136,7 +136,7 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) persist_notification(template_id=sample_job.template.id, template_version=sample_job.template.version, recipient='+447111111111', - service_id=sample_job.service.id, + service=sample_job.service, personalisation=None, notification_type='sms', api_key_id=sample_api_key.id, key_type=sample_api_key.key_type,