From 2ca7986649797aec72ac5f9a3a249281f86f3c65 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 26 Nov 2024 13:29:18 -0800 Subject: [PATCH 01/49] increase db size from micro to small on staging --- terraform/staging/main.tf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terraform/staging/main.tf b/terraform/staging/main.tf index 4fdbf9e38..d59b063ea 100644 --- a/terraform/staging/main.tf +++ b/terraform/staging/main.tf @@ -18,7 +18,7 @@ module "database" { cf_org_name = local.cf_org_name cf_space_name = local.cf_space_name name = "${local.app_name}-rds-${local.env}" - rds_plan_name = "micro-psql" + rds_plan_name = "small-psql" } module "redis-v70" { From 12844bc806305b4070bbe75065a3b17a21c77678 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 26 Nov 2024 14:24:03 -0800 Subject: [PATCH 02/49] upgrade demo and production dbs --- terraform/demo/main.tf | 2 +- terraform/production/main.tf | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/terraform/demo/main.tf b/terraform/demo/main.tf index ea0f259e4..1a9e091db 100644 --- a/terraform/demo/main.tf +++ b/terraform/demo/main.tf @@ -18,7 +18,7 @@ module "database" { cf_org_name = local.cf_org_name cf_space_name = local.cf_space_name name = "${local.app_name}-rds-${local.env}" - rds_plan_name = "micro-psql" + rds_plan_name = "small-psql" } module "redis-v70" { diff --git a/terraform/production/main.tf b/terraform/production/main.tf index e3bd10a26..466b8d03c 100644 --- a/terraform/production/main.tf +++ b/terraform/production/main.tf @@ -18,7 +18,7 @@ module "database" { cf_org_name = local.cf_org_name cf_space_name = local.cf_space_name name = "${local.app_name}-rds-${local.env}" - rds_plan_name = "small-psql-redundant" + rds_plan_name = "medium-psql-redundant" } module "redis-v70" { From bfbae127a89ced38adf5964b1ca09ae83d68d2a0 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 26 Nov 2024 14:28:20 -0800 Subject: [PATCH 03/49] change to gp --- terraform/production/main.tf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terraform/production/main.tf b/terraform/production/main.tf index 466b8d03c..9543fdd86 100644 --- a/terraform/production/main.tf +++ b/terraform/production/main.tf @@ -18,7 +18,7 @@ module "database" { cf_org_name = local.cf_org_name cf_space_name = local.cf_space_name name = "${local.app_name}-rds-${local.env}" - rds_plan_name = "medium-psql-redundant" + rds_plan_name = "medium-gp-psql-redundant" } module "redis-v70" { From 12776354e399f4863ea1bc3af973312d39707925 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 27 Nov 2024 08:36:52 -0800 Subject: [PATCH 04/49] fix broken go live email notification --- app/celery/provider_tasks.py | 14 ++++++++++---- app/service/sender.py | 13 +++++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 011b00d98..9165fe37b 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -19,7 +19,7 @@ from app.dao.notifications_dao import ( from app.delivery import send_to_providers from app.enums import NotificationStatus from app.exceptions import NotificationTechnicalFailureException -from app.utils import utc_now +from app.utils import hilite, utc_now # This is the amount of time to wait after sending an sms message before we check the aws logs and look for delivery # receipts @@ -170,7 +170,7 @@ def deliver_sms(self, notification_id): @notify_celery.task( - bind=True, name="deliver_email", max_retries=48, default_retry_delay=300 + bind=True, name="deliver_email", max_retries=48, default_retry_delay=30 ) def deliver_email(self, notification_id): try: @@ -182,9 +182,15 @@ def deliver_email(self, notification_id): if not notification: raise NoResultFound() personalisation = redis_store.get(f"email-personalisation-{notification_id}") + recipient = redis_store.get(f"email-recipient-{notification_id}") + if personalisation: + notification.personalisation = json.loads(personalisation) + if recipient: + notification.recipient = json.loads(recipient) - notification.personalisation = json.loads(personalisation) - send_to_providers.send_email_to_provider(notification) + print(hilite(f"HERE IS THE NOTIFICATION {notification.serialize_for_csv()}")) + if recipient: + send_to_providers.send_email_to_provider(notification) except EmailClientNonRetryableException: current_app.logger.exception(f"Email notification {notification_id} failed") update_notification_status_by_id(notification_id, "technical-failure") diff --git a/app/service/sender.py b/app/service/sender.py index 4b954f60b..4661dbb9b 100644 --- a/app/service/sender.py +++ b/app/service/sender.py @@ -1,5 +1,8 @@ +import json + from flask import current_app +from app import redis_store from app.config import QueueNames from app.dao.services_dao import ( dao_fetch_active_users_for_service, @@ -40,7 +43,17 @@ def send_notification_to_service_users( key_type=KeyType.NORMAL, reply_to_text=notify_service.get_default_reply_to_email_address(), ) + redis_store.set( + f"email-personalisation-{notification.id}", + json.dumps(personalisation), + ex=24 * 60 * 60, + ) + redis_store.set( + f"email-recipient-{notification.id}", notification.to, ex=24 * 60 * 60 + ) + send_notification_to_queue(notification, queue=QueueNames.NOTIFY) + return notification def _add_user_fields(user, personalisation, fields): From 639d2e23354c0c0ba00d31c7886b99fd224f4764 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 27 Nov 2024 08:50:12 -0800 Subject: [PATCH 05/49] fix broken go live email notification --- app/celery/provider_tasks.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 9165fe37b..b0c6a4c9b 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -19,7 +19,7 @@ from app.dao.notifications_dao import ( from app.delivery import send_to_providers from app.enums import NotificationStatus from app.exceptions import NotificationTechnicalFailureException -from app.utils import hilite, utc_now +from app.utils import utc_now # This is the amount of time to wait after sending an sms message before we check the aws logs and look for delivery # receipts @@ -188,9 +188,7 @@ def deliver_email(self, notification_id): if recipient: notification.recipient = json.loads(recipient) - print(hilite(f"HERE IS THE NOTIFICATION {notification.serialize_for_csv()}")) - if recipient: - send_to_providers.send_email_to_provider(notification) + send_to_providers.send_email_to_provider(notification) except EmailClientNonRetryableException: current_app.logger.exception(f"Email notification {notification_id} failed") update_notification_status_by_id(notification_id, "technical-failure") From a1dea3c6122fcc87121df0fcd555db27a1fb0b2f Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 27 Nov 2024 09:02:34 -0800 Subject: [PATCH 06/49] fix broken go live email notification --- tests/app/service/test_sender.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/app/service/test_sender.py b/tests/app/service/test_sender.py index 4b9c10ee1..d85ac3eea 100644 --- a/tests/app/service/test_sender.py +++ b/tests/app/service/test_sender.py @@ -58,6 +58,7 @@ def test_send_notification_to_service_users_includes_user_fields_in_personalisat ): persist_mock = mocker.patch("app.service.sender.persist_notification") mocker.patch("app.service.sender.send_notification_to_queue") + mocker.patch("app.service.sender.redis_store") user = sample_service.users[0] From d23fece060789c85a9e0450daef63381a2b420da Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 27 Nov 2024 09:16:20 -0800 Subject: [PATCH 07/49] fix broken go live email notification --- tests/app/service/test_sender.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/app/service/test_sender.py b/tests/app/service/test_sender.py index d85ac3eea..d974f1694 100644 --- a/tests/app/service/test_sender.py +++ b/tests/app/service/test_sender.py @@ -83,6 +83,7 @@ def test_send_notification_to_service_users_sends_to_active_users_only( notify_service, mocker ): mocker.patch("app.service.sender.send_notification_to_queue") + mocker.patch("app.service.sender.redis_store") first_active_user = create_user(email="foo@bar.com", state="active") second_active_user = create_user(email="foo1@bar.com", state="active") From 9b392af576fe7d7d8cd7b91db86cde4244b2d4ee Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 27 Nov 2024 10:05:41 -0800 Subject: [PATCH 08/49] fix broken go live email notification --- app/service/sender.py | 3 +++ tests/app/service/test_sender.py | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/app/service/sender.py b/app/service/sender.py index 4661dbb9b..2a8a7775e 100644 --- a/app/service/sender.py +++ b/app/service/sender.py @@ -14,6 +14,7 @@ from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue, ) +from app.utils import hilite def send_notification_to_service_users( @@ -28,6 +29,7 @@ def send_notification_to_service_users( for user in active_users: personalisation = _add_user_fields(user, personalisation, include_user_fields) + print(hilite(f"PERSONALISATION IS {personalisation}")) notification = persist_notification( template_id=template.id, template_version=template.version, @@ -43,6 +45,7 @@ def send_notification_to_service_users( key_type=KeyType.NORMAL, reply_to_text=notify_service.get_default_reply_to_email_address(), ) + print(hilite(f"NOTIFICATION IS {notification.serialize_for_csv()}")) redis_store.set( f"email-personalisation-{notification.id}", json.dumps(personalisation), diff --git a/tests/app/service/test_sender.py b/tests/app/service/test_sender.py index d974f1694..ff1749d2a 100644 --- a/tests/app/service/test_sender.py +++ b/tests/app/service/test_sender.py @@ -83,7 +83,9 @@ def test_send_notification_to_service_users_sends_to_active_users_only( notify_service, mocker ): mocker.patch("app.service.sender.send_notification_to_queue") - mocker.patch("app.service.sender.redis_store") + mocker.patch( + "app.service.sender.redis_store", + ) first_active_user = create_user(email="foo@bar.com", state="active") second_active_user = create_user(email="foo1@bar.com", state="active") From 6c0cbd2d34304e8d034d6575bebf0f81fe1d0471 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 27 Nov 2024 10:19:09 -0800 Subject: [PATCH 09/49] fix broken go live email notification --- app/service/sender.py | 1 + tests/app/service/test_sender.py | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/service/sender.py b/app/service/sender.py index 2a8a7775e..8e03d5964 100644 --- a/app/service/sender.py +++ b/app/service/sender.py @@ -25,6 +25,7 @@ def send_notification_to_service_users( template = dao_get_template_by_id(template_id) service = dao_fetch_service_by_id(service_id) active_users = dao_fetch_active_users_for_service(service.id) + print(hilite(f"ACTIVE USERS ARE {active_users}")) notify_service = dao_fetch_service_by_id(current_app.config["NOTIFY_SERVICE_ID"]) for user in active_users: diff --git a/tests/app/service/test_sender.py b/tests/app/service/test_sender.py index ff1749d2a..a66f1e462 100644 --- a/tests/app/service/test_sender.py +++ b/tests/app/service/test_sender.py @@ -1,9 +1,10 @@ import pytest from flask import current_app +from app.utils import hilite from sqlalchemy import func, select from app import db -from app.dao.services_dao import dao_add_user_to_service +from app.dao.services_dao import dao_add_user_to_service, dao_fetch_active_users_for_service from app.enums import NotificationType, TemplateType from app.models import Notification from app.service.sender import send_notification_to_service_users @@ -91,8 +92,15 @@ def test_send_notification_to_service_users_sends_to_active_users_only( second_active_user = create_user(email="foo1@bar.com", state="active") pending_user = create_user(email="foo2@bar.com", state="pending") service = create_service(user=first_active_user) + print(hilite(f"CREATED THE SERVICE {service} with user {first_active_user}")) dao_add_user_to_service(service, second_active_user) + print(hilite(f"ADDED user {second_active_user}")) + dao_add_user_to_service(service, pending_user) + print(hilite(f"ADDED PENDING USER {pending_user}")) + + active_users = dao_fetch_active_users_for_service(service.id) + print(hilite(f"ACTIVE USERS IN THE TEST {active_users}")) template = create_template(service, template_type=TemplateType.EMAIL) send_notification_to_service_users(service_id=service.id, template_id=template.id) From 4a31b2ece6db0b2f0fa9b5fad32e1b2d6dda7b09 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 27 Nov 2024 10:30:03 -0800 Subject: [PATCH 10/49] fix broken go live email notification --- tests/app/service/test_sender.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/app/service/test_sender.py b/tests/app/service/test_sender.py index a66f1e462..b37e5468c 100644 --- a/tests/app/service/test_sender.py +++ b/tests/app/service/test_sender.py @@ -1,13 +1,16 @@ import pytest from flask import current_app -from app.utils import hilite from sqlalchemy import func, select from app import db -from app.dao.services_dao import dao_add_user_to_service, dao_fetch_active_users_for_service +from app.dao.services_dao import ( + dao_add_user_to_service, + dao_fetch_active_users_for_service, +) from app.enums import NotificationType, TemplateType from app.models import Notification from app.service.sender import send_notification_to_service_users +from app.utils import hilite from tests.app.db import create_service, create_template, create_user From 61b994827098441a520fa08fce0558d2988de839 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 27 Nov 2024 10:46:49 -0800 Subject: [PATCH 11/49] fix broken go live email notification --- app/service/sender.py | 2 ++ tests/app/service/test_sender.py | 4 +--- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/service/sender.py b/app/service/sender.py index 8e03d5964..484126d0f 100644 --- a/app/service/sender.py +++ b/app/service/sender.py @@ -52,9 +52,11 @@ def send_notification_to_service_users( json.dumps(personalisation), ex=24 * 60 * 60, ) + print(hilite("GOT PAST FIRST SET")) redis_store.set( f"email-recipient-{notification.id}", notification.to, ex=24 * 60 * 60 ) + print(hilite("GOT PAST SECOND SET")) send_notification_to_queue(notification, queue=QueueNames.NOTIFY) return notification diff --git a/tests/app/service/test_sender.py b/tests/app/service/test_sender.py index b37e5468c..3cf60588d 100644 --- a/tests/app/service/test_sender.py +++ b/tests/app/service/test_sender.py @@ -87,9 +87,7 @@ def test_send_notification_to_service_users_sends_to_active_users_only( notify_service, mocker ): mocker.patch("app.service.sender.send_notification_to_queue") - mocker.patch( - "app.service.sender.redis_store", - ) + mocker.patch("app.service.sender.redis_store", autospec=True) first_active_user = create_user(email="foo@bar.com", state="active") second_active_user = create_user(email="foo1@bar.com", state="active") From 43eba01bb46b4a86940eb96c517084b8771671fb Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 27 Nov 2024 10:56:13 -0800 Subject: [PATCH 12/49] fix broken go live email notification --- app/service/sender.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/service/sender.py b/app/service/sender.py index 484126d0f..1a3b1400a 100644 --- a/app/service/sender.py +++ b/app/service/sender.py @@ -59,7 +59,7 @@ def send_notification_to_service_users( print(hilite("GOT PAST SECOND SET")) send_notification_to_queue(notification, queue=QueueNames.NOTIFY) - return notification + def _add_user_fields(user, personalisation, fields): From a1155dff7ea755c1a0a85953fe72a91ac436de93 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 27 Nov 2024 11:03:01 -0800 Subject: [PATCH 13/49] fix broken go live email notification --- app/service/sender.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/service/sender.py b/app/service/sender.py index 1a3b1400a..9370db0a9 100644 --- a/app/service/sender.py +++ b/app/service/sender.py @@ -61,7 +61,6 @@ def send_notification_to_service_users( send_notification_to_queue(notification, queue=QueueNames.NOTIFY) - def _add_user_fields(user, personalisation, fields): for field in fields: personalisation[field] = getattr(user, field) From a5b83f5eff673b66c733901e23863dc961c7122a Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 27 Nov 2024 11:12:21 -0800 Subject: [PATCH 14/49] fix broken go live email notification --- app/service/sender.py | 6 ------ tests/app/service/test_sender.py | 11 +---------- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/app/service/sender.py b/app/service/sender.py index 9370db0a9..a769dc4d9 100644 --- a/app/service/sender.py +++ b/app/service/sender.py @@ -14,7 +14,6 @@ from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue, ) -from app.utils import hilite def send_notification_to_service_users( @@ -25,12 +24,10 @@ def send_notification_to_service_users( template = dao_get_template_by_id(template_id) service = dao_fetch_service_by_id(service_id) active_users = dao_fetch_active_users_for_service(service.id) - print(hilite(f"ACTIVE USERS ARE {active_users}")) notify_service = dao_fetch_service_by_id(current_app.config["NOTIFY_SERVICE_ID"]) for user in active_users: personalisation = _add_user_fields(user, personalisation, include_user_fields) - print(hilite(f"PERSONALISATION IS {personalisation}")) notification = persist_notification( template_id=template.id, template_version=template.version, @@ -46,17 +43,14 @@ def send_notification_to_service_users( key_type=KeyType.NORMAL, reply_to_text=notify_service.get_default_reply_to_email_address(), ) - print(hilite(f"NOTIFICATION IS {notification.serialize_for_csv()}")) redis_store.set( f"email-personalisation-{notification.id}", json.dumps(personalisation), ex=24 * 60 * 60, ) - print(hilite("GOT PAST FIRST SET")) redis_store.set( f"email-recipient-{notification.id}", notification.to, ex=24 * 60 * 60 ) - print(hilite("GOT PAST SECOND SET")) send_notification_to_queue(notification, queue=QueueNames.NOTIFY) diff --git a/tests/app/service/test_sender.py b/tests/app/service/test_sender.py index 3cf60588d..d35eb2edc 100644 --- a/tests/app/service/test_sender.py +++ b/tests/app/service/test_sender.py @@ -3,14 +3,10 @@ from flask import current_app from sqlalchemy import func, select from app import db -from app.dao.services_dao import ( - dao_add_user_to_service, - dao_fetch_active_users_for_service, -) +from app.dao.services_dao import dao_add_user_to_service from app.enums import NotificationType, TemplateType from app.models import Notification from app.service.sender import send_notification_to_service_users -from app.utils import hilite from tests.app.db import create_service, create_template, create_user @@ -93,15 +89,10 @@ def test_send_notification_to_service_users_sends_to_active_users_only( second_active_user = create_user(email="foo1@bar.com", state="active") pending_user = create_user(email="foo2@bar.com", state="pending") service = create_service(user=first_active_user) - print(hilite(f"CREATED THE SERVICE {service} with user {first_active_user}")) dao_add_user_to_service(service, second_active_user) - print(hilite(f"ADDED user {second_active_user}")) dao_add_user_to_service(service, pending_user) - print(hilite(f"ADDED PENDING USER {pending_user}")) - active_users = dao_fetch_active_users_for_service(service.id) - print(hilite(f"ACTIVE USERS IN THE TEST {active_users}")) template = create_template(service, template_type=TemplateType.EMAIL) send_notification_to_service_users(service_id=service.id, template_id=template.id) From 791d18b4ecc44cfe01a1b8123d3c4bf2672020ab Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Mon, 2 Dec 2024 11:48:26 -0800 Subject: [PATCH 15/49] fix logic --- app/celery/tasks.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index c8ad8cc6d..63d4fdc09 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -347,14 +347,13 @@ def save_api_email_or_sms(self, encrypted_notification): document_download_count=notification["document_download_count"], ) - provider_task.apply_async([notification["id"]], queue=q) - current_app.logger.debug( - f"{notification['notification_type']} {notification['id']} has been persisted and sent to delivery queue." - ) except IntegrityError: current_app.logger.info( f"{notification['notification_type']} {notification['id']} already exists." ) + # If we don't have the return statement here, we will fall through and end + # up retrying because IntegrityError is a subclass of SQLAlchemyError + return except SQLAlchemyError: try: @@ -363,6 +362,10 @@ def save_api_email_or_sms(self, encrypted_notification): current_app.logger.exception( f"Max retry failed Failed to persist notification {notification['id']}", ) + provider_task.apply_async([notification["id"]], queue=q) + current_app.logger.debug( + f"{notification['notification_type']} {notification['id']} has been persisted and sent to delivery queue." + ) def handle_exception(task, notification, notification_id, exc): From 6124fbc6a4be2b1bb47609b7eb000061c8e71006 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Mon, 2 Dec 2024 12:15:32 -0800 Subject: [PATCH 16/49] fix logic --- app/celery/tasks.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 63d4fdc09..0794ad4da 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -346,9 +346,14 @@ def save_api_email_or_sms(self, encrypted_notification): status=notification["status"], document_download_count=notification["document_download_count"], ) + # Only get here if save to the db was successful (i.e. first time) + provider_task.apply_async([notification["id"]], queue=q) + current_app.logger.debug( + f"{notification['notification_type']} {notification['id']} has been persisted and sent to delivery queue." + ) except IntegrityError: - current_app.logger.info( + current_app.logger.warning( f"{notification['notification_type']} {notification['id']} already exists." ) # If we don't have the return statement here, we will fall through and end @@ -362,10 +367,6 @@ def save_api_email_or_sms(self, encrypted_notification): current_app.logger.exception( f"Max retry failed Failed to persist notification {notification['id']}", ) - provider_task.apply_async([notification["id"]], queue=q) - current_app.logger.debug( - f"{notification['notification_type']} {notification['id']} has been persisted and sent to delivery queue." - ) def handle_exception(task, notification, notification_id, exc): From ed30a85bc8ba660652fae862f8b9dbf463356002 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 3 Dec 2024 12:44:57 -0800 Subject: [PATCH 17/49] change create notification to an upsert --- app/dao/notifications_dao.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index cbde45d30..4c40a7e81 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -71,7 +71,9 @@ def dao_create_notification(notification): # notify-api-742 remove phone numbers from db notification.to = "1" notification.normalised_to = "1" - db.session.add(notification) + + # notify-api-1454 change to an upsert + db.session.merge(notification) def country_records_delivery(phone_prefix): From b4093e8152a359e08b7960fd8a87f614928ad361 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 3 Dec 2024 12:55:52 -0800 Subject: [PATCH 18/49] change create notification to an upsert --- app/dao/notifications_dao.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 4c40a7e81..d231ece42 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -73,7 +73,10 @@ def dao_create_notification(notification): notification.normalised_to = "1" # notify-api-1454 change to an upsert - db.session.merge(notification) + stmt = select(Notification).where(Notification.id == notification.id) + result = db.session.execute(stmt).scalar() + if result is None: + db.session.add(notification) def country_records_delivery(phone_prefix): From 14e4d761fc4daf769f0cd346d0682ba05131f634 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 3 Dec 2024 13:50:42 -0800 Subject: [PATCH 19/49] try to fix tests --- app/celery/tasks.py | 16 +++++++++++----- app/dao/notifications_dao.py | 2 +- app/notifications/process_notifications.py | 5 +++++ 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 0794ad4da..8cece7aa4 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -20,7 +20,10 @@ from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id from app.dao.templates_dao import dao_get_template_by_id from app.enums import JobStatus, KeyType, NotificationType from app.errors import TotalRequestsError -from app.notifications.process_notifications import persist_notification +from app.notifications.process_notifications import ( + get_notification, + persist_notification, +) from app.notifications.validators import check_service_over_total_message_limit from app.serialised_models import SerialisedService, SerialisedTemplate from app.service.utils import service_allowed_to_send_to @@ -329,6 +332,8 @@ def save_api_email_or_sms(self, encrypted_notification): if notification["notification_type"] == NotificationType.EMAIL else provider_tasks.deliver_sms ) + + original_notification = get_notification(notification["id"]) try: persist_notification( notification_id=notification["id"], @@ -347,10 +352,11 @@ def save_api_email_or_sms(self, encrypted_notification): document_download_count=notification["document_download_count"], ) # Only get here if save to the db was successful (i.e. first time) - provider_task.apply_async([notification["id"]], queue=q) - current_app.logger.debug( - f"{notification['notification_type']} {notification['id']} has been persisted and sent to delivery queue." - ) + if original_notification is None: + provider_task.apply_async([notification["id"]], queue=q) + current_app.logger.debug( + f"{notification['id']} has been persisted and sent to delivery queue." + ) except IntegrityError: current_app.logger.warning( diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d231ece42..8371aaa85 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -72,7 +72,7 @@ def dao_create_notification(notification): notification.to = "1" notification.normalised_to = "1" - # notify-api-1454 change to an upsert + # notify-api-1454 insert only if it doesn't exist stmt = select(Notification).where(Notification.id == notification.id) result = db.session.execute(stmt).scalar() if result is None: diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 4f5d8d06c..8568b8894 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -8,6 +8,7 @@ from app.config import QueueNames from app.dao.notifications_dao import ( dao_create_notification, dao_delete_notifications_by_id, + get_notification_by_id, ) from app.enums import KeyType, NotificationStatus, NotificationType from app.errors import BadRequestError @@ -53,6 +54,10 @@ def check_placeholders(template_object): raise BadRequestError(fields=[{"template": message}], message=message) +def get_notification(notification_id): + return get_notification_by_id(notification_id) + + def persist_notification( *, template_id, From 9b1eb70bbebff28a07f66d15a9a3951d709b5f59 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 3 Dec 2024 14:05:25 -0800 Subject: [PATCH 20/49] try to fix tests --- app/celery/tasks.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 8cece7aa4..7526e6619 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -274,7 +274,7 @@ def save_email( "Email {} failed as restricted service".format(notification_id) ) return - + original_notification = get_notification(notification_id) try: saved_notification = persist_notification( template_id=notification["template"], @@ -291,10 +291,11 @@ def save_email( notification_id=notification_id, reply_to_text=reply_to_text, ) - - provider_tasks.deliver_email.apply_async( - [str(saved_notification.id)], queue=QueueNames.SEND_EMAIL - ) + # we only want to send once + if original_notification is None: + provider_tasks.deliver_email.apply_async( + [str(saved_notification.id)], queue=QueueNames.SEND_EMAIL + ) current_app.logger.debug( "Email {} created at {}".format( From 55f538b10f71bcefb1a8d5c71c1a3e68152499d0 Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Tue, 3 Dec 2024 17:28:36 -0500 Subject: [PATCH 21/49] Update Restage workflow to use latest cg-cli-tools This changeset updates our restage workflow and GitHub action to use the latest version of the cg-cli-tools to help prevent future issues with performing restage actions for our apps. Signed-off-by: Carlo Costino --- .github/workflows/restage-apps.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/restage-apps.yml b/.github/workflows/restage-apps.yml index abdadcfe0..23c78f8cf 100644 --- a/.github/workflows/restage-apps.yml +++ b/.github/workflows/restage-apps.yml @@ -19,18 +19,18 @@ jobs: app: ["api", "admin"] steps: - name: Restage ${{matrix.app}} - uses: 18f/cg-deploy-action@main + uses: cloud-gov/cg-cli-tools@main with: cf_username: ${{ secrets.CLOUDGOV_USERNAME }} cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-${{ inputs.environment }} - full_command: "cf restage --strategy rolling notify-${{matrix.app}}-${{inputs.environment}}" + command: "cf restage --strategy rolling notify-${{matrix.app}}-${{inputs.environment}}" - name: Restage ${{matrix.app}} egress - uses: 18f/cg-deploy-action@main + uses: cloud-gov/cg-cli-tools@main with: cf_username: ${{ secrets.CLOUDGOV_USERNAME }} cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-${{ inputs.environment }}-egress - full_command: "cf restage --strategy rolling egress-proxy-notify-${{matrix.app}}-${{inputs.environment}}" + command: "cf restage --strategy rolling egress-proxy-notify-${{matrix.app}}-${{inputs.environment}}" From 2036e575e0018585f9335794d0cde92cc62075f3 Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Tue, 3 Dec 2024 20:57:25 -0500 Subject: [PATCH 22/49] Bump Redis plan to 5node-large This changeset boosts our production Redis configuration to the 5node-large plan to give us a bit more wiggle room for heavy production workloads. This should still keep us in the threshold of the second block of 10 nodes we are already signed up for as we currently have 12 nodes going across all environments at the moment. Signed-off-by: Carlo Costino --- terraform/production/main.tf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terraform/production/main.tf b/terraform/production/main.tf index 9543fdd86..d8a46693b 100644 --- a/terraform/production/main.tf +++ b/terraform/production/main.tf @@ -27,7 +27,7 @@ module "redis-v70" { cf_org_name = local.cf_org_name cf_space_name = local.cf_space_name name = "${local.app_name}-redis-v70-${local.env}" - redis_plan_name = "redis-3node-large" + redis_plan_name = "redis-5node-large" json_params = jsonencode( { "engineVersion" : "7.0", From 84343087bf8dd9db9e134e4479145d531fc043a9 Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Wed, 4 Dec 2024 09:48:42 -0500 Subject: [PATCH 23/49] Revert Redis plan back Changing the Redis plan in place is not supported - we would need to create a new cluster and then remove the old one. Signed-off-by: Carlo Costino --- terraform/production/main.tf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terraform/production/main.tf b/terraform/production/main.tf index d8a46693b..9543fdd86 100644 --- a/terraform/production/main.tf +++ b/terraform/production/main.tf @@ -27,7 +27,7 @@ module "redis-v70" { cf_org_name = local.cf_org_name cf_space_name = local.cf_space_name name = "${local.app_name}-redis-v70-${local.env}" - redis_plan_name = "redis-5node-large" + redis_plan_name = "redis-3node-large" json_params = jsonencode( { "engineVersion" : "7.0", From 665de72059b0e395ed7970b5a708b2e3dd64ebd7 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 4 Dec 2024 07:37:59 -0800 Subject: [PATCH 24/49] add expire times for redis objects --- app/celery/process_ses_receipts_tasks.py | 8 +++++--- app/celery/provider_tasks.py | 14 ++++++++++---- app/celery/tasks.py | 15 +++++++++++---- app/config.py | 1 + 4 files changed, 27 insertions(+), 11 deletions(-) diff --git a/app/celery/process_ses_receipts_tasks.py b/app/celery/process_ses_receipts_tasks.py index c03df0c98..c0b7d3413 100644 --- a/app/celery/process_ses_receipts_tasks.py +++ b/app/celery/process_ses_receipts_tasks.py @@ -12,7 +12,7 @@ from app.celery.service_callback_tasks import ( send_complaint_to_service, send_delivery_status_to_service, ) -from app.config import QueueNames +from app.config import Config, QueueNames from app.dao import notifications_dao from app.dao.complaint_dao import save_complaint from app.dao.notifications_dao import dao_get_notification_history_by_reference @@ -65,7 +65,9 @@ def process_ses_results(self, response): f"Callback may have arrived before notification was" f"persisted to the DB. Adding task to retry queue" ) - self.retry(queue=QueueNames.RETRY) + self.retry( + queue=QueueNames.RETRY, expires=Config.DEFAULT_REDIS_EXPIRE_TIME + ) else: current_app.logger.warning( f"Notification not found for reference: {reference} " @@ -115,7 +117,7 @@ def process_ses_results(self, response): except Exception: current_app.logger.exception("Error processing SES results") - self.retry(queue=QueueNames.RETRY) + self.retry(queue=QueueNames.RETRY, expires=Config.DEFAULT_REDIS_EXPIRE_TIME) def determine_notification_bounce_type(ses_message): diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index b0c6a4c9b..1e6c7e96f 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -10,7 +10,7 @@ from app import aws_cloudwatch_client, notify_celery, redis_store from app.clients.email import EmailClientNonRetryableException from app.clients.email.aws_ses import AwsSesClientThrottlingSendRateException from app.clients.sms import SmsClientResponseException -from app.config import QueueNames +from app.config import Config, QueueNames from app.dao import notifications_dao from app.dao.notifications_dao import ( sanitize_successful_notification_by_id, @@ -152,9 +152,15 @@ def deliver_sms(self, notification_id): try: if self.request.retries == 0: - self.retry(queue=QueueNames.RETRY, countdown=0) + self.retry( + queue=QueueNames.RETRY, + countdown=0, + expires=Config.DEFAULT_REDIS_EXPIRE_TIME, + ) else: - self.retry(queue=QueueNames.RETRY) + self.retry( + queue=QueueNames.RETRY, expires=Config.DEFAULT_REDIS_EXPIRE_TIME + ) except self.MaxRetriesExceededError: message = ( "RETRY FAILED: Max retries reached. The task send_sms_to_provider failed for notification {}. " @@ -203,7 +209,7 @@ def deliver_email(self, notification_id): f"RETRY: Email notification {notification_id} failed" ) - self.retry(queue=QueueNames.RETRY) + self.retry(queue=QueueNames.RETRY, expires=Config.DEFAULT_REDIS_EXPIRE_TIME) except self.MaxRetriesExceededError: message = ( "RETRY FAILED: Max retries reached. " diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 7526e6619..f27cc4b75 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -7,7 +7,7 @@ from sqlalchemy.exc import IntegrityError, SQLAlchemyError from app import create_uuid, encryption, notify_celery from app.aws import s3 from app.celery import provider_tasks -from app.config import QueueNames +from app.config import Config, QueueNames from app.dao.inbound_sms_dao import dao_get_inbound_sms_by_id from app.dao.jobs_dao import dao_get_job_by_id, dao_update_job from app.dao.notifications_dao import ( @@ -146,6 +146,7 @@ def process_row(row, template, job, service, sender_id=None): ), task_kwargs, queue=QueueNames.DATABASE, + expires=Config.DEFAULT_REDIS_EXPIRE_TIME, ) return notification_id @@ -369,7 +370,7 @@ def save_api_email_or_sms(self, encrypted_notification): except SQLAlchemyError: try: - self.retry(queue=QueueNames.RETRY) + self.retry(queue=QueueNames.RETRY, expires=Config.DEFAULT_REDIS_EXPIRE_TIME) except self.MaxRetriesExceededError: current_app.logger.exception( f"Max retry failed Failed to persist notification {notification['id']}", @@ -390,7 +391,11 @@ def handle_exception(task, notification, notification_id, exc): # This probably (hopefully) is not an issue with Redis as the celery backing store current_app.logger.exception("Retry" + retry_msg) try: - task.retry(queue=QueueNames.RETRY, exc=exc) + task.retry( + queue=QueueNames.RETRY, + exc=exc, + expires=Config.DEFAULT_REDIS_EXPIRE_TIME, + ) except task.MaxRetriesExceededError: current_app.logger.exception("Max retry failed" + retry_msg) @@ -439,7 +444,9 @@ def send_inbound_sms_to_service(self, inbound_sms_id, service_id): ) if not isinstance(e, HTTPError) or e.response.status_code >= 500: try: - self.retry(queue=QueueNames.RETRY) + self.retry( + queue=QueueNames.RETRY, expires=Config.DEFAULT_REDIS_EXPIRE_TIME + ) except self.MaxRetriesExceededError: current_app.logger.exception( "Retry: send_inbound_sms_to_service has retried the max number of" diff --git a/app/config.py b/app/config.py index 53ea039a0..a94e2991e 100644 --- a/app/config.py +++ b/app/config.py @@ -53,6 +53,7 @@ class TaskNames(object): class Config(object): NOTIFY_APP_NAME = "api" + DEFAULT_REDIS_EXPIRE_TIME = 8 * 24 * 60 * 60 NOTIFY_ENVIRONMENT = getenv("NOTIFY_ENVIRONMENT", "development") # URL of admin app ADMIN_BASE_URL = getenv("ADMIN_BASE_URL", "http://localhost:6012") From 3aa5732b1dbb932d334d5560d0062d2d2238a503 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 4 Dec 2024 07:45:00 -0800 Subject: [PATCH 25/49] add expire times for redis objects --- tests/app/celery/test_tasks.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 5720b15f9..197bcf1e5 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,7 +1,7 @@ import json import uuid from datetime import datetime, timedelta -from unittest.mock import Mock, call +from unittest.mock import ANY, Mock, call import pytest import requests_mock @@ -115,6 +115,7 @@ def test_should_process_sms_job(sample_job, mocker): (str(sample_job.service_id), "uuid", "something_encrypted"), {}, queue="database-tasks", + expires=ANY, ) job = jobs_dao.dao_get_job_by_id(sample_job.id) assert job.job_status == JobStatus.FINISHED @@ -135,6 +136,7 @@ def test_should_process_sms_job_with_sender_id(sample_job, mocker, fake_uuid): (str(sample_job.service_id), "uuid", "something_encrypted"), {"sender_id": fake_uuid}, queue="database-tasks", + expires=ANY, ) @@ -179,6 +181,7 @@ def test_should_process_job_if_send_limits_are_not_exceeded( ), {}, queue="database-tasks", + expires=ANY, ) @@ -237,6 +240,7 @@ def test_should_process_email_job(email_job_with_placeholders, mocker): ), {}, queue="database-tasks", + expires=ANY, ) job = jobs_dao.dao_get_job_by_id(email_job_with_placeholders.id) assert job.job_status == JobStatus.FINISHED @@ -262,6 +266,7 @@ def test_should_process_email_job_with_sender_id( (str(email_job_with_placeholders.service_id), "uuid", "something_encrypted"), {"sender_id": fake_uuid}, queue="database-tasks", + expires=ANY, ) @@ -387,6 +392,7 @@ def test_process_row_when_sender_id_is_provided(mocker, fake_uuid): ), {"sender_id": fake_uuid}, queue="database-tasks", + expires=ANY, ) @@ -839,7 +845,9 @@ def test_save_sms_should_go_to_retry_queue_if_database_errors(sample_template, m encryption.encrypt(notification), ) assert provider_tasks.deliver_sms.apply_async.called is False - tasks.save_sms.retry.assert_called_with(exc=expected_exception, queue="retry-tasks") + tasks.save_sms.retry.assert_called_with( + exc=expected_exception, queue="retry-tasks", expires=ANY + ) assert _get_notification_query_count() == 0 @@ -868,7 +876,7 @@ def test_save_email_should_go_to_retry_queue_if_database_errors( ) assert not provider_tasks.deliver_email.apply_async.called tasks.save_email.retry.assert_called_with( - exc=expected_exception, queue="retry-tasks" + exc=expected_exception, queue="retry-tasks", expires=ANY ) assert _get_notification_query_count() == 0 From f694c752d08ec55fb9122635e6de13d46d509734 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 4 Dec 2024 07:56:00 -0800 Subject: [PATCH 26/49] add expire times for redis objects --- tests/app/celery/test_scheduled_tasks.py | 1 + tests/app/celery/test_tasks.py | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 90a29f5ed..553c4338e 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -415,6 +415,7 @@ def test_check_for_missing_rows_in_completed_jobs_calls_save_email( ), {}, queue="database-tasks", + expires=ANY ) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 197bcf1e5..41bc43c41 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -356,6 +356,7 @@ def test_process_row_sends_letter_task( ), {}, queue=expected_queue, + expires=ANY ) From 921eefbb1e9719af33d42bbc098b8e35c3d049e6 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 4 Dec 2024 08:04:34 -0800 Subject: [PATCH 27/49] add expire times for redis objects --- tests/app/celery/test_provider_tasks.py | 11 +++++++---- tests/app/celery/test_scheduled_tasks.py | 2 +- tests/app/celery/test_tasks.py | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index a22a3fb93..80d5a0d7e 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,4 +1,5 @@ import json +from unittest.mock import ANY import pytest from botocore.exceptions import ClientError @@ -148,7 +149,7 @@ def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_sms_task deliver_sms(notification_id) app.delivery.send_to_providers.send_sms_to_provider.assert_not_called() app.celery.provider_tasks.deliver_sms.retry.assert_called_with( - queue="retry-tasks", countdown=0 + queue="retry-tasks", countdown=0, expires=ANY ) @@ -208,7 +209,7 @@ def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task( assert str(sample_notification.id) in str(e.value) provider_tasks.deliver_sms.retry.assert_called_with( - queue="retry-tasks", countdown=0 + queue="retry-tasks", countdown=0, expires=ANY ) assert sample_notification.status == NotificationStatus.TEMPORARY_FAILURE @@ -240,7 +241,7 @@ def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_email_ta deliver_email(notification_id) app.delivery.send_to_providers.send_email_to_provider.assert_not_called() app.celery.provider_tasks.deliver_email.retry.assert_called_with( - queue="retry-tasks" + queue="retry-tasks", expires=ANY ) @@ -268,7 +269,9 @@ def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task deliver_email(sample_notification.id) assert str(sample_notification.id) in str(e.value) - provider_tasks.deliver_email.retry.assert_called_with(queue="retry-tasks") + provider_tasks.deliver_email.retry.assert_called_with( + queue="retry-tasks", expires=ANY + ) assert sample_notification.status == NotificationStatus.TECHNICAL_FAILURE diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 553c4338e..ae9ea571d 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -415,7 +415,7 @@ def test_check_for_missing_rows_in_completed_jobs_calls_save_email( ), {}, queue="database-tasks", - expires=ANY + expires=ANY, ) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 41bc43c41..4fccfb8cb 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -356,7 +356,7 @@ def test_process_row_sends_letter_task( ), {}, queue=expected_queue, - expires=ANY + expires=ANY, ) From 8624776a47bfa88a7102458e4da11a210e2082a1 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 4 Dec 2024 09:20:38 -0800 Subject: [PATCH 28/49] add expire times for redis objects --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index a94e2991e..12159e289 100644 --- a/app/config.py +++ b/app/config.py @@ -53,7 +53,7 @@ class TaskNames(object): class Config(object): NOTIFY_APP_NAME = "api" - DEFAULT_REDIS_EXPIRE_TIME = 8 * 24 * 60 * 60 + DEFAULT_REDIS_EXPIRE_TIME = 4 * 24 * 60 * 60 NOTIFY_ENVIRONMENT = getenv("NOTIFY_ENVIRONMENT", "development") # URL of admin app ADMIN_BASE_URL = getenv("ADMIN_BASE_URL", "http://localhost:6012") From dc8660191c635b4c5e3e4b49ada5d4202cdfa645 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 10 Dec 2024 08:04:55 -0800 Subject: [PATCH 29/49] change retry schedule --- app/celery/provider_tasks.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 1e6c7e96f..2cd006927 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -1,5 +1,6 @@ import json import os +import random from datetime import timedelta from botocore.exceptions import ClientError @@ -29,8 +30,7 @@ DELIVERY_RECEIPT_DELAY_IN_SECONDS = 30 @notify_celery.task( bind=True, name="check_sms_delivery_receipt", - max_retries=48, - default_retry_delay=300, + max_retries=72, ) def check_sms_delivery_receipt(self, message_id, notification_id, sent_at): """ @@ -62,7 +62,10 @@ def check_sms_delivery_receipt(self, message_id, notification_id, sent_at): carrier=carrier, provider_response=provider_response, ) - raise self.retry(exc=ntfe) + base_delay = 3600 # one hour + jitter = random.randint(-1200, +1200) # plus or minus 20 minutes + retry_delay = base_delay + jitter + raise self.retry(countdown=retry_delay, exc=ntfe) except ClientError as err: # Probably a ThrottlingException but could be something else error_code = err.response["Error"]["Code"] @@ -77,7 +80,10 @@ def check_sms_delivery_receipt(self, message_id, notification_id, sent_at): carrier=carrier, provider_response=provider_response, ) - raise self.retry(exc=err) + base_delay = 3600 # one hour + jitter = random.randint(-1200, +1200) # plus or minus 20 minutes + retry_delay = base_delay + jitter + raise self.retry(countdown=retry_delay, exc=err) if status == "success": status = NotificationStatus.DELIVERED From 06725af4173a0ec45ea2a9e1219da3f322825785 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 10 Dec 2024 08:49:07 -0800 Subject: [PATCH 30/49] try to bypass static scan false positive --- app/celery/provider_tasks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 2cd006927..b88bf741c 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -63,7 +63,7 @@ def check_sms_delivery_receipt(self, message_id, notification_id, sent_at): provider_response=provider_response, ) base_delay = 3600 # one hour - jitter = random.randint(-1200, +1200) # plus or minus 20 minutes + jitter = random.randint(-1200, +1200) # noqa retry_delay = base_delay + jitter raise self.retry(countdown=retry_delay, exc=ntfe) except ClientError as err: @@ -81,7 +81,7 @@ def check_sms_delivery_receipt(self, message_id, notification_id, sent_at): provider_response=provider_response, ) base_delay = 3600 # one hour - jitter = random.randint(-1200, +1200) # plus or minus 20 minutes + jitter = random.randint(-1200, +1200) # noqa retry_delay = base_delay + jitter raise self.retry(countdown=retry_delay, exc=err) From 50aeb0ab0f52180ed2117faa3460952726ff892a Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 10 Dec 2024 08:53:38 -0800 Subject: [PATCH 31/49] try to bypass static scan false positive --- app/celery/provider_tasks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index b88bf741c..4bc8b99e1 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -63,7 +63,7 @@ def check_sms_delivery_receipt(self, message_id, notification_id, sent_at): provider_response=provider_response, ) base_delay = 3600 # one hour - jitter = random.randint(-1200, +1200) # noqa + jitter = random.randint(-1200, +1200) # nosec B311 retry_delay = base_delay + jitter raise self.retry(countdown=retry_delay, exc=ntfe) except ClientError as err: @@ -81,7 +81,7 @@ def check_sms_delivery_receipt(self, message_id, notification_id, sent_at): provider_response=provider_response, ) base_delay = 3600 # one hour - jitter = random.randint(-1200, +1200) # noqa + jitter = random.randint(-1200, +1200) # nosec B311 retry_delay = base_delay + jitter raise self.retry(countdown=retry_delay, exc=err) From fede173a3e05b0f23bda88f6cf3c41b8537b3e71 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 10 Dec 2024 10:39:26 -0800 Subject: [PATCH 32/49] change worker_max_tasks_per_child to 2000 --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index 12159e289..1ec8abd59 100644 --- a/app/config.py +++ b/app/config.py @@ -167,7 +167,7 @@ class Config(object): current_minute = (datetime.now().minute + 1) % 60 CELERY = { - "worker_max_tasks_per_child": 500, + "worker_max_tasks_per_child": 2000, "broker_url": REDIS_URL, "broker_transport_options": { "visibility_timeout": 310, From bdb73e9db29997cf3c14a487fbb8149b39ddb1da Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 10 Dec 2024 12:26:46 -0800 Subject: [PATCH 33/49] fix time limit in checking delivery receipts --- app/clients/cloudwatch/aws_cloudwatch.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/clients/cloudwatch/aws_cloudwatch.py b/app/clients/cloudwatch/aws_cloudwatch.py index 36bcf5dca..8ef34abac 100644 --- a/app/clients/cloudwatch/aws_cloudwatch.py +++ b/app/clients/cloudwatch/aws_cloudwatch.py @@ -158,7 +158,7 @@ class AwsCloudwatchClient(Client): message["delivery"].get("phoneCarrier", "Unknown Carrier"), ) - if time_now > (created_at + timedelta(hours=3)): + if time_now > (created_at + timedelta(hours=73)): # see app/models.py Notification. This message corresponds to "permanent-failure", # but we are copy/pasting here to avoid circular imports. return "failure", "Unable to find carrier response." From 8c59b474b1d3873cdebe1117a9e5cd3a8b1e76c9 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 10 Dec 2024 20:52:17 -0800 Subject: [PATCH 34/49] add message_id column to notifications --- migrations/versions/0413_add_message_id.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 migrations/versions/0413_add_message_id.py diff --git a/migrations/versions/0413_add_message_id.py b/migrations/versions/0413_add_message_id.py new file mode 100644 index 000000000..e2f5240c3 --- /dev/null +++ b/migrations/versions/0413_add_message_id.py @@ -0,0 +1,21 @@ +""" + +Revision ID: 0413_add_message_id +Revises: 412_remove_priority +Create Date: 2023-12-11 11:35:22.873930 + +""" + +import sqlalchemy as sa +from alembic import op + +revision = "0413_add_message_id" +down_revision = "0412_remove_priority" + + +def upgrade(): + op.add_column("notifications", sa.Column("message_id", sa.Text)) + + +def downgrade(): + op.drop_column("notifications", "message_id") From 69ecb2f096ee0fbca35fa593c6cdd50f953c3e56 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 10 Dec 2024 21:00:31 -0800 Subject: [PATCH 35/49] add message_id column to notifications --- migrations/versions/0413_add_message_id.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/migrations/versions/0413_add_message_id.py b/migrations/versions/0413_add_message_id.py index e2f5240c3..00aeaafc2 100644 --- a/migrations/versions/0413_add_message_id.py +++ b/migrations/versions/0413_add_message_id.py @@ -15,7 +15,14 @@ down_revision = "0412_remove_priority" def upgrade(): op.add_column("notifications", sa.Column("message_id", sa.Text)) + op.create_index( + "ix_notifications_message_id", + "notifications", + ["message_id"], + unique=False, + ) def downgrade(): + op.drop_index("ix_notifications_message_id", table_name="notifications") op.drop_column("notifications", "message_id") From a15d81ea1454fd95913414e78743d067d3a7c114 Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Thu, 12 Dec 2024 09:11:59 -0500 Subject: [PATCH 36/49] Revert "12/11/2024 Production Deployment API hotfixes" --- app/celery/provider_tasks.py | 14 ++++---------- app/clients/cloudwatch/aws_cloudwatch.py | 2 +- app/config.py | 2 +- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 4bc8b99e1..1e6c7e96f 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -1,6 +1,5 @@ import json import os -import random from datetime import timedelta from botocore.exceptions import ClientError @@ -30,7 +29,8 @@ DELIVERY_RECEIPT_DELAY_IN_SECONDS = 30 @notify_celery.task( bind=True, name="check_sms_delivery_receipt", - max_retries=72, + max_retries=48, + default_retry_delay=300, ) def check_sms_delivery_receipt(self, message_id, notification_id, sent_at): """ @@ -62,10 +62,7 @@ def check_sms_delivery_receipt(self, message_id, notification_id, sent_at): carrier=carrier, provider_response=provider_response, ) - base_delay = 3600 # one hour - jitter = random.randint(-1200, +1200) # nosec B311 - retry_delay = base_delay + jitter - raise self.retry(countdown=retry_delay, exc=ntfe) + raise self.retry(exc=ntfe) except ClientError as err: # Probably a ThrottlingException but could be something else error_code = err.response["Error"]["Code"] @@ -80,10 +77,7 @@ def check_sms_delivery_receipt(self, message_id, notification_id, sent_at): carrier=carrier, provider_response=provider_response, ) - base_delay = 3600 # one hour - jitter = random.randint(-1200, +1200) # nosec B311 - retry_delay = base_delay + jitter - raise self.retry(countdown=retry_delay, exc=err) + raise self.retry(exc=err) if status == "success": status = NotificationStatus.DELIVERED diff --git a/app/clients/cloudwatch/aws_cloudwatch.py b/app/clients/cloudwatch/aws_cloudwatch.py index 8ef34abac..36bcf5dca 100644 --- a/app/clients/cloudwatch/aws_cloudwatch.py +++ b/app/clients/cloudwatch/aws_cloudwatch.py @@ -158,7 +158,7 @@ class AwsCloudwatchClient(Client): message["delivery"].get("phoneCarrier", "Unknown Carrier"), ) - if time_now > (created_at + timedelta(hours=73)): + if time_now > (created_at + timedelta(hours=3)): # see app/models.py Notification. This message corresponds to "permanent-failure", # but we are copy/pasting here to avoid circular imports. return "failure", "Unable to find carrier response." diff --git a/app/config.py b/app/config.py index 1ec8abd59..12159e289 100644 --- a/app/config.py +++ b/app/config.py @@ -167,7 +167,7 @@ class Config(object): current_minute = (datetime.now().minute + 1) % 60 CELERY = { - "worker_max_tasks_per_child": 2000, + "worker_max_tasks_per_child": 500, "broker_url": REDIS_URL, "broker_transport_options": { "visibility_timeout": 310, From 6bc329006ec4970fb37932ec4b02a13e74d0b818 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 12 Dec 2024 11:49:51 -0800 Subject: [PATCH 37/49] change retries --- app/celery/tasks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index f27cc4b75..b612933ef 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -170,7 +170,7 @@ def __total_sending_limits_for_job_exceeded(service, job, job_id): return True -@notify_celery.task(bind=True, name="save-sms", max_retries=5, default_retry_delay=300) +@notify_celery.task(bind=True, name="save-sms", max_retries=2, default_retry_delay=600) def save_sms(self, service_id, notification_id, encrypted_notification, sender_id=None): """Persist notification to db and place notification in queue to send to sns.""" notification = encryption.decrypt(encrypted_notification) @@ -315,7 +315,7 @@ def save_api_email(self, encrypted_notification): @notify_celery.task( - bind=True, name="save-api-sms", max_retries=5, default_retry_delay=300 + bind=True, name="save-api-sms", max_retries=2, default_retry_delay=600 ) def save_api_sms(self, encrypted_notification): save_api_email_or_sms(self, encrypted_notification) From 5ce31c78fdfa4ffbad428f35b17212cb17fd91b0 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 13 Dec 2024 07:30:41 -0800 Subject: [PATCH 38/49] start writing message ids to the notifications table --- app/celery/provider_tasks.py | 4 ++++ app/dao/notifications_dao.py | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 1e6c7e96f..dd95472c9 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -14,6 +14,7 @@ from app.config import Config, QueueNames from app.dao import notifications_dao from app.dao.notifications_dao import ( sanitize_successful_notification_by_id, + update_notification_message_id, update_notification_status_by_id, ) from app.delivery import send_to_providers @@ -128,6 +129,9 @@ def deliver_sms(self, notification_id): ) # Code branches off to send_to_providers.py message_id = send_to_providers.send_sms_to_provider(notification) + update_notification_message_id(notification_id, message_id) + + # DEPRECATED # We have to put it in UTC. For other timezones, the delay # will be ignored and it will fire immediately (although this probably only affects developer testing) my_eta = utc_now() + timedelta(seconds=DELIVERY_RECEIPT_DELAY_IN_SECONDS) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 8371aaa85..af3f800e7 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -110,6 +110,16 @@ def _update_notification_status( return notification +@autocommit +def update_notification_message_id(notification_id, message_id): + stmt = ( + update(Notification) + .where(Notification.id == notification_id) + .values({"message_id": message_id}) + ) + db.session.execute(stmt) + + @autocommit def update_notification_status_by_id( notification_id, status, sent_by=None, provider_response=None, carrier=None From 0d280d608a132e7eff4cec0adeb97ac35fd35f0a Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 13 Dec 2024 07:42:08 -0800 Subject: [PATCH 39/49] start writing message ids to the notifications table --- tests/app/celery/test_provider_tasks.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 80d5a0d7e..7923f9498 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -131,6 +131,7 @@ def test_should_call_send_sms_to_provider_from_deliver_sms_task( ): mocker.patch("app.delivery.send_to_providers.send_sms_to_provider") mocker.patch("app.celery.provider_tasks.check_sms_delivery_receipt") + mocker.patch("app.celery.provider_tasks.update_notification_message_id") deliver_sms(sample_notification.id) app.delivery.send_to_providers.send_sms_to_provider.assert_called_with( From 26e53a80ba1ecd36868be3bed06a80db45421119 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 13 Dec 2024 08:43:23 -0800 Subject: [PATCH 40/49] avoid message is none scenario --- app/celery/provider_tasks.py | 3 ++- app/dao/notifications_dao.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index dd95472c9..ab8d8435a 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -129,7 +129,8 @@ def deliver_sms(self, notification_id): ) # Code branches off to send_to_providers.py message_id = send_to_providers.send_sms_to_provider(notification) - update_notification_message_id(notification_id, message_id) + if message_id is not None: # can be none if technical failure happens + update_notification_message_id(notification_id, message_id) # DEPRECATED # We have to put it in UTC. For other timezones, the delay diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index af3f800e7..066c21387 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -110,7 +110,6 @@ def _update_notification_status( return notification -@autocommit def update_notification_message_id(notification_id, message_id): stmt = ( update(Notification) @@ -118,6 +117,7 @@ def update_notification_message_id(notification_id, message_id): .values({"message_id": message_id}) ) db.session.execute(stmt) + db.session.commit() @autocommit From f706086bbf5f4b92d8fb4bdba36d3488574cfd74 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 13 Dec 2024 09:48:48 -0800 Subject: [PATCH 41/49] cleanup --- app/celery/provider_tasks.py | 3 --- app/dao/notifications_dao.py | 2 +- app/delivery/send_to_providers.py | 3 ++- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index ab8d8435a..a75a68c96 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -14,7 +14,6 @@ from app.config import Config, QueueNames from app.dao import notifications_dao from app.dao.notifications_dao import ( sanitize_successful_notification_by_id, - update_notification_message_id, update_notification_status_by_id, ) from app.delivery import send_to_providers @@ -129,8 +128,6 @@ def deliver_sms(self, notification_id): ) # Code branches off to send_to_providers.py message_id = send_to_providers.send_sms_to_provider(notification) - if message_id is not None: # can be none if technical failure happens - update_notification_message_id(notification_id, message_id) # DEPRECATED # We have to put it in UTC. For other timezones, the delay diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 066c21387..052242644 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -114,7 +114,7 @@ def update_notification_message_id(notification_id, message_id): stmt = ( update(Notification) .where(Notification.id == notification_id) - .values({"message_id": message_id}) + .values(message_id=message_id) ) db.session.execute(stmt) db.session.commit() diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 07763823f..4cc57fa6a 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -16,7 +16,7 @@ from app import ( from app.aws.s3 import get_personalisation_from_s3, get_phone_number_from_s3 from app.celery.test_key_tasks import send_email_response, send_sms_response from app.dao.email_branding_dao import dao_get_email_branding_by_id -from app.dao.notifications_dao import dao_update_notification +from app.dao.notifications_dao import dao_update_notification, update_notification_message_id from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.dao.service_sms_sender_dao import dao_get_sms_senders_by_service_id from app.enums import BrandType, KeyType, NotificationStatus, NotificationType @@ -117,6 +117,7 @@ def send_sms_to_provider(notification): message_id = provider.send_sms(**send_sms_kwargs) current_app.logger.info(f"got message_id {message_id}") + update_notification_message_id(notification.id, message_id) except Exception as e: n = notification msg = f"FAILED send to sms, job_id: {n.job_id} row_number {n.job_row_number} message_id {message_id}" From 6de1b226d0cd01429f6d461537a275c9fc22481e Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 13 Dec 2024 10:15:47 -0800 Subject: [PATCH 42/49] cleanup --- app/delivery/send_to_providers.py | 5 ++++- tests/app/celery/test_provider_tasks.py | 1 - 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 4cc57fa6a..e41062b41 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -16,7 +16,10 @@ from app import ( from app.aws.s3 import get_personalisation_from_s3, get_phone_number_from_s3 from app.celery.test_key_tasks import send_email_response, send_sms_response from app.dao.email_branding_dao import dao_get_email_branding_by_id -from app.dao.notifications_dao import dao_update_notification, update_notification_message_id +from app.dao.notifications_dao import ( + dao_update_notification, + update_notification_message_id, +) from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.dao.service_sms_sender_dao import dao_get_sms_senders_by_service_id from app.enums import BrandType, KeyType, NotificationStatus, NotificationType diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 7923f9498..80d5a0d7e 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -131,7 +131,6 @@ def test_should_call_send_sms_to_provider_from_deliver_sms_task( ): mocker.patch("app.delivery.send_to_providers.send_sms_to_provider") mocker.patch("app.celery.provider_tasks.check_sms_delivery_receipt") - mocker.patch("app.celery.provider_tasks.update_notification_message_id") deliver_sms(sample_notification.id) app.delivery.send_to_providers.send_sms_to_provider.assert_called_with( From 674320b72dfb2a8929f7d44e8962cfc49177b858 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 13 Dec 2024 11:54:46 -0800 Subject: [PATCH 43/49] add message_id to model duh --- app/models.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models.py b/app/models.py index 6b008f64b..ec6eac335 100644 --- a/app/models.py +++ b/app/models.py @@ -1532,6 +1532,7 @@ class Notification(db.Model): provider_response = db.Column(db.Text, nullable=True) carrier = db.Column(db.Text, nullable=True) + message_id = db.Column(db.Text, nullable=True) # queue_name = db.Column(db.Text, nullable=True) From 02c23a29ca5587411c596d924f6680cc538ca504 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 13 Dec 2024 12:12:19 -0800 Subject: [PATCH 44/49] fix tests --- tests/app/delivery/test_send_to_providers.py | 28 ++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 20b0f7186..672a48734 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -627,6 +627,10 @@ def test_should_update_billable_units_and_status_according_to_research_mode_and_ ): mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None) + mocker.patch( + "app.delivery.send_to_providers.update_notification_message_id", + return_value=None, + ) mocker.patch( "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"] ) @@ -637,6 +641,11 @@ def test_should_update_billable_units_and_status_according_to_research_mode_and_ key_type=key_type, reply_to_text="testing", ) + + mocker.patch( + "app.delivery.send_to_providers.update_notification_message_id", + return_value=None, + ) mocker.patch("app.aws_sns_client.send_sms") mocker.patch( "app.delivery.send_to_providers.send_sms_response", @@ -707,6 +716,10 @@ def test_should_send_sms_to_international_providers( mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") mock_s3.return_value = "601117224412" + mocker.patch( + "app.delivery.send_to_providers.update_notification_message_id", + return_value=None, + ) mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) @@ -744,6 +757,11 @@ def test_should_handle_sms_sender_and_prefix_message( mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None) mocker.patch("app.aws_sns_client.send_sms") + + mocker.patch( + "app.delivery.send_to_providers.update_notification_message_id", + return_value=None, + ) service = create_service_with_defined_sms_sender( sms_sender_value=sms_sender, prefix_sms=prefix_sms ) @@ -803,6 +821,11 @@ def test_send_sms_to_provider_should_use_normalised_to(mocker, client, sample_te mocker.patch( "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"] ) + + mocker.patch( + "app.delivery.send_to_providers.update_notification_message_id", + return_value=None, + ) send_mock = mocker.patch("app.aws_sns_client.send_sms") notification = create_notification( template=sample_template, @@ -866,6 +889,11 @@ def test_send_sms_to_provider_should_return_template_if_found_in_redis( mocker.patch( "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"] ) + + mocker.patch( + "app.delivery.send_to_providers.update_notification_message_id", + return_value=None, + ) from app.schemas import service_schema, template_schema service_dict = service_schema.dump(sample_template.service) From d455d6a82998d03382a6630c1f7981f9ffe2c956 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 13 Dec 2024 12:35:19 -0800 Subject: [PATCH 45/49] fix tests --- tests/app/delivery/test_send_to_providers.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 672a48734..585983311 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -365,6 +365,7 @@ def test_send_sms_should_use_service_sms_sender( mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None) mocker.patch("app.aws_sns_client.send_sms") + mocker.patch("app.delivery.send_to_providers.update_notification_message_id") sms_sender = create_service_sms_sender( service=sample_service, sms_sender="123456", is_default=False @@ -405,6 +406,8 @@ def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_c ) mocker.patch("app.aws_ses_client.send_email") mocker.patch("app.delivery.send_to_providers.send_email_response") + + mocker.patch("app.delivery.send_to_providers.update_notification_message_id") mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") mock_phone.return_value = "15555555555" @@ -656,6 +659,8 @@ def test_should_update_billable_units_and_status_according_to_research_mode_and_ sample_template.service.research_mode = True mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + + mocker.patch("app.delivery.send_to_providers.update_notification_message_id") mock_phone.return_value = "15555555555" mock_personalisation = mocker.patch( @@ -679,6 +684,8 @@ def test_should_set_notification_billable_units_and_reduces_provider_priority_if assert sample_notification.sent_by is None mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + + mocker.patch("app.delivery.send_to_providers.update_notification_message_id") mock_phone.return_value = "15555555555" mock_personalisation = mocker.patch( @@ -714,6 +721,8 @@ def test_should_send_sms_to_international_providers( ) mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + + mocker.patch("app.delivery.send_to_providers.update_notification_message_id") mock_s3.return_value = "601117224412" mocker.patch( From ee6eded8e40c2e5f752dd6edf15ff3c3409398e7 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 13 Dec 2024 12:46:58 -0800 Subject: [PATCH 46/49] fix tests --- tests/app/delivery/test_send_to_providers.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 585983311..7a6259551 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -93,6 +93,7 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") mock_s3.return_value = "2028675309" + mocker.patch("app.delivery.send_to_providers.update_notification_message_id") mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) @@ -233,6 +234,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") mock_s3.return_value = "2028675309" + mocker.patch("app.delivery.send_to_providers.update_notification_message_id") mock_s3_p = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) @@ -327,6 +329,7 @@ def test_should_send_sms_with_downgraded_content(notify_db_session, mocker): # ī, grapes, tabs, zero width space and ellipsis are not # ó isn't in GSM, but it is in the welsh alphabet so will still be sent + mocker.patch("app.delivery.send_to_providers.update_notification_message_id") mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None) mocker.patch( "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"] From 020af71574a301206c636923777892307928725e Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 17 Dec 2024 10:50:55 -0800 Subject: [PATCH 47/49] fix census BOM error --- app/aws/s3.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index e0022f20b..07b53d687 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -402,7 +402,7 @@ def extract_phones(job): phone_index = 0 for item in first_row: # Note: may contain a BOM and look like \ufeffphone number - if item.lower() in ["phone number", "\\ufeffphone number"]: + if item.lower() in ["phone number", "\\ufeffphone number", "phone number\n"]: break phone_index = phone_index + 1 From c2d822b2883c8de62e8e3522b07d55adaabd38f7 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 17 Dec 2024 10:56:27 -0800 Subject: [PATCH 48/49] add tests --- app/aws/s3.py | 7 ++++++- tests/app/aws/test_s3.py | 10 ++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 07b53d687..d97c421f2 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -402,7 +402,12 @@ def extract_phones(job): phone_index = 0 for item in first_row: # Note: may contain a BOM and look like \ufeffphone number - if item.lower() in ["phone number", "\\ufeffphone number", "phone number\n"]: + if item.lower() in [ + "phone number", + "\\ufeffphone number", + "\\ufeffphone number\n", + "phone number\n", + ]: break phone_index = phone_index + 1 diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index e4a9c1c07..f9baa2fde 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -219,6 +219,16 @@ def test_get_s3_file_makes_correct_call(notify_api, mocker): 2, "5555555552", ), + ( + # simulate file saved with utf8withbom + "\\ufeffPHONE NUMBER\n", + "5555555552", + ), + ( + # simulate file saved without utf8withbom + "\\PHONE NUMBER\n", + "5555555552", + ), ], ) def test_get_phone_number_from_s3( From 1a5e8824482f128fd641778b30dcb3b1b172c317 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 17 Dec 2024 11:02:40 -0800 Subject: [PATCH 49/49] fix tests --- tests/app/aws/test_s3.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index f9baa2fde..843ce3ba0 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -222,11 +222,15 @@ def test_get_s3_file_makes_correct_call(notify_api, mocker): ( # simulate file saved with utf8withbom "\\ufeffPHONE NUMBER\n", + "eee", + 2, "5555555552", ), ( # simulate file saved without utf8withbom "\\PHONE NUMBER\n", + "eee", + 2, "5555555552", ), ],