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}}" diff --git a/app/aws/s3.py b/app/aws/s3.py index e0022f20b..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"]: + if item.lower() in [ + "phone number", + "\\ufeffphone number", + "\\ufeffphone number\n", + "phone number\n", + ]: break phone_index = phone_index + 1 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 011b00d98..a75a68c96 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, @@ -128,6 +128,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) + + # 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) @@ -152,9 +154,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 {}. " @@ -170,7 +178,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,8 +190,12 @@ 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) except EmailClientNonRetryableException: current_app.logger.exception(f"Email notification {notification_id} failed") @@ -199,7 +211,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 c8ad8cc6d..b612933ef 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 ( @@ -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 @@ -143,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 @@ -166,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) @@ -271,7 +275,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"], @@ -288,10 +292,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( @@ -310,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) @@ -329,6 +334,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"], @@ -346,19 +353,24 @@ 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) + 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." + ) - 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 + # up retrying because IntegrityError is a subclass of SQLAlchemyError + return 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']}", @@ -379,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) @@ -428,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..12159e289 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 = 4 * 24 * 60 * 60 NOTIFY_ENVIRONMENT = getenv("NOTIFY_ENVIRONMENT", "development") # URL of admin app ADMIN_BASE_URL = getenv("ADMIN_BASE_URL", "http://localhost:6012") diff --git a/app/dao/api_key_dao.py b/app/dao/api_key_dao.py index be0d53461..6f7d0ba99 100644 --- a/app/dao/api_key_dao.py +++ b/app/dao/api_key_dao.py @@ -36,13 +36,9 @@ def expire_api_key(service_id, api_key_id): def get_model_api_keys(service_id, id=None): if id: - return ( - db.session.execute( - select(ApiKey).filter_by(id=id, service_id=service_id, expiry_date=None) - ) - .scalars() - .one() - ) + return db.session.execute( + select(ApiKey).where(id=id, service_id=service_id, expiry_date=None) + ).one() seven_days_ago = utc_now() - timedelta(days=7) return ( db.session.execute( @@ -63,13 +59,9 @@ def get_unsigned_secrets(service_id): """ This method can only be exposed to the Authentication of the api calls. """ - api_keys = ( - db.session.execute( - select(ApiKey).filter_by(service_id=service_id, expiry_date=None) - ) - .scalars() - .all() - ) + api_keys = db.session.execute( + select(ApiKey).where(service_id=service_id, expiry_date=None) + ).all() keys = [x.secret for x in api_keys] return keys @@ -78,9 +70,7 @@ def get_unsigned_secret(key_id): """ This method can only be exposed to the Authentication of the api calls. """ - api_key = ( - db.session.execute(select(ApiKey).filter_by(id=key_id, expiry_date=None)) - .scalars() - .one() - ) + api_key = db.session.execute( + select(ApiKey).where(id=key_id, expiry_date=None) + ).one() return api_key.secret diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d93a002d8..d74e85ba9 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -72,7 +72,12 @@ 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 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: + db.session.add(notification) def country_records_delivery(phone_prefix): @@ -106,6 +111,16 @@ def _update_notification_status( return notification +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) + db.session.commit() + + @autocommit def update_notification_status_by_id( notification_id, status, sent_by=None, provider_response=None, carrier=None diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 07763823f..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 +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 +120,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}" 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) 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, diff --git a/app/service/sender.py b/app/service/sender.py index 4b954f60b..a769dc4d9 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,6 +43,15 @@ 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) diff --git a/migrations/versions/0413_add_message_id.py b/migrations/versions/0413_add_message_id.py new file mode 100644 index 000000000..00aeaafc2 --- /dev/null +++ b/migrations/versions/0413_add_message_id.py @@ -0,0 +1,28 @@ +""" + +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)) + 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") 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..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 = "small-psql-redundant" + rds_plan_name = "medium-gp-psql-redundant" } module "redis-v70" { 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" { diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index e4a9c1c07..843ce3ba0 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -219,6 +219,20 @@ def test_get_s3_file_makes_correct_call(notify_api, mocker): 2, "5555555552", ), + ( + # simulate file saved with utf8withbom + "\\ufeffPHONE NUMBER\n", + "eee", + 2, + "5555555552", + ), + ( + # simulate file saved without utf8withbom + "\\PHONE NUMBER\n", + "eee", + 2, + "5555555552", + ), ], ) def test_get_phone_number_from_s3( 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 90a29f5ed..ae9ea571d 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 7fceeb3f2..1974d91ed 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, ) @@ -351,6 +356,7 @@ def test_process_row_sends_letter_task( ), {}, queue=expected_queue, + expires=ANY, ) @@ -387,6 +393,7 @@ def test_process_row_when_sender_id_is_provided(mocker, fake_uuid): ), {"sender_id": fake_uuid}, queue="database-tasks", + expires=ANY, ) @@ -849,7 +856,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 @@ -878,7 +887,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 diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 88569bcd4..91970e968 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -94,6 +94,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" ) @@ -242,6 +243,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" ) @@ -336,6 +338,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"] @@ -374,6 +377,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 @@ -414,6 +418,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" @@ -636,6 +642,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"] ) @@ -646,6 +656,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", @@ -656,6 +671,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 +696,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,8 +733,14 @@ 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( + "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" ) @@ -753,6 +778,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 ) @@ -812,6 +842,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, @@ -875,6 +910,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) diff --git a/tests/app/service/test_sender.py b/tests/app/service/test_sender.py index 4de5e6a6e..bb1b9baeb 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] @@ -82,13 +83,16 @@ 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", autospec=True) first_active_user = create_user(email="foo@bar.com", state="active") 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) dao_add_user_to_service(service, second_active_user) + dao_add_user_to_service(service, pending_user) + template = create_template(service, template_type=TemplateType.EMAIL) send_notification_to_service_users(service_id=service.id, template_id=template.id)