From d051955d659f4f95b5c61180655650f77f95e691 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 08:01:51 -0800 Subject: [PATCH 01/20] change total message limit to 100000 --- app/config.py | 2 +- tests/app/conftest.py | 2 +- tests/app/db.py | 2 +- tests/app/service/test_rest.py | 22 +++++++++++----------- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/app/config.py b/app/config.py index 13d9daf9d..2cabab9b7 100644 --- a/app/config.py +++ b/app/config.py @@ -339,7 +339,7 @@ class Config(object): FREE_SMS_TIER_FRAGMENT_COUNT = 250000 - TOTAL_MESSAGE_LIMIT = 250000 + TOTAL_MESSAGE_LIMIT = 100000 DAILY_MESSAGE_LIMIT = notifications_utils.DAILY_MESSAGE_LIMIT diff --git a/tests/app/conftest.py b/tests/app/conftest.py index b0bbf132b..d402aa8cb 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -224,7 +224,7 @@ def sample_service(sample_user): data = { "name": service_name, "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "email_from": email_from, "created_by": sample_user, diff --git a/tests/app/db.py b/tests/app/db.py index 56a778406..4177c6b05 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -121,7 +121,7 @@ def create_service( email_from=None, prefix_sms=True, message_limit=1000, - total_message_limit=250000, + total_message_limit=100000, organization_type=OrganizationType.FEDERAL, check_if_service_exists=False, go_live_user=None, diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 9aacf2c21..2019eab95 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -397,7 +397,7 @@ def test_create_service( "name": "created service", "user_id": str(sample_user.id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "created.service", @@ -468,7 +468,7 @@ def test_create_service_with_domain_sets_organization( "name": "created service", "user_id": str(sample_user.id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "created.service", @@ -495,7 +495,7 @@ def test_create_service_should_create_annual_billing_for_service( "name": "created service", "user_id": str(sample_user.id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "created.service", @@ -520,7 +520,7 @@ def test_create_service_should_raise_exception_and_not_create_service_if_annual_ "name": "created service", "user_id": str(sample_user.id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "created.service", @@ -557,7 +557,7 @@ def test_create_service_inherits_branding_from_organization( "name": "created service", "user_id": str(sample_user.id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "created.service", @@ -576,7 +576,7 @@ def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_u "email_from": "service", "name": "created service", "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "created_by": str(fake_uuid), @@ -597,7 +597,7 @@ def test_should_error_if_created_by_missing(notify_api, sample_user): "email_from": "service", "name": "created service", "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "user_id": str(sample_user.id), @@ -623,7 +623,7 @@ def test_should_not_create_service_with_missing_if_user_id_is_not_in_database( "user_id": fake_uuid, "name": "created service", "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "created_by": str(fake_uuid), @@ -666,7 +666,7 @@ def test_should_not_create_service_with_duplicate_name( "name": sample_service.name, "user_id": str(sample_service.users[0].id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "sample.service2", @@ -694,7 +694,7 @@ def test_create_service_should_throw_duplicate_key_constraint_for_existing_email "name": service_name, "user_id": str(first_service.users[0].id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "first.service", @@ -1220,7 +1220,7 @@ def test_default_permissions_are_added_for_user_service( "name": "created service", "user_id": str(sample_user.id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "created.service", From 3002dbf46f1a2b0024ef64f57a9a15cb6e6cf613 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 08:11:17 -0800 Subject: [PATCH 02/20] change total message limit to 100000 --- tests/app/notifications/test_validators.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index f9df6fb91..f95313fde 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -62,13 +62,13 @@ def test_check_service_over_total_message_limit_fails( service = create_service() mocker.patch( "app.redis_store.get", - return_value="250001", + return_value="100001", ) with pytest.raises(TotalRequestsError) as e: check_service_over_total_message_limit(key_type, service) assert e.value.status_code == 429 - assert e.value.message == "Exceeded total application limits (250000) for today" + assert e.value.message == "Exceeded total application limits (100000) for today" assert e.value.fields == [] From 2d2a54bda63a16bf9b0f5b5f46c9b983db6ad3cf Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 10:12:17 -0800 Subject: [PATCH 03/20] change the redis limit tracker to annual --- app/celery/tasks.py | 7 ++++++ app/notifications/validators.py | 11 ++++++-- .../0414_change_total_message_limit.py | 25 +++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 migrations/versions/0414_change_total_message_limit.py diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 331d95364..6fea63e1b 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -159,7 +159,14 @@ def process_row(row, template, job, service, sender_id=None): return notification_id +# TODO +# Originally this was checking a daily limit +# It is now checking an overall limit (annual?) for the free tier +# Is there any limit for the paid tier? +# Assuming the limit is annual, is it calendar year, fiscal year, MOU year? +# Do we need a command to run to clear the redis value, or should it happen automatically? def __total_sending_limits_for_job_exceeded(service, job, job_id): + try: total_sent = check_service_over_total_message_limit(KeyType.NORMAL, service) if total_sent + job.notification_count > service.total_message_limit: diff --git a/app/notifications/validators.py b/app/notifications/validators.py index f0a7f2a8f..48d35dfe9 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -45,10 +45,17 @@ def check_service_over_total_message_limit(key_type, service): cache_key = total_limit_cache_key(service.id) service_stats = redis_store.get(cache_key) + + ## Originally this was a daily limit check. It is now a free-tier limit check. + ## TODO is this annual or forever for each service? + ## TODO do we need a way to clear this out? How do we determine if it is + ## free-tier or paid? What are the limits for paid? Etc. + ## TODO + ## setting expiration to one year for now on the assume that the free tier + ## limit resets annually. if service_stats is None: - # first message of the day, set the cache to 0 and the expiry to 24 hours service_stats = 0 - redis_store.set(cache_key, service_stats, ex=86400) + redis_store.set(cache_key, service_stats, ex=365*24*60*60) return service_stats if int(service_stats) >= service.total_message_limit: current_app.logger.warning( diff --git a/migrations/versions/0414_change_total_message_limit.py b/migrations/versions/0414_change_total_message_limit.py new file mode 100644 index 000000000..1ee9adb08 --- /dev/null +++ b/migrations/versions/0414_change_total_message_limit.py @@ -0,0 +1,25 @@ +""" + +Revision ID: 0414_change_total_message_limit +Revises: 413_add_message_id +Create Date: 2025-01-23 11:35:22.873930 + +""" + +import sqlalchemy as sa +from alembic import op + +down_revision = "0413_add_message_id" +revision = "0414_change_total_message_limit" + + +def upgrade(): + """ + This limit is only used + """ + op.execute("UPDATE services set total_message_limit=100000") + + + +def downgrade(): + op.execute("UPDATE services set total_message_limit=250000") From a5a952205640b27a025dead2512a528c59ce4b5f Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 10:26:11 -0800 Subject: [PATCH 04/20] automate formatting and import sorting --- .github/workflows/checks.yml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 5244276bd..19641cf8f 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -47,10 +47,13 @@ jobs: NOTIFY_E2E_TEST_HTTP_AUTH_PASSWORD: ${{ secrets.NOTIFY_E2E_TEST_HTTP_AUTH_PASSWORD }} NOTIFY_E2E_TEST_HTTP_AUTH_USER: ${{ secrets.NOTIFY_E2E_TEST_HTTP_AUTH_USER }} NOTIFY_E2E_TEST_PASSWORD: ${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} + + - name: Check imports alphabetized + run: poetry run isort ./app ./tests + - name: Run formatting + run: poetry run black . - name: Run style checks run: poetry run flake8 . - - name: Check imports alphabetized - run: poetry run isort --check-only ./app ./tests - name: Check for dead code run: make dead-code - name: Run tests with coverage From 7e913983a4886d1e27e8f0416938bfbb0effab82 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 10:32:12 -0800 Subject: [PATCH 05/20] ugh fix flake 8 --- app/notifications/validators.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 48d35dfe9..51c77b577 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -46,13 +46,13 @@ def check_service_over_total_message_limit(key_type, service): cache_key = total_limit_cache_key(service.id) service_stats = redis_store.get(cache_key) - ## Originally this was a daily limit check. It is now a free-tier limit check. - ## TODO is this annual or forever for each service? - ## TODO do we need a way to clear this out? How do we determine if it is - ## free-tier or paid? What are the limits for paid? Etc. - ## TODO - ## setting expiration to one year for now on the assume that the free tier - ## limit resets annually. + # Originally this was a daily limit check. It is now a free-tier limit check. + # TODO is this annual or forever for each service? + # TODO do we need a way to clear this out? How do we determine if it is + # free-tier or paid? What are the limits for paid? Etc. + # TODO + # setting expiration to one year for now on the assume that the free tier + # limit resets annually. if service_stats is None: service_stats = 0 redis_store.set(cache_key, service_stats, ex=365*24*60*60) From 0d874828e42f45ca0679aaa02dd89e3386f0c022 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 13:28:26 -0800 Subject: [PATCH 06/20] fix total message limit so it works --- app/celery/provider_tasks.py | 4 +++ app/celery/tasks.py | 2 +- app/delivery/send_to_providers.py | 11 ++++++-- app/notifications/validators.py | 27 +++++++------------ .../0414_change_total_message_limit.py | 8 +++--- 5 files changed, 27 insertions(+), 25 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 3bdd2d9c0..a3ed1f9ef 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -14,6 +14,7 @@ from app.dao.notifications_dao import update_notification_status_by_id from app.delivery import send_to_providers from app.enums import NotificationStatus from app.exceptions import NotificationTechnicalFailureException +from notifications_utils.clients.redis import total_limit_cache_key @notify_celery.task( @@ -41,6 +42,9 @@ def deliver_sms(self, notification_id): # Code branches off to send_to_providers.py send_to_providers.send_sms_to_provider(notification) + cache_key = total_limit_cache_key(notification.service_id) + redis_store.incr(cache_key) + except Exception as e: update_notification_status_by_id( notification_id, diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 6fea63e1b..8b3d9a353 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -166,7 +166,7 @@ def process_row(row, template, job, service, sender_id=None): # Assuming the limit is annual, is it calendar year, fiscal year, MOU year? # Do we need a command to run to clear the redis value, or should it happen automatically? def __total_sending_limits_for_job_exceeded(service, job, job_id): - + print(hilite("ENTER __total_sending_limits_for_job_exceeded")) try: total_sent = check_service_over_total_message_limit(KeyType.NORMAL, service) if total_sent + job.notification_count > service.total_message_limit: diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index e41062b41..6d9961ab6 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -26,6 +26,7 @@ from app.enums import BrandType, KeyType, NotificationStatus, NotificationType from app.exceptions import NotificationTechnicalFailureException from app.serialised_models import SerialisedService, SerialisedTemplate from app.utils import hilite, utc_now +from notifications_utils.clients.redis import total_limit_cache_key from notifications_utils.template import ( HTMLEmailTemplate, PlainTextEmailTemplate, @@ -118,8 +119,10 @@ def send_sms_to_provider(notification): } db.session.close() # no commit needed as no changes to objects have been made above + 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 @@ -132,10 +135,14 @@ def send_sms_to_provider(notification): else: # Here we map the job_id and row number to the aws message_id n = notification - msg = f"Send to aws for job_id {n.job_id} row_number {n.job_row_number} message_id {message_id}" + msg = f"Send to AWS!!! for job_id {n.job_id} row_number {n.job_row_number} message_id {message_id}" current_app.logger.info(hilite(msg)) notification.billable_units = template.fragment_count + current_app.logger.info("GOING TO UPDATE NOTI TO SENDING") update_notification_to_sending(notification, provider) + + cache_key = total_limit_cache_key(service.id) + redis_store.incr(cache_key) return message_id diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 51c77b577..da4b9b290 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -11,7 +11,7 @@ from app.models import ServicePermission from app.notifications.process_notifications import create_content_for_notification from app.serialised_models import SerialisedTemplate from app.service.utils import service_allowed_to_send_to -from app.utils import get_public_notify_type_text +from app.utils import get_public_notify_type_text, hilite from notifications_utils import SMS_CHAR_COUNT_LIMIT from notifications_utils.clients.redis import ( rate_limit_cache_key, @@ -24,26 +24,13 @@ from notifications_utils.recipients import ( ) -def check_service_over_api_rate_limit(service, api_key): - if ( - current_app.config["API_RATE_LIMIT_ENABLED"] - and current_app.config["REDIS_ENABLED"] - ): - cache_key = rate_limit_cache_key(service.id, api_key.key_type) - rate_limit = service.rate_limit - interval = 60 - if redis_store.exceeded_rate_limit(cache_key, rate_limit, interval): - current_app.logger.info( - "service {} has been rate limited for throughput".format(service.id) - ) - raise RateLimitError(rate_limit, interval, api_key.key_type) - - def check_service_over_total_message_limit(key_type, service): + print(hilite("ENTER check_service_over_total_message_limit")) if key_type == KeyType.TEST or not current_app.config["REDIS_ENABLED"]: return 0 cache_key = total_limit_cache_key(service.id) + print(hilite(f"CACHE_KEY = {cache_key}")) service_stats = redis_store.get(cache_key) # Originally this was a daily limit check. It is now a free-tier limit check. @@ -53,17 +40,23 @@ def check_service_over_total_message_limit(key_type, service): # TODO # setting expiration to one year for now on the assume that the free tier # limit resets annually. + + # add column for actual charges to notifications and notifification_history table + # add service api to return total_message_limit and actual number of messages for service if service_stats is None: service_stats = 0 redis_store.set(cache_key, service_stats, ex=365*24*60*60) return service_stats - if int(service_stats) >= service.total_message_limit: + if int(service_stats) >= 5: + #if int(service_stats) >= service.total_message_limit: current_app.logger.warning( "service {} has been rate limited for total use sent {} limit {}".format( service.id, int(service_stats), service.total_message_limit ) ) raise TotalRequestsError(service.total_message_limit) + else: + print(hilite(f"TOTAL MESSAGE LIMIT {service.total_message_limit} CURRENT {service_stats}")) return int(service_stats) diff --git a/migrations/versions/0414_change_total_message_limit.py b/migrations/versions/0414_change_total_message_limit.py index 1ee9adb08..f4cb775d0 100644 --- a/migrations/versions/0414_change_total_message_limit.py +++ b/migrations/versions/0414_change_total_message_limit.py @@ -14,12 +14,10 @@ revision = "0414_change_total_message_limit" def upgrade(): - """ - This limit is only used - """ - op.execute("UPDATE services set total_message_limit=100000") + # TODO This needs updating when the agreement model is ready. We only want free tier at 100k + op.execute("UPDATE services set total_message_limit=100000 where total_message_limit=250000") def downgrade(): - op.execute("UPDATE services set total_message_limit=250000") + op.execute("UPDATE services set total_message_limit=250000 where total_message_limit=100000") From 49f4129e5ba17df12fccfc572549d95960c27043 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 13:41:13 -0800 Subject: [PATCH 07/20] add tada to makefile --- .github/workflows/checks.yml | 4 +--- Makefile | 8 +++++++ app/delivery/send_to_providers.py | 2 -- app/notifications/validators.py | 22 ++++++++----------- .../0414_change_total_message_limit.py | 9 +++++--- 5 files changed, 24 insertions(+), 21 deletions(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 19641cf8f..0de22c0fd 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -49,9 +49,7 @@ jobs: NOTIFY_E2E_TEST_PASSWORD: ${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} - name: Check imports alphabetized - run: poetry run isort ./app ./tests - - name: Run formatting - run: poetry run black . + run: poetry run isort --check-only ./app ./tests - name: Run style checks run: poetry run flake8 . - name: Check for dead code diff --git a/Makefile b/Makefile index 3d29046cb..741ceae5b 100644 --- a/Makefile +++ b/Makefile @@ -31,6 +31,14 @@ bootstrap-with-docker: ## Build the image to run the app in Docker run-procfile: poetry run honcho start -f Procfile.dev + + +.PHONY: tada +tada: + poetry run isort . + poetry run black . + poetry run flake8 . + .PHONY: avg-complexity avg-complexity: echo "*** Shows average complexity in radon of all code ***" diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 6d9961ab6..e34847397 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -119,10 +119,8 @@ def send_sms_to_provider(notification): } db.session.close() # no commit needed as no changes to objects have been made above - message_id = provider.send_sms(**send_sms_kwargs) - update_notification_message_id(notification.id, message_id) except Exception as e: n = notification diff --git a/app/notifications/validators.py b/app/notifications/validators.py index da4b9b290..615e87dfb 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -6,17 +6,14 @@ from app.dao.notifications_dao import dao_get_notification_count_for_service from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id from app.enums import KeyType, NotificationType, ServicePermissionType, TemplateType -from app.errors import BadRequestError, RateLimitError, TotalRequestsError +from app.errors import BadRequestError, TotalRequestsError from app.models import ServicePermission from app.notifications.process_notifications import create_content_for_notification from app.serialised_models import SerialisedTemplate from app.service.utils import service_allowed_to_send_to from app.utils import get_public_notify_type_text, hilite from notifications_utils import SMS_CHAR_COUNT_LIMIT -from notifications_utils.clients.redis import ( - rate_limit_cache_key, - total_limit_cache_key, -) +from notifications_utils.clients.redis import total_limit_cache_key from notifications_utils.recipients import ( get_international_phone_info, validate_and_format_email_address, @@ -45,10 +42,10 @@ def check_service_over_total_message_limit(key_type, service): # add service api to return total_message_limit and actual number of messages for service if service_stats is None: service_stats = 0 - redis_store.set(cache_key, service_stats, ex=365*24*60*60) + redis_store.set(cache_key, service_stats, ex=365 * 24 * 60 * 60) return service_stats if int(service_stats) >= 5: - #if int(service_stats) >= service.total_message_limit: + # if int(service_stats) >= service.total_message_limit: current_app.logger.warning( "service {} has been rate limited for total use sent {} limit {}".format( service.id, int(service_stats), service.total_message_limit @@ -56,7 +53,11 @@ def check_service_over_total_message_limit(key_type, service): ) raise TotalRequestsError(service.total_message_limit) else: - print(hilite(f"TOTAL MESSAGE LIMIT {service.total_message_limit} CURRENT {service_stats}")) + print( + hilite( + f"TOTAL MESSAGE LIMIT {service.total_message_limit} CURRENT {service_stats}" + ) + ) return int(service_stats) @@ -77,11 +78,6 @@ def check_application_over_retention_limit(key_type, service): return int(total_stats) -def check_rate_limiting(service, api_key): - check_service_over_api_rate_limit(service, api_key) - check_application_over_retention_limit(api_key.key_type, service) - - def check_template_is_for_notification_type(notification_type, template_type): if notification_type != template_type: message = "{0} template is not suitable for {1} notification".format( diff --git a/migrations/versions/0414_change_total_message_limit.py b/migrations/versions/0414_change_total_message_limit.py index f4cb775d0..8a3d9b3e2 100644 --- a/migrations/versions/0414_change_total_message_limit.py +++ b/migrations/versions/0414_change_total_message_limit.py @@ -15,9 +15,12 @@ revision = "0414_change_total_message_limit" def upgrade(): # TODO This needs updating when the agreement model is ready. We only want free tier at 100k - op.execute("UPDATE services set total_message_limit=100000 where total_message_limit=250000") - + op.execute( + "UPDATE services set total_message_limit=100000 where total_message_limit=250000" + ) def downgrade(): - op.execute("UPDATE services set total_message_limit=250000 where total_message_limit=100000") + op.execute( + "UPDATE services set total_message_limit=250000 where total_message_limit=100000" + ) From 2ac436f8470bf10692b4bf00a4dbda715ce4aeaf Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 13:46:25 -0800 Subject: [PATCH 08/20] add tada to makefile --- .ds.baseline | 4 +- tests/app/notifications/test_validators.py | 100 +-------------------- 2 files changed, 4 insertions(+), 100 deletions(-) diff --git a/.ds.baseline b/.ds.baseline index 2baf278e1..18fb40388 100644 --- a/.ds.baseline +++ b/.ds.baseline @@ -295,7 +295,7 @@ "filename": "tests/app/notifications/test_validators.py", "hashed_secret": "6c1a8443963d02d13ffe575a71abe19ea731fb66", "is_verified": false, - "line_number": 768, + "line_number": 672, "is_secret": false } ], @@ -384,5 +384,5 @@ } ] }, - "generated_at": "2024-12-19T19:09:50Z" + "generated_at": "2025-01-23T21:46:22Z" } diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index f95313fde..5cf9f2de0 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -1,11 +1,8 @@ import pytest -from flask import current_app -from freezegun import freeze_time -import app from app.dao import templates_dao from app.enums import KeyType, NotificationType, ServicePermissionType, TemplateType -from app.errors import BadRequestError, RateLimitError, TotalRequestsError +from app.errors import BadRequestError, TotalRequestsError from app.notifications.process_notifications import create_content_for_notification from app.notifications.sns_cert_validator import ( VALID_SNS_TOPICS, @@ -17,10 +14,8 @@ from app.notifications.validators import ( check_if_service_can_send_files_by_email, check_is_message_too_long, check_notification_content_is_not_empty, - check_rate_limiting, check_reply_to, check_service_email_reply_to_id, - check_service_over_api_rate_limit, check_service_over_total_message_limit, check_service_sms_sender_id, check_template_is_active, @@ -29,16 +24,11 @@ from app.notifications.validators import ( validate_and_format_recipient, validate_template, ) -from app.serialised_models import ( - SerialisedAPIKeyCollection, - SerialisedService, - SerialisedTemplate, -) +from app.serialised_models import SerialisedService, SerialisedTemplate from app.service.utils import service_allowed_to_send_to from app.utils import get_template_instance from notifications_utils import SMS_CHAR_COUNT_LIMIT from tests.app.db import ( - create_api_key, create_reply_to_email, create_service, create_service_guest_list, @@ -482,92 +472,6 @@ def test_validate_template_calls_all_validators_exception_message_too_long( assert not mock_check_message_is_too_long.called -@pytest.mark.parametrize("key_type", [KeyType.TEAM, KeyType.NORMAL, KeyType.TEST]) -def test_check_service_over_api_rate_limit_when_exceed_rate_limit_request_fails_raises_error( - key_type, sample_service, mocker -): - with freeze_time("2016-01-01 12:00:00.000000"): - mocker.patch("app.redis_store.exceeded_rate_limit", return_value=True) - - sample_service.restricted = True - api_key = create_api_key(sample_service, key_type=key_type) - serialised_service = SerialisedService.from_id(sample_service.id) - serialised_api_key = SerialisedAPIKeyCollection.from_service_id( - serialised_service.id - )[0] - - with pytest.raises(RateLimitError) as e: - check_service_over_api_rate_limit(serialised_service, serialised_api_key) - - app.redis_store.exceeded_rate_limit.assert_called_with( - f"{sample_service.id}-{api_key.key_type}", - sample_service.rate_limit, - 60, - ) - assert e.value.status_code == 429 - assert e.value.message == ( - f"Exceeded rate limit for key type " - f"{key_type.name if key_type != KeyType.NORMAL else 'LIVE'} of " - f"{sample_service.rate_limit} requests per {60} seconds" - ) - assert e.value.fields == [] - - -def test_check_service_over_api_rate_limit_when_rate_limit_has_not_exceeded_limit_succeeds( - sample_service, - mocker, -): - with freeze_time("2016-01-01 12:00:00.000000"): - mocker.patch("app.redis_store.exceeded_rate_limit", return_value=False) - - sample_service.restricted = True - api_key = create_api_key(sample_service) - serialised_service = SerialisedService.from_id(sample_service.id) - serialised_api_key = SerialisedAPIKeyCollection.from_service_id( - serialised_service.id - )[0] - - check_service_over_api_rate_limit(serialised_service, serialised_api_key) - app.redis_store.exceeded_rate_limit.assert_called_with( - f"{sample_service.id}-{api_key.key_type}", - 3000, - 60, - ) - - -def test_check_service_over_api_rate_limit_should_do_nothing_if_limiting_is_disabled( - sample_service, mocker -): - with freeze_time("2016-01-01 12:00:00.000000"): - current_app.config["API_RATE_LIMIT_ENABLED"] = False - - mocker.patch("app.redis_store.exceeded_rate_limit", return_value=False) - - sample_service.restricted = True - create_api_key(sample_service) - serialised_service = SerialisedService.from_id(sample_service.id) - serialised_api_key = SerialisedAPIKeyCollection.from_service_id( - serialised_service.id - )[0] - - check_service_over_api_rate_limit(serialised_service, serialised_api_key) - app.redis_store.exceeded_rate_limit.assert_not_called() - - -def test_check_rate_limiting_validates_api_rate_limit_and_daily_limit( - notify_db_session, mocker -): - mock_rate_limit = mocker.patch( - "app.notifications.validators.check_service_over_api_rate_limit" - ) - service = create_service() - api_key = create_api_key(service=service) - - check_rate_limiting(service, api_key) - - mock_rate_limit.assert_called_once_with(service, api_key) - - @pytest.mark.parametrize("key_type", [KeyType.TEST, KeyType.NORMAL]) @pytest.mark.skip( "We currently don't support international numbers, our validation fails before here" From 481f27d443f18968dfa9f02b78059a2d67712e9f Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 09:01:24 -0800 Subject: [PATCH 09/20] remove check_rate_limiting --- app/v2/notifications/post_notifications.py | 3 -- .../test_send_notification.py | 47 +------------------ 2 files changed, 1 insertion(+), 49 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index a5ad17646..a8dc894c7 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -18,7 +18,6 @@ from app.notifications.process_notifications import ( from app.notifications.validators import ( check_if_service_can_send_files_by_email, check_is_message_too_long, - check_rate_limiting, check_service_email_reply_to_id, check_service_has_permission, check_service_sms_sender_id, @@ -54,8 +53,6 @@ def post_notification(notification_type): check_service_has_permission(notification_type, authenticated_service.permissions) - check_rate_limiting(authenticated_service, api_user) - template, template_with_content = validate_template( form["template_id"], form.get("personalisation", {}), diff --git a/tests/app/service/send_notification/test_send_notification.py b/tests/app/service/send_notification/test_send_notification.py index 4c4a1792a..14802b56e 100644 --- a/tests/app/service/send_notification/test_send_notification.py +++ b/tests/app/service/send_notification/test_send_notification.py @@ -14,7 +14,7 @@ from app.dao.api_key_dao import save_model_api_key from app.dao.services_dao import dao_update_service from app.dao.templates_dao import dao_get_all_templates_for_service, dao_update_template from app.enums import KeyType, NotificationType, TemplateType -from app.errors import InvalidRequest, RateLimitError +from app.errors import InvalidRequest from app.models import ApiKey, Notification, NotificationHistory, Template from app.service.send_notification import send_one_off_notification from notifications_utils import SMS_CHAR_COUNT_LIMIT @@ -1124,51 +1124,6 @@ def test_create_template_raises_invalid_request_when_content_too_large( } -@pytest.mark.parametrize( - "notification_type, send_to", - [ - (NotificationType.SMS, "2028675309"), - ( - NotificationType.EMAIL, - "sample@email.com", - ), - ], -) -def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( - client, sample_service, mocker, notification_type, send_to -): - sample = create_template(sample_service, template_type=notification_type) - persist_mock = mocker.patch("app.notifications.rest.persist_notification") - deliver_mock = mocker.patch("app.notifications.rest.send_notification_to_queue") - - mocker.patch( - "app.notifications.rest.check_rate_limiting", - side_effect=RateLimitError("LIMIT", "INTERVAL", "TYPE"), - ) - - data = {"to": send_to, "template": str(sample.id)} - - auth_header = create_service_authorization_header(service_id=sample.service_id) - - response = client.post( - path=f"/notifications/{notification_type}", - data=json.dumps(data), - headers=[("Content-Type", "application/json"), auth_header], - ) - - message = json.loads(response.data)["message"] - result = json.loads(response.data)["result"] - assert response.status_code == 429 - assert result == "error" - assert message == ( - "Exceeded rate limit for key type TYPE of LIMIT " - "requests per INTERVAL seconds" - ) - - assert not persist_mock.called - assert not deliver_mock.called - - def test_should_allow_store_original_number_on_sms_notification( client, sample_template, mocker ): From 6b318b80212a8c7fc69b0b079967d7ede5d25b3e Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 10:22:23 -0800 Subject: [PATCH 10/20] add api for message limit, messages_sent --- app/notifications/validators.py | 14 +++----------- app/service/rest.py | 25 ++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 615e87dfb..9ec572e8b 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -22,28 +22,20 @@ from notifications_utils.recipients import ( def check_service_over_total_message_limit(key_type, service): - print(hilite("ENTER check_service_over_total_message_limit")) if key_type == KeyType.TEST or not current_app.config["REDIS_ENABLED"]: return 0 cache_key = total_limit_cache_key(service.id) - print(hilite(f"CACHE_KEY = {cache_key}")) service_stats = redis_store.get(cache_key) - # Originally this was a daily limit check. It is now a free-tier limit check. - # TODO is this annual or forever for each service? - # TODO do we need a way to clear this out? How do we determine if it is - # free-tier or paid? What are the limits for paid? Etc. # TODO - # setting expiration to one year for now on the assume that the free tier - # limit resets annually. - - # add column for actual charges to notifications and notifification_history table - # add service api to return total_message_limit and actual number of messages for service + # For now we are using calendar year + # Switch to using service agreement dates when the Agreement model is ready if service_stats is None: service_stats = 0 redis_store.set(cache_key, service_stats, ex=365 * 24 * 60 * 60) return service_stats + # TODO CHANGE THIS BACK TO SERVICE TOTAL MESSAGE LIMIT if int(service_stats) >= 5: # if int(service_stats) >= service.total_message_limit: current_app.logger.warning( diff --git a/app/service/rest.py b/app/service/rest.py index 533bf1bff..083ea23c4 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -7,7 +7,7 @@ from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound from werkzeug.datastructures import MultiDict -from app import db +from app import db, redis_store from app.aws.s3 import get_personalisation_from_s3, get_phone_number_from_s3 from app.config import QueueNames from app.dao import fact_notification_status_dao, notifications_dao @@ -109,6 +109,7 @@ from app.service.service_senders_schema import ( from app.service.utils import get_guest_list_objects from app.user.users_schema import post_set_permissions_schema from app.utils import get_prev_next_pagination_links, utc_now +from notifications_utils.clients.redis import total_limit_cache_key service_blueprint = Blueprint("service", __name__) @@ -1120,6 +1121,28 @@ def modify_service_data_retention(service_id, data_retention_id): return "", 204 +@service_blueprint.route("/get-service-message-ratio") +def get_service_message_ratio(): + service_id = request.args.get("service_id") + + my_service = dao_fetch_service_by_id(service_id) + + cache_key = total_limit_cache_key(service_id) + messages_sent = redis_store.get(cache_key) + if messages_sent is None: + messages_sent = 0 + current_app.logger.warning( + f"Messages sent was not being tracked for service {service_id}" + ) + else: + messages_sent = int(messages_sent) + + return { + "messages_sent": messages_sent, + "total_message_limit": my_service.total_message_limit, + } + + @service_blueprint.route("/monthly-data-by-service") def get_monthly_notification_data_by_service(): start_date = request.args.get("start_date") From c7553b1f21028784812c40c8906e15a1381ed05d Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 10:42:46 -0800 Subject: [PATCH 11/20] add test --- tests/app/service/test_rest.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 2019eab95..c00daf0cf 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1665,6 +1665,27 @@ def test_remove_user_from_service(client, sample_user_service_permission): assert resp.status_code == 204 +def test_get_service_message_ratio(mocker, client, sample_user_service_permission): + service = sample_user_service_permission.service + + mock_redis = mocker.patch("app.service.rest.redis_store.get") + mock_redis.return_value = 1 + + endpoint = url_for( + "service.get_service_message_ratio", + service_id=str(service.id), + ) + auth_header = create_admin_authorization_header() + + resp = client.get( + endpoint, headers=[("Content-Type", "application/json"), auth_header] + ) + assert resp.status_code == 200 + result = resp.json + assert result["total_message_limit"] == 100000 + assert result["message_count"] == 1 + + def test_remove_non_existant_user_from_service(client, sample_user_service_permission): second_user = create_user(email="new@digital.fake.gov") endpoint = url_for( From c52cbe8456d7fc0942924db77a012cc4d3e1e6f2 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 10:51:08 -0800 Subject: [PATCH 12/20] add test --- tests/app/service/test_rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index c00daf0cf..6dd488ab9 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1683,7 +1683,7 @@ def test_get_service_message_ratio(mocker, client, sample_user_service_permissio assert resp.status_code == 200 result = resp.json assert result["total_message_limit"] == 100000 - assert result["message_count"] == 1 + assert result["messages_sent"] == 1 def test_remove_non_existant_user_from_service(client, sample_user_service_permission): From efe902444226c499c8c93e8c70b2e5e3f079c09a Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 12:11:37 -0800 Subject: [PATCH 13/20] add tests --- .ds.baseline | 4 ++-- app/notifications/validators.py | 13 ++++++++++++- tests/app/service/test_rest.py | 24 +++++++++++++++++++++++- 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/.ds.baseline b/.ds.baseline index 18fb40388..1a39bc074 100644 --- a/.ds.baseline +++ b/.ds.baseline @@ -305,7 +305,7 @@ "filename": "tests/app/service/test_rest.py", "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8", "is_verified": false, - "line_number": 1285, + "line_number": 1286, "is_secret": false } ], @@ -384,5 +384,5 @@ } ] }, - "generated_at": "2025-01-23T21:46:22Z" + "generated_at": "2025-01-28T20:11:33Z" } diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 9ec572e8b..38d8263c7 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -1,3 +1,6 @@ +from datetime import datetime +from zoneinfo import ZoneInfo + from flask import current_app from sqlalchemy.orm.exc import NoResultFound @@ -31,9 +34,17 @@ def check_service_over_total_message_limit(key_type, service): # TODO # For now we are using calendar year # Switch to using service agreement dates when the Agreement model is ready + # If the service stat has never been set before, compute the remaining seconds for 2025 + # and set it (all services) to expire on 12/31/2025. if service_stats is None: + now_et = datetime.now(ZoneInfo("America/New_York")) + target_time = datetime( + 2025, 12, 31, 23, 59, 59, tzinfo=ZoneInfo("America/New_York") + ) + time_difference = target_time - now_et + seconds_difference = int(time_difference.total_seconds()) service_stats = 0 - redis_store.set(cache_key, service_stats, ex=365 * 24 * 60 * 60) + redis_store.set(cache_key, service_stats, ex=seconds_difference) return service_stats # TODO CHANGE THIS BACK TO SERVICE TOTAL MESSAGE LIMIT if int(service_stats) >= 5: diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 6dd488ab9..a88f8e21e 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1,7 +1,7 @@ import json import uuid from datetime import date, datetime, timedelta -from unittest.mock import ANY +from unittest.mock import ANY, MagicMock import pytest from flask import current_app, url_for @@ -38,6 +38,7 @@ from app.models import ( ServiceSmsSender, User, ) +from app.service.rest import check_request_args from app.utils import utc_now from tests import create_admin_authorization_header from tests.app.db import ( @@ -3712,3 +3713,24 @@ def test_get_service_notification_statistics_by_day( assert mock_get_service_statistics_for_specific_days.assert_called_once assert response == mock_data + + +def test_valid_request(): + request = MagicMock() + request.args = { + "service_id": "123", + "name": "Test Name", + "email_from": "test@example.com", + } + result = check_request_args(request) + assert result == ("123", "Test Name", "test@example.com") + + +def test_missing_service_id(): + request = MagicMock() + request.args = {"name": "Test Name", "email_from": "test@example.com"} + try: + check_request_args(request) + except Exception as e: + assert e.status_code == 400 + assert {"service_id": ["Can't be empty"] in e.errors} From ae499fd27823f8f5fff9f7ff4751c9735e1c1cc8 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 12:19:04 -0800 Subject: [PATCH 14/20] add tests --- tests/app/service/test_rest.py | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index a88f8e21e..d1ae6c012 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3715,22 +3715,22 @@ def test_get_service_notification_statistics_by_day( assert response == mock_data -def test_valid_request(): - request = MagicMock() - request.args = { - "service_id": "123", - "name": "Test Name", - "email_from": "test@example.com", - } - result = check_request_args(request) - assert result == ("123", "Test Name", "test@example.com") +# def test_valid_request(): +# request = MagicMock() +# request.args = { +# "service_id": "123", +# "name": "Test Name", +# "email_from": "test@example.com", +# } +# result = check_request_args(request) +# assert result == ("123", "Test Name", "test@example.com") -def test_missing_service_id(): - request = MagicMock() - request.args = {"name": "Test Name", "email_from": "test@example.com"} - try: - check_request_args(request) - except Exception as e: - assert e.status_code == 400 - assert {"service_id": ["Can't be empty"] in e.errors} +# def test_missing_service_id(): +# request = MagicMock() +# request.args = {"name": "Test Name", "email_from": "test@example.com"} +# try: +# check_request_args(request) +# except Exception as e: +# assert e.status_code == 400 +# assert {"service_id": ["Can't be empty"] in e.errors} From 8863400051b5776ee3802f44dd3a1ddecbebddc6 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 12:24:52 -0800 Subject: [PATCH 15/20] add tests --- .ds.baseline | 4 ++-- tests/app/service/test_rest.py | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/.ds.baseline b/.ds.baseline index 1a39bc074..ad8d6eb1e 100644 --- a/.ds.baseline +++ b/.ds.baseline @@ -305,7 +305,7 @@ "filename": "tests/app/service/test_rest.py", "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8", "is_verified": false, - "line_number": 1286, + "line_number": 1285, "is_secret": false } ], @@ -384,5 +384,5 @@ } ] }, - "generated_at": "2025-01-28T20:11:33Z" + "generated_at": "2025-01-28T20:24:49Z" } diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index d1ae6c012..81341a0a0 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1,7 +1,7 @@ import json import uuid from datetime import date, datetime, timedelta -from unittest.mock import ANY, MagicMock +from unittest.mock import ANY import pytest from flask import current_app, url_for @@ -38,7 +38,6 @@ from app.models import ( ServiceSmsSender, User, ) -from app.service.rest import check_request_args from app.utils import utc_now from tests import create_admin_authorization_header from tests.app.db import ( From c1c7e7b9e6e5d583126ab194bf08ecf195f10f0e Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 12:51:55 -0800 Subject: [PATCH 16/20] add tests --- .../notification_dao/test_notification_dao.py | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index db369c5fe..aa0d68bc6 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -1,7 +1,7 @@ import uuid from datetime import date, datetime, timedelta from functools import partial -from unittest.mock import MagicMock, patch +from unittest.mock import ANY, MagicMock, patch import pytest from freezegun import freeze_time @@ -30,6 +30,7 @@ from app.dao.notifications_dao import ( get_notifications_for_service, get_service_ids_with_notifications_on_date, notifications_not_yet_sent, + sanitize_successful_notification_by_id, update_notification_status_by_id, update_notification_status_by_reference, ) @@ -2094,3 +2095,28 @@ def test_get_service_ids_with_notifications_on_date_checks_ft_status( ) == 1 ) + + +def test_sanitize_successful_notification_by_id(): + notification_id = "12345" + carrier = "CarrierX" + provider_response = "Success" + + mock_session = MagicMock() + mock_text = MagicMock() + with patch("app.dao.notification_dao.db.session", mock_session), patch( + "app.dao.notification_dao.text", mock_text + ): + sanitize_successful_notification_by_id( + notification_id, carrier, provider_response + ) + mock_text.assert_called_once_with("x") + mock_session.execute.assert_called_once_with( + mock_text.return_value, + { + "notification_id": notification_id, + "carrier": carrier, + "response": provider_response, + "sent_at": ANY, + }, + ) From d17a4ede666885b2bcbc793d6ccfb7553e7b6141 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 13:00:30 -0800 Subject: [PATCH 17/20] add tests --- tests/app/dao/notification_dao/test_notification_dao.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index aa0d68bc6..19db72192 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -2104,8 +2104,8 @@ def test_sanitize_successful_notification_by_id(): mock_session = MagicMock() mock_text = MagicMock() - with patch("app.dao.notification_dao.db.session", mock_session), patch( - "app.dao.notification_dao.text", mock_text + with patch("app.dao.notifications_dao.db.session", mock_session), patch( + "app.dao.notifications_dao.text", mock_text ): sanitize_successful_notification_by_id( notification_id, carrier, provider_response From 0381768e587a6498e0c078b89bf881350ee90bcc Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 13:18:59 -0800 Subject: [PATCH 18/20] add tests --- tests/app/dao/notification_dao/test_notification_dao.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 19db72192..1a145538a 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -2110,7 +2110,9 @@ def test_sanitize_successful_notification_by_id(): sanitize_successful_notification_by_id( notification_id, carrier, provider_response ) - mock_text.assert_called_once_with("x") + mock_text.assert_called_once_with( + "\n update notifications set provider_response=:response, carrier=:carrier,\n notification_status='delivered', sent_at=:sent_at, \"to\"='1', normalised_to='1'\n where id=:notification_id\n " # noqa + ) mock_session.execute.assert_called_once_with( mock_text.return_value, { From 91585a8b9e38eb8447fb1b2de38a2bbd3c2ae6cd Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 29 Jan 2025 07:50:23 -0800 Subject: [PATCH 19/20] cleanup --- app/celery/tasks.py | 2 +- app/delivery/send_to_providers.py | 6 +++++- app/notifications/validators.py | 12 ++---------- app/service/rest.py | 2 +- 4 files changed, 9 insertions(+), 13 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 8b3d9a353..92795c44a 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -179,7 +179,7 @@ def __total_sending_limits_for_job_exceeded(service, job, job_id): dao_update_job(job) current_app.logger.exception( "Job {} size {} error. Total sending limits {} exceeded".format( - job_id, job.notification_count, service.message_limit + job_id, job.notification_count, service.total_message_limit ), ) return True diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index e34847397..89d84b793 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -136,11 +136,15 @@ def send_sms_to_provider(notification): msg = f"Send to AWS!!! for job_id {n.job_id} row_number {n.job_row_number} message_id {message_id}" current_app.logger.info(hilite(msg)) notification.billable_units = template.fragment_count - current_app.logger.info("GOING TO UPDATE NOTI TO SENDING") update_notification_to_sending(notification, provider) cache_key = total_limit_cache_key(service.id) redis_store.incr(cache_key) + current_app.logger.info( + hilite( + f"message count for service {n.service_id} now {redis_store.get(cache_key)}" + ) + ) return message_id diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 38d8263c7..32cefcf2a 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -14,7 +14,7 @@ from app.models import ServicePermission from app.notifications.process_notifications import create_content_for_notification from app.serialised_models import SerialisedTemplate from app.service.utils import service_allowed_to_send_to -from app.utils import get_public_notify_type_text, hilite +from app.utils import get_public_notify_type_text from notifications_utils import SMS_CHAR_COUNT_LIMIT from notifications_utils.clients.redis import total_limit_cache_key from notifications_utils.recipients import ( @@ -46,21 +46,13 @@ def check_service_over_total_message_limit(key_type, service): service_stats = 0 redis_store.set(cache_key, service_stats, ex=seconds_difference) return service_stats - # TODO CHANGE THIS BACK TO SERVICE TOTAL MESSAGE LIMIT - if int(service_stats) >= 5: - # if int(service_stats) >= service.total_message_limit: + if int(service_stats) >= service.total_message_limit: current_app.logger.warning( "service {} has been rate limited for total use sent {} limit {}".format( service.id, int(service_stats), service.total_message_limit ) ) raise TotalRequestsError(service.total_message_limit) - else: - print( - hilite( - f"TOTAL MESSAGE LIMIT {service.total_message_limit} CURRENT {service_stats}" - ) - ) return int(service_stats) diff --git a/app/service/rest.py b/app/service/rest.py index 083ea23c4..84179e150 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -1140,7 +1140,7 @@ def get_service_message_ratio(): return { "messages_sent": messages_sent, "total_message_limit": my_service.total_message_limit, - } + }, 200 @service_blueprint.route("/monthly-data-by-service") From 88e623cb1090d6899f81818f2170e543289eceec Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 29 Jan 2025 08:04:33 -0800 Subject: [PATCH 20/20] cleanup --- app/delivery/send_to_providers.py | 6 +----- app/notifications/validators.py | 1 + 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 89d84b793..515d418e7 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -140,11 +140,7 @@ def send_sms_to_provider(notification): cache_key = total_limit_cache_key(service.id) redis_store.incr(cache_key) - current_app.logger.info( - hilite( - f"message count for service {n.service_id} now {redis_store.get(cache_key)}" - ) - ) + return message_id diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 32cefcf2a..8358b3c8a 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -53,6 +53,7 @@ def check_service_over_total_message_limit(key_type, service): ) ) raise TotalRequestsError(service.total_message_limit) + return int(service_stats)